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

smallrye-jwt/implementation/jwt-build/src/main/java/io/smallrye/jwt/build/impl/JwtSignatureImpl.java

Line 203 in 7807b3f

alg = SignatureAlgorithm.valueOf(alg.toUpperCase()).getAlgorithm();

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]

We don't need another dependency IMHO

[0rzech]

If we want to be able to write property-based tests, then we do need it. Given it's just a test dependency, it would be shame to resign from this. But it's your call.

[sberyozkin]

@0rzech I see, I missed you meant to use it for tests only. These tests look fine, but for some reasons this dependency requires a dedicated exclusion in .gitignore (what exactly is generated there ?). I appreciate you like the style offered but me being quite conservative, I'd say we can easily enough, even if it will be more verbose, to write similar tests without it.

Let me ask Mike. @MikeEdgar, do you think we should bring it or just use existing test dependencies ?

[0rzech]

.jqwik-database is a file to store data of previous runs. Given how property-based testing works with shrinking etc., I don't think we should try to emulate it ourselves.

[sberyozkin]

At the same time, we don't really need to introduce it IMHO if it keeps some local state after runs

[0rzech]

It's helpful if one wants to re-run failing properties, but I can disable it.

[MikeEdgar]

I'm not entirely seeing the benefit of jqwik versus just using @ParameterizedTest with one of the arguments being some predetermined invalid value. I'm not saying there is no value for something like jqwik, rather it may not be worth it to have a few tests that look/behave differently from the rest of the suite for what otherwise appears to be a minor fix/change.

[0rzech]

The whole point of property-based testing is not to rely on one or few specific input examples, but to have tests that cover large space of inputs and help with pinpointing specific errors. @ParameterizedTest was not made for this.

IMHO, it's worth investing even more into property-based testing where appropriate, especially in security-related project. For instance, property-based tests could be used to randomize claim headers in JWTs, instead of just making sure that headers work fine for https://issuer.com issuer and customHeader with custom-header-value, because this is what current tests actually prove.

The reason I was hit with the bug was that the tests did not cover the code path my project triggered, that's why I've added tests covering all other signature algorithms. To be fair, though, because all possible algorithms is a relatively small finite input space, @ParameterizedTest could do it. It's opposite for invalid inputs in this particular case.

If you are worried about jqwik being some random library, then here's Oracle's Java Magazine article about it from almost 5 years ago. Jqwik has also plugins for Spring Boot and Micronaut, and even someone wanted to add jqwik extension to Quarkus, though - unfortunately - to no avail it seems (here and here), and here's Apache Kafka using @ParameterizedTest, @Property and normal @Test in a single test class no problem.

Having said all that, I fully respect it's your project, not mine. I just hope that at least most arguments are taken into consideration, that's it.

May I ask to help me with understanding why it is such an issue with adding this library, though? I genuinely don't get it, given it's not code that will run in production anyway and the jqwik project seems to have been started over 8 years ago. I may lack some context, though. It's been years since I was programming Java for a living and I'm completely new to Quarkus.

[MikeEdgar]

I think for me it's less about a new test dependency and more about having a small subset of the tests using a different paradigm. Based on what I've seen/read, I'm not really opposed to something like property-based testing per se, but rather it might be confusing to do things multiple ways within a single project.

Another possible "gotcha" is IDE support for jqwik. It might be fine, but just guessing it might trip up Eclipse's JUnit runner (for example).

[0rzech]

Thanks for your reply!

You can't use property-based testing only. Not everything makes sense that way, because not every test input is large.

The same way one wouldn't want to do everything with builder pattern, because most constructors have short list of parameters. And the opposite - one wouldn't want to resign from builder pattern completely just to not confuse developers with doing things multiple ways in a single project. It's a matter of familiarity.

I don't know how Eclipse will react, but jqwik is an alternative engine for Junit 5 platform, so there shouldn't be problems with running tests and reporting. Perhaps one would have to make Eclipse aware of jqwik's test annotations, so that the code won't be marked as unused. I don't know, I tend not to write code to make IDE's happy, but to adjust IDEs to the code. ;) Nevertheless, IntelliJ was able to run jqwik tests pretty fine.

[sberyozkin]

We only need to make sure that if someone typed rs256 or RS256, it works, so this is why to upper case conversion was there, and do a single specific if for EdDSA, this kind of check does not need a new dependency IMHO

[0rzech]

Jqwik is used here as well just not to mix it with Junit Parametrized.

The true reason for jqwik is testing the method's property of failing for any other string than the valid ones here, and most likely in other tests I have yet to write.

As this is still WIP, I haven't added checking mixed-case names yet.

[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