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

xds: added changes to fix the client cert leakage issue #11909

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 28 additions & 0 deletions netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.tls(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
Expand Down
2 changes: 2 additions & 0 deletions xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ public void requestConnection() {

@Override
public void shutdown() {
System.out.println("calling shutdown in ClusterImplLoadBalancer");
if (dropStats != null) {
dropStats.release();
}
Expand Down Expand Up @@ -346,6 +347,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()
Expand Down
1 change: 1 addition & 0 deletions xds/src/main/java/io/grpc/xds/XdsServerWrapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -202,10 +203,14 @@ 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);

Expand All @@ -215,19 +220,23 @@ protected void handlerAdded0(final ChannelHandlerContext ctx) {
@Override
public void updateSslContext(SslContext sslContext) {
if (ctx.isRemoved()) {
System.out.println("ctx.isRemoved() invoked");
return;
}
logger.log(
Level.FINEST,
"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);
System.out.println("ClientSecurityHandler.updateSslContext invoked");
//}
}

@Override
Expand Down Expand Up @@ -308,14 +317,25 @@ 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));
ctx.fireUserEventTriggered(pne);
return;
} else {
ctx.pipeline()
.replace(
this,
null,
new ServerSecurityHandler(
grpcHandler, sslContextProviderSupplier));
ctx.fireUserEventTriggered(pne);
return;
ctx.fireUserEventTriggered(pne);
return;
}
}
} else {
super.userEventTriggered(ctx, evt);
Expand Down Expand Up @@ -345,10 +365,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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ final class FileWatcherCertificateProvider extends CertificateProvider implement
@Override
public void start() {
scheduleNextRefreshCertificate(/* delayInSeconds= */0);
System.out.println("Executed start()");
}

@Override
Expand All @@ -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) {
Expand Down
Loading