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

Partial schema validation #8

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

Conversation

john-h-kastner-aws
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws commented Jun 16, 2023

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.

(Got delayed, so pushing a couple of comments but not a complete review.)

@john-h-kastner-aws john-h-kastner-aws changed the title Partial schema validation RFC Partial schema validation Jun 26, 2023
@cdisselkoen cdisselkoen added the pending This RFC is pending; for definitions see the README label Jul 3, 2023
@max2me
Copy link

max2me commented Jul 7, 2023

I just want to point out that ability to declare additionalAttributes on any Record type (as this RFC proposed) provides a fine-grained control over validation and makes it possible to accomodate scenarios that AWS Verified Access has. There service knows all top level keys of the context but might not have information about all attributes of nested Records.


# Summary

This RFC proposes extending the Cedar validator to work with partial or even complete empty schemas,
Copy link
Contributor

Choose a reason for hiding this comment

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

complete -> completely

When a policy mentions an undefined entity type, or attempts to access an undefined attribute of some entity type,
the validator will not signal an error, unless it recognizes that there is an inevitable type error.
For example, even if the type of `access_level` is not declared, we know `principal.access_level > "5"` will inevitably error due to the `>` comparison against a string.
Partial schema validation does not attempt to detect all type errors, and is therefore unsound.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per offline discussion, it would be interesting to emit warnings in cases where the current proposal silently accepts the policy (e.g., in cases where the policy mentions an undefined entity type, or attempts to access an undefined attribute).

I like this idea because it allows us to prove some kind of soundness property (namely that in the absence of validation warnings or errors, the policy will not produce an error at authorization time). The other benefit of this design (as pointed out in offline discussion) is that it allows for tooling to help users improve their schemas (e.g., offering to add a definition for a new entity type).

# Motivation

We want to provide some of the benefits of policy validation to Cedar users who have not written or may not be able to write a schema.
Detecting any amount potential errors during development is preferable to discovering the errors later,
Copy link
Contributor

Choose a reason for hiding this comment

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

amount -> amount of

Undeclared entity types and actions are treated as incomplete according to the second two kinds of incompleteness.
At the extreme, it is possible to have a schema that does not declare any entity types or actions.

This policy will validate with an empty schema that does not declares either of the entity types or the action.
Copy link
Contributor

Choose a reason for hiding this comment

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

either of the entity types or the action -> any entity types or actions

The bottom type functions like the [TypeScript `any` type](https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#any).
It is used to ensure that the type is allowed to appear in any expression without causing additional type errors.
The `>` operator expects a value of type `Long` as its operand.
It will in fact accept any subtype of `Long`, and the bottom type is a subtype of every type, so it accepted.
Copy link
Contributor

Choose a reason for hiding this comment

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

accepted -> is accepted

so an expression `principal.role == "Admin"` would still be accepted by the validator.

Alternatively, we might have complete information about the attributes present for `User` entities.
When we know that `access_level` does not exist, the `shape` for the principal entity type can include `"additionalAttributes": false` to specify that there are no undeclared attributes for this entity type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that "additionalAttributes": false is the default, so it is unnecessary to say this explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that it's noted in a later section, but still good to include here to head off confusion.

We extend the schema format to support the above example schema.
Specifically, we add the following properties to the JSON:

* Any record type, including the record types for entity attributes and action context attributes, may include `"additionalAttributes": true` to specify that values having that record type may have attribute other than those declared.
Copy link
Contributor

Choose a reason for hiding this comment

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

may have attribute -> may have attributes

Comment on lines +184 to +185
This change does not necessarily require any change to the behavior of the standard validator,
but the additions to the schema format could be soundly supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the implementation make these changes, or is this noting that the changes are possible?

Comment on lines +198 to +199
A sound, type inference based, approach to partial schema validation would report _more_ type error than this proposal,
so it could not be used as a drop-in replacement.
Copy link
Contributor

Choose a reason for hiding this comment

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

type error -> type errors

Also: why would an inference-based approach result in more errors?


* Policy validation is not required to use Cedar,
so we could continue to support validation only for users who are able to provide a complete schema.
* We could design and implement a type inference algorithm for 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 would be useful to have more details about the schema inference option to determine whether it is a viable alternative.

john-h-kastner-aws added a commit to cedar-policy/cedar that referenced this pull request Nov 27, 2023
Experimental implementation of partial schema validation as described in cedar-policy/rfcs#8.
@john-h-kastner-aws john-h-kastner-aws added pre-acceptance-experimental This RFC is pre-acceptance experimental; for definitions see the README and removed pending This RFC is pending; for definitions see the README labels Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pre-acceptance-experimental This RFC is pre-acceptance experimental; for definitions see the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants