Improve Security events by assuring we add identity and RoutingContext whenever available and help to locate security check with secured method property

[michalvavrik]

fixes: #40170

Changes:

1. • change: when proactive auth is enabled and identity available, add it to the Security event but never ever perform auth when only PermitAll check
   • logic behind change: the identity cannot be access from CDI request context when async observer is used as the next context is created
2. • change: add RoutingContext to security event when security check is performed down on the CDI bean (not JAX-RS resource)
   • logic behind change:
     • users may not understand it is not there as Quarkus Security extension has no concept of Vert.x Routing Context
     • users cannot access it from the observer method as new CDI request context is created for async observer
     • even on CDI beans you want to have HTTP request context (like request path, headers, ...)
3. • change: add class and method name to the security event properties when security checks are performed for standard security annotations
   • logic behind change:
     • when using standard security annotations on multiple levels (JAX-RS resource -> CDI bean -> ..?) different security checks are performed, for example endpoints requires @RolesAllowed({"user", "admin"}) and dynamically resolved bean requires @RolesAllowed("admin")
     • it is useful to know where the check failed and it cannot be derived from thrown exception stacktrace in same cases (reactive data types, ...)
4. as I already needed to make SecurityConstrainer a synthetic bean, I also improved runtime config detection so that we avoid synchronized block when possible (that's just few lines)

[sberyozkin]

Is SecurityIdentity needed ? It is already available in https://github.com/quarkusio/quarkus/blob/main/extensions/security/runtime-spi/src/main/java/io/quarkus/security/spi/runtime/AbstractSecurityEvent.java#L19

[michalvavrik]

You have good eyes, this is the weakest place of this PR and I wasn't sure how to deal with it.

• In Quarkus Security I cannot take the identity from RoutingContext.
• Inside SecurityConstrainer, I cannot call:
  https://github.com/quarkusio/quarkus/blob/main/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/SecurityIdentityAssociation.java#L62 because it would trigger authentication.
• I cannot add a new method like SecurityIdentityAssociation#getIdentityIfPresent, because visibility would have to be public (it's different package) and I wonder if it would confuse users [though IMHO they must use CurrentIdentityAssociation in order to program against interface].

so the identity must come from Vert.x HTTP. I put it to the properties and I absolutely agree it's strange to have the identity in properties only in this scenario.

• The easiest way would to to simply remove it before creating the event somewhere around here (outside that IF):

  quarkus/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityConstrainer.java

  Line 132 in aeba22d

  identity = (SecurityIdentity) additionalEventProps.get(SecurityIdentity.class.getName());

• if that is not acceptable, I would require a new runtime SPI class, which I tried to avoid because users work with the SPI (reproducer of this issue works with AbstractSecurityEvent); plus it's one extra class to metadata space that can be avoided

WDYT?

[sberyozkin]

Duplicating SecurityIdentity as one of the event properties is not a big problem.

What I don't understand is this: HttpAuthorizer already fires authorization failure events if the access was denied, and there it includes the identity, so, what case does this PR cover ?

[michalvavrik]

Duplicating SecurityIdentity as one of the event properties is not a big problem.

cool, that was my thinking

What I don't understand is this: HttpAuthorizer already fires authorization failure events if the access was denied, and there it includes the identity, so, what case does this PR cover ?

Unsecured JAX-RS resources that calls CDI beans with standard security annotations. Inside JAX-RS endpoint you will never be able to statically analyze what CDI beans are going to be invoked (it depends on business logic).

If you want, you can have a look into the tests for it, it's the one called testNestedRolesAllowed and testNestedPermitAll, but I can also describe it for you. What is happening is that CDI beans are secured by CDI interceptors. and the CDI interceptors are inside Security extension that is not based on the Vert.x. In #40170 (comment) user described that they have requirement to log some actions for security audit and inside the issue description #40170 he says When a method (as opposed to a REST resource) is annotated with an RBAC annotation ..., please have a look.

[sberyozkin]

Is it because the security events are not always covered at the HttpAuthorizer (quarkus-vertx-http-level) level only and sometimes at the quarkus-security level ?

[michalvavrik]

Is it because the security events are not always covered at the HttpAuthorizer (quarkus-vertx-http-level) level only and sometimes at the quarkus-security level ?

Yes, same as authorization. I can be even more specific:

@ApplicationScoped
public class SecuredBean {

  @RolesAllowed("admin")
  public String sayAdmin() {
     return "admin";
  }

}

In this example, the SecuredBean#sayAdmin is secured by CDI interceptor, not the HttpAuthorizer. It is not possible to use the HttpAuthorizer because the authorizer is for HTTP request, but this is method chain invocation. In other worlds, you do not know that SecuredBean#sayAdmin is going to be called.

[sberyozkin]

Would it make sense to update AbstractSecurityEvent.getSecurityIdentity to check the properties if the identity property is null ? Otherwise we'd need to recommend users in the docs that you have to check both getIdentity and properties which may be a bit confusing for users

[michalvavrik]

Would it make sense to update AbstractSecurityEvent.getSecurityIdentity to check the properties if the identity property is null ? Otherwise we'd need to recommend users in the docs that you have to check both getIdentity and properties which may be a bit confusing for users

I didn't think of this solution. What I did here

quarkus/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/interceptor/SecurityConstrainer.java

Line 132 in aeba22d

identity = (SecurityIdentity) additionalEventProps.get(SecurityIdentity.class.getName());

is that I copied the identity from properties to the io.quarkus.security.spi.runtime.AbstractSecurityEvent#securityIdentity, this way, AbstractSecurityEvent.getSecurityIdentity works (I have it tested).

What you suggest would work as well. I am open to changing it if you prefer that.

[sberyozkin]

What you did looks better as it can only happen on this security constrainer path

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit aeba22d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

• Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

• Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

[mrickly]

Thank you @michalvavrik , @sberyozkin ! 👍

[gsmet]

This looks more like a new feature than a bugfix. It's more involved than I feel comfortable to backport.