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

refactor(jans-cedarling): consolidate JwtService initialization into the module #9980

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Oct 30, 2024

Prepare


Description

This PR refactors the initialization process for the jwt module to enhance modularity and reduce the likelihood of merge conflicts when modifications occur in the init or authz modules.

Target issue

The current initialization logic for JwtService is cumbersome and not encapsulated within its module, leading to a tangled initialization process. This lack of modularity complicates maintenance and fosters tight coupling between multiple modules. Any changes to the JwtService constructor can cause cascading modifications across the init and common modules, resulting in unintended dependencies and increased complexity.

closes #9945

Implementation Details

  • Consolidated JwtService initialization logic into its dedicated module to enhance maintainability and clarity.
  • Improved error handling during initialization, ensuring that error messages are descriptive and easy to trace.
  • Defined separate error types for each sub-module within the jwt module, facilitating better error categorization and debugging.
  • Enhanced documentation for error variants to provide clearer guidance on potential issues and their causes.

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.

…nhance error handling

- consolidate `JwtService` initialization logic into the module to enhance maintainability and clarity.
- improve error handling during initialization, ensuring that errors are more descriptive and easier to trace.
- define separate error types for each sub-module within the `jwt` module, allowing for better error categorization and easier debugging.
- enhance documentation for error variants to provide clearer guidance on potential issues and their causes.

Signed-off-by: rmarinn <[email protected]>
@rmarinn rmarinn self-assigned this Oct 30, 2024
@rmarinn rmarinn added the comp-jans-cedarling Touching folder /jans-cedarling label Oct 30, 2024
Copy link

DryRun Security Summary

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

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 within the cedarling project. The changes span multiple files and address various aspects of JWT initialization, decoding, and validation.

Key security-related changes include:

  1. Introducing a DecodingStrategy with "Disabled" and "Enabled" configurations, ensuring that JWT validation is only performed when explicitly enabled.
  2. Validating the presence of trusted issuers in the policy store before using them for JWT validation, preventing the use of untrusted issuers.
  3. Improving error handling throughout the JWT-related code, providing more detailed and informative error messages to aid in debugging and security monitoring.
  4. Enhancing the JWT decoding and validation process, including additional checks to ensure the consistency of token attributes (e.g., matching audience and issuer between access and ID tokens).
  5. Simplifying the ServiceConfig struct by removing JWT-specific fields, potentially reducing the attack surface and complexity of the application's configuration.

Overall, these changes demonstrate a strong focus on improving the security and robustness of the JWT handling functionality within the cedarling project. The attention to secure configuration, validation of trusted entities, and comprehensive error handling are commendable from an application security perspective.

Files Changed:

  1. jans-cedarling/cedarling/src/init/mod.rs: The changes remove the jwt_algorithm module and add new modules related to policy store, service configuration, and service factory. The security implications depend on how these new modules are implemented and integrated with the JWT validation process.
  2. jans-cedarling/cedarling/src/init/service_factory.rs: The changes focus on the secure initialization and configuration of the JWT service, including handling "Disabled" and "Enabled" configurations, validating trusted issuers, and planned improvements in error handling.
  3. jans-cedarling/cedarling/src/init/service_config.rs: The changes simplify the ServiceConfig struct by removing JWT-specific fields, potentially reducing the complexity and attack surface of the application's configuration.
  4. jans-cedarling/cedarling/src/jwt/decoding_strategy.rs: The changes introduce a DecodingStrategy with validation capabilities, ensuring that only valid and trusted JWTs are accepted by the application.
  5. jans-cedarling/cedarling/src/jwt/jwt_service_config.rs: The changes suggest a more flexible representation of supported algorithms, which should be reviewed to ensure proper input validation and sanitization.
  6. jans-cedarling/cedarling/src/jwt/decoding_strategy/key_service.rs: The changes demonstrate a well-designed and secure implementation of a key management service for verifying JWTs, including features like key caching, refresh, and comprehensive error handling.
  7. jans-cedarling/cedarling/src/jwt/mod.rs: The changes focus on improving the error handling and initialization process of the JwtService, enhancing the overall reliability and security of the JWT handling functionality.

Code Analysis

We ran 9 analyzers against 12 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 kind-enhancement Issue or PR is an enhancement to an existing functionality label Oct 30, 2024
@abaghinyan abaghinyan self-requested a review October 30, 2024 21:28
crate::JwtConfig::Enabled { .. } => JwtServiceConfig::WithValidation {
supported_algs: self.service_config.jwt_algorithms.clone(),
crate::JwtConfig::Enabled { signature_algorithms } => JwtServiceConfig::WithValidation {
supported_algs: signature_algorithms.to_vec(),
trusted_idps: self.policy_store().trusted_issuers.expect("Expected trusted issuers to be present for JWT validation, but found None. Ensure that the policy store is properly initialized with trusted issuers before using JWT validation.").clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not panic!. We need to get value from service_config


// TODO: handle intialization errors
let jwt_service = Arc::new(
JwtService::new_with_config(config).expect("should initialize jwt service"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not panic!

Copy link
Contributor Author

@rmarinn rmarinn Nov 1, 2024

Choose a reason for hiding this comment

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

i know we shouldn't panic but look at the code calling it...

    // get jwt service
    pub fn jwt_service(&mut self) -> Arc<JwtService> {
        if let Some(jwt_service) = &self.container.jwt_service {
            jwt_service.clone()
        } else {
            let config = match &self.bootstrap_config.jwt_config {
                crate::JwtConfig::Disabled => JwtServiceConfig::WithoutValidation,
                crate::JwtConfig::Enabled { signature_algorithms } => JwtServiceConfig::WithValidation {
                    supported_algs: signature_algorithms.to_vec(),
                    trusted_idps: self.policy_store().trusted_issuers.expect("Expected trusted issuers to be present for JWT validation, but found None. Ensure that the policy store is properly initialized with trusted issuers before using JWT validation.").clone(),
                },
            };
            
            // TODO: handle intialization errors 
            let jwt_service = Arc::new(
                JwtService::new_with_config(config).expect("should initialize jwt service"),
            );
            self.container.jwt_service = Some(jwt_service.clone());
            jwt_service
        }
    }

ServiceFactory::jwt_service doesn't return a Result when initializing...

@@ -169,7 +199,7 @@ fn decode_and_validate_jwt<T: DeserializeOwned>(
///
/// # Errors
/// Returns an error if the algorithm is not implemented.
pub fn string_to_alg(algorithm: &str) -> Result<jwt::Algorithm, Error> {
pub fn string_to_alg(algorithm: &str) -> Result<jwt::Algorithm, Box<str>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we return error as string?

trusted_idps: Vec<TrustedIssuer>,
) -> Result<Self, Error> {
) -> Result<Self, InitError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should get already validated value and do not return any errors

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-enhancement Issue or PR is an enhancement to an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(jans-cedarling): consolidate JwtService initialization logic into the module
4 participants