From ddf4d5ca083283cb2ed7e9426d6731befc5577c0 Mon Sep 17 00:00:00 2001 From: vinodhabib Date: Tue, 18 Feb 2025 11:26:03 +0000 Subject: [PATCH 1/3] xds: added changes to fix the client cert leakage issue --- .../netty/InternalProtocolNegotiators.java | 28 ++++++++++++++++ .../io/grpc/xds/ClusterImplLoadBalancer.java | 4 +++ .../java/io/grpc/xds/XdsServerWrapper.java | 1 + .../security/SecurityProtocolNegotiators.java | 32 +++++++++++++++---- .../FileWatcherCertificateProvider.java | 3 ++ 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java b/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java index 3d1aa83d9ff..9f43db93e5f 100644 --- a/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java +++ b/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java @@ -75,6 +75,34 @@ public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslCo return tls(sslContext, null, Optional.absent()); } + /** + * Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will + * be negotiated, the {@code handler} is added and writes to the {@link io.netty.channel.Channel} + * may happen immediately, even before the TLS Handshake is complete. + */ + public static InternalProtocolNegotiator.ProtocolNegotiator ClientTls(SslContext sslContext) { + final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.serverTls(sslContext); + final class ClientTlsNegotiator implements InternalProtocolNegotiator.ProtocolNegotiator { + + @Override + public AsciiString scheme() { + return negotiator.scheme(); + } + + @Override + public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) { + return negotiator.newHandler(grpcHandler); + } + + @Override + public void close() { + negotiator.close(); + } + } + + return new ClientTlsNegotiator(); + } + /** * Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will be * negotiated, the server TLS {@code handler} is added and writes to the {@link diff --git a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java index fd4f49fbb83..b22b9873f64 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java @@ -101,6 +101,7 @@ final class ClusterImplLoadBalancer extends LoadBalancer { ClusterImplLoadBalancer(Helper helper) { this(helper, ThreadSafeRandomImpl.instance); + System.out.println("inside ClusterImplLoadBalancer constructor with 1 param"); } ClusterImplLoadBalancer(Helper helper, ThreadSafeRandom random) { @@ -109,6 +110,7 @@ final class ClusterImplLoadBalancer extends LoadBalancer { InternalLogId logId = InternalLogId.allocate("cluster-impl-lb", helper.getAuthority()); logger = XdsLogger.withLogId(logId); logger.log(XdsLogLevel.INFO, "Created"); + System.out.println("inside ClusterImplLoadBalancer constructor with 2 params"); } @Override @@ -175,6 +177,7 @@ public void requestConnection() { @Override public void shutdown() { + System.out.println("calling shutdown in ClusterImplLoadBalancer"); if (dropStats != null) { dropStats.release(); } @@ -346,6 +349,7 @@ private void updateMaxConcurrentRequests(@Nullable Long maxConcurrentRequests) { } private void updateSslContextProviderSupplier(@Nullable UpstreamTlsContext tlsContext) { + System.out.println("calling updateSslContextProviderSupplier in ClusterImplLoadBalancer"); UpstreamTlsContext currentTlsContext = sslContextProviderSupplier != null ? (UpstreamTlsContext)sslContextProviderSupplier.getTlsContext() diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java index 3a9b98ee321..42f04a2e049 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java @@ -445,6 +445,7 @@ public void onError(final Status error) { } private void shutdown() { + System.out.println("calling shutdown in XdsServerWrapper"); stopped = true; cleanUpRouteDiscoveryStates(); logger.log(Level.FINE, "Stop watching LDS resource {0}", resourceName); diff --git a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java index c34fab74032..11551302a98 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java @@ -134,6 +134,7 @@ public AsciiString scheme() { @Override public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) { + System.out.println("inside newHandler()"); // check if SslContextProviderSupplier was passed via attributes SslContextProviderSupplier localSslContextProviderSupplier = grpcHandler.getEagAttributes().get(ATTR_SSL_CONTEXT_PROVIDER_SUPPLIER); @@ -202,10 +203,15 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception { checkNotNull(grpcHandler, "grpcHandler"); this.grpcHandler = grpcHandler; this.sslContextProviderSupplier = sslContextProviderSupplier; + + //new Throwable().printStackTrace(); + //System.out.println("SecurityProtocolNegotiators.ClientSecurityHandler instance=" + this); } @Override protected void handlerAdded0(final ChannelHandlerContext ctx) { + System.out.println("inside ClientSecurityHandler handlerAdded0()"); + System.out.println("ctx.name=" + ctx.name() + "ctx.handler=" + ctx.handler()); final BufferReadsHandler bufferReads = new BufferReadsHandler(); ctx.pipeline().addBefore(ctx.name(), null, bufferReads); @@ -222,12 +228,14 @@ public void updateSslContext(SslContext sslContext) { "ClientSecurityHandler.updateSslContext authority={0}, ctx.name={1}", new Object[]{grpcHandler.getAuthority(), ctx.name()}); ChannelHandler handler = - InternalProtocolNegotiators.tls(sslContext).newHandler(grpcHandler); + InternalProtocolNegotiators.ClientTls(sslContext).newHandler(grpcHandler); // Delegate rest of handshake to TLS handler - ctx.pipeline().addAfter(ctx.name(), null, handler); - fireProtocolNegotiationEvent(ctx); - ctx.pipeline().remove(bufferReads); + //if(!ctx.isRemoved()) { + ctx.pipeline().addAfter(ctx.name(), null, handler); + fireProtocolNegotiationEvent(ctx); + ctx.pipeline().remove(bufferReads); + //} } @Override @@ -308,13 +316,22 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc ctx.fireUserEventTriggered(pne); return; } else { - ctx.pipeline() + if (ctx.name().contains("ClientSecurityHandler")) { + ctx.pipeline() + .replace( + this, + null, + new ClientSecurityHandler( + grpcHandler, sslContextProviderSupplier)); + } else { + ctx.pipeline() .replace( this, null, new ServerSecurityHandler( grpcHandler, sslContextProviderSupplier)); - ctx.fireUserEventTriggered(pne); + ctx.fireUserEventTriggered(pne); + } return; } } else { @@ -345,10 +362,13 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception { checkNotNull(grpcHandler, "grpcHandler"); this.grpcHandler = grpcHandler; this.sslContextProviderSupplier = sslContextProviderSupplier; + //new Throwable().printStackTrace(); + //System.out.println("SecurityProtocolNegotiators.ServerSecurityHandler instance=" + this); } @Override protected void handlerAdded0(final ChannelHandlerContext ctx) { + System.out.println("inside ServerSecurityHandler handlerAdded0()"); final BufferReadsHandler bufferReads = new BufferReadsHandler(); ctx.pipeline().addBefore(ctx.name(), null, bufferReads); diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java index 304124cc7f2..16bd21a3b45 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java @@ -90,6 +90,7 @@ final class FileWatcherCertificateProvider extends CertificateProvider implement @Override public void start() { scheduleNextRefreshCertificate(/* delayInSeconds= */0); + //System.out.println("Executed start()"); } @Override @@ -101,6 +102,8 @@ public synchronized void close() { scheduledFuture = null; } getWatcher().close(); + //System.out.println("FWCP close()=" + this); + System.out.println("Executed close()"); } private synchronized void scheduleNextRefreshCertificate(long delayInSeconds) { From fe3577067979b099f571407789c1a46f5c90fda4 Mon Sep 17 00:00:00 2001 From: vinodhabib Date: Wed, 19 Feb 2025 06:47:08 +0000 Subject: [PATCH 2/3] xds: added changes to fix the client cert leakage issue --- .../io/grpc/netty/InternalProtocolNegotiators.java | 2 +- .../security/SecurityProtocolNegotiators.java | 11 ++++++----- .../certprovider/FileWatcherCertificateProvider.java | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java b/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java index 9f43db93e5f..9619eb43141 100644 --- a/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java +++ b/netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java @@ -81,7 +81,7 @@ public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslCo * may happen immediately, even before the TLS Handshake is complete. */ public static InternalProtocolNegotiator.ProtocolNegotiator ClientTls(SslContext sslContext) { - final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.serverTls(sslContext); + final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.tls(sslContext); final class ClientTlsNegotiator implements InternalProtocolNegotiator.ProtocolNegotiator { @Override diff --git a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java index 11551302a98..033d9cbf51b 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java @@ -203,9 +203,8 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception { checkNotNull(grpcHandler, "grpcHandler"); this.grpcHandler = grpcHandler; this.sslContextProviderSupplier = sslContextProviderSupplier; - - //new Throwable().printStackTrace(); - //System.out.println("SecurityProtocolNegotiators.ClientSecurityHandler instance=" + this); + new Throwable().printStackTrace(); + System.out.println("SecurityProtocolNegotiators.ClientSecurityHandler instance=" + this); } @Override @@ -221,6 +220,7 @@ protected void handlerAdded0(final ChannelHandlerContext ctx) { @Override public void updateSslContext(SslContext sslContext) { if (ctx.isRemoved()) { + System.out.println("ctx.isRemoved() invoked"); return; } logger.log( @@ -235,6 +235,7 @@ public void updateSslContext(SslContext sslContext) { ctx.pipeline().addAfter(ctx.name(), null, handler); fireProtocolNegotiationEvent(ctx); ctx.pipeline().remove(bufferReads); + System.out.println("ClientSecurityHandler.updateSslContext invoked"); //} } @@ -362,8 +363,8 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception { checkNotNull(grpcHandler, "grpcHandler"); this.grpcHandler = grpcHandler; this.sslContextProviderSupplier = sslContextProviderSupplier; - //new Throwable().printStackTrace(); - //System.out.println("SecurityProtocolNegotiators.ServerSecurityHandler instance=" + this); + new Throwable().printStackTrace(); + System.out.println("SecurityProtocolNegotiators.ServerSecurityHandler instance=" + this); } @Override diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java index 16bd21a3b45..c6dd52fa2fb 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProvider.java @@ -90,7 +90,7 @@ final class FileWatcherCertificateProvider extends CertificateProvider implement @Override public void start() { scheduleNextRefreshCertificate(/* delayInSeconds= */0); - //System.out.println("Executed start()"); + System.out.println("Executed start()"); } @Override From 8b5877c9fb6313781293a1fa78a76c00bacaba52 Mon Sep 17 00:00:00 2001 From: vinodhabib Date: Wed, 19 Feb 2025 07:15:08 +0000 Subject: [PATCH 3/3] xds: added changes to fix the client cert leakage issue --- xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java | 2 -- .../xds/internal/security/SecurityProtocolNegotiators.java | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java index b22b9873f64..0b49bfaa4fb 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java @@ -101,7 +101,6 @@ final class ClusterImplLoadBalancer extends LoadBalancer { ClusterImplLoadBalancer(Helper helper) { this(helper, ThreadSafeRandomImpl.instance); - System.out.println("inside ClusterImplLoadBalancer constructor with 1 param"); } ClusterImplLoadBalancer(Helper helper, ThreadSafeRandom random) { @@ -110,7 +109,6 @@ final class ClusterImplLoadBalancer extends LoadBalancer { InternalLogId logId = InternalLogId.allocate("cluster-impl-lb", helper.getAuthority()); logger = XdsLogger.withLogId(logId); logger.log(XdsLogLevel.INFO, "Created"); - System.out.println("inside ClusterImplLoadBalancer constructor with 2 params"); } @Override diff --git a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java index 033d9cbf51b..f09aab0152f 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java @@ -324,6 +324,8 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc null, new ClientSecurityHandler( grpcHandler, sslContextProviderSupplier)); + ctx.fireUserEventTriggered(pne); + return; } else { ctx.pipeline() .replace( @@ -332,8 +334,8 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc new ServerSecurityHandler( grpcHandler, sslContextProviderSupplier)); ctx.fireUserEventTriggered(pne); + return; } - return; } } else { super.userEventTriggered(ctx, evt);