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

Cedar standard library #58

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

cdisselkoen
Copy link
Contributor

Rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Craig Disselkoen <[email protected]>
@cdisselkoen cdisselkoen force-pushed the cdisselkoen/standard-library branch from 29de6e1 to 2929508 Compare March 12, 2024 20:20
@cdisselkoen cdisselkoen added the pending This RFC is pending; for definitions see the README label Mar 12, 2024
Copy link
Contributor

@aaronjeline aaronjeline left a comment

Choose a reason for hiding this comment

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

I really like this. I think having a stdlib is a good idea.
This RFC doesn't actually say what the type deceleration for OIDC::User is, is that intended?
I like the listing of alternatives, but think they can all wait for a future RFC when needed.

@shaobo-he-aws
Copy link
Contributor

A meta question: Is it for human-readable format only? If not, we should provide the JSON representation as well.

@D-McAdams
Copy link
Contributor

I support the "extends" feature and have several scenarios where it would help us manage schemas in large-scale systems with many services and application builders, in which central teams define a common structure which others can expand.

The Cedar standard library is a special case in which a common structure is so ubiquitous as to be nearly universal. I think this could be useful, but will naturally take longer to finalize these types and so it makes sense to decouple the standard type definitions (like OIDC User) from the rest of the proposal. We'd probably want to run the standard types through a process that looks like a standards community, including audiences from the OpenID Foundation, for example.

@D-McAdams
Copy link
Contributor

When it comes to bikeshedding the naming, if we sought to avoid "extends" because of it's object-oriented connotations, I would be in favor of the aforementioned suggestion of "expands".

Signed-off-by: Craig Disselkoen <[email protected]>
@cdisselkoen
Copy link
Contributor Author

@aaronjeline says

This RFC doesn't actually say what the type deceleration for OIDC::User is, is that intended?

@D-McAdams says

will naturally take longer to finalize these types and so it makes sense to decouple the standard type definitions (like OIDC User) from the rest of the proposal. We'd probably want to run the standard types through a process that looks like a standards community, including audiences from the OpenID Foundation, for example.

These are good thoughts. I would say that this RFC proposes three things:

  • the concept/mechanisms for a standard library
  • the extends keyword (or whatever we end up bikeshedding it as)
  • the idea of adding OIDC::User as a standard library type

but this RFC does not propose a specific declaration for OIDC::User, only proposes that we somehow arrive at a consensus one, and then add it to the standard library. I propose that we can delay hashing out the details of the OIDC::User declaration to an implementation PR, or to some other kind of process with more stakeholders as @D-McAdams suggests.

@jszmajda
Copy link

I'm a big fan of extends, that will make my life easier in my own schema definitions. One detail on the stdlib - I like the idea of a reference library, but I'm not a fan of prepending it to the schema because from what I understand this would allow any policy author to reference those entities regardless of whether they're being used in a particular application. If there's a way to "not import unless referenced or used somehow," that would solve my concern.

Copy link
Contributor

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

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

I would propose we table this RFC and instead consider a library mechanism for schemas, and then rely on that for different user communities to build a de-facto standard libraries.

or
```
entity Group { myAttribute: String };
entity User extends __cedar::OIDC::User in [Group] { emailOptOut: Boolean };
Copy link
Contributor

Choose a reason for hiding this comment

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

While we say below that we will not support subtyping (with strict validation) using this construct, that may disappoint users who expect to get it, since morally we are extending/reusing an existing base type.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a technical issue: Suppose I have

entity Group { myAttribute: String, sibling: [Group] };
entity HierarchicalGroup extends Group { tag: String };

A strict interpretation of extends would make HierarchicalGroup equivalent to

entity HierarchicalGroup { myAttribute: String, sibling: [Group], tag: String };

But users might be expecting that sibling's type is updated with the extended type, i.e.,

entity HierarchicalGroup { myAttribute: String, sibling: [HierarchicalGroup], tag: String };

Doing so would be important for strict validation, since it does not allow subtyping. With permissive validation, leaving sibling as [Group] is less problematic because HierarchicalGroups are effectively subtypes of Groups, and we could introduce subtyping edges explicitly to support that.

Copy link

Choose a reason for hiding this comment

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

Another option could be to do what IonSchema does: https://amazon-ion.github.io/ion-schema/docs/isl-2-0/spec#all_of.

They allow an "any_of" and an "all_of". Then you're encouraged to use composition without the overhead of inheritance.

Comment on lines +14 to +15
Create a Cedar Standard Library of type declarations that are likely to be
useful to many Cedar users, and are distributed along with Cedar.
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to me to call this a standard "library" because there is no general notion of library that this proposal instantiates with a standard. Indeed, I expect there is interest in supporting 3rd party libraries of type definitions (and data definitions, if we adopt RFC 61), and I'd propose we think through how that might work, and then leverage that for our particular proposed definitions, here.

Comment on lines +50 to +53
2. Ensuring the community's alignment on the "best" way to declare each type.
For instance, should OIDC `updatedAt` be expressed as a `String`, as a `Long`
(eg Unix timestamp), or as a record with multiple components?
This allows Cedar to encourage best practices.
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine some variation in how to define things, rather than a single source of truth. For example, some apps might want to make OIDC attributes out of enumerated entities, but others might prefer to make them strings. (This is a real disagreement with real customers!) It's a problem if the "standard" definition doesn't work for 95% of customers, I would claim.

Copy link
Contributor

Choose a reason for hiding this comment

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

In short: Being in the business of maintaining a standard library is an opportunity for regret, since we might discover that many things we introduce are wrong, but we can't easily remove them. Better to have a general mechanism the community can use, and then take clear winners into a standard location when/if they emerge.

Comment on lines +47 to +48
1. Saving each Cedar user the effort of writing the declaration themselves.
This facilitates code reuse in schemas, and makes it easier to get started
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a benefit of libraries in general, not necessarily a standard library. If we provided a library facility, then we could let third parties vend these definitions and customers adopt the ones they want.

Comment on lines +55 to +57
When everyone shares a common definition of `OIDC::User`, the community could
conceivably converge on a reusable library function for, e.g., converting an
OIDC token into Cedar entity data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, the community could converge on such a thing -- so we should give them a mechanism to do that, properly. Rather than pull requests to the Cedar core repository.

Comment on lines +76 to +78
This RFC also includes the new keyword `extends`, allowing a schema to extend a
previously existing entity declaration by adding more parent types and/or more
attributes.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using permissive validation, is it always the case that if A extends B, then A can be used as a subtype of B? I think the answer is probably yes, but we should confirm.

It may be disappointing to provide this feature but then not get subtyping in strict validation. If we anticipate a future in which customers do provide functions or other operators over "standardized" types, it will be very sad if they cannot use those functions on types that extend those types.

Comment on lines +123 to +124
4. Users may incorrectly assume that `extends` provides subtyping, while this
RFC does not propose that it provides subtyping.
Copy link
Contributor

Choose a reason for hiding this comment

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

It does provide it for permissive validation, just not for strict. But that might still be disappointing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it won't unless we add a specific rule that says it does. But we can probably do that.

Comment on lines +135 to +136
## Alternatives

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative X: Do nothing, and instead propose a general way to define and import libraries, perhaps via URLs or some other distribution mechanism. Then let the community decide.

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.

I like the idea of the extends operator in schema (although I'd prefer a different name). But I'm not so keen on the idea of the standard library -- I think the OIDC definition should live in its own repository controlled by a standards committee / invested third parties (so, Alternative A).

Would you consider splitting this RFC into two?

Comment on lines +125 to +130
For instance, users may assume that an action with principal type
`BaseEntityType` could also be used with principal type `MyEntityType` which
`extends` `BaseEntityType`; but this is not the case in the current proposal.
It's also possible that the keyword being named `extends` in particular might
make this problem worse due to existing associations with object-oriented
inheritance; see Alternative H.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my main gripe with the word extends. If we don't allow for this kind of subtyping then I think we should switch to expands (or similar).

Comment on lines +276 to +283
This RFC proposes that `extends` must appear before `in` and attributes:
```
entity MyEntityType extends A in [B];
```
We could instead require `extends` to appear _after_ `in` but before attributes:
```
entity MyEntityType in [B] extends A;
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the RFC as-is; it's useful to have MyEntityType and A as close together as possible.

@andrewmwells-amazon
Copy link
Contributor

I think it would make more sense to break this into two RFCs: adding libraries to Cedar (combining this RFC with parts of #61) and an RFC proposing what types to include in the standard library.

@nynymike
Copy link

nynymike commented Jun 3, 2024

Here's a schema we're using for our Janssen Project Cedar science project that you might want to consider for your library:

A few things to think about that this schema has:

  1. I left the user claim section pretty sparse, as domains can always extend.
  2. I added the ubiquitous "Roles"
  3. I added some standard "Context" claims... which will make all the AI/ML people excited
  4. A lot of tokens share certain standard claims, like iss or jti... so I abstracted these
  5. A lot of enterprise policies are about OAuth clients and OAuth access tokens, so these need to be modeled too
  6. It's important to make the relationship between the trusted issuer and the assertion

I understand the desire to map these to RFC's, but there is a lot of interconnection... for example OpenID Connect Dynamic Client Reg, OAuth Client Reg and Draft OAuth Client Reg with Attestation. I don't think you should boil the ocean here. Just start with a small schema?

@cdisselkoen
Copy link
Contributor Author

cdisselkoen commented Jun 7, 2024

These last four comments (@mwhicks1, @khieta, @andrewmwells-amazon, and @nynymike) all make good points; I'll work on splitting this RFC into (at least) two parts, separating the ideas of library mechanisms, extends, and a Cedar-maintained "standard library".

@cdisselkoen
Copy link
Contributor Author

As the first step of that, I just posted RFC 69: Schema libraries. Comments welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending This RFC is pending; for definitions see the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants