Skip to content

KEYCLOAK-19699 RSA key provider with key use = enc cannot select corresponding algorithm on Admin Console#8708

Merged
mposolda merged 1 commit into
keycloak:mainfrom
Hitachi:KEYCLOAK-19699
Nov 18, 2021
Merged

KEYCLOAK-19699 RSA key provider with key use = enc cannot select corresponding algorithm on Admin Console#8708
mposolda merged 1 commit into
keycloak:mainfrom
Hitachi:KEYCLOAK-19699

Conversation

@tnorimat

@tnorimat tnorimat commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

This PR is for fixing a bug reported by keycloak-dev and by KEYCLOAK-19699 that describes it in detail.

mposolda
mposolda requested changes Nov 5, 2021
View reviewed changes

@mposolda mposolda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tnorimat Thanks Takashi!

I've added one minor comment inline.

One more question: Is this change backwards compatible? I wonder if someone already used rsa-generated provider with the ENC use (even if he was not able to select algorithm before). Or wasn't it possible before this PR at all to use rsa-generated for the ENC?
If backwards compatibility is an issue, I wonder that possible alternative solution might be instead of introducing new provider, just change the method isSupportedRsaAlgorithm to return algorithms based on whether the use is SIG or ENC. But I am not sure... If you think this PR is fine from backwards compatible perspective, I am ok with the approach you used in the PR.

Comment thread services/src/main/java/org/keycloak/keys/GeneratedRsaKeyProviderFactory.java Outdated

@mposolda mposolda Nov 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the help text be updated to explicitly mention that it is used for "signatures" ? Ideally align it with the help text you used for new "enc" based provider

@tnorimat tnorimat Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mposolda Yes, I will change the help text as follows.

"Generates RSA signature keys and creates a self-signed certificate"

@mposolda

mposolda commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

@tnorimat ah, and I see that there are failing tests. This would also require fix...

@mposolda mposolda self-assigned this Nov 5, 2021
@tnorimat

tnorimat commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

@mposolda It seems that test itself have problems. I'll try to fix them.

OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingKid
OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingRSA_OAEPWithA128CBC_HS256
OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingRSA_OAEP_256WithA256GCM
OIDCAdvancedRequestParamsTest#testSignedAndEncryptedRequestObject
OIDCAdvancedRequestParamsTest#testWrongContentEncryptionAlgorithm
OIDCAdvancedRequestParamsTest#testWrongEncryptionAlgorithm

@tnorimat

tnorimat commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

@mposolda Also, I will consider backward compatibility issue and get back to you.

@tnorimat
tnorimat force-pushed the KEYCLOAK-19699 branch 2 times, most recently from 702c8f8 to ef388fd Compare November 5, 2021 20:55
@tnorimat

tnorimat commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

@mposolda I've concluded that this original PR does not break backward compatibility. The reason is as follows.

  • The current keycloak does not look at an algorithm for key provider whose key use is "enc".
    The key provider whose key use is "enc" has been used for encrypting client generating JWT (e.g. Request Object).
    The current keycloak uses this key provider to decode this encrypted JWT in DefaultTokenManager.decodeClientJWT .

keycloak/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java

Lines 133 to 140 in 58d403c

if (kid == null) {
activeKey = keys.filter(k -> KeyUse.ENC.equals(k.getUse()) && k.getPublicKey() != null)
.sorted(Comparator.comparingLong(KeyWrapper::getProviderPriority).reversed())
.findFirst();
} else {
activeKey = keys
.filter(k -> KeyUse.ENC.equals(k.getUse()) && k.getKid().equals(kid)).findAny();
}

These codes check key use but do not check key algorithm. Therefore, the existing key provider whose key use is "enc" and key algorithm is like "PS256" still works well.

However, to check this backward compatibility by the following existing tests, I've modified the codes of GeranatedRsaKeyProviderFactory.create .

OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingKid
OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingRSA_OAEPWithA128CBC_HS256
OIDCAdvancedRequestParamsTest#testRealmPublicKeyEncryptedRequestObjectUsingRSA_OAEP_256WithA256GCM
OIDCAdvancedRequestParamsTest#testSignedAndEncryptedRequestObject
OIDCAdvancedRequestParamsTest#testWrongContentEncryptionAlgorithm
OIDCAdvancedRequestParamsTest#testWrongEncryptionAlgorithm

@tnorimat

tnorimat commented Nov 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

There are two options to implement this feature.

  1. Add new key provider for "enc"
  2. Make existing key provider show algorithms depending on its key use

I've taken the fist option by the following reasons.

  • By leveraging angularJS's feature, the existing key provider's configuration UI is automatically rendered without its own javascript codes/HTML/FTL. I'm afraid that if I took the second option, I would have to add new javascript logic and FTL/HTML.

  • All other key provider does not allow keycloak admin to select key use.
    aes-generated(enc), ecdsa-generated(sig), hmac-generated(sig), java-keystore(sig), rsa(sig)
    I've followed this style.
    rsa-generated(sig), rsa-enc-generated(enc)

mposolda
mposolda approved these changes Nov 16, 2021
View reviewed changes

@mposolda mposolda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tnorimat Thanks! This looks good to me.

I will merge after Keycloak 16 release (AFAIK we try to avoid merge non-critical issues into Keycloak master now to avoid the potential regressions in the Keycloak 16 release).

@mposolda mposolda added this to the 17.0.0 milestone Nov 16, 2021
@mposolda
mposolda merged commit 10c3e14 into keycloak:main Nov 18, 2021
@tnorimat tnorimat mentioned this pull request Nov 20, 2021
Merged
@stianst stianst modified the milestones: 17.0.0, 15.1.0 Dec 10, 2021
@tnorimat
tnorimat deleted the KEYCLOAK-19699 branch January 6, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda approved these changes

Assignees

@mposolda mposolda

Labels

None yet

Projects

None yet

Milestone

15.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tnorimat @mposolda @stianst