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): make all tokens optional when calling Cedarling::authorize #10436

Merged
merged 46 commits into from
Dec 24, 2024

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Dec 17, 2024

Prepare


Description

This PR implements making tokens optional, making it more flexible for users who doesn't have all the tokens.

Target issue

target issue: #10408

closes #10408

Implementation Details

  • Implement making tokens optional. Cedarling should create the entities to the best of its ability based on the token(s) it has.
  • implement check if if CEDARLING_USER_AUTHZ is enabled; if it is, then the user entity is required
  • implement check if CEDARLING_WORKLOAD_AUTHZ enabled; if it is, then the workload entity is required
  • if both CEDARLING_USER_AUTHZ and CEDARLING_WORKLOAD_AUTHZ is disabled, error on startup
  • renamed some structs and errors that doesn't align with their implementations

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.

- make the `access_token` param for the JwtService::process_tokens
  optional
- make the `id_token` param for the JwtService::process_tokens
  optional
- return an `Unimplemented` error when any `None` is passed in for the
  tokens since the `authz` module doesn't support optional params yet.

Signed-off-by: rmarinn <[email protected]>
- implement JwtService::process_token which decodes and optionally
  validates a single JWT.

Signed-off-by: rmarinn <[email protected]>
- replace the old jwt validation implementation that needs multiple
  tokens at once with the new implementation which validates the tokens
  one at a time.

Signed-off-by: rmarinn <[email protected]>
- implement allowing passing `None` for some tokens when calling
  authorize.

Signed-off-by: rmarinn <[email protected]>
implement convenience function for getting the following data from a
decoded JWT:
- metadata
- user_mapping
- claim_mapping
- role_mapping
- logging_info

Signed-off-by: rmarinn <[email protected]>
…nation

- change order for creating workload entities to: id_token then
  access_token
- improve `create_workload` error handling
- renamed `CedarPolicyCreateTypeError` to `CreateCedarEntityError`

Signed-off-by: rmarinn <[email protected]>
- change order for creating workload entities to: id_token,
  access_token, then userinfo_token
- improve `create_user_entitiy` error handling

Signed-off-by: rmarinn <[email protected]>
- Error when both `CEDARLING_USER_AUTHZ` and
  `CEDARLING_WORKLOAD_AUTHZ` are disabled.

Signed-off-by: rmarinn <[email protected]>
- add `trusted` and `principal_identifier` to the TokenEntityMetadata
  struct so all tokens have access to them.

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

dryrunsecurity bot commented Dec 17, 2024

DryRun Security Summary

The pull request focuses on enhancing the security and robustness of the Cedarling application by improving JWT validation, error handling, entity creation, policy schema parsing, and documentation across multiple modules and files.

Expand for full summary

Summary:

The code changes in this pull request cover a wide range of updates and improvements to the Cedarling application, with a strong emphasis on application security. The changes span multiple modules and files, addressing various aspects of the authorization and policy enforcement mechanisms, token validation, error handling, and documentation.

Some of the key security-focused changes include:

  1. Improvements to JWT validation, including the ability to enable/disable signature and status validation, as well as configurable trust modes for ID tokens.
  2. Enhancements to error handling and custom error types to provide more detailed information about issues that may arise during the authorization process.
  3. Refinements to the entity creation and validation logic, ensuring that user input is properly sanitized and that the application can handle various types of errors and unexpected data.
  4. Additions to the Cedar Policy schema parsing and representation, with a focus on robust deserialization and type validation to prevent potential security vulnerabilities.
  5. Updates to the documentation, including clarifications and improvements to the token metadata schema and the Cedarling Python bindings.

Overall, the changes in this pull request demonstrate a strong commitment to maintaining the security and integrity of the Cedarling application. The attention to detail in the error handling, input validation, and token management mechanisms is particularly noteworthy and should help to mitigate potential security risks.

