Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jmx): restore error codes for auth and TLS connection failures #350

Merged
merged 9 commits into from
Apr 15, 2024
Merged

fix(jmx): restore error codes for auth and TLS connection failures #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
51f1f2f
restore error response codes for JMX Auth and JMX TLS failures
andrewazores Apr 2, 2024
29e63b3
cleanup
andrewazores Apr 3, 2024
fa33f14
handle null exception cause
andrewazores Apr 3, 2024
e5eaca7
make methods private non-static
andrewazores Apr 3, 2024
e5d8864
fixup! cleanup
andrewazores Apr 3, 2024
8cd6a98
spotless
andrewazores Apr 3, 2024
05ff4e3
fixup! make methods private non-static
andrewazores Apr 3, 2024
091c575
do not fire retries for SnapshotCreationException
andrewazores Apr 3, 2024
2606aee
fixup! restore error response codes for JMX Auth and JMX TLS failures
andrewazores Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions src/main/java/io/cryostat/ExceptionMappers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -89,22 +86,6 @@ public RestResponse<Void> mapIllegalArgumentException(IllegalArgumentException e
return RestResponse.status(HttpResponseStatus.BAD_REQUEST.code());
}

@ServerExceptionMapper
public RestResponse<Void> mapJmxConnectionException(ConnectionException ex) {
logger.warn(ex);
return RestResponse.status(HttpResponseStatus.BAD_GATEWAY.code());
}

@ServerExceptionMapper
public RestResponse<Void> 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<Void> mapMutinyTimeoutException(TimeoutException ex) {
logger.warn(ex);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/cryostat/recordings/RecordingHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ public RecordingNotFoundException(Pair<String, String> key) {
}
}

static class SnapshotCreationException extends Exception {
public static class SnapshotCreationException extends Exception {
public SnapshotCreationException(String message) {
super(message);
}
Expand Down
87 changes: 53 additions & 34 deletions src/main/java/io/cryostat/targets/TargetConnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -196,13 +197,24 @@ public <T> Uni<T> executeConnectedTaskUni(Target target, ConnectedTask<T> 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> T executeConnectedTask(Target target, ConnectedTask<T> task) {
Expand All @@ -220,13 +232,24 @@ public <T> Uni<T> 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));
}

/**
Expand Down Expand Up @@ -374,17 +397,21 @@ public interface ConnectedTask<T> {
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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/itest/TargetEventsGetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public void testGetTargetEventsV2WithQueryReturnsRequestedEvents() throws Except
LinkedHashMap<String, Object> expectedResults = new LinkedHashMap<String, Object>();
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(
Expand Down
Loading