NimbusJwtEncoder should simplify constructing with javax.security Keys #17033

surajbh123 · 2025-05-02T20:49:55Z

No description provided.

surajbh123 · 2025-05-03T05:25:45Z

Working on test cases and code refactoring

surajbh123 · 2025-05-04T18:52:34Z

Helper Methods for Key Generation

        // 1. Using SecretKey with defaults
        SecretKey defaultSecretKey = createSecretKey("HmacSHA256", 256); // Key for default HS256
        NimbusJwtEncoder encoderWithSecretDefaults = NimbusJwtEncoder.withSecretKey(defaultSecretKey)
                // .macAlgorithm(MacAlgorithm.HS256) // Default is HS256
                // .keyId(null)                     // Default keyId is null
                // .jwkSelector(...)               // Default selector throws if multiple keys match
                .build();
                
        // 2. Using SecretKey with specific algorithm and keyId
        SecretKey hs512SecretKey = createSecretKey("HmacSHA512", 512);
        String secretKeyId = "my-hmac-key-01";
        NimbusJwtEncoder encoderWithSecretCustom = NimbusJwtEncoder.withSecretKey(hs512SecretKey)
                .macAlgorithm(MacAlgorithm.HS512) // Specify HS512
                .keyId(secretKeyId)               // Specify a key ID
                .build();

        // 3. Using RSA KeyPair with defaults
        KeyPair rsaKeyPair = createRsaKeyPair(2048);
        NimbusJwtEncoder encoderWithRsaDefaults = NimbusJwtEncoder.withKeyPair(rsaKeyPair)
                // .signatureAlgorithm(SignatureAlgorithm.RS256) // Default for RSA is RS256
                // .keyId(UUID generated)                      // Default keyId is a random UUID
                // .jwkSelector(...)                          // Default selector picks first if multiple
                .build();
                
        // 4. Using RSA KeyPair with specific algorithm (PSS) and keyId
        String rsaKeyId = "my-rsa-key-01";
        NimbusJwtEncoder encoderWithRsaCustom = NimbusJwtEncoder.withKeyPair(rsaKeyPair)
                .signatureAlgorithm(SignatureAlgorithm.PS256) // Specify PS256
                .keyId(rsaKeyId)                              // Specify a key ID
                .build();

        // 5. Using EC KeyPair with defaults
        KeyPair ecP256KeyPair = createEcKeyPair(Curve.P_256); // Key for default ES256
        NimbusJwtEncoder encoderWithEcDefaults = NimbusJwtEncoder.withKeyPair(ecP256KeyPair)
                // .signatureAlgorithm(SignatureAlgorithm.ES256) // Default for EC is ES256
                // .keyId(UUID generated)                      // Default keyId is a random UUID
                // .jwkSelector(...)                          // Default selector picks first if multiple
                .build();

        // 6. Using EC KeyPair with specific algorithm and keyId
        KeyPair ecP521KeyPair = createEcKeyPair(Curve.P_521); // Key for ES512
        String ecKeyId = "my-ec-key-01";
        NimbusJwtEncoder encoderWithEcCustom = NimbusJwtEncoder.withKeyPair(ecP521KeyPair)
                .signatureAlgorithm(SignatureAlgorithm.ES512) // Specify ES512
                .keyId(ecKeyId)                               // Specify a key ID
                .build();

jzheaux

Thanks, @surajbh123! This PR will make using NimbusJwtEncoder much simpler.

I've left my feedback inline. In addition to that, please ensure that your commit only has the signoff in the description and not in the title. Please also ensure that the description has the issue that this PR resolves:

Add NimbusJwtEncoder Builders

Closes gh-16267

Signed-off-by ...

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

jzheaux · 2025-05-07T21:42:42Z

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

