Introduce JwtEncoder #87

anoopgarlapati · 2020-06-11T10:42:52Z

Implementation Requirements

define interface JwtEncoder
provide an implementation of JwtEncoder - NimbusJwtEncoder NOTE: The implementation should use the Nimbus JOSE + JWT library
the NimbusJwtEncoder should have a Builder
the NimbusJwtEncoder should support HS256 (HMAC using SHA-256)
the NimbusJwtEncoder should support RS256 (RSASSA-PKCS1-v1_5 using SHA-256)
the NimbusJwtEncoder should have a "KeyManager" that provides the public/private/symmetric keys (also required by Implement JWK Set Endpoint #82)
the NimbusJwtEncoder should be used by OAuth2AuthorizationCodeAuthenticationProvider Implement authorization_code AuthenticationProvider #68 to produce a JWS
javadoc class and public methods
Unit tests

jzheaux

Thanks, @anoopgarlapati! I think @jgrandja may have some additional feedback; I'm just going to leave my feedback as comments for the time being.

jzheaux · 2020-06-11T13:28:20Z

.../src/main/java/org/springframework/security/oauth2/server/authorization/jose/KeyManager.java

+ * @author Anoop Garlapati
+ * @since 0.0.1
+ */
+public interface KeyManager {


I wonder if this should be called KeyService. Several other interfaces that retrieve resources in Spring Security are services, like UserDetailsService, OAuth2UserService, AclService, etc.

jzheaux · 2020-06-11T13:34:32Z

...src/main/java/org/springframework/security/oauth2/server/authorization/jose/JoseHeaders.java

+		return this.headers;
+	}
+
+	public String getHeaderAsString(String header) {


I realize that these are following the JwtClaimAccessor pattern; however, I think that it's better to coerce the values to the correct type on write instead of converting them on each read.

Further, I think the JwtClaimAccessor makes a bit more sense when we are reading a JWT that another library has deserialized and chosen the types for each claim. Since we are the ones serializing in this case, I'm not seeing a huge need for these converters.

Is there any reason that we can't just have one method for now:

public <H> H getHeader(String name) { return (H) this.headers.get(name) }

and add the others later on, if needed?

I agree that since we are the ones serializing we might not need converters. I will simplify this in my next draft.

jzheaux · 2020-06-11T13:40:19Z

...ain/java/org/springframework/security/oauth2/server/authorization/jose/NimbusJwtEncoder.java

+		}
+
+		public NimbusJwtEncoder build() {
+			Key key = keyManager.getActiveKey();


Will getActiveKey guarantee non-null or should there also be an exception here to indicate that no key was returned? One way to address this would be to change the existing exception message from "Unsupported key provided." to "No supported key provided".

I'll add an exception here if a key is not returned and also will change the message.

jzheaux · 2020-06-11T13:46:57Z

...ain/java/org/springframework/security/oauth2/server/authorization/jose/NimbusJwtEncoder.java

+				} else if (X5C.equals(name)) {
+					jwsHeaderBuilder = jwsHeaderBuilder.x509CertChain(
+							X509CertChainUtils.toBase64List(JSONArrayUtils.parse(headers.getHeaderAsString(X5C))));
+				} else if (KID.equals(name)) {


How would the caller know this? It seems intrinsically tied to which key is used.

jzheaux · 2020-06-11T13:49:56Z

...ain/java/org/springframework/security/oauth2/server/authorization/jose/NimbusJwtEncoder.java

+		}
+
+		public NimbusJwtEncoder build() {
+			Key key = keyManager.getActiveKey();


Is the build method the right place to get the key? I'm thinking about key rotation.

It makes me wonder if the KeyManager should be in the NimbusJwtEncoder constructor.

I missed this use-case. Yes, we do need to have KeyManager in the NimbusJwtEncoder and fetch the active key to create the signer before signing the JWT. I'll make the appropriate changes in the next draft.

Do you think it would be worth trying to find a key from the manager that works with the selected algorithm?

krajcsovszkig-ms · 2020-06-30T15:37:20Z

...src/main/java/org/springframework/security/oauth2/server/authorization/jose/JoseHeaders.java

+				ClaimConversionService.getSharedInstance().convert(getHeaders().get(header), Boolean.class);
+	}
+
+	private Boolean containsHeader(String header) {


Can this ever be null? Does it have to be a Boolean?

krajcsovszkig-ms · 2020-06-30T15:38:09Z

...rc/main/java/org/springframework/security/oauth2/server/authorization/jose/JwtClaimsSet.java

+
+		public Builder(JwtClaimsSet jwtClaimsSet) {
+			Assert.notNull(jwtClaimsSet, "jwtClaimsSet cannot be null");
+			this.claims = jwtClaimsSet.claims;


claims is an unmodifiable map, you should add all its entries to this.claims instead.

krajcsovszkig-ms · 2020-06-30T16:03:30Z

...ain/java/org/springframework/security/oauth2/server/authorization/jose/NimbusJwtEncoder.java

+		// Parse optional and custom header parameters
+		Map<String, Object> headersMap = headers.getHeaders();
+		try {
+			for (final String name: headersMap.keySet()) {


I wonder if copying the map and getting each known key via remove, then only looping over the rest (the custom params) would be a bit nicer.

But if you do it in a loop, IMO you should loop over the entrySet() to avoid having to get the values separately for the keys you find, and perhaps a switch would be a little nicer than an if with this many branches.

krajcsovszkig-ms · 2020-06-30T16:05:53Z

...ain/java/org/springframework/security/oauth2/server/authorization/jose/NimbusJwtEncoder.java

+		return jwsHeaderBuilder.build();
+	}
+
+	private JWTClaimsSet createJwtClaimsSet(JwtClaimsSet claimsSet) {


This JWTClaimsSet vs JwtClaimsSet is a little confusing, it might be a good idea to write out the package name for one of them for clarity.

krajcsovszkig-ms

Given that the client needs this functionality as well, should this go into spring-security-oauth2-jose? Or are the two codebases going to be separate?

anoopgarlapati · 2020-06-30T16:36:07Z

Given that the client needs this functionality as well, should this go into spring-security-oauth2-jose? Or are the two codebases going to be separate?

@krajcsovszkig-ms Thanks for the review comments. I am close to revising my draft. This is something brought to my attention by @jgrandja as well. I am refactoring the relevant classes into the spring-security-oauth2-jose package in my next draft so that they can be extracted and merged into spring-security seamlessly.

krajcsovszkig-ms · 2020-06-30T16:38:16Z

@anoopgarlapati great, thanks for your work on this!

krajcsovszkig-ms · 2020-07-10T15:58:02Z

Hi @anoopgarlapati , are there any updates on this PR? Do you have an approximate ETA for when it will be ready?

Thanks!

anoopgarlapati · 2020-07-13T10:19:45Z

@krajcsovszkig-ms Unfortunately due to my work related project I will not be able to meet the timeline for this task. I have discussed this with @jgrandja and he will be taking this task forward.

krajcsovszkig-ms · 2020-07-13T10:31:03Z

Thanks for getting back!

@jgrandja do you have an approximate timeline for this task? Let me know if I can help to move it forward.

jgrandja · 2020-07-13T17:21:27Z

@krajcsovszkig-ms I'll be working on it this week and next week. My goal is to have this merged later next week.

krajcsovszkig-ms · 2020-07-13T17:27:05Z

That's great, thanks!

jgrandja · 2020-07-22T19:43:10Z

Closing in favour of #96

Introduce JwtEncoder

01af1dd

Issue spring-projectsgh-81

jzheaux reviewed Jun 11, 2020

View reviewed changes

jgrandja added the type: enhancement A general enhancement label Jun 11, 2020

jgrandja added this to the 0.0.1 milestone Jun 11, 2020

jgrandja self-assigned this Jun 11, 2020

krajcsovszkig-ms reviewed Jun 30, 2020

View reviewed changes

jgrandja closed this Jul 22, 2020

jgrandja removed this from the 0.0.1 milestone Jul 22, 2020

jgrandja added the status: duplicate A duplicate of another issue label Jul 22, 2020

anoopgarlapati deleted the gh-81-jwt-encoder branch October 24, 2020 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce JwtEncoder #87

Introduce JwtEncoder #87

anoopgarlapati commented Jun 11, 2020

jzheaux left a comment

jzheaux Jun 11, 2020

jzheaux Jun 11, 2020

anoopgarlapati Jun 15, 2020

jzheaux Jun 11, 2020

anoopgarlapati Jun 15, 2020

jzheaux Jun 11, 2020

jzheaux Jun 11, 2020

anoopgarlapati Jun 15, 2020

krajcsovszkig-ms Jun 30, 2020

krajcsovszkig-ms Jun 30, 2020

krajcsovszkig-ms Jun 30, 2020

krajcsovszkig-ms Jun 30, 2020

krajcsovszkig-ms Jun 30, 2020

krajcsovszkig-ms left a comment

anoopgarlapati commented Jun 30, 2020 •

edited

Loading

krajcsovszkig-ms commented Jun 30, 2020

krajcsovszkig-ms commented Jul 10, 2020

anoopgarlapati commented Jul 13, 2020

krajcsovszkig-ms commented Jul 13, 2020

jgrandja commented Jul 13, 2020

krajcsovszkig-ms commented Jul 13, 2020

jgrandja commented Jul 22, 2020

Introduce JwtEncoder #87

Introduce JwtEncoder #87

Conversation

anoopgarlapati commented Jun 11, 2020

jzheaux 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

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

krajcsovszkig-ms left a comment

Choose a reason for hiding this comment

anoopgarlapati commented Jun 30, 2020 • edited Loading

krajcsovszkig-ms commented Jun 30, 2020

krajcsovszkig-ms commented Jul 10, 2020

anoopgarlapati commented Jul 13, 2020

krajcsovszkig-ms commented Jul 13, 2020

jgrandja commented Jul 13, 2020

krajcsovszkig-ms commented Jul 13, 2020

jgrandja commented Jul 22, 2020

anoopgarlapati commented Jun 30, 2020 •

edited

Loading