Resource Server JWK support #5476

jzheaux · 2018-07-02T19:25:07Z

Introduce support for OAuth 2.0 Bearer Token Authentication with JWT-encoded bearer tokens. This includes support for remote JWK set signature verification.

#5121
#5125
#5128
#5130
#5237

jzheaux · 2018-07-02T22:37:58Z

...g/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java

+		this.mvc.perform(get("/").with(bearerToken(token)))
+				.andExpect(status().isUnauthorized())
+				.andExpect(invalidTokenHeader("An error occurred while attempting to decode the Jwt: " +
+						"Unsecured (plain) JWTs are rejected, extend class to handle"));


Separate pull request out to override this error message - #5459

jzheaux · 2018-07-02T22:46:52Z

...2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderJwkSupport.java

@@ -119,8 +121,18 @@ public Jwt decode(String token) throws JwtException {

 			jwt = new Jwt(token, issuedAt, expiresAt, headers, jwtClaimsSet.getClaims());

+		} catch ( RemoteKeySourceException jwkFailure ) {
+			if ( jwkFailure.getCause() instanceof ParseException ) {
+				throw new JwtException("An error occurred while attempting to decode the Jwt: Malformed Jwk set");


This Nimbus exception message reveals the underlying jwk set or the contents of whatever page it is pointed at, so this catches and overrides it with a less-revealing message.

jzheaux · 2018-07-02T22:47:52Z

...2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderJwkSupport.java

 		} catch (Exception ex) {
-			throw new JwtException("An error occurred while attempting to decode the Jwt: " + ex.getMessage(), ex);
+			if ( ex.getCause() instanceof ParseException ) {
+				throw new JwtException("An error occurred while attempting to decode the Jwt: Malformed payload");


This Nimbus exception message reveals the underlying jwt contents.

rwinch · 2018-07-03T15:36:29Z

samples/boot/oauth2resourceserver/src/main/resources/application.yml

@@ -0,0 +1,4 @@
+server:
+  port: 8081


Is there a reason that we have the port explicitly provided vs using 8080?

Was actually just trying to stay out of the way--several authorization servers use 8080 by default (e.g. uaa, keycloak).

Of course, this is very easy for the user to edit. Do you prefer to remove it in favor of having a more minimal example?

Yes. We default all our samples to 8080. If you like you could place instructions in the README on how to customize the port.

rwinch · 2018-07-03T15:37:09Z

samples/boot/oauth2resourceserver/src/main/java/sample/provider/MockProvider.java

+ *
+ * @author Josh Cummings
+ */
+public class MockProvider implements EnvironmentPostProcessor {


Please create a ticket to switch to using a static public key by default and provide a link here. If there isn't already an issue, please create another ticket for supporting a static key.

fyi - #5486

Interesting approach on mocking the Jwk Set endpoint. However, I'm wondering if this will confuse the user as the EnvironmentPostProcessor is not intended for starting up a mock server. I'm wondering if there is another approach, for example, an ApplicationContextInitializer via SpringApplication.addInitializers().

@rwinch What are your thoughts?

rwinch · 2018-07-03T15:40:27Z

samples/boot/oauth2resourceserver/src/main/java/sample/provider/MockProvider.java

+				if ("/.well-known/jwks.json".equals(request.getPath())) {
+					return JWKS_RESPONSE;
+				}
+


When are these other response types used? If they are not used anywhere, I think it would be valuable tot just always return the JWKS_RESPONSE and remove all the other responses.

The other two responses are so that the reader can follow all the instructions in the readme regardless of the authorization server she is using (mock or otherwise). Specifically, she can hit the /token endpoint for either a token that has no scopes or a token that has the message:read scope from the command-line.

While your comment about hardcoding the token in the instructions obviates the technical need for the mock's /token endpoint, the endpoint is still instructive for folks drawing the conceptual bridge from this sample to using their real authorization server.

For example, a user could write a simple bash script that retrieves a token, sets it to an environment variable, and then requests the endpoint using that token. Now, the user can point that script at either the mock authorization server or their own authorization server.

Does this step out of the bounds of the purpose of the sample?

I think for the sake of simplicity we should just give the user the token rather than pretend the mock server is a valid authorization server. The MockProvider is not a valid provider, nor do we want it to be. It does not validate the client id / secret, it does not validate the request, etc. Instead, leave this for a step 2 in the using a real Authorization Server steps. This helps the user build their knowledge gradually vs trying to force them to learn too much at once. It also avoids confusing them since the MockProvider is not a valid authorization server.

rwinch · 2018-07-03T15:42:01Z

samples/boot/oauth2resourceserver/src/main/java/sample/provider/MockProvider.java

+		if (uri.startsWith("mock://")) {
+			try {
+				this.server.start(new URI(uri).getPort());
+			} catch (IOException | URISyntaxException e ) {


Use URI.create to avoid needing to handle the URISyntaxException

rwinch · 2018-07-03T15:50:25Z

samples/boot/oauth2resourceserver/src/main/java/sample/provider/MockProvider.java

+			200);
+
+	private static final MockResponse NOT_FOUND_RESPONSE = response(
+			"{ \"message\" : \"Heyo, to hit this mock server you should either GET /.well-known/jwks.json or POST /token\" }",


I'd try making this message more informative. I'd also avoid using language like "Heyo" which could be confusing to someone who is not a native English speaker.

rwinch · 2018-07-03T19:42:29Z

...vy/org/springframework/security/config/annotation/web/configurers/CsrfConfigurerTests.groovy

@@ -507,6 +509,108 @@ class CsrfConfigurerTests extends BaseSpringSpec {
 		}
 	}

+	def 'ignoring request matchers augment configured request matcher'() {


Please do not create new Groovy tests. Instead you can create another Java based test with the new tests in it. Sometimes I will rename the old Groovy tests to end in Spec to avoid duplicate classes with the same name.

This comment should be applied generally to all code going into Spring Security (i.e. make this change for all .groovy files that were edited in this PR)

Ah, okay, sure. I can move these easily to their Java equivalent.

I see that there is a class called CsrfConfigurerNoWebMvcTests. Was going to create CsrfConfigurerJavaTests (to differentiate from the groovy CsrfConfigurerTests) before I saw this.

What convention should I be shooting for? For example, is naming it CsrfConfigurerRequestMatcherTests more appropriate?

rwinch · 2018-07-03T19:44:56Z

core/src/main/java/org/springframework/security/core/TransientAuthentication.java

+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface TransientAuthentication {
+}


Please create this as separate ticket and link to that ticket. Users will be interested to know about this separately. The separate commit can remain in this PR, but please make it a separate ticket and commit.

fyi - #5481

rwinch · 2018-07-03T19:50:00Z

...pringframework/security/oauth2/server/resource/authentication/JwtAuthenticationProvider.java

+
+	private static Collection<String> getScopes(Jwt jwt) {
+		for ( String attributeName : WELL_KNOWN_SCOPE_ATTRIBUTE_NAMES ) {
+			Object scopes = jwt.getClaims().get(attributeName);


Consider using jwt.getClaimAsStringList instead.

Ah, good catch.

Actually, it appears that this method only returns the list if it is already a list, otherwise, it returns null. I'm inclined to leave this for now and possibly submit an enhancement for getClaimAsStringList, maybe an overloaded method that takes a delimiter as a parameter?

rwinch · 2018-07-03T19:52:32Z

...java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java

+	@Override
+	public String resolve(HttpServletRequest request) {
+		String authorizationHeaderToken = resolveFromAuthorizationHeader(request);
+		String parameterToken = request.getParameter("access_token");


We should also fail if the parameter is included twice. This can be checked using request.getParameterValues("access_token")

rwinch · 2018-07-03T19:56:52Z

...java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

+	}
+
+	@TransientAuthentication
+	private static class SomeTransientAuthentication extends AbstractAuthenticationToken {


Add tests for meta annotations and super class that is annotated with TransientAuthentication

rwinch · 2018-07-06T15:15:19Z

...springframework/security/oauth2/server/resource/web/BearerTokenAuthenticationEntryPoint.java

+ */
+public final class BearerTokenAuthenticationEntryPoint implements AuthenticationEntryPoint {
+
+	private String defaultRealmName;


Is there a reason this is named defaultRealmName vs realmName? I think that this should be renamed to realmName since it is the realm name and not just a default. The property realmName is also more consistent with BasicAuthenticationEntryPoint.

jgrandja · 2018-07-09T14:48:52Z

...main/java/org/springframework/security/web/context/HttpSessionSecurityContextRepository.java

@@ -437,6 +444,14 @@ private HttpSession createNewSessionIfAllowed(SecurityContext context) {
 		}
 	}

+	private boolean isTransientAuthentication(Authentication authentication) {
+		if (authentication == null ) {


This check is not needed as authentication is never null based on the call path

jgrandja · 2018-07-09T16:05:43Z

...java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

+	@Target(ElementType.TYPE)
+	@Retention(RetentionPolicy.RUNTIME)
+	@TransientAuthentication
+	public @interface TestTransientAnnotation {


Rename to TestTransientAuthentication? The goal is to remove *Annotation from the name.

Rename to TestTransientAuthentication?

Yep, will do.

jgrandja · 2018-07-09T16:09:39Z

...java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

@@ -633,4 +619,102 @@ public void failsWithStandardResponse() {

 		repo.saveContext(context, request, response);
 	}
+
+	@Test
+	public void saveContextWhenStatelessAuthenticationThenSkipped() {


Consider renaming method to saveContextWhenTransientAuthenticationThenNotSaved

jgrandja · 2018-07-09T16:10:21Z

...java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

+	}
+
+	@Test
+	public void saveContextWhenStatelessAuthenticationSubclassThenSkipped() {


Consider renaming method to saveContextWhenTransientAuthenticationSubclassThenNotSaved

jgrandja · 2018-07-09T16:11:04Z

...java/org/springframework/security/web/context/HttpSessionSecurityContextRepositoryTests.java

+	}
+
+	@Test
+	public void saveContextWhenStatelessAuthenticationWithCustomAnnotationThenSkipped() {


Consider renaming method to saveContextWhenTransientAuthenticationWithCustomAnnotationThenNotSaved

jgrandja · 2018-07-09T20:46:54Z

...g/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java

+/**
+ * @author Josh Cummings
+ */
+public class OAuth2ResourceServerConfigurerTests {


There are a lot of tests which is great. But at the same time, quite long to consume. I'm wondering if there is a way to split up the tests into different classes based on some grouping/category?

Sure can. I've seen that in the codebase in a couple of places, though it appeared to be more tactical,--e.g. a place to put Java tests while we wait for the corresponding Groovy test class to be migrated--than strategic.

I think something like this is better-suited for a separate ticket. Thoughts?

I'm fine with another ticket. After looking at it again, it's not too long but I feel it will get long sooner than later so having a ticket to re-org it at a later point is fine with me.

jgrandja · 2018-07-09T20:52:56Z

...java/org/springframework/security/oauth2/server/resource/BearerTokenAuthenticationToken.java

+	 */
+	@Override
+	public Object getPrincipal() {
+		return this.getToken();


I'm not sure if this makes sense? Does the token really represent the Principal at this point given it's an Authentication Request?

I believe this comes down to whether or not the token is a principal at all. Note that UsernamePasswordAuthenticationToken, when used as an authentication request, does take the value from the constructor, set it as the principal, and returns it from getPrincipal. The difference is that username is more clearly a principal than token.

@rwinch and I were just discussing this one a few days ago. Another option is to return null. Really for me, the best Authentication would be one that is principal-less since Bearer Tokens are primarily about authority and incidentally about identity (say, when there is a sub).

Returning null clearly states that there is no "entity" that is part of this request, returning null just feels like a smell.

Thoughts, @rwinch ?

I still struggle with this myself, but I think at least or now we should leave as is. Perhaps we should create a ticket to consider this separately.

I personally feel getCredentials() should return the token and getPrincipal() should return "".
Logging a ticket to revisit is fine with me too.

jgrandja · 2018-07-09T20:54:40Z

...java/org/springframework/security/oauth2/server/resource/BearerTokenAuthenticationToken.java

+	}
+
+	/**
+	 * Get the <a href="https://tools.ietf.org/html/rfc6750#section-1.2" target="_blank">Bearer Token</a>


Remove the javadoc in all methods as it looks like copy-paste

Are we removing it because it is copy-paste? If I am reading a Javadoc, I want to know what a method does, and I won't know if it isn't described.

jgrandja · 2018-07-09T21:01:54Z

...ringframework/security/oauth2/server/resource/web/access/BearerTokenAccessDeniedHandler.java

+		Map<String, Object> attributes = token.getTokenAttributes();
+
+		for (String attributeName : WELL_KNOWN_SCOPE_ATTRIBUTE_NAMES) {
+			Object scopes = attributes.get(attributeName);


scopes could be null so should do null check first

I believe this is what instanceof does for me, no? e.g. A NullPointerException is not possible in the ensuing block of code.

Are you suggesting that this makes it more readable? I tend to see unnecessary null checks as extra verbosity, of which Java already has plenty. :)

jgrandja · 2018-07-09T21:06:53Z

...org/springframework/security/oauth2/server/resource/web/BearerTokenAuthenticationFilter.java

+	 * @param authenticationManager
+	 * @param authenticationEntryPoint
+	 */
+	public BearerTokenAuthenticationFilter(


Remove this constructor as it's only being used in a test.

jgrandja · 2018-07-10T01:16:38Z

samples/boot/oauth2resourceserver/src/main/java/sample/provider/MockProvider.java

+ *
+ * @author Josh Cummings
+ */
+public class MockProvider implements EnvironmentPostProcessor {


Interesting approach on mocking the Jwk Set endpoint. However, I'm wondering if this will confuse the user as the EnvironmentPostProcessor is not intended for starting up a mock server. I'm wondering if there is another approach, for example, an ApplicationContextInitializer via SpringApplication.addInitializers().

@rwinch What are your thoughts?

jgrandja · 2018-07-13T19:23:00Z

...ava/org/springframework/security/web/access/RequestMatcherDelegatingAccessDeniedHandler.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright 2002-2015 the original author or authors.


Update header to 2018

jgrandja · 2018-07-13T19:45:24Z

...config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurer.java

+
+		http.addFilterBefore(filter, BasicAuthenticationFilter.class);
+
+		if (this.jwtConfigurer != null) {


I spoke to @rwinch about this use case and he also agrees that this should be left out of this PR. Please log a new ticket for this.

jgrandja · 2018-07-13T19:48:43Z

...g/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java

+/**
+ * @author Josh Cummings
+ */
+public class OAuth2ResourceServerConfigurerTests {


I'm fine with another ticket. After looking at it again, it's not too long but I feel it will get long sooner than later so having a ticket to re-org it at a later point is fine with me.

This commit introduces support for transient authentication tokens which indicate to the filter chain, specifically the HttpSessionSecurityContextRepository, whether or not the token ought to be persisted across requests. To leverage this, simply annotate any Authentication implementation with @TransientAuthentication, extend from an Authentication that uses this annotation, or annotate a custom annotation. Implementations of SecurityContextRepository may choose to not persist tokens that are marked with @TransientAuthentication in the same way that HttpSessionSecurityContextRepository does. Fixes: spring-projectsgh-5481

Other configurers can now offer their preference on session creation policy without trumping what a user provided via the sessionCreationPolicy method. This is valuable for configurer's like Resource Server that would like to have session management be stateless, but not at the expense of the user's direct configuration. Fixes: spring-projectsgh-5518

This introduces an evolution on CsrfConfigurer#ignoreAntMatchers, allowing users to specify a RequestMatcher in the circumstance where more than just the path needs to be analyzed to determine whether CsrfFilter should require a token for the request. Simply put, a user can now selectively disable csrf by request matcher in addition to the way it can already be done with ant matchers. Fixes: spring-projectsgh-5477

This introduces the capability for users to wire denial handling by request matcher, similar to how users can already do with authentication entry points. This is handy for when denial behavior differs based on the contents of the request, for example, when the Authorization header indicates an OAuth2 Bearer Token request vs Basic authentication. Fixes: spring-projectsgh-5478

When Nimbus fails to parse either a JWK response or a JWT response, the error message contains information that either should or cannot be included in a Bearer Token response. For example, if the response from a JWK endpoint is invalid JSON, then Nimbus will send the entire response from the authentication server in the resulting exception message. This commit captures these exceptions and removes the parsing detail, replacing it with more generic information about the nature of the error. Fixes: spring-projectsgh-5517

Introducing initial support for Jwt-Encoded Bearer Token authorization with remote JWK set signature verification. High-level features include: - Accepting bearer tokens as headers and form or query parameters - Verifying signatures from a remote Jwk set And: - A DSL for easy configuration - A sample to demonstrate usage Fixes: spring-projectsgh-5128 Fixes: spring-projectsgh-5125 Fixes: spring-projectsgh-5121 Fixes: spring-projectsgh-5130 Fixes: spring-projectsgh-5226 Fixes: spring-projectsgh-5237

rwinch · 2018-07-16T16:56:14Z

Merged in via https://github.com/spring-projects/spring-security/commits/d5950988231d03c7669c5c9433efed0d1af0b406

jzheaux requested review from rwinch and jgrandja July 2, 2018 19:25

jzheaux commented Jul 2, 2018

View reviewed changes

jzheaux force-pushed the resource-server-jwks branch from ff0a6cd to 48cb2e9 Compare July 3, 2018 18:35

rwinch requested changes Jul 3, 2018

View reviewed changes

rwinch reviewed Jul 6, 2018

View reviewed changes

jzheaux force-pushed the resource-server-jwks branch 14 times, most recently from 94b1e3a to 3db1000 Compare July 6, 2018 19:43

jgrandja requested changes Jul 9, 2018

View reviewed changes

jgrandja requested changes Jul 10, 2018

View reviewed changes

jzheaux force-pushed the resource-server-jwks branch 6 times, most recently from 07b1dc7 to 21e81a5 Compare July 11, 2018 21:13

jzheaux force-pushed the resource-server-jwks branch 2 times, most recently from 10322f9 to e6ebaf0 Compare July 12, 2018 16:41

jgrandja reviewed Jul 13, 2018

View reviewed changes

jzheaux force-pushed the resource-server-jwks branch from e6ebaf0 to e2b0b79 Compare July 13, 2018 21:45

jzheaux force-pushed the resource-server-jwks branch from e2b0b79 to 88c02d5 Compare July 13, 2018 21:49

jzheaux added 5 commits July 16, 2018 08:24

jzheaux force-pushed the resource-server-jwks branch from 88c02d5 to 2b628f6 Compare July 16, 2018 14:26

rwinch self-assigned this Jul 16, 2018

rwinch approved these changes Jul 16, 2018

View reviewed changes

rwinch added this to the 5.1.0.M2 milestone Jul 16, 2018

rwinch added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue labels Jul 16, 2018

jgrandja approved these changes Jul 16, 2018

View reviewed changes

rwinch closed this Jul 16, 2018

		@@ -0,0 +1,76 @@
		/*
		* Copyright 2002-2015 the original author or authors.


		http.addFilterBefore(filter, BasicAuthenticationFilter.class);

		if (this.jwtConfigurer != null) {

Resource Server JWK support #5476

Resource Server JWK support #5476

Conversation

jzheaux commented Jul 2, 2018 • edited Loading

jzheaux Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

jzheaux Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch commented Jul 16, 2018

jzheaux commented Jul 2, 2018 •

edited

Loading

jzheaux Jul 2, 2018 •

edited

Loading

jzheaux Jul 2, 2018 •

edited

Loading

jzheaux Jul 5, 2018 •

edited

Loading