Repository navigation
Add a client policy executor for the JWT authorization grant settings - #51720
mahdi-al-hakim wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens dynamic client registration against client-controlled JWT authorization grant trust configuration.
Changes:
- Restricts identity-provider and audience attributes to administrators.
- Prevents clients from re-enabling the grant during updates.
- Adds regression and authorization-path tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
AbstractClientRegistrationProvider.java |
Enforces JWT grant configuration restrictions. |
ClientRegistrationAuth.java |
Exposes bearer-token detection within the package. |
ClientRegistrationTest.java |
Tests protected attributes and authorization paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| String requested = attributes.get(OIDCConfigAttributes.JWT_AUTHORIZATION_GRANT_ENABLED); | ||
| if (!Boolean.parseBoolean(requested)) { | ||
| return; |
There was a problem hiding this comment.
Good catch, added as disableJWTAuthorizationGrantViaRegistrationTokenShouldSucceed. An administrator turns the grant on, then the client turns it back off through its registration access token, and the test asserts both that the update succeeds and that the stored value is false. It fails if that branch is tightened to reject every write of the attribute.
I kept it on the default registration endpoint rather than going through OIDC grant_types, because this test class only uses that endpoint and adding the OIDC representation machinery here would be out of place. The value the client sends is the one DescriptionConverter writes for a registration request without the grant type in grant_types, and there is a comment in the test saying so. Happy to move it to an OIDC flavoured test if you would rather see the full round trip.
There was a problem hiding this comment.
That test went away with the revert. The equivalent now does the full round trip through the OpenID Connect endpoint, which is what was suggested here.
It is testClientCanDropTheGrantTypeButNotAddItBack. A client is registered with the grant, an administrator adds the allow lists, and the client then drops the grant from its own grant_types using its registration access token. That succeeds and the stored value ends up false. Putting it back is refused, and the allow lists the administrator set are still there.
A second test registers a client that asks for the grant while the policy is active, so the grant_types path is exercised and not only the default endpoint.
2c6bc34 to
dc4f5cd
Compare
|
The second guard, the one that stops a client turning the grant back on, rested on a code read. I ran it against a stock 26.7.1 without this change, so the state it protects is something observed rather than argued. An administrator configures the client and the grant works. The administrator sets Arm A is there so a failure cannot be read as the rig never working, and arm B so it cannot be read as the disable having no effect. C1 and C2 use only the registration access token and the client secret. The script creates its own realms, deletes them at teardown and voids instead of passing if either control misbehaves. Happy to attach it if that is useful. |
There was a problem hiding this comment.
@mahdi-al-hakim Thanks for the PR. I've added inline comment. Can you please check?
| * providers the client accepts assertions from, and which token audiences are valid for it, so they | ||
| * are administrator configuration and must not be set by the client itself. | ||
| */ | ||
| private static final Set<String> JWT_AUTHORIZATION_GRANT_TRUST_ATTRIBUTES = Set.of( |
There was a problem hiding this comment.
Referencing the JWT_AUTHORIZATION_GRANT_TRUST_ATTRIBUTES directly in this file is not good approach and it lacks flexibility as for some deployments (where client administrators are trustful), it is fine to allow updating this.
Please see my comment here about possible approach to solve this (AFAIK same was replied to you by @lhanusov yesterday on security mailing list) : #51718 (comment) .
There was a problem hiding this comment.
You are right, and I have reverted that approach completely. The PR no longer modifies any existing Java file, so nothing references those attribute names inside AbstractClientRegistrationProvider any more.
I took option 1 from your list and added a client policy executor, jwt-authorization-grant-settings-disabled. It follows FullScopeDisabledExecutor, handles REGISTER and UPDATE, and therefore covers both dynamic client registration and the admin REST API.
On the flexibility, the useful part is the scoping rather than just leaving the executor out. On its own it applies to whoever the policy matches, administrators included. Combined with the client-updater-context condition restricted to ByRegistrationAccessToken, ByInitialAccessToken and ByAnonymous, it refuses the client and leaves administrators free to configure it, which is the deployment you described. There is a test for that composition and the guide shows it. Being explicit about the cost: this is opt in, so the issue stays open in the default configuration until someone builds the policy. A flag on the identity provider is the option that could ship enabled, and I am happy to do that instead or as well.
On backwards compatibility, the executor rejects only a change, never the presence of a value. It compares the proposed value against ClientCRUDContext.getTargetClient(), which is the stored client on update and null on register. That matters because an update sends the whole representation back, so a client that already has Custom audience mapping re-sends it on every unrelated edit and must not be rejected for it. There is a test that turns the policy on over a client that already has both settings, edits an unrelated field, and asserts the settings survive and the edit succeeds.
Documentation is on the page you linked and in the admin guide, matching where the sibling executors are documented. On your page the executor is described in the existing Client policies and JWT Authorization Grant list next to downscope-assertion-grant-enforcer and jwt-claim-enforcer, and the Custom audience mapping note now ends with a pointer to it. The executor list in client-policies.adoc gets a bullet as well.
Two things worth your view.
I know #46532 removed a JWTAuthorizationGrantAudienceExecutor from this area in favour of the client level attribute. I read this as a different concern, since it governs who may configure the setting rather than how audiences are validated at grant time, and it is option 1 from your list. If that history means you would rather not add another executor here, the identity provider flag is a small change and I am happy to do that instead.
I have handled oauth2.jwt.authorization.grant.enabled rather than leaving it. It cannot be treated like the other two, because DescriptionConverter writes it from the standard grant_types field and rejecting every write would reject a spec compliant registration. So the executor only refuses turning it on for a client that already exists, where the allow lists configured earlier are still present and the flag alone is enough to make the grant work again. Turning it off stays allowed, so a client can still drop the grant type from its own registration.
dc4f5cd to
a3ed4f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/base/src/test/java/org/keycloak/tests/client/policies/JWTAuthorizationGrantSettingsDisabledExecutorTest.java:236
- This only exercises the admin REST update. The compatibility branch that lets a client disable an administrator-enabled grant through its registration access token/OIDC
grant_typesis not covered, so please restore a dynamic-registration regression test for that path.
// turning it off is still permitted
updateClientByAdmin(realm, cId,
c -> c.getAttributes().put(OIDCConfigAttributes.JWT_AUTHORIZATION_GRANT_ENABLED, "false"));
a3ed4f8 to
26d6c1a
Compare
|
@mposolda I have addressed your review by switching to the client policy executor approach. I have re-requested review. Let me know if there is anything else you would like changed. |
26d6c1a to
d8882f8
Compare
|
@mposolda Rebased on main and trimmed the description. On reusing the client attributes condition: it matches on the client's stored attributes, so on register there is nothing to match yet and on update it sees the old value rather than the one being set. It also only votes on whether a policy applies, so rejecting the change still needs an executor. The WebAuthn IT failure before the rebase was #52074, the flaky |
|
@mahdi-al-hakim please, can you rebase it once again? then I will do a final review and ask for merge, thanks for your help and patience 👍 |
d8882f8 to
ef973df
Compare
ef973df to
2959ea1
Compare
2959ea1 to
6a72cf8
Compare
6a72cf8 to
7bd7559
Compare
The identity provider allow list and the token audience allow list decide which assertions the JWT authorization grant accepts for a client, so a deployment may not want client administrators to change them. The new executor rejects a request that sets or changes either attribute, on both dynamic client registration and the admin REST API, and only where a client policy puts it. Only a change is rejected, so an existing configuration is preserved and an unrelated edit that sends the whole client representation back is still allowed. Closes keycloak#51718 Signed-off-by: mahdi-al-hakim <mahdi.a.alhakim@gmail.com>
7bd7559 to
f58d46c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Although no confirmed defects were identified, this security-sensitive change warrants human validation of administrator and dynamic registration behavior, including integration-test results.
1 open issue
1 resolved since last review
Review effort: Balanced
Closes #51718
Adds a client policy executor,
jwt-authorization-grant-settings-disabled, that rejects requests changing the two client attributes controlling which assertions the JWT authorization grant accepts:oauth2.jwt.authorization.grant.idpandoauth2.jwt.authorization.grant.audience. It followsFullScopeDisabledExecutorand handles REGISTER and UPDATE, so dynamic client registration and the admin REST API are both covered.Only a change is rejected. An update sends the whole client representation back, so a request repeating a value the client already has still passes and unrelated edits keep working. Enabling the grant on an existing client is rejected. Turning it off is allowed.
Combined with the
client-updater-contextcondition the policy can be limited to the client's own requests, leaving administrators free to configure the client.Client policies only bind the paths that fire client policy events, so realm import and partial import are not covered. Both need
manage-realm, which can disable the policy anyway.Docs updated in
docs/guides/securing-apps/jwt-authorization-grant.adocanddocs/documentation/server_admin/topics/clients/client-policies.adoc, alongside the sibling executors.Eleven integration tests in
JWTAuthorizationGrantSettingsDisabledExecutorTest, all passing.AI agents were used to help analyse the code paths and draft this change. I have reviewed and verified all of it.