Enhance Builtin users SPI security#11763
Conversation
This comment has been minimized.
This comment has been minimized.
…ON_ID_MATCH to work
This comment has been minimized.
This comment has been minimized.
…n-users-api-auth-enhance
This comment has been minimized.
This comment has been minimized.
…R_AUTH_USE_BUILTIN_USER_ON_ID_MATCH
…hout SPI, for IT test to work as before
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
|
|
||
| We’ve strengthened the security of the `api-bearer-auth-use-builtin-user-on-id-match` feature flag. It will now only work when the provided bearer token includes an `idp` claim that matches the Keycloak Service Provider identifier. | ||
|
|
||
| By enforcing this check, the risk of impersonation from other identity providers is significantly reduced, since they would need to be explicitly configured with this specific, non-standard identifier. |
There was a problem hiding this comment.
Can this be part of the keycloak config, i.e. a static entry added there? Then we don't have to rely on the external provider. This is maybe less relevant for the internal users where you control the code, but it would be more under the site admin's control if there was a static value added to the token by keycloak (that was set for each login type).
There was a problem hiding this comment.
Yes. We could consider making the expected idp identifier configurable via a parameter in the SPI. Dataverse could then retrieve this value from a JVM option when verifying the token.
The main advantage I see in this approach is that it prevents the expected idp identifier claim value from being publicly exposed. This could indeed result in a more secure solution. That said, anyone who can successfully authenticate and obtain a token would still be able to decode it and see the idp claim value. Perhaps I’m missing other advantages of your approach.
There was a problem hiding this comment.
qqmyers
left a comment
There was a problem hiding this comment.
Looks OK to me. I left a question about an ~alternate design, but I think what's here is fine to use now/should help security. I did not try to set this up at all so having QA try the whole setup and checking the tests is important.
|
Looks good to me - merging! |

What this PR does / why we need it:
Security improvements for
api-bearer-auth-use-builtin-user-on-id-matchWe’ve strengthened the security of the
api-bearer-auth-use-builtin-user-on-id-matchfeature flag. It will now only work when the provided bearer token includes anidpclaim that matches the Keycloak Service Provider identifier.By enforcing this check, the risk of impersonation from other identity providers is significantly reduced, since they would need to be explicitly configured with this specific, non-standard identifier.
Which issue(s) this PR closes:
Notes for the reviewer:
I’ve updated the warning in the documentation related to impersonation risk. With this additional security measure in place, user impersonation is unlikely to occur unless an IdP is intentionally misconfigured.
Suggestions on how to test this:
Run the containerized dev environment under
conf/keycloakObtain a bearer token by logging into Keycloak with built-in account credentials
curl -X POST http://keycloak.mydomain.com:8090/realms/test/protocol/openid-connect/token -H "Content-Type: application/x-www-form-urlencoded" -d "client_id=test" -d "client_secret=94XHrfNRwXsjqTqApRrwWmhDLDHpIYV8" -d "grant_type=password" -d "username=dataverseAdmin" -d "password=admin1" -d "scope=openid"Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
Yes, attached
Additional documentation:
N/A