quarkus
quarkus copied to clipboard
quarkusio
RESTEasy Reactive - prevent repeating of standard security checks
fix: #26536
EagerSecurityHandler performs standard security checks for endpoints annotated with an RBAC annotation and the same checks are later done by SecurityHandler. We still need interceptors to run for CDI beans and gRPC. This PR prevents repeating of security checks by propagating the information that check is done to SecurityHandler via invocation context.
:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.
Failing Jobs - Building e74b7d851bee0117e3158fb8ecc094ccaa74ef8f
| Status | Name | Step | Failures | Logs | Raw logs |
|---|---|---|---|---|---|
| ✖ | JVM Tests - JDK 11 | Build |
Failures | Logs | Raw logs |
| ✖ | JVM Tests - JDK 11 Windows | Build |
Failures | Logs | Raw logs |
| ✖ | JVM Tests - JDK 17 | Build |
Failures | Logs | Raw logs |
| ✖ | JVM Tests - JDK 18 | Build |
Failures | Logs | Raw logs |
Full information is available in the Build summary check run.
Failures
:gear: JVM Tests - JDK 11 #
- Failing: extensions/grpc/deployment integration-tests/oidc
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment extensions/opentelemetry/opentelemetry/deployment and 19 more
:package: extensions/grpc/deployment
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureMultiEndpoint line 86 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.grpc.auth.GrpcAuthTest that uses java.util.List was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureUniEndpoint line 72 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.grpc.auth.GrpcAuthTest that uses java.util.concurrent.atomic.AtomicInteger was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
:package: integration-tests/oidc
✖ io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 48 - More details - Source on GitHub
org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
:gear: JVM Tests - JDK 11 Windows #
- Failing: extensions/grpc/deployment extensions/mongodb-client/deployment
! Skipped: extensions/liquibase-mongodb/deployment extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment and 30 more
:package: extensions/grpc/deployment
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureMultiEndpoint line 86 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.grpc.auth.GrpcAuthTest that uses java.util.List was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureUniEndpoint line 72 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.grpc.auth.GrpcAuthTest that uses java.util.concurrent.atomic.AtomicInteger was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
:package: extensions/mongodb-client/deployment
✖ io.quarkus.mongodb.NamedReactiveMongoClientConfigTest. - More details - Source on GitHub
java.lang.RuntimeException:
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration
:gear: JVM Tests - JDK 17 #
- Failing: extensions/grpc/deployment integration-tests/oidc
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment extensions/opentelemetry/opentelemetry/deployment and 19 more
:package: extensions/grpc/deployment
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureMultiEndpoint line 86 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.grpc.auth.GrpcAuthTest was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureUniEndpoint line 72 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.grpc.auth.GrpcAuthTest was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
:package: integration-tests/oidc
✖ io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 48 - More details - Source on GitHub
org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
:gear: JVM Tests - JDK 18 #
- Failing: extensions/grpc/deployment extensions/smallrye-reactive-messaging-kafka/deployment integration-tests/oidc
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment extensions/opentelemetry/opentelemetry/deployment and 24 more
:package: extensions/grpc/deployment
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureMultiEndpoint line 86 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.grpc.auth.GrpcAuthTest was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
✖ io.quarkus.grpc.auth.GrpcAuthTest.shouldSecureUniEndpoint line 72 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.grpc.auth.GrpcAuthTest was not fulfilled within 5 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
:package: extensions/smallrye-reactive-messaging-kafka/deployment
✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario2 line 83 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 4 State{lastRun=3, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario2(KafkaDevServicesContinuousTestingTestCase.java:83)
:package: integration-tests/oidc
✖ io.quarkus.it.keycloak.WebsocketOidcTestCase.websocketTest line 48 - More details - Source on GitHub
org.opentest4j.AssertionFailedError: expected: <hello [email protected]> but was: <null>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
![quarkus-bot[bot] avatar](https://avatars.githubusercontent.com/in/90234?v=4 "Published by a pub.dev verified publisher"){kind=small}
@michalvavrik Thanks, it looks technically totally fine, thanks for working out how to handle it.
But it feels like we are doing a workaround, which in the RestEasy Reactive security case will involve running an extra interceptor with a few extra checks involved.
Would it be possible instead to remove RolesAllowedInterceptor/etc at the build level in the case where they are known to duplicate the checks ?
@sberyozkin yes, it would be possible and I actually already had it in progress... I stopped as the binding is between annotation (and its values) and the interceptor, not between method and the interceptor. Please remember we still need other annotated methods not checked by EagerSecurityHandler to be intercepted.
I had in progress InterceptedMethodBindingFilter that would be registered in the build time and break the binding between the method and interceptor during the build time somewhere around io.quarkus.arc.processor.BeanInfo#initInterceptedMethods. I just didn't think you would be happy if I messed with Arc Processor :-).
Is that how you meant it?
My point is that the check can only be removed on "method invocation" scope, not request scope as you need nested CDI beans to be able to perform its own (different checks) within same request.
Hey @michalvavrik the last thing that should be of concern is if some code in PR can make me less excited :-), you keep creating good quality PRs I may not be even getting from the first round of review :-).
But IMHO, if tweaking around io.quarkus.arc.processor.BeanInfo#initInterceptedMethods can disable those CDI interceptors then it would be preferred; have a look please when you get a chance, I guess we should not rush with merging it asap, so please take your time
thanks
:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.
Failing Jobs - Building 870c78ed18a2763454258160c1e7e093d96439bc
| Status | Name | Step | Failures | Logs | Raw logs |
|---|---|---|---|---|---|
| :heavy_check_mark: | Gradle Tests - JDK 11 | ||||
| ✖ | Gradle Tests - JDK 11 Windows | Build |
Failures | Logs | Raw logs |
| :heavy_check_mark: | JVM Tests - JDK 11 | ||||
| ✖ | JVM Tests - JDK 11 Windows | Build |
Failures | Logs | Raw logs |
| :heavy_check_mark: | JVM Tests - JDK 17 | ||||
| :heavy_check_mark: | JVM Tests - JDK 18 |
Full information is available in the Build summary check run.
Failures
:gear: Gradle Tests - JDK 11 Windows #
- Failing: integration-tests/gradle
:package: integration-tests/gradle
✖ io.quarkus.gradle.devmode.CompositeBuildWithDependenciesDevModeTest.main line 24 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
:gear: JVM Tests - JDK 11 Windows #
- Failing: extensions/mongodb-client/deployment
! Skipped: extensions/liquibase-mongodb/deployment extensions/panache/mongodb-panache-common/deployment extensions/panache/mongodb-panache-kotlin/deployment and 8 more
:package: extensions/mongodb-client/deployment
✖ io.quarkus.mongodb.NamedMongoClientConfigTest. - More details - Source on GitHub
java.lang.RuntimeException:
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration
✖ io.quarkus.mongodb.NamedReactiveMongoClientConfigTest. - More details - Source on GitHub
java.lang.RuntimeException:
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration
Sure, I'll find a build time solution as discussed. I changed PR to draft then.
:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.
Failing Jobs - Building 68d5fb0ec90e570597b29079e42349d93bf29887
| Status | Name | Step | Failures | Logs | Raw logs |
|---|---|---|---|---|---|
| :heavy_check_mark: | Maven Tests - JDK 11 | ||||
| :hourglass: | Maven Tests - JDK 11 Windows | Build |
:warning: Check → | Logs | Raw logs |
@michalvavrik Great, sorry for a delay, can you please resolve the conflict ?
@mkouba Hi Martin, can you please have a look at the Arc related code in this PR when you are get a chance ?
@geoand Have a look please when you get a chance :-), I agree it is a much more complex solution compared to the original approach of passing the flags, but the complexity is isolated to the build time and is really about neatly disabling the duplicating interceptors, so in the end it is only the eager security checks which are done
I'm out until further notice.
Furthermore my backlog has grown pretty large, so I can't promise when I'll get to this.
This definitely needs a review from @mkouba for the Arc parts as I am not 100% what is introduces is something we want
@michalvavrik @sberyozkin @geoand I haven't had time to look into the details yet but is the InterceptedMethodBindingFilter about ignoring an interceptor binding annotation instance? If so would an annotation transformer be a solution?
but we should keep the annotation in place as the developers will count on it. what if they will bind their own interceptors to it. It's dodgy. Or did I misunderstood you @mkouba ?
but we should keep the annotation in place as the developers will count on it. what if they will bind their own interceptors to it. It's dodgy. Or did I misunderstood you @mkouba ?
I'm sorry but I don't understand. If you ignore the annotation like this then no CDI interceptors are associated. Could you be more specific? An example would be helpful. Also I'm sorry for my ignorance because I may be missing some context mentioned somewhere in this PR.
In short -
annotation transformer solution: one annotation instance - possibly many interceptors -> you remove it -> you loose all the bindings
this pr: one annotation instance - possibly many interceptor -> you apply filter -> you loose the binding between an interceptor and the method (so all other bindings are still intercepted). I only implemented what I really need so non-static invocation interceptions of methods)
there is the test from which the behavior should be clear - MethodInterceptorBindingFilterTest
Ok, I get the idea but I really don't get why it's needed. I mean why do we need to keep the annotations anyway? Also I don't think it's very likely that some other interceptors use the same bindings (although it's theoretially possible ;-).
I think this solution was isolated to repeated checks while other assumes we know what developers do (it could be breaking change theoretically, right?).
I accept that you agreed it's not necessary to keep the annotation and will rewrite the PR. Thanks for the review.
@knutwannheden Can you please check the last few comments from @michalvavrik and @mkouba and clarify if needed ? I think you typed it all in #26536 but it might help to get some context here as well
:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.
Failing Jobs - Building 809091d5ec5596bf39e0f5da9d0dd0645baf759b
| Status | Name | Step | Failures | Logs | Raw logs |
|---|---|---|---|---|---|
| :heavy_check_mark: | JVM Tests - JDK 11 | ||||
| :heavy_check_mark: | JVM Tests - JDK 17 | ||||
| ✖ | JVM Tests - JDK 18 | Build |
Failures | Logs | Raw logs |
Full information is available in the Build summary check run.
Failures
:gear: JVM Tests - JDK 18 #
- Failing: extensions/smallrye-reactive-messaging-kafka/deployment
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/reactive-messaging-hibernate-reactive and 2 more
:package: extensions/smallrye-reactive-messaging-kafka/deployment
✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 line 54 - More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 4 State{lastRun=3, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3(KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.java:54)
@sberyozkin Yes, we do in fact register a custom interceptor against the authorization annotations. The purpose is to perform custom security logging both for successful and failed security checks. If another mechanism were available, we would however be happy to use that instead, as the failed authorization logging currently only works with RestEasy Classic anyway. (There is also a few other drawbacks to the CDI interceptor approach.) But it would be unfortunate if this refactoring means that there is no way to implement the security logging anymore.
@mkouba @geoand could you please have a look at @knutwannheden comment and consider the arc method interceptor filter again? I need confirmation or instructions what's next as I have already provided 3 possible solutions and it doesn't make sense to make any other adjustments till there is an agreement on solution. Thank you