Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New qrexec policy format #4370

Open
4 of 5 tasks
marmarek opened this issue Oct 4, 2018 · 14 comments
Open
4 of 5 tasks

New qrexec policy format #4370

marmarek opened this issue Oct 4, 2018 · 14 comments
Labels
C: core P: major Priority: major. Between "default" and "critical" in severity. release notes This issue should be mentioned in the release notes. T: enhancement Type: enhancement. A new feature that does not yet exist or improvement of existing functionality.

Comments

@marmarek
Copy link
Member

marmarek commented Oct 4, 2018

New policy format, draft specification: https://github.com/woju/qubes-core-qrexec/blob/master/Documentation/multifile-policy.markdown

The major change in using one policy, divided into separate files (.d approach), with service name and its argument as first two fields in each entry. This allows more flexible policy, easier backup&restore and generally manage it from scripts. Including specific policy API to manage policy itself.

Tasks here:

  • write a specification
  • write new policy parser (based on the old one)
  • make sure that all tests from previous parser passes
  • ensure clean migration (according to the current spec, one rule should handle it all)
  • use it to restore policy from a backup (backup & restore qrexec policy #3550)
@marmarek marmarek added T: enhancement Type: enhancement. A new feature that does not yet exist or improvement of existing functionality. C: core P: major Priority: major. Between "default" and "critical" in severity. labels Oct 4, 2018
@marmarek marmarek added this to the Release 4.1 milestone Oct 4, 2018
@marmarek marmarek added the release notes This issue should be mentioned in the release notes. label Oct 4, 2018
@ghost
Copy link

ghost commented Oct 4, 2018

A few questions/remarks:

  • the parsing logic of the alphabetical files is "apply first match and quit", right ?
  • it's usually not a good idea to mix both system and user files in the same location, it is inevitable that some users will mistakenly modify the{40,50,60}-* files; what will then happen to those files after an rpm update ?
    • .rpmnew suffix ? (that defeats the purpose of easy updates when a policy default must be updated for instance because of a security hole.)
    • .rpmsave suffix ? (not good since users may think their policy is still in place.)
    • or are the files silently overwritten ? (worse).
  • Policy files in /etc/qubes-rpc-policy are currently installed by several rpms, eg. qubes.USB is from qubes-usb-proxy-dom0, while most of the other policy files are from qubes-core-dom0. Does that mean that the (seemingly) single 40-default policy file will include the policy of all installable packages, even those not installed ? If yes, what about conflicting packages/policies ? (there could be for instance two or more packages providing the same functionality but with different policies).
  • won't it become a mess if there are many {70,75,80}-* vendor provided files ?
  • more generally, what is the rationale behind not keeping single files named after the policy ? There could be exceptions to the rule if needed, but having filenames identical to the policy's name will allow to use something like systemd's definitions which are well thought and flexible: system policy files in /usr/lib/... are clearly separated from the user ones that are in /etc/... ; the user files could augment the default policy in /usr/lib/... with [before] and [after] sections, which would respectively add rules before or after the default policy, or completely replace the default policy with a [default] section.

@marmarek
Copy link
Member Author

marmarek commented Oct 4, 2018

* the parsing logic of the alphabetical files is "apply first match and quit", right ?

Yes.

* it's usually not a good idea to mix both system and user files in the same location, it is inevitable that some users will mistakenly modify the`{40,50,60}-*` files; what will then happen to those files after an rpm update ?

We may consider separate directory like /usr/lib/qubes/policy for system policy files (with similar semantic to many XDG and systemd standards - file in higher-priority directory override a same-named file in lower-priority dir, then all files are sorted and evaluated). But given the planned GUI domain, where user have no direct access to dom0 and policy can be modified only through specific API, this shouldn't be a problem in practice, as such conflicting modifications would not be easily possible. Those files will also have a comment that they will overwritten on update, for those accessing dom0 directly anyway (this assume you already know what you're doing).

  • Policy files in /etc/qubes-rpc-policy are currently installed by several rpms, eg. qubes.USB is from qubes-usb-proxy-dom0, while most of the other policy files are from qubes-core-dom0. Does that mean that the (seemingly) single 40-default policy file will include the policy of all installable packages, even those not installed ? If yes, what about conflicting packages/policies ? (there could be for instance two or more packages providing the same functionality but with different policies).

