Skip to content

Commit

Permalink
fix(discovery): respond 400, not 500, on validation failures (#816)
Browse files Browse the repository at this point in the history
* test(discovery): add basic plugin registration validations

* fix(discovery): respond 400, not 500, on validation failures

* add exception messages

* do not require TLS for plugin registration during testing

* fix up non-registered publication test

* flesh out tests
  • Loading branch information
andrewazores authored Feb 26, 2025
1 parent 8b330f1 commit a99ba01
Show file tree
Hide file tree
Showing 7 changed files with 490 additions and 468 deletions.
172 changes: 108 additions & 64 deletions src/main/java/io/cryostat/discovery/Discovery.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import io.cryostat.util.URIUtil;

import com.fasterxml.jackson.annotation.JsonView;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.nimbusds.jose.JOSEException;
import com.nimbusds.jwt.proc.BadJWTException;
Expand Down Expand Up @@ -163,15 +162,18 @@ public DiscoveryNode get() {
@Path("/api/v4/discovery/{id}")
@RolesAllowed("read")
public void checkRegistration(
@Context RoutingContext ctx, @RestPath UUID id, @RestQuery String token)
throws SocketException,
UnknownHostException,
MalformedURLException,
ParseException,
JOSEException,
URISyntaxException {
@Context RoutingContext ctx, @RestPath UUID id, @RestQuery String token) {
DiscoveryPlugin plugin = DiscoveryPlugin.find("id", id).singleResult();
jwtValidator.validateJwt(ctx, plugin, token, true);
try {
jwtValidator.validateJwt(ctx, plugin, token, true);
} catch (MalformedURLException
| URISyntaxException
| UnknownHostException
| SocketException
| JOSEException
| ParseException e) {
throw new BadRequestException(e);
}
}

@Transactional
Expand All @@ -182,39 +184,54 @@ public void checkRegistration(
@RolesAllowed("write")
@SuppressFBWarnings("DLS_DEAD_LOCAL_STORE")
public PluginRegistration register(@Context RoutingContext ctx, JsonObject body)
throws URISyntaxException,
MalformedURLException,
JOSEException,
UnknownHostException,
SocketException,
ParseException,
BadJWTException,
SchedulerException {
throws SchedulerException {
String pluginId = body.getString("id");
String priorToken = body.getString("token");
String realmName = body.getString("realm");
URI callbackUri = new URI(body.getString("callback"));
URI callbackUri;
try {
String callback = body.getString("callback");
if (StringUtils.isBlank(callback)) {
throw new BadRequestException("callback cannot be blank");
}
callbackUri = new URI(callback);
} catch (URISyntaxException e) {
throw new BadRequestException(e);
}
URI unauthCallback = UriBuilder.fromUri(callbackUri).userInfo(null).build();

// URI range validation
if (!uriUtil.validateUri(callbackUri)) {
throw new BadRequestException(
String.format(
"cryostat.target.callback of \"%s\" is unacceptable with the"
+ " current URI range settings",
callbackUri));
InetAddress remoteAddress = getRemoteAddress(ctx);
URI remoteURI;
try {
remoteURI = new URI(remoteAddress.getHostAddress());
} catch (URISyntaxException e) {
throw new BadRequestException(e);
}

InetAddress remoteAddress = getRemoteAddress(ctx);
URI remoteURI = new URI(remoteAddress.getHostAddress());
if (!uriUtil.validateUri(remoteURI)) {
throw new BadRequestException(
String.format(
"Remote Address of \"%s\" is unacceptable with the"
+ " current URI range settings",
remoteURI));
try {
if (!uriUtil.validateUri(callbackUri)) {
throw new BadRequestException(
String.format(
"cryostat.target.callback of \"%s\" is unacceptable with the"
+ " current URI range settings",
callbackUri));
}
if (!uriUtil.validateUri(remoteURI)) {
throw new BadRequestException(
String.format(
"Remote Address of \"%s\" is unacceptable with the"
+ " current URI range settings",
remoteURI));
}
} catch (MalformedURLException e) {
throw new BadRequestException(e);
}

for (var e : new String[] {callbackUri.getScheme(), callbackUri.getHost()}) {
if (StringUtils.isBlank(e)) {
throw new BadRequestException("callback must contain scheme and host");
}
}
if (agentTlsRequired && !callbackUri.getScheme().equals("https")) {
throw new BadRequestException(
String.format(
Expand All @@ -233,10 +250,20 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body)
throw new ForbiddenException();
}
if (!Objects.equals(plugin.callback, unauthCallback)) {
throw new BadRequestException();
throw new BadRequestException("plugin callback mismatch");
}
try {
location = jwtFactory.getPluginLocation(plugin);
jwtFactory.parseDiscoveryPluginJwt(
plugin, priorToken, location, remoteAddress, false);
} catch (URISyntaxException
| UnknownHostException
| SocketException
| BadJWTException
| JOSEException
| ParseException e) {
throw new BadRequestException(e);
}
location = jwtFactory.getPluginLocation(plugin);
jwtFactory.parseDiscoveryPluginJwt(plugin, priorToken, location, remoteAddress, false);
} else {
// check if a plugin record with the same callback already exists. If it does,
// ping it:
Expand Down Expand Up @@ -274,7 +301,11 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body)
universe.children.add(plugin.realm);
universe.persist();

location = jwtFactory.getPluginLocation(plugin);
try {
location = jwtFactory.getPluginLocation(plugin);
} catch (URISyntaxException e) {
throw new BadRequestException(e);
}

var dataMap = new JobDataMap();
dataMap.put(PLUGIN_ID_MAP_KEY, plugin.id);
Expand All @@ -297,7 +328,12 @@ public PluginRegistration register(@Context RoutingContext ctx, JsonObject body)
scheduler.scheduleJob(jobDetail, trigger);
}

String token = jwtFactory.createDiscoveryPluginJwt(plugin, remoteAddress, location);
String token;
try {
token = jwtFactory.createDiscoveryPluginJwt(plugin, remoteAddress, location);
} catch (URISyntaxException | JOSEException | UnknownHostException | SocketException e) {
throw new BadRequestException(e);
}

// TODO implement more generic env map passing by some platform detection
// strategy or generalized config properties
Expand All @@ -318,26 +354,32 @@ public void publish(
@Context RoutingContext ctx,
@RestPath UUID id,
@RestQuery String token,
List<DiscoveryNode> body)
throws SocketException,
UnknownHostException,
MalformedURLException,
ParseException,
JOSEException,
URISyntaxException {
List<DiscoveryNode> body) {
DiscoveryPlugin plugin = DiscoveryPlugin.find("id", id).singleResult();
jwtValidator.validateJwt(ctx, plugin, token, true);
try {
jwtValidator.validateJwt(ctx, plugin, token, true);
} catch (MalformedURLException
| URISyntaxException
| UnknownHostException
| SocketException
| JOSEException
| ParseException e) {
throw new BadRequestException(e);
}
plugin.realm.children.clear();
plugin.realm.children.addAll(body);
for (var b : body) {
if (b.target != null) {
// URI range validation
if (!uriUtil.validateUri(b.target.connectUrl)) {
throw new BadRequestException(
String.format(
"Connect URL of \"%s\" is unacceptable with the"
+ " current URI range settings",
b.target.connectUrl));
try {
if (!uriUtil.validateUri(b.target.connectUrl)) {
throw new BadRequestException(
String.format(
"Connect URL of \"%s\" is unacceptable with the"
+ " current URI range settings",
b.target.connectUrl));
}
} catch (MalformedURLException e) {
throw new BadRequestException(e);
}
if (!uriUtil.isJmxUrl(b.target.connectUrl)) {
if (agentTlsRequired && !b.target.connectUrl.getScheme().equals("https")) {
Expand Down Expand Up @@ -370,15 +412,18 @@ public void publish(
@Path("/api/v4/discovery/{id}")
@PermitAll
public void deregister(@Context RoutingContext ctx, @RestPath UUID id, @RestQuery String token)
throws SocketException,
UnknownHostException,
MalformedURLException,
ParseException,
JOSEException,
URISyntaxException,
SchedulerException {
throws SchedulerException {
DiscoveryPlugin plugin = DiscoveryPlugin.find("id", id).singleResult();
jwtValidator.validateJwt(ctx, plugin, token, false);
try {
jwtValidator.validateJwt(ctx, plugin, token, false);
} catch (MalformedURLException
| URISyntaxException
| UnknownHostException
| SocketException
| JOSEException
| ParseException e) {
throw new BadRequestException(e);
}
if (plugin.builtin) {
throw new ForbiddenException();
}
Expand All @@ -396,8 +441,7 @@ public void deregister(@Context RoutingContext ctx, @RestPath UUID id, @RestQuer
@JsonView(DiscoveryNode.Views.Flat.class)
@Path("/api/v4/discovery_plugins")
@RolesAllowed("read")
public List<DiscoveryPlugin> getPlugins(@RestQuery String realm)
throws JsonProcessingException {
public List<DiscoveryPlugin> getPlugins(@RestQuery String realm) {
// TODO filter for the matching realm name within the DB query
return DiscoveryPlugin.findAll().<DiscoveryPlugin>list().stream()
.filter(p -> StringUtils.isBlank(realm) || p.realm.name.equals(realm))
Expand All @@ -407,7 +451,7 @@ public List<DiscoveryPlugin> getPlugins(@RestQuery String realm)
@GET
@Path("/api/v4/discovery_plugins/{id}")
@RolesAllowed("read")
public DiscoveryPlugin getPlugin(@RestPath UUID id) throws JsonProcessingException {
public DiscoveryPlugin getPlugin(@RestPath UUID id) {
return DiscoveryPlugin.find("id", id).singleResult();
}

Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/application-test.properties
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ cryostat.discovery.kubernetes.enabled=true
quarkus.test.env.JAVA_OPTS_APPEND=-Dquarkus.http.host=0.0.0.0 -Djava.util.logging.manager=org.jboss.logmanager.LogManager -Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=9091 -Dcom.sun.management.jmxremote.rmi.port=9091 -Djava.rmi.server.hostname=127.0.0.1 -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false -Dcom.sun.management.jmxremote.local.only=false
quarkus.http.test-timeout=60s

cryostat.agent.tls.required=false

quarkus.datasource.devservices.enabled=true
quarkus.datasource.devservices.image-name=quay.io/cryostat/cryostat-db
quarkus.hibernate-orm.log.sql=true
Expand Down
97 changes: 97 additions & 0 deletions src/test/java/io/cryostat/AbstractTestBase.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright The Cryostat Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.cryostat;

import static io.restassured.RestAssured.given;

import java.time.Duration;

import io.cryostat.util.HttpStatusCodeIdentifier;

import io.restassured.http.ContentType;
import jakarta.inject.Inject;
import org.apache.http.client.utils.URLEncodedUtils;
import org.eclipse.microprofile.config.inject.ConfigProperty;
import org.jboss.logging.Logger;
import org.junit.jupiter.api.BeforeEach;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.HeadBucketRequest;

public abstract class AbstractTestBase {

public static final String SELF_JMX_URL = "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi";
public static String SELF_JMX_URL_ENCODED =
URLEncodedUtils.formatSegments(SELF_JMX_URL).substring(1);
public static final String SELFTEST_ALIAS = "selftest";

@ConfigProperty(name = "storage.buckets.archives.name")
String archivesBucket;

@ConfigProperty(name = "test.storage.timeout", defaultValue = "5m")
Duration storageTimeout;

@ConfigProperty(name = "test.storage.retry", defaultValue = "5s")
Duration storageRetry;

@Inject Logger logger;
@Inject S3Client storage;

@BeforeEach
void waitForStorage() throws InterruptedException {
long totalTime = 0;
while (!bucketExists(archivesBucket)) {
long start = System.nanoTime();
Thread.sleep(storageRetry.toMillis());
long elapsed = System.nanoTime() - start;
totalTime += elapsed;
if (Duration.ofNanos(totalTime).compareTo(storageTimeout) > 0) {
throw new IllegalStateException("Storage took too long to become ready");
}
}
}

private boolean bucketExists(String bucket) {
boolean exists = false;
try {
exists =
HttpStatusCodeIdentifier.isSuccessCode(
storage.headBucket(HeadBucketRequest.builder().bucket(bucket).build())
.sdkHttpResponse()
.statusCode());
logger.debugv("Storage bucket \"{0}\" exists? {1}", bucket, exists);
} catch (Exception e) {
logger.warn(e);
}
return exists;
}

protected int defineSelfCustomTarget() {
return given().basePath("/")
.log()
.all()
.contentType(ContentType.URLENC)
.formParam("connectUrl", SELF_JMX_URL)
.formParam("alias", SELFTEST_ALIAS)
.when()
.post("/api/v4/targets")
.then()
.log()
.all()
.extract()
.jsonPath()
.getInt("id");
}
}
Loading

0 comments on commit a99ba01

Please sign in to comment.