refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721

rmarinn · 2025-01-23T09:36:07Z

Prepare

Read PR guidelines
Read license information

Description

This PR implements gracefully handling recoverable errors -- specifically, "bad input" errors where the issue is either a malformed or invalid input. In the case of a bad input, the call to Cedarling::authorize will not no longer return an error but will return an Ok with a DENY decision together with a reason.

Target issue

closes #10589

Implementation Details

All the errors from the AuthorizeError enum that are just bad inputs and recoverable has been moved to a BadInputErrorEnum.
AuthorizeResult has been updated to this:

#[derive(Debug, Serialize)]
pub struct AuthorizeResult {
    #[serde(serialize_with = "serialize_opt_response")]
    pub workload: Option<cedar_policy::Response>,
    #[serde(serialize_with = "serialize_opt_response")]
    pub user: Option<cedar_policy::Response>,
    #[serde(
        serialize_with = "serialize_reason_input",
        skip_serializing_if = "Option::is_none"
    )]
    pub reason_input: Option<BadInputError>,
    pub decision: bool,
}

which looks like this when serialized to json:

{
  "workload": null,
  "user": null,
  "reason_input": "Invalid Access token: Failed to decode JWT from Base64: Invalid last symbol 48, offset 749.",
  "decision": false
}

Test and Document the changes

Static code analysis has been run locally and issues have been fixed
Relevant unit and integration tests have been added/updated
Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

I confirm that there is no impact on the docs due to the code changes in this PR.

Signed-off-by: rmarinn <[email protected]>

- gracefully handle authorization errors where the problem is just input errors or failing to build entities. - update AuthorizeResult to return deny with some information if the authz was denied because of malformed inputs Signed-off-by: rmarinn <[email protected]>

…ction Signed-off-by: rmarinn <[email protected]>

Signed-off-by: rmarinn <[email protected]>

- move input errors from AuthorizeError to the new BadInputErrors enum Signed-off-by: rmarinn <[email protected]>

Signed-off-by: rmarinn <[email protected]>

rmarinn · 2025-01-23T09:40:30Z

try to merge #10591 before this one

olehbozhok · 2025-01-23T22:45:44Z

jans-cedarling/cedarling/src/authz/mod.rs

-            })
-            .set_message("Result of authorize.".to_string()),
-        );
+        self.log_authz(LogAuthzArgs {


Please, do not touch logging part, to avoid conflicts in the #10705

If you wish to make this update, how about extract this changes to another branch? And I will merge it to my branch

The changes does not only involve moving the logging to another file, some of the inputs change as well. Thus, if you want to move the changes, you might as well merge this whole branch onto your branch.

There will be conflicts no matter what, and we'll just have to deal with it.

Another option is to just merge main into your branch if this merges first.

Or I could also just wait for your PR then fix the conflicts if you want.

olehbozhok · 2025-01-23T23:31:25Z

jans-cedarling/cedarling/src/authz/mod.rs

+
+/// Error returned because of invalid or malformed inputs to [`Authz::authorize`]
+#[derive(thiserror::Error, Debug)]
+pub enum BadInputError {


Does all this error can happen only because of bad input only?
If we can't check jwt because of problems on issuer page (can't load keys), it is also bad input?

If we can't check jwt because of problems on issuer page (can't load keys), it is also bad input?

Getting the keys happens on startup and in the background/SSE (not implemented yet) so JWT validation wont fail because of an HTTP request. However, if a decoding key cannot be found to decode the JWT or if the JWT is expired, etc., it's under the BadInputError.

jans-cedarling/cedarling/src/log/log_entry.rs

olehbozhok · 2025-01-23T23:44:17Z

jans-cedarling/cedarling/src/authz/authorize_result.rs

+        serialize_with = "serialize_reason_input",
+        skip_serializing_if = "Option::is_none"
+    )]
+    pub reason_input: Option<BadInputError>,


