jans icon indicating copy to clipboard operation
jans copied to clipboard

feat(jans-lock): Acquire config from JS, Improved codebase structure

Open sokorototo opened this issue 1 year ago • 12 comments

Prepare


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)

\nPlease 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.\n- [x] I confirm that there is no impact on the docs due to the code changes in this PR.

sokorototo avatar Jun 28 '24 20:06 sokorototo

Hi there :wave:, @dryrunsecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer :white_check_mark: 0 findings
Configured Codepaths Analyzer :white_check_mark: 0 findings
Secrets Analyzer :white_check_mark: 0 findings
Authn/Authz Analyzer :white_check_mark: 0 findings
SQL Injection Analyzer :white_check_mark: 0 findings
Sensitive Files Analyzer :white_check_mark: 0 findings
IDOR Analyzer :white_check_mark: 0 findings

[!Note] :green_circle: Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy :robot:. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request span across multiple files in the jans-lock/cedarling project, primarily focusing on updates to the application's security-related functionality, such as JSON Web Token (JWT) validation, policy store management, and dynamic client registration for OAuth 2.0.

The key security-relevant changes include:

  1. JWT Validation: The code in the crypto/decode.rs and crypto/types.rs files has been updated to improve the handling and validation of JWTs, including the use of a trust store to look up the appropriate JSON Web Keys (JWKs) for verifying the JWT signatures.

  2. Policy Store Management: The changes in the startup/mod.rs and startup/types.rs files introduce new configuration options for the policy store, including support for local, remote, and "lock-master" configurations. The security of the policy store is crucial, as it contains the application's authorization policies.

  3. Dynamic Client Registration: The code in the lock_master/mod.rs file demonstrates the implementation of dynamic client registration for OAuth 2.0, which is a security-conscious approach to avoid hardcoding client credentials.

  4. Token Handling: The application's handling of access tokens, ID tokens, and other sensitive tokens has been reviewed and updated to ensure proper validation, storage, and usage.

  5. Input Validation: The code changes highlight the importance of properly validating and sanitizing user input, especially in the context of deserialization of untrusted data, to prevent potential security vulnerabilities.

Overall, the changes in this pull request appear to be focused on improving the security posture of the Cedarling application, particularly in the areas of JWT validation, policy store management, and OAuth 2.0 client registration. As an application security engineer, I would recommend thoroughly reviewing the implementation of these security-critical components to ensure that they are designed and implemented securely.