+		 */
+		public KeyPairJwtEncoderBuilder signatureAlgorithm(SignatureAlgorithm signatureAlgorithm) {
+			Assert.notNull(signatureAlgorithm, "signatureAlgorithm cannot be null");
+			this.jwsAlgorithm = JWSAlgorithm.parse(signatureAlgorithm.getName());


Let's please validate the signature algorithm at this point instead.

can,t do now as as now we have two separate builder for RSA and EC

please push this down to the subclass, have the subclass provide default and valid algorithms by calling the super constructor, or do not use the abstract class so that you can validate here. From a security perspective, it's valuable to validate as early as possible.

What I mean is that the RSA-specific checking should go in the RSA-specific class. And the EC-specific checking should go in the EC-specific class.

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

jzheaux · 2025-05-07T21:50:24Z

Just one more thing, @surajbh123, when you submit, it will save you time in the review process if you run the following before submitting:

./gradlew format && ./gradlew :spring-security-oauth2-jose:check

surajbh123 · 2025-05-08T20:16:47Z

Working on reviews comments
Not all resolved yet

Closes spring-projectsgh-16267 Signed-off-by: Suraj Bhadrike <[email protected]>

jzheaux

Thanks for the quick turnaround, @surajbh123! This is coming together nicely. I've added some additional inline feedback.

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

jzheaux · 2025-05-09T18:30:23Z

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

+		 * with a {@link SecretKey}.
+		 */
+		public NimbusJwtEncoder build() {
+			this.jwsAlgorithm = (this.jwsAlgorithm != null) ? this.jwsAlgorithm : JWSAlgorithm.HS256;


Since you are giving this a default value and there isn't a way for the application to set it back to null, I believe you can remove this line.

In the following case, it may be null.

SecretKey defaultSecretKey = createSecretKey("HmacSHA256", 256); // Key for default HS256 NimbusJwtEncoder encoderWithSecretDefaults = NimbusJwtEncoder.withSecretKey(defaultSecretKey) // .macAlgorithm(MacAlgorithm.HS256) // Default is HS256 // .keyId(null) // Default keyId is null // .jwkSelector(...) // Default selector throws if multiple keys match .build();

Please take a look at my suggestions for the constructor so that this value need not be checked in this way.

By setting reasonable defaults in the constructor, readying the JWK is as simple as:

OctetSequenceKey jwk = this.builder.build()

jzheaux · 2025-05-09T18:31:35Z

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

+		 */
+		public KeyPairJwtEncoderBuilder signatureAlgorithm(SignatureAlgorithm signatureAlgorithm) {
+			Assert.notNull(signatureAlgorithm, "signatureAlgorithm cannot be null");
+			this.jwsAlgorithm = JWSAlgorithm.parse(signatureAlgorithm.getName());


please push this down to the subclass, have the subclass provide default and valid algorithms by calling the super constructor, or do not use the abstract class so that you can validate here. From a security perspective, it's valuable to validate as early as possible.

jzheaux · 2025-05-09T18:33:00Z

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

+	 *
+	 * @since 7.0
+	 */
+	public abstract static class KeyPairJwtEncoderBuilder {


I think it might be best to not have an abstract class since that can be quickly complex for builders. As it stands, the amount of reuse is quite small.

Hey @jzheaux
Running ./gradlew :spring-security-oauth2-jose:check is raising a warning that says:
Class KeyPairJwtEncoderBuilder should be declared as final. |
So I think we should keep the class abstract

Sorry, I wasn't clear here. What I'm saying is that the abstract class itself doesn't provide enough reuse value to justify the maintenance. I think that we should just have SecretKeyJwtEncoderBuilder, RsaKeyJwtEncoderBuilder, and EcKeyJwtEncoderBuilder.

jzheaux

Hi, @surajbh123, thanks for the updates and for your feedback.

Given some of the confusion I caused with some of my feedback, I wanted to take some more time with the PR before replying. You'll see that there are many more comments now with more specifics about what and why; hopefully that's clearer this time around.

To help with processing the comments, I also added a commit to my fork for you to review which applies my comments. I feel these changes make the builder classes easier to use and more maintainable by our team. You are welcome to review this commit to get a holistic view of the changes I'm proposing:

jzheaux@7e1847d

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

jzheaux · 2025-05-19T17:46:35Z

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

+		 * with a {@link SecretKey}.
+		 */
+		public NimbusJwtEncoder build() {
+			this.jwsAlgorithm = (this.jwsAlgorithm != null) ? this.jwsAlgorithm : JWSAlgorithm.HS256;


Please take a look at my suggestions for the constructor so that this value need not be checked in this way.

By setting reasonable defaults in the constructor, readying the JWK is as simple as:

OctetSequenceKey jwk = this.builder.build()

jzheaux · 2025-05-20T23:35:32Z

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

-		if (headers == null) {
-			headers = DEFAULT_JWS_HEADER;
-		}
+		headers = (headers != null) ? headers : (this.jwsHeader != null) ? this.jwsHeader : DEFAULT_JWS_HEADER;


Instead of checking for this.jwsHeader being non-null, please ensure that it is not null as part of the construction process. Then you can change this to:

headers = (headers != null) ? headers : this.jwsHeader;

jzheaux · 2025-05-20T23:35:46Z

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

@@ -83,6 +95,8 @@ public final class NimbusJwtEncoder implements JwtEncoder {

 	private static final JWSSignerFactory JWS_SIGNER_FACTORY = new DefaultJWSSignerFactory();

+	private JwsHeader jwsHeader;


Please rename this to defaultJwsHeader for clarity.

jzheaux · 2025-05-21T00:03:02Z

...oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtEncoderTests.java

+
+	@Test
+	void keyPairBuilderWithRsaDefaultAlgorithm() throws Exception {
+		KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");


Instead of re-constructing the generator, please move this to a member variable. Also, I feel this will be simpler if you use RSAKeyGenerator from Nimbus instead:

private final RSAKeyGenerator rsaGenerator = new RSAKeyGenerator(2048);

Then in this test you can do:

RSAKey jwk = this.rsaGenerator.generate();

and then use that instance to construct the builder. The other unit tests would benefit from this same polish.

jzheaux · 2025-05-21T00:05:42Z

...oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtEncoderTests.java

+
+	@Test
+	void keyPairBuilderWithEcDefaultAlgorithm() throws Exception {
+		KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("EC");


Instead of re-constructing the generator, please move this to a member variable. Also, I feel this will be simpler if you use ECKeyGenerator from Nimbus instead:

private final ECKeyGenerator ecGnerator = new ECKeyGenerator(Curve.P_256);

Then in this test you can do:

ECKey jwk = this.ecGnerator.generate();

and then use that instance to construct the builder.

jzheaux · 2025-05-21T00:06:17Z

...oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtEncoderTests.java

+		KeyPair keyPair = keyPairGenerator.generateKeyPair();
+		NimbusJwtEncoder encoder = NimbusJwtEncoder.withKeyPair(keyPair)
+			.keyId(UUID.randomUUID().toString())
+			.signatureAlgorithm(SignatureAlgorithm.ES256)


Since EC defaults to ES256, I think this should be a different algorithm, given the test name.

jzheaux · 2025-05-21T00:16:24Z

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

+	 * @since 7.0
+	 */
+	public static SecretKeyJwtEncoderBuilder withSecretKey(SecretKey secretKey) {
+		Assert.notNull(secretKey, "secretKey cannot be null");


Please move the assertion into the builder constructor so that the subclass can be self-sufficient. This is helpful in the event that we turn the subclass into a top-level class.

jzheaux · 2025-05-21T00:17:12Z

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

+	 * @since 7.0
+	 */
+	public static KeyPairJwtEncoderBuilder withKeyPair(KeyPair keyPair) {
+		Assert.isTrue(keyPair != null && keyPair.getPrivate() != null && keyPair.getPublic() != null,


Please move any null checking to the subclass constructors so they are self-sufficient. This is helpful in the event that we turn the subclasses into top-level classes.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 2, 2025

surajbh123 force-pushed the nimbus-ecoder-builder branch 2 times, most recently from 6e3841d to abddfb3 Compare May 2, 2025 20:54

surajbh123 mentioned this pull request May 2, 2025

NimbusJwtEncoder should simplify constructing with javax.security Keys #16267

Open

surajbh123 force-pushed the nimbus-ecoder-builder branch 2 times, most recently from 78ef577 to 6370999 Compare May 4, 2025 18:43

jzheaux added this to the 7.0.x milestone May 6, 2025

jzheaux added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels May 6, 2025

jzheaux requested changes May 7, 2025

View reviewed changes

jzheaux self-assigned this May 7, 2025

surajbh123 force-pushed the nimbus-ecoder-builder branch from 6370999 to 567e729 Compare May 8, 2025 20:14

surajbh123 force-pushed the nimbus-ecoder-builder branch 3 times, most recently from bb6d143 to d6154cb Compare May 9, 2025 15:45

Add NimbusJwtEncoder Builders

52ef97d

Closes spring-projectsgh-16267 Signed-off-by: Suraj Bhadrike <[email protected]>

surajbh123 force-pushed the nimbus-ecoder-builder branch from d6154cb to 52ef97d Compare May 9, 2025 15:52

jzheaux requested changes May 9, 2025

View reviewed changes

surajbh123 force-pushed the nimbus-ecoder-builder branch from a53bfb3 to 2d1e2c6 Compare May 13, 2025 18:46

Merge github.com:surajbh123/spring-security into nimbus-ecoder-builder

24300cb

surajbh123 force-pushed the nimbus-ecoder-builder branch from 2d1e2c6 to 24300cb Compare May 13, 2025 19:50

jzheaux requested changes May 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NimbusJwtEncoder should simplify constructing with javax.security Keys #17033

NimbusJwtEncoder should simplify constructing with javax.security Keys #17033

surajbh123 commented May 2, 2025

surajbh123 commented May 3, 2025 •

edited

Loading

surajbh123 commented May 4, 2025

jzheaux left a comment

jzheaux May 7, 2025

surajbh123 May 9, 2025

jzheaux May 9, 2025 •

edited

Loading

surajbh123 May 13, 2025

jzheaux May 20, 2025

jzheaux commented May 7, 2025

surajbh123 commented May 8, 2025 •

edited

Loading

jzheaux left a comment

jzheaux May 9, 2025

surajbh123 May 13, 2025

jzheaux May 19, 2025

jzheaux May 9, 2025 •

edited

Loading

jzheaux May 9, 2025 •

edited

Loading

surajbh123 May 13, 2025

surajbh123 May 13, 2025 •

edited

Loading

jzheaux May 20, 2025

surajbh123 May 21, 2025

jzheaux left a comment

jzheaux May 19, 2025

jzheaux May 20, 2025

jzheaux May 20, 2025

jzheaux May 21, 2025

jzheaux May 21, 2025

jzheaux May 21, 2025

jzheaux May 21, 2025

jzheaux May 21, 2025

		@@ -83,6 +95,8 @@ public final class NimbusJwtEncoder implements JwtEncoder {

		private static final JWSSignerFactory JWS_SIGNER_FACTORY = new DefaultJWSSignerFactory();

		private JwsHeader jwsHeader;

NimbusJwtEncoder should simplify constructing with javax.security Keys #17033

Are you sure you want to change the base?

NimbusJwtEncoder should simplify constructing with javax.security Keys #17033

Conversation

surajbh123 commented May 2, 2025

surajbh123 commented May 3, 2025 • edited Loading

surajbh123 commented May 4, 2025

Helper Methods for Key Generation

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

jzheaux May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux commented May 7, 2025

surajbh123 commented May 8, 2025 • edited Loading

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

jzheaux May 9, 2025 • edited Loading

Choose a reason for hiding this comment

jzheaux May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajbh123 May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

surajbh123 commented May 3, 2025 •

edited

Loading

jzheaux May 9, 2025 •

edited

Loading

surajbh123 commented May 8, 2025 •

edited

Loading

jzheaux May 9, 2025 •

edited

Loading

jzheaux May 9, 2025 •

edited

Loading

surajbh123 May 13, 2025 •

edited

Loading