I think It should be Vec because it can be more that one recoverable errors.

And in the issue is said that for "decision": "DENY" will be key with "errors" and for "decision": "DENY" will be key with "warnings".

It is planning?

I think It should be Vec because it can be more that one recoverable errors.

We return immediately instead of going through the request the in the first recoverable error since there's no point continuing if for example the access token is malformed. In that context, is does Vec still make sense since i think it will always only have 1 item.

And in the issue is said that for "decision": "DENY" will be key with "errors" and for "decision": "DENY" will be key with "warnings".

Yeah, i just changed it from "warnings" to "reason_input" after looking at authzen. It looks like they prefer using "reason_*" for adding additional context.

Hmm, the task handle recoverable errors gracefully in authorization. As I understand it means that if we have error on build role (for example) or other entity, and we can continue to execute authorize request we should continue. And this kind of errors should be collected in the vector (and be logged).

@nynymike what do you think?

I agree with your statement, but I think the scope of the issue was beyond authz. I don't want the Cedarling to crash ever... it should keep running. Your web server never crashes... A crash should only happen if something really catastrophic happens. Right now it seems like there are too many reasons the PDP might just crash.
That was my goal here...

@SafinWasi does flask-sidecar crash on bad input? Do you use try ... expect ... statement like here?

The sidecar does not crash on invalid input. It can catch cedarling exceptions and puts them in the response.

jans/jans-cedarling/flask-sidecar/main/base/cedarling/cedarling.py

Line 121 in 2c0cc62

try:

jans-cedarling/cedarling/src/tests/mapping_entities.rs

Signed-off-by: rmarinn <[email protected]>

rmarinn added 7 commits January 22, 2025 13:37

refactor(jans-cedarling): error message when creatig workload and user

9e4eee7

Signed-off-by: rmarinn <[email protected]>

refactor(jans-cedarling): move logging authz result to a separate fun…

e37b740

…ction Signed-off-by: rmarinn <[email protected]>

refactor(jans-cedarling): restore workload & user in AuthorizeResult

6dac85a

Signed-off-by: rmarinn <[email protected]>

refactor(jans-cedarling): remove input errors from AuthorizeError

d46ad36

- move input errors from AuthorizeError to the new BadInputErrors enum Signed-off-by: rmarinn <[email protected]>

refactor(jans-cedarling): update python bindings with new implementation

02c127a

Signed-off-by: rmarinn <[email protected]>

refactor(jans-cedarling): update wasm bindings with new implementation

4df20d6

Signed-off-by: rmarinn <[email protected]>

rmarinn requested review from djellemah, nynymike, olehbozhok and SafinWasi January 23, 2025 09:36

Merge branch 'main' into jans-cedarling-10589

3eae8dc

Signed-off-by: rmarinn <[email protected]>

olehbozhok previously approved these changes Jan 23, 2025

View reviewed changes

chore(jans-cedarling): fix typo

7599ac7

Signed-off-by: rmarinn <[email protected]>

rmarinn dismissed olehbozhok’s stale review via 7599ac7 January 24, 2025 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721

refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721

rmarinn commented Jan 23, 2025

rmarinn commented Jan 23, 2025

olehbozhok Jan 23, 2025

olehbozhok Jan 23, 2025

rmarinn Jan 24, 2025

olehbozhok Jan 23, 2025

rmarinn Jan 24, 2025 •

edited

Loading

olehbozhok Jan 23, 2025

rmarinn Jan 24, 2025

olehbozhok Jan 24, 2025

olehbozhok Jan 24, 2025

nynymike Jan 24, 2025

olehbozhok Jan 24, 2025

SafinWasi Jan 24, 2025

refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721

Are you sure you want to change the base?

refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721

Conversation

rmarinn commented Jan 23, 2025

Prepare

Description

Target issue

Implementation Details

Test and Document the changes

rmarinn commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinn Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinn Jan 24, 2025 •

edited

Loading