Files Changed:

  1. jans-lock/cedarling/Cargo.toml: Added a new dependency on the jsonwebtoken crate, which is likely used for handling JSON Web Tokens (JWT) in the application.
  2. jans-lock/cedarling/.gitignore: Added a new line to exclude the /out directory from being tracked by Git.
  3. jans-lock/cedarling/Cargo.lock: Updated the dependencies, including the addition of the jsonwebtoken crate.
  4. jans-lock/cedarling/README.md: Updated the documentation to provide information about the policy store strategies and other security-relevant features of the Cedarling application.
  5. jans-lock/cedarling/src/authz/token2entity.rs: Added a comment describing the purpose of this file, which is to map tokens to entities on the fly for the authorization module.
  6. jans-lock/cedarling/src/authz/mod.rs: Removed the Tokens struct and introduced new modules (token2entity and types) related to authorization functionality.
  7. jans-lock/cedarling/src/authz/types.rs: Defined new structs for deserialization of various token types, such as access tokens, transaction tokens, and ID tokens.
  8. jans-lock/cedarling/src/crypto/decode.rs: Implemented the logic for validating JSON Web Tokens (JWT) using a trust store.
  9. jans-lock/cedarling/src/crypto/mod.rs: Initialized the trust store for verifying JWT signatures from trusted issuers.
  10. jans-lock/cedarling/src/crypto/types.rs: Defined a new TrustedIssuer struct to represent a trusted identity provider.
  11. jans-lock/cedarling/src/http.rs: Made minor changes to the post function, which is responsible for making HTTP POST requests.
  12. jans-lock/cedarling/src/lib.rs: Refactored the startup sequence, including the initialization of the authorization module.
  13. jans-lock/cedarling/src/lock_master/sse.rs: Implemented the handling of Server-Sent Events (SSE) updates from the lock master service.
  14. jans-lock/cedarling/src/lock_master/mod.rs: Initialized the lock master component, including the dynamic client registration and access token acquisition.
  15. `jans-lock

Powered by DryRun Security

dryrunsecurity[bot] avatar Jun 28 '24 20:06 dryrunsecurity[bot]

@kdhttps while I do understand the importance of Clean Architecture and writing sufficient tests, can we get something that works first then we can start the polish work? I'll definitely do it, I just don't want to get bogged down in structural work right now

sokorototo avatar Jul 01 '24 11:07 sokorototo

@SafinWasi https://lock.master.gluu.cloud doesn't seem to point to anything, so I can't get the relevant OpenId config, and thus can't get the supported scopes. I might resolve to reading through the source code, but I'm not sure if that's a forward-looking solution.

On the matter of writing the README, I'm restoring my Typora license then I'll get to it.

sokorototo avatar Jul 01 '24 11:07 sokorototo

@kdhttps while I do understand the importance of Clean Architecture and writing sufficient tests, can we get something that works first then we can start the polish work? I'll definitely do it, I just don't want to get bogged down in structural work right now

I'm sorry, but I must respectfully disagree with you. At least we should write unit test cases for each function.

kdhttps avatar Jul 01 '24 12:07 kdhttps

@kdhttps, can we agree then that the tests are not final and are written to fail.

sokorototo avatar Jul 01 '24 13:07 sokorototo

@kdhttps, can we agree then that the tests are not final and are written to fail.

agreed! it is ok if tests fail but we will get an idea of the module what it's accepting, and rejecting, and what the final result should look like.

kdhttps avatar Jul 01 '24 13:07 kdhttps

@sokorototo you can check out the cedar-wasm code, it is a good example of TDD. It has small functions, accepting everything in parameters and respecting true/false test cases. The goal is code should look and execute health(bug free) and it is only possible by adding test cases.

kdhttps avatar Jul 01 '24 13:07 kdhttps

Looks good to me. @kdhttps can you check functionality and JS bindings?

@sokorototo I am getting this on my HTML page.

image

My JS Code looks like:

<script type="module">
      import init from "./out/cedarling.js";

      const config = {
        policyStore: {
          // can be "local", "remote" or "lock-master",
          // each strategy requires different parameters, see below
          strategy: "remote",
          url: "https://raw.githubusercontent.com/JanssenProject/jans/main/jans-lock/cedarling/policy-store/default.json",
        },
        // if policy-store.json is compressed using deflate-zlib
        decompressPolicyStore: false,
        // [OPTIONAL] How often, in milliseconds, will the cedarling refresh it's TrustStore. The cedarling won't refresh it's trust store if not omitted
        trustStoreRefreshRate: 2000,
        // Set of jwt algorithms that the cedarling will allow, empty if omitted
        supportedAlgorithms: ["HS256", "HS384", "RS256"],
      };

      async function run() {
        await init(config);
      }

      run();
    </script>

kdhttps avatar Jul 03 '24 17:07 kdhttps

@kdhttps to build for the Web, you use wasm-bindgen --out-dir out ./target/wasm32-unknown-unknown/release/cedarling.wasm --target web. I was testing with node, so I forgot to include this, let me patch it in. Building for the Web is somewhat complicated, let me add further instructions.

sokorototo avatar Jul 03 '24 18:07 sokorototo

ok, thanks but now getting this @sokorototo .

image

kdhttps avatar Jul 04 '24 09:07 kdhttps

@kdhttps can you check if your dev server is setup correctly? Is the wasm binary being loaded in the Network Section of the dev tools? Since everything seems to load on my side

sokorototo avatar Jul 04 '24 21:07 sokorototo

I investigate the cedarling.js. It is not invoking the wasm binary load function. so I manually added it to my HTML page. Not sure if this way is right or wrong. My HTML JS code is:

    <script type="module">
      import __wbg_init, { init } from "./out/cedarling.js";

      const config = {
        policyStore: {
          strategy: "remote",
          url: "https://raw.githubusercontent.com/kdhttps/cedar-policies/main/example-policy-store.json",
        },
        decompressPolicyStore: false,
        trustStoreRefreshRate: 2000,
        supportedAlgorithms: ["HS256", "HS384", "RS256"],
        policyStoreId: "gluu_store",
      };

      async function run() {
        await __wbg_init();
        await init(config);
      }

      run();
    </script>

await __wbg_init(); after manually invoking the wasm binary I can now see it in the network tab. and now looks like it is actually calling the wasm init function but getting an error:

image

kdhttps avatar Jul 05 '24 08:07 kdhttps

DryRun Security Summary

The pull request includes various code changes to the Jans Cedarling application, focusing on JWT handling, policy store management, authorization, and HTTP request handling, with a focus on ensuring secure implementation and mitigating potential security vulnerabilities.

Expand for full summary

Summary:

The code changes in this pull request are focused on various aspects of the Jans Cedarling application, including updates to the .gitignore file, the Cargo.toml file, the README.md, and several source code files. The changes cover a wide range of functionality, such as JWT handling, policy store management, authorization, and HTTP request handling.

From an application security perspective, the key areas to focus on are:

  1. JWT Handling: The addition of the jsonwebtoken dependency and the changes in the crypto module suggest that the application is handling JSON Web Tokens (JWTs) for authentication and authorization purposes. It is crucial to ensure that the JWT handling is implemented securely, with proper validation, verification, and protection against common JWT-related vulnerabilities.

  2. Policy Store Management: The changes in the startup and lock_master modules indicate that the application is using different policy store configurations (local, remote, and lock-master). Proper validation and secure handling of the policy store data are essential to prevent unauthorized access or tampering.

  3. Authorization and Access Control: The changes in the authz module suggest that the application has some form of authorization and access control mechanisms. These mechanisms should be thoroughly reviewed to ensure that they are implemented securely and follow the principles of least privilege and defense-in-depth.

  4. Input Validation and Sanitization: The application is deserializing various data structures, including those containing sensitive information (e.g., tokens, configuration parameters). Proper input validation and sanitization should be implemented to mitigate potential vulnerabilities, such as injection attacks or deserialization issues.

  5. Secure Configuration Management: The application's configuration, including sensitive values like URIs, client credentials, and supported cryptographic algorithms, should be managed securely to prevent unauthorized access or modifications.

Overall, the changes in this pull request introduce several security-relevant components and functionality. As an application security engineer, it is crucial to review the implementation details, ensure that security best practices are followed, and verify that the application is not introducing any new vulnerabilities or security risks.

Files Changed:

  1. .gitignore: The changes add a new line to ignore the "/out" directory, which is a common practice to avoid accidentally committing generated artifacts.
  2. Cargo.toml: The changes update dependencies, including the addition of the jsonwebtoken crate, which is used for handling JSON Web Tokens (JWTs).
  3. README.md: The changes provide more detailed information about the Cedarling application, including configuration options, trust store management, and integration with the Jans Lock Master.
  4. src/authz/mod.rs: The changes refactor the authz module, including the removal of the Tokens struct and the introduction of the AuthzInput struct for handling authorization-related input data.
  5. src/authz/types.rs: The changes introduce new data structures for handling various types of tokens and input data related to authorization, which should be reviewed for proper input validation and secure handling of sensitive information.
  6. src/crypto/decode.rs: The changes focus on the implementation of JWT validation, including the extraction of the JSON Web Key (JWK) and the creation of the DecodingKey and Validation objects.
  7. src/crypto/mod.rs: The changes introduce the SUPPORTED_ALGORITHMS static variable and the TrustedIssuer data structure, which are used for verifying the authenticity of JWT tokens.
  8. src/crypto/types.rs: The changes add the TrustedIssuer data structure, which is used to represent trusted OpenID Connect (OIDC) identity providers.
  9. src/http.rs: The changes update the HTTP request method and provide options for handling different types of request bodies, which should be reviewed for proper input validation and sanitization.
  10. src/lock_master/mod.rs: The changes focus on the implementation of the dynamic client registration and token acquisition functionality, which should be reviewed for secure handling of sensitive information, such as client credentials.
  11. src/lib.rs: The changes are related to the initialization and startup of the Cedarling application, including the handling of unexpected panics and the initialization of the authz module.
  12. src/startup/mod.rs: The changes simplify the policy store initialization process and remove the handling of trusted issuers, which may have security implications that should be reviewed.
  13. src/lock_master/sse.rs: The changes introduce the

Code Analysis

We ran 7 analyzers against 22 files and 0 analyzers had findings. 7 analyzers had no findings.

Riskiness

:green_circle: Risk threshold not exceeded.

View PR in the DryRun Dashboard.

dryrunsecurity[bot] avatar Jul 09 '24 12:07 dryrunsecurity[bot]