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.

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(tls): use TLS by default in the Agent Client #747

Merged
merged 9 commits into from
Dec 20, 2024

Conversation

Josh-Matsuoka
Copy link
Contributor

Hi,

This is the other half to cryostatio/cryostat-agent#554 , enforcing TLS by default on the server side for agent communication. Controlled by a configuration option cryostat.agent.tls.enabled

Addresses #686

@andrewazores
Copy link
Member

This doesn't perform the TLS check until Cryostat is going to initiate a connection to the Agent. Would it make sense to do an https assertion on the Agent's callback URI scheme at registration time, too? That would tie in closely with #746 . Then Cryostat can actually reject non-TLS Agents when it is in this "TLS required" mode, instead of allowing them to register and then blocking all outgoing connections to them.

@andrewazores
Copy link
Member

andrewazores commented Dec 18, 2024

I think this still isn't blocking Agent registrations early enough:

cryostat-1                | 2024-12-18 20:12:44,190 WARN  [com.git.ben.caf.cac.LocalAsyncCache] (executor-thread-5) Exception thrown during asynchronous load: java.util.concurrent.CompletionException: io.quarkus.narayana.jta.QuarkusTransactionException: java.net.MalformedURLException: Agent connections are required to be TLS by (cryostat.agent.tls.required)
cryostat-1                | 	at io.cryostat.targets.TargetConnectionManager$ConnectionLoader.lambda$asyncLoad$0(TargetConnectionManager.java:359)
cryostat-1                | 	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
cryostat-1                | 	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
cryostat-1                | 	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
cryostat-1                | 	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
cryostat-1                | 	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
cryostat-1                | 	at java.base/java.lang.Thread.run(Thread.java:1583)
cryostat-1                | Caused by: io.quarkus.narayana.jta.QuarkusTransactionException: java.net.MalformedURLException: Agent connections are required to be TLS by (cryostat.agent.tls.required)
cryostat-1                | 	at io.quarkus.narayana.jta.QuarkusTransactionImpl.callInOurTx(QuarkusTransactionImpl.java:150)
cryostat-1                | 	at io.quarkus.narayana.jta.QuarkusTransactionImpl.callJoinExisting(QuarkusTransactionImpl.java:79)
cryostat-1                | 	at io.quarkus.narayana.jta.QuarkusTransactionImpl.call(QuarkusTransactionImpl.java:33)
cryostat-1                | 	at io.quarkus.narayana.jta.TransactionRunnerImpl.call(TransactionRunnerImpl.java:34)
cryostat-1                | 	at io.cryostat.targets.TargetConnectionManager.connect(TargetConnectionManager.java:303)
cryostat-1                | 	at io.cryostat.targets.TargetConnectionManager$ConnectionLoader.lambda$asyncLoad$0(TargetConnectionManager.java:357)
cryostat-1                | 	... 9 more
cryostat-1                | Caused by: java.net.MalformedURLException: Agent connections are required to be TLS by (cryostat.agent.tls.required)
cryostat-1                | 	at io.cryostat.targets.AgentConnection$Factory.createConnection(AgentConnection.java:170)
cryostat-1                | 	at io.cryostat.targets.AgentConnection_Factory_ClientProxy.createConnection(Unknown Source)
cryostat-1                | 	at io.cryostat.targets.TargetConnectionManager.connect(TargetConnectionManager.java:320)
cryostat-1                | 	at io.cryostat.targets.TargetConnectionManager.lambda$connect$10(TargetConnectionManager.java:307)
cryostat-1                | 	at io.quarkus.narayana.jta.QuarkusTransactionImpl.callInOurTx(QuarkusTransactionImpl.java:136)
cryostat-1                | 	... 14 more

image

I set this up by:

  1. Check out and build this PR: ./mvnw package
  2. Check out feat(tls): Use TLS by default in the agent cryostat-agent#554 and ./mvnw install
  3. Rebuild quarkus-cryostat-agent: CRYOSTAT_AGENT_VERSION=0.5.0-snapshot ./build.bash
  4. Run Cryostat smoketest with sample app TLS disabled: SAMPLE_APPS_USE_TLS=false ./smoketest.bash -O -t quarkus-cryostat-agent
  5. Check Topology view, then go to Recordings and select the http:// Agent sample application. See if it's able to list active recordings.

I think what I'd like to see in this scenario is that Cryostat refuses the Agent's attempts to register itself in the first place. This would happen in Discovery.java with the endpoints for discovery plugin registration and publication, like the URI range checking in #746. This way we can say that Agent HTTPS connections are really enforced. In the current implementation the server blocks itself from opening connections to such Agents, but why should it even accept their registrations to begin with?

@andrewazores
Copy link
Member

andrewazores commented Dec 19, 2024

The Agent registration is "blocked" now, but I think not in the intended way:

2024-12-19 21:41:58,458 DEBUG [io.qua.ver.htt.run.ForwardedParser] (vert.x-eventloop-thread-0) Recalculated absoluteURI to https://auth:8443/api/v4/discovery
2024-12-19 21:41:58,458 DEBUG [io.qua.jfr.run.htt.res.JfrReactiveServerFilter] (executor-thread-7) Enter Jfr Reactive Request Filter
2024-12-19 21:41:58,459 DEBUG [org.jbo.res.rea.com.cor.AbstractResteasyReactiveContext] (executor-thread-7) Attempting to handle unrecoverable exception: java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because the return value of "java.net.URI.getScheme()" is null
	at io.cryostat.discovery.Discovery.register(Discovery.java:218)
	at io.cryostat.discovery.Discovery_Subclass.register$$superforward(Unknown Source)
	at io.cryostat.discovery.Discovery_Subclass$$function$$1.apply(Unknown Source)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:136)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:107)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.doIntercept(TransactionalInterceptorRequired.java:38)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.intercept(TransactionalInterceptorBase.java:61)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.intercept(TransactionalInterceptorRequired.java:32)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
	at io.cryostat.discovery.Discovery_Subclass.register(Unknown Source)
	at io.cryostat.discovery.Discovery$quarkusrestinvoker$register_121ad36d63a7d07486382c062cb0b09b644a4ff2.invoke(Unknown Source)
	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)

2024-12-19 21:41:58,459 DEBUG [io.ver.ext.web.RoutingContext] (executor-thread-7) RoutingContext failure (500): java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because the return value of "java.net.URI.getScheme()" is null
	at io.cryostat.discovery.Discovery.register(Discovery.java:218)
	at io.cryostat.discovery.Discovery_Subclass.register$$superforward(Unknown Source)
	at io.cryostat.discovery.Discovery_Subclass$$function$$1.apply(Unknown Source)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:136)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:107)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.doIntercept(TransactionalInterceptorRequired.java:38)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.intercept(TransactionalInterceptorBase.java:61)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.intercept(TransactionalInterceptorRequired.java:32)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
	at io.cryostat.discovery.Discovery_Subclass.register(Unknown Source)
	at io.cryostat.discovery.Discovery$quarkusrestinvoker$register_121ad36d63a7d07486382c062cb0b09b644a4ff2.invoke(Unknown Source)
	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)

2024-12-19 21:41:58,459 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-7) HTTP Request to /api/v4/discovery failed, error id: 3be0e910-662a-4de3-aa5e-81ab9c70f2c8-28: java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because the return value of "java.net.URI.getScheme()" is null
	at io.cryostat.discovery.Discovery.register(Discovery.java:218)
	at io.cryostat.discovery.Discovery_Subclass.register$$superforward(Unknown Source)
	at io.cryostat.discovery.Discovery_Subclass$$function$$1.apply(Unknown Source)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:136)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:107)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.doIntercept(TransactionalInterceptorRequired.java:38)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.intercept(TransactionalInterceptorBase.java:61)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.intercept(TransactionalInterceptorRequired.java:32)
	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
	at io.cryostat.discovery.Discovery_Subclass.register(Unknown Source)
	at io.cryostat.discovery.Discovery$quarkusrestinvoker$register_121ad36d63a7d07486382c062cb0b09b644a4ff2.invoke(Unknown Source)
	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)

I think this is because the check in Discovery.java uses the connection remote address to try to form a URL, and then to assert the "https" scheme on that URL. But that URL is the connection address, ie it isn't an HTTP URL but rather just a host:port pair, so we get this NPE above.

