Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OIDC introspection of opaque access-token does not fill principal of SecurityIdentity #12755

Closed
haraldatbmw opened this issue Oct 16, 2020 · 10 comments · Fixed by #16879
Closed
Labels
area/oidc kind/enhancement New feature or request
Milestone

Comments

@haraldatbmw
Copy link
Contributor

Describe the bug
My service uses OIDC (application-type=service) and is called with an opaque access-token. The REST endpoints are protected with @RolesAllowed annotations.

Because of the opaque access-token, the introspect endpoint is called by Quarkus/Vert.x to get the JWT.
Because my identity-provider is ForgeRock the response of the introspection endpoint does not contain a key "username" but "user_id".

Response:

{
  "active": true,
  "scope": "openid profile email myroles",
  "client_id": "***",
  "user_id": "***",
  "token_type": "Bearer",
  "exp": 1602839252,
  "sub": "***",
  "iss": "***",
  "auth_level": 1000,
  "auditTrackingId": "***"
}

The Vert.x and Quarkus-implementation expect the "username" key, so the principal of the SecurityIdentity is not filled.
image

See:

  • io.quarkus.oidc.runtime.OidcIdentityProvider.validateTokenWithOidcServer
  • io.vertx.ext.auth.oauth2.impl.OAuth2TokenImpl.introspect

Expected behavior
Principal of SecurityIdentity gets filled.

Actual behavior
Principal is null

Configuration

quarkus.oidc.application-type=service
quarkus.oidc.auth-server-url=***
quarkus.oidc.client-id=***
quarkus.oidc.credentials.client-secret.value=***
quarkus.oidc.token.issuer=***

quarkus.oidc.authentication.user-info-required=true
quarkus.oidc.roles.source=userinfo
quarkus.oidc.roles.role-claim-path=myroles
quarkus.oidc.discovery-enabled=false
quarkus.oidc.introspection-path=/introspect
quarkus.oidc.user-info-path=/userinfo

Environment (please complete the following information):

  • Quarkus version or git rev: 1.9.0.CR1
@haraldatbmw haraldatbmw added the kind/bug Something isn't working label Oct 16, 2020
@sberyozkin
Copy link
Member

sberyozkin commented Oct 16, 2020

@haraldatbmw Unfortunately it will take awhile to fix it given that all the unrecognized values are dropped in Vert.x.
I can suggest a workaround, call the introspection endpoint manually from the custom SecurityIdentityAugmentor and replace the principal

@haraldatbmw
Copy link
Contributor Author

@sberyozkin Yes sure that will be a valid workaround. I was wondering if all the keys that Vert.x is using are not standardized but the values KeyCloak uses.

@sberyozkin
Copy link
Member

sberyozkin commented Oct 16, 2020

@haraldatbmw username in particular is a standard property, but in general, including the unrecognized properties would be a good idea - then you'd be able to use a custom principal claim property.

@sberyozkin sberyozkin added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Oct 22, 2020
@sberyozkin
Copy link
Member

Labelling it as an enhancement since the introspection response uses a non-standard property

@sberyozkin
Copy link
Member

@haraldatbmw quick question, do you have the required user name returned in the userinfo response ? It may be easier to resolve it via that route

@haraldatbmw
Copy link
Contributor Author

@sberyozkin yes, userinfo-data is my current fallback for this issue.

    @Inject
    SecurityIdentity securityIdentity;

    @Inject
    UserInfo userInfo;

    @GET
    @RolesAllowed({"xyz"})
    public String getData() {

        // principal is null because json of introspection call does not contain a key "username"
        var principal = securityIdentity.getPrincipal();

        var sub = userInfo.getString("sub");
        var roles = securityIdentity.getRoles();

        ...

@sberyozkin
Copy link
Member

sberyozkin commented Nov 4, 2020

@haraldatbmw you can also reset a principal in a custom SecurityIdentityAugmentor, UserInfo is available as a SecurityIdentity userinfo attribute

@KarstenWintermann
Copy link
Contributor

KarstenWintermann commented Apr 23, 2021

Hello @sberyozkin, I believe that

Fix QuarkusSecurityIdentity.isAnonymous check #15730

breaks the possibility for an easy workaround using a SecurityIdentityAugmentor, since now QuarkusSecurityIdentity.Builder raises an IllegalStateException("Principal is null but anonymous status is false") before the SecurityIdentityAugmentors are invoked. So the issue described by @haraldatbmw remains, that there are Identity Providers like Forgerock, which don't include a username in the response from the introspect endpoint.

According to RFC 7662, username is optional, so relying on its presence is a bug.

There should either be a fallback to sub in case username is not present or the claim should be configurable for the tenant.

@sberyozkin
Copy link
Member

@KarstenWintermann sorry I've noticed this message only after seeing your PR. Let me check

@sberyozkin
Copy link
Member

@KarstenWintermann I see, sorry about that - I think QuarkusSecurityIdentityBuilder check in itself is correct - but your PR also takes care of the fallback, let me comment there as well

@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 29, 2021
@gsmet gsmet modified the milestones: 2.0.0.Alpha2, 1.13.4.Final May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants