-
Notifications
You must be signed in to change notification settings - Fork 78
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): custom tokens and putting tokens in principal attrs #10706
base: main
Are you sure you want to change the base?
Conversation
…perty Signed-off-by: rmarinn <[email protected]>
- automatically add token entity references to the principal entities if they are required in the principal's schema Signed-off-by: rmarinn <[email protected]>
- implement support for custom tokens - implement `CEDARLING_TOKEN_ENTITY_MAPPER` bootstrap property to support putting token entities into principal entities - implement `CEDARLING_MAPPING_TOKENS` bootstrap property to support having custom tokens - implement `CEDARLING_MAPPING_ROLE` bootstrap property to set the entity name of the role entity - implement `CEDARLING_TOKEN_VALIDATION_SETTINGS` bootstrap property to support having custom tokens - add support for custom token entity metatdata Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- remove token entity mapping in entity builder. the token entity mapping is enforced in the jwt module. if the token isn't in the mapper, the token isn't decoded or validated. 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]>
Signed-off-by: rmarinn <[email protected]>
- add docstring for token entity mapper - add docstring for token entity mapping Signed-off-by: rmarinn <[email protected]>
Implement CEDARLING_TOKEN_CONFIGS which replaces CEDARLING_TOKEN_VALIDATION_SETTINGS and CEDARLING_MAPPING_TOKENS 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]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
@@ -25,14 +23,14 @@ use crate::{ | |||
/// - if a Userinfo token is present: | |||
/// - `userinfo_token.aud` == `access_token.client_id` | |||
/// - `userinfo_token.sub` == `id_token.sub` | |||
pub fn validate_id_tkn_trust_mode(tokens: &DecodedTokens) -> Result<(), IdTokenTrustModeError> { | |||
pub fn validate_id_tkn_trust_mode( |
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.
Should we check other tokens?
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.
For now, I don't think so since this is just for checking the tokens defined in openid connect core.
Though users want to add additional checks for their custom tokens, they can just do so via the Cedar policies since token entities get created.
// we just ignore input tokens that are not defined | ||
// in the token entity mapper bootstrap config | ||
// | ||
// TODO: should we log that we skip some tokens? |
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 think it would be helpful to log this, at least in debug level.
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.
will add logging in issue #10730 after this merges
@@ -484,3 +485,17 @@ pub struct LogTokensInfo<'a> { | |||
pub userinfo: Option<HashMap<&'a str, &'a serde_json::Value>>, | |||
pub access: Option<HashMap<&'a str, &'a serde_json::Value>>, | |||
} | |||
|
|||
#[derive(Debug, Clone, PartialEq, serde::Serialize)] | |||
pub struct NewLogTokensInfo<'a>(pub HashMap<&'a str, HashMap<&'a str, &'a serde_json::Value>>); |
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 need to stay LogTokensInfo
structure? maybe we can remove LogTokensInfo
and rename current to the LogTokensInfo
?
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 forgot to remove that since rustanalyzer wasn't complaining that it was unused.
Removed here 673ded1.
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.
That's fine for me, but I would appreciate it if you could reply to the comments above.
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.
OK for me. It would be great to define how to correctly validate all tokens that present in request
The validation was not changed. Validation settings are set via the |
Prepare
Description
This PR implements:
Target issue
Target issue: #10591
closes #10591
Implementation Details
Updated bootstrap config
The following properties that are related to the JWTs are removed from the bootstrap config:
CEDARLING_MAPPING_ID_TOKEN
CEDARLING_MAPPING_ACCESS_TOKEN
CEDARLING_MAPPING_USERINFO_TOKEN
CEDARLING_AT_ISS_VALIDATION
CEDARLING_AT_JTI_VALIDATION
CEDARLING_AT_NBF_VALIDATION
CEDARLING_AT_EXP_VALIDATION
CEDARLING_IDT_ISS_VALIDATION
CEDARLING_IDT_SUB_VALIDATION
CEDARLING_IDT_EXP_VALIDATION
CEDARLING_IDT_IAT_VALIDATION
CEDARLING_IDT_AUD_VALIDATION
CEDARLING_USERINFO_ISS_VALIDATION
CEDARLING_USERINFO_SUB_VALIDATION
CEDARLING_USERINFO_AUD_VALIDATION
CEDARLING_USERINFO_EXP_VALIDATION
To configure the token validation settings and mappings, the new
CEDARLING_TOKEN_CONFIGS
will now be used.Note that tokens that are not in the
CEDARLING_TOKEN_CONFIGS
will be ignored and fields that are not defined will default to"disabled"
. Furthermore, if the boostrap property is not set, a default containing theaccess_token
,id_token
, anduserinfo_token
with some validation enabled will be used.The shape of the input to authz will remain the same, though the user can now add their custom tokens in the
"tokens"
field:Additionally, the a new bootstrap config,
CEDARLING_MAPPING_ROLE
, was added. This bootrap property tells Cedarling what the entity type name of theRole
is in the schema.Automatically adding token entities to the principal entity attributes
To automatically add the token entities to the principal entity attributes, two conditions must be met:
CEDARLING_TOKEN_CONFIGS
bootstrap property:If these two conditions are met, the built entity will have a token entity reference in it's attributes which it can access when defining policies
Updated Policy Store
To be able to support custom tokens, the
"trusted_issuers"
field in the policy store has been updated. The"access_tokens"
,"id_tokens"
,"userinfo_tokens"
, and"tx_tokens"
have been replaced with"tokens_metadata"
. The"claim_mapping"
field is a map of token_name -> token_metadata.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.