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

Notes on metadata policy operators #11

Open
OIDF-automation opened this issue May 27, 2024 · 5 comments
Open

Notes on metadata policy operators #11

OIDF-automation opened this issue May 27, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request major

Comments

@OIDF-automation
Copy link

Imported from AB/Connect bitbucket: https://bitbucket.org/openid/connect/issues/2156

Original Reporter: gzachmann

Hi,

while going through the metadata policies operators of the newest spec version and found some things to comment on. I wanted to hear your opinion on them, if those are valid points or might already have been discussed before.

  • value: Why can it only be combined with essential? I think it should be possible to combine with all value checks, e.g. one IA might set a one_of policy value check and another IA/TA sets a value, this could still work perfectly fine.
  • add: Combination with superset_of: I don't think we should have the requirement that the values from add MUST be a superset of superset_of. Only after add is done the result MUST be a superset of the values in superset_of.  I'd argue we could just strip the stated requirement, since the consistency with value checks is checked anyway later.
  • default: Merging: Personally, I would like to have the possibility to merge default in the sense that superiors overwrite subordinate policies. This would enable a national federation to set another default than an intra-national-fed. Since default is rather weak (it's just a default, if the value is something else it's still fine) - I don't feel like a "conflict" between different IAs, is something critical/incompatible)
  • superset_of: Combination with add: See above
  • essential: Merging: The spec states that "If a Superior has specified essential=true, then a Subordinate MUST NOT change that." It's not completely clear to me what the meaning of 'MUST NOT change' is:

    a) essential is true as soon as any entity in the chain says so, i.e. subordinates cannot overwrite true with false - if they try it does not matter, the chain is still valid
    b) if a subordinate defines essential=false and a superior defined essential=true this MUST result in a policy error.

@OIDF-automation
Copy link
Author

Imported from AB/Connect bitbucket - Original Commenter: mbj

Thanks for your thoughts, Gabriel. A few reactions to your suggestions…

  • I would be OK with a rule that says that value can be merged with one_of provided that the value value is in the one_of set of values. (Although since it would result in additional code, I’d want to be convinced that this is a worthwhile addition.)
  • I can see the argument that add can be combined with superset_of provided that the result satisfies the superset_of.
  • I disagree with default being used to override another default. This would violate the “Equal Opportunity“ principal at https://openid.bitbucket.io/connect/openid-federation-1_0.html#name-principles.
  • For essential, I tend to favor your interpretation (a) - that it’s not an error if both true and false values are in the chain, but that if anywhere in the chain, the value true occurs then the result of the merge is true. Although my read of the spec currently is that it specifies (b).

Thanks again!

@OIDF-automation
Copy link
Author

Imported from AB/Connect bitbucket - Original Commenter: gzachmann

On value:

  • one_of was just an example, I would say value can be combined with all value checks, i.e. one_of, subset_of, superset_of, if the value from value is valid for those parameters. I don’t think that this would (generally) result in additional code, since the value check must be done anyways.
    The exception might be subset_of: here we must make sure that only the value check part of this operator is done, not the value modifier part, i.e. implementations must explicitly check that the value from value is a subset of subset_of before/instead of applying the subset_of operator. For one_of and superset_of I would argue that no additional consistency check is required, since applying the operator already does the check (and does not modify the value).
    So I would say combining value with one_of and superset_of should not be a problem, and not require additional code; I can see that it would be reasonable to not allow combination with subset_of, because of the additionally needed code, however, it also feels a bit inconsistent to me.
  • I also currently do not see reasons against combining it with default, also no code is needed for that, value will be applied first, then default does nothing.

I can see your point on default overwriting default.

For essential, I originally assumed interpretation (a), but it was not clear to me what the spec currently says, I don’t care too much what it will be in the end, it just should be more clear.

@vdzhuvinov
Copy link
Collaborator

@zachmann Hi Gabriel,

I studied PR #111 - "Policy operators: no restrictions when combining add and superset_of".

Every operator definition follows an identical template. This template includes (among other things):

  • a spec of the allowed combinations for the operator
  • a spec of the order in which the operator is applied (relative to other operators)

It is correct that if a software doesn't implement the combination checks for add + superset_of the order of applying superset_of after add will enforce the rule nevertheless. The combination spec, as description or result, is still valid however:

add: MAY be combined with subset_of, in which case the values of add MUST be a subset of the values of subset_of.

subset_of: MAY be combined with add, in which case the values of add MUST be a subset of the values of subset_of.

The current combination specs were worded like this to make it easier for policy designers to reason about operators and how they may be combined without having to also consider the order of the operator application. If we remove the conditionals ("... in which case...") this feature will be lost to the reader.

I see two ways going forward:

  • Keep the combo specs like there are and leave it up to implementers to figure out and decide not to implement the combo check (and rely only on the application order). E.g. for efficiency or because it's easier to code.

  • Edit all combination specs to delineate clearly between the check that must absolutely be implemented and what is just a "description" of the allowed combination.

I'm not in favour of completely removing the normative description, because this will make it more difficult for the reader to figure out the allowed combinations.

@zachmann
Copy link
Collaborator

I understand the reasoning. I would also say in general it is fine as it currently is.
In the case of combining add and superset_of together I found it too restrictive, as only the result after applying add but not already the add value(s) have to be a superset of superset_of.

When submitting the PR I considered wording it something like that, but found it too complex, if it also just could be left out.

But for the sake of consistency it makes sense to add a condition, I'll update the PR.

@Razumain
Copy link
Collaborator

Razumain commented Nov 7, 2024

I'm kind of sympathetic to what is raised here.

I think the standard could be simplified and processing be made more secure by simply stating that the final metadata MUST be consistent with all policy operators, or policy processing MUST fail.

vdzhuvinov added a commit to vdzhuvinov/federation that referenced this issue Jan 15, 2025
vdzhuvinov added a commit to vdzhuvinov/federation that referenced this issue Jan 15, 2025
vdzhuvinov added a commit to vdzhuvinov/federation that referenced this issue Jan 17, 2025
@peppelinux peppelinux moved this to Todo in Federation Jan 22, 2025
@peppelinux peppelinux moved this from Todo to In Progress in Federation Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major
Projects
Status: In Progress
Development

No branches or pull requests

4 participants