Files Changed:

  1. jans-cedarling/bindings/cedarling_python/PYTHON_TYPES.md: Updates to the documentation for the Cedarling Python bindings, including changes to the Request class and the addition of new error types.
  2. jans-cedarling/bindings/cedarling_python/cedarling_python.pyi: Changes to the Python type hint (stub) file, including making the token attributes of the Request class optional.
  3. jans-cedarling/bindings/cedarling_python/src/authorize/errors.rs: Improvements to error handling and exception management in the authorize module, including the definition of custom exception types.
  4. docs/cedarling/cedarling-policy-store.md: Updates to the documentation for the Cedarling Policy Store, with a focus on the token metadata schema and trusted issuers.
  5. jans-cedarling/bindings/cedarling_python/src/authorize/request.rs: Refactoring of the Request struct to make the token fields optional.
  6. jans-cedarling/cedarling/examples/authorize_without_jwt_validation.rs: An example of authorizing a request without JWT validation, which raises security concerns.
  7. jans-cedarling/cedarling/examples/authorize_with_jwt_validation.rs: An example of authorizing a request with JWT validation, demonstrating the importance of proper token handling.
  8. jans-cedarling/cedarling/src/authz/entities/create.rs: Implementation of entity creation, with a focus on input validation and error handling.
  9. jans-cedarling/cedarling/src/authz/entities/mod.rs: Improvements to the creation of various entities, including tokens, resources, and roles.
  10. jans-cedarling/cedarling/src/authz/authorize_result.rs: Refactoring of the AuthorizeResult struct and associated functions.
  11. jans-cedarling/cedarling/src/authz/entities/test_create.rs: Test cases covering the creation of entities, including error handling.
  12. jans-cedarling/cedarling/src/authz/entities/trait_as_expression.rs: Refactoring and cleanup of the AsExpression trait.
  13. jans-cedarling/cedarling/src/authz/entities/user.rs: Implementation of the create_user_entity function, with a focus on token validation and error handling.
  14. jans-cedarling/cedarling/src/authz/merge_json.rs: Implementation of a function to merge JSON values, with conflict handling.
  15. jans-cedarling/cedarling/src/authz/entities/workload.rs: Implementation of the create_workload_entity function, with token validation and error handling.
  16. jans-cedarling/cedarling/src/authz/request.rs: Changes to the Request struct, making the token fields optional.

Code Analysis

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

View PR in the DryRun Dashboard.

- add tests for creating user entity
- add tests for creating workload entity

Signed-off-by: rmarinn <[email protected]>
- errors that occur because no tokens were available should now be
  tracable when creating user and workload entities

Signed-off-by: rmarinn <[email protected]>
- combine `entities::create_<token_kind>_token` functions into a single,
  unified function.
- prevent incorrect token types from creating entities of the wrong kind (e.g.,
  using access_tokens to create id_token entities).
- simplify logic and enforce stricter type safety for token-to-entity mapping.

Signed-off-by: rmarinn <[email protected]>
…ation

- remove the use of access_token in the create_user_entity function
- simplify the user entity creation process.

Signed-off-by: rmarinn <[email protected]>
- fix field name conflict by using serde_rename for UserAuthorizeInfo
  and WorkloadAuthorizeInfo

Signed-off-by: rmarinn <[email protected]>
- change the token priority when creating the userinfo entity to be
  1. userinfo_token then 2. id_token

Signed-off-by: rmarinn <[email protected]>
@rmarinn rmarinn marked this pull request as ready for review December 20, 2024 14:12
@rmarinn rmarinn requested a review from olehbozhok December 20, 2024 14:12
/// cedar-policy user/person principal
pub person_principal: String,
pub principal: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

how about diagnostics ?

}
}

#[allow(dead_code)]
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 need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a TokenKind::Transaction so i added it for completeness' sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just removed it here since it seems easy enough to add back later: 87b399f.

- rename UserAuthorizeInfo.principal to user_principal
- rename UserAuthorizeInfo.diagnostics to user_diagnostics
- rename UserAuthorizeInfo.decision to user_decision
- rename WorkloadAuthorizeInfo.principal to workload_principal
- rename WorkloadAuthorizeInfo.diagnostics to workload_diagnostics
- rename WorkloadAuthorizeInfo.decision to workload_decision

