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

test(jans-cedarling): add tests and fix bugs caught in testing #9999

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

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Oct 31, 2024

Prepare


Description

This PR adds tests for JwtService to improve test coverage. Please see the target issue for the tests covered.

Target issue

target issue: #9995

closes #9995

Implementation Details


Test and Document the changes

  • [x 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.

…able types

- replace token structs in test utils with generic serializable types for greater test flexibility

Signed-off-by: rmarinn <[email protected]>
- Implement tests to verify error handling when required claims are missing
  (iss, aud, sub, iat, exp).
- Add test for when the access_token has an invalid signature.

Signed-off-by: rmarinn <[email protected]>
- Implement tests to verify error handling when required claims are missing
  (iss, aud, sub, iat, exp).
- Add test for when the id_token has an invalid signature.
- Add test for when the id_token has a different iss with
  access_token.
- Add test for when the id_token has a different aud with
  access_token.
- Add test for when the id_token is expired.

Signed-off-by: rmarinn <[email protected]>
- Implement tests to verify error handling when required claims are missing
  (iss, aud, sub, iat, exp).
- Add test for when the userinfo_token has an invalid signature.
- Add test for when the userinfo_token has a different iss with
  the access_token.
- Add test for when the userinfo_token has a different aud with
  the access_token.
- Add test for when the userinfo_token has a different sub with
  the id_token.

Signed-off-by: rmarinn <[email protected]>
- fixed a bug where the validation for the `aud` and `iss` of the
  userinfo_token is mixed up

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

- This change consolidates the parameters for the `decode` function into a single
  `DecodingArgs` struct, for easier code readability and maintainability.

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

- renamed decoding_strategy::Error::JwkMissingKid to decoding_strategy::Error::JwtMissingKeyId

Signed-off-by: rmarinn <[email protected]>
- add test expecting to error when using access_token before nbf
- add test expecting to error when using id_token before nbf
- add test expecting to error when using userinfo_token nbf

Signed-off-by: rmarinn <[email protected]>
@rmarinn rmarinn added the comp-jans-cedarling Touching folder /jans-cedarling label Oct 31, 2024
@rmarinn rmarinn self-assigned this Oct 31, 2024
@rmarinn rmarinn linked an issue Oct 31, 2024 that may be closed by this pull request
28 tasks
Copy link

dryrunsecurity bot commented Oct 31, 2024

DryRun Security Summary

The pull request focuses on improving the security and reliability of the JWT (JSON Web Token) handling functionality in the Cedarling application, with changes spanning several files and covering various aspects of JWT decoding, validation, and error handling.

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 functionality in the Cedarling application. The changes span several files and cover various aspects of JWT decoding, validation, and error handling.

Key security-related improvements include:

  1. Improved Error Handling: The code introduces a more comprehensive error handling mechanism, with detailed error types that cover various failure scenarios, such as unsupported algorithms, missing key IDs, and validation failures.
  2. Enhanced Validation Logic: The JWT decoding and validation process has been strengthened, with more thorough checks for claims, such as ensuring the consistency of the iss (issuer), aud (audience), and sub (subject) claims across different token types.
  3. Secure Key Management: The code demonstrates a well-designed approach to fetching and managing the keys used for JWT signature verification, including handling scenarios where the JWKS (JSON Web Key Set) or OpenID configuration endpoints are unreachable.
  4. Comprehensive Testing: The test suite has been expanded to cover a wide range of negative scenarios, ensuring that the JWT service can properly handle invalid or malicious tokens and maintain the application's security.

Overall, the changes in this pull request show a strong focus on improving the security and robustness of the JWT handling functionality in the Cedarling application, which is a critical component for securing the application's authentication and authorization processes.

Files Changed:

  1. authz/mod.rs: Updates the AuthorizeError enum to use the more generic jwt::Error instead of the specific jwt::JwtDecodingError.
  2. jwt/decoding_strategy.rs: Introduces two decoding strategies, WithoutValidation and WithValidation, with the latter performing important validation checks on the JWT.
  3. common/policy_store.rs: Includes changes related to the handling of trusted issuers and token metadata, which are important for ensuring the security of the authorization process.
  4. authorize_with_jwt_validation.rs: Updates the sample JWT claims to include more detailed user information, which can enhance the security and usability of the application.
  5. key_service/error.rs: Simplifies the error handling in the key service component, which is crucial for the security of the JWT validation process.
  6. mod.rs: Improves the validation logic for the access_token, id_token, and userinfo_token, ensuring consistency and security across the different token types.
  7. Various test files: Expand the test suite to cover a wide range of negative scenarios, ensuring the robustness and security of the JWT handling functionality.

Code Analysis

We ran 9 analyzers against 20 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 area-CI Issue or changes required in automatic builds or CI infrastructure label Oct 31, 2024
@rmarinn rmarinn marked this pull request as draft October 31, 2024 13:32
@abaghinyan abaghinyan self-requested a review October 31, 2024 19:41
- references to `JwtService::decode_claims` updated to `JwtService::decode_tokens`

Signed-off-by: rmarinn <[email protected]>
- add test that should error when a key with a given `kid`
  that should be used for validating a token can't be found.
- add a test that panics when the openid configuration cannot
  be fetched at JwtService's initialization.
  the openid configuration cannot be fetched
- add a test that panics when the JWKS cannot be fetched at
  JwtService's initialization.

Signed-off-by: rmarinn <[email protected]>
- moved `can_update_local_jwks` from `with_validation.rs` to
  `key_service.rs`

Signed-off-by: rmarinn <[email protected]>
- updated docstrings on some test files to more accurately indicate what they contain.
- remove unnecessary "unexpected" data checks on tests and just have it on one.

Signed-off-by: rmarinn <[email protected]>
@rmarinn rmarinn marked this pull request as ready for review November 1, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CI Issue or changes required in automatic builds or CI infrastructure comp-jans-cedarling Touching folder /jans-cedarling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test(jans-cedarling): add tests and fix bugs caught in testing
2 participants