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.

Sign up for GitHub

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

Jump to bottom

Support configurable principal claim in JWT Realm Tokens #86533

Merged
merged 38 commits into from
May 24, 2022
Merged

Support configurable principal claim in JWT Realm Tokens #86533

merged 38 commits into from
May 24, 2022

Conversation

justincr-elastic
Copy link
Contributor

@justincr-elastic justincr-elastic commented May 6, 2022 •
edited
Loading

JWT Realm Tokens use hard-coded claims iss/aud/sub to compute token principal. The principal is used to cache realm order, so a previously successful realm can be bumped to the start of the realm list for that principal.

This works well for JWTs that are OIDC ID Tokens, because those 3 claims are mandatory. It does not work for OAuth 2.0 Access Tokens, which are typically opaque values. If those Access Tokens are JWTs, sometimes they use similar claims but not all of them. Access Tokens can be created via a number of different OAuth 2.0 flows, including but not limited to:

  • Authorization Code Flow (Resource Owner is End-User)
  • Implicit Code Flow (Resource Owner is End-User)
  • Client Credential Flow (Resource Owner is Web Server)

This PR will allow overriding JWT Realm Token parsing to override using principal=iss/aud/sub. Realm order cache keys are used outside of individual JWT realms, so it is not possible to configure this per realm. Multiple JWT realms can still be configured and work, but processing order might change at runtime depending on the override claims selected.

tvernum and others added 2 commits May 3, 2022 18:53
@tvernum
Make user-id claim configurable in JWT
6b7fd5a 
The JwtAuthenticationToken relied on the "sub" claim always existing
because OIDC requires that ID tokens contain a sub, as does the spec
for JWT format OAuth access tokens (RFC 9068).

However, the may be cases where JWTs do not contain a "sub", so we
instead consult an ordered list of "user id claims" that can be
configured via the xpack.security.authc.jwt.userid_claims setting
@justincr-elastic
Merge branch 'jwt/config-id-claims' of https://github.com/tvernum/ela…
7276361 
…sticsearch into enhancement/jwt-realm-token-config
@justincr-elastic justincr-elastic added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.3.0 labels May 6, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @justincr-elastic, I've created a changelog YAML for you.

@justincr-elastic justincr-elastic mentioned this pull request May 11, 2022
Closed
@justincr-elastic
Copy link
Contributor Author

@elasticmachine update branch

@justincr-elastic
Copy link
Contributor Author

@elasticmachine update branch

@justincr-elastic
Copy link
Contributor Author

@elasticmachine update branch

@justincr-elastic
Copy link
Contributor Author

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@justincr-elastic justincr-elastic requested a review from gwbrown May 20, 2022 15:03
gwbrown
gwbrown reviewed May 20, 2022
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this, Justin! The logic looks good as far as I can tell, not being very familiar with JWT - but it seems aligned with the email threads and such on the topic.

I left a few comments, mostly cleanups and minor issues.

@justincr-elastic justincr-elastic requested a review from gwbrown May 20, 2022 20:32
gwbrown
gwbrown approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Thanks, good work. LGTM!

@justincr-elastic
Copy link
Contributor Author

Hi @evilwon22, LGTM is a commonly used acronym in Elasticsearch PRs which means Looks Good To Me.

@justincr-elastic justincr-elastic requested a review from tvernum May 20, 2022 21:53
@justincr-elastic
Copy link
Contributor Author

justincr-elastic commented May 20, 2022 •
edited
Loading

Hi @tvernum, Gordon reviewed and approved, but we would like you to take a second look at these changes too.

Instead of calling it JwtAuthenticatorTokenFactory from your initial commit, I generalized the name to JwtRealms. In theory, it holds settings and code which are common for every JwtRealm.

If you have a suggestion for a better name, let me know. Is it a Manager, Factory, Proxy, or something else? For now, it includes common code for parsing the principalClaimNames setting, and token object. However, it could be generalized to handle other things on behalf of all JwtRealm instances.

It is not part of the scope of this PR, but I am wondering in future if this JwtRealms pattern of having a list of each JwtRealm could be utilized to improve performance of token calls. For example, if a JWT is missing claims in the principalClaimNames setting, JwtRealm.token() will execute N times the number of configured JwtRealms, and all of those calls will fail. There might be a way to remember if the first call to JwtRealm.token() failed, and skip the remaining N-1 calls to the same method.

Another future idea is have the first call to JwtRealm.token() iterate over all of the JwtRealm objects, and use the principal claim name setting from each realm to decide which claim to use from a JWT for the realm order cache key, instead of duplicating those claim names in the new setting introduced by this PR.

tvernum
tvernum reviewed May 23, 2022
View reviewed changes
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I'm a bit surprised about where this PR ended up.

It contains additional validation checks (and realm tracking) that I don't think we need.
Overall, I'm OK with it, but I think the check that the realm's principal is in the principal_claims is an obstacle.

@justincr-elastic
Copy link
Contributor Author

@elasticmachine update branch

@justincr-elastic justincr-elastic requested a review from tvernum May 23, 2022 16:09
tvernum
tvernum approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

@justincr-elastic justincr-elastic merged commit 76be7bc into elastic:master May 24, 2022
@justincr-elastic justincr-elastic changed the title Support configurable claims in JWT Realm Tokens Support configurable principal claim in JWT Realm Tokens May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gwbrown gwbrown gwbrown approved these changes

@tvernum tvernum tvernum approved these changes

Assignees
No one assigned
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@justincr-elastic @elasticsearchmachine @elasticmachine @gwbrown @tvernum