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

Merged
merged 14 commits into from
Jan 18, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat(jans-cedarling): implement CEDARLING_ID_TOKEN_TRUST_MODE
Signed-off-by: rmarinn <[email protected]>
rmarinn committed Jan 10, 2025

Verified

This commit was signed with the committer’s verified signature.
rmarinn Richard Marin
commit 5d02f1ae5241051dbdb05df3c1eef079d4fd74af
Original file line number Diff line number Diff line change
@@ -120,6 +120,13 @@ create_exception!(
"Error encountered while parsing all entities to json for logging"
);

create_exception!(
authorize_errors,
IdTokenTrustModeError,
AuthorizeError,
"Error encountered while parsing all entities to json for logging"
);

create_exception!(
authorize_errors,
AddEntitiesIntoContextError,
@@ -179,7 +186,8 @@ errors_functions! {
UserRequestValidation => UserRequestValidationError,
BuildContext => AddEntitiesIntoContextError,
Entities => EntitiesError,
EntitiesToJson => EntitiesToJsonError
EntitiesToJson => EntitiesToJsonError,
IdTokenTrustMode => IdTokenTrustModeError
}

pub fn authorize_errors_module(m: &Bound<'_, PyModule>) -> PyResult<()> {
djellemah marked this conversation as resolved.
Show resolved Hide resolved
Empty file.
10 changes: 10 additions & 0 deletions jans-cedarling/cedarling/src/authz/mod.rs
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ use std::io::Cursor;
use std::str::FromStr;
use std::sync::Arc;

use crate::authorization_config::IdTokenTrustMode;
use crate::bootstrap_config::AuthorizationConfig;
use crate::common::app_types;
use crate::common::cedar_schema::cedar_json::{BuildJsonCtxError, FindActionError};
@@ -26,6 +27,7 @@ use crate::log::{

mod authorize_result;
mod merge_json;
mod trust_mode;

pub(crate) mod entities;
pub(crate) mod request;
@@ -42,6 +44,7 @@ use entities::{
use merge_json::{merge_json_values, MergeError};
use request::Request;
use serde_json::Value;
use trust_mode::*;

/// Configuration to Authz to initialize service without errors
pub(crate) struct AuthzConfig {
@@ -124,6 +127,10 @@ impl Authz {
let schema = &self.config.policy_store.schema;
let tokens = self.decode_tokens(&request)?;

if let IdTokenTrustMode::Strict = self.config.authorization.id_token_trust_mode {
enforce_id_tkn_trust_mode(&tokens)?;
}

// Parse action UID.
let action = cedar_policy::EntityUid::from_str(request.action.as_str())
.map_err(AuthorizeError::Action)?;
@@ -552,6 +559,9 @@ pub enum AuthorizeError {
/// Error encountered while building the context for the request
#[error("Failed to build context: {0}")]
BuildContext(#[from] BuildContextError),
/// Error encountered while building the context for the request
#[error("error while running on strict id token trust mode: {0}")]
IdTokenTrustMode(#[from] IdTokenTrustModeError),
}

#[derive(Debug, thiserror::Error)]
321 changes: 321 additions & 0 deletions jans-cedarling/cedarling/src/authz/trust_mode.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,321 @@
// This software is available under the Apache-2.0 license.
// See https://www.apache.org/licenses/LICENSE-2.0.txt for full text.
//
// Copyright (c) 2024, Gluu, Inc.

use super::entities::DecodedTokens;
use crate::{
common::policy_store::TokenKind,
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

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

let access_tkn = tokens
.access_token
.as_ref()
.ok_or(IdTokenTrustModeError::MissingAccessToken)?;
let id_tkn = tokens
.id_token
.as_ref()
.ok_or(IdTokenTrustModeError::MissingIdToken)?;

let access_tkn_client_id = get_tkn_claim_as_str(access_tkn, "client_id")?;
let id_tkn_aud = get_tkn_claim_as_str(id_tkn, "aud")?;

if access_tkn_client_id != id_tkn_aud {
return Err(IdTokenTrustModeError::ClientIdIdTokenAudMismatch);
}

let userinfo_tkn = match tokens.userinfo_token.as_ref() {
Some(token) => token,
None => return Ok(()),
};
let userinfo_tkn_aud = get_tkn_claim_as_str(userinfo_tkn, "aud")?;

if userinfo_tkn_aud != id_tkn_aud {
return Err(IdTokenTrustModeError::SubMismatchIdTokenUserinfo);
}
if userinfo_tkn_aud != access_tkn_client_id {
return Err(IdTokenTrustModeError::ClientIdUserinfoAudMismatch);
}

Ok(())
}

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


#[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

#[error("an access token is required when using strict mode")]
MissingAccessToken,
#[error("an id token is required when using strict mode")]
MissingIdToken,
#[error("the id token's `sub` does not match with the userinfo token's `sub`")]
SubMismatchIdTokenUserinfo,
#[error("the access token's `client_id` does not match with the userinfo token's `aud`")]
ClientIdUserinfoAudMismatch,
#[error("missing a required claim `{0}` from `{1}` token")]
MissingRequiredClaim(String, TokenKind),
#[error("invalid claim type in {0} token: {1}")]
TokenClaimTypeError(TokenKind, TokenClaimTypeError),
}

#[cfg(test)]
mod test {
use super::{enforce_id_tkn_trust_mode, IdTokenTrustModeError};
use crate::authz::entities::DecodedTokens;
use crate::common::policy_store::TokenKind;
use crate::jwt::{Token, TokenClaims};
use serde_json::json;
use std::collections::HashMap;

#[test]
fn success_without_userinfo_tkn() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: None,
};
enforce_id_tkn_trust_mode(&tokens).expect("should not error");
}

#[test]
fn errors_when_missing_access_tkn() {
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let tokens = DecodedTokens {
access_token: None,
id_token: Some(id_token),
userinfo_token: None,
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(err, IdTokenTrustModeError::MissingAccessToken),
"expected error due to missing access token, got: {:?}",
err
)
}

#[test]
fn errors_when_access_tkn_missing_required_claim() {
let access_token = Token::new_access(TokenClaims::new(HashMap::new()), None);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: None,
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(
err,
IdTokenTrustModeError::MissingRequiredClaim(ref claim_name, tkn_kind)
if claim_name == "client_id" &&
tkn_kind == TokenKind::Access
),
"expected error due to access token missing a required claim, got: {:?}",
err
)
}

#[test]
fn errors_when_missing_id_tkn() {
let access_token = Token::new_id(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: None,
userinfo_token: None,
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(err, IdTokenTrustModeError::MissingIdToken),
"expected error due to missing id token, got: {:?}",
err
)
}

#[test]
fn errors_when_id_tkn_missing_required_claim() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(TokenClaims::new(HashMap::new()), None);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: None,
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(
err,
IdTokenTrustModeError::MissingRequiredClaim(ref claim_name, tkn_kind)
if claim_name == "aud" &&
tkn_kind == TokenKind::Id
),
"expected error due to id token missing a required claim, got: {:?}",
err
)
}

#[test]
fn errors_when_access_tkn_client_id_id_tkn_aud_mismatch() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([(
"aud".to_string(),
json!("another-id-123"),
)])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: None,
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(err, IdTokenTrustModeError::ClientIdIdTokenAudMismatch),
"expected error due to the access_token's `client_id` not matching with the id_token's `aud`, got: {:?}",
err
)
}

#[test]
fn success_with_userinfo_tkn() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let userinfo_token = Token::new_userinfo(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: Some(userinfo_token),
};
enforce_id_tkn_trust_mode(&tokens).expect("should not error");
}

