quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.82k stars 2.69k forks source link

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

Closed haraldatbmw closed 3 years ago

haraldatbmw commented 4 years ago

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:

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):

sberyozkin commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

sberyozkin commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

KarstenWintermann commented 3 years ago

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 commented 3 years ago

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

sberyozkin commented 3 years ago

@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