Trace SSL handshakes in netty 4.0 (#4635)

instrumentation/netty/netty-4.0/javaagent/build.gradle.kts

@@ -37,16 +37,19 @@ tasks {
   val testConnectionSpan by registering(Test::class) {
     filter {
       includeTestsMatching("Netty40ConnectionSpanTest")
+      includeTestsMatching("Netty40ClientSslTest")
       isFailOnNoMatchingTests = false
     }
-    include("**/Netty40ConnectionSpanTest.*")
+    include("**/Netty40ConnectionSpanTest.*", "**/Netty40ClientSslTest.*")
     jvmArgs("-Dotel.instrumentation.netty.always-create-connect-span=true")
+    jvmArgs("-Dotel.instrumentation.netty.ssl-telemetry.enabled=true")
   }
 
   test {
     dependsOn(testConnectionSpan)
     filter {
       excludeTestsMatching("Netty40ConnectionSpanTest")
+      excludeTestsMatching("Netty40ClientSslTest")
       isFailOnNoMatchingTests = false
     }
   }

instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java

@@ -7,10 +7,9 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_0;
 
 import static io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.NettyClientSingletons.connectionInstrumenter;
 import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.returns;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
-import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelPromise;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -32,16 +31,16 @@ public class BootstrapInstrumentation implements TypeInstrumentation {
   @Override
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
-        named("doConnect")
+        named("doConnect0")
-            .and(takesArgument(0, SocketAddress.class))
+            .and(takesArgument(2, SocketAddress.class))
-            .and(returns(named("io.netty.channel.ChannelFuture"))),
+            .and(takesArgument(4, named("io.netty.channel.ChannelPromise"))),
         BootstrapInstrumentation.class.getName() + "$ConnectAdvice");
   }
 
   public static class ConnectAdvice {
     @Advice.OnMethodEnter
     public static void startConnect(
-        @Advice.Argument(0) SocketAddress remoteAddress,
+        @Advice.Argument(2) SocketAddress remoteAddress,
         @Advice.Local("otelContext") Context context,
         @Advice.Local("otelRequest") NettyConnectionRequest request,
         @Advice.Local("otelScope") Scope scope) {
@@ -60,7 +59,7 @@ public class BootstrapInstrumentation implements TypeInstrumentation {
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void endConnect(
         @Advice.Thrown Throwable throwable,
-        @Advice.Return ChannelFuture channelFuture,
+        @Advice.Argument(4) ChannelPromise channelPromise,
         @Advice.Local("otelContext") Context context,
         @Advice.Local("otelRequest") NettyConnectionRequest request,
         @Advice.Local("otelScope") Scope scope) {
@@ -73,7 +72,7 @@ public class BootstrapInstrumentation implements TypeInstrumentation {
       if (throwable != null) {
         connectionInstrumenter().end(context, request, null, throwable);
       } else {
-        channelFuture.addListener(
+        channelPromise.addListener(
             new ConnectionCompleteListener(connectionInstrumenter(), context, request));
       }
     }

instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.netty.v4_0;
 
+import static io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.NettyClientSingletons.sslInstrumenter;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
 import static net.bytebuddy.matcher.ElementMatchers.named;
@@ -22,6 +23,7 @@ import io.opentelemetry.instrumentation.api.field.VirtualField;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
 import io.opentelemetry.javaagent.instrumentation.netty.common.AbstractNettyChannelPipelineInstrumentation;
+import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettySslInstrumentationHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.HttpClientRequestTracingHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.HttpClientResponseTracingHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.HttpClientTracingHandler;
@@ -90,6 +92,10 @@ public class NettyChannelPipelineInstrumentation
         ourHandler = new HttpClientRequestTracingHandler();
       } else if (handler instanceof HttpResponseDecoder) {
         ourHandler = new HttpClientResponseTracingHandler();
+        // the SslHandler lives in the netty-handler module, using class name comparison to avoid
+        // adding a dependency
+      } else if (handler.getClass().getName().equals("io.netty.handler.ssl.SslHandler")) {
+        ourHandler = new NettySslInstrumentationHandler(sslInstrumenter());
       }
 
       if (ourHandler != null) {

instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java

@@ -11,21 +11,26 @@ import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
 import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyClientInstrumenterFactory;
 import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyConnectionInstrumenter;
+import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettySslInstrumenter;
 
 public final class NettyClientSingletons {
 
   private static final boolean alwaysCreateConnectSpan =
       Config.get().getBoolean("otel.instrumentation.netty.always-create-connect-span", false);
+  private static final boolean sslTelemetryEnabled =
+      Config.get().getBoolean("otel.instrumentation.netty.ssl-telemetry.enabled", false);
 
   private static final Instrumenter<HttpRequestAndChannel, HttpResponse> INSTRUMENTER;
   private static final NettyConnectionInstrumenter CONNECTION_INSTRUMENTER;
+  private static final NettySslInstrumenter SSL_INSTRUMENTER;
 
   static {
     NettyClientInstrumenterFactory factory =
         new NettyClientInstrumenterFactory(
-            "io.opentelemetry.netty-4.0", alwaysCreateConnectSpan, false);
+            "io.opentelemetry.netty-4.0", alwaysCreateConnectSpan, sslTelemetryEnabled);
     INSTRUMENTER = factory.createHttpInstrumenter();
     CONNECTION_INSTRUMENTER = factory.createConnectionInstrumenter();
+    SSL_INSTRUMENTER = factory.createSslInstrumenter();
   }
 
   public static Instrumenter<HttpRequestAndChannel, HttpResponse> instrumenter() {
@@ -36,5 +41,9 @@ public final class NettyClientSingletons {
     return CONNECTION_INSTRUMENTER;
   }
 
+  public static NettySslInstrumenter sslInstrumenter() {
+    return SSL_INSTRUMENTER;
+  }
+
   private NettyClientSingletons() {}
 }

instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientSslTest.groovy

@@ -0,0 +1,215 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import io.netty.bootstrap.Bootstrap
+import io.netty.buffer.Unpooled
+import io.netty.channel.Channel
+import io.netty.channel.ChannelInitializer
+import io.netty.channel.ChannelPipeline
+import io.netty.channel.EventLoopGroup
+import io.netty.channel.nio.NioEventLoopGroup
+import io.netty.channel.socket.SocketChannel
+import io.netty.channel.socket.nio.NioSocketChannel
+import io.netty.handler.codec.http.DefaultFullHttpRequest
+import io.netty.handler.codec.http.HttpClientCodec
+import io.netty.handler.codec.http.HttpHeaders
+import io.netty.handler.codec.http.HttpMethod
+import io.netty.handler.codec.http.HttpVersion
+import io.netty.handler.ssl.SslHandler
+import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestServer
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
+import spock.lang.Shared
+
+import javax.net.ssl.SSLContext
+import javax.net.ssl.SSLHandshakeException
+import java.util.concurrent.CompletableFuture
+import java.util.concurrent.ExecutionException
+import java.util.concurrent.TimeUnit
+
+import static io.opentelemetry.api.trace.SpanKind.CLIENT
+import static io.opentelemetry.api.trace.SpanKind.INTERNAL
+import static io.opentelemetry.api.trace.SpanKind.SERVER
+import static io.opentelemetry.api.trace.StatusCode.ERROR
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
+
+class Netty40ClientSslTest extends AgentInstrumentationSpecification {
+
+  @Shared
+  HttpClientTestServer server
+  @Shared
+  EventLoopGroup eventLoopGroup
+
+  def setupSpec() {
+    server = new HttpClientTestServer(openTelemetry)
+    server.start()
+    eventLoopGroup = new NioEventLoopGroup()
+  }
+
+  def cleanupSpec() {
+    server.stop().get(10, TimeUnit.SECONDS)
+    eventLoopGroup.shutdownGracefully().sync()
+  }
+
+  def "should fail SSL handshake"() {
+    given:
+    def bootstrap = createBootstrap(eventLoopGroup, ["SSLv3"])
+
+    def uri = server.resolveHttpsAddress("/success")
+    def request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.toString(), Unpooled.EMPTY_BUFFER)
+    HttpHeaders.setHost(request, uri.host)
+
+    when:
+    Channel channel = null
+    runWithSpan("parent") {
+      channel = bootstrap.connect(uri.host, uri.port).sync().channel()
+      def result = new CompletableFuture<Integer>()
+      channel.pipeline().addLast(new ClientHandler(result))
+      channel.writeAndFlush(request).get(10, TimeUnit.SECONDS)
+      result.get(10, TimeUnit.SECONDS)
+    }
+
+    then:
+    Throwable thrownException = thrown()
+    if (thrownException instanceof ExecutionException) {
+      thrownException = thrownException.cause
+    }
+
+    assertTraces(1) {
+      trace(0, 3) {
+        span(0) {
+          name "parent"
+          status ERROR
+          errorEvent(thrownException.class, thrownException.message)
+        }
+        span(1) {
+          name "CONNECT"
+          kind INTERNAL
+          childOf span(0)
+          attributes {
+            "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
+            "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
+            "${SemanticAttributes.NET_PEER_PORT.key}" uri.port
+            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" }
+          }
+        }
+        span(2) {
+          name "SSL handshake"
+          kind INTERNAL
+          childOf span(0)
+          status ERROR
+          // netty swallows the exception, it doesn't make any sense to hard-code the message
+          errorEventWithAnyMessage(SSLHandshakeException)
+          attributes {
+            "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
+            "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
+            "${SemanticAttributes.NET_PEER_PORT.key}" uri.port
+            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" }
+          }
+        }
+      }
+    }
+
+    cleanup:
+    channel?.close()?.sync()
+  }
+
+  def "should successfully establish SSL handshake"() {
+    given:
+    def bootstrap = createBootstrap(eventLoopGroup, ["TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3"])
+
+    def uri = server.resolveHttpsAddress("/success")
+    def request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.toString(), Unpooled.EMPTY_BUFFER)
+    HttpHeaders.setHost(request, uri.host)
+
+    when:
+    Channel channel = null
+    runWithSpan("parent") {
+      channel = bootstrap.connect(uri.host, uri.port).sync().channel()
+      def result = new CompletableFuture<Integer>()
+      channel.pipeline().addLast(new ClientHandler(result))
+      channel.writeAndFlush(request).get(10, TimeUnit.SECONDS)
+      result.get(10, TimeUnit.SECONDS)
+    }
+
+    then:
+    assertTraces(1) {
+      trace(0, 5) {
+        span(0) {
+          name "parent"
+        }
+        span(1) {
+          name "CONNECT"
+          kind INTERNAL
+          childOf span(0)
+          attributes {
+            "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
+            "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
+            "${SemanticAttributes.NET_PEER_PORT.key}" uri.port
+            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" }
+          }
+        }
+        span(2) {
+          name "SSL handshake"
+          kind INTERNAL
+          childOf span(0)
+          attributes {
+            "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
+            "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
+            "${SemanticAttributes.NET_PEER_PORT.key}" uri.port
+            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" }
+          }
+        }
+        span(3) {
+          name "HTTP GET"
+          kind CLIENT
+          childOf(span(0))
+        }
+        span(4) {
+          name "test-http-server"
+          kind SERVER
+          childOf(span(3))
+        }
+      }
+    }
+
+    cleanup:
+    channel?.close()?.sync()
+  }
+
+  // list of default ciphers copied from netty's JdkSslContext
+  private static final String[] SUPPORTED_CIPHERS = [
+    "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
+    "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+    "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",
+    "TLS_RSA_WITH_AES_128_GCM_SHA256",
+    "TLS_RSA_WITH_AES_128_CBC_SHA",
+    "TLS_RSA_WITH_AES_256_CBC_SHA",
+    "SSL_RSA_WITH_3DES_EDE_CBC_SHA"
+  ]
+
+  private static Bootstrap createBootstrap(EventLoopGroup eventLoopGroup, List<String> enabledProtocols) {
+    def bootstrap = new Bootstrap()
+    bootstrap.group(eventLoopGroup)
+      .channel(NioSocketChannel)
+      .handler(new ChannelInitializer<SocketChannel>() {
+        @Override
+        protected void initChannel(SocketChannel socketChannel) throws Exception {
+          ChannelPipeline pipeline = socketChannel.pipeline()
+
+          def sslContext = SSLContext.getInstance("TLS")
+          sslContext.init(null, null, null)
+          def sslEngine = sslContext.createSSLEngine()
+          sslEngine.setUseClientMode(true)
+          sslEngine.setEnabledProtocols(enabledProtocols as String[])
+          sslEngine.setEnabledCipherSuites(SUPPORTED_CIPHERS)
+          pipeline.addLast(new SslHandler(sslEngine))
+
+          pipeline.addLast(new HttpClientCodec())
+        }
+      })
+    bootstrap
+  }
+}