This would mean that actually all Agent registrations are now blocked (broken), not only ones that fail the TLS assertion.

andrewazores
andrewazores previously approved these changes Dec 20, 2024
@andrewazores
Copy link
Member

/build_test

Copy link

Workflow started at 12/20/2024, 2:52:43 PM. View Actions Run.

Copy link

No OpenAPI schema changes detected.

Copy link

No GraphQL schema changes detected.

@andrewazores andrewazores dismissed their stale review December 20, 2024 19:58

manual test not working as expected

Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat/actions/runs/12437599678

@andrewazores
Copy link
Member

When I run the SAMPLE_APPS_USE_TLS=false ./smoketest.bash -O -t quarkus-cryostat-agent manual test case above, it now looks like the server is correctly rejecting the Agent's registration in the expected manner.

However, when I do ./smoketest.bash -O -t quarkus-cryostat-agent, ie to try to exercise the normal case where the Agent is using TLS, then the Agent still fails to register.

cryostat-1                | 2024-12-20 20:04:55,309 DEBUG [WebApplicationException] (executor-thread-5) Application failed the request: jakarta.ws.rs.BadRequestException: TLS for agent connections is required by (cryostat.agent.tls.required)
cryostat-1                | 	at io.cryostat.discovery.Discovery.publish(Discovery.java:344)
cryostat-1                | 	at io.cryostat.discovery.Discovery_Subclass.publish$$superforward(Unknown Source)
cryostat-1                | 	at io.cryostat.discovery.Discovery_Subclass$$function$$3.apply(Unknown Source)
cryostat-1                | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
cryostat-1                | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:136)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx(TransactionalInterceptorBase.java:107)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.doIntercept(TransactionalInterceptorRequired.java:38)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.intercept(TransactionalInterceptorBase.java:61)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired.intercept(TransactionalInterceptorRequired.java:32)
cryostat-1                | 	at io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorRequired_Bean.intercept(Unknown Source)
cryostat-1                | 	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
cryostat-1                | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
cryostat-1                | 	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
cryostat-1                | 	at io.cryostat.discovery.Discovery_Subclass.publish(Unknown Source)
cryostat-1                | 	at io.cryostat.discovery.Discovery$quarkusrestinvoker$publish_633af03127849917d890ce59fbb98d67cef2e040.invoke(Unknown Source)
cryostat-1                | 	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
cryostat-1                | 	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
cryostat-1                | 	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
cryostat-1                | 	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
cryostat-1                | 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
cryostat-1                | 	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
cryostat-1                | 	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
cryostat-1                | 	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
cryostat-1                | 	at java.base/java.lang.Thread.run(Thread.java:1583)

I suspect the problem is Agent dual-registration. Since this Agent instance in the smoketest has JMX enabled as well, after the Agent registers itself it actually publishes two Target definitions - one for itself using HTTP(S), and the other for itself via JMX. The Discovery.java line 344 referenced in the stacktrace points to the new https URL scheme check. Makes sense - the JMX URL on the second publication is not going to match that, so the server is rejecting it. I think we still want to allow that case. I'm not sure if we have any better way to determine if a JMX connection is using TLS or not - it doesn't seem like this is indicated in the URL protocol or any other attributes like it conventionally is for http:///https://.

So I guess when we're checking for the target connection URLs in that publish method, we need to check whether it's a JMX or HTTP URL, and if it's HTTP then check if we're enforcing HTTPS.

So something roughly like:

var url = b.target.connectUrl;
var isJmx = uriUtil.isJmxUrl(url);
if (!isJmx) {
  var scheme = b.target.connectUrl.getScheme();
  var isHttp = scheme.equals("http");
  var isHttps = scheme.equals("https");
  if (agentTlsRequired && !isHttps) throw new BadRequestException(); // enforce TLS
  if (!isHttp && !isHttps) throw new BadRequestException(); // non-JMX and also non-HTTP(S)
}
// if we got here, it's either JMX, or HTTPS, or HTTP with TLS enforcement disabled, so we're ok - continue processing
// ...

@andrewazores andrewazores merged commit 443c9c0 into cryostatio:main Dec 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants