spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.86k stars 5.92k forks source link

Provide Resource Server Configurer #5226

Closed jzheaux closed 6 years ago

jzheaux commented 6 years ago

Summary

HttpSecurity should support the configuration of an application to operate as a resource server, e.g.

http
    .oauth2().resourceServer()

Relates to #4887

jzheaux commented 6 years ago

I'm moving the discussion about the structure if the DSL here. I'll begin with a restatement of what I last "officially" proposed and then explain some revisions, which are based on team feedback.

Initial Proposal

A typical minimal configuration would look like this under the current proposal:

http
    .oauth2().resourceServer()
        .accessTokens()
            .formats().jwt();

And a complete configuration would look like this:

http
    .oauth2().resourceServer()
        .bearerTokenResolver(customBearerTokenResolver())
        .accessTokens()
            .verifiers()
                .signature().keys("file:///the-location-of-my-key")
                .encryption().keys("http://a-remote-jwks-uri")
                .addVerifier(customTokenVerifier()).and()
            .formats()
                .jwt().processor(customJwtProcessor()).and()
                .opaque().processor(customOpaqueTokenProcessor());

Updated proposal

A number of changes have been suggested, and rather than try and represent everyone's feedback perfectly, I'll invite them to comment on the following updated idea, which incorporates the points that, at least for me, made sense:

http
    .oauth2().resourceServer()
        .jwt()
            .signature().keys("https://jwks.json");

or

http
    .oauth2().resourceServer()
        .bearerTokenResolver(customBearerTokenResolver())
        .jwt()
            .processor(customJwtProcessor())
            .verifiers(oneCustomVerifier(), anotherCustomVerifier());

The above would be a highly custom circumstance where the code needs to customize the way the bearer token is resolved from the request, the way the token is processed into a verified JWT, and has custom ways for verifying the resulting JWT. Verifying now needs to be couched underneath the format because the verifier contract is typed by the token format.

Do we need processor()?

One concern that was brought up was whether or not processor was strictly necessary. I think we could also do something like the following and address my concern, if a little less readably:

JwtAccessTokenAuthenticationProvider provider =
    new JwtAccessTokenAuthenticationProvider(customJWTProcessor(), customVerifiers());

http
    .oauth2().resourceServer()
        .jwt(provider);

The argument for stuff like this has been that it makes so that we can stick with spec language in the DSL while still allowing folks to provide custom implementations that may not read like the spec.

For example, we may need an authorities mapper in JwtAccessTokenAuthenticationProvider, but authorities mapper is not "spec language". So instead of having .jwt().authoritiesMapper(customAuthoritiesMapper()), I would instantiate the provider outside of the DSL and provide it in jwt(JwtAccessTokenAuthenticationProvider).

Even with that, though, I would assert that the way that a token is turned into a Java type is what the JWT spec refers to with the term "processing". So, I don't see:

http.oauth2().resourceServer().jwt().processor(customJWTProcessor())

as a departure from spec language.

Minimal Example

A more typical example would look like:

http
    .oauth2().resourceServer()
        .jwt()
            .signature().keys("https://jwks.json");

Where the configurer would know that it is a URL instead of a raw key by parsing the string. Note that .signature() and .processor() would be mutually exclusive as "process" implies a complete take-over of how to translate the token into a Java type.

jwkSetUri()

Another view of this same approach is:

http
    .oauth2().resourceServer()
        .opaque()
            .introspectionEndpoint(url("https://introspector").connectionTimeout(30000))
            .verifiers(customVerifier())
            .and()
        .jwt()
            .signature()
                .algorithm("EC521")
                .keys(url("https://jwks.json").readTimeout(30000));

I'm advocating that the jwks URI not be elevated to the same level as the introspection URI because it seems to me that there are may ways to retrieve the signature verification key, JWK being just one of many, though, I wouldn't mind supporting jwkSetUri as well as signature, the first implying a stronger Spring Security opinion than the second, sort of in the same way that processor would be a still weaker opinion than both jwkSetUri and signature.

accessTokens() layer

