From 81a872d84df643c45c49e1e4fa0c1960ed9b9a3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Vav=C5=99=C3=ADk?= Date: Mon, 15 Jan 2024 15:57:12 +0100 Subject: [PATCH] Fix slow Arc req ctx on duplicated Vertx ctx & enable sec events --- .../main/asciidoc/security-customization.adoc | 2 - .../grpc/deployment/GrpcServerProcessor.java | 36 ------------- .../quarkus/grpc/auth/GrpcAuthTestBase.java | 53 ++++++++++++++++++- .../grpc/auth/SecurityEventObserver.java | 2 + .../SecurityEventsValidationFailureTest.java | 31 ----------- .../grpc/auth/GrpcSecurityInterceptor.java | 40 ++++++++++++-- .../grpc/auth/GrpcSecurityRecorder.java | 18 ------- .../io/quarkus/arc/impl/RequestContext.java | 3 +- 8 files changed, 93 insertions(+), 92 deletions(-) delete mode 100644 extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventsValidationFailureTest.java diff --git a/docs/src/main/asciidoc/security-customization.adoc b/docs/src/main/asciidoc/security-customization.adoc index f6f60fbdf4292..658fa2007436e 100644 --- a/docs/src/main/asciidoc/security-customization.adoc +++ b/docs/src/main/asciidoc/security-customization.adoc @@ -691,8 +691,6 @@ Depending on the application, that can be a lot of the `AuthenticationSuccessEve For that reason, asynchronous processing can have positive effect on performance. <2> Common code for all supported security event types is possible because they all implement the `io.quarkus.security.spi.runtime.SecurityEvent` interface. -IMPORTANT: The gRPC extension currently does not support security events. - == References * xref:security-overview.adoc[Quarkus Security overview] diff --git a/extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java b/extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java index 330d1fd0479f3..677c243adb9e0 100644 --- a/extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java +++ b/extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java @@ -45,7 +45,6 @@ import io.quarkus.arc.deployment.BeanContainerBuildItem; import io.quarkus.arc.deployment.CustomScopeAnnotationsBuildItem; import io.quarkus.arc.deployment.RecorderBeanInitializedBuildItem; -import io.quarkus.arc.deployment.SynthesisFinishedBuildItem; import io.quarkus.arc.deployment.SyntheticBeanBuildItem; import io.quarkus.arc.deployment.SyntheticBeansRuntimeInitBuildItem; import io.quarkus.arc.deployment.UnremovableBeanBuildItem; @@ -53,7 +52,6 @@ import io.quarkus.arc.processor.AnnotationsTransformer; import io.quarkus.arc.processor.BeanInfo; import io.quarkus.arc.processor.BuiltinScope; -import io.quarkus.arc.processor.ObserverInfo; import io.quarkus.deployment.ApplicationArchive; import io.quarkus.deployment.Capabilities; import io.quarkus.deployment.Capability; @@ -69,7 +67,6 @@ import io.quarkus.deployment.builditem.ExtensionSslNativeSupportBuildItem; import io.quarkus.deployment.builditem.FeatureBuildItem; import io.quarkus.deployment.builditem.LaunchModeBuildItem; -import io.quarkus.deployment.builditem.RuntimeConfigSetupCompleteBuildItem; import io.quarkus.deployment.builditem.ServiceStartBuildItem; import io.quarkus.deployment.builditem.ShutdownContextBuildItem; import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBuildItem; @@ -93,7 +90,6 @@ import io.quarkus.kubernetes.spi.KubernetesPortBuildItem; import io.quarkus.netty.deployment.MinNettyAllocatorMaxOrderBuildItem; import io.quarkus.runtime.LaunchMode; -import io.quarkus.security.spi.runtime.SecurityEvent; import io.quarkus.smallrye.health.deployment.spi.HealthBuildItem; import io.quarkus.vertx.deployment.VertxBuildItem; import io.quarkus.vertx.http.deployment.VertxWebRouterBuildItem; @@ -797,36 +793,4 @@ void initGrpcSecurityInterceptor(List bindables, Capab } } - @Record(RUNTIME_INIT) - @Consume(RuntimeConfigSetupCompleteBuildItem.class) - @BuildStep - void validateSecurityEventsNotObserved(SynthesisFinishedBuildItem synthesisFinished, - Capabilities capabilities, - GrpcSecurityRecorder recorder, - BeanArchiveIndexBuildItem indexBuildItem) { - if (!capabilities.isPresent(Capability.SECURITY)) { - return; - } - - // collect all SecurityEvent classes - Set knownSecurityEventClasses = new HashSet<>(); - knownSecurityEventClasses.add(DotName.createSimple(SecurityEvent.class)); - indexBuildItem - .getIndex() - .getAllKnownImplementors(SecurityEvent.class) - .stream() - .map(ClassInfo::name) - .forEach(knownSecurityEventClasses::add); - - // find at least one CDI observer and validate security events are disabled - knownClasses: for (DotName knownSecurityEventClass : knownSecurityEventClasses) { - for (ObserverInfo observer : synthesisFinished.getObservers()) { - if (observer.getObservedType().name().equals(knownSecurityEventClass)) { - recorder.validateSecurityEventsDisabled(knownSecurityEventClass.toString()); - break knownClasses; - } - } - } - } - } diff --git a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthTestBase.java b/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthTestBase.java index fdd9fb7c254d2..706521d4d7ab5 100644 --- a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthTestBase.java +++ b/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthTestBase.java @@ -2,8 +2,14 @@ import static com.example.security.Security.ThreadInfo.newBuilder; import static io.quarkus.grpc.auth.BlockingHttpSecurityPolicy.BLOCK_REQUEST; +import static io.quarkus.security.spi.runtime.AuthorizationSuccessEvent.AUTHORIZATION_CONTEXT; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; @@ -12,11 +18,13 @@ import java.util.concurrent.atomic.AtomicReference; import jakarta.annotation.security.RolesAllowed; +import jakarta.inject.Inject; import org.jboss.shrinkwrap.api.ShrinkWrap; import org.jboss.shrinkwrap.api.asset.StringAsset; import org.jboss.shrinkwrap.api.spec.JavaArchive; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import com.example.security.SecuredService; @@ -26,6 +34,11 @@ import io.quarkus.grpc.GrpcClient; import io.quarkus.grpc.GrpcClientUtils; import io.quarkus.grpc.GrpcService; +import io.quarkus.security.UnauthorizedException; +import io.quarkus.security.runtime.interceptor.check.RolesAllowedCheck; +import io.quarkus.security.spi.runtime.AuthenticationSuccessEvent; +import io.quarkus.security.spi.runtime.AuthorizationFailureEvent; +import io.quarkus.security.spi.runtime.AuthorizationSuccessEvent; import io.quarkus.test.QuarkusUnitTest; import io.smallrye.common.annotation.Blocking; import io.smallrye.mutiny.Multi; @@ -54,7 +67,7 @@ protected static QuarkusUnitTest createQuarkusUnitTest(String extraProperty, boo props += extraProperty; } var jar = ShrinkWrap.create(JavaArchive.class) - .addClasses(Service.class, BlockingHttpSecurityPolicy.class) + .addClasses(Service.class, BlockingHttpSecurityPolicy.class, SecurityEventObserver.class) .addPackage(SecuredService.class.getPackage()) .add(new StringAsset(props), "application.properties"); return useGrpcAuthMechanism ? jar.addClass(BasicGrpcSecurityMechanism.class) : jar; @@ -67,6 +80,14 @@ protected static QuarkusUnitTest createQuarkusUnitTest(String extraProperty, boo @GrpcClient SecuredService securityClient; + @Inject + SecurityEventObserver securityEventObserver; + + @BeforeEach + void clearEvents() { + securityEventObserver.getStorage().clear(); + } + @Test void shouldSecureUniEndpoint() { Metadata headers = new Metadata(); @@ -83,6 +104,7 @@ void shouldSecureUniEndpoint() { await().atMost(10, TimeUnit.SECONDS) .until(() -> resultCount.get() == 1); + assertSecurityEventsFired("john"); } @Test @@ -101,6 +123,7 @@ void shouldSecureBlockingUniEndpoint() { await().atMost(10, TimeUnit.SECONDS) .until(() -> resultCount.get() == 1); + assertSecurityEventsFired("john"); } @Test @@ -117,6 +140,7 @@ void shouldSecureMultiEndpoint() { .until(() -> results.size() == 5); assertThat(results.stream().filter(e -> !e)).isEmpty(); + assertSecurityEventsFired("paul"); } @Test @@ -133,6 +157,7 @@ void shouldSecureBlockingMultiEndpoint() { .until(() -> results.size() == 5); assertThat(results.stream().filter(e -> e)).isEmpty(); + assertSecurityEventsFired("paul"); } @Test @@ -167,6 +192,16 @@ void shouldFailWithInvalidInsufficientRole() { await().atMost(10, TimeUnit.SECONDS) .until(() -> error.get() != null); + + // we don't check exact count as HTTP Security policies are not supported when gRPC is running on separate server + assertFalse(securityEventObserver.getStorage().isEmpty()); + // fails RolesAllowed check as the anonymous identity has no roles + AuthorizationFailureEvent event = (AuthorizationFailureEvent) securityEventObserver + .getStorage().get(securityEventObserver.getStorage().size() - 1); + assertNotNull(event.getSecurityIdentity()); + assertTrue(event.getSecurityIdentity().isAnonymous()); + assertInstanceOf(UnauthorizedException.class, event.getAuthorizationFailure()); + assertEquals(RolesAllowedCheck.class.getName(), event.getAuthorizationContext()); } @Test @@ -186,6 +221,7 @@ void shouldSecureUniEndpointWithBlockingHttpSecurityPolicy() { await().atMost(10, TimeUnit.SECONDS) .until(() -> resultCount.get() == 1); + assertSecurityEventsFired("john"); } @Test @@ -205,6 +241,7 @@ void shouldSecureBlockingUniEndpointWithBlockingHttpSecurityPolicy() { await().atMost(10, TimeUnit.SECONDS) .until(() -> resultCount.get() == 1); + assertSecurityEventsFired("john"); } @Test @@ -224,6 +261,7 @@ void shouldSecureMultiEndpointWithBlockingHttpSecurityPolicy() { .until(() -> results.size() == 5); assertThat(results.stream().filter(e -> !e)).isEmpty(); + assertSecurityEventsFired("paul"); } @Test @@ -241,6 +279,19 @@ void shouldSecureBlockingMultiEndpointWithBlockingHttpSecurityPolicy() { .until(() -> results.size() == 5); assertThat(results.stream().filter(e -> e)).isEmpty(); + assertSecurityEventsFired("paul"); + } + + private void assertSecurityEventsFired(String username) { + // expect at least authentication success and RolesAllowed security check success + // we don't check exact count as HTTP Security policies are not supported when gRPC is running on separate server + assertTrue(securityEventObserver.getStorage().size() >= 2); + assertTrue(securityEventObserver.getStorage().stream().anyMatch(e -> e instanceof AuthenticationSuccessEvent)); + AuthorizationSuccessEvent event = (AuthorizationSuccessEvent) securityEventObserver.getStorage() + .get(securityEventObserver.getStorage().size() - 1); + assertNotNull(event.getSecurityIdentity()); + assertEquals(username, event.getSecurityIdentity().getPrincipal().getName()); + assertEquals(RolesAllowedCheck.class.getName(), event.getEventProperties().get(AUTHORIZATION_CONTEXT)); } private static void addBlockingHeaders(Metadata headers) { diff --git a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventObserver.java b/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventObserver.java index 8f067128b4efc..26e9ac0782617 100644 --- a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventObserver.java +++ b/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventObserver.java @@ -3,10 +3,12 @@ import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.event.Observes; import io.quarkus.security.spi.runtime.SecurityEvent; +@ApplicationScoped public class SecurityEventObserver { private final List storage = new CopyOnWriteArrayList<>(); diff --git a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventsValidationFailureTest.java b/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventsValidationFailureTest.java deleted file mode 100644 index 599d790cb7343..0000000000000 --- a/extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/SecurityEventsValidationFailureTest.java +++ /dev/null @@ -1,31 +0,0 @@ -package io.quarkus.grpc.auth; - -import static org.junit.jupiter.api.Assertions.assertTrue; - -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; - -import com.example.security.SecuredService; - -import io.quarkus.runtime.configuration.ConfigurationException; -import io.quarkus.test.QuarkusUnitTest; - -public class SecurityEventsValidationFailureTest { - - @RegisterExtension - static final QuarkusUnitTest config = new QuarkusUnitTest() - .withApplicationRoot(jar -> jar - .addClass(SecurityEventObserver.class) - .addPackage(SecuredService.class.getPackage())) - .assertException(throwable -> { - assertTrue(throwable instanceof ConfigurationException); - assertTrue(throwable.getMessage().contains("quarkus.security.events.enabled")); - }); - - @Test - void test() { - // must be here to run test - Assertions.fail(); - } -} diff --git a/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityInterceptor.java b/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityInterceptor.java index 3abd823123758..7399a5088d942 100644 --- a/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityInterceptor.java +++ b/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityInterceptor.java @@ -1,5 +1,7 @@ package io.quarkus.grpc.auth; +import static io.quarkus.security.spi.runtime.SecurityEventHelper.AUTHENTICATION_FAILURE; +import static io.quarkus.security.spi.runtime.SecurityEventHelper.AUTHENTICATION_SUCCESS; import static io.quarkus.vertx.core.runtime.context.VertxContextSafetyToggle.isExplicitlyMarkedAsUnsafe; import static io.quarkus.vertx.http.runtime.security.QuarkusHttpUser.DEFERRED_IDENTITY_KEY; import static io.smallrye.common.vertx.VertxContext.isDuplicatedContext; @@ -10,8 +12,11 @@ import java.util.List; import java.util.Map; import java.util.concurrent.Executor; +import java.util.function.Consumer; +import jakarta.enterprise.event.Event; import jakarta.enterprise.inject.Instance; +import jakarta.enterprise.inject.spi.BeanManager; import jakarta.enterprise.inject.spi.Prioritized; import jakarta.inject.Inject; import jakarta.inject.Singleton; @@ -29,6 +34,9 @@ import io.quarkus.security.identity.IdentityProviderManager; import io.quarkus.security.identity.SecurityIdentity; import io.quarkus.security.identity.request.AuthenticationRequest; +import io.quarkus.security.spi.runtime.AuthenticationFailureEvent; +import io.quarkus.security.spi.runtime.AuthenticationSuccessEvent; +import io.quarkus.security.spi.runtime.SecurityEventHelper; import io.quarkus.vertx.http.runtime.security.QuarkusHttpUser; import io.smallrye.mutiny.Uni; import io.vertx.core.Context; @@ -55,6 +63,7 @@ public final class GrpcSecurityInterceptor implements ServerInterceptor, Priorit private final Map> serviceToBlockingMethods = new HashMap<>(); private boolean hasBlockingMethods = false; private final boolean notUsingSeparateGrpcServer; + private final SecurityEventHelper securityEventHelper; @Inject public GrpcSecurityInterceptor( @@ -62,7 +71,12 @@ public GrpcSecurityInterceptor( IdentityProviderManager identityProviderManager, Instance securityMechanisms, Instance exceptionHandlers, - @ConfigProperty(name = "quarkus.grpc.server.use-separate-server") boolean usingSeparateGrpcServer) { + @ConfigProperty(name = "quarkus.grpc.server.use-separate-server") boolean usingSeparateGrpcServer, + @ConfigProperty(name = "quarkus.security.events.enabled") boolean securityEventsEnabled, + BeanManager beanManager, Event authFailureEvent, + Event authSuccessEvent) { + this.securityEventHelper = new SecurityEventHelper<>(authSuccessEvent, authFailureEvent, AUTHENTICATION_SUCCESS, + AUTHENTICATION_FAILURE, beanManager, securityEventsEnabled); this.identityAssociation = identityAssociation; this.identityProviderManager = identityProviderManager; this.notUsingSeparateGrpcServer = !usingSeparateGrpcServer; @@ -131,6 +145,23 @@ public void handle(Void event) { } } }); + if (securityEventHelper.fireEventOnSuccess()) { + auth = auth.invoke(new Consumer() { + @Override + public void accept(SecurityIdentity securityIdentity) { + securityEventHelper + .fireSuccessEvent(new AuthenticationSuccessEvent(securityIdentity, null)); + } + }); + } + if (securityEventHelper.fireEventOnFailure()) { + auth = auth.onFailure().invoke(new Consumer() { + @Override + public void accept(Throwable throwable) { + securityEventHelper.fireFailureEvent(new AuthenticationFailureEvent(throwable, null)); + } + }); + } identityAssociation.setIdentity(auth); error = null; identityAssociationNotSet = false; @@ -143,8 +174,11 @@ public void handle(Void event) { } } if (error != null) { // if parsing for all security mechanisms failed, let's propagate the last exception - identityAssociation.setIdentity(Uni.createFrom() - .failure(new AuthenticationFailedException("Failed to parse authentication data", error))); + var authFailedEx = new AuthenticationFailedException("Failed to parse authentication data", error); + if (securityEventHelper.fireEventOnFailure()) { + securityEventHelper.fireFailureEvent(new AuthenticationFailureEvent(authFailedEx, null)); + } + identityAssociation.setIdentity(Uni.createFrom().failure(authFailedEx)); } } if (identityAssociationNotSet && notUsingSeparateGrpcServer) { diff --git a/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityRecorder.java b/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityRecorder.java index 5a7e3be440a39..2a3d54f33ce72 100644 --- a/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityRecorder.java +++ b/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityRecorder.java @@ -6,16 +6,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; - -import org.eclipse.microprofile.config.ConfigProvider; import io.grpc.BindableService; import io.grpc.ServerMethodDefinition; import io.quarkus.arc.runtime.BeanContainer; import io.quarkus.grpc.runtime.GrpcContainer; import io.quarkus.runtime.annotations.Recorder; -import io.quarkus.runtime.configuration.ConfigurationException; @Recorder public class GrpcSecurityRecorder { @@ -46,18 +42,4 @@ public void initGrpcSecurityInterceptor(Map> serviceClassTo container.beanInstance(GrpcSecurityInterceptor.class).init(svcToMethods); } - - public void validateSecurityEventsDisabled(String observedSecurityEvent) { - boolean securityEventsEnabled = ConfigProvider - .getConfig() - .getOptionalValue("quarkus.security.events.enabled", boolean.class) - .orElse(Boolean.TRUE); - if (securityEventsEnabled) { - throw new ConfigurationException(""" - Found observer method for event type '%s', but the gRPC extension does not support security - events. Either disable security events with the 'quarkus.security.events.enabled' - configuration property, or remove security event CDI observers.""".formatted(observedSecurityEvent), - Set.of("quarkus.security.events.enabled")); - } - } } diff --git a/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java b/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java index 0e81d5b5865b4..6ba7414ca9324 100644 --- a/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java +++ b/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java @@ -172,7 +172,8 @@ public ContextState getState() { } public ContextState getStateIfActive() { - return currentContext.get(); + ContextState state = currentContext.get(); + return state != null && state.isValid() ? state : null; } @Override