Skip to content

Conversation

derekhiggins
Copy link
Contributor

@derekhiggins derekhiggins commented Oct 15, 2025

fix: nested claims mapping in OAuth2 token validation

The get_attributes_from_claims function was only checking for top-level
claim keys, causing token validation to fail when using nested claims
like "resource_access.llamastack.roles" (common in Keycloak JWT tokens).

Updated the function to support dot notation for traversing nested claim
structures. Give precedence to dot notation over literal keys with dots
in claims mapping.

Added test coverage.

Closes: #3812

@derekhiggins
Copy link
Contributor Author

Note: This would break the case where the top level key is an actual string with a ".", I'm wondering do we need to support both cases?

Copy link
Contributor

ehhuang commented Oct 16, 2025

Note: This would break the case where the top level key is an actual string with a ".", I'm wondering do we need to support both cases?

can you add a validation to the config class?

@derekhiggins
Copy link
Contributor Author

Note: This would break the case where the top level key is an actual string with a ".", I'm wondering do we need to support both cases?

can you add a validation to the config class?

I'm not sure what could be validated at config/startup. We won't see the claims to know which it is until runtime, I can think of 3 options

  1. Support only nested keys with dot's (this PR)
  2. Adding an explicit config option to enable/disable dot notation traversal
  3. Try nested keys first, then fall back to literal key if nested traversal fails (support both with precedence)

@ashwinb
Copy link
Contributor

ashwinb commented Oct 16, 2025

Any thoughts here @grs and @leseb? Outside my area of expertise!

@grs
Copy link
Contributor

grs commented Oct 17, 2025

I can think of 3 options

  1. Support only nested keys with dot's (this PR)
  2. Adding an explicit config option to enable/disable dot notation traversal
  3. Try nested keys first, then fall back to literal key if nested traversal fails (support both with precedence)

I think option 3 is the best. Option 2 is ugly and pushes the problem into configuration. Option 1, i.e. not supporting dots in keys at all, seems too drastic.

I think we could consider the dotted form and nested form 'equivalent'. It seems very unlikely that a given token would have separate claims, one nested and another with the equivalent dotted key, especially with different values.

Supporting both does not require much more code and will 'just work' in every likely case.

The get_attributes_from_claims function was only checking for top-level
claim keys, causing token validation to fail when using nested claims
like "resource_access.llamastack.roles" (common in Keycloak JWT tokens).

Updated the function to support dot notation for traversing nested claim
structures. Give precedence to dot notation over literal keys with dots
in claims mapping.

Added test coverage.

Closes: llamastack#3812

Signed-off-by: Derek Higgins <derekh@redhat.com>
@derekhiggins
Copy link
Contributor Author

I can think of 3 options

  1. Support only nested keys with dot's (this PR)
  2. Adding an explicit config option to enable/disable dot notation traversal
  3. Try nested keys first, then fall back to literal key if nested traversal fails (support both with precedence)

I think option 3 is the best. Option 2 is ugly and pushes the problem into configuration. Option 1, i.e. not supporting dots in keys at all, seems too drastic.

I think we could consider the dotted form and nested form 'equivalent'. It seems very unlikely that a given token would have separate claims, one nested and another with the equivalent dotted key, especially with different values.

Supporting both does not require much more code and will 'just work' in every likely case.

thanks @grs , I've updated the PR to support both with precedence to dot notation

Copy link
Contributor

@grs grs left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth2 Token Validation Fails with Nested Claims

4 participants