#[test]
fn errors_when_userinfo_tkn_missing_required_claim() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let userinfo_token = Token::new_userinfo(TokenClaims::new(HashMap::new()), None);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: Some(userinfo_token),
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(
err,
IdTokenTrustModeError::MissingRequiredClaim(ref claim_name, tkn_kind)
if claim_name == "aud" &&
tkn_kind == TokenKind::Userinfo
),
"expected error due to id token missing a required claim, got: {:?}",
err
)
}

#[test]
fn errors_when_access_tkn_client_id_userinfo_tkn_aud_mismatch() {
let access_token = Token::new_access(
TokenClaims::new(HashMap::from([(
"client_id".to_string(),
json!("some-id-123"),
)])),
None,
);
let id_token = Token::new_id(
TokenClaims::new(HashMap::from([("aud".to_string(), json!("some-id-123"))])),
None,
);
let userinfo_token = Token::new_userinfo(
TokenClaims::new(HashMap::from([(
"aud".to_string(),
json!("another-id-123"),
)])),
None,
);
let tokens = DecodedTokens {
access_token: Some(access_token),
id_token: Some(id_token),
userinfo_token: Some(userinfo_token),
};
let err = enforce_id_tkn_trust_mode(&tokens).expect_err("should error");
assert!(
matches!(err, IdTokenTrustModeError::SubMismatchIdTokenUserinfo),
"expected error due to the id_token's `aud` not matching with the userinfo_token's `aud`, got: {:?}",
err
)
}
}
Loading