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

Simplify configuration based mapping of token roles to deployment-specific SecurityIdentity #39269

Closed
michalvavrik opened this issue Mar 7, 2024 · 8 comments · Fixed by #39339
Assignees
Labels
area/security kind/enhancement New feature or request
Milestone

Comments

@michalvavrik
Copy link
Contributor

michalvavrik commented Mar 7, 2024

Description

We have shared HTTP permissions that allow to configure checks and mappings that are always applied when the path is matched in addition to winning most-specific path policy. Here #37989 (comment) it was suggested we could add configuration properties like

quarkus.http.auth.roles-mapping.administrator=admin
quarkus.http.auth.roles-mapping.customer=user

that will always be merged to whatever winning most specific policy mapping is applied. This way, user can avoid repeating path patterns. Also it simplifies token roles to deployment-specific SecurityIdentity roles mapping that right now requires:

quarkus.http.auth.policy.role-policy1.roles.administrator=admin
quarkus.http.auth.policy.role-policy1.roles.customer=user
quarkus.http.auth.permission.roles1.paths=/*        
quarkus.http.auth.permission.roles1.policy=role-policy1
quarkus.http.auth.permission.roles1.shared=true

which also requires that all HTTP requests to application endpoints are authenticated, which is something users need to avoid at many scenarios (app has often public endpoints, or decides to secure some endpoints with standard security annotations instead, of just optionally do stuff if user is authenticated and has certain role etc.).

Implementation ideas

No response

@michalvavrik michalvavrik added the kind/enhancement New feature or request label Mar 7, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 7, 2024

/cc @sberyozkin (security)

@michalvavrik michalvavrik self-assigned this Mar 7, 2024
@sberyozkin
Copy link
Member

Thanks @michalvavrik Sure, the quarkus.security namespace is good for this. But we have to align it with the HTTP policy way too, what happens if both quarkus.security.root.roles and quarkus.http.auth.permission.roles1.shared=true is set...
Probably, since HTTO policy is more specific - every shared policy should accumulate root roles set in quarkus.security.root.roles

In general it is not only about making a few properties to set for the @RolesAllowed case, but also avoiding having to set the path twice, once with @Path and then also with HTTP Security policy

By the way, there was another issue related to optimizing things for the @RolesAllowed case, was it for roles to group mapping ?

@sberyozkin
Copy link
Member

I see, you've created the issue for optimizing the mapping of the roles but linked to sharing the policies :-), if I'm not confused, maybe both cases can be covered with this issue

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Mar 7, 2024

But we have to align it with the HTTP policy way too, what happens if both quarkus.security.root.roles and quarkus.http.auth.permission.roles1.shared=true is set...

Yeah, I definitely didn't plan to align anything. I thought the idea was to define roles that are always mapped, regardless of path and policies. That was your suggestion I remembered, hope I'm not confusing it...

Probably, since HTTO policy is more specific - every shared policy should accumulate root roles set in quarkus.security.root.roles

+1

In general it is not only about making a few properties to set for the @RolesAllowed case, but also avoiding having to set the path twice, once with @path and then also with HTTP Security policy

Hmm, I think there was some suggestion for @RolesAllowed attribute role mapping, but we can't use it as we don't own the annotation. Also TBH, annotation values are not as flexible and configurable as SR Configuration source like application.properties. It allows greater variability.

I don't remember details.

By the way, there was another issue related to optimizing things for the @RolesAllowed case, was it for roles to group mapping ?

No idea, sorry. Please give me some hint.

I see, you've created the issue for optimizing the mapping of the roles but linked to sharing the policies :-), if I'm not confused, maybe both cases can be covered with this issue

I couldn't really remember where and what we agreed on, so I took a guess, linked shared policies and made some rationale from your reminder :-)

@michalvavrik
Copy link
Contributor Author

Sergey, thinking of it, if you suggest something else than in the issue description, you will have to adjust the description or create new issue because I'm bit lost. Sorry. This was the only thing I had in a memory.

@sberyozkin
Copy link
Member

@michalvavrik OK, lets try to figure out what we'd like to optimize.
What I thought you'd look at is at optimizing the mapping as suggested at the end of https://github.com/quarkusio/quarkus/issues/37989 , you were ok with that suggestion.

And you name this issue accordingly, but you refer to the different situation, where roles can be shared with other policies which is not related to the problem of mapping. See what I mean ?
So may be you can rename this issue to deal with optimizing the sharing of roles for the @RolesAllowed case and I can create an issue to optimize the mapping ?

@michalvavrik
Copy link
Contributor Author

I understand it now, alright, let me update the description.

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Mar 8, 2024

This is on hold till #39236 is in because it requires touching same lines of code and I want to avoid conflicts. Will have a look after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/enhancement New feature or request
Projects
None yet
2 participants