From 794db332e93e58fe3ab4473703e58c2a59818822 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 15 Apr 2024 09:13:45 -0400 Subject: [PATCH] fix(jmx): restore error codes for auth and TLS connection failures (#350) --- .../java/io/cryostat/ExceptionMappers.java | 19 ---- .../cryostat/recordings/RecordingHelper.java | 2 +- .../targets/TargetConnectionManager.java | 87 +++++++++++-------- src/test/java/itest/TargetEventsGetTest.java | 2 +- 4 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/main/java/io/cryostat/ExceptionMappers.java b/src/main/java/io/cryostat/ExceptionMappers.java index acb7c439a..9a05221c7 100644 --- a/src/main/java/io/cryostat/ExceptionMappers.java +++ b/src/main/java/io/cryostat/ExceptionMappers.java @@ -19,9 +19,6 @@ import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; -import org.openjdk.jmc.rjmx.ConnectionException; - -import io.cryostat.targets.TargetConnectionManager; import io.cryostat.util.EntityExistsException; import com.nimbusds.jwt.proc.BadJWTException; @@ -89,22 +86,6 @@ public RestResponse mapIllegalArgumentException(IllegalArgumentException e return RestResponse.status(HttpResponseStatus.BAD_REQUEST.code()); } - @ServerExceptionMapper - public RestResponse mapJmxConnectionException(ConnectionException ex) { - logger.warn(ex); - return RestResponse.status(HttpResponseStatus.BAD_GATEWAY.code()); - } - - @ServerExceptionMapper - public RestResponse mapFlightRecorderException( - org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException ex) { - logger.warn(ex); - if (TargetConnectionManager.isJmxAuthFailure(ex)) { - return RestResponse.status(HttpResponseStatus.FORBIDDEN.code()); - } - return RestResponse.status(HttpResponseStatus.BAD_GATEWAY.code()); - } - @ServerExceptionMapper public RestResponse mapMutinyTimeoutException(TimeoutException ex) { logger.warn(ex); diff --git a/src/main/java/io/cryostat/recordings/RecordingHelper.java b/src/main/java/io/cryostat/recordings/RecordingHelper.java index ca12cb6db..8ba809f86 100644 --- a/src/main/java/io/cryostat/recordings/RecordingHelper.java +++ b/src/main/java/io/cryostat/recordings/RecordingHelper.java @@ -957,7 +957,7 @@ public RecordingNotFoundException(Pair key) { } } - static class SnapshotCreationException extends Exception { + public static class SnapshotCreationException extends Exception { public SnapshotCreationException(String message) { super(message); } diff --git a/src/main/java/io/cryostat/targets/TargetConnectionManager.java b/src/main/java/io/cryostat/targets/TargetConnectionManager.java index e48e772b1..17b7791f2 100644 --- a/src/main/java/io/cryostat/targets/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/targets/TargetConnectionManager.java @@ -31,7 +31,6 @@ import java.util.concurrent.Semaphore; import javax.management.remote.JMXServiceURL; -import javax.naming.ServiceUnavailableException; import javax.security.sasl.SaslException; import org.openjdk.jmc.rjmx.ConnectionException; @@ -42,6 +41,7 @@ import io.cryostat.core.net.JFRConnectionToolkit; import io.cryostat.credentials.Credential; import io.cryostat.expressions.MatchExpressionEvaluator; +import io.cryostat.recordings.RecordingHelper.SnapshotCreationException; import io.cryostat.targets.Target.EventKind; import io.cryostat.targets.Target.TargetDiscovery; @@ -54,6 +54,7 @@ import io.quarkus.vertx.ConsumeEvent; import io.smallrye.mutiny.Uni; import io.smallrye.mutiny.unchecked.Unchecked; +import io.vertx.ext.web.handler.HttpException; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.transaction.Transactional; @@ -196,13 +197,24 @@ public Uni executeConnectedTaskUni(Target target, ConnectedTask task) } })) .onFailure(RuntimeException.class) - .transform(this::unwrapRuntimeException) + .transform(t -> unwrapNestedException(RuntimeException.class, t)) .onFailure() .invoke(logger::warn) - .onFailure(t -> isTargetConnectionFailure(t) || isUnknownTargetFailure(t)) + .onFailure(this::isJmxAuthFailure) + .transform(t -> new HttpException(427, t)) + .onFailure(this::isJmxSslFailure) + .transform(t -> new HttpException(502, t)) + .onFailure(this::isServiceTypeFailure) + .transform(t -> new HttpException(504, t)) + .onFailure( + t -> + !(t instanceof HttpException) + && !(t instanceof SnapshotCreationException)) .retry() .withBackOff(failedBackoff) - .expireIn(failedTimeout.plusMillis(System.currentTimeMillis()).toMillis()); + .expireIn(failedTimeout.plusMillis(System.currentTimeMillis()).toMillis()) + .onFailure(this::isTargetConnectionFailure) + .transform(t -> new HttpException(504, t)); } public T executeConnectedTask(Target target, ConnectedTask task) { @@ -220,13 +232,24 @@ public Uni executeDirect( } })) .onFailure(RuntimeException.class) - .transform(this::unwrapRuntimeException) + .transform(t -> unwrapNestedException(RuntimeException.class, t)) .onFailure() .invoke(logger::warn) - .onFailure(t -> isTargetConnectionFailure(t) || isUnknownTargetFailure(t)) + .onFailure(this::isJmxAuthFailure) + .transform(t -> new HttpException(427, t)) + .onFailure(this::isJmxSslFailure) + .transform(t -> new HttpException(502, t)) + .onFailure(this::isServiceTypeFailure) + .transform(t -> new HttpException(504, t)) + .onFailure( + t -> + !(t instanceof HttpException) + && !(t instanceof SnapshotCreationException)) .retry() .withBackOff(failedBackoff) - .expireIn(failedTimeout.plusMillis(System.currentTimeMillis()).toMillis()); + .expireIn(failedTimeout.plusMillis(System.currentTimeMillis()).toMillis()) + .onFailure(this::isTargetConnectionFailure) + .transform(t -> new HttpException(504, t)); } /** @@ -374,17 +397,21 @@ public interface ConnectedTask { T execute(JFRConnection connection) throws Exception; } - public Throwable unwrapRuntimeException(Throwable t) { + public Throwable unwrapNestedException(Class klazz, Throwable t) { final int maxDepth = 10; int depth = 0; Throwable cause = t; - while (cause instanceof RuntimeException && depth++ < maxDepth) { + while (klazz.isInstance(t) && depth++ < maxDepth) { + var c = cause.getCause(); + if (c == null) { + break; + } cause = cause.getCause(); } return cause; } - public static boolean isTargetConnectionFailure(Throwable t) { + private boolean isTargetConnectionFailure(Throwable t) { if (!(t instanceof Exception)) { return false; } @@ -393,16 +420,26 @@ public static boolean isTargetConnectionFailure(Throwable t) { || ExceptionUtils.indexOfType(e, FlightRecorderException.class) >= 0; } - public static boolean isJmxAuthFailure(Throwable t) { + /** + * Check if the exception happened because the connection required authentication, and we had no + * credentials to present. + */ + private boolean isJmxAuthFailure(Throwable t) { if (!(t instanceof Exception)) { return false; } Exception e = (Exception) t; - return ExceptionUtils.indexOfType(e, SecurityException.class) >= 0 + return ExceptionUtils.indexOfType(e, javax.security.auth.login.FailedLoginException.class) + >= 0 + || ExceptionUtils.indexOfType(e, SecurityException.class) >= 0 || ExceptionUtils.indexOfType(e, SaslException.class) >= 0; } - public static boolean isJmxSslFailure(Throwable t) { + /** + * Check if the exception happened because the connection presented an SSL/TLS cert which we + * don't trust. + */ + private boolean isJmxSslFailure(Throwable t) { if (!(t instanceof Exception)) { return false; } @@ -412,7 +449,7 @@ public static boolean isJmxSslFailure(Throwable t) { } /** Check if the exception happened because the port connected to a non-JMX service. */ - public static boolean isServiceTypeFailure(Throwable t) { + private boolean isServiceTypeFailure(Throwable t) { if (!(t instanceof Exception)) { return false; } @@ -421,23 +458,9 @@ public static boolean isServiceTypeFailure(Throwable t) { && ExceptionUtils.indexOfType(e, SocketTimeoutException.class) >= 0; } - public static boolean isUnknownTargetFailure(Throwable t) { - if (!(t instanceof Exception)) { - return false; - } - Exception e = (Exception) t; - return ExceptionUtils.indexOfType(e, java.net.UnknownHostException.class) >= 0 - || ExceptionUtils.indexOfType(e, java.rmi.UnknownHostException.class) >= 0 - || ExceptionUtils.indexOfType(e, ServiceUnavailableException.class) >= 0; - } - - @Name("io.cryostat.net.TargetConnectionManager.TargetConnectionOpened") + @Name("io.cryostat.targets.TargetConnectionManager.TargetConnectionOpened") @Label("Target Connection Opened") @Category("Cryostat") - // @SuppressFBWarnings( - // value = "URF_UNREAD_FIELD", - // justification = "The event fields are recorded with JFR instead of accessed - // directly") public static class TargetConnectionOpened extends Event { String serviceUri; boolean exceptionThrown; @@ -452,13 +475,9 @@ void setExceptionThrown(boolean exceptionThrown) { } } - @Name("io.cryostat.net.TargetConnectionManager.TargetConnectionClosed") + @Name("io.cryostat.targets.TargetConnectionManager.TargetConnectionClosed") @Label("Target Connection Closed") @Category("Cryostat") - // @SuppressFBWarnings( - // value = "URF_UNREAD_FIELD", - // justification = "The event fields are recorded with JFR instead of accessed - // directly") public static class TargetConnectionClosed extends Event { URI serviceUri; boolean exceptionThrown; diff --git a/src/test/java/itest/TargetEventsGetTest.java b/src/test/java/itest/TargetEventsGetTest.java index 0323d8d5a..aa0e6582d 100644 --- a/src/test/java/itest/TargetEventsGetTest.java +++ b/src/test/java/itest/TargetEventsGetTest.java @@ -114,7 +114,7 @@ public void testGetTargetEventsV2WithQueryReturnsRequestedEvents() throws Except LinkedHashMap expectedResults = new LinkedHashMap(); expectedResults.put("name", "Target Connection Opened"); expectedResults.put( - "typeId", "io.cryostat.net.TargetConnectionManager.TargetConnectionOpened"); + "typeId", "io.cryostat.targets.TargetConnectionManager.TargetConnectionOpened"); expectedResults.put("description", ""); expectedResults.put("category", List.of("Cryostat")); expectedResults.put(