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

feat(jans-cedarling): improve error handling for JWKS responses #9982

Merged
merged 23 commits into from
Nov 7, 2024

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Oct 30, 2024

NOTE: this merge is dependent on #9999

Prepare


Description

This PR enhances the error handling mechanism when fetching from a remote JWKS so that Cedarling does not halt initialization when encountering a key with an unsupported algorithm. This improvement will allow for smoother operation and better handling of dynamic key sets.

Target issue

target issue: #9966

closes #9966

Implementation Details

  • added logic to skip over JWKs that use unsupported algorithms without breaking the initialization process
  • updated error handling to avoid stopping service initialization when encountering unknown algorithm variants in JWKs

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.

@rmarinn rmarinn linked an issue Oct 30, 2024 that may be closed by this pull request
Copy link

dryrunsecurity bot commented Oct 30, 2024

DryRun Security Summary

The pull request focuses on improving the security and reliability of the JWT (JSON Web Token) handling and validation functionality in the cedarling project, with changes spanning multiple files and addressing various aspects of JWT processing, including decoding, validation, error handling, and key management.

Expand for full summary

Summary:

The code changes in this pull request focus on improving the security and reliability of the JWT (JSON Web Token) handling and validation functionality in the cedarling project. The changes span multiple files and address various aspects of JWT processing, including decoding, validation, error handling, and key management.

The key security-related improvements include:

  1. Conditional inclusion of the "exp" (expiration) claim in the list of required claims, allowing the application to optionally skip the validation of the expiration claim.
  2. Explicit handling of unsupported algorithms, ensuring that the application only processes tokens signed with approved algorithms.
  3. Comprehensive error handling and reporting, with the introduction of new error types to provide detailed information about various failure scenarios.
  4. Secure key management, including the use of thread-safe and shared-ownership wrappers for the cryptographic keys used in the JWT decoding process.
  5. Robust validation of JWT claims, such as "iss", "aud", "sub", "nbf", and "exp", to ensure the integrity and authenticity of the tokens.
  6. Graceful handling of missing or unsupported keys in the JWKS (JSON Web Key Set) returned by the trusted issuer.

Overall, the changes in this pull request demonstrate a security-conscious approach to the implementation of JWT-based authentication and authorization in the cedarling project, with a focus on mitigating potential security vulnerabilities and maintaining the reliability and robustness of the system.

Files Changed:

  1. jans-cedarling/cedarling/src/jwt/decoding_strategy.rs: Improvements to the JWT decoding and validation logic, including the conditional inclusion of the "exp" claim and the handling of unsupported algorithms.
  2. jans-cedarling/cedarling/src/jwt/decoding_strategy/error.rs: Introduction of a new JwtDecodingError enum to handle various error cases that can occur during JWT decoding and validation.
  3. jans-cedarling/cedarling/examples/authorize_with_jwt_validation.rs: Demonstration of JWT validation and policy-based authorization using the Cedarling library.
  4. jans-cedarling/cedarling/src/init/service_config.rs: Changes to the HTTP client implementation and the handling of trusted issuers and OpenID configurations.
  5. jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service/openid_config.rs: Improvements to the thread-safe and shared-ownership handling of the cryptographic keys used for JWT decoding.
  6. jans-cedarling/cedarling/src/jwt/jwt_service_config.rs: Refactoring of the JWT service configuration to use an abstraction for the HTTP client.
  7. jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service.rs: Implementation of a KeyService for retrieving and caching the cryptographic keys used for JWT decoding.
  8. jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service/error.rs: Enhancements to the KeyServiceError enum to handle various error conditions related to key management.
  9. jans-cedarling/cedarling/src/jwt/mod.rs: Improvements to the JWT validation logic, including more comprehensive checks on the access_token, id_token, and userinfo_token.
  10. jans-cedarling/cedarling/src/jwt/test/with_validation/: Addition of various test cases to ensure the proper handling of different JWT-related error scenarios.

Code Analysis

We ran 9 analyzers against 17 files and 0 analyzers had findings. 9 analyzers had no findings.

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

@mo-auto mo-auto added the comp-jans-cedarling Touching folder /jans-cedarling label Oct 30, 2024
@rmarinn rmarinn changed the title Jans cedarling 9966 feat(jans-cedarling): improve error handling for JWKS responses Oct 30, 2024
@mo-auto mo-auto added the kind-feature Issue or PR is a new feature request label Oct 30, 2024
@abaghinyan abaghinyan self-requested a review October 30, 2024 21:35
Copy link

@abaghinyan abaghinyan left a comment

Choose a reason for hiding this comment

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

Some suggestions for improvement

@rmarinn rmarinn self-assigned this Nov 1, 2024
@rmarinn rmarinn force-pushed the jans-cedarling-9966 branch 4 times, most recently from e186594 to b92364a Compare November 2, 2024 06:30
abaghinyan
abaghinyan previously approved these changes Nov 3, 2024
rmarinn added a commit that referenced this pull request Nov 3, 2024
feat(jans-cedarling): add graceful error handling for unsupported algorithms in JWKs

- added logic to skip over JWKs that use unsupported algorithms without breaking the initialization process
- updated error handling to avoid stopping service initialization when encountering unknown algorithm variants in JWKs

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

refactor(jans-cedarling): extract repeated code into a method for better code reusability

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

chore(jans-cedarling): fix misspellings

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

fix(jans-cedarling): add error handling to update_jwks_for_iss

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

test(jans-cedarling): improve test assertions and messages

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

test(jans-cedarling): improve panic message in test utils

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

test(jans-cedarling): Make generate_token_using_claims return a Result

- Make gernerate_token_using_claims return a Result instead of panicking
  for improved error management in tests.

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

feat(jans-cedarling): add retry mechanism for KeyService HTTP requests

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

chore(jans-cedarling): add missing license header

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

chore(jans-cedarling): resolve clippy issues

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

chore(jans-cedarling): update example for authorize_with_jwt_validation.rs

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

refactor(jans-cedarling): remove `exp` and `nbf` requirement for userinfo_token

Signed-off-by: rmarinn <[email protected]>
@rmarinn rmarinn marked this pull request as ready for review November 4, 2024 17:20
@djellemah djellemah self-requested a review November 4, 2024 17:30
@@ -0,0 +1,5 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why am I still seeing raw binary tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the example file to show how the user can use their own tokens from the test server. please see authorize_with_validation.rs for the instructions.

nynymike
nynymike previously approved these changes Nov 4, 2024
djellemah
djellemah previously approved these changes Nov 4, 2024
Copy link
Contributor

@djellemah djellemah left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

abaghinyan
abaghinyan previously approved these changes Nov 4, 2024
@rmarinn rmarinn dismissed stale reviews from abaghinyan, djellemah, and nynymike via c324a97 November 5, 2024 01:51
duttarnab
duttarnab previously approved these changes Nov 5, 2024
…orithms in JWKs

- added logic to skip over JWKs that use unsupported algorithms without breaking the initialization process
- updated error handling to avoid stopping service initialization when encountering unknown algorithm variants in JWKs

Signed-off-by: rmarinn <[email protected]>
- Make gernerate_token_using_claims return a Result instead of panicking
  for improved error management in tests.

Signed-off-by: rmarinn <[email protected]>
- Implement `generate_tokens_using_claims` which is a helper function
  that generates an access_token, id_token, and userinfo_token from the
  given claims.

Signed-off-by: rmarinn <[email protected]>
- return struct instead of tuple for generate_token_using_claims

Signed-off-by: rmarinn <[email protected]>
- the user should create their own tokens.json

Signed-off-by: rmarinn <[email protected]>
..Default::default()
};

// serialize token to a string
jwt::encode(&header, &claims, &encodking_key.key).expect("should generate token")
Ok(jwt::encode(&header, &claims, &encoding_key.key)?)
}

/// Invalidates a JWT Token by altering the first two characters in its signature
///
/// # Panics
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can remove /// # Panics if it has no text

@duttarnab duttarnab enabled auto-merge (squash) November 7, 2024 14:38
@duttarnab duttarnab merged commit f56c118 into main Nov 7, 2024
1 of 2 checks passed
@duttarnab duttarnab deleted the jans-cedarling-9966 branch November 7, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(jans-cedarling): improve error handling for JWKS responses
8 participants