No, general rule is that default policy is installed with a component providing given service. So, for example default policy for qubes.USB will be in in {70,75,80}-qubes-usb-proxy.

* won't it become a mess if there are many `{70,75,80}-*` vendor provided files ?

Shouldn't be a problem, as those files in most cases will be about different services, so order for example between any two 70-* doesn't matter. This approach works pretty well for multiple software packages (udev, fontconfig, sysctl etc).

* more generally, what is the rationale behind not keeping single files named after the policy ? There could be exceptions to the rule if needed, but having filenames identical to the policy's name will allow to use something like systemd's definitions which are well thought and flexible: system policy files in `/usr/lib/...` are clearly separated from the user ones that are in  `/etc/...` ; the user files could augment the default policy in /usr/lib/... with `[before]` and `[after]` sections, which would respectively add rules before or after the default policy, or completely replace the default policy with a `[default]` section.

This would be a nightmare to analyze (both by human and a script). And also to maintain. Note that there may be more than two entities ("system defaults" and "user") willing to edit policy. For example we already have Salt formulas to modify policy for Whonix needs. There may be another system like this. Adding corporate Management VMs to the picture doesn't make it easier too. With new policy format, it's enough to drop appropriately named file, no need to modify config files touched by the user or owned by another package.

Theoretically it could be possible with service-name.d directories too. But that case is still messy around services with arguments ("allow calls only with specific argument", "allow calls only with empty argument" etc). The new format solve this problem. And additionally allow to easily make a VM isolated from all calls (either incoming or outgoing), regardless how many extra policy files will be installed in the future (as long as are properly named).

@ghost
Copy link

ghost commented Oct 4, 2018