I'm still going back and forth with accessTokens(), which is an extra layer between resourceServer() and jwt(), opaque(), and verifiers(). Is it unnecessary typing for the user? http.oauth2().resourceServer().jwt() has the same feel as http.httpBasic(). We wouldn't do http.credentials().httpBasic(), which is why, for now, I lean towards removing accessTokens() (and formats() also, for the same reason).

jzheaux commented 6 years ago

@rwinch, @jgrandja I've had a bit more time to work with the DSL, and I've got the following as options:

http.oauth2().resourceServer()
    .jwt().signature().keys(url("https://jwks.json"));

This is currently the simplest configuration possible. I don't recommend going simpler than this as it would involve inventing a convention for where on the filesystem to find keys or making non-spec assumptions about the value of the JWK set uri.

Of course, the following should be possible:

http.oauth2().resourceServer();

@Bean
public JwtDecoder decoder() {
    return new MyJwtDecoder();
}

in which case we could assume that the code wants to use the bearer token support based on the existence of a JwtDecoder bean.

Also, this is possible:

http.oauth2().resourceServer().jwt(new MyJwtDecoder());

Which is simply the DSL analog for the bean wiring.

One comment that was made early on is that the DSL should make it as clear as possible what is a reasonable configuration and what isn't. To that end, I think that the key configuration should be strongly-typed and should be done in such a way that only one key configuration is possible. For example:

http.oauth2().resourceServer()
    .jwt().signature().keys(myKeyMap())

Would return the parent configurer (JwtConfigurer) instead of the current configurer (SignatureConfigurer). In this way, a sequence of keys calls isn't possible. This could possibly box us in if we have other confgiuration methods off of signature() in the future, so I don't mind backing off of that. It does make the DSL read better, though, IMHO.

These are the three key signatures that I am planning on (and have corresponding samples for your perusal):

.keys(String kid, Key key);
.keys(Map<String, Key> keys);
.keys(UrlConfigurer url)

There is a bit to unpack here.

First, I don't want to deal with byte[] or String-specified keys. In the local case, folks can retrieve their keys however they want, parse them into java.security.Keys and then deposit them here in the configuration.

Second, this is kid-centric. Some of the early feedback I received was that it isn't uncommon to completely ignore the kid and just iterate through the list based on custom criteria. To that end, I originally also had this in mind:

.keys(myKeyProvider())

but I've left it out thus far as I'm still mulling over the contract and running into some roadblocks with Nimbus. Feedback is of course welcome.

Third, is the url. I brought up a concern early on that I could not easily tweak the timeouts for the JWK set url. We need to provide this somewhere, and I'm proposing introducing a UrlConfigurer class that can be reused across the DSL to describe URLs. Some early feedback I received from Rob was a concern about the DSL becoming overly sophisticated relative to something that, in the end, isn't really Spring Security's concern (namely URL connections). What I'm proposing here is a separate class where such configuration can be isolated. And, if that is still unsettling for folks, I can see the keyProvider approach being another possibility.

Here is a slightly more sophsisticated example:

http.oauth2().resourceServer(a(), list(), of(), audiences())
    .bearerToken(myResolver())
    .jwt()
        .algorithm(JwsAlgorithms.RS256)
        .signature().keys(myJwkUrl())
        .encryption().keys(myPrivateKeyMap())
        .verifications(my(), jwt(), verifiers())
        .grants(myAuthorityGranter())
        .and()
    .opaque()
        .token(myTokenUrl())
        .verifications(my(), opaque(), verifiers())
        .and()
    .and()
.csrf()
// ....

I like the pattern keys and then provide a way to get the keys, and it seems like a good match for bearerToken, say, as a way to get the bearer token. It was originally bearerTokenResolver. I don't have a really strong opinion here if someone feels that bearerTokenResolver is clearer. Same goes for verifications (vs verifiers) and grants (vs authorityGranters).

Originally, there were verifiers at the resourceServer() level, though I like the value of passing a richer object through the verify method. This means, semantically, that verifiers will be tied to the token format.

