KEYCLOAK-7560 Refactor Token Sign and Verify by Token Signature SPI

[tnorimat]

This PR is for refactoring token signing and verifying codes by introducing token signature SPI.

This first commit is specialized for the SPI/Provider/Factory design and implementation.

I would be happy if it is reviewed. If approved, I would like to do next refactoring current codes for signing and verifying tokens in RSASSA-PKCS1-v1_5.

After that, I would like to support newly ECDSA.

[supported signature algorithm]
RS256(as default), RS384, RS512

[on which we can select signature algorithm]
Realm, Client(supersede realm setting)

The corresponding JIRA ticket si the following.
https://issues.jboss.org/browse/KEYCLOAK-7560

[stianst]

Thanks. I've only had a quick glance so far, but it looks good. Both me and @mposolda will review this in detail soon. A couple of comments for now:

• Realm should have a default signing method, which can be changed through the admin console. This should be a dropdown listing available signing methods
• Clients should have a signing method where each client can override the realm default. Again, this should be available in the admin console
• Not 100% sure, but I think there's some standard client metadata to set the signing algorithm defined in the OIDC dynamic client registration specification
• Similar to above I believe the OIDC discovery endpoint has metadata to advertise what signing algorithms are supported
• We need to add explicit checks to prevent 'none' algorithm in JWTs
• KeyManager should probably be refactored to take Algorithm as an input rather than getActiveRsaKey. So should be something like getActiveKey(AlgorithmType type)

[tnorimat]

@stianst, thanks a lot for your comments.

I would like to wait for others' comments. But my proposals incorporating your comments are the followings. What do you think about them?

• Realm Token Signature Algorithm on Admin Console
  [UI]
  Add a dropdown named "Token Signature Algorithm" on "Token" tab of "Realm Settings".
  [Items shown on this dropdown]
  RS256 only. Add ES256 after supporting ECDSA.
  This PR can select RS256/RS384/RS512 to demonstrate signature algorithm selection.
  However, it is adequate to support RS256 only in RSASSA-PKCS1_v1_5.
  Also, it is sufficient to support ES256 only in ECDSA.
  [Tooltip of this dropdown]
  Mention that this setting is overridden by the one in Client settings.

• Client Token Signature Algorithm on Admin Console
  [UI]
  Add a dropdown named "Token Signature Algorithm" on "Fine Grain OpenID Connect Configuration" area of "Settings" tab.
  [Items shown on this dropdown]
  Same as Realm settings.
  [Tooltip of this dropdown]
  Mention that this setting is overridden by the one in Client settings.

• Client Token Signature Algorithm Metadata in OIDC Dynamic Client Registration
  This PR has already supported it.
  The master branch had already had corresponding field as OIDCClientRepresentation.id_token_signed_response_alg. However, this had not yet been used.
  Therefore, I've used it to support this. Please refer to this PR's OIDCAdvancedConfigWrapper and DescriptionConverter

• Advertising supported token signing algorithm in OIDC Discovery
  The master branch had already had corresponding field as OIDCWellKnownProvider.DEFAULT_ID_TOKEN_SIGNING_ALG_VALUES_SUPPORTED, and it had been advertised.
  Add ES256 afterwards if ECDSA would be supported.

• Avoid "none" algorithm in JWTs
  As you said, I would like to check algorithm to avoid it.
  If no algorithm is specified, use RS256 as default.

• Refactoring KeyManager
  As you said, I would like to replace algorithm specific methods like getActiveRsaKey with algorithm neutral methods like getActiveKey.

Aside from it, to accommodate the token signature algorithm setting in Realm, two ideas hit on me.

• use Attribute (get/setAttribute)
• add dedicated method like set/getTokenSignatureAlgorithm

This feature seems to be common. Therefore, the latter might be preferable. However, if do that, need a lot of work I'm not familiar with. Ex. prepare liquibase changelog for realm schema change, add codes to RealmModel implementing classes (infinispan, jpa).

[tnorimat]

algorithm specific codes removed from JWSBuilder.
HMAC Token Signature Provider added (it might be not used for token signing).

[tnorimat]

mainly add KeyManager's algorithm specific methods like getActiveRsaKey with algorithm neutral methods. I stop committing further changes until receiving reviews and comments.

[stianst]

• Realm/Client signature algorithm on admin console - your proposals are good here. One thing to mention unless you're aware of it already is the server info endpoint. There's a service exposed in Angular so you can just do serverInfo.listProviderIds('tokenSignature') and it will return the list of providers you can use as options in the dropdown. Populating the list with the list from the server side allows plugin custom signature providers and having them available in the admin console.

• Metadata in OIDC Dynamic Client Registration - nice I like how you did that

• OIDC Discovery - same as for admin console we should populate the list from the list of available providers so custom signature providers just works.

• For token signature algorithm in realm and clients just use attributes. You can add getter/setter methods to the models, but the JPA entities should just use attributes. Having specific fields on the entities is really the old way of doing it, which requires more work adding, but also more work for the migration.

[stianst]

KeyManager - I like what you did so far. I think there's one last piece of the puzzle here and that is allowing custom keys.

To explain that a bit better what I'd like is imagine if you wanted ECDSA and we didn't want it in the code base (that's not the case we want ECDSA, but just in an imaginary world where we didn't). You should still be able to develop it as a custom extension and deploy it to Keycloak. That means you need to be able to plugin the key provider (which is possible today) and the signature provider which you're working on now. What it also enables is also that you can add a new algorithm to core Keycloak without having to wire it in everywhere. All that should be needed is to list it in META-INF/services and all the wiring should be automatic.

[tnorimat]

Thank you for your comments.
I see, I would like to try to implement signature and key providers with no need to modify code base to support new signature algorithm and its accompanying key. But please confirm that in order to realize such the mechanism I need to add and modify the current code base.

[stianst]

What would be great is if you could start developing ECDSA as an extension. That way we can really see how pluggable this is. Then once this is merged it would be trivial to move the ECDSA extension to the Keycloak code base.

[tnorimat]

I've already implemented ECDSA key and its provider, but it has not yet been pluggable. Therefore, I would try to make it pluggable and commit it on this PR.

[tnorimat]

The goal for this commit is to support new signature algorithm and corresponding new key if necessary without modifying the current code base.

To do that, I've come up with the following design concepts and realized them on this commit. Could you please review them and give me some feedback?

• Not use Algorithm and AlgorithmType in the current code base.

Alternatively, make TokenSignatureProvider and KeyProvider in the plugin extension return the key type (AlgorithmType) as String each of which requires or provides respectively.

• Not use RSAProvider providing primitive signing and verifying features in the current code base.

Alternatively, make TokenSignatureProvider in the plugin extension provide the same features.

• Introduce interface specifying a key role

E.g., SignatureKeyProvider interface for signing and verifying KeyProvider. The user of a key recognize it as its role, not its type.

• Introduce TokenSignature engine class

Instead of use bare TokenSignatureProvider, use API provided by this TokenSignature which internally get an appropriate TokenSignatureProvider and KeyProvider, and conduct signing and verifying, and returns its result.

• Not use KeyManager in the current code base which defines a key representation and provides it.

Alternatively, make KeyProvider itself in the plugin extension provide its key representaion.
Newly introduced TokenSignature engine class mentioned just above provides key provider search and its aquisition.

• In JWSBuilder, not use RSAProvider.

Alternatively, provide signing capable JWSSignatureProvider instance externally and JWSBuilder use it.
TokenSignatureProvider in keycloak-server-spi-private project implements JWSSignatureProvider in keycloak-core project.
JWSBuilder in keycloak-core project can access signing capability via this JWSSignatureProvider interface.

This commit is for only Access, Refresh, ID Token treated in TokenManager. Not treat other signature related points such as /certs endpoint to provide verification keys' information, codes calling RSAProvider directly, codes using RSATokenVerifier, Client Adapter and so on.

[stianst]

I've been digging into this a bit today and realised there's quite a lot of cleanup needed in the KeyProvider, TokenSignatureProvider and other existing code. At this point I can't really give you a long list of things to do so will see if I can get time to sort this out myself. I will try to make time next week to look at this if not it's not going to be until I return from PTO a few weeks after that.

[stianst]

@tnorimat done a first round and refactored key providers. There are no generic methods to obtain keys given the algorithm type. There's also additional metadata available for keys including use (enc/sign) and algorithms it supports.

[stianst]

#5309

[tnorimat]

After finishing your key provider refactoring, could I continue token signature provider support by incorporating it?

[tnorimat]

I've revised codes to incorporate refactored key provider.

[tnorimat]

Add ECDSA key provider (NIST P-256, P-384, P-521) and token signature provider (ES256, ES384, ES512) in authorization server side only. Those are in keycloak's existing projects (keycloak-services and so on) but it seems to be possible to make them put into a separated new individual project (e.g. keycloak-ecdsa).

Not yet support TokenVerifier (used in UserInfo endpoint, TokenIntrospection endpoint, client adapter and test cases) for ECDSA so that added test cases only verify tokens' ECDSA signature.

Not yet support ECDSA in client adapter so that also not yet support JWK Set documents at jwks_uri (/certs) which currently only supports RSA key.

In my opinion, ECDSA support for client adapters and test suites seems not to be pluggable such as authorization server side (keycloak).

[stianst]

Thanks for the updates. I'm working on finishing something else and I'm away next week on holiday. After that I will give this some priority.

For clients I think at least for now we need to live with the providers being hardcoded and not pluggable. We would need to introduce some sort of SPI framework for clients if we wanted to have it pluggable, which would be nice, but to much work in this context.

[hmlnarik]

Thanks @tnorimat for this PR, it looks rather nice. Few comments inline

[hmlnarik]

should be volatile

[tnorimat]

It seems that KEY and EXPIRES are guarded against multiple accesses concurrently by synchronized (FailsafeEcdsaKeyProvider.class) block in the constructor and also make them visible
out of this block from other threads. Moreover, KEY and EXPIRES are not accessed other than the constructor.

Does it already suffice to guard KEY and EXPIRE static members against simultaneous multiple accesses and make them visible from other threads ?

[hmlnarik]

Yes, you're right.

[hmlnarik]

verify() calls verifySignature(), and then now verifySignature() calls verify() => stack overflow
Should be this.verifyKey != null instead?
Even in that case, I'd say it should suffice that this.signatureProvider != null since e.g. in case of hardware token, one might have no access to the key.

[tnorimat]

Yes, I also think that only this.signatureProvider != null is sufficient to check.
I've commented above that this TokenVerifier's refactoring had not yet completely been implemented and I had not yet written arquillian integration test cases for it so I had not recognized this stack overflow potential. Thank you very much.

[hmlnarik]

getTokenSignatureProvider is called rather often and would benefit from caching c.getProviderId() object per sigAlgName.

[tnorimat]

To make new provider instance by its corresponding provider factory, need to use its corresponding ComponentModel instance. Therefore, whole ComponentModel c needs to be cached instead of c.getProviderId().

Or how about caching the provider instance per sigAlgName?

[hmlnarik]

Good idea yet it would also need a mechanism for invalidating the provider instances upon change of the component configuration. Let's leave this out for this PR for now and file a JIRA (once this PR would be merged ) with this optimization later.

[tnorimat]

Yes. I agree with you. So I will not modify here and make a JIRA issue for its optimization.

[tnorimat]

@hmlnarik, thanks for your review. I'll incorporate them and push new commit afterward.

[hmlnarik]

This will be always true. Should be moved before the previous line?

[tnorimat]

Yes. I will move
logger.warnv("Keys expired, re-generated kid={0}", KEY.getKid());
just below
EXPIRES = Time.currentTime() + 60 * 10;

The existing FailsafeRsaKeyProvider.java also have the same codes. What do you think about it ? Should I do the same to FailsafeRsaKeyProvider.java ?

[hmlnarik]

Let's keep this commit focused on ECDSA, I've created https://issues.jboss.org/browse/KEYCLOAK-8027 for this nit. Feel free to either fix it separately or leave it for later.

[tnorimat]

I see. I will only modify FailsafeEcdsaKeyProvider.

[stianst]

I've created a new branch to be able to collaborate on this work: https://github.com/keycloak/keycloak/tree/token-signature-spi

I've opened a new PR from this branch here #5478 and will close this PR.

@tnorimat let's collaborate in #5478 to get this work completed. First thing I've done is to refactor token providers to not use component model. Component model is useful when you want to have multiple configurable instances of a provider in a realm (like user federation allows creating multiple instances of the LDAP provider). That does not apply in this case. Further, it's a lot simpler to use the providers if they are not using the component model and there's no need for config in the realm.

[tnorimat]

I see. I would like to continue collaborating this issue on the new PR.