-
Notifications
You must be signed in to change notification settings - Fork 90
feat(jans-cedarling): improve error handling for JWKS responses #9982
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
Conversation
DryRun Security SummaryThe pull request focuses on improving the security and reliability of the JWT (JSON Web Token) handling and validation functionality in the Expand for full summarySummary: 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 The key security-related improvements include:
Overall, the changes in this pull request demonstrate a security-conscious approach to the implementation of JWT-based authentication and authorization in the Files Changed:
Code AnalysisWe ran Riskiness🟢 Risk threshold not exceeded. |
There was a problem hiding this 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
jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service.rs
Outdated
Show resolved
Hide resolved
jans-cedarling/cedarling/src/jwt/test/can_support_dynamic_jwks.rs
Outdated
Show resolved
Hide resolved
jans-cedarling/cedarling/src/jwt/test/can_decode_claims_with_validation.rs
Outdated
Show resolved
Hide resolved
e186594
to
b92364a
Compare
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 <34529290+rmarinn@users.noreply.github.com> refactor(jans-cedarling): extract repeated code into a method for better code reusability Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> chore(jans-cedarling): fix misspellings Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> fix(jans-cedarling): add error handling to update_jwks_for_iss Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> test(jans-cedarling): improve test assertions and messages Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> test(jans-cedarling): improve panic message in test utils Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> 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 <34529290+rmarinn@users.noreply.github.com> feat(jans-cedarling): add retry mechanism for KeyService HTTP requests Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> chore(jans-cedarling): add missing license header Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> chore(jans-cedarling): resolve clippy issues Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> chore(jans-cedarling): update example for authorize_with_jwt_validation.rs Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com> refactor(jans-cedarling): remove `exp` and `nbf` requirement for userinfo_token Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
@@ -0,0 +1,5 @@ | |||
{ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
c324a97
jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service.rs
Outdated
Show resolved
Hide resolved
jans-cedarling/cedarling/src/jwt/test/with_validation/key_service.rs
Outdated
Show resolved
Hide resolved
jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service.rs
Outdated
Show resolved
Hide resolved
…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 <34529290+rmarinn@users.noreply.github.com>
…ter code reusability Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
- Make gernerate_token_using_claims return a Result instead of panicking for improved error management in tests. Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
…on.rs Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
…info_token Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
- 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 <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
…full path Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
- return struct instead of tuple for generate_token_using_claims Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
- the user should create their own tokens.json Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
af8335c
to
6d4a38a
Compare
..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 |
There was a problem hiding this comment.
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
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
Test and Document the changes
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.