Signed-off-by: rmarinn <[email protected]>
Comment on lines 142 to 149
// Convert [`AuthorizeEntitiesData`] to [`cedar_policy::Entities`] structure,
// hold all entities that will be used on authorize check.
let entities = entities_data.entities(Some(&schema.schema))?;

// Check authorize where principal is `"Jans::Workload"` from cedar-policy schema.
let workload_result: Option<Response> = if self.config.authorization.use_workload_principal
{
match self.execute_authorize(ExecuteAuthorizeParameters {
entities: &entities,
principal: principal_workload_uid.clone(),
action: action.clone(),
resource: resource_uid.clone(),
context: context.clone(),
}) {
Ok(resp) => Some(resp),
Err(err) => return Err(AuthorizeError::CreateRequestWorkloadEntity(err)),
}
} else {
None
};
let entities = entities_data.clone().entities(Some(&schema.schema))?;

let (workload_authz_result, workload_authz_info, workload_entity_claims) =
if let Some(workload) = entities_data.workload {
let principal = workload.uid();

Copy link
Contributor

Choose a reason for hiding this comment

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

adding 2 lines we can avoid clone whole structure of entities_data

Suggested change
// Convert [`AuthorizeEntitiesData`] to [`cedar_policy::Entities`] structure,
// hold all entities that will be used on authorize check.
let entities = entities_data.entities(Some(&schema.schema))?;
// Check authorize where principal is `"Jans::Workload"` from cedar-policy schema.
let workload_result: Option<Response> = if self.config.authorization.use_workload_principal
{
match self.execute_authorize(ExecuteAuthorizeParameters {
entities: &entities,
principal: principal_workload_uid.clone(),
action: action.clone(),
resource: resource_uid.clone(),
context: context.clone(),
}) {
Ok(resp) => Some(resp),
Err(err) => return Err(AuthorizeError::CreateRequestWorkloadEntity(err)),
}
} else {
None
};
let entities = entities_data.clone().entities(Some(&schema.schema))?;
let (workload_authz_result, workload_authz_info, workload_entity_claims) =
if let Some(workload) = entities_data.workload {
let principal = workload.uid();
let workload_principal = entities_data.workload.as_ref().map(|e| e.uid()).to_owned();
let user_principal = entities_data.user.as_ref().map(|e| e.uid()).to_owned();
// Convert [`AuthorizeEntitiesData`] to [`cedar_policy::Entities`] structure,
// hold all entities that will be used on authorize check.
let entities = entities_data.entities(Some(&schema.schema))?;
let (workload_authz_result, workload_authz_info, workload_entity_claims) =
if let Some(workload) = workload_principal {
let principal = workload;

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved in a4cbbda

@olehbozhok
Copy link
Contributor

added comment
#10436 (comment)

Copy link
Contributor

@olehbozhok olehbozhok left a comment

Choose a reason for hiding this comment

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

Looks OK to me, but need to notice about #10436 (comment)

@rmarinn rmarinn merged commit 5566a33 into main Dec 24, 2024
10 of 11 checks passed
@rmarinn rmarinn deleted the jans-cedarling-10408 branch December 24, 2024 05:42
ossdhaval pushed a commit that referenced this pull request Dec 27, 2024
…ling::authorize (#10436)

* refactor(jans-cedarling): make process_tokens params optional

- make the `access_token` param for the JwtService::process_tokens
  optional
- make the `id_token` param for the JwtService::process_tokens
  optional
- return an `Unimplemented` error when any `None` is passed in for the
  tokens since the `authz` module doesn't support optional params yet.

Signed-off-by: rmarinn <[email protected]>

* feat(jans-cedarling): implement processing single tokens

- implement JwtService::process_token which decodes and optionally
  validates a single JWT.

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): replace jwt validation implementation

- replace the old jwt validation implementation that needs multiple
  tokens at once with the new implementation which validates the tokens
  one at a time.

Signed-off-by: rmarinn <[email protected]>

* feat(jans-cedarling): make tokens optional when calling authorize

- implement allowing passing `None` for some tokens when calling
  authorize.

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): make creation of workload entity optional

Signed-off-by: rmarinn <[email protected]>

* fix(jans-cedarling): user and role entity creation

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): make creation of user entity optional

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): use if-else instead of match for bool

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): improve functionality for decoded JWTs

