Repository navigation
Introduce mechanism for different trust material sources - #48869
Conversation
eaa2f60 to
831b5fa
Compare
There was a problem hiding this comment.
@dominikschlosser Nice, Thanks a lot!
Few comments.
-
It may be better to use per-client configuration of IDPs rather than "global" configuration at the level of authenticator. Added inline comment with the details.
-
Instead of introducing 2 new IDPs, would it be possible to introduce only single IDP like
DefaultTrustIdentityProvider, which would have options aligned with the OIDC IDP? This means options like:
Use JWKS URL(On/off switch)JWKS URL(Shown just when the previous is ON and it is mandatory in that case)Validating public key id(Shown jsut whenUse JWKS URLis off)Validating public key(Shown just whenUse JWKS URLis off)
This will be aligned with the OIDCIdentityProvider, which already uses those options (note they have tooltips already etc).
Also it can be nice if OIDCIdentityProvider itself implements TrustMaterialIdentityPRovider (or extends from AbstractJwksTrustIdentityProvider). Hopefully both IDP can somehow re-use same code/logic for obtain keys from "JWKS URL" or "hardcoded". WDYT?
For some inspiration, you can maybe take a look at the JWTAuthorizationGrantProvider. This one also has dedicated IDP impl (JWTAuthorizationGrantIdentityProvider) but at the same time, the OIDCIdentityProvider implements it. So this might have some consistency with that.
Also I think that new IDP does not need Client ID and Client secret as configuration options? (In your PR, both new IDPs have those). I guess you added those just by mistake or any need for them?
- I think it can be good if we re-use same mechanism like for ABCA also for the OID4VCI credential-request "proofs" to retrieve attestation keys. As those are currently also configured per-realm, which is not very flexible. But this may be good as a follow-up rather than this PR as this PR is already relatively big. Just for the reference. Related issue: #48414
| jwks.setType(ProviderConfigProperty.TEXT_TYPE); | ||
| jwks.setHelpText("JWKS containing trusted attester public keys"); | ||
| return List.of(jwks); | ||
| ProviderConfigProperty trustIdps = new ProviderConfigProperty(); |
There was a problem hiding this comment.
Is it possible to use the per-client configuration instead of using "global" configuration at the authenticator level?
As I think that in production, realm will have different clients/wallets, but each of them may need different attestation key. For example client my-wallet-1 and my-wallet-11 may need attestations from IDP idp1, when my-wallet2 will need attestations from IDP idp2 .
It should be possible to do this just by moving this config option to the method getConfigPropertiesPerClient() instead of getConfigProperties(). Then in the admin console, it will appear in the client credentials tab. See screenshot
Also you can see for example FederatedJWTClientAuthenticator for some "inspiration" .
There was a problem hiding this comment.
- I will look into it, good suggestion
- The goal of this PR is just to illustrate the mechanism. I agree that there should be only one IdP-implementation for external JWKS and Cert configured in Keycloak itself. But this mechanism should also be extensible for other trust sources like ETSI Trust Lists in the future. Thats why i did it this way here.
- Yes i agree but that would be out-of-scope here
If you agree with the general direction i will rework this PR to be mergeable
|
@dominikschlosser FYI. This will require rebase . Especially the change 6aeccb2 is related and will likely affect this PR as well, but hope in a positive way (as ABCA authenticator is now treated as proper client authenticator) |
376a966 to
4f42fe3
Compare
|
@mposolda Could you please review again? ABCA is included in this PR as first user of the new mechanism. There are of course more places where this can be used but that should be follow-up PRs. |
4f42fe3 to
2f4c20a
Compare
There was a problem hiding this comment.
@dominikschlosser Thanks!
I have few comments when testing your PR and I've ended up sending another PR to your branch dominikschlosser#1 :-) If you are ok with those changes and you merge that, this PR #48869 should be automatically updated. And from my point of view, this would be good to merge.
My changes in that other PR:
-
Make
DefaultTrustIdentityProviderFactoryavailable just when either ABCA or OID4VCI feature is enabled. As this is new thing and should be guarded behind the feature and moreover, it seems this IDP will be useful just if some of those features is also enabled (This PR uses it just for ABCA, but I think it may be useful just for other things as you mentioned. Like for example proofs in OID4VCI credential requests). -
Removed some code duplications (Introduced
JWKSServerUtils.toJwkandTrustKeyUtil.filterKeys). Especially the first one is important as we would need to support multiple key types in the near future (in relation to post-quantum cryptography) and would be nice if related "utility" code for stuff like converting KEyWrapper to JWK is on single place rather than requiring update of many places when new key-type is introduced... -
Updated
AttestationBasedClientAuthenticatorconfig option to be required and use the typeSTRING_TYPE. I agree thatIDENTITY_PROVIDER_MULTI_LIST_TYPEwould be better, but it is currently broken in the admin console (See screenshot below). TheSTRING_TYPEis not so convenient, but it is OK for now as it is exactly the same what alsoSigned JWT- Federatedclient authenticator is using. It would be good to have follow-up issue to fix this (for bothSigned JWT - Federatedas well asAttestation-based), but I consider this out-of-scope of this PR (it is probably bug in the UI component and would require some changes in the admin console UI libraries to fix this...)
Besides that, as a follow-up, it might be good to polish the configuration of Default-Trust Identity provider in the admin console. Right now, it requires configuration of Client ID and Client secret (which is non-sense for this IDP) . Also would be nice to make name of the "JWKS" related options consistent with other IDPs (OIDC Identity Provider, JWT Authorization grant provider). But that can be also a follow-up and not a blocker for this PR IMO...
|
@mposolda Thank you! Good suggestions and changes. I'm actually not sure where client-id/secret config comes from, otherwise i would have removed it... |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BruteForceTest#testMaxTemporaryLockoutsResetKeycloak CI - Forms IT (chrome) |
15f2f3b to
eddf010
Compare
Signed-off-by: Dominik Schlosser <dominik.schlosser@gmail.com>
Signed-off-by: mposolda <mposolda@gmail.com>
eddf010 to
6bb47f1
Compare
There was a problem hiding this comment.
@dominikschlosser Thanks!
|
@dominikschlosser For tracking: Follow-up issues are #49281 (for configuration polishing) and #48414 (for use same mechanism for OID4VCI). |
) closes keycloak#48269 Signed-off-by: Dominik Schlosser <dominik.schlosser@gmail.com> Signed-off-by: mposolda <mposolda@gmail.com> Co-authored-by: mposolda <mposolda@gmail.com>
closes #48269
This PR illustrates a possible approach for #48269
@mposolda @Captain-P-Goldfish @VinodAnandan @tdiesler @dteleguin @thomasdarimont WDYT?