Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IllegalArgumentException when using EdDSA signature algorithm #801

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Fix IllegalArgumentException when using EdDSA signature algorithm #801

merged 1 commit into from
Jun 13, 2024

Conversation

0rzech
Copy link
Contributor

@0rzech 0rzech commented May 31, 2024

This fixes java.lang.IllegalArgumentException: No enum constant io.smallrye.jwt.algorithm.SignatureAlgorithm.EdDSA when EDDSA is set through
smallrye.jwt.new-token.signature-algorithm property, or when it is set with
JwtClaimsBuilderImpl.

Currently, JwtSignatureImpl.getConfiguredSignatureAlgorithm() returns
algorithm name as a String from SignatureAlgorithm.algorithmName field,
in case of it being loaded from a configuration file.

If the algorithm was set through JwtClaimsBuilderImpl, the value is returned
as-is from the header, which means EdDSA, because this is how
JwtClaimsBuilderImpl puts the value there.

This name is then used to get appropriate SignatureAlgorithm enum variant
in JwtSignatureImpl.getSigningKeyFromKeyContent(String), but without
using toUpperCase() on the name, causing exception when EdDSA is used.

The fix adds toUpperCase() call on algorithm name before passing it
to SignatureAlgorithm.valueOf(String).

@0rzech 0rzech changed the title Fix IllegalArgumentException when using EdDSA signature algorithm Fix IllegalArgumentException when using EdDSA signature algorithm without using property-based testing Jun 1, 2024
@sberyozkin sberyozkin changed the title Fix IllegalArgumentException when using EdDSA signature algorithm without using property-based testing Fix IllegalArgumentException when using EdDSA signature algorithm Jun 11, 2024
@sberyozkin sberyozkin self-requested a review June 12, 2024 15:31
sberyozkin
sberyozkin previously approved these changes Jun 12, 2024
Copy link
Contributor

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0rzech Thanks for your help and patience with fixing this issue.

Before I merge, can you give me a favor and squash commits, and have IllegalStateException thrown indirectly by adding it to https://github.com/smallrye/smallrye-jwt/blob/main/implementation/jwt-build/src/main/java/io/smallrye/jwt/build/impl/ImplMessages.java instead

Thanks

@sberyozkin
Copy link
Contributor

@0rzech You may still have to make these two latest tests conditional as Java 11 is still required, let me run the build

@sberyozkin
Copy link
Contributor

Looks like so

@0rzech
Copy link
Contributor Author

0rzech commented Jun 12, 2024

@0rzech Thanks for your help and patience with fixing this issue.

Before I merge, can you give me a favor and squash commits, and have IllegalStateException thrown indirectly by adding it to https://github.com/smallrye/smallrye-jwt/blob/main/implementation/jwt-build/src/main/java/io/smallrye/jwt/build/impl/ImplMessages.java instead

Thanks

I can't put it there, because it would create circular library dependency. I have wrapped all calls to fromAlgorithm(String algorithmName) in appropriate try/catch clauses instead.

@0rzech You may still have to make these two latest tests conditional as Java 11 is still required, let me run the build

Looks like so

Whoops, sorry for that. I tried to avoid post-Java-11 features. Which part have I overlooked? Do you mean running tests conditional on JRE version? If yes, then I've added min 17 condition to new tests.

This fixes `java.lang.IllegalArgumentException: No enum constant
io.smallrye.jwt.algorithm.SignatureAlgorithm.EdDSA` when `EDDSA` is set through
`smallrye.jwt.new-token.signature-algorithm` property, or when it is set with
`JwtClaimsBuilderImpl`.

Currently, `JwtSignatureImpl.getConfiguredSignatureAlgorithm()` returns
algorithm name as a String from `SignatureAlgorithm.algorithmName` field,
in case of it being loaded from a configuration file.

If the algorithm was set through `JwtClaimsBuilderImpl`, the value is returned
as-is from the header, which means `EdDSA`, because this is how
`JwtClaimsBuilderImpl` puts the value there.
using `toUpperCase()` on the name, causing exception when `EdDSA` is used.

The fix adds `toUpperCase()` call on algorithm name before passing it
to `SignatureAlgorithm.valueOf(String)`.

As requested, this fix does not introduce property-based testing
in the project.
@sberyozkin sberyozkin added this to the 4.5.3 milestone Jun 13, 2024
@sberyozkin sberyozkin merged commit 1122009 into smallrye:main Jun 13, 2024
7 checks passed
@0rzech 0rzech deleted the loading-eddsa-signature-algorithm-fix-no-property-tests branch June 13, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants