-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat(jans-cedarling): update PolicyStore parser to support agama-lab generated policies #10098
Conversation
Signed-off-by: rmarinn <[email protected]>
- implement a `ClaimMapping` struct for the new `claim_mapping` field in the policy store - implement deserialize for the `ClaimMapping` struct Signed-off-by: rmarinn <[email protected]>
- implement `TokenEntityMetada` struct - implement `Deserialize` for `TokenEntityMetadata` Signed-off-by: rmarinn <[email protected]>
- implement new struct TrustedIssuerMetadata - implement Deserialize for TrustedIssuerMetadata Signed-off-by: rmarinn <[email protected]>
- Implement AgamaPolicyStore struct. - Implement Deserialize for AgamaPolicyStore struct. Signed-off-by: rmarinn <[email protected]>
- change the type of AgamaPolicyStore.cedar_schema from cedar_policy::Schema to CedarSchema to make it compatibale with the existsing implementation Signed-off-by: rmarinn <[email protected]>
- update the token_metadata implementation in the Cerdarling PolicyStore to support the new schema. Signed-off-by: rmarinn <[email protected]>
- remove old implementation for IdentitySource struct and related implementations. The new implementation, TrustedIssuerMetadata, has now been implementd with the main policy store. Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
DryRun Security SummaryThe provided code changes focus on improving the security of the Cedarling application, with updates to the policy store management, JWT token handling, and authorization enforcement, as well as the introduction of robust error handling, input validation, and comprehensive test cases. Expand for full summarySummary: The provided code changes cover a wide range of updates and improvements to the Cedarling application, with a strong focus on application security. The changes span various components, including the policy store management, JWT token handling, and authorization enforcement. Key security-related aspects of the changes include:
Overall, the changes demonstrate a security-conscious approach to the application's development, with a focus on ensuring the integrity, reliability, and robustness of the security-critical components. However, it's important to continue reviewing the entire codebase and deployment configuration to identify any potential security vulnerabilities or areas for improvement. Files Changed:
Code AnalysisWe ran Riskiness🟢 Risk threshold not exceeded. |
please don't approve merge yet, this is still a work in progress. i just made this so i could close the other PR. |
…ption Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
63bea68
to
194e776
Compare
Signed-off-by: rmarinn <[email protected]>
- Simplify YAML test files by removing the need for a top-level `policy_store` ID - Ensure YAML test files exclusively contain human-readable Cedar code; base64-encoded schemas are now only used for JSON test files. - Pending: Replace the existing implementation with the new parser. Signed-off-by: rmarinn <[email protected]>
…p instead of None Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- split the parsing for TokenEntityMetadata into separate functions per field for easier unit-testing Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- Refactor deserialization logic to utilize existing helper functions. Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- rename AgamaPolicyStore to PolicySource - move PolicySouceJson into it's own file - move PolicySouceYaml into it's own file Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
…entation Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- moved a test file to the /test_files directory Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
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.
Please see inline comments.
``` json | ||
"policy_content": "cGVybWl0KAogICAgc..." | ||
``` | ||
- **policy_content** : (*String*) The Cedar Policy Encoded in Base64. |
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.
The policy content is mentioned as string. But at L55 it is shown as Json.
"principal_identifier": "some_user123", | ||
"role_mapping": "role", | ||
"token_type": { | ||
"user_id": "<field name in token (e.g., 'email', 'sub', 'uid', etc.) or '' if not used>", |
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.
Let's use the claim name
instead of the field name to show the exactness with the OIDC standard.
ref: https://openid.net/specs/openid-connect-basic-1_0.html#IDToken
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.
i was just following the wiki: Token Entity Metadata Schema... maybe a discussion should be made first for this one? I thought we should be just following the wiki because discussions have been made for that already.
- **token_type:** The type of token being processed, such as `access_tokens`, `id_tokens`, `userinfo_tokens`, and `tx_tokens`. | ||
- **user_id (_Optional_):** The field in the token used to identify the user. If not needed, set to an empty string (`""`). | ||
- **role_mapping (_Optional_):** Indicates which field in the token should be used for role-based access control. If not needed, set to an empty string (`""`). | ||
- **claim_mapping:** Defines how to extract and transform specific claims from the token. Each claim can have its own parser (`regex` or `json`) and type (`Acme::Email`, `Acme::URI`, etc.). |
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.
I believe it would be easier to understand if the regex parser in claim_mapping is explained with an example—showing how Cedarling would use the regex parser to populate the claim value from the token to the entity. Otherwise, the point remains unclear. Additionally, the json parser could also be explained with an example.
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.
I was only following this: https://github.com/JanssenProject/jans/wiki/Cedarling-Nativity-Plan. I honestly don't know how to use this as well so i couldn't provide an example on the behavior that Cedarling will do after it parsed the config.
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.
Firstly, I'm strongly opposed to having separate code paths for yaml and json parsing. A large part of the justification for introducing yaml was to maintain exactly the same code paths as json, so that yaml test fixtures will exercise the same code that the json text fixtures were previously exercising.
As far as I know, the Agama Lab Policy Designer team has no plans to provide output in yaml?
Secondly, the purpose of having the alternative String | Object
structure as the value for schema
and content_policy
was to allow for human-readability in test fixtures, while simultaneously providing the Agama Lab Policy Designer team with a backwards-compatible schema, that would also allow them to start using human-readable values when and if that works for them.
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.
Actually I agree with @djellemah.
@rmarinn you made a lot of work, but it was not discussed and as a result it needs redo.
Using different parsing code for YAML and JSON will lead to errors.
.ok_or_else(|| de::Error::missing_field("parser"))?; | ||
|
||
match parser { | ||
"regex" => { |
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 we can't just create struct smt like this and deserialize?
Definitely with better naming..
struct SomeName{
#[serde(rename(deserialize = "type"))]
cedar_type: String,
regex_expression: String,
#[serde(flatten)]
fields: HashMap<String, RegexField>,
}
pub struct PolicyStoreDataJson { | ||
/// Optional name of the policy store. | ||
#[serde(deserialize_with = "parse_option_string", default)] | ||
pub name: Option<String>, |
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.
This value is mandatory. Because it defines namespace of cedar-policy schema
pub struct PolicyStoreJson { | ||
/// Optional Cedar version information. | ||
#[serde(deserialize_with = "parse_maybe_cedar_version")] | ||
cedar_version: Option<Version>, |
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.
cedar_version
is defined in https://github.com/JanssenProject/jans/wiki/Cedarling-Nativity-Plan#cedarling-policy-store so it is mandatory
@@ -184,7 +184,7 @@ impl<'de> serde::Deserialize<'de> for CedarSchema { | |||
mod deserialize { | |||
#[derive(Debug, thiserror::Error)] | |||
pub enum ParseCedarSchemaSetMessage { | |||
#[error("unable to decode cedar policy schema base64")] | |||
#[error("Failed to decode Base64 encoded string")] |
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.
it would be great to extend
#[error("Failed to decode Base64 encoded string")] | |
#[error("Failed to decode Base64 encoded string cedar policy schema")] |
.decode(value) | ||
.map_err(|e| de::Error::custom(format!("Failed to decode Base64 encoded string: {}", e)))?; | ||
String::from_utf8(buf) | ||
.map_err(|e| de::Error::custom(format!("Failed to decode Base64 encoded string: {}", e))) |
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.
in this case we dont decode base64 but cast base64 decoded string to utf8
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.
We need to merge this and fix later
Signed-off-by: Oleh Bohzok <[email protected]>
I just made a new PR instead of trying to revert this to fix the issues. Please see: #10141. |
Prepare
Description
This PR enhances the
PolicyStore
struct to support policy stores generated by the Agama Lab Policy Designer, enabling seamless loading of policies in both JSON and YAML formats.New Features:
PolicyStore::load_from_json(json_str)
to load policy stores from JSON.PolicyStore::load_from_yaml(yaml_str)
to load policy stores from YAML.Target issue
The issue addressed by this PR involves users being unable to directly use policy stores exported from Agama Lab's Policy Designer. This enhancement enables seamless integration of Agama-generated policies (see Updated JSON Policy Store Schema).
closes #10038
Implementation Details
This update introduces a revised schema for the
PolicyStore
, including updates totrusted_issuers
andTokenEntityMetadata
. The following sections provide detailed information on the changes.Updated JSON Policy Store Schema
The JSON structure has been updated to include additional fields and configurations:
Updated
trusted_issuers
schemaUpdated Token Entity Metadata schema (used for:
access_tokens
,id_tokens
,usrinfo_tokens
, andtx_tokens
).Updated YAML Policy Store Schema
For easier readability and authoring, the YAML format has been simplified:
Updated Rust Implementation
The
PolicyStore
struct and related methods have been refactored to support the new schema:Testing and Validation
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.