More complete SecurityEvents

[mrickly]

Description

I have noticed the following when using SecurityEvents:

• An AuthorizationSuccessEvent is fired when a REST resource or method annotated with @PermitAll is called. However the SecurityIdentity of that event is null. I'd rather know the SecurityIdentity even in that case.
• When a method (as opposed to a REST resource) is annotated with an RBAC annotation, then the event properties of the AuthorizationSuccessEvent or AuthorizationFailureEvent fired has not RoutingContext. It would be nice to have a way to determine the method.

Implementation ideas

No response

[quarkus-bot]

/cc @sberyozkin (security)

[mrickly]

@sberyozkin : Do you have an opinion on this enhancement proposal?

[michalvavrik]

An AuthorizationSuccessEvent is fired when a REST resource or method annotated with @permitAll is called. However the SecurityIdentity of that event is null. I'd rather know the SecurityIdentity even in that case.

We don't have the identity available when proactivate authentication is disabled and permit all check is used because the authentication is not required. However when the proactivate authentication is enabled, we can do that.

Reason why is it not there is just an implementation detail (basically we use deferred identity in these parts of the code, so we would need to take it from the event instead if and only if proactive=true).

When a method (as opposed to a REST resource) is annotated with an RBAC annotation, then the event properties of the AuthorizationSuccessEvent or AuthorizationFailureEvent fired has not RoutingContext. It would be nice to have a way to determine the method.

Basically the issue is that security checks on CDI beans (not resources) are performed in Quarkus Security extension that has no knowledge of Vert.x, therefore it doesn't know about the RoutingContext. I'd expect that SecurityIdentity will have attribute with the RoutingContext set. Check the attributes. Or access it programmatically from the Arc container.

With that said, I am glad you reported and I don't say you are wrong. Thanks!

[mrickly]

Hello @michalvavrik , thank you for your reply. Could you clarify this statement for me?

However when the proactivate authentication is enabled, we can do that.

Does that mean that you could do it, but don't do it currently? Apparently, it is not done, since I get an authentication event with the correct principal, but the AuthorizationSuccessEvent has no SecurityIdentity.

[michalvavrik]

Does that mean that you could do it, but don't do it currently?

Yes. It's fairly easy to fix. There was a technical reason why I didn't do it at first (I tried to give background to that decision above), but you are right, it belongs there when proactive auth is enabled.

[mrickly]

Thanks for confirming. Regarding the second question, you wrote

I'd expect that SecurityIdentity will have attribute with the RoutingContext set. Check the attributes. Or access it programmatically from the Arc container

It looks like the SecurityConstrainer.check method gets the location (the full name of the method annotated with RolesAllowed), but that information is not passed as an attribute of the AuthorizationSuccessEvent/AuthorizationFailureEvent.

[michalvavrik]

It looks like the SecurityConstrainer.check method gets the location (the full name of the method annotated with RolesAllowed), but that information is not passed as an attribute of the AuthorizationSuccessEvent/AuthorizationFailureEvent.

During the review of the PR that added these Security events, some reviewers had doubts whether these kind of information are useful to users. We basically thought that user feedback will be best to determine what is it that should be in the event attributes :-).

So I take it you would like there:

• security check like RolesAllowedSecurityCheck so that you know it was denied roles
• reference to the annotated method

If I get it right, sure I agree, though others might have other opinions (cc @sberyozkin ). I'll propose this in the PR and let see how it goes.

the most tricky part if this issue that you opened is adding RoutingContext when security checks are performed on CDI beans rather then resources as Security extension has no link to the Vert.x and it shouldn't. It would require additional CDI bean in the SPI which seems like unnecessary considering at that point, you already have RoutingContext available in the CDI request context and inside SecurityIdentity attributes. So if and only if the RoutingContext is not there, I suggest 2 options:

SecurityIdentity identity = getIdentity();
var ctx = identity.getAttribute(RoutingContext.class.getName());

# this will not work always because some security checks like HTTP Permissions are performed very early (ASAP)
# that is why you should prefer getting the RoutingContext from the SecurityEvent
ctx = Arc.container().instance(RoutingContext.class).isAvailable() ? Arc.container().instance(RoutingContext.class).get() : null;

@mrickly if you can agree that we will just document how you get RoutingContext then I'll address it. Any complains or other questions? Shoot!

[mrickly]

@michalvavrik : To summarize what we would like:

• identity in authorization events (even PermitAll), provided proactive authentication is enabled
• method annotated with RolesAllowed in authorization events (as a substitute for the routing context when it isn't available)
• roles in the RolesAllowed annotation in the case of an AuthorizationFailedEvent

I am not sure that we need the routing context at all cost (if the method name is available instead). In any case, none of the two options seem to work currently for a RolesAllowed check performed in a bean downstream from the REST resource: the ctx from the identity is null and any method call on the RoutingContext returned by Arc.container().instance(RoutingContext.class).get() throws a jakarta.enterprise.inject.IllegalProductException.

[michalvavrik]

I am not sure that we need the routing context at all cost (if the method name is available instead). In any case, none of the two options seem to work currently for a RolesAllowed check performed in a bean downstream from the REST resource: the ctx from the identity is null and any method call on the RoutingContext returned by Arc.container().instance(RoutingContext.class).get() throws a jakarta.enterprise.inject.IllegalProductException.

Knowing the implementation details this is hard to believe. I'll try to reproduce it but openly I'm quite busy ATM. I suppose you don't have a reproducer (some code I could run or look into?) These things implemented differently based on REST stack and config you use (Quarkus REST vs RESTEasy Classic, proactive auth enabled / disabled). If you had a repro, I could promise to look this week.

[michalvavrik]

• identity in authorization events (even PermitAll), provided proactive authentication is enabled
• method annotated with RolesAllowed in authorization events (as a substitute for the routing context when it isn't available)

sounds reasonable to me

• roles in the RolesAllowed annotation in the case of an AuthorizationFailedEvent

I'm not sure TBH, there would be a cost and you can use method reference to look this up. On the other hand it sounds very user friendly from Quarkus to say: here is failed security check method and required roles while here is your identity that is missing these roles. We can let others to comment on this and decide based on the feedback? Can you provide insight why is it useful to you when you have method reference, are you going to export it or something like that?

[mrickly]

I agree that the roles in the RolesAllowed annotation in the case of an AuthorizationFailedEvent is more of a nice to have. It's true that if a user gets a 403 from somewhere, he will probably make sure to obtain the required roles. In general, we have the requirement that security relevant actions (covered by the events) must be logged for security audit purposes.

I will try to provide a minimal reproducer for my claim today or tomorrow.

[michalvavrik]

I agree that the roles in the RolesAllowed annotation in the case of an AuthorizationFailedEvent is more of a nice to have. It's true that if a user gets a 403 from somewhere, he will probably make sure to obtain the required roles. In general, we have the requirement that security relevant actions (covered by the events) must be logged for security audit purposes.

oki, thanks for the insight

I will try to provide a minimal reproducer for my claim today or tomorrow.

thank you (I can probably deal with non-minimal if it was for some reason hard :-))

[mrickly]

quarkus-40170-reproducer.zip
@michalvavrik : This is a small reproducer with a test for the standard case (RolesAllowed on the resource) and the non-standard case (RolesAllowed delegated to a bean method). The second test shows that:

1. event.getSecurityIdentity().getAttribute(RoutingContext.class.getName()) is null
2. Arc.container().instance(RoutingContext.class).get().currentRoute().getPath() throws IllegalProductException.

[michalvavrik]

thanks @mrickly ,

Arc.container().instance(RoutingContext.class).get().currentRoute().getPath() throws IllegalProductException

okay so it seems that async observer is activated with a brand new CDI request context which is not pre-populated with the RoutingContext

event.getSecurityIdentity().getAttribute(RoutingContext.class.getName()) is null

I believe this is @TestSecurity specific and you won't reproduce this in a production unless you have a custom HttpAuthenticationMechanism. However it is still a bug (in the quarkus-test-security) I'll look into ways of fixing it. I'll also re-check if we cannot just add the RoutingContext to the event itself without bigger cost, though I think it cannot be avoided.