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

fix(cedarling): Make cedarling trusted store compatible with agama lab #10039

Closed
wants to merge 14 commits into from

Conversation

olehbozhok
Copy link
Contributor

Prepare


Description

fix loading cedarling trusted issuer from agama lab

Target issue

closes #10038

Implementation Details


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.

@olehbozhok olehbozhok self-assigned this Nov 5, 2024
@olehbozhok olehbozhok requested review from duttarnab, rmarinn and djellemah and removed request for duttarnab and rmarinn November 5, 2024 01:31
Copy link

dryrunsecurity bot commented Nov 5, 2024

DryRun Security Summary

The pull request focuses on improving the handling and management of policy stores, including support for multiple policy stores, enhanced policy content parsing, and integration with identity sources, which are crucial for the application's security and functionality.

Expand for full summary

Summary:

The code changes in this pull request are focused on improving the handling and management of policy stores, which are a critical component for the application's security and functionality. The key changes include:

  1. Policy Store Handling: The policy_store.rs file has been updated to support multiple policy stores, with each store having additional metadata such as name, description, Cedar policy version, and trusted identity sources.
  2. Policy Content Parsing: The code has been enhanced to handle the deserialization of base64-encoded policy content, ensuring that the policy information can be properly parsed and validated.
  3. Identity Source Integration: The service_config.rs file has been updated to fetch the OpenID configuration for the identity sources specified in the policy stores, allowing the application to properly validate the JWT tokens used for authentication and authorization.

From an application security perspective, these changes are important as they improve the flexibility, robustness, and security of the policy store management. Proper handling of policy stores and identity sources is crucial for ensuring the integrity and reliability of the application's authorization and access control mechanisms.

However, it's important to ensure that the parsing and validation of the policy content and the integration with external identity sources are implemented securely to prevent potential vulnerabilities, such as improper input handling or insecure token validation. The code comments and TODO items suggest that there might be further work needed to fully address these security considerations.

Files Changed:

  1. jans-cedarling/cedarling/src/init/policy_store.rs: This file has been updated to handle multiple policy stores, with improved error handling and policy content parsing.
  2. jans-cedarling/cedarling/src/common/cedar_schema.rs: The changes in this file are focused on improving the deserialization of the policy store data, with a comprehensive test suite to ensure the robustness of the process.
  3. docs/cedarling/cedarling-policy-store.md: The documentation for the Cedarling Policy Store has been updated to reflect the changes in the schema and the new features, such as the handling of identity sources and token claim mapping.
  4. jans-cedarling/cedarling/src/common/policy_store.rs: This file has been updated to include additional fields in the PolicyStore struct, providing more configuration options and information about the policy stores.
  5. jans-cedarling/cedarling/src/init/service_config.rs: The changes in this file focus on the handling of identity sources and their associated OpenID configurations, which is an important security consideration for the application.

Code Analysis

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

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

@olehbozhok olehbozhok requested a review from rmarinn November 5, 2024 01:31
@mo-auto mo-auto added comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality labels Nov 5, 2024
@olehbozhok olehbozhok requested a review from SafinWasi November 5, 2024 01:33
rmarinn
rmarinn previously approved these changes Nov 5, 2024
Comment on lines 51 to 56
let Ok(result_map) =
serde_json::from_str::<HashMap<String, PolicyStore>>(policies_json)
else {
// return previous error to be unit test compatible
return Err(err);
};

Choose a reason for hiding this comment

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

in Rust, it's better to use match then if else

let result_map = match serde_json::from_str::<HashMap<String, PolicyStore>>(policies_json) {
    Ok(map) => map,
    Err(_) => return Err(err), // Return the original error here
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we ignore error it is OK to use

let OK(_) = some else{
...
}

https://rust-lang.github.io/rfcs/3137-let-else.html#guide-level-explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Err(err)=> is just more clear then else to identify the error part

jans-cedarling/cedarling/src/init/policy_store.rs Outdated Show resolved Hide resolved
"policy_content": "cGVybWl0KAogICAgcHJpbmNpcGFsIGlzIEphbnM6OlVzZXIsCiAgICBhY3Rpb24gaW4gW0phbnM6OkFjdGlvbjo6IlVwZGF0ZSJdLAogICAgcmVzb3VyY2UgaXMgSmFuczo6SXNzdWUKKXdoZW57CiAgICBwcmluY2lwYWwuY291bnRyeSA9PSByZXNvdXJjZS5jb3VudHJ5Cn07"
}
},
"schema": "eyJKYW5zIjp7ImNvbW1vblR5cGVzIjp7IlVybCI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJob3N0Ijp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifSwicGF0aCI6eyJ0eXBlIjoiRW50aXR5T3JDb21tb24iLCJuYW1lIjoiU3RyaW5nIn0sInByb3RvY29sIjp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifX19fSwiZW50aXR5VHlwZXMiOnsiVHJ1c3RlZElzc3VlciI6eyJzaGFwZSI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJpc3N1ZXJfZW50aXR5X2lkIjp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJVcmwifX19fSwiV29ya2xvYWQiOnsic2hhcGUiOnsidHlwZSI6IlJlY29yZCIsImF0dHJpYnV0ZXMiOnsiY2xpZW50X2lkIjp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifSwiaXNzIjp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJUcnVzdGVkSXNzdWVyIn0sIm5hbWUiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJvcmdfaWQiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9fX19LCJVc2VyIjp7Im1lbWJlck9mVHlwZXMiOlsiUm9sZSJdLCJzaGFwZSI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJjb3VudHJ5Ijp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifSwiZW1haWwiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJzdWIiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJ1c2VybmFtZSI6eyJ0eXBlIjoiRW50aXR5T3JDb21tb24iLCJuYW1lIjoiU3RyaW5nIn19fX0sIkFjY2Vzc190b2tlbiI6eyJzaGFwZSI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJhdWQiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJleHAiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IkxvbmcifSwiaWF0Ijp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJMb25nIn0sImlzcyI6eyJ0eXBlIjoiRW50aXR5T3JDb21tb24iLCJuYW1lIjoiVHJ1c3RlZElzc3VlciJ9LCJqdGkiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9fX19LCJpZF90b2tlbiI6eyJzaGFwZSI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJhY3IiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJhbXIiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJhdWQiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJleHAiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IkxvbmcifSwiaWF0Ijp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJMb25nIn0sImlzcyI6eyJ0eXBlIjoiRW50aXR5T3JDb21tb24iLCJuYW1lIjoiVHJ1c3RlZElzc3VlciJ9LCJqdGkiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9LCJzdWIiOnsidHlwZSI6IkVudGl0eU9yQ29tbW9uIiwibmFtZSI6IlN0cmluZyJ9fX19LCJJc3N1ZSI6eyJzaGFwZSI6eyJ0eXBlIjoiUmVjb3JkIiwiYXR0cmlidXRlcyI6eyJjb3VudHJ5Ijp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifSwib3JnX2lkIjp7InR5cGUiOiJFbnRpdHlPckNvbW1vbiIsIm5hbWUiOiJTdHJpbmcifX19fSwiUm9sZSI6e319LCJhY3Rpb25zIjp7IlVwZGF0ZSI6eyJhcHBsaWVzVG8iOnsicmVzb3VyY2VUeXBlcyI6WyJJc3N1ZSJdLCJwcmluY2lwYWxUeXBlcyI6WyJXb3JrbG9hZCIsIlVzZXIiLCJSb2xlIl19fX19fQ==",

Choose a reason for hiding this comment

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

It could be more human readable to put the decoded version as comment

"840da5d85403f35ea76519ed1a18a33989f855bf1cf8": {
"description": "simple policy example for principal workload",
"creation_date": "2024-09-20T17:22:39.996050",
"policy_content": "cGVybWl0KAogICAgcHJpbmNpcGFsIGlzIEphbnM6Oldvcmtsb2FkLAogICAgYWN0aW9uIGluIFtKYW5zOjpBY3Rpb246OiJVcGRhdGUiXSwKICAgIHJlc291cmNlIGlzIEphbnM6Oklzc3VlCil3aGVuewogICAgcHJpbmNpcGFsLm9yZ19pZCA9PSByZXNvdXJjZS5vcmdfaWQKfTs="

Choose a reason for hiding this comment

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

It could be more human readable to put the decoded version as comment

@djellemah djellemah force-pushed the jans-cedaling-issue-10038 branch from 2730057 to d6a7c32 Compare November 6, 2024 18:11
@olehbozhok
Copy link
Contributor Author

Looks like implemented in the #10036

@olehbozhok olehbozhok marked this pull request as draft November 7, 2024 00:36
@@ -134,8 +134,8 @@ Kubernetes: `>=v1.22.0-0`
| cnCouchbasePasswordFile | string | `"/etc/jans/conf/couchbase_password"` | Path to Couchbase password file |
| cnCouchbaseSuperuserPasswordFile | string | `"/etc/jans/conf/couchbase_superuser_password"` | Path to Couchbase superuser password file |
| cnDocumentStoreType | string | `"DB"` | Document store type to use for shibboleth files DB. |
| cnGoogleApplicationCredentials | string | `"/etc/jans/conf/google-credentials.json"` | Base64 encoded service account. The sa must have roles/secretmanager.admin to use Google secrets and roles/spanner.databaseUser to use Spanner. Leave as this is a sensible default. |
| cnPersistenceType | string | `"sql"` | Persistence backend to run Janssen with couchbase|hybrid|sql|spanner. |
| cnGoogleApplicationCredentials | string | `"/etc/jans/conf/google-credentials.json"` | Base64 encoded service account. The sa must have roles/secretmanager.admin to use Google secrets. Leave as this is a sensible default. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is you change in charts/janssen-all-in-one/README.md ?
I guess not, probably exit the way to remove all changes in MR for this file..

"identity_source": { ... }
}
}
"name": "Jans::Store",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name only Jans and it is defined the agama-lab
and looks like you need to revert this change...

"name": "Jans::store",
"description": "a simple policy example",
"policies": {
"fbd921a51b...": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, left only one policy. Because cedarling support only one policy in store per file. And in agama-lab will do the same

@rmarinn
Copy link
Contributor

rmarinn commented Nov 10, 2024

Closing this PR in favor of a newer one that addresses the same issue with an improved approach and cleaner git history. Please see: #10098

@rmarinn rmarinn closed this Nov 10, 2024
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-bug Issue or PR is a bug in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(cedarling): Make cedarling Policy Store compatible with Agama Lab
5 participants