implement convenience function for getting the following data from a
decoded JWT:
- metadata
- user_mapping
- claim_mapping
- role_mapping
- logging_info

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): removed unused functions for TrustedIssuer

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): improve create_workload_entity func implemenation

- change order for creating workload entities to: id_token then
  access_token
- improve `create_workload` error handling
- renamed `CedarPolicyCreateTypeError` to `CreateCedarEntityError`

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): improve create_user_entity func implemenation

- change order for creating workload entities to: id_token,
  access_token, then userinfo_token
- improve `create_user_entitiy` error handling

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): resolve clippy issues

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): error check when loading bootstrap config

- Error when both `CEDARLING_USER_AUTHZ` and
  `CEDARLING_WORKLOAD_AUTHZ` are disabled.

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): add new fields to TokenEntityMetadata

- add `trusted` and `principal_identifier` to the TokenEntityMetadata
  struct so all tokens have access to them.

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): remove some cloning when logging authz

Signed-off-by: rmarinn <[email protected]>

* test(jans-cedarling): add unit tests for entity creation

- add tests for creating user entity
- add tests for creating workload entity

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): improve error handling for entity creation

- errors that occur because no tokens were available should now be
  tracable when creating user and workload entities

Signed-off-by: rmarinn <[email protected]>

* test(jans-cedarling): update python tests expected error

Signed-off-by: rmarinn <[email protected]>

* feat(jans-cedarling): make tokens optional in the python binding

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): change error message to start with a lowercase letter

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): flattened `use` statements for readability

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): remove unused JwtProcessingError variant

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): simplify iterator creation

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): make workload entity creation optional

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): make user entity creation optional

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): remove outdated comment

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): convert Token into a struct from an enum

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): unify token entity creation logic

- combine `entities::create_<token_kind>_token` functions into a single,
  unified function.
- prevent incorrect token types from creating entities of the wrong kind (e.g.,
  using access_tokens to create id_token entities).
- simplify logic and enforce stricter type safety for token-to-entity mapping.

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): eliminate access_token from user entity creation

- remove the use of access_token in the create_user_entity function
- simplify the user entity creation process.

Signed-off-by: rmarinn <[email protected]>

* fix(jans-cedarling): field name conflict for serializing/deserializing

- fix field name conflict by using serde_rename for UserAuthorizeInfo
  and WorkloadAuthorizeInfo

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): remove unused commented code

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): token priority when creating userinfo entity

- change the token priority when creating the userinfo entity to be
  1. userinfo_token then 2. id_token

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): resolve clippy issues

Signed-off-by: rmarinn <[email protected]>

* test(jans-cedarling): update expected error in python bindings test

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): implement rename for logging info fields

- rename UserAuthorizeInfo.principal to user_principal
- rename UserAuthorizeInfo.diagnostics to user_diagnostics
- rename UserAuthorizeInfo.decision to user_decision
- rename WorkloadAuthorizeInfo.principal to workload_principal
- rename WorkloadAuthorizeInfo.diagnostics to workload_diagnostics
- rename WorkloadAuthorizeInfo.decision to workload_decision

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): run cargo fmt and add missing license headers

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): rename the Vec of role from `role` to `roles`

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): remove unused function

Signed-off-by: rmarinn <[email protected]>

* refactor(jans-cedarling): rename UserAuthorizeInfo fields

- change `user` to `person`

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): run cargo fmt

Signed-off-by: rmarinn <[email protected]>

* chore(jans-cedarling): refactor to avoid unnecessary cloning

Signed-off-by: Oleh Bohzok <[email protected]>

---------

Signed-off-by: rmarinn <[email protected]>
Signed-off-by: Oleh Bohzok <[email protected]>
Co-authored-by: Oleh Bohzok <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(jans-cedarling): make all tokens optional when calling Cedarling::authorize
3 participants