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

Resource Server - Multi-Tenant Jwt Decoder by Issuer #6778

Closed
gburboz opened this issue Apr 14, 2019 · 19 comments
Closed

Resource Server - Multi-Tenant Jwt Decoder by Issuer #6778

gburboz opened this issue Apr 14, 2019 · 19 comments
Assignees

Comments

@gburboz
Copy link

gburboz commented Apr 14, 2019

Summary

This is related to Issue #5351 but takes different approach to support multi-tenant Jwt Decoders by issuer

Actual Behavior

Currently Resource Server with jwt is configured as shown below which is then configured with underlying NimbusJwtDecoder to decode tokens.

security.oauth2.resourceserver:
  jwt:
    issuer-uri: https://idp.example.com

OR

security.oauth2.resourceserver:
  jwt:
    jwks_url: https://idp.example.com

Expected Behavior

Proposal is to add MultiTenantDelegatingJwtDecoder which is composed of multiple NimbusJwtDecoder indexed by a mandatory issuer-uri (Map<URL, NimbusJwtDecoder>) .

Use following configuration:

security.oauth2.resourceserver:
  multi-tenant-jwt:
    -
        jwt:
          issuer-uri: https://idp.example.com
          jwks_url: https://idp.example.com
    -
        jwt:
          issuer-uri: https://idp-other.example.com
          jwks_url: https://idp-other.example.com

The multi tenant decoder does initial parsing (JWT jwt = parse(token);) and lookup the issuer claim from parsed JWT.

Based off issuer claim, it will look up underlying NimbusJwtDecoder and delegates further processing to it.

NimbusJwtDecoder can optionally be modified so that it will have additional Jwt decode(JWT token) to avoid double parsing (JWT jwt = parse(token);).

Version

Spring Security 5.1.x

Sample

If NimbusJwtDecoder is not to be modified, MultiTenantDelegatingJwtDecoder can be implemented with pull request #6779

@gburboz gburboz changed the title Multi-Tenant by Issuer JwtDecoder Resource Server - Multi-Tenant Jwt Decoder by Issuer Apr 14, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Apr 25, 2019

Related to #5385

@gburboz
Copy link
Author

gburboz commented Apr 25, 2019

@jzheaux and @jgrandja , can you please review pull request #6817

@jzheaux
Copy link
Contributor

jzheaux commented Apr 25, 2019

Thanks for reaching out! It's good to see you again, @gburboz.

This ticket is one that we've been discussing a bit internally. We'll get back to you as soon as possible.

@jgrandja
Copy link
Contributor

jgrandja commented May 2, 2019

@gburboz I think a JwtDecoder implementation that delegates to one of it's composed JwtDecoder(s) based on the iss claim makes a lot of sense to provide further support for multi-tenancy. This was my initial thinking when #5385 was logged.

Looking at MultiTenantDelegatingJwtDecoder provides this but I think there is a limitation with Map<String, JwtDecoder> decoderByIssuer. The class is generally named but it's quite specific as it relies on iss claim. Even though the iss claim is an obvious claim that can be used to match on a specific JwtDecoder, I wonder if there are other use cases where more than iss claim would be required to match the JwtDecoder? I can't think of a use case at the moment but I would prefer an API that would support other use cases other than just iss. So Instead of supplying String as the key, we should try to be more flexible so this API can grow with other use cases. What are your thoughts on this?

@gburboz
Copy link
Author

gburboz commented May 3, 2019

@jgrandja these were my exact thoughts if we should make it more generic to support mapping based off any user specified claim. However, it was trade off between simplicity and flexibility so I decided to keep it simpler unless we see some demand for such use cases. Note that claims in JWT need not be plain String, they can be complex objects as well.

@gburboz
Copy link
Author

gburboz commented May 3, 2019

On a separate unrelated note iss claim value, we should consistently make as URI instead of URL or String because of it's semantics during equals comparison.

  • URL will use underlying Hostname/IP lookup which in some environments could give different results even for same hostname in URL and has overhead of DNS lookup.
  • We cannot even keep it as String because scheme and hostname part of it is expected to be case-insensitive while other parts are case sensitive.

Drawback is that issuer format is expected to be an URL and not any generic URI. This can be taken up as separate issue if there is interest in discussing this further.

@xsreality
Copy link

xsreality commented May 4, 2019

@gburboz I am working on a similar problem at my company where I need to be able to connect to multiple identity providers with unique issuer URLs. Your solution is promising but the underlying Map<String, JwtDecoder> decoderbyIssuer is initialized at startup. In the scenario of a new tenant added (with its own issuer), is there a way to update the map?

To address this, I came up with a solution that involves a registry of JwtDecoder. I customized the JwtConfigurer inside OAuth2ResourceServerConfigurer to accept a registry of decoders which is basically a wrapper around map of tenant identifier and JwtDecoder with methods to register/unregister decoders. What do you think?

I like your idea of parsing the iss from the token to identify the tenant. It is preferable to enforcing a tenant-identifying header or some other request attribute instead.

