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

Requiring the 'groups' claims is too restrictive #129

Closed
sberyozkin opened this issue Apr 12, 2019 · 15 comments · Fixed by #179
Closed

Requiring the 'groups' claims is too restrictive #129

sberyozkin opened this issue Apr 12, 2019 · 15 comments · Fixed by #179
Assignees
Milestone

Comments

@sberyozkin
Copy link
Contributor

I have just seen a user reporting that the MP-JWT runtime fails to validate Auth0 tokens. I think it is a major barrier to the wider adoption of MP JWT given that many popular OIDC providers will not be able to include the groupsclaim.
IMHO the 'group' claim should be treated somehow similarly to the (required) upn claim. For example, one option could be for the MP JWT implementation to default to 'Default' groups or some other default value.
Rejecting the otherwise perfectly valid tokens used in the productions to secure the services is too restrictive

@crivera
Copy link

crivera commented Apr 12, 2019

I like this idea.

In the properties file we define what the auth mechanism is:

quarkus.smallrye-jwt.auth-mechanism: MP-JWT

Maybe we could have a second one which is just regular JWT which would support Auth0 & Firebase Auth.

@sberyozkin
Copy link
Contributor Author

Thanks... MP-JWTis just a trigger for the MP-JWT implementations. The token can be coming from anywhere, self-issued or provider-issued, so the spec might require some minor adjustments to recommend what should be done if a `groups' claim is missing to ensure a given MP-JWT implementation can still get the current token accepted

@ospringauf
Copy link

+1 ... the groups claim gives me a major headache. Our OpenID provider has no access to fine-grained application/business roles and cannot provide them in the ID token. Therefore, it is currently incompatible with MP-JWT.
It would be great to have a default behavior for missing groups defined in the MP-JWT spec. Even better if we could plug in our own roles provider. I know I can do this with JSR-375 IdentityStores, but I'd love to see a solution in Microprofile.

@sberyozkin
Copy link
Contributor Author

We introduced a smallrye.jwt.claims.groups property which offers an optional default value if the token has no such a claim. We may generalize it to provide some other missing claims, example, smallrye.jwt.claims.abc.

So may be MP JWT can introduce a similar property, mp.jwt.claims.groups ?

@aerodc
Copy link

aerodc commented Jul 15, 2019

any updates on this?

@MikeEdgar
Copy link
Member

Another possible approach would be to update the spec to include a collision resistant namespace for the new claims introduced (i.e. groups and upn). For example, replace groups with https://microprofile.io/jwt-auth/groups, or something similar.

This would be in alignment with RFC 7519, section 4.2 and should be supported by Auth0, for example. Namespaces are supported currently in Payara as well.

@sberyozkin
Copy link
Contributor Author

@MikeEdgar Not sure introducing the namespace would solve this issue though which is really about the implementations being effectively required to reject the tokens without the groups .
I suggest we make this claim optional as so many providers can't provide it which restricts MP JWT reach

@sberyozkin sberyozkin added this to the JWT-1.2 milestone Jan 30, 2020
@sberyozkin
Copy link
Contributor Author

sberyozkin commented Mar 20, 2020

I propose to resolve this issue by updating the spec text to note that

Implementations are required to use a `scope`claim for RBAC if no `groups` claim is available where a scope claim is a string containing one or more values separated by a space character.

Implementations can use the roles information from the other claims if neither `groups` nor `scope` claim are available in the token but the users should be aware that it will be an implementation specific solution.

JWT tokens which contain no roles information can only be used for the authentication. Such tokens can only be used to invoke the service methods with `@RolesAllowed("*")` or `PermitAll` or `DenyAll` annotations or with no RBAC annotations.

Something along these lines

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Mar 22, 2020

However it appears we can't do it for 1.2 if such a text constitutes a breaking change. Example, processing a scope may lead to an unexpected successful call going through, so we can definitely drop a requirement to support scope and have it for 2.0. Likewise, listing all those annotations is a bit too much for the spec text. Simply stating:

Implementations can use the roles information from the other claims if no `groups` claim is available in the token but the users should be aware that it will be an implementation specific solution.
JWT tokens which contain no roles information can only be used for the authentication.

should be enough.

The only problem then is that some implementations may return 401/403 if no groups exists before the RBAC barrier is even reached.
I think it will be beneficial to relax a groups requirement as per the suggested fragment but I'd like to ask
@rdebusscher, @dblevins and other MP JWT colleagues if they see any problem for 1.2 with it.
If there will be no objections then I'll go ahead with the proposed update, otherwise - mark this issue for 2.0

thanks

@radcortez
Copy link
Contributor

I see no issues in doing this for 1.2.

@rdebusscher
Copy link
Member

Defining a required property to optional is not a breaking change. All tokens which are valid today are still valid with the change in groups and scope and will result in the same behavior.

So it can be included in 1.2

@radcortez
Copy link
Contributor

Should we also provide a way to retrieve the roles from another claim? With a configuration property?

@rdebusscher
Copy link
Member

Should we also provide a way to retrieve the roles from another claim? With a configuration property?

A Mapper SPI is more useful in that case, and the default mapper takes it from groups claim.

@sberyozkin
Copy link
Contributor Author

@radcortez @rdebusscher OK, if everyone is good with it then I'll relax the text with a couple of tests, thanks

@sberyozkin
Copy link
Contributor Author

@andreas-eberle See @rdebusscher's response to Roberto's proposal :-). May be we can do it for 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants