Skip to content

Commit a6fc3da

Browse files
michalvavrikmichalvavrik
committed
fix(grpc,security): Improve security error handling
1 parent f3068af commit a6fc3da

File tree

11 files changed

+220
-14
lines changed

11 files changed

+220
-14
lines changed

‎extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -719,6 +719,9 @@ ServiceStartBuildItem initializeServer(GrpcServerRecorder recorder,
719719
.filter(filter -> filter.getPriority() == FilterBuildItem.AUTHENTICATION
720720
|| filter.getPriority() == FilterBuildItem.AUTHORIZATION)
721721
.collect(Collectors.toMap(f -> f.getPriority() * -1, FilterBuildItem::getHandler));
722+
// for the moment being, the main router doesn't have QuarkusErrorHandler, but we need to make
723+
// sure that exceptions raised during proactive authentication or HTTP authorization are handled
724+
recorder.addMainRouterErrorHandlerIfSameServer(routerRuntimeValue, config);
722725
}
723726
} else {
724727
routerRuntimeValue = routerBuildItem.getHttpRouter();

‎extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/BasicGrpcSecurityMechanism.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ public AuthenticationRequest createAuthenticationRequest(Metadata metadata) {
3131
String userName = plainChallenge.substring(0, colonPos);
3232
char[] password = plainChallenge.substring(colonPos + 1).toCharArray();
3333
return new UsernamePasswordAuthenticationRequest(userName, new PasswordCredential(password));
34+
} else if (!plainChallenge.isEmpty()) {
35+
// this is illegal argument exception because ATM when Basic HTTP Auth mechanism doesn't throw auth exception
36+
// for the same credentials, so let's simulate the same situation in this test
37+
throw new IllegalArgumentException("Empty authentication request");
3438
} else {
3539
return null;
3640
}

‎extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthTestBase.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import jakarta.annotation.security.RolesAllowed;
2121
import jakarta.inject.Inject;
2222

23+
import org.awaitility.Awaitility;
2324
import org.jboss.shrinkwrap.api.ShrinkWrap;
2425
import org.jboss.shrinkwrap.api.asset.StringAsset;
2526
import org.jboss.shrinkwrap.api.spec.JavaArchive;
@@ -31,11 +32,15 @@
3132
import com.example.security.Security;
3233

3334
import io.grpc.Metadata;
35+
import io.grpc.Status;
36+
import io.grpc.StatusRuntimeException;
3437
import io.quarkus.grpc.GrpcClient;
3538
import io.quarkus.grpc.GrpcClientUtils;
3639
import io.quarkus.grpc.GrpcService;
40+
import io.quarkus.security.AuthenticationFailedException;
3741
import io.quarkus.security.UnauthorizedException;
3842
import io.quarkus.security.runtime.interceptor.check.RolesAllowedCheck;
43+
import io.quarkus.security.spi.runtime.AuthenticationFailureEvent;
3944
import io.quarkus.security.spi.runtime.AuthenticationSuccessEvent;
4045
import io.quarkus.security.spi.runtime.AuthorizationFailureEvent;
4146
import io.quarkus.security.spi.runtime.AuthorizationSuccessEvent;
@@ -107,6 +112,37 @@ void shouldSecureUniEndpoint() {
107112
assertSecurityEventsFired("john");
108113
}
109114

115+
@Test
116+
void shouldHandleAuthenticationFailure() {
117+
Metadata headers = new Metadata();
118+
headers.put(AUTHORIZATION, "Basic wrongcredentials");
119+
SecuredService client = GrpcClientUtils.attachHeaders(securityClient, headers);
120+
AtomicInteger authenticationFailedCount = new AtomicInteger();
121+
client.unaryCall(Security.Container.newBuilder().setText("woo-hoo").build())
122+
.subscribe().with(ti -> {
123+
}, error -> {
124+
if (error instanceof StatusRuntimeException statusException) {
125+
if (Status.UNAUTHENTICATED.getCode().equals(statusException.getStatus().getCode())) {
126+
authenticationFailedCount.incrementAndGet();
127+
} else {
128+
Assertions.fail("Expected unauthenticated response status, got " + statusException.getStatus());
129+
}
130+
} else {
131+
Assertions.fail("Expected 'StatusRuntimeException' exception, got " + error);
132+
}
133+
});
134+
135+
await().atMost(10, TimeUnit.SECONDS).until(() -> authenticationFailedCount.get() == 1);
136+
Awaitility.await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> {
137+
AuthenticationFailureEvent authFailureEvent = securityEventObserver.getStorage().stream()
138+
.filter(e -> e instanceof AuthenticationFailureEvent)
139+
.map(e -> (AuthenticationFailureEvent) e)
140+
.findFirst().orElse(null);
141+
Assertions.assertNotNull(authFailureEvent);
142+
Assertions.assertInstanceOf(AuthenticationFailedException.class, authFailureEvent.getAuthenticationFailure());
143+
});
144+
}
145+
110146
@Test
111147
void shouldSecureBlockingUniEndpoint() {
112148
Metadata headers = new Metadata();
@@ -192,6 +228,8 @@ void shouldFailWithInvalidInsufficientRole() {
192228

193229
await().atMost(10, TimeUnit.SECONDS)
194230
.until(() -> error.get() != null);
231+
StatusRuntimeException statusRuntimeException = (StatusRuntimeException) error.get();
232+
Assertions.assertEquals(Status.PERMISSION_DENIED, statusRuntimeException.getStatus());
195233

196234
// we don't check exact count as HTTP Security policies are not supported when gRPC is running on separate server
197235
assertFalse(securityEventObserver.getStorage().isEmpty());

‎extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcAuthCustomRootPathTest.java‎ renamed to ‎extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcEagerAuthCustomRootPathTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import io.quarkus.test.QuarkusUnitTest;
66

7-
public class GrpcAuthCustomRootPathTest extends GrpcAuthTestBase {
7+
public class GrpcEagerAuthCustomRootPathTest extends GrpcAuthTestBase {
88

99
@RegisterExtension
1010
static final QuarkusUnitTest config = createQuarkusUnitTest("""

‎extensions/grpc/deployment/src/test/java/io/quarkus/grpc/auth/GrpcLazyAuthCustomRootPathTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package io.quarkus.grpc.auth;
2+
3+
import org.junit.jupiter.api.extension.RegisterExtension;
4+
5+
import io.quarkus.test.QuarkusUnitTest;
6+
7+
public class GrpcLazyAuthCustomRootPathTest extends GrpcAuthTestBase {
8+
9+
@RegisterExtension
10+
static final QuarkusUnitTest config = createQuarkusUnitTest("""
11+
quarkus.grpc.server.use-separate-server=false
12+
quarkus.grpc.clients.securityClient.host=localhost
13+
quarkus.grpc.clients.securityClient.port=8081
14+
quarkus.http.root-path=/api
15+
quarkus.http.auth.proactive=false
16+
""", true);
17+
18+
}

‎extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/AuthExceptionHandler.java‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
package io.quarkus.grpc.auth;
22

3+
import static io.quarkus.grpc.auth.DefaultAuthExceptionHandlerProvider.transformToStatusException;
4+
35
import jakarta.enterprise.inject.spi.Prioritized;
46

57
import io.grpc.Metadata;
68
import io.grpc.ServerCall;
7-
import io.grpc.Status;
89
import io.quarkus.grpc.ExceptionHandler;
9-
import io.quarkus.security.AuthenticationFailedException;
10+
import io.quarkus.security.AuthenticationException;
1011

1112
/**
1213
* Exception mapper for authentication and authorization exceptions
@@ -16,9 +17,17 @@
1617
*/
1718
public class AuthExceptionHandler<ReqT, RespT> extends ExceptionHandler<ReqT, RespT> implements Prioritized {
1819

20+
private final boolean addStatusDescription;
21+
1922
public AuthExceptionHandler(ServerCall.Listener<ReqT> listener, ServerCall<ReqT, RespT> serverCall,
20-
Metadata metadata) {
23+
Metadata metadata, boolean addStatusDescription) {
2124
super(listener, serverCall, metadata);
25+
this.addStatusDescription = addStatusDescription;
26+
}
27+
28+
public AuthExceptionHandler(ServerCall.Listener<ReqT> listener, ServerCall<ReqT, RespT> serverCall,
29+
Metadata metadata) {
30+
this(listener, serverCall, metadata, false);
2231
}
2332

2433
/**
@@ -29,10 +38,8 @@ public AuthExceptionHandler(ServerCall.Listener<ReqT> listener, ServerCall<ReqT,
2938
* @param metadata call metadata
3039
*/
3140
protected void handleException(Throwable exception, ServerCall<ReqT, RespT> serverCall, Metadata metadata) {
32-
if (exception instanceof AuthenticationFailedException) {
33-
serverCall.close(Status.UNAUTHENTICATED.withDescription(exception.getMessage()), metadata);
34-
} else if (exception instanceof SecurityException) {
35-
serverCall.close(Status.PERMISSION_DENIED.withDescription(exception.getMessage()), metadata);
41+
if (exception instanceof AuthenticationException || exception instanceof SecurityException) {
42+
serverCall.close(transformToStatusException(addStatusDescription, exception), metadata);
3643
} else if (exception instanceof RuntimeException) {
3744
throw (RuntimeException) exception;
3845
} else {

‎extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/AuthExceptionHandlerProvider.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import io.grpc.Metadata;
66
import io.grpc.ServerCall;
77
import io.grpc.ServerCall.Listener;
8+
import io.grpc.StatusException;
89

910
/**
1011
* Provider for AuthExceptionHandler.
@@ -17,4 +18,22 @@ public interface AuthExceptionHandlerProvider extends Prioritized {
1718

1819
<ReqT, RespT> AuthExceptionHandler<ReqT, RespT> createHandler(Listener<ReqT> listener,
1920
ServerCall<ReqT, RespT> serverCall, Metadata metadata);
21+
22+
/**
23+
* @param failure security exception this provider can handle according to the {@link #handlesException(Throwable)}
24+
* @return status exception
25+
*/
26+
default StatusException transformToStatusException(Throwable failure) {
27+
// because by default we don't handle any exception
28+
// the original behavior (before introduction of this method) is kept because 'handlesException' return false
29+
throw new IllegalStateException("Cannot transform exception " + failure + " to a status exception");
30+
}
31+
32+
/**
33+
* @param failure any gRPC request failure
34+
* @return whether this provider should create response status for given failure
35+
*/
36+
default boolean handlesException(Throwable failure) {
37+
return false;
38+
}
2039
}

‎extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/DefaultAuthExceptionHandlerProvider.java‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,20 @@
44

55
import io.grpc.Metadata;
66
import io.grpc.ServerCall;
7+
import io.grpc.Status;
8+
import io.grpc.StatusException;
9+
import io.quarkus.runtime.LaunchMode;
10+
import io.quarkus.security.AuthenticationException;
711

812
@Singleton
913
public class DefaultAuthExceptionHandlerProvider implements AuthExceptionHandlerProvider {
1014

15+
private final boolean addStatusDescription;
16+
17+
public DefaultAuthExceptionHandlerProvider() {
18+
this.addStatusDescription = LaunchMode.current().isDevOrTest();
19+
}
20+
1121
@Override
1222
public int getPriority() {
1323
return DEFAULT_PRIORITY;
@@ -16,6 +26,34 @@ public int getPriority() {
1626
@Override
1727
public <ReqT, RespT> AuthExceptionHandler<ReqT, RespT> createHandler(ServerCall.Listener<ReqT> listener,
1828
ServerCall<ReqT, RespT> serverCall, Metadata metadata) {
19-
return new AuthExceptionHandler<>(listener, serverCall, metadata);
29+
return new AuthExceptionHandler<>(listener, serverCall, metadata, addStatusDescription);
30+
}
31+
32+
@Override
33+
public StatusException transformToStatusException(Throwable t) {
34+
return new StatusException(transformToStatusException(addStatusDescription, t));
35+
}
36+
37+
@Override
38+
public boolean handlesException(Throwable failure) {
39+
return failure instanceof AuthenticationException || failure instanceof SecurityException;
40+
}
41+
42+
static Status transformToStatusException(boolean addExceptionMessage, Throwable exception) {
43+
if (exception instanceof AuthenticationException) {
44+
if (addExceptionMessage) {
45+
return Status.UNAUTHENTICATED.withDescription(exception.getMessage());
46+
} else {
47+
return Status.UNAUTHENTICATED;
48+
}
49+
} else if (exception instanceof SecurityException) {
50+
if (addExceptionMessage) {
51+
return Status.PERMISSION_DENIED.withDescription(exception.getMessage());
52+
} else {
53+
return Status.PERMISSION_DENIED;
54+
}
55+
} else {
56+
throw new IllegalStateException("Cannot transform exception " + exception, exception);
57+
}
2058
}
2159
}

‎extensions/grpc/runtime/src/main/java/io/quarkus/grpc/auth/GrpcSecurityInterceptor.java‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import java.util.Map;
1414
import java.util.concurrent.Executor;
1515
import java.util.function.Consumer;
16+
import java.util.function.Predicate;
1617

1718
import jakarta.enterprise.event.Event;
1819
import jakarta.enterprise.inject.Instance;
@@ -29,10 +30,12 @@
2930
import io.grpc.ServerCallHandler;
3031
import io.grpc.ServerInterceptor;
3132
import io.quarkus.grpc.GlobalInterceptor;
33+
import io.quarkus.security.AuthenticationException;
3234
import io.quarkus.security.AuthenticationFailedException;
3335
import io.quarkus.security.identity.CurrentIdentityAssociation;
3436
import io.quarkus.security.identity.IdentityProviderManager;
3537
import io.quarkus.security.identity.SecurityIdentity;
38+
import io.quarkus.security.identity.request.AnonymousAuthenticationRequest;
3639
import io.quarkus.security.identity.request.AuthenticationRequest;
3740
import io.quarkus.security.spi.runtime.AuthenticationFailureEvent;
3841
import io.quarkus.security.spi.runtime.AuthenticationSuccessEvent;
@@ -179,6 +182,7 @@ public void accept(Throwable throwable) {
179182
securityEventHelper.fireFailureEvent(new AuthenticationFailureEvent(authFailedEx, null));
180183
}
181184
identityAssociation.setIdentity(Uni.createFrom().failure(authFailedEx));
185+
identityAssociationNotSet = false;
182186
}
183187
}
184188
if (identityAssociationNotSet && notUsingSeparateGrpcServer) {
@@ -188,10 +192,26 @@ public void accept(Throwable throwable) {
188192
if (capturedContext.getLocal(IDENTITY_KEY) != null) {
189193
identityAssociation.setIdentity(capturedContext.<SecurityIdentity> getLocal(IDENTITY_KEY));
190194
} else if (capturedContext.getLocal(DEFERRED_IDENTITY_KEY) != null) {
191-
identityAssociation.setIdentity(capturedContext.<Uni<SecurityIdentity>> getLocal(DEFERRED_IDENTITY_KEY));
195+
Uni<SecurityIdentity> identityUni = capturedContext
196+
.<Uni<SecurityIdentity>> getLocal(DEFERRED_IDENTITY_KEY)
197+
.onFailure(new Predicate<Throwable>() {
198+
@Override
199+
public boolean test(Throwable t) {
200+
return !(t instanceof AuthenticationException);
201+
}
202+
})
203+
// even if it were internal error, we must be able to identify exceptions
204+
// raised during authentication so that we don't append body by default, hence wrapping it
205+
.transform(AuthenticationFailedException::new);
206+
identityAssociation.setIdentity(identityUni);
192207
}
208+
identityAssociationNotSet = false;
193209
}
194210
}
211+
if (identityAssociationNotSet) {
212+
Uni<SecurityIdentity> identityUni = identityProviderManager.authenticate(AnonymousAuthenticationRequest.INSTANCE);
213+
identityAssociation.setIdentity(identityUni);
214+
}
195215
ServerCall.Listener<ReqT> listener = serverCallHandler.startCall(serverCall, metadata);
196216
return exceptionHandlerProvider.createHandler(listener, serverCall, metadata);
197217
}
@@ -213,6 +233,10 @@ public static void propagateSecurityIdentityWithDuplicatedCtx(RoutingContext eve
213233
getCapturedVertxContext().putLocal(IDENTITY_KEY, existing.getSecurityIdentity());
214234
} else {
215235
getCapturedVertxContext().putLocal(DEFERRED_IDENTITY_KEY, QuarkusHttpUser.getSecurityIdentity(event, null));
236+
// we will handle failures ourselves, so that response is written once
237+
// do this even if the authentication failure handler is not DefaultAuthFailureHandler because
238+
// it might be the failure handler added by the Quarkus REST when it is present
239+
event.remove(QuarkusHttpUser.AUTH_FAILURE_HANDLER);
216240
}
217241
}
218242
}

‎extensions/grpc/runtime/src/main/java/io/quarkus/grpc/runtime/GrpcServerRecorder.java‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import io.quarkus.runtime.ShutdownContext;
6060
import io.quarkus.runtime.annotations.Recorder;
6161
import io.quarkus.vertx.http.runtime.PortSystemProperties;
62+
import io.quarkus.vertx.http.runtime.QuarkusErrorHandler;
6263
import io.quarkus.vertx.http.runtime.security.HttpAuthenticator;
6364
import io.quarkus.virtual.threads.VirtualThreadsRecorder;
6465
import io.vertx.core.AbstractVerticle;
@@ -93,6 +94,29 @@ public static List<GrpcServiceDefinition> getServices() {
9394
return services;
9495
}
9596

97+
public void addMainRouterErrorHandlerIfSameServer(RuntimeValue<Router> mainRouter, GrpcConfiguration config) {
98+
if (!config.server().useSeparateServer()) {
99+
mainRouter.getValue().route().last().failureHandler(new Handler<>() {
100+
101+
private final Handler<RoutingContext> errorHandler = new QuarkusErrorHandler(LaunchMode.current().isDevOrTest(),
102+
false, Optional.empty());
103+
104+
@Override
105+
public void handle(RoutingContext event) {
106+
if (isGrpc(event)) {
107+
// this is for failures before that occurred before gRPC started processing, it could be:
108+
// 1. authentication failure
109+
// 2. internal error raised during authentication
110+
// 3. unrelated failure
111+
// if there is an exception on the gRPC route, we should handle it because the most likely cause
112+
// of the failure is authentication; as for the '3.', this is better than having unhandled failures
113+
errorHandler.handle(event);
114+
}
115+
}
116+
});
117+
}
118+
}
119+
96120
public void initializeGrpcServer(RuntimeValue<Vertx> vertxSupplier,
97121
RuntimeValue<Router> routerSupplier,
98122
GrpcConfiguration cfg,
@@ -198,12 +222,13 @@ private void buildGrpcServer(Vertx vertx, GrpcServerConfiguration configuration,
198222
if (securityHandlers != null) {
199223
for (Map.Entry<Integer, Handler<RoutingContext>> e : securityHandlers.entrySet()) {
200224
Handler<RoutingContext> handler = e.getValue();
225+
boolean isAuthenticationHandler = e.getKey() == -200;
201226
Route route = router.route().order(e.getKey()).handler(new Handler<RoutingContext>() {
202227
@Override
203228
public void handle(RoutingContext ctx) {
204229
if (!isGrpc(ctx)) {
205230
ctx.next();
206-
} else if (ctx.get(HttpAuthenticator.class.getName()) != null) {
231+
} else if (isAuthenticationHandler && ctx.get(HttpAuthenticator.class.getName()) != null) {
207232
// this IF branch shouldn't be invoked with current implementation
208233
// when gRPC is attached to the main router when the root path is not '/'
209234
// because HTTP authenticator and authorizer handlers are not added by default on the main

0 commit comments

Comments
 (0)