@gburboz
Copy link
Author

gburboz commented May 7, 2019

@xsreality your use case and solution is very interesting but I am not sure how common this use case is. I would suggest @jgrandja and @jzheaux to comment on if we want to bring this feature into JwtDecoder itself or not.

Alternative-1: Share instance of ConcurrentHashMap<String, JwtDecoder> decoderbyIssuer between JwtDecoder and a registry of your choice that can register/unregister decoders and update decoderbyIssuer on fly.

Alternative-2: Refactor MultiTenantJwtDecoder so that it will be possible to extend it with an implementation which has additional feature to register/unregister decoders on fly.

Also note that as per current architecture, there is more to decoders than just decoding the token. They also have delegates for authentication converter and token validators.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019
@jzheaux
Copy link
Contributor

jzheaux commented May 8, 2019

(@xsreality) To address this, I came up with a solution that involves a registry of JwtDecoder.
(@jgrandja) The class ... relies on iss claim.

Yes, I agree that there is an advantage to using a contract where the JwtDecoder can be resolved at runtime. ClientRegistrationRepository exists for this same reason for OAuth 2.0 Clients. I think Function<String, JwtDecoder> or Function<JWT, NimbusJwtDecoder> show some promise.

It is preferable to enforcing a tenant-identifying header or some other request attribute instead.

Can you clarify @xsreality which you are stating is preferable and why? Subdomains and paths are often used for tenant identification, and for good reason.

By the way, in cases where request material is used to identify tenants, AuthenticationManagerResolver is more suitable. In fact, I almost would recommend using that interface instead here, if there weren't the double-parsing concern.

NimbusJwtDecoder can optionally be modified so that it will have additional Jwt decode(JWT token) to avoid double parsing (JWT jwt = parse(token);).

I think the double-parsing is only a mild annoyance. It's common in multi-tenancy scenarios to need to parse something - the URL, headers, etc. - in order to identify the tenant.

That said, performance is important, and we want to avoid duplicate work, if possible. We could do that by instead introducing a factory method to NimbusJwtDecoder:

static JwtDecoder fromJwtDecoderResolver(Function<JWT, NimbusJwtDecoder> jwtDecoderResolver) {
    return token -> {
        JWT jwt = parse(token);
        return jwtDecoderResolver.apply(jwt).decode(token, jwt);
    }
}

However, it was trade off between simplicity and flexibility

Good point, @gburboz. It can be tricky to know where to simplify with multi-tenancy. Intrinsic to a lot of multi-tenancy scenarios is the idea that tenants can be added and removed at runtime.

You might be able to achieve both simplicity and flexibility by separating the concerns:

Map<URI, NimbusJwtDecoder> jwtDecoders = // ...
JwtDecoder multitenant = NimbusJwtDecoder.fromJwtDecoderResolver(
          jwt -> jwtDecoders.get(URI.create(jwt.getIssuer().toExternalForm())));

@bertramn
Copy link

I am not very close to the spring oauth code, so perhaps treat the comment as user feedback ;).

If multi-tenancy can't easily be added, maybe the current implementation is not aligned to the token processing stages of a resource server.

