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

feat(jans-cedarling): implement CEDARLING_ID_TOKEN_TRUST_MODE #10585

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Jan 10, 2025

Prepare


Description

This PR implements ID token trust mode which can be set via the CEDARLING_ID_TOKEN_TRUST_MODE bootstrap property. The trust mode implements additional checks done on the input token's claims. This PR introduces two modes: None and Strict (more info below).

Target issue

target issue: #10479

closes: #10479

Implementation Details

None Mode

No additional validations checks on the tokens are implemented.

Strict Mode
  • id_token.aud must match the access_token.client_id.
  • If a Userinfo token is present:
    • The sub must match the id_token.sub.
    • The aud must match the access_token.client_id.

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.

@rmarinn rmarinn added the comp-jans-cedarling Touching folder /jans-cedarling label Jan 10, 2025
@rmarinn rmarinn self-assigned this Jan 10, 2025
@rmarinn rmarinn linked an issue Jan 10, 2025 that may be closed by this pull request
4 tasks
@mo-auto mo-auto added the kind-feature Issue or PR is a new feature request label Jan 10, 2025
@mo-auto mo-auto added the area-documentation Documentation needs to change as part of issue or PR label Jan 10, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove 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.

removed in a7a888e

Strict,
}

impl FromStr for IdTokenTrustMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that, must've missed it since rustanalyzer isn't complaining. removed in 9e964d4

jwt::{Token, TokenClaimTypeError},
};

pub fn enforce_id_tkn_trust_mode(tokens: &DecodedTokens) -> Result<(), IdTokenTrustModeError> {

Choose a reason for hiding this comment

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

Please write a small description for at least public functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a docstring that describes what the function does here: 4208d86

jwt::{Token, TokenClaimTypeError},
};

pub fn enforce_id_tkn_trust_mode(tokens: &DecodedTokens) -> Result<(), IdTokenTrustModeError> {

Choose a reason for hiding this comment

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

maybe you can have something more descriptive by renaming this function to validate_id_token_trust_mode, but it's up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that might be a better name. renamed the function here: ff0c60e

#[derive(Debug, thiserror::Error)]
pub enum IdTokenTrustModeError {
#[error("the access token's `client_id` does not match with the id token's `aud`")]
ClientIdIdTokenAudMismatch,

Choose a reason for hiding this comment

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

maybe you can rename this one to AccessTokenClientIdMismatch to avoid some typing error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed here 2195d90

Comment on lines 45 to 59
fn get_tkn_claim_as_str(
token: &Token,
claim_name: &str,
) -> Result<Box<str>, IdTokenTrustModeError> {
let claim = token
.get_claim(claim_name)
.ok_or(IdTokenTrustModeError::MissingRequiredClaim(
claim_name.to_string(),
token.kind,
))?;
let claim_str = claim
.as_str()
.map_err(|e| IdTokenTrustModeError::TokenClaimTypeError(token.kind, e))?;
Ok(claim_str.into())
}

Choose a reason for hiding this comment

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

You can simplify this function by

fn get_tkn_claim_as_str(
    token: &Token,
    claim_name: &str,
) -> Result<Box<str>, IdTokenTrustModeError> {
    token
        .get_claim(claim_name)
        .ok_or_else(|| IdTokenTrustModeError::MissingRequiredClaim(claim_name.to_string(), token.kind))
        .and_then(|claim| claim.as_str()
            .map(|s| s.into())
            .map_err(|e| IdTokenTrustModeError::TokenClaimTypeError(token.kind, e)))
}

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 think that is less readable... but I think can get used to it so i changed it here fdda12e

olehbozhok
olehbozhok previously approved these changes Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-documentation Documentation needs to change as part of issue or PR comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(jans-cedarling): implement CEDARLING_ID_TOKEN_TRUST_MODE
5 participants