smallrye / smallrye-jwt

Apache License 2.0
75 stars 49 forks source link

KeyLocationResolver does not honor algorithm in JWT header #812

Closed danielbobbert closed 3 months ago

danielbobbert commented 3 months ago

We have a service that accepts tokens using different key. One is RSA one is EC. We are supplying a JWKS that contains both keys. So we are receiving tokens that use either RS256 or EC256 as an algorithm.

But KeyLocationResolver.tryAsVerificationJwk() forces us to configure exactly one verification algorithm (or takes RS256 as a default, if mp.jwt.verify.publickey.algorithm is unconfigured), even though the JWT contains the specific algorithm that was used to sign the token.

Current implementation of KeyLocationResolver:

    private Key tryAsVerificationJwk(JsonWebSignature jws) throws UnresolvableKeyException {
        JsonWebKey jwk = super.tryAsJwk(jws, authContextInfo.getSignatureAlgorithm().getAlgorithm());
        return fromJwkToVerificationKey(jwk);
    }

The implementation should instead honor the algorithm given in the token itself (if present):

    private Key tryAsVerificationJwk(JsonWebSignature jws) throws UnresolvableKeyException {
        String algo = jws.getAlgorithmHeaderValue();
        if (algo == null) {
            algo = authContextInfo.getSignatureAlgorithm().getAlgorithm();
        }

        JsonWebKey jwk = super.tryAsJwk(jws, algo);
        return fromJwkToVerificationKey(jwk);
    }

The would probably also solve #754

sberyozkin commented 3 months ago

@danielbobbert thanks for the ideas, but the model where the client tells the server which algorithm it must use for the signature verification cannot be supported, it must be driven by the server allowing a given set of algorithms. Indeed, #754 is the way to do it. Also, FYI, there is a proposal to have both RS256 and ES256 supported if no algorithm is configured, which will cover your case. It is only the totally unrestricted dynam8c algorithm selection driven by the client that cannot be supported

Thanks

danielbobbert commented 3 months ago

OK, I can see that, since it would allow a client to use "weak" algorithms (e.g. RS256), when the server insists on using "strong" algorithms. But a fix for #754 would of course also solve the issue for me. Any plans, when this will be implemented?

sberyozkin commented 3 months ago

@danielbobbert Indeed, in general, we try to avoid it, though of course you are right that technically it can work.

OK, then, since we have agreed, let me close this one.

754 is expected to be fixed shortly as there is indeed some pressure now on making it consistent with the encryption algorithm property allowing a list