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

entity slice validation #76

Merged
merged 5 commits into from
Oct 3, 2024
Merged

entity slice validation #76

merged 5 commits into from
Oct 3, 2024

Conversation

mwhicks1
Copy link
Contributor

@mwhicks1 mwhicks1 commented Aug 1, 2024

Validating policies for entity slices using levels

Rendered

Signed-off-by: Mike Hicks <mwhicks@amazon.com>
@oflatt
Copy link

oflatt commented Aug 1, 2024

Nice idea, and much simpler than entity manifests.
We should add a note about how this is related to #74. Given a type-annotated entity manifest, calculating the level is easy

Copy link
Contributor

@shaobo-he-aws shaobo-he-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool.

Signed-off-by: Mike Hicks <mwhicks@amazon.com>
@mwhicks1
Copy link
Contributor Author

mwhicks1 commented Aug 2, 2024

Nice idea, and much simpler than entity manifests. We should add a note about how this is related to #74. Given a type-annotated entity manifest, calculating the level is easy

Thanks! I've updated the RFC to compare the two, landing on the position that we should ultimately have both.

@khieta khieta added the pending This RFC is pending; for definitions see the README label Aug 5, 2024
@D-McAdams
Copy link
Contributor

Thanks for adding the cross-link to Entity Manifests in #74.
Agree that they are best as a pair. Entity Manifests give instructions on what to load, and Entity Slice Validation (this RFC) ensures the instructions are practical by capping the complexity of incoming policies.

Also agree the alternatives in this RFC are less necessary when both techniques are used together.

Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few small comments, but I like this proposal in general. I think it makes sense as a first step before RFC 74.

text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
Comment on lines +276 to +278
### Alternative: Per-entity levels, rather than a global level

We might refine in-schema `level` to not apply to all entities, but rather to particular entity types. For example, for `User` entities (bound to `principal`) we might specify level 2 but for any other entity type we specify `level` as 0, as per the following schema:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of this alternative. I think it will make the validator behavior more difficult for users to understand.

text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
text/0076-entity-slice-validation.md Show resolved Hide resolved
text/0076-entity-slice-validation.md Show resolved Hide resolved
text/0076-entity-slice-validation.md Outdated Show resolved Hide resolved
Signed-off-by: Mike Hicks <mwhicks@amazon.com>
Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com>
Copy link

@patjakdev patjakdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally find RFC 74 to be a more appealing approach to slicing. I think that were RFC 74 to be accepted, this one wouldn't have much utility given the cost to implement and (IMO) unlikeliness for it to apply well generally.

If you've got some real-world use cases where validation-time depth limitations would have solved performance problems for users in a way that RFC 74 wouldn't or couldn't, I'd definitely be open to changing my mind.

The one that springs to mind is maliciously authored policies, but I wonder if that wouldn't be something better guarded against in the Cedar library implementations rather than as part of the schema.


All of the Cedar example policy sets validate with level-based validation, either at level 1 or 2; see the Appendix for details.

## Motivation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having read through the motivation section, I'm not entirely convinced of the overall utility of this proposal, especially in a world where RFC 74 is implemented.

The proposal seems to consider a narrow dimension of policy evaluation performance (entity graph depth) in such a coarse-grained way that it feels unlikely to be a solution to anyone's specific policy performance problems. I'd like to see some justification for this proposal working in the 80% case. We can easily imagine lumpy or extremely wide entity graphs wherein a single level number provides almost no utility to the slicer and a great frustration to the policy author who needs to go just one level deeper than is allowed.

Something like RFC 74 seems to give a lot more flexibility to both policy authors and slicers since slicers don't have to be so coarse-grained, although they might choose to be. You could imagine implementing the depth traversal limit proposed here in the entity loader interface proposed in that RFC if the entity type level proposed here were passed along, although that would end up being an evaluation error rather than a validation error, which perhaps would be undesirable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm curious if there are real-world examples which motivated this proposal. If so, I think it would be useful to include them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that #74 is likely to be more suitable for most consumers of Cedar. This RFC gives a less granular tool for describing the entity data that needs to be included in the slice. Sometimes, being more granular is better.

The usecase I find most compelling for this RFC is one where one party writes the entity slicing algorithm and another writes the policies (e.g., you offer a service where end-users can author Cedar policies). With #74, the entity slicing potentially needs to be changed whenever policies change (because we were passing in only the entities needed). With this RFC, we pass in some entity data that may be unused, but this means policies can change freely as long as they continue to only use the entity data at level n.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"End-users writing policies" is (in vague terms) the real-world example that motivated this.


### Alternative: Level as a validation parameter, not in the schema

This RFC has suggested that the level should be specified in the schema. Alternatively, we could specify the level as a parameter to the validator itself, leaving schemas unchanged. The benefit of doing so is that entity slicing becomes an orthogonal concern: The schema specifies the expected type structure of data provided with a request, but not how much of that data is required. The drawback is that policy writers cannot look in one place to know the limits on the policies they can write.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawback is that policy writers cannot look in one place to know the limits on the policies they can write.

I don't think this is so awful. The error message from the validator will tell the author what the limit is, if they hit it. This also allows for more flexibility in granularity of level validation in the future without having to change the Cedar language.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After implementing this (cedar-policy/cedar#1146) I am inclined towards making this a parameter.

Arguments for:

  • In the non-JSON schema format, level can occur at the top of any namespace. This makes finding level annoying. We error if level occurs more than once, but we could just take it as a single parameter to validate.
  • The JSON schema format has the same issue, but JSON isn't ordered, so it feels more native.
  • Schema format remains unchanged and we can add level to the schema later if desired. In particular, adding the per-entity level as described in alternatives would require new syntax (something like @level(1), as proposed in the alternative). Delaying this decision will let us hear if more people would prefer that and a global level or something similar.

Arguments against:

  • Keep data in one place.
  • Easier to change schema than to change code (but the entity slicing code would still need to change).

Signed-off-by: Mike Hicks <mwhicks@amazon.com>
@andrewmwells-amazon andrewmwells-amazon added final-comment-period This RFC is in its final comment period; for definitions see the README and removed pending This RFC is pending; for definitions see the README labels Aug 21, 2024
@aaronjeline aaronjeline merged commit 5e926c6 into main Oct 3, 2024
1 check failed
@aaronjeline aaronjeline deleted the entity-slice-validation branch October 3, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period This RFC is in its final comment period; for definitions see the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.