Fix IllegalArgumentException when using EdDSA signature algorithm

[0rzech]

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]

It is also possible to call toUpperCase() here, but it would make algorithm naming lax everywyhere, as SignatureAlgorithm.fromAlgorithm(String) is also used here.

Alternatively, EDDSA("EdDSA") could be changed to EDDSA("EDDSA") here, because the public interface seems to expect all letters to be upper case when using builder anyway, so the change should not be breaking.

[sberyozkin]

@0rzech Thanks for the PR, that should have been fixed with 4.5.2, can you retry please ?

[sberyozkin]

If you still see the problem, can you type the code here which leads to the EdDSA algorithm name conversion issue ?

[0rzech]

It still happens with 4.5.2 for me.

All you need to do is set

smallrye:
  jwt:
#    new-token:
#      signature-algorithm: EDDSA
    sign:
      key: >
        {
          "kty": "OKP",
          "crv": "Ed25519",
          "x": "9WDvp1MEM25AFpemGlPrciBA9IAYFSPNZ4xetx2v8HE",
          "d": "ogjlWAHjc3KZ1DCe8B8V7Satr6g0jpQjkGEV2lFPPIE"
        }

in application.yaml and then call

Jwt.claims()
    .upn("someone")
    .jws()
    .algorithm(SignatureAlgorithm.EDDSA)
    .sign();

in your code. Alternatively, you can remove .jws() and .algorithm(SignatureAlgorithm.EDDSA) calls from the builder step and uncomment new-token and signature-algorithm lines in yaml example.

[sberyozkin]

@0rzech But the correct name is EdDSA, the enum value is EDDSA but its .getAlgorithm is used

[0rzech]

EdDSA won't work either. And in fact it doesn't matter, because the name from the setting will have called .toUpperCase() on it first anyway here. Also, there's the example with using SignatureAlgorithm.EDDSA enum directly with builder, which should not lead to this error.

[sberyozkin]

@0rzech Ok, thanks, let me have a look a little bit later just to refresh in my mind the whole logic of delaing with algo names, in general, I think I'd prefer to tune it inside SignatureAlgorithm if needed.

[0rzech]

Sure! I can adjust PR to your decision. Unless you prefer to fix it yourself, of course. 😉

IMHO, it would be even better not to map back and forth between String and enum representation of SignatureAlgorithm, but such change would require more work.

[0rzech]

I forgot to add, that the public interface requiring one to use EDDSA (all upper case) String I mentioned in one of my previous comments is this.

[0rzech]

@sberyozkin It looks like when I don't set smallrye.jwt.token.new-token.signature-algorithm at all and have JWK inlined in smallrye.jwt.sign.key, then EdDSA key type is inferred from the key and the buggy code path is not executed. I don't know if it's the same for smallrye.jwt.sign.key.location setting.

On the other hand, I have to set mp.jwt.verify.publickey.algorithm explicitly to EDDSA to make verification work, because the key type won't be inferred from JWK inlined in mp.jwt.verify.publickey. Btw., EdDSA (notice small letter) will result in exception here too, this time because of java.lang.IllegalArgumentException: (...): Cannot convert EdDSA to enum class io.smallrye.jwt.algorithm.SignatureAlgorithm, allowed values: es256,es384,hs512,hs256,rs256,hs384,es512,ps512,eddsa,rs512,ps256,ps384,rs384.

IMHO, in both these cases exception suggestions are confusing to Quarkus users, because they mention internal signature algorithm representation and the rest of stack traces' lines do not mention values expected from users at all.

[sberyozkin]

Thanks for investigating @0rzech, I'll get to adding tests for different cases, it will take a bit of time, but we'll get it cleaned up

[sberyozkin]

@0rzech So, lets focus on generating tokens with EdDSA first, I think we have these 2 cases working OK

1. EdDSA private key is supplied directly to sign(PrivateKey) - we have a test for this case
2. As you have confirmed, with the inlined JWK alone, the algorithm is correctly inferred - if I remember correctly the test for it does exist too

These 2 cases do not work:

• .jws().algorithm(SignatureAlgorithm.EDDSA)
• smallrye.jwt.new-token.signature-algorithm=EDDSA

Can you please try to add 2 tests for these failing cases ? I'd also prefer if we can manage the correct conversion for EdDSA inside SignatureAlgorithm . Code like https://github.com/smallrye/smallrye-jwt/blob/7807b3f1b5db36c7ad4484a5dae4ed40230713e1/implementation/jwt-build/src/main/java/io/smallrye/jwt/build/impl/JwtSignatureImpl.java#L203 should be gone and replaced with .fromAlgorithm().

Please take your time, I'll be on PTO and realistically I'll have time to review and merge this PR from 11th June onwards

[sberyozkin]

https://github.com/smallrye/smallrye-jwt/blob/main/implementation/common/src/main/java/io/smallrye/jwt/algorithm/SignatureAlgorithm.java#L34 can handle it nicely.

Please also add extra log/error messages if you prefer

[sberyozkin]

@0rzech It certainly looks like a high quality PR, thanks a million for it. But, please don't get me wrong, this dependency makes me a bit nervous, possibly due to my ignorance, but having to add these exclusions here and there looks like an extra hassle that can be avoided. And PR goes beyond what the issue is about. In general I'd like to avoid somewhat mixed updates to avoid even slightest side-effects with the library now used not only in Quarkus but WildFly etc.

Let me offer you a compromise. In this PR lets handle only a case of EdDSA conversion, I think a trivial implementation of fromAlgorithm can check if it is compares in lower or upper with EdDSA and if yes treat it as such, otherwise, just convert to upper case. I'm not worried at all about cases like rS256 failing. And add 2 tests only to address EdDsa conversion problem.

Once we merge and tag this fix, lets then do another PR to comprehensively test all the name conversions for all types of algorithms, and there please check with @MikeEdgar if we really want to add this one extra test dependency.

Hope this proposal is not too bad, thanks for understanding. FYI, the next time I'll be to respond here will be starting from 11th June, talk to you then

Thanks

[0rzech]

@sberyozkin The code has already been allowing any-cased letters when signature algorithm is set through properties file. Letters case is already being ignored on that code path and this is already a part of library's public interface.

Only when it is set through header builder method, will it fail for any other letter cases than those matching enum variants, and this is what this PR changes - to match behaviour when reading from file.

The exception is thrown due to how SignatureAlgorithm is internally converted to and from String with use of .getAlgorithm() method and then .valueOf(String). This is why it can fail even when algorithm is set through builder's algorithm(SignatureAlgorithm) method.

So, IMHO, adding a special case just for EdDSA will make the code and tests more confusing and the tests won't reflect reality that way.

Nevertheless, I have created #801 with just 3 tests.

[sberyozkin]

Closing in favor of #801