Lastly, we need a way to give the resource server a list of supported audiences. I'm okay with an ids method off of resourceServer():

resourceServer().ids(a(), list(), of(), audiences())

Though placing them in the resourceServer() method also felt natural.

jgrandja commented 6 years ago

The minimal configuration looks good. This configuration will meet the requirements for #5130.

http
  .oauth2()
    .resourceServer()
      .jwt().signature().keys(url("https://jwks.json"));

An alternative could be:

http
  .oauth2()
    .resourceServer()
      .jwt().jws().keys(url("https://jwks.json"));

This naming would align with the spec language for JSON Web Signature (JWS). However, I think signature() may be more clear as users might not understand what jws represents. Either way, throwing it out there.

I also like:

http
  .oauth2()
    .resourceServer()
      .jwt().encryption()...

For JSON Web Encryption (JWE) support...when we get to it.

Alternatively:

http
  .oauth2()
    .resourceServer()
      .jwt().jwe()...

Again, I think encryption() may be more clear for the same reasons as signature().

Either way, JWE is not in scope for 5.1 but at least we know that the DSL can grow with this design structure.

Third, is the url. I brought up a concern early on that I could not easily tweak the timeouts for the JWK set url. We need to provide this somewhere, and I'm proposing introducing a UrlConfigurer class that can be reused across the DSL to describe URLs. Some early feedback I received from Rob was a concern about the DSL becoming overly sophisticated relative to something that, in the end, isn't really Spring Security's concern (namely URL connections).

I agree with @rwinch here, we should remove the UrlConfigurer and associated readTimeout and connectTimeout. We are aware that we need a way to configure the connect/read timeout but we need to come up with a consistent way across the board. We face the same issue with NimbusJwtDecoderJwkSupport, NimbusUserInfoResponseClient and NimbusAuthorizationCodeTokenResponseClient. For now we hard-coded 30 secs for both connect/ready timeout until we come up with a consistent strategy for configuring the underlying HTTP clients. I would simplify it to something like this:

http
  .oauth2()
    .resourceServer()
      .jwt()
         .signature().keys(String keysUri);

or maybe...

http
  .oauth2()
    .resourceServer()
      .jwt()
         .jwk().keys(String keysUri);

In regards to this configuration:

.keys(String kid, Key key);
.keys(Map<String, Key> keys);

We cannot rely on kid as a mapping as this is an OPTIONAL parameter in the spec. Also, I don't really see the need for this mapping either.

I believe the only configuration points we need for 5.1 is something like this:

signature().keys(String keysUri);
signature().keys(java.security.PublicKey... pk);

This would meet the requirements for #5130 and #5131.

I'm still mulling over the contract and running into some roadblocks with Nimbus

Can you elaborate on the roadblocks, maybe I can help?

I don't have a really strong opinion here if someone feels that bearerTokenResolver() is clearer.

I prefer bearerTokenResolver() as it's clear to me that it's the resolver. Having just bearerToken() almost reads that you configure the Bearer token here?

Originally, there were verifiers at the resourceServer() level, though I like the value of passing a richer object through the verify method. This means, semantically, that verifiers will be tied to the token format.

I'm not convinced this is the best strategy to go with. If we provide a more strongly typed configurer for the various claims, for example audiences(), than where do we draw the line? There are so many standard claims, for example iss, aud, exp, nbp, azp, etc. If we provide for aud do we provide for all these other ones as well? The data types vary for each claim as well, either String or String[] or Date etc. which complicates things even more.

This simple configuration point provides all the flexibility we need to provide:

http
  .oauth2()
    .resourceServer()
      .verifiers(OAuth2TokenVerifier... verifiers)
public interface OAuth2TokenVerifier<T extends AbstractOAuth2Token> {
    void verify(T token) throws OAuth2AuthenticationException;
}

We can provide an implementation as such:

public class AudienceTokenVerifier implements OAuth2TokenVerifier<Jwt> {
    private List<String> restrictedAuds;

    public AudienceTokenVerifier(List<String> auds) {
        this.restrictedAuds = auds;
    }

    @Override
    public void verify(Jwt token) throws OAuth2AuthenticationException {
        List<String> tokenAuds = token.getClaimAsStringList(JwtClaimNames.AUD);

        // TODO Compare tokenAuds to this.restrictedAuds
    }
}
http
  .oauth2()
    .resourceServer()
      .verifiers(new AudienceTokenVerifier(Arrays.asList('aud1', 'aud2')));

We can take this further and have Spring Boot specific property:

spring.security.resourceserver.aud: aud1, aud2, aud3

The auto-config would read this property and create a AudienceTokenVerifier and configure it via

http
  .oauth2()
    .resourceServer()
      .verifiers(audienceTokenVerifer);

This same pattern can be followed for any standard claim as well as non-standard claim.

I feel like we need to keep the DSL simple as such:

http
  .oauth2()
    .resourceServer()
      .verifiers(OAuth2TokenVerifier... verifiers)

And let Spring Boot do it's typical magic using the pattern as described above.

For non Spring Boot users, they can configure the various OAuth2TokenVerifier implementations that we provide and configure it themselves.

Does this make sense?

jzheaux commented 6 years ago

+1 to @jgrandja's comments. A couple of clarifications:

I'm not convinced this is the best strategy to go with. If we provide a more strongly typed configurer for the various claims, for example audiences(), than where do we draw the line?

I wasn't clear here, but I think one of your examples points out my concern. Your AudienceTokenVerifier only works with Jwts, correct? So, wouldn't it be more correct to specify it underneath jwt()?

Yes, I like the idea of audiences just being verifiers. That's clean. I can see the value in placing them at the top of the config, if you can help resolve my above concern.

jzheaux commented 6 years ago

Regarding KeyProvider, Nimbus does have a hook for this (paraphrase):

List<? extends Key> selectJWSKeys(JWSHeader jwsHeader, SecurityContext context)

And KeyProvider could hypothetically tie into this like so:

interface KeyProvider {
    List<? extends Key> provide(Map<String, Object> header)
}

(jwsHeader, context) ->
    keyProvider.provide(new LinkedHashMap<>(jwsHeader.toJSONObject()))

So, I guess it's not so much of a roadblock as a question. It seems like this same interface would be valuable for encryption, when retrieving keys to decrypt with. Nimbus's contract here is JWS-specific (which is fine), but it just makes me wonder if doing Map<String, Object> is overgeneralizing and if it would be better to have a dedicated object.

I do like doing a key provider as we can represent several behaviors that way, e.g. the single key configuration can use it, too.

jgrandja commented 6 years ago

@jzheaux

I wasn't clear here, but I think one of your examples points out my concern. Your AudienceTokenVerifier only works with Jwts, correct? So, wouldn't it be more correct to specify it underneath jwt()?

Let's say we have:

public class AudienceJwtTokenVerifier implements OAuth2TokenVerifier<Jwt> {
    private List<String> restrictedAuds;

    public AudienceJwtTokenVerifier(List<String> auds) {
        this.restrictedAuds = auds;
    }

    @Override
    public void verify(Jwt token) throws OAuth2AuthenticationException {
        // TODO

    }
}

// NOTE: OpaqueAccessToken extends AbstractOAuth2Token
public class AudienceOpaqueTokenVerifier implements OAuth2TokenVerifier<OpaqueAccessToken> {
    private List<String> restrictedAuds;

    public AudienceOpaqueTokenVerifier(List<String> auds) {
        this.restrictedAuds = auds;
    }

    @Override
    public void verify(OpaqueAccessToken token) throws OAuth2AuthenticationException {
        // TODO

    }
}

The user would create the verifiers as such:

OAuth2TokenVerifier[] verifiers = {
        new AudienceJwtTokenVerifier(Arrays.asList("aud1", "aud2")),
        new AudienceOpaqueTokenVerifier(Arrays.asList("aud1", "aud2"))
};

And than configure it:

http
  .oauth2()
    .resourceServer()
      .verifiers(verifiers)

The signature for .resourceServer().verifiers() would be:

public <T extends AbstractOAuth2Token> **Configurer<B> verifiers(OAuth2TokenVerifier<T>... verifiers)

This would work. However, I don't like the fact that the Audience OAuth2TokenVerifier's are duplicating logic. Either way, the .verifiers() at the .resourceServer() level works.

However, you got me thinking if it's better to put it under jwt() and opaque()? Something to think about.

jgrandja commented 6 years ago

@jzheaux I'm not sure if we need this:

interface KeyProvider {
    List<? extends Key> provide(Map<String, Object> header)
}

The user would configure one or more keys via:

...keys().signature().keys(java.security.PublicKey... pk)

and the ResourceServerConfigurer would associate those keys with a new JwtDecoder implementation. This implementation would convert each java.security.PublicKey to a com.nimbusds.jose.jwk.JWK and then the com.nimbusds.jose.jwk.JWK(s) would be associated to a com.nimbusds.jose.jwk.source.ImmutableJWKSet. The ImmutableJWKSet would than be the source for JWSKeySelector which is used by the DefaultJWTProcessor. This would result in a very similar implementation as NimbusJwtDecoderJwkSupport but would use the ImmutableJWKSet.

Dig deep into the Nimbus API's. There is a way to convert java.security.PublicKey to com.nimbusds.jose.jwk.JWK. See RSAKey.load() to start.

jzheaux commented 6 years ago

It seems to me that if I can provide more than one key, then I will also want to provide a heuristic for how to select which key. A hook like KeyProvider allows the user to do that without prescribing the Nimbus API to the user.

Also, it isn't clear to me why I would go from a Key to a JWK. It seems like I would want to go the other way, e.g retrieve a Key from some source, be it a JWK or something else. KeyProvider is a "something else" here. What is the advantage of the direction you are suggesting?

jgrandja commented 6 years ago

It seems to me that if I can provide more than one key, then I will also want to provide a heuristic for how to select which key.

How about we start with one key instead of multiple:

keys().signature().keys(java.security.PublicKey pk)

Let's work through this case first and than we can take the next step and possibly expose multiple input keys. Let's keep it simple to start.

Also, I want us to take advantage of Nimbus JWSVerificationKeySelector for Key selection. If we start providing an API for key selection it will get complicated. The JWSVerificationKeySelector is capable of selecting a key based on the JWS headers.

See this in the spec for further info:

6. Key Identification Appendix D. Notes on Key Selection

Also, it isn't clear to me why I would go from a Key to a JWK

Converting from a java.security.PublicKey to a com.nimbusds.jose.jwk.JWK allows us to take advantage of Nimbus API's, as mentioned the JWSVerificationKeySelector which uses JWKSource (or ImmutableJWKSet).

com.nimbusds.jose.jwk.JWK is a representation of a java.security.Key com.nimbusds.jose.jwk.RSAKey is a representation of a java.security.interfaces.RSAPublicKey and java.security.interfaces.RSAPrivateKey

The JOSE framework operates on JWK's. In this way, it makes sense why you would convert a java.security.PublicKey to a com.nimbusds.jose.jwk.JWK so that you can leverage the library features.

Does this make sense?

jzheaux commented 6 years ago

Also, I want us to take advantage of Nimbus JWSVerificationKeySelector for Key selection. If we start providing an API for key selection it will get complicated.

I don't think I am proposing this. While I understand that we, Spring Security want to leverage Nimbus's sophistication, we need to be careful to not be too constrained on how we project that out to the user. A key provider allows the user the flexibility to do what he needs to do in the circumstance that we have not anticipated his need (very likely).

I actually don't see this as a departure from how Nimbus is designed. When configuring a JWT processor, we specify a JWSKeySelector which simply returns a list of Keys, how ever they were obtained.

One way to get this list of keys is by giving it a JWK Set Url in which case Nimbus provides ample support.

Another is to simply use one key. Because the final result of all of Nimbus's support is just a List of Keys (as returned by JWSKeySelector), I don't see the benefit of going through more of Nimbus's plumbing, e.g. I give Nimbus a single key so that it can turn it into a JWK so that it can turn it back into the Key I gave it? I don't see it (though this also may not be what you are saying). In addition, the JWSKeySelector API is completely unaware of JWKs for a reason, I think--I'm not sure the intent of Nimbus is to push all keys through the JWK representation.

Another is whatever way the user feels is necessary--query a custom endpoint, derive from custom headers--which I don't really need to anticipate other than to hand her the header, though I understand what you mean about addressing the single key issue first. I think I would really only argue for one official implementation at this point which is SingleKeyProvider.

jzheaux commented 6 years ago

Here are a few more thoughts on verifiers (which I'm henceforth going to refer to as validators).

Here is one way that validators could be wired:

http.oauth2().resourceServer()
    .jwt()
        .validators(
            new JwtAccessTokenValidator(Duration.ofSeconds(30)),
            new AudienceValidator(...),
            new JwtClaimValidator(...));

This is nice in that it is pretty easy to remain passive, e.g. we just accept a list of validators and call it good.

There are several places in the DSL that, instead of accepting a list, accept a chain of individual invocations, e.g. antMatchers, authenticationProviders, etc. So, I like this:

http.oauth2().resourceServer()
    .jwt()
        .validator(new JwtAccessTokenValidator(Duration.ofSeconds(30)))
        .validator(new AudienceValidator(...))
        .validator(new JwtClaimValidator(...));

I think it is more readable in addition to being potentially more familiar for those already acclimated to the Spring Security DSL.

I also like this:

http.oauth2().resourceServer()
    .jwt()
        .timestamps().areValidWithin(30).seconds()
        .audience().is(...)
        .issuer().is(...)
        .custom("custom-claim").is(...)
        .validator(new MyCustomValidator(...));

I think that this is even more readable, but it does commit the DSL to more JWT semantics, making it bigger overall. I don't think it is a problem to include support for each claim in the JWT spec, e.g. sub, aud, iss, etc., though I don't really see the value and would prefer not to--I don't really see ppl going beyond audience, issuer, and custom claims before they just create their own validator.

I also pondered "and" and "or" validators, but I'm of the opinion to simply encourage folks to create their own validator if they need something that sophisticated.

An alternative that I'm also playing with (which I don't really like, but am happy to get feedback on) is:

http.oauth2().resourceServer()
    .jwt()
        .validators(
            timestamps().areValidWithin(30).seconds(),
            audience().in(...),
            issuer().is(...));

I think in the above case, I'm trying too hard to live in both words and it comes out feeling choppy. If it inspires feedback from the community, though, I'm happy to hear it.

jzheaux commented 6 years ago

The other thing that I'm just keeping an eye out for is resource servers that need to support more than one signature type. Today, the code doesn't seem to support this very well, and so it's not one of my immediate concerns, but here is a thought for the future:

http.oauth2().resourceServer()
    .jwt()
        .having(JwsAlgorithms.RS256)
            .signature().keys(publicKey())
            .issuer(...).and()
        .having(JwsAlgorithms.HS512)
            .signature().keys(symmetricKey())
            .issuer(...).and() // applies just to HS512 JWTs
        .audience() // applies to everyone
        .authoritiesGranter(...)
        .scopeAttributeName(...);
Clockwork-Muse commented 6 years ago
http.oauth2().resourceServer()
    .jwt()
        .validator(new JwtAccessTokenValidator(Duration.ofSeconds(30)));

...whatever the way to validate time looks like, would it be possible to provide a Clock? For ease of testing, mostly. I'd anticipate it defaulting to Clock.systemUTC(), of course (uh, speaking of which, does spring provide a default clock instance anyways?).

jzheaux commented 6 years ago

@Clockwork-Muse that sounds like it could be a helpful feature though I'd be worried about devs hanging themselves with flexibility like that.

Also, note that this might not be enough to get what you want since our backing implementation (Nimbus) might not have the appropriate hook (it does not use java.time yet), but there are probably ways around that if this is the right thing to do.

I've created an issue in our sandbox project that you can track: https://github.com/jzheaux/spring-security-oauth2-resource-server/issues/57