RPC policy rule syntax: the difference between target= and default_target= can be confusing

[andrewdavidwong]

How to file a helpful issue

Qubes OS release

4.1.1

Description

This issue is branched from #7720. It concerns the parts in bold below.

@marmarek wrote in #7720 (comment):

Those rules, with the current policy semantic, does not allow the call. The default_target selects default target from targets allowed (ask or allow) by other rules already. This is different from target, which overrides target regardless of whether it is otherwise allowed or not. See https://github.com/QubesOS/qubes-core-qrexec/blob/master/doc/qubes-policy.rst (hmm, it seems we don't have this rendered anywhere else).
And indeed no other rule allows calls to @dispvm:dvm-default (@dispvm means @dispvm:dvm-other when it has dvm-other set as default_dispvm).

So, we have few points here:

1. Error handling - it should be clear message what really happened, not AssertionError
2. Confusing difference between target= and default_target= - the later requires another rule that would allow/ask the call, while the former does not
3. Setting just ask target= does not select that target as default, even though it makes it the only allowed choice. You'd need to specify both target= and default_target= for that.

The first point is an obvious bug. The second is intended semantics, but I can be persuaded to change it. The third one was intentional choice, but when looking at it now, I think it was a wrong one and should be changed.

Therefore, this issue is for deciding how to handle the semantics of target= and default_target= with the aim of reducing potential user confusion regarding the difference between them.

Related issues

The other points mentioned in the quoted text above are handled in other issues:

• First point = qubes.OpenInVM * @anyvm @dispvm ask default_target=@dispvm:[...] causes AssertionError #7720
• Second point = [this issue]
• Third point = Pre-select qvm-copy/move destination VM in dom0 confirmation window when it's the only possible option #5510

[marmarek]

Continuing from #8227 (comment) (by @ben-grande):

understand it is intentional. It is okay to override the destination on the same line by using target=, but is it okay to ignore previous deny rules to that destination?
My PR aboves consider previous deny rules and if encounters none, uses the redirect target=.

No, it additionally requires other "allow" rule to be present.

Generally, the current behavior of target= rule is meant o limit leaking target name (or possibility to enumerate qubes) to the source qube.
Consider the following example:

some.service * source @default target=destination

With current implementation, only the call with @default target will get redirected to destination. Specifying any call target explicitly (being actual destination or something else) will get denied. With your change, all calls (also with @default target) will get denied, as collect_targets_for_ask will return an empty list.
But then, if you add an allow rule:

some.service * source destination allow
some.service * source @default allow target=destination

the source qube can make a call to destination explicitly and confirm its existence this way.

Note also, in this (and few other) examples, the order of allow/deny rules rarely matter if you override the target, as the original target will usually be different than the overridden one. For example, with your change:

some.service * source dest-1 deny
some.service * source dest-2 allow target=dest-1

is the same as

some.service * source dest-2 allow target=dest-1
some.service * source dest-1 deny

because dest-2 != dest-1, so collect_targets_for_ask will consider both rules.

All that said, even with your change, it's still possible to prevent source qube learning real target name, by using different rule(s):

some.service * source @anyvm allow target=destination

This, instead of refusing other calls (requested to actual destination or elsewhere), will allow them but redirect. This may be a bit confusing when configuring your system (you think you make a call to target1, but actually it gets redirected to target2), but OTOH arguably the current behavior of target= is even more confusing.

[ben-grande]

On 23-11-27 11:01:13, Marek Marczykowski-Górecki wrote: > understand it is intentional. It is okay to override the destination on the same line by using target=, but is it okay to ignore previous deny rules to that destination? > My PR aboves consider previous deny rules and if encounters none, uses the redirect `target=`. No, it additionally requires other "allow" rule to be present. Generally, the current behavior of `target=` rule is meant o limit leaking taret name (or possibility to enumerate qubes) to the source qube. Consider the following example: ``` some.service * source @default target=destination ``` With current implementation, only the call with ***@***.***` target will get redirected to destination. Specifying any call target explicitly (being actual `destination` or something else) will get denied. With your change, all calls (also with ***@***.***` target) will get denied, as `collect_targets_for_ask` will return an empty list.

It will return an empty list `only` if `deny` rule is placed before the `ask` or `allow` rule. That is the behavior I am proposing. When `dev` calls: ``` qrexec-client-vm -- @default qubes.GitInit+dir ``` #### Current behaviour The call will be allowed (with interaction): ``` qubes.GitInit * dev @default ask default_target=sys-git target=sys-git qubes.GitInit * dev @AnyVM deny ``` which is the intended behavior. The call will be allowed (with interaction): ``` qubes.GitInit * dev @AnyVM deny qubes.GitInit * dev @default ask default_target=sys-git target=sys-git ``` and this is the weird case we are discussing. #### My patch The call will be allowed (with interaction): ``` qubes.GitInit * dev @default ask default_target=sys-git target=sys-git qubes.GitInit * dev @AnyVM deny ``` because the ask comes before the deny. The call will be rejected: ``` qubes.GitInit * dev @AnyVM deny qubes.GitInit * dev @default ask default_target=sys-git target=sys-git ``` because sys-git matches in previous rule ***@***.***` that was denied to that service+arg.

------

But then, if you add an allow rule: ``` some.service * source destination allow some.service * source @default allow target=destination ``` the source qube can make a call to `destination` explicitly and confirm its existence this way. Note also, in this (and few other) examples, the order of allow/deny rules rarely matter if you override the target, as the original target will usually be different than the overridden one. For example, with your change: ``` some.service * source dest-1 deny some.service * source dest-2 allow target=dest-1 ``` is the same as ``` some.service * source dest-2 allow target=dest-1 some.service * source dest-1 deny ``` because `dest-2` != `dest-1`, so `collect_targets_for_ask` will consider both rules.

And this behavior I don't want, the deny rule precedence not being enforced when using redirects. User sets in 30-user.policy: ``` some.service * source dest-1 deny ``` AdminVM, 3rd-party packagers and QubesOS sets in 80-package.policy: ``` some.service * source @default allow target=dest-1 ``` And I do not like that later rules override previous rules. This behavior does not allow users to fully control their deny rules, as they can be overriden at a later time. Isn't this the exception to the rule? Overriding user configuration? I believe that following this norm and allowing redirects only if there are no previous deny rule to that qube is more easy to grasp and does not break current qubes policies at 90-*.policy. Side note that the PR is not finished for some edge cases, but all that can be worked on if you agree that the current behavior should change and is against the norm that previous rules can enforce later rules.

All that said, even with your change, it's still possible to prevent source qube learning real target name, by using different rule(s): ``` some.service * source @AnyVM allow target=destination ```

I do not use direct qube names, always tokens.

This, instead of refusing other calls (requested to actual `destination` or elsewhere), will allow them but redirect. This may be a bit confusing when configuring your system (you think you make a call to target1, but actually it gets redirected to target2), but OTOH arguably the current behavior of `target=` is even more confusing.

I don't find the behavior of `target=` confusing and I don't think the issues were duplicate. I understand the currenct behavior is to always override the destination distegarding of its value and redirect to the target you want. I am however trying to limit the `target=` to a value that has no preceding `deny` rule, making this call be denied: ``` some.service * source destination deny some.service * source @default allow target=destination ``` But not this call when the destination qube value is the token ***@***.***`. ``` some.service * source @default allow target=destination some.service * source destination deny ```

…

-- Benjamin Grande

[marmarek]

Email parsing mangled formatting and masked out some keywords...

[marmarek]

The call will be allowed (with interaction):

qubes.GitInit * dev @AnyVM deny
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git

No, it won't. @anyvm matches also @default, so it will never reach the ask action.

But I think you meant this example instead:

qubes.GitInit * dev sys-git deny
qubes.GitInit * dev @default ask default_target=sys-git target=sys-git

With your patch indeed this will be denied (which wasn't the case before your patch).

Note to self: the patch indeed considers only earlier rules, not later ones (even though target column may not match) only because collect_targets_for_ask prefers target= field if present over the target column.

I don't find the behavior of target= confusing and I don't think the issues were duplicate.

The change you propose fixes part of this issue, the difference in allowed targets between

qubes.GitInit * dev sys-git deny
# with or without default_target=
qubes.GitInit * dev @default ask target=sys-git

and

qubes.GitInit * dev sys-git deny
qubes.GitInit * dev @default ask default_target=sys-git

Before your change, the first will have one possible target, while the latter will have possible targets list empty (effectively denying the call). After your change, both will have empty targets list.

But indeed, the difference remains without the earlier deny rule. So it isn't fully the same issue, but I'd call it still close enough, especially since that's the difference that was considered to be changed.

[marmarek]

So, generally I agree with the proposed change. But we need some safe approach to deploy it, because it is a semantic change of existing rules. The qrexec-legacy-convert tool uses qrexec-policy-graph before and after to verify if semantic remained the same (BTW, thanks for helping with improving this tool!), but that was about changing rule files, not changing meaning of rules without changing rules themselves.

The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.

[DemiMarie]

The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.

With the default rules this change is a no-op, so it is definitely safe to deploy in that case. What about:

• Making the change conditional on some setting.
• Enabling the setting by default if it would be a no-op.
• Allowing users to opt-in manually if this was not done automatically.

[ben-grande]

So, generally I agree with the proposed change. But we need some safe approach to deploy it, because it is a semantic change of existing rules. The qrexec-legacy-convert tool uses qrexec-policy-graph before and after to verify if semantic remained the same (BTW, thanks for helping with improving this tool!),

Thanks for reviewing!

but that was about changing rule files, not changing meaning of rules without changing rules themselves.

As far as I can see, this migration would be difficult because it needs to change the rules order. The destination can have tokens such as tags which the policy doesn't know which qube has that. Placing all deny rules at the bottom of the file would facilitate not having to find the correct line number to but the rule below, but this would mess even further with user setups by leaving ask and allow at the top and deny at the bottom, but without semantic changes.

But.... unman and andrew and me were expecting for this to be already the behavior and Qubes default policies also put deny rules after ask and allow that have redirects.

The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.

I understand it is not safe to assume how the user has configured their own system... and maybe I am overcomplicating how difficult it is to do this migration. I don't think it is too late for R4.2, maybe for the first release yes, but for a point release it might be okay depending on how much you consider this a breaking change.

The change should only result in some actions to be denied that were previously allowed, so it is a "safer" direction, but still it may break user setups in unintended ways. This also means it is too late to deploy it into R4.2, I think.

With the default rules this change is a no-op, so it is definitely safe to deploy in that case. What about:

• Making the change conditional on some setting.
• Enabling the setting by default if it would be a no-op.
• Allowing users to opt-in manually if this was not done automatically.

If it is worth it, !compat-4.1 can be used, which can be set on 35-compat.policy or another file if that one can not be replaced.