Thanks for the detailed answers, it makes much more sense now (especially how there's actually no concerns about people editing policy files because of the gui domain).

@marmarek
Copy link
Member Author

The parser is almost done (QubesOS/qubes-core-qrexec#2).
Updated specification: https://github.com/QubesOS/qubes-core-qrexec/pull/2/files#diff-2a993db74ba3b579503e1fd33fbf1aaf

Today we had discussion with @woju whether this should be included in R4.1, or R5.0. And whether the next Qubes version should be R4.1 or R5.0.

@woju is on position it is such a major change that warrants bumping major number (R5.0), while I think it is not (R4.1). While the new format is not directly compatible with the old one (meaning there will be a change where and how custom policy files should be placed), we will provide conversion tool, preserving as much of user modifications as possible.

Also, I don't think other changes for the next release (ingredients for GUI qube(?), without actually enabling it by default) are major enough to warrant naming it R5.0.

@andrewdavidwong do you have any opinion here?

@andrewdavidwong
Copy link
Member

The parser is almost done (QubesOS/qubes-core-qrexec#2).
Updated specification: https://github.com/QubesOS/qubes-core-qrexec/pull/2/files#diff-2a993db74ba3b579503e1fd33fbf1aaf

Today we had discussion with @woju whether this should be included in R4.1, or R5.0. And whether the next Qubes version should be R4.1 or R5.0.

@woju is on position it is such a major change that warrants bumping major number (R5.0), while I think it is not (R4.1). While the new format is not directly compatible with the old one (meaning there will be a change where and how custom policy files should be placed), we will provide conversion tool, preserving as much of user modifications as possible.

Also, I don't think other changes for the next release (ingredients for GUI qube(?), without actually enabling it by default) are major enough to warrant naming it R5.0.

@andrewdavidwong do you have any opinion here?

I don't have a strong opinion about this, but one thing to consider is how many users actually "experience" qrexec. How user-facing is it? My guess is that most users don't really touch it and don't edit RPC policies (except via "yes to all" prompts). If so, then most users might be underwhelmed by the perceived lack of major changes in "5.0." Even those users who do write custom RPC policies might wonder why such a seemingly minor change merits a major version increase.

@woju
Copy link
Member

woju commented Feb 18, 2019

How user-facing is it?

Unfortunately it depends on particular user. I'd say this is technically incompatible change, so it should not be 4.1.

@marmarek said this release in question is a preparation for 5.0, because apart from new qrexec policy there is also new, unconfigured gui protocol (am I correct?). If so, can we instead name this release 4.9 or 4.5?

@andrewdavidwong
Copy link
Member

Unfortunately it depends on particular user. I'd say this is technically incompatible change, so it should not be 4.1.

@marmarek said this release in question is a preparation for 5.0, because apart from new qrexec policy there is also new, unconfigured gui protocol (am I correct?). If so, can we instead name this release 4.9 or 4.5?

In that case, perhaps you should save this change for 5.0, and the next release should be 4.1 (without this backward-compatible change). It doesn't sound like 4.1 will suffer from not having this change.

I think that naming it 4.5 or 4.9 would be confusing. People would wonder what happened to 4.1, 4.2, 4.3, 4.4, etc.

@marmarek
Copy link
Member Author

marmarek commented Mar 5, 2019

We've discussed this some more, and I think we've found a solution that do not break compatibility and even make the migration simpler.
The idea is to add to the new policy format a special rule meaning "include all the old policy from /etc/qubes-rpc/policy here". New policy in R4.1 will include such rule by default, which means the old policy (including all the changes made by the user) will still work. It will be placed close to the top, so it will take precedence over new format. Having old policy loaded by such rule will result in some kind of warning (we'll make it not too intrusive). In addition, take advantage of rpm which allows to detect which policy files were modified by the user. Then, drop support for this rule in R5.0, forcing migration to the new format.

The migration to new format will work like this:

  1. In R4.1 user doesn't need to change anything, the old format and location still works. But if they want, they can use the new format too.
  2. RPM keeps track of all the package-shipped files, including possible user modifications in configuration files (rpm have MD5 of each config file, to detect user changes). If there were no user changes, old policy files will be removed on upgrade, cleanly migrating to the new format. If any of the policy file was modified, such file will be kept intact.
  3. During R4.1 life cycle, user can gradually migrate custom rules to the new format. At this point /etc/qubes-rpc/policy will contain only policy files manually modified by the user, so in most cases it will be just few files, if any at all. When given policy file is migrated, it's enough to remove the old one. We'll also provide a tool to translate rules.
  4. In the process of upgrading to R5.0, we'll force translating remaining rules to the new format. I'm not yet sure if this step will be totally automated, or will force some kind of review/confirmation from the user. In some cases it may be also reasonable to drop custom rules and start with fresh defaults, the migration tool may suggest such solution too, based on existing rules. But I think the final decision here should be taken by the user.

@marmarek
Copy link
Member Author

marmarek commented Mar 5, 2019

@woju other notes from the discussion:

  • new format should require .policy file extension

  • old files should be sorted by filename, with files specifying an argument before files without it

  • after loading each file there needs to be a pair of deny rules (which were implicit in the old format):

    @anyvm @anyvm deny
    @anyvm @adminvm deny
    

@jpouellet
Copy link
Contributor

In the process of upgrading to R5.0, we'll force translating remaining rules to the new format. I'm not yet sure if this step will be totally automated, or will force some kind of review/confirmation from the user. In some cases it may be also reasonable to drop custom rules and start with fresh defaults, the migration tool may suggest such solution too, based on existing rules. But I think the final decision here should be taken by the user.

It should be possible to have a static analysis tool evaluate equivalence of policies. I believe such "aggregate effective policy diff" tool would be very useful for writing and reasoning about policies in general. Such a tool could be used to ensure (hopefully) safe migration to the new format. Writing such a tools is, of course, left as an exercise to the reader :P

@jpouellet
Copy link
Contributor

jpouellet commented Aug 11, 2019

One thing I really want is the ability to grant qrexec rights to a specific DispVM once (until it is shut down), and not have the possibility of those rights being assumed by a later different VM whose disp[0-9]{1,4} happens to collide. The best way to achieve this is not clear to me with the current architecture.

Essentially I want capability delegation bound to specific VM instances.

@marmarek
Copy link
Member Author

marmarek commented Aug 13, 2019

It should be possible to have a static analysis tool evaluate equivalence of policies.

There is qrexec-policy-graph tool that could be a help here.

One thing I really want is the ability to grant qrexec rights to a specific DispVM once (until it is shut down), and not have the possibility of those rights being assumed by a later different VM whose disp[0-9]{1,4} happens to collide. The best way to achieve this is not clear to me with the current architecture.

What do you think about adding another syntax for domain UUID? Those are hard enough to not conflict? Alternatively, there could be a mechanism that remove policy rules involving a VM name explicitly - when you remove that VM. That would be more fragile though.
On the other hand, there are similar issues not only about DispVM:

  • policy after VM rename (clone & remove) - would be nice to have an option to adjust policy here (including target= arguments)
  • unrelated re-creation of a VM with a name that got a special permission before - in most cases, should not inherit that permission; but probably not all the cases - for example restoring a VM from a backup probably should not invalidate policy (but this particular case could be solved by also included policy in the backup)

The second point would be solved by UUID. But not the first one - in the current design of rename operation you can't possibly preserve UUID, by the definition of the second "unique" there.

BTW it's already possible to have similar effect using the current syntax: add a tag uuid-.... and use that in the policy (@tag:uuid-...). It will even work for renames (tags are preserved). But tags will be also inherited during non-rename clone operation. Which may or may not be a good thing here. It will not solve target= and similar arguments.

marmarek added a commit to marmarek/qubes-core-admin that referenced this issue May 12, 2020
marmarek added a commit to marmarek/qubes-core-qrexec that referenced this issue May 15, 2020
marmarek added a commit to marmarek/qubes-core-admin that referenced this issue May 15, 2020
marmarek added a commit to marmarek/qubes-core-admin that referenced this issue May 15, 2020
Keep it at original files, to still load it using compat rules.
This way the update should not break user's policies.

Note the unchanged policy files are still going to be removed - meaning
those calls will use the new policy.

QubesOS/qubes-issues#4370
marmarek added a commit to marmarek/qubes-core-qrexec that referenced this issue May 15, 2020
marmarek added a commit to marmarek/qubes-mgmt-salt that referenced this issue May 17, 2020
Use a single /etc/qubes/policy.d/50-qubesctl-salt.policy policy file for
dynamic rules, instead of legacy per-service files in
/etc/qubes-rpc/policy. This way it's easier to manage the policies by
salt (add a comment it's an automatically generated file), while it's
still possible for the admin to enforce other rules (in other files).

QubesOS/qubes-issues#4370
marmarek added a commit to fepitre/qubes-mgmt-salt-dom0-virtual-machines that referenced this issue May 21, 2020
Make it conform to the convention set in qrexec documentation.

And also use new policy location for included admin api policy.

QubesOS/qubes-issues#4370
marmarek added a commit to marmarek/qubes-mgmt-salt-dom0-virtual-machines that referenced this issue May 21, 2020
Make it conform to the convention set in qrexec documentation.

QubesOS/qubes-issues#4370
marmarek added a commit to marmarek/qubes-core-admin that referenced this issue May 24, 2020
marmarek added a commit to marmarek/qubes-core-admin that referenced this issue May 24, 2020
Keep it at original files, to still load it using compat rules.
This way the update should not break user's policies.

Note the unchanged policy files are still going to be removed - meaning
those calls will use the new policy.

QubesOS/qubes-issues#4370
@pefu
Copy link

pefu commented Feb 16, 2022

I came here, because this issue is mentioned in 4.1.0 release notes.
Ist this the correct URL of the specification? The URL given in the first message of this issue from Oct 4th 2018 leads to the github 404 page.

@ben-grande
Copy link

#1939 (comment)

Is there any new design of a policy not yet discussed in public? Is there a issue tracking that? This one seems relevant to qrexec v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: core P: major Priority: major. Between "default" and "critical" in severity. release notes This issue should be mentioned in the release notes. T: enhancement Type: enhancement. A new feature that does not yet exist or improvement of existing functionality.
Projects
None yet
Development

No branches or pull requests

6 participants