Provide EC signature verification support in JWTSignatureValidator #25
Conversation
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
| @@ -36,6 +40,10 @@ | |||
|
|
|||
| public class JWTSignatureValidator implements TokenValidator { | |||
|
|
|||
| static { | |||
| Security.insertProviderAt(new BouncyCastleProvider(), 1); | |||
There was a problem hiding this comment.
This will add BC as a security provider for a whole bunch of algorithms, not just ECDSA, right? And we don't know whether the user of the library trusts BC? So maybe we should only register when we encounter a token using EC and only if there's not existing EC provision already registered? That would allow a user who really cares to register some other provider for EC and we'd just pick it up.
Also, I don't think we can assume that there aren't other providers already registered in some specific order. Adding at index 1 might then mean BC is chosen instead of their preferred provider. So maybe if/when we do add BC, we should always add at the end (which is still a bit racy, but less likely to be a problem in practice)?
There was a problem hiding this comment.
Right. That is a dilemma. The thing is that for EC, the Keycloak helper library that we use requests ECDSA which is provided by Bouncycastle, but is otherwise not available in JDK. To make make matters worse, there's usage of both "EC" and "ECDSA" in the code. That means if Bouncycastle is not installed in first place what will happen is that when "EC" is requested it would use Sun's provider, and when "ECDSA" is requested the Bouncycastle provider would be used - unless the provider is bumped to first position among security providers. Multiple impls might work together fine or they might not.
We could add a configuration: oauth.crypto.bouncycastle.provider.enable to require explicitly enabling the provider before we install it. And another - oauth.crypto.bouncycastle.provider.position which, if set to 0 or negative number would install the provider at the end, otherwise at the position specified - or at the end if position is beyond number of providers.
I'm not sure about lazy enabling it upon detection of EC. That may hide any potential issues to long after Kafka broker has started.
There was a problem hiding this comment.
I'm guessing that the helper library has no mechanism for selecting algorithm impls with a given provider?
It not really clear to me whether the "EC" signature algo is actually the same thing as "ECDSA" (in which case Keycloak's use of EC starts to look like a bug). But I guess we need to provide something which works with the version of the library which actually exists. So I suppose System properties like oauth.crypto.bouncycastle.provider.enable would at least provide a mechanism, even if it's less than ergonomic.
There was a problem hiding this comment.
Right, the Keycloak code is what it is, and it does not explicitly request BouncyCastle provider. With the patched Keycloak library that requests "EC" instead of "ECDSA" things work fine without BouncyCastle. But that would require a patch that probably won't make it into the master, and will only be released later while we would like a fix ASAP.
...src/test/java/io/strimzi/testsuite/oauth/KeycloakClientCredentialsWithJwtValidationTest.java
Show resolved
Hide resolved
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
…astle` and `oauth.crypto.provider.bouncycastle.position` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
|
I made installing BouncyCastle provider optional. It is required for EC signature verification to work, though. It can be installed as the last security provider by passing to JAAS config of Position in the providers list can be controlled with another parameter: Alternatively, these can be set as a system properties or as ENV vars called |
|
@ppatierno Do you plan to review this or can it be merged? |
oauth-common/src/main/java/io/strimzi/kafka/oauth/validator/ECDSASignatureVerifierContext.java
Outdated
Show resolved
Hide resolved
| sb.append(" - " + p.toString() + " [" + p.getClass().getName() + "]\n"); | ||
| sb.append(" " + p.getInfo() + "\n"); | ||
| } | ||
| log.debug(sb.toString()); |
There was a problem hiding this comment.
this should log all the installed security providers but it happens only if the outer if (enableBouncyCastleProvider && !bouncyInstalled.getAndSet(true)) is true. Is it really what we want? Shouldn't we log the installed security providers anyway?
There was a problem hiding this comment.
This is the way I intended. Usually you're not really interested in security providers, you're working with what's packaged and enabled in JDK. If you really want to see the providers for some reason, you can just enable bouncy castle provider in the config, and it will be added at the end, not overwriting any existing providers, but you'll also see them all listed.
Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>
This PR attempts to address: strimzi/strimzi-kafka-operator#2327
Signed-off-by: Marko Strukelj marko.strukelj@gmail.com