JWK endpoint as filter #31

jbellmann · 2020-04-18T20:57:57Z

Fixes #2

Provides an JWKSet endpoint implemented as filter with basic tests.

jgrandja · 2020-04-21T18:01:11Z

@jbellmann Thanks for the PR! FYI, when you are interested on working on a specific issue please reach out to us on the issue first so that other users can see that the issue is taken.

@ovidiupopa91 reached out in this comment requesting to work on this. All good either way, I just wanted to make you aware of the process. I will review this PR shortly. Thanks.

Captain-P-Goldfish · 2020-04-26T17:22:13Z

src/main/java/sample/JWKSetEndpoint.java

+@RequiredArgsConstructor
+public class JWKSetEndpoint extends OncePerRequestFilter {
+
+    static final String WELL_KNOWN_JWK_URIS = "/.well-known/jwk_uris";


wouldn't it be better if the uri was configurable and the constant especially accessbile?

Thanks for the feedback @Captain-P-Goldfish. The goal is to start with the minimal amount of code to get things working. We will add custom configurations as we build things out.

Captain-P-Goldfish · 2020-04-26T17:26:32Z

src/main/java/sample/JWKSetEndpoint.java

+        try (Writer writer = response.getWriter()) {
+            writer.write(jwkSet.toPublicJWKSet().toJSONObject().toJSONString());
+        }


what about error handling?
NullPointerExceptions would be pretty unpleasant

jgrandja

Thanks for the PR @jbellmann. Please see my review comments.

jgrandja · 2020-04-27T09:55:23Z

src/main/java/sample/JWKSetEndpoint.java

+
+import com.nimbusds.jose.jwk.JWKSet;
+
+import lombok.RequiredArgsConstructor;


Please remove lombok as Spring Security does not use it.

jgrandja · 2020-04-27T09:56:45Z

src/main/java/sample/JWKSetEndpoint.java

+public class JWKSetEndpoint extends OncePerRequestFilter {
+
+    static final String WELL_KNOWN_JWK_URIS = "/.well-known/jwk_uris";
+    private final JWKSet jwkSet;


The JWKSet should be provided with a default set of key(s). Please see main issue comment:

The keys that are used can be generated at startup or be constants

jgrandja · 2020-04-27T09:59:24Z

build.gradle

@@ -1,22 +1,38 @@
 plugins {
+	id 'java'


Please move the code into the minimal sample and remove this gradle build.

jgrandja · 2020-04-27T10:03:08Z

src/main/java/sample/JWKSetEndpoint.java

+@RequiredArgsConstructor
+public class JWKSetEndpoint extends OncePerRequestFilter {
+
+    static final String WELL_KNOWN_JWK_URIS = "/.well-known/jwk_uris";


Thanks for the feedback @Captain-P-Goldfish. The goal is to start with the minimal amount of code to get things working. We will add custom configurations as we build things out.

jgrandja · 2020-04-27T10:05:34Z

src/test/java/sample/JWKSetEndpointTest.java

+    }
+
+    @Test
+    void testIfRequestMatches() throws Exception {


We should setup for keys to be returned here

rorybyrne · 2020-04-30T00:12:45Z

samples/boot/minimal/src/main/java/sample/JWKSetEndpoint.java

+    }
+
+    protected boolean ifRequestMatches(HttpServletRequest request) {
+        return request.getRequestURI().endsWith(WELL_KNOWN_JWK_URIS);


This will also match /stuff/.well-known/jwk_uris, right? Is that acceptable or should it be an exact match?

Please use RequestMatcher, specifically AntPathRequestMatcher

jgrandja

Thank you for the updates @jbellmann. Please see my review comments.

jgrandja · 2020-05-01T18:37:46Z

.gitignore

@@ -12,6 +12,7 @@ build/
 .settings
 .springBeans
 .sts4-cache
+bin/


Please rebase on master as bin has been added in a recent commit.

jgrandja · 2020-05-01T18:40:07Z

samples/boot/minimal/src/main/java/sample/Config.java

+    @Bean
+    FilterRegistrationBean<JWKSetEndpoint> registerJWKSetEndpoint() throws JOSEException {
+        FilterRegistrationBean<JWKSetEndpoint> fb = new FilterRegistrationBean<>();
+        fb.setFilter(new JWKSetEndpoint(generateJwkSet()));


Let's register JWKSetEndpoint with the Spring Security filter chain via HttpSecurity.addFilter...

jgrandja · 2020-05-01T18:40:48Z

samples/boot/minimal/src/main/java/sample/Config.java

+import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;
+
+@Configuration
+public class Config {


Rename to SecurityConfig and extend WebSecurityConfigurerAdapter

jgrandja · 2020-05-01T18:42:12Z

samples/boot/minimal/src/main/java/sample/JWKSetEndpoint.java

+
+import com.nimbusds.jose.jwk.JWKSet;
+
+public class JWKSetEndpoint extends OncePerRequestFilter {


Please rename to JwkSetEndpointFilter

jgrandja · 2020-05-01T18:43:53Z

samples/boot/minimal/src/main/java/sample/JWKSetEndpoint.java

+    }
+
+    protected boolean ifRequestMatches(HttpServletRequest request) {
+        return request.getRequestURI().endsWith(WELL_KNOWN_JWK_URIS);


Please use RequestMatcher, specifically AntPathRequestMatcher

jgrandja

Thanks for the updates @jbellmann. Please see review comments.

Before you push the updates, please rebase on master and squash to 1 commit. Our git workflow is rebase instead of merge. Thanks.

jgrandja · 2020-05-07T18:01:48Z

samples/boot/minimal/src/main/java/sample/SecurityConfig.java

+import com.nimbusds.jose.jwk.KeyUse;
+import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;
+
+@Configuration


Replace with @EnableWebSecurity

jgrandja · 2020-05-07T18:02:50Z

samples/boot/minimal/src/main/java/sample/SecurityConfig.java

+	}
+
+	protected JWKSet generateJwkSet() throws JOSEException {
+		JWK jwk = new RSAKeyGenerator(2048).keyID("minimal-ASA").keyUse(KeyUse.ENCRYPTION).generate();


Change to KeyUse.SIGNATURE

jgrandja · 2020-05-07T18:04:11Z

samples/boot/minimal/src/test/java/sample/FakeController.java

+public class FakeController {
+
+	@RequestMapping("/fake")
+	public String hello() {


Can you move this declaration in the test class instead

jgrandja · 2020-05-07T18:08:19Z

samples/boot/minimal/src/test/java/sample/JwkSetEndpointFilterTest.java

+
+	@Test
+	void testRequestMatcher() {
+		assertTrue(this.filter.ifRequestMatches(new MockHttpServletRequest(GET.name(), WELL_KNOWN_JWK_URIS)));


Can you change all the assertion checks to use AssertJ instead as that is the primary test lib we use in Spring Security.

jgrandja · 2020-05-07T18:16:24Z

samples/boot/minimal/src/test/java/sample/JwkSetEndpointFilterTest.java

+	void testRequestMatcher() {
+		assertTrue(this.filter.ifRequestMatches(new MockHttpServletRequest(GET.name(), WELL_KNOWN_JWK_URIS)));
+
+		assertFalse(this.filter.ifRequestMatches(new MockHttpServletRequest(POST.name(), WELL_KNOWN_JWK_URIS)));


We should test the doFilter() method directly. Please change the protected methods to private in JwkSetEndpointFilter. See OAuth2AuthorizationCodeGrantFilterTests (spring-security-oauth2-client) on how we test Filter's.

jgrandja · 2020-05-18T01:13:46Z

Thanks for all the updates @jbellmann ! FYI, I added a polish commit to get this merged. This is now in master.

anzap mentioned this pull request Apr 20, 2020

Add Resource Server Sample #30

Closed

jgrandja mentioned this pull request Apr 21, 2020

Add JWK Set Endpoint #2

Closed

Captain-P-Goldfish suggested changes Apr 26, 2020

View reviewed changes

jgrandja requested changes Apr 27, 2020

View reviewed changes

jgrandja self-assigned this Apr 27, 2020

jbellmann requested a review from jgrandja April 28, 2020 11:17

rorybyrne reviewed Apr 30, 2020

View reviewed changes

jgrandja requested changes May 1, 2020

View reviewed changes

jbellmann requested a review from jgrandja May 3, 2020 23:23

jgrandja requested changes May 7, 2020

View reviewed changes

jbellmann force-pushed the gh_2-JWK-Endpoint branch from 557b3c3 to 8a7629c Compare May 10, 2020 20:27

spring-projects#2 JwkSet endpoint as filter

01f3ee1

jbellmann force-pushed the gh_2-JWK-Endpoint branch from 8a7629c to 01f3ee1 Compare May 10, 2020 20:54

jbellmann requested a review from jgrandja May 10, 2020 20:58

jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement labels May 18, 2020

jgrandja added this to the 0.0.1 milestone May 18, 2020

jgrandja added a commit that referenced this pull request May 18, 2020

Polish gh-31

26c3941

jgrandja closed this May 18, 2020

jgrandja mentioned this pull request May 28, 2020

Implement JWK Set Endpoint #82

Closed

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish spring-projectsgh-31

49212b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JWK endpoint as filter #31

JWK endpoint as filter #31

jbellmann commented Apr 18, 2020 •

edited

Loading

jgrandja commented Apr 21, 2020 •

edited

Loading

Captain-P-Goldfish Apr 26, 2020

jgrandja Apr 27, 2020

Captain-P-Goldfish Apr 26, 2020

jgrandja left a comment

jgrandja Apr 27, 2020

jgrandja Apr 27, 2020

jgrandja Apr 27, 2020

jgrandja Apr 27, 2020

jgrandja Apr 27, 2020

rorybyrne Apr 30, 2020

jgrandja May 1, 2020

jgrandja left a comment

jgrandja May 1, 2020

jgrandja May 1, 2020

jgrandja May 1, 2020

jgrandja May 1, 2020

jgrandja May 1, 2020

jgrandja left a comment

jgrandja May 7, 2020

jgrandja May 7, 2020

jgrandja May 7, 2020

jgrandja May 7, 2020

jgrandja May 7, 2020

jgrandja commented May 18, 2020


		import com.nimbusds.jose.jwk.JWKSet;

		import lombok.RequiredArgsConstructor;


		import com.nimbusds.jose.jwk.JWKSet;

		public class JWKSetEndpoint extends OncePerRequestFilter {

JWK endpoint as filter #31

JWK endpoint as filter #31

Conversation

jbellmann commented Apr 18, 2020 • edited Loading

jgrandja commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja left a 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

jgrandja left a 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

jgrandja left a 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

jgrandja commented May 18, 2020

jbellmann commented Apr 18, 2020 •

edited

Loading

jgrandja commented Apr 21, 2020 •

edited

Loading