We had this need a year ago (#5351) and ended up implementing a bunch of components including an authentication filter along these lines:

  1. parse token
  2. extract claim(s) from token to select issuer (default iss claim)
  3. setup a validation processing context
  4. validate the parsed token
  5. map principal and other claims from parsed token to security context

The parts to make 1-4 work in a default implementation are already present in the nimbus library. We basically implemented a custom nimbus SecurityContext and added the criteria for key selection extracted in step 2. The SecurityContext is then provided together with the parsed JWT to JWTProcessor#process. The validation step then offers the SecurityContext to a custom JwkLocator which used the context information to retrieve and present the right JWKs to the processor for validation.

Note: I have the locator facade present the entire JWK set of the resolved issuer to the processor. That way the processor can make the final JWK selection based on the kid to support key rotation. Failing that, a developer would need to (re)implement parts of a standard spec.

Seems to me the missing link in your design centres around the processing context (or lack thereof) passed from parse token stage to the validation stage and the delegation of the key selection to a configurable component which has access to parse stage information to offer the correct JWKs to the processor. Step 5 gets a bit more tricky as in multi-tenant environments you will have to normalise the claims of all issuers to something that your spring app understands (as in hard codes in the auth annotations). But claims mapper configuration could be attached to the JWK selector.

Not sure if that makes sense, but sometimes less is more.

@gburboz
Copy link
Author

gburboz commented Jun 1, 2019

@jzheaux and @jgrandja do we have any update or direction how we would want to proceed with this issue?

@jzheaux
Copy link
Contributor

jzheaux commented Jun 12, 2019

@bertramn these are good points - often times, less is more. I agree that key selection by issuer is likely the primary use case.

@gburboz do you have a concrete use case for composing of completely custom JwtDecoder implementations and selecting them by issuer?

For key selection, I think a simpler route would be to enhance the Nimbus API so that DefaultJWTProcessor sends the entire JWT down for key selection instead of just the JWSHeader - I've logged a ticket with Nimbus to see what they think about an enhancement along those lines. This would resolve the double-parsing issue and address the primary use case. Short of that, we could add support not dissimilar from what @bertramn has suggested, though I believe it's out of scope for this ticket.

For more complex scenarios, I lean towards an implementation of AuthenticationManagerResolver, since this is the target interface for multi-tenancy, instead of having a composite JwtDecoder. Additionally, as @xsreality pointed out, I think that a strategy for resolving the AuthenticationManager by the tenant will be important.

To @jgrandja's point, while it would be nice to be more generic, I think this is something we could pretty easily add later on in another ticket - maybe a resolver that takes any claim name.

@gburboz
Copy link
Author

gburboz commented Jun 17, 2019

@jzheaux , we have a need to be able to handle incoming JWTs from different OAuth Providers to be handled by single resource server. As off today claims mapping and token validations are also tied to JwtDecoder, hence the need. Do you think I should explore AuthenticationManagerResolver if that could help solve my requirement here?

@jzheaux
Copy link
Contributor

jzheaux commented Jun 18, 2019

@gburboz Possibly, but if you don't mind, let's explore another option first.

Nimbus 7.3 ships with the ability to resolve a JWK set by the issuer claim, e.g.:

Map<String, JWSKeySelector> keySelectors = ...;
JWTClaimSetAwareJWSKeySelector selector = (header, claims, context) -> {
    String issuer = claims.getIssuer();
    return Optional.ofNullable(keySelectors.get(issuer))
        .map(keySelector -> keySelector.selectJWSKeys(header, context))
        .orElseThrow(() ...);
};
ConfigurableJWTProcessor<?> processor = new DefaultJWTProcessor<?>();
processor.setJWTClaimSetAwareJWSKeySelector(selector);

In Spring Security 5.2, you can supply your own JWTProcessor instance, so you could then do:

NimbusJwtDecoder decoder = new NimbusJwtDecoder(processor);

As for the validator and claims mapper, do you need tenant isolation (for example, can each tenant describe their own unique validation rules)? If so, you could use a similar strategy as above, just for OAuth2TokenValidator<Jwt> and MappedJwtClaimSetConverter and then do:

decoder.setJwtValidator(multiTenantJwtValidator);
decoder.setClaimSetConverter(multiTenantClaimSetConverter);

This is more verbose, but it will be more efficient since the JWT won't be parsed multiple times.

Would the above give you the tenant isolation that you need?

@jzheaux jzheaux self-assigned this Jul 1, 2019
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 11, 2019
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jul 18, 2019
@spring-projects-issues
Copy link

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Jul 25, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Dec 17, 2019

@gburboz @xsreality @bertramn Would you comment on how well this implementation would address your multi-tenancy use cases?

The essence of the idea is:

JwtAuthenticationManagerResolver resolver = 
    new JwtAuthenticationManagerResolver("whitelist", "of", "issuers");

or

Converter<String, AuthenticationManager> issuerManagerConverter = // ...
JwtAuthenticationManagerResolver resolver =
    new JwtAuthenticationManagerResolver(issuerManagerConverter);

Where said Converter could be Map#get, Repository#load, etc.

UPDATE - Actually, let's move the discussion about this particular idea over to the PR that implements it - #7733

@RavindraSengar
Copy link

RavindraSengar commented Oct 13, 2020

Hi @jzheaux

I have similar use case.
One Backend API endpoint/resource
2 consumers of this BE API both will have their corresponding OAUTH2/OKTA authorization servers. Each with a different RSA key configured for creation of the JWT tokens.
In BE API, we want to accept tokens from either auth server and authenticate.
Problem - BE API/Resource is configured to use only 1 set of security.oauth2.resource.jwk.key-set-uri and security.jwt.issuer Is there a way we can mention 2 set of security.oauth2.resource.jwk.key-set-uri and security.jwt.issuer ? Or how to authenticate tokens from 2 different OKTA/OAUTH authorization servers ?

Project setup Details -

org.springframework.boot" version "2.3.1.RELEASE"
io.spring.dependency-management" version "1.0.8.RELEASE"
springSecurityVersion = "5.3.3.RELEASE"

I have gone through this support documentation but need some help to implement this.
Not sure this is right place to ask this question. If this is not right place to seek help, please let me know correct way.
Thanks.

@jzheaux
Copy link
Contributor

jzheaux commented Oct 13, 2020

Hi, @RavindraSengar. Spring Boot does not at this time support listing multiple issuers as application properties.

Spring Security does support multiple issuers, though, with JwtIssuerAuthenticationManagerResolver, which sounds like it will suit your needs.

To address your other question, we prefer Stack Overflow questions since it's a common forum for usage questions. If you've got further questions, please feel free to ask them there and paste the SO link here, if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants