Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KEYCLOAK-6768 Signed and Encrypted ID Token Support #5779

Closed
wants to merge 10 commits into from

Conversation

tnorimat
Copy link
Contributor

@tnorimat tnorimat commented Dec 5, 2018

JIRA ticket is the following.
https://issues.jboss.org/browse/KEYCLOAK-6768

The brief summary of the specification is as follows.

[Content Encryption Key(CEK) Management Mode]
Key Encryption

[how to load client public key]
client's jwks_url

[CEK Management Algorithm]
RSA1_5
RSA-OAEP

[Content Encryption Algorithm]
A128CBC-HS256
A128GCM

[ID Token Encryption Setting]
per Client

default setting is "not encrypt"

[how to specify a CEK management and content encryption algorithm]
On Admin UI
Clients -> -> Settings -> Fine Grain OIDC Client Settings

OIDC Dynamic Client Registration

[ID Tokens to be encrypted]
all ID Tokens issued in all OAuth 2.0 Grants/OIDC Flows

[advertising server capability]
by Server Metadata
id_token_encryption_alg_values_supported
id_token_encryption_enc_values_supported

[Other major modification]
Support several key use (signature, encryption) in KeyWrapper, client key loader and Infinispan's cache entry

@stianst
Copy link
Contributor

@stianst stianst commented Dec 11, 2018

Thanks for the PR. We are currently wrapping up development of 4.x and are in a feature freeze so are not able to review/merge this right now. Will get back to you when we open up for 5.x development.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Dec 11, 2018

I see. I'll wait for you.

@stianst
Copy link
Contributor

@stianst stianst commented May 15, 2019

Sorry for the long wait for reviewing this one. @mposolda and myself will start reviewing it as soon as we can.

@stianst
Copy link
Contributor

@stianst stianst commented May 15, 2019

Both me and @mposolda will provide a full review of this PR by the end of May

Copy link
Contributor

@mposolda mposolda left a comment

I think the PR is great work and I am sorry for the review taking so long. Besides the comments above, which IMO are not major blockers, I have few more points to discuss, but seems that none of them is blocker for this PR, rather something for the future...:

  1. Support for Key wrapping with A128KW. This is not supported in the PR as there is no Keycloak provider for that - There is just AesKeyWrapAlgorithmProvider in the "core" module right now. I guess this is not needed for FAPI and hence you did not yet added that? IMO This is definitely not a blocker for the PR and can be added later eventually.

  2. Global "default" algorithm for ID Token encryptions configured at the realm level. Will be overriden just in case that it is explicitly re-configured for the client. I think it depends on the point (1) a bit as global encryption algorithm will probably need to use the symmetric key available to the realm - otherwise there will be "hard" requirement for all the clients to provide public/private keypair, which may be complicated and cause various issues... Also it will mean that every client is able to decrypt tokens by other clients, which is not so great and partially bypass the purpose of JWE.

  3. Support for "encrypted" access tokens. Although regarding this, I am again not 100% sure. Because when encrypted access token is sent to the REST service, it will be required for REST services to use token introspection endpoint, which bypass one of the main advantages of Keycloak (ability for "offline" validation of access tokens without need to send introspection request). Another thing is, that access tokens will need to use symmetric algorithm for the encryption as well (hence point 1) as otherwise Keycloak won't be able to decrypt them - for assymetric algorithms it is usually just client, who is able to decrypt token as just client owns the private key (not KC server and not the REST service).

I am pretty sure that the 3 points mentioned above are not the blocker for FAPI and for this PR and can be added later - if we ever add them.

  • Also it will be likely needed to add documentation for this, but that's for later (once also @stianst reviews and we agree on the "implementation" PR).

}

private byte[] encryptBytes(byte[] contentBytes, byte[] ivBytes, Key aesKey, byte[] aad) throws GeneralSecurityException {
Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we rather want Cipher.getInstance with second argument to be "BC" security provider as used for example in AesCbcHmacShaEncryptionProvider? AFAIK that may have bigger probability to avoid issues that some stuff doesn't work as expected on various JVM versions etc.

Copy link
Contributor Author

@tnorimat tnorimat Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you. I will use BC(Bouncy Castle) as the security provider the same as in AesCbcHmacShaEncryptionProvider.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, Thanks

@Override
protected Cipher getCipherProvider() throws Exception {
String jcaAlgorithmName = null;
if (JWEConstants.RSA1_5.equals(jweAlgorithmName)) {
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code snippet is duplicated with some code in RsaCekManagementProvider. Maybe it is possible to have this class RsaKeyEncryptionJWEAlgorithmProvider available as public class in the "core" module and remove the class RsaKeyEncryptionJWEAlgorithmProvider you added to the services module? Then RsaCekManagementProvider can just call "new RsaKeyEncryptionJWEAlgorithmProvider(jweAlgorithmName)"

Copy link
Contributor Author

@tnorimat tnorimat Jun 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I will move RsaKeyEncryptionJWEAlgorithmProvider from keycloak-services to keycloak-core.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, Thanks

Copy link
Contributor Author

@tnorimat tnorimat Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that what you said can be applied to AesGcmJWEEncryptionProvider and AesCbcHmacShaJWEEncryptionProvider. Should I move those from "services" module to "core" module?

Copy link
Contributor

@mposolda mposolda Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

return encodedToken;
}

private boolean isTokenEncyptRequired(TokenCategory category) {
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in the method name

Copy link
Contributor Author

@tnorimat tnorimat Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix it as isTokenEncryptRequired.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks


private KeyWrapper getEncryptionKekWrapper(String algAlgorithm, ClientModel client) {
if (algAlgorithm == null) return null;
switch (algAlgorithm) {
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why algorithms are explicitly mentioned there? If someone wants to add his own AlgorithmProvider, it won't work with this restriction?

Copy link
Contributor Author

@tnorimat tnorimat Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I should not include algorithm specific codes in this DefaultTokenManager. I will clean out them.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor Author

@tnorimat tnorimat Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the following codes?

    private KeyWrapper getEncryptionKekWrapper(String algAlgorithm, ClientModel client) {
        if (algAlgorithm == null) return null;
        List<String> providerIds = new LinkedList<>(session.listProviderIds(CekManagementProvider.class));
        if (providerIds.contains(algAlgorithm)) return PublicKeyStorageManager.getClientPublicKeyWrapper(session, client, JWK.Use.ENCRYPTION);
        return null;
    }

Copy link
Contributor

@mposolda mposolda Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry I did not spot this before, but now when thinking about it and looking at it one more, I can see some potential issues with this approach. IMO the possible issue is, that algorithm is not passed to the PublicKeyStorageManager. There can be scenario when the "jwk" set returned from the client "jwks_url" contains more keys for the KeyUse.ENC for example. In that case, it would be a risk that bad key would be returned for different algorithm that you want. Filtering algorithms this way you did may not be sufficient as there can be keys and all of them could be of the algorithms, which are actually supported by Keycloak. There can be even more keys with the usage of KeyUse.ENC and for the same algorithm. The combination (keyUse, algorithm) is not guaranteed to be unique within JWK Set. Only guaranteed unique thing is "kid" AFAIK.

I wonder if we can remove method "getEncryptionKekWrapper(algAlgorithm, client)" on the DefaultTokenManager class at all. In the method "getEncryptedToken(TokenCategory category, String encodedToken)" there can be a possibility to first retrieve "kid" from the encoded token? Maybe some more changes will be needed in the class "JWE" to achieve this, but I guess this may be possible? I am seeing that retrieving "kid" first is done for example in the method "decode(String token, Class clazz)", so I hope this may be possible in our case as well. Then we can lookup PublicKeyStorageManager.getClientPublicKeyWrapper by "kid". This will actually mean that PR can be simplified if I look correctly as there won't be a need to change anything in the class PublicKeyStorageManager.getClientPublicKeyWrapper and all the related classes used from that. WDYT?

Copy link
Contributor Author

@tnorimat tnorimat Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is better to use "kid" instead of combination of "alg" and "use" in order to specify the KEK uniquely.

To do so, we need to know "kid" of KEK before encrypting the ID token.
The way hit on me is that keycloak loads Client's public keys from "jwks_url" in advance and an administrator select one of them as KEK.

The potential problem of this way is that Client's public keys loaded by keycloak does not sync with public keys Client actually manages. (e.g. Client might delete the key used for KEK marked in keycloak, and keycloak doen not know about it.)

To get around this problem, keycloak loads Client's public keys from "jwks_url" just before encrypting the ID token every time.

However, this way needs to determine which Client's public key is used as KEK when encrypting the ID token. An administrator can not intervene manually at that time.

The next best way instead of using "kid" seems to be that (using "alg" and "enc" as filtering key) :

  • get client's public keys from "jwks_url" when just encrypting the ID token
  • filter out of them with keycloak specified "alg" and "use" (e.g. "alg"="RSA1_5", "use"="enc")
  • if multiple public keys are selected, first one of them is used for KEK

What do you think about it?

When verifying a token in JWS, or decrypting a token in JWE, it is easy to determine "kid" because it is included in the token's JOSE header.

However, signing a token in JWS, or encrypting a token in JWE, keycloak itself needs to determine which key are used for signing or encyrpting KEK. It is easy to do so when keycloak itself manage keys used for these purpose. That is the case for signing a token in JWS. It seems not easy to do so when keycloak itself does not manage keys used for these purpose. That is the case for encrypting a token in JWE using Key Encryption for Key Management Mode.

Copy link
Contributor

@mposolda mposolda Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see and agree that using "kid" is not very ideal though. Will be good to use the "alg" + "enc" combination as you proposed. Will be probably good to add some more new methods, for example add new method to PublicKeyStorageProvider, which will avoid to pass null "kid" as an argument. So maybe something along the lines PublicKeyStorageProvider.getFirstPublicKey(modelKey, algorithm, loader); OR PublicKeyStorageProvider.getFirstPublicKey(modelKey, KeyUse.ENC, algorithm, loader); ?

Copy link
Contributor Author

@tnorimat tnorimat Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your idea saying that method's name (getFirstPublicKey) explicitly tell its behavior (if multiple client public key are found, first one is retrieved).

I think PublicKeyStorageProvider.getFirstPublicKey(modelKey, algorithm, loader) seems better.
the reasons are as follows :

  • JWK's "alg" implicitly determines "use" (e.g. "alg" = "RSA-OAEP" implies "use"="enc", "alg" = "RS256" implies "use"="sig") that means specifying usage of key in .getFirstPublicKey() seems redundant.

  • Looking into PublicKeyStorageManager.getClientPublicKeyWrapper(KeycloakSession session, ClientModel client, JWK.Use keyUse) and ClientPublicKeyLoader, the loaded client public keys have already filtered out by key usage (if key use = "enc", only client public keys with "use" = "enc" are loaded). Therefore, it seems that no need to considering "use" when calling PublicKeyStorageProvider.getFirstPublicKey().

    // Algorithm need to be specified
    public static KeyWrapper getClientPublicKeyWrapper(KeycloakSession session, ClientModel client, JWK.Use keyUse, String algAlgorithm) {
        PublicKeyStorageProvider keyStorage = session.getProvider(PublicKeyStorageProvider.class);
        String modelKey = PublicKeyStorageUtils.getClientModelCacheKey(client.getRealm().getId(), client.getId(), keyUse);
        // here client's pubic keys are loaded and filtered out by key usage
        ClientPublicKeyLoader loader = new ClientPublicKeyLoader(session, client, keyUse);
        // here only need to consider algorithm
        return keyStorage.getFirstPublicKey(modelKey, algAlgorithm, loader);
    }

What do you think about it?

Copy link
Contributor

@mposolda mposolda Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnorimat That works for me. Thanks!

@@ -338,6 +338,10 @@ access-token-signed-response-alg=Access Token Signature Algorithm
access-token-signed-response-alg.tooltip=JWA algorithm used for signing access tokens.
id-token-signed-response-alg=ID Token Signature Algorithm
id-token-signed-response-alg.tooltip=JWA algorithm used for signing ID tokens.
id-token-encrypted-response-alg=ID Token Encryption Key Management Algorithm
id-token-encrypted-response-alg.tooltip=JWA Algorithm used for key management in encrypting ID tokens.
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have the tooltip a bit more clear and explicitly mention also something like: This option is needed just if you want encrypted ID tokens. If left empty, ID Tokens are just signed, but not encrypted.

Copy link
Contributor Author

@tnorimat tnorimat Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you. As for the tooltip text, your suggestion is very well. I will use the same as you suggested. How do you think about the following tooltip? :

id-token-encrypted-response-alg.tooltip=JWA Algorithm used for key management in encrypting ID tokens. This option is needed just if you want encrypted ID tokens. If left empty, ID Tokens are just signed, but not encrypted.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

id-token-encrypted-response-alg=ID Token Encryption Key Management Algorithm
id-token-encrypted-response-alg.tooltip=JWA Algorithm used for key management in encrypting ID tokens.
id-token-encrypted-response-enc=ID Token Encryption Content Encryption Algorithm
id-token-encrypted-response-enc.tooltip=JWA Algorithm used for content encryption in encrypting ID tokens.
Copy link
Contributor

@mposolda mposolda May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor Author

@tnorimat tnorimat Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you think about the following tooltip? :

id-token-encrypted-response-enc.tooltip=JWA Algorithm used for content encryption in encrypting ID tokens. This option is needed just if you want encrypted ID tokens. If left empty, ID Tokens are just signed, but not encrypted.

Copy link
Contributor

@mposolda mposolda Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@mposolda
Copy link
Contributor

@mposolda mposolda commented May 30, 2019

ah, and it will be needed to rebase as there are currently some conflicts... Hope it won't be major headache to fix the conflicts.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented May 30, 2019

@mposolda Thank you for your review comments and suggestions. I'll look through them and reply. And I'll also consider documentation.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented May 31, 2019

Yes, I'll resolve conflicts and push afterwards.

As for 3, I intended to encrypt only ID token, not access token nor refresh token. Therefore, it seems that what you pointed out does not happen.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented May 31, 2019

As for 1, yes I think that Key Wrapping (A128KW) seems to be appropriate rather than Key Encryption (RSA1_5, RSA-OAEP). For example, using client secret as KEK.

I brought back to memory to find why I chosen Key Wrapping, and found that I could get client's public key easily the same manner as the existing mechanism of Request Object (jwks_url).

As you pointed at 3, if Key Encryption is used, keycloak can not decrypt ID token because it does not know its client's private key. It causes problems where the user's browser send ID token as id_token_hint to Logout Endpoint (GET).

Considering them, I will try to modify codes to use Key Wrapping (A128KW). What do you think about it?

@mposolda
Copy link
Contributor

@mposolda mposolda commented May 31, 2019

@tnorimat Well, for the A128KW, global encryption of tokens configured at realm level and access tokens, it can maybe wait for the other follow-up PR (if we ever add support for global encryption and for access tokens as there are some downsides to it)?

If you think that adding A128KW is easy (together with keep support for RSA1_5, RSA-OAEP), then it should be fine in this PR. I guess the complication could be that A128KW is symmetric, which may require more changes in the SPI itself. So if it's too complicated, I definitely rather vote for having it in separate PR.

Typically, it is bit better to have more separate PRs rather than one super-huge PR due the easier review. And I really hope that for your next contribution, we will be able to review them much faster than this one (Again, sorry for so long delay).

@mposolda
Copy link
Contributor

@mposolda mposolda commented May 31, 2019

@tnorimat I am sorry, but one thing, which I missed during the review is the admin console support for encryption keys. For this one, I would like to hear the opinion from other members of the team and also from you or the others from the community. So I've started the thread on keycloak-dev with subject "Encrypted OIDC ID Tokens support and admin console" . Feel free to follow-up there. Thanks!

@fidgi
Copy link

@fidgi fidgi commented Jun 19, 2019

@tnorimat Hi, do you plan to expand this PR to support encryption of access tokens or may be to create a new PR for that in the near future ? (the question is linked to this thread https://lists.jboss.org/pipermail/keycloak-user/2019-June/018563.html)

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 20, 2019

@fidgi Hello, I intend to sign and encrypt ID Token only, not Access Token. I would like to make authenticated user's PII included in ID Token keep in secrecy.

However, It seems to be easy to sign and encrypt Access Token by using the feature of this PR because I design the feature of this PR to offer signing and encryption for general JWT, not ID Token specific.

One point I would like to ask you, why would you like to encrypt Access Token?

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 20, 2019

@mposolda After resolving conflicts, incorporating your comments and being approved, I'll squash commits onto one as usual.

@fidgi
Copy link

@fidgi fidgi commented Jun 20, 2019

@tnorimat Encrypting access tokens is recommanded when the token is delivered to browser-side application to protect information (like roles) that are intended to be known and used only by the server tjat serve resources to this application. Another option is to issue a token with no sensible information to the application but it will require a call to the authentication server for the resource server to exchange it with a full-fledged token.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 20, 2019

@fidgi Is that the case of Implicit Grant/Flow? I think it is the only Grant/Flow where the user's browser receive an access token.

If we want to hide some information not intended to the user, the following 2 options hit on me:

  1. encrypt an access token
    This might be acheived by using encryption feature of this PR.
    But I concern that the lenght of URL included encrypted access token becomes very long and the browser cannot receive such long URL.
    AFAIK, the valid length of URL is not specified by RFC so it depends on the browser's implementation about how long URL is accepted.

  2. information not intended to the user is not included into an access token
    This might be achieved because keycloak can select which information is included into Access Token, ID Token, UserInfo Endpoint response by Mapper.
    But as you pointed out, one round trip is needed to retrieve such information from keycloak.

Aside from it, you might have already know that Implicit Grant/Flow be not recommended due to security concerns.

We can find out why Implicit Grant/Flow is not recommended by the following internet drafts:

OAuth 2.0 Security Best Current Practice
https://tools.ietf.org/html/draft-ietf-oauth-security-topics-12#section-3.1.2

OAuth 2.0 for Browser-Based Apps
https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-01#section-9.8

OAuth 2.0 for Native Apps
https://tools.ietf.org/html/rfc8252#section-8.2

@fidgi
Copy link

@fidgi fidgi commented Jun 20, 2019

@tnorimat This is for an Authorization code grant flow in which it is not guaranted that the 3rd party application will handle the tokens in a back-end component as recommended.
BTW, I'm interested by the option 2) where I can configure the content of the tokens.
I will switch back to the keycloak mailing list to get more information about how to achieve that.
Many thanks for your kind answers. Regards

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 20, 2019

@fidgi Sorry, I guessed wrong.

As for option 2), please refer to https://www.keycloak.org/docs/latest/server_admin/index.html#_protocol-mappers

We can not put intentionally some claims in an access token and an ID token but put them into UserInfo endpoint's response in return to the access token.

However, I'm not sure this can satisfy what you want to do.

@mposolda
Copy link
Contributor

@mposolda mposolda commented Jun 26, 2019

@tnorimat I've replied to the inline comments in the PR. Regarding The A128KW, I suggest to eventually create other JIRA and use another PR for this, not to mix into this one. Same for encryption of access tokens. Rather go one piece at the time instead of very big PR :)

IMO the main point of the encryption is, that not all parties (client, REST service, Keycloak server) are always able to decrypt the tokens. This is limitation, but also an advantage requested for specific deployments. So the fact that if tokens is encrypted with assymetric key and just client is able to decrypt it (not Keycloak or REST service) is an advantage and may be requested setup for some deployment. The fact that id_token_hint won't work is natural limitation of this setup.

Same can be applied for access tokens. As pointed by @fidgi, I can imagine setup when access tokens are encrypted with the realm keys and hence just the Keycloak server will be able to decrypt them (not the client or the REST service). This has another "natural" limitation that additional round-trip is needed for REST service to send request to token-introspection-endpoint or user-info-endpoint.

@mposolda
Copy link
Contributor

@mposolda mposolda commented Jun 26, 2019

BTV. The browser can receive tokens also in the authorization code grant, not just in implicit grant. By default, the keycloak.js adapter uses authorization code grant (not implicit grant due the security issues you mentioned). So any JS application secured by keycloak has access tokens available inside browser.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 27, 2019

I forgot browser-based client. As you said, the browser-based client receives an access token in the authorization code grant/flow.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jun 27, 2019

Thank you for comments.
Considering your comments, I think it might be better for this PR's task to be split onto the following JIRAs/PRs:

  1. this PR
  • support only Key Encryption
  • keycloak retrieves client's public key from client hosted URL specified by jwks_uri in RFC 8414 / OIDC Discovery
  • this jwks_uri can be set by OIDC Dynamic Client Registration or client's authentication setting - signed JWT
  1. Client Key Management Mechanism
    It seems that the current keycloak does not have the feature for managing client's public key explicitly (supporting jwks_uri for client authentication).
    Therefore, support such the feature explicitly not only from jwks_uri but jwks, which can be used for the token encryption, signature verification.

  2. JWE Key Wrapping Support
    As you commented, A128KW and so on.
    I'm not sure how to manage the shared key...
    The existing shared secret might be used, but not used as it is. Therefore, it might be needed to create the key for Key Wrapping based on this existing shared secret by some way (Key Derivation Function?). If so, the client needs to know not only the shared secret but the way how to derive the key for Key Wrapping.

  3. Signed and Encrypted ID Token support in Key Wrapping

  4. signed and encryption for other JWT based token (e.g. Access Token)

What do you think about that?

@mposolda
Copy link
Contributor

@mposolda mposolda commented Jul 1, 2019

Agree to first go with this PR (so your point 1) and do the other things as a follow-up.

In addition to what you added, I would also like to mention https://issues.jboss.org/browse/KEYCLOAK-10462 Encrypted OIDC ID Tokens support and admin console . The more details are described in the keycloak-dev mails referenced from the JIRA.

I don't understand your point 2 "Client Key Management Mechanism" . Currently you can configure client public key in KC admin console with both "jwks_uri" or with "jwks" . If you go to client -> Tab "Credentials" -> Provider "Signed JWT" , you can specify if you want to use JWKS_URI or directly specify the public key / certificate. Not sure what exactly is missing here?

Regarding point 3, I suggest to use existing client secret for key wrapping. As you pointed, it can't be used as is. However could we for example generate SHA-256 hash from the client secret and then use first half of it (EG. first 128 bits) as the AES 128 KW key? Maybe there is some better alternative, but hopefully something along those lines is possible?

Agree with points 4 and 5. I suggest to create JIRAs for 3, 4 and 5. Not sure about (2), as I would like to clarify with you first.

@mposolda
Copy link
Contributor

@mposolda mposolda commented Jul 1, 2019

@tnorimat BTV. One important point: I know you are involved also in WebAuthn and TBH The WebAuthn is currently bigger priority for KC team then the follow-up tasks of this JIRA mentioned in the comments above. There is some WebAuthn related work planned by KC team as well, but we need to properly plan it.

I still need to take a look at your design proposal for WebAuthn and I plan to do it today/tomorrow. Hope we can discuss more details afterwards.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 2, 2019

@mposolda ,as for 2,

I don't understand your point 2 "Client Key Management Mechanism" . Currently you can configure >client public key in KC admin console with both "jwks_uri" or with "jwks" . If you go to client -> Tab >"Credentials" -> Provider "Signed JWT" , you can specify if you want to use JWKS_URI or directly specify >the public key / certificate. Not sure what exactly is missing here?

My intention is the same as you mentioned in https://issues.jboss.org/browse/KEYCLOAK-10462 and https://lists.jboss.org/pipermail/keycloak-dev/2019-May/012101.html .
Therefore, the JIRA ticket of 2 that I mentioned here has still existed and been KEYCLOAK-10462.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 2, 2019

@mposolda as for 3, 4, and 5, I'll create corresponding JIRA tickets.

As for 3, I'll try to create KEK for key wrapping in the manner you said.

As for 1, it is treated in this PR. I'll incorporate your comments and push the commit.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 2, 2019

@mposolda , as for WebAuthn, I'll try to integrate our prototype implementation realized as independent project onto forked keycloak codebase and implement features along with our design documentation. I would like to receive feedback on this design document form you KC team and community, and revise this design documentation itself and corresponding implementation.

When I encounter issues during this work, I would like to discuss it with you KC team and community. Which way of discussion is appropriate, keycloak dev-ml or pull request and its comments in keycloak-community's repository?

@mposolda
Copy link
Contributor

@mposolda mposolda commented Jul 2, 2019

Thanks for clarifying the point (2) and thanks for creating JIRAs for 3, 4 and 5. Also thanks for incorporating the feedback.

Regarding WebAuthn, I plan to look at your design ASAP, should be this week for sure. I briefly looked and I think it's very good. I think that if something, I will maybe have just some minor things (EG. merge some configuration options similarly like for your PKCE PR).

When you have some prototype ready, I suggest to send PR. For the open questions, which you will face during implementation, it is usually better to stick with keycloak-dev if it is something bigger, which may require bigger discussion. Github PR discussion is fine just for some smaller things IMO. I think that it's good to reference the keycloak-dev ML from your PR and vice-versa to ensure that people are aware of have the information/prototype on more places.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 4, 2019

I've created JIRA tickets as follows :
3. JWE Key Wrapping Support
https://issues.jboss.org/browse/KEYCLOAK-10759
4 Signed and Encrypted ID Token support in Key Wrapping
https://issues.jboss.org/browse/KEYCLOAK-10760
5. signed and encryption for other JWT based token (e.g. Access Token)
https://issues.jboss.org/browse/KEYCLOAK-10761

Copy link
Contributor

@mposolda mposolda left a comment

@tnorimat Thanks for incorporating feedback! Still have few minor things, but IMO we're much closer to have this in.

  • There are 2 left-over methods, which can be removed from the PR now. Added inline comments for the details.

  • It seems that some tests are failing? At least IdTokenEncryptionTest.testIdTokenEncryptionWithoutEncryptionKEK looks failing in travis and this one seems like an issue in this test? Sorry that travis is not very useful right now and contains more failing builds, which means some false-negatives and hide some real issues... Hopefully this is fixed soon.

  • Some small documentation may be needed? IMO it will be nice to add a note to keycloak release notes as well about this (file changes.adoc in the "keycloak-documentation"). So if you can add few sentences about this addition, it will be cool. Thanks!

@@ -54,6 +55,20 @@ public static KeyWrapper getClientPublicKeyWrapper(KeycloakSession session, Clie
return keyStorage.getPublicKey(modelKey, kid, loader);
}

public static KeyWrapper getClientPublicKeyWrapper(KeycloakSession session, ClientModel client, JWK.Use keyUse) {
Copy link
Contributor

@mposolda mposolda Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method can be removed?

Copy link
Contributor Author

@tnorimat tnorimat Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this method has already not been used. I'll remove it.

return encryptedToken;
}

private KeyWrapper getEncryptionKekWrapper(String algAlgorithm, ClientModel client) {
Copy link
Contributor

@mposolda mposolda Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, this private method is not used now and can be removed? This will allow to remove PublicKeyStorageManager.getClientPublicKeyWrapper(session, client, JWK.Use.ENCRYPTION) as well as I mentioned in the comment above.

Copy link
Contributor Author

@tnorimat tnorimat Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this method has already not been used. I'll remove it.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 17, 2019

@mposolda I'm willing to prepare its documentation. I'll send its documentation PR to keycloak-document repository as usual.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Jul 17, 2019

@mposolda As for the error occured in IdTokenEncryptionTest.testIdTokenEncryptionWithoutEncryptionKEK I'm in doubt about https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java#L415 where improper exception handling might occur. I'm still investigating it.

As an aside from it, this error did not occur on my local environment.

This test (fail to load client public key for KEK) throws RuntimeException following other error cases (e.g. fail to sign signature) which causes the failure of the test. To clarify it, adding UncaughtServerErrorExpected annotation to the test method intentionally.

@mposolda
Copy link
Contributor

@mposolda mposolda commented Aug 7, 2019

@tnorimat I see the test passed with your latest changes, so it should be good. The failed adapter-tests can be ignored for now as they are broken in general (not the fault of this PR).

When you remove the unused methods pointed in the comments and once you do the documentation PR, we are good to go.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Aug 8, 2019

@mposolda I'm afraid I can not squash all commits onto one and rebase because I've merged master branch to resolve conflicts.

To get around this situation, I'd like to do the following. WDYT?

  1. close this PR
  2. port these commits of this PR onto the current master branch and issue new PR whose comment includes the reference to this PR.
  3. prepare documentation PR and issue it onto keycloak-documentation repository.

@mposolda
Copy link
Contributor

@mposolda mposolda commented Aug 8, 2019

@tnorimat +1 to the steps you mentioned. Thanks! Also please remove the unused methods pointed in the comments. Then we will be good to go.

@tnorimat
Copy link
Contributor Author

@tnorimat tnorimat commented Aug 13, 2019

the new PR is #6237 .
its documentation PR is keycloak/keycloak-documentation#712 .

@mposolda
Copy link
Contributor

@mposolda mposolda commented Aug 13, 2019

Closing this as it was replaced by #6237

@mposolda mposolda closed this Aug 13, 2019
@tnorimat tnorimat deleted the jwe-idtoken-enc branch Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants