From a619b76d1a89574d36b5e08d6191fcce70696c89 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 23 Jun 2023 15:35:09 -0400 Subject: [PATCH 1/4] initial content --- text/0011-validate-requests.md | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 text/0011-validate-requests.md diff --git a/text/0011-validate-requests.md b/text/0011-validate-requests.md new file mode 100644 index 00000000..62e1c0ae --- /dev/null +++ b/text/0011-validate-requests.md @@ -0,0 +1,72 @@ +# Validate input requests + +## Related issues and PRs + +- Reference Issues: +- Implementation PR(s): + +## Timeline + +- Start Date: 2021-06-23 +- Date Entered FCP: +- Date Accepted: +- Date Landed: + +## Summary + +Given a schema, _validation_ checks that policies conform to the schema, and _schema-based parsing_ checks that entities conform to the schema. +This RFC proposes to add support for checking that a query conforms to a schema. + +## Basic example + +Say the schema specifies that an action `readFile` should only be used with principals of type `User` and resources of type `File`. +This RFC proposes to change the signature of the `Request` constructor to add an optional schema argument that can be used to check argument types when constructing a `Request`. + +Examples (w/ invalid syntax for EUIDs): + +```rust +Request::new(Some(principal), Some(action), Some(resource), context, None); // no schema; works like before +Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, Some(schema)); // ok +Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(User::"bob"), context, Some(schema)); // error, invalid resource +``` + +## Motivation + +(Example migrated from #5) + +Say that a user wants to allow any `User` to view files in the folder `Folder::"public"`. +Currently, to achieve the desired behavior, the user should define a schema with the action entity `readFile` that can apply to principals of type `User` and resources of type `File`, and then write the following policy. + +``` +permit(principal, action == Action::"readFile", resource in Folder::"public"); +``` + +Assuming that they pass in inputs where the principal is of type `User` and the resource is of type `File`, this will work as expected. +However, it could also return "allow" for other principal types like `Folder` or resource types like `User`. +This is a bug on the user's end: they shouldn’t make a query with the action `readFile` when the principal is not a `User` and the resource is not a `File`. +They are breaking the contract they stated in their schema. +Nevertheless, it might be confusing that such a query does not result in a "deny". + +This RFC proposes to add a check, when constructing the `Request` type, that the query is valid according to the schema. + +## Detailed design + +Validation and schema-based parsing are both "opt-in" in the sense that they are not required, so we'll the same for query validation. +Validation is done by a separate API (`Validator::validate`), while schema-based parsing is done by passing a (optional) schema argument into various parsing APIs, e.g., `Entities::from_json_str` and `Context::from_json_str`. +If the schema argument is `None`, then parsing does no validation. + +This RFC proposes to modify the constructor for the `Request` type to add an optional schema type, similar to schema-based parsing. + +## Drawbacks + +- This is a breaking change that will affect any users that are manually constructing `Request`s (which is likely all users). + +## Alternatives + +- Maintain the status quo. Users should enforce that queries conform to their schema prior to authorization. +- Add support for authorization-time checks of entity types, per #5. +This would allow users to manually encode typing restrictions on `principal`s and `resource`s within their policies. + +## Unresolved questions + +None, at the moment. From 15f831ab22fb8b899547d2308738196e5de898ad Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Tue, 27 Jun 2023 11:31:12 -0400 Subject: [PATCH 2/4] updates --- text/0011-validate-requests.md | 53 +++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/text/0011-validate-requests.md b/text/0011-validate-requests.md index 62e1c0ae..286a62f1 100644 --- a/text/0011-validate-requests.md +++ b/text/0011-validate-requests.md @@ -14,25 +14,27 @@ ## Summary -Given a schema, _validation_ checks that policies conform to the schema, and _schema-based parsing_ checks that entities conform to the schema. -This RFC proposes to add support for checking that a query conforms to a schema. +Given a schema, _validation_ checks that policies conform to the schema, and _schema-based parsing_ checks that entities and context conform to the schema. +This RFC proposes to add support for checking that a request conforms to a schema. ## Basic example Say the schema specifies that an action `readFile` should only be used with principals of type `User` and resources of type `File`. This RFC proposes to change the signature of the `Request` constructor to add an optional schema argument that can be used to check argument types when constructing a `Request`. -Examples (w/ invalid syntax for EUIDs): +Examples (using informal syntax for EUIDs): ```rust Request::new(Some(principal), Some(action), Some(resource), context, None); // no schema; works like before + Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, Some(schema)); // ok -Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(User::"bob"), context, Some(schema)); // error, invalid resource + +Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(Folder::"some_folder"), context, Some(schema)); // error, invalid resource ``` ## Motivation -(Example migrated from #5) +### Example 1: Unexpected authorization decisions Say that a user wants to allow any `User` to view files in the folder `Folder::"public"`. Currently, to achieve the desired behavior, the user should define a schema with the action entity `readFile` that can apply to principals of type `User` and resources of type `File`, and then write the following policy. @@ -42,16 +44,38 @@ permit(principal, action == Action::"readFile", resource in Folder::"public"); ``` Assuming that they pass in inputs where the principal is of type `User` and the resource is of type `File`, this will work as expected. -However, it could also return "allow" for other principal types like `Folder` or resource types like `User`. -This is a bug on the user's end: they shouldn’t make a query with the action `readFile` when the principal is not a `User` and the resource is not a `File`. -They are breaking the contract they stated in their schema. -Nevertheless, it might be confusing that such a query does not result in a "deny". +However, it could also return "allow" for other principal and resource types. +For example, say that the user constructs a request with a `User` principal and `Folder` resource. +If that folder happens to be nested inside `Folder::"public"`, then this request may return an "allow" decision even though (as its name suggests) `readFile` should not be applied to folders. +This is a bug on the user's end: they are breaking the contract they stated in their schema. +Nevertheless, it might be confusing that this request does not result in "deny". + +### Example 2: Unexpected authorization-time errors + +Validation assures users that their policies are free from certain types of errors. +But it only does so under certain assumptions, one of which being that the input request conforms to the schema. +We currently do not provide utilities to check that this assumption holds. + +Consider the setup from [Example 1](#example-1-unexpected-authorization-decisions), where the action `readFile` can only apply to principals of type `User` and resources of type `File`. +In addition, say that entities of type `File` have a Boolean field `isPrivate` that restricts which users can view the file, and entities of type `Folder` do not have this field. +Now consider the following variation of the policy from [Example 1](#example-1-unexpected-authorization-decisions). + +``` +permit(principal, action == Action::"readFile", resource in Folder::"public") +unless { resource.isPrivate }; +``` + +The validator will find no faults with this policy, guaranteeing that there will be no type errors at authorization time, modulo some restrictions. +But say that the user unintentionally violates these restrictions by passing a request with a `Folder` as the resource. +This request will result in a "deny" (which is presumably what the user intended), but the diagnostics will report a type error (which is unexpected, given that validation succeeded) due to the invalid access of the field `isPrivate`. + +### Proposed solution -This RFC proposes to add a check, when constructing the `Request` type, that the query is valid according to the schema. +This RFC proposes to add a check, when constructing the `Request` type, that the request is valid according to the schema. ## Detailed design -Validation and schema-based parsing are both "opt-in" in the sense that they are not required, so we'll the same for query validation. +Validation and schema-based parsing are both "opt-in" in the sense that they are not required, so we'll want the same for request validation. Validation is done by a separate API (`Validator::validate`), while schema-based parsing is done by passing a (optional) schema argument into various parsing APIs, e.g., `Entities::from_json_str` and `Context::from_json_str`. If the schema argument is `None`, then parsing does no validation. @@ -64,9 +88,10 @@ This RFC proposes to modify the constructor for the `Request` type to add an opt ## Alternatives - Maintain the status quo. Users should enforce that queries conform to their schema prior to authorization. -- Add support for authorization-time checks of entity types, per #5. -This would allow users to manually encode typing restrictions on `principal`s and `resource`s within their policies. +- Add support for authorization-time checks of entity types, per [#5](https://github.com/cedar-policy/rfcs/pull/5). +This would allow users to manually encode typing restrictions on principals and resources within their policies. +So, in the example [above](#motivation), the user could include `principal is User` and `resource is File` in the policy text to force a "deny" decision when the principal and resource are not of the appropriate types. ## Unresolved questions -None, at the moment. +- The text above proposes to modify the current `Request::new` interface. A less invasive alternative would be to provide a new constructor `Request::new_with_validation` that performs the validation check. The downside of the first approach is that it will break existing users. The downside of the second is that it is easy to ignore: Users may not realize they have the option to perform validation on input requests, and may not realize the importance. From 907028449f83e99203929f9c42bb92be36a08aa4 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 30 Jun 2023 08:47:15 -0400 Subject: [PATCH 3/4] updates --- text/0011-validate-requests.md | 38 ++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/text/0011-validate-requests.md b/text/0011-validate-requests.md index 286a62f1..1421d7c1 100644 --- a/text/0011-validate-requests.md +++ b/text/0011-validate-requests.md @@ -20,16 +20,16 @@ This RFC proposes to add support for checking that a request conforms to a schem ## Basic example Say the schema specifies that an action `readFile` should only be used with principals of type `User` and resources of type `File`. -This RFC proposes to change the signature of the `Request` constructor to add an optional schema argument that can be used to check argument types when constructing a `Request`. +This RFC proposes to add a new constructor for the `Request` type, which takes a schema argument that is used to validate fields when constructing a `Request`. Examples (using informal syntax for EUIDs): ```rust -Request::new(Some(principal), Some(action), Some(resource), context, None); // no schema; works like before +Request::new(Some(principal), Some(action), Some(resource), context); // original API; unchanged by this RFC -Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, Some(schema)); // ok +Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, Some(schema)); // returns a Request -Request::new(Some(User::"alice"), Some(Action::"readFile"), Some(Folder::"some_folder"), context, Some(schema)); // error, invalid resource +Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(Folder::"some_folder"), context, Some(schema)); // returns an error (invalid resource) ``` ## Motivation @@ -79,19 +79,35 @@ Validation and schema-based parsing are both "opt-in" in the sense that they are Validation is done by a separate API (`Validator::validate`), while schema-based parsing is done by passing a (optional) schema argument into various parsing APIs, e.g., `Entities::from_json_str` and `Context::from_json_str`. If the schema argument is `None`, then parsing does no validation. -This RFC proposes to modify the constructor for the `Request` type to add an optional schema type, similar to schema-based parsing. +This RFC proposes to add a new constructor for the `Request` type, which takes a schema argument that is used to validate fields when constructing a `Request`. +More concretely, the new API will have the following signature: + +```rust +pub fn new_with_validation( + principal: Option, + action: Option, + resource: Option, + context: Context, + schema: &Schema + ) -> Result { + ... + } +``` + +(This is the same as the signature for `Request::new`, aside from the schema argument and return type.) + +If the action is `None` (indicating that it is "unspecified"), then the function performs no additional checks. +If the action is `Some`, then the function checks that the specified action is present in the schema, and that the principal and resource are consistent with the action's `appliesTo` lists in the schema. ## Drawbacks -- This is a breaking change that will affect any users that are manually constructing `Request`s (which is likely all users). +One drawback of the current approach is that users may default to using `Request::new` rather than `Request::new_with_validation`, when they should really be using the latter if (1) they have a schema and (2) they are not already checking request fields through some other means. ## Alternatives -- Maintain the status quo. Users should enforce that queries conform to their schema prior to authorization. +- Modify the current `Request::new` interface to take an optional schema argument, rather than providing a new constructor. The downside of this approach is that it will break existing users. The benefit is that it is impossible to ignore: When the current API breaks, users will realize they have the option to perform validation on input requests. The optional schema argument would be consistent with other APIs that perform validation, like `Entities::from_json_str`. + - _Decision:_ it is not worth breaking existing users. We may revisit this decision in the future and replace `Request::new` with `Request::new_with_validation`, with appropriate deprecation warnings in advance. - Add support for authorization-time checks of entity types, per [#5](https://github.com/cedar-policy/rfcs/pull/5). This would allow users to manually encode typing restrictions on principals and resources within their policies. So, in the example [above](#motivation), the user could include `principal is User` and `resource is File` in the policy text to force a "deny" decision when the principal and resource are not of the appropriate types. - -## Unresolved questions - -- The text above proposes to modify the current `Request::new` interface. A less invasive alternative would be to provide a new constructor `Request::new_with_validation` that performs the validation check. The downside of the first approach is that it will break existing users. The downside of the second is that it is easy to ignore: Users may not realize they have the option to perform validation on input requests, and may not realize the importance. + - _Decision:_ The `is` operator can help with the current problem somewhat, but is not an ideal solution (what happens if users forget to include `is` in their policies?). The `is` operator has its own merits, and its RFC will be considered independently of this one. From e794ae68c568fc7897ca3f1ce14b4194506faa1a Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Fri, 30 Jun 2023 11:02:42 -0400 Subject: [PATCH 4/4] drop Some --- text/0011-validate-requests.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0011-validate-requests.md b/text/0011-validate-requests.md index 1421d7c1..f7a360dc 100644 --- a/text/0011-validate-requests.md +++ b/text/0011-validate-requests.md @@ -27,9 +27,9 @@ Examples (using informal syntax for EUIDs): ```rust Request::new(Some(principal), Some(action), Some(resource), context); // original API; unchanged by this RFC -Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, Some(schema)); // returns a Request +Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(File::"secret_file.txt"), context, schema); // returns a Request -Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(Folder::"some_folder"), context, Some(schema)); // returns an error (invalid resource) +Request::new_with_validation(Some(User::"alice"), Some(Action::"readFile"), Some(Folder::"some_folder"), context, schema); // returns an error (invalid resource) ``` ## Motivation