KEYCLOAK-10761 Add support for encrypted access tokens#6796
KEYCLOAK-10761 Add support for encrypted access tokens#6796thomasdarimont wants to merge 3 commits intokeycloak:masterfrom
Conversation
tnorimat
left a comment
There was a problem hiding this comment.
I've made 2 comments. I would be happy if you check them.
|
@tnorimat thanks for your review! I adapted the PR according to your suggestions. |
b2a6302 to
7dc25cc
Compare
|
Haven't had a chance to look at this yet, but how does this work with the token introspection endpoint, and things like the userinfo endpoint? For the token introspection endpoint it is obviously important that it can't be used by any client to decrypt the token, as that would then to some extent defeat the purpose of encrypting it. Keycloak can't really even decrypt the token can it? For the userinfo endpoint I would imagine that Keycloak can't actually verify the token, so can't use the token for this purpose. Does this also have some impact on authz services, or anything else where the app may pass the token to Keycloak? Token exchange perhaps? |
There was a problem hiding this comment.
@thomasdarimont @stianst Your concerns are relevant.
in addition to them, I think we are also missing some other parts in regards to ID Token encryption (which we already support). For instance, endpoints using id_token_hint (e.g.: logout) do not seem to support encrypted ID Tokens. Probably a separate issue.
For this particular change, I think we also need to change the TokenVerifier to support encryption. This one is used in several places today to check signatures and decode tokens. Including UserInfo and Introspection endpoints.
For authorization services, the same is also true.
@thomasdarimont Would you mind providing tests for userinfo and introspection endpoints ?
7dc25cc to
5d7af6b
Compare
|
I just rebased this on the current master. I'll take a look into tests for userinfo and introspection endpoints. |
6e85e1d to
06234cd
Compare
|
@thomasdarimont Any updates regarding the tests as discussed in previous comments? |
Leveraged existing IDToken encryption support for AccessToken encryption. Note: Signed & Encrypted JWTs need cty: "JWT" (the content-type) in the JWE header.
06234cd to
5a9c5eb
Compare
|
@pedroigor I had no time to look at it yet. I just rebased the stuff on top of current master. |
|
@thomasdarimont Yeah, making sure both user-info and introspection endpoints work with your changes should be OK. |
|
Hey @thomasdarimont. Were you able to test encrypt tokens using both userinfo and introspection endpoints? |
|
@thomasdarimont did you get a chance to take a look? |
|
I am closing the PR due the lack of feedback. @thomasdarimont Feel free to re-open anytime you have a chance to address the comments above. |
|
@mposolda sorry for letting this go stale again... I didn't have much time lately and therefore I couldn't work on this as much as I hoped. Also this is getting quite complex and I think I need some additional guidance or even pair programming. It currently feels like a walk through the desert with lots of scorpions, thorny bushes, sandsnakes and misguiding Fata Morganas, which might feel a bit less uneasy with some company ;-) |
Leveraged existing IDToken encryption support for AccessToken encryption.
Added appropriate configuration options to "Advanced OIDC Client Settings" in admin-console.
Note: Signed & Encrypted JWTs need cty: "JWT" (the content-type) in the JWE header.