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

Schema libraries #69

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

Schema libraries #69

wants to merge 3 commits into from

Conversation

cdisselkoen
Copy link
Contributor

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
declarations.
(In the future, another RFC could propose allowing imports in other positions.)

The target of the import must be a raw file containing a valid Cedar schema.
Copy link

Choose a reason for hiding this comment

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

Should they instead be a namespace? And then we leave providing the schemas from a location up to an upstream process?

Copy link

Choose a reason for hiding this comment

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

Sort of like how Smithy does it: https://smithy.io/1.0/spec/core/idl.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should they instead be a namespace?

I might be misunderstanding something -- this RFC as-written allows importing items in a namespace; in fact, all declared items in the library (import target) must be in a (possibly empty) namespace, as otherwise it wouldn't be a valid Cedar schema.

You might be looking for a mechanism for "opening" a namespace, i.e., to use items from a namespace unqualified in the rest of the schema/policies? (Akin to Rust's use.) I think it makes sense to separate that into a different RFC; this one is about providing definitions from reusable libraries, while another one could be about making namespaced definitions usable without qualification.

(I realize that Java's import, and maybe imports in other languages, provide both functionalities -- introducing the library definitions and also making them available for use unqualified. The import being proposed here provides only the first functionality. For that reason maybe we should consider a different keyword to avoid confusion?)

Copy link

Choose a reason for hiding this comment

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

I was thinking about how I would use this for a specific use-case I have now. But after thinking a bit more, I think the answer is: I wouldn't. In my case, I want to define my schema dependencies in "packages" and then use a package/dependency manager tool to declare my dependencies and deal with retrieving the artifacts. All I really need from the Cedar library is that ability to load a schema from a set of files (you can already mostly do this but there are some sharp edges.) I do think the "opening" a namespace feature would be useful, but I agree thats a separate RFC.

But that raises the question: do we want "import" in the schema spec at all? Are we bringing unnecessary complexity into Cedar that is best left to some upstream system more suited to deal with it (e.g. a package manager)? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can already mostly do this but there are some sharp edges

Can you talk more about the sharp edges? We'd like to track these as issues :)

Are we bringing unnecessary complexity into Cedar that is best left to some upstream system more suited to deal with it (e.g. a package manager)?

Valid point; this kind of question was one reason for posting the RFC. Interested to see if a majority of folks feel this way, or if there are folks who feel that import is solving a problem for them. Probably this will become clear as this RFC generates more feedback.

Copy link

Choose a reason for hiding this comment

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

The biggest sharp edge was I wanted to load each schema file as a SchemaFragment and then produce the final schema and validate. However, there were cases where it would fail to validate the fragment since it referenced things in other fragments. So I end up handing to convert all the schemas to the human readable format and append them together. Then load and validate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. In my understanding, referencing things in other fragments should "just work", and if it doesn't, that is a bug. If you have any specific examples, it would be great if we could get reproducers so we can fix the bugs.

This would allow the Cedar community to gradually coalesce on the "best" way to
represent an OIDC user in Cedar.

### Motivations common to both scenarios
Copy link

Choose a reason for hiding this comment

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

Could we also have mechanism to use a particular namespace? For instance, say I'm building an app where everything is under the AWS namespace. It would be nice if I could just use AWS::IdentityCenter and then policy authors only need to specify the sub-entity. e.g. User vs AWS::IdentityCenter::User.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can totally see this being useful, but I'm torn whether it should be part of this RFC or a separate RFC.

It would be harder to envision what a use would look like for policies (as opposed to schemas); I'm not sure if users will be happy with a use mechanism for schemas but not having one for policies?

Copy link

Choose a reason for hiding this comment

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

Yeah, its most useful for policies IMO. Perhaps a separate RFC is better.

Instead, it proposes that versioning would be done _above_ the Cedar layer,
i.e., should be the responsibility of library authors.
For instance, library authors could provide a different URL for different
versions of their library, avoiding changing the contents of the URL for the
Copy link
Contributor

Choose a reason for hiding this comment

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

We should provide something akin to sub resource integrity so users can guard against a changing import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bringing here the summary of an offline conversation -- it makes a lot of sense to have some feature like this, but since including hashes directly in the schema file is a little ugly, maybe we'd want a separate lockfile, and at that point we might want to consider some separate Cedar.toml file (analogous to Cargo.toml) where you could declare all your dependencies, and possibly other global configuration. Does the long-term path see Cedar having something more and more equivalent to cargo, complete with dependency/lockfile manager, package repository, etc?

The target of the import must be a raw file containing a valid Cedar schema.
This schema may contain any definitions that are valid today in Cedar schemas,
including namespaces, entity type definitions, common type definitions, and
action declarations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the actions in a schema can impact authorization, a schema library adding or removing an action from a group could result in unexpected authorization decisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you propose that libraries cannot include action declarations, and can only include entity type definitions and common type definitions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't that's worth it. Presumably, if you're using a schema library, you're consuming entity data from the IdP as well. The issue is just that you might not expect a schema to impact authorization. Sub-resource integrity plus documentation should mitigate this concern.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
that it could download libraries from remote URLs. To mitigate this for users
who are concerned about this and don't need/want this feature (e.g., in
resource-constrained environments, offline environments, Wasm, etc), we could
put this RFC's functionality behind a Cargo feature, so that it and its
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws Jun 10, 2024

Choose a reason for hiding this comment

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

Possibly local file system dependency enabled by default, with network dependencies behind a flag? Or maybe we could consider not including network dependencies at all if we think this is a large concern. Though that option would doubtlessly lead us to wanting to build a separate dependency management tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment makes me realize I should definitely explicitly call out allowing local file system URIs in addition to URLs, which is not currently made explicit in the RFC. I'll amend

Copy link

Choose a reason for hiding this comment

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

Take a look at what IonSchema does with their "SchemaAuthority": https://amazon-ion.github.io/ion-schema/docs/isl-2-0/spec. Essentially decoupling the loading of dependencies from their declaration.

But I'm wondering if we really need/want to bring this into Cedar. (See my other comment.)

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@sarahcec sarahcec added the archived This RFC is pending, but no action is currently planned -- please comment if you are interested label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived This RFC is pending, but no action is currently planned -- please comment if you are interested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants