Skip to content

Commit 962df75

Browse files
kannanjgithubkannanjgithub
committed
Fix plumbings.
1 parent 7219706 commit 962df75

25 files changed

+1078
-594
lines changed

‎api/src/main/java/io/grpc/EquivalentAddressGroup.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ public final class EquivalentAddressGroup {
5555
*/
5656
public static final Attributes.Key<String> ATTR_LOCALITY_NAME =
5757
Attributes.Key.create("io.grpc.EquivalentAddressGroup.LOCALITY");
58-
5958
private final List<SocketAddress> addrs;
6059
private final Attributes attrs;
6160

‎netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ private InternalProtocolNegotiators() {}
4040
* Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will
4141
* be negotiated, the {@code handler} is added and writes to the {@link io.netty.channel.Channel}
4242
* may happen immediately, even before the TLS Handshake is complete.
43-
*
4443
* @param executorPool a dedicated {@link Executor} pool for time-consuming TLS tasks
4544
*/
4645
public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslContext,
@@ -67,7 +66,6 @@ public void close() {
6766
negotiator.close();
6867
}
6968
}
70-
7169
return new TlsNegotiator();
7270
}
7371

‎netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ public static FromChannelCredentialsResult from(ChannelCredentials creds) {
131131
trustManagers = tlsCreds.getTrustManagers();
132132
} else if (tlsCreds.getRootCertificates() != null) {
133133
trustManagers = Arrays.asList(CertificateUtils.createTrustManager(
134-
new ByteArrayInputStream(tlsCreds.getRootCertificates())));
134+
new ByteArrayInputStream(tlsCreds.getRootCertificates())));
135135
} else { // else use system default
136136
TrustManagerFactory tmf = TrustManagerFactory.getInstance(
137137
TrustManagerFactory.getDefaultAlgorithm());
@@ -142,7 +142,7 @@ public static FromChannelCredentialsResult from(ChannelCredentials creds) {
142142
TrustManager x509ExtendedTrustManager =
143143
CertificateUtils.getX509ExtendedTrustManager(trustManagers);
144144
return FromChannelCredentialsResult.negotiator(tlsClientFactory(builder.build(),
145-
(X509TrustManager) x509ExtendedTrustManager));
145+
(X509TrustManager) x509ExtendedTrustManager));
146146
} catch (SSLException | GeneralSecurityException ex) {
147147
log.log(Level.FINE, "Exception building SslContext", ex);
148148
return FromChannelCredentialsResult.error(
@@ -458,7 +458,7 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
458458
}
459459
SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
460460
if (!sslContext.applicationProtocolNegotiator().protocols().contains(
461-
sslHandler.applicationProtocol())) {
461+
sslHandler.applicationProtocol())) {
462462
logSslEngineDetails(Level.FINE, ctx, "TLS negotiation failed for new client.", null);
463463
ctx.fireExceptionCaught(unavailableException(
464464
"Failed protocol negotiation: Unable to find compatible protocol"));
@@ -618,18 +618,21 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler {
618618
private final int port;
619619
private Executor executor;
620620
private final Optional<Runnable> handshakeCompleteRunnable;
621-
private final X509TrustManager x509ExtendedTrustManager;
621+
private final X509TrustManager x509TrustManager;
622622
private SSLEngine sslEngine;
623623

624-
ClientTlsHandler(ChannelHandler next, SslContext sslContext, String sniHostPort,
624+
ClientTlsHandler(ChannelHandler next, SslContext sslContext, String authority,
625625
Executor executor, ChannelLogger negotiationLogger,
626626
Optional<Runnable> handshakeCompleteRunnable,
627627
ClientTlsProtocolNegotiator clientTlsProtocolNegotiator,
628-
X509TrustManager x509ExtendedTrustManager) {
628+
X509TrustManager x509TrustManager) {
629629
super(next, negotiationLogger);
630630
this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext");
631-
if (!Strings.isNullOrEmpty(sniHostPort)) {
632-
HostPort hostPort = parseAuthority(sniHostPort);
631+
// TODO: For empty authority and fallback flag
632+
// GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE present, we should parse authority
633+
// but prevent it from being used for SAN validation in the TrustManager.
634+
if (authority != null && !authority.isEmpty()) {
635+
HostPort hostPort = parseAuthority(authority);
633636
this.host = hostPort.host;
634637
this.port = hostPort.port;
635638
} else {
@@ -638,7 +641,7 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler {
638641
}
639642
this.executor = executor;
640643
this.handshakeCompleteRunnable = handshakeCompleteRunnable;
641-
this.x509ExtendedTrustManager = x509ExtendedTrustManager;
644+
this.x509TrustManager = x509TrustManager;
642645
}
643646

644647
@Override
@@ -706,7 +709,7 @@ private void propagateTlsComplete(ChannelHandlerContext ctx, SSLSession session)
706709
.set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.PRIVACY_AND_INTEGRITY)
707710
.set(Grpc.TRANSPORT_ATTR_SSL_SESSION, session)
708711
.set(GrpcAttributes.ATTR_AUTHORITY_VERIFIER, new X509AuthorityVerifier(
709-
sslEngine, x509ExtendedTrustManager))
712+
sslEngine, x509TrustManager))
710713
.build();
711714
replaceProtocolNegotiationEvent(existingPne.withAttributes(attrs).withSecurity(security));
712715
if (handshakeCompleteRunnable.isPresent()) {
@@ -1058,8 +1061,8 @@ static final class PlaintextHandler extends ProtocolNegotiationHandler {
10581061
protected void protocolNegotiationEventTriggered(ChannelHandlerContext ctx) {
10591062
ProtocolNegotiationEvent existingPne = getProtocolNegotiationEvent();
10601063
Attributes attrs = existingPne.getAttributes().toBuilder()
1061-
.set(GrpcAttributes.ATTR_AUTHORITY_VERIFIER, (authority) -> Status.OK)
1062-
.build();
1064+
.set(GrpcAttributes.ATTR_AUTHORITY_VERIFIER, (authority) -> Status.OK)
1065+
.build();
10631066
replaceProtocolNegotiationEvent(existingPne.withAttributes(attrs));
10641067
fireProtocolNegotiationEvent(ctx);
10651068
}
@@ -1219,4 +1222,4 @@ public String getPeerHost() {
12191222
return peerHost;
12201223
}
12211224
}
1222-
}
1225+
}

‎netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -918,7 +918,7 @@ public String applicationProtocol() {
918918

919919
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, sslContext,
920920
"authority", elg, noopLogger, Optional.absent(),
921-
null, null);
921+
getClientTlsProtocolNegotiator(), null);
922922
pipeline.addLast(handler);
923923
pipeline.replace(SslHandler.class, null, goodSslHandler);
924924
pipeline.fireUserEventTriggered(ProtocolNegotiationEvent.DEFAULT);
@@ -957,7 +957,7 @@ public String applicationProtocol() {
957957

958958
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, sslContext,
959959
"authority", elg, noopLogger, Optional.absent(),
960-
null, null);
960+
getClientTlsProtocolNegotiator(), null);
961961
pipeline.addLast(handler);
962962
pipeline.replace(SslHandler.class, null, goodSslHandler);
963963
pipeline.fireUserEventTriggered(ProtocolNegotiationEvent.DEFAULT);
@@ -982,7 +982,7 @@ public String applicationProtocol() {
982982

983983
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, sslContext,
984984
"authority", elg, noopLogger, Optional.absent(),
985-
null, null);
985+
getClientTlsProtocolNegotiator(), null);
986986
pipeline.addLast(handler);
987987

988988
final AtomicReference<Throwable> error = new AtomicReference<>();
@@ -1011,7 +1011,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
10111011
public void clientTlsHandler_closeDuringNegotiation() throws Exception {
10121012
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, sslContext,
10131013
"authority", null, noopLogger, Optional.absent(),
1014-
null, null);
1014+
getClientTlsProtocolNegotiator(), null);
10151015
pipeline.addLast(new WriteBufferingAndExceptionHandler(handler));
10161016
ChannelFuture pendingWrite = channel.writeAndFlush(NettyClientHandler.NOOP_MESSAGE);
10171017

@@ -1023,6 +1023,12 @@ public void clientTlsHandler_closeDuringNegotiation() throws Exception {
10231023
.isEqualTo(Status.Code.UNAVAILABLE);
10241024
}
10251025

1026+
private ClientTlsProtocolNegotiator getClientTlsProtocolNegotiator() throws SSLException {
1027+
return new ClientTlsProtocolNegotiator(GrpcSslContexts.forClient().trustManager(
1028+
TlsTesting.loadCert("ca.pem")).build(),
1029+
null, Optional.absent(), null, "");
1030+
}
1031+
10261032
@Test
10271033
public void engineLog() {
10281034
ChannelHandler handler = new ServerTlsHandler(grpcHandler, sslContext, null);

‎xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,8 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
153153
childSwitchLb.handleResolvedAddresses(
154154
resolvedAddresses.toBuilder()
155155
.setAttributes(attributes.toBuilder()
156-
.set(NameResolver.ATTR_BACKEND_SERVICE, cluster)
157-
.build())
156+
.set(NameResolver.ATTR_BACKEND_SERVICE, cluster)
157+
.build())
158158
.setLoadBalancingPolicyConfig(config.childConfig)
159159
.build());
160160
return Status.OK;
@@ -321,7 +321,7 @@ private ClusterLocality createClusterLocalityFromAttributes(Attributes addressAt
321321
(lrsServerInfo == null)
322322
? null
323323
: xdsClient.addClusterLocalityStats(lrsServerInfo, cluster,
324-
edsServiceName, locality);
324+
edsServiceName, locality);
325325

326326
return new ClusterLocality(localityStats, localityName);
327327
}
@@ -363,7 +363,7 @@ private void updateSslContextProviderSupplier(@Nullable UpstreamTlsContext tlsCo
363363
sslContextProviderSupplier =
364364
tlsContext != null
365365
? new SslContextProviderSupplier(tlsContext,
366-
(TlsContextManager) xdsClient.getSecurityConfig())
366+
(TlsContextManager) xdsClient.getSecurityConfig())
367367
: null;
368368
}
369369

@@ -378,9 +378,9 @@ private class RequestLimitingSubchannelPicker extends SubchannelPicker {
378378
private final Map<String, Struct> filterMetadata;
379379

380380
private RequestLimitingSubchannelPicker(SubchannelPicker delegate,
381-
List<DropOverload> dropPolicies,
382-
long maxConcurrentRequests,
383-
Map<String, Struct> filterMetadata) {
381+
List<DropOverload> dropPolicies,
382+
long maxConcurrentRequests,
383+
Map<String, Struct> filterMetadata) {
384384
this.delegate = delegate;
385385
this.dropPolicies = dropPolicies;
386386
this.maxConcurrentRequests = maxConcurrentRequests;
@@ -544,4 +544,4 @@ void release() {
544544
}
545545
}
546546
}
547-
}
547+
}

‎xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,7 @@ public UpstreamTlsContext(
9898
public static UpstreamTlsContext fromEnvoyProtoUpstreamTlsContext(
9999
io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext
100100
upstreamTlsContext) {
101-
UpstreamTlsContext o = new UpstreamTlsContext(upstreamTlsContext);
102-
return o;
101+
return new UpstreamTlsContext(upstreamTlsContext);
103102
}
104103

105104
public String getSni() {
@@ -118,7 +117,7 @@ public boolean getAutoSniSanValidation() {
118117
public String toString() {
119118
return "UpstreamTlsContext{"
120119
+ "commonTlsContext=" + commonTlsContext
121-
+ "sni=" + sni
120+
+ "\nsni=" + sni
122121
+ "\nauto_host_sni=" + autoHostSni
123122
+ "\nauto_sni_san_validation=" + autoSniSanValidation
124123
+ "}";

‎xds/src/main/java/io/grpc/xds/internal/security/ClientSslContextProviderFactory.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,9 @@ final class ClientSslContextProviderFactory
4242

4343
/** Creates an SslContextProvider from the given UpstreamTlsContext. */
4444
@Override
45-
public SslContextProvider create(UpstreamTlsContext key) {
45+
public SslContextProvider create(UpstreamTlsContext upstreamTlsContext) {
4646
return certProviderClientSslContextProviderFactory.getProvider(
47-
key,
47+
upstreamTlsContext,
4848
bootstrapInfo.node().toEnvoyProtoNode(),
4949
bootstrapInfo.certProviders());
5050
}

‎xds/src/main/java/io/grpc/xds/internal/security/DynamicSslContextProvider.java‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public abstract class DynamicSslContextProvider extends SslContextProvider {
4343
protected final List<Callback> pendingCallbacks = new ArrayList<>();
4444
@Nullable protected final CertificateValidationContext staticCertificateValidationContext;
4545
@Nullable protected AbstractMap.SimpleImmutableEntry<SslContext, TrustManager>
46-
sslContextAndExtendedX509TrustManager;
46+
sslContextAndTrustManager;
4747

4848
protected DynamicSslContextProvider(
4949
BaseTlsContext tlsContext, CertificateValidationContext staticCertValidationContext) {
@@ -53,17 +53,15 @@ protected DynamicSslContextProvider(
5353

5454
@Nullable
5555
public AbstractMap.SimpleImmutableEntry<SslContext, TrustManager>
56-
getSslContextAndExtendedX509TrustManager() {
57-
return sslContextAndExtendedX509TrustManager;
56+
getSslContextAndTrustManager() {
57+
return sslContextAndTrustManager;
5858
}
5959

6060
protected abstract CertificateValidationContext generateCertificateValidationContext();
6161

62-
/**
63-
* Gets a server or client side SslContextBuilder.
64-
*/
62+
/** Gets a server or client side SslContextBuilder. */
6563
protected abstract AbstractMap.SimpleImmutableEntry<SslContextBuilder, TrustManager>
66-
getSslContextBuilderAndExtendedX509TrustManager(
64+
getSslContextBuilderAndTrustManager(
6765
CertificateValidationContext certificateValidationContext)
6866
throws CertificateException, IOException, CertStoreException;
6967

@@ -73,7 +71,7 @@ protected final void updateSslContext() {
7371
CertificateValidationContext localCertValidationContext =
7472
generateCertificateValidationContext();
7573
AbstractMap.SimpleImmutableEntry<SslContextBuilder, TrustManager> sslContextBuilderAndTm =
76-
getSslContextBuilderAndExtendedX509TrustManager(localCertValidationContext);
74+
getSslContextBuilderAndTrustManager(localCertValidationContext);
7775
CommonTlsContext commonTlsContext = getCommonTlsContext();
7876
if (commonTlsContext != null && commonTlsContext.getAlpnProtocolsCount() > 0) {
7977
List<String> alpnList = commonTlsContext.getAlpnProtocolsList();
@@ -89,9 +87,9 @@ protected final void updateSslContext() {
8987
AbstractMap.SimpleImmutableEntry<SslContext, TrustManager>
9088
sslContextAndExtendedX09TrustManagerCopy;
9189
synchronized (pendingCallbacks) {
92-
sslContextAndExtendedX509TrustManager = new AbstractMap.SimpleImmutableEntry<>(
90+
sslContextAndTrustManager = new AbstractMap.SimpleImmutableEntry<>(
9391
sslContextBuilderAndTm.getKey().build(), sslContextBuilderAndTm.getValue());
94-
sslContextAndExtendedX09TrustManagerCopy = sslContextAndExtendedX509TrustManager;
92+
sslContextAndExtendedX09TrustManagerCopy = sslContextAndTrustManager;
9593
pendingCallbacksCopy = clonePendingCallbacksAndClear();
9694
}
9795
makePendingCallbacks(sslContextAndExtendedX09TrustManagerCopy, pendingCallbacksCopy);
@@ -121,8 +119,8 @@ public final void addCallback(Callback callback) {
121119
// if there is a computed sslContext just send it
122120
AbstractMap.SimpleImmutableEntry<SslContext, TrustManager> sslContextCopy = null;
123121
synchronized (pendingCallbacks) {
124-
if (sslContextAndExtendedX509TrustManager != null) {
125-
sslContextCopy = sslContextAndExtendedX509TrustManager;
122+
if (sslContextAndTrustManager != null) {
123+
sslContextCopy = sslContextAndTrustManager;
126124
} else {
127125
pendingCallbacks.add(callback);
128126
}

‎xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,8 @@ public void updateSslContextAndExtendedX509TrustManager(
261261
public void onException(Throwable throwable) {
262262
ctx.fireExceptionCaught(throwable);
263263
}
264-
});
264+
}
265+
);
265266
}
266267

267268
@Override
@@ -351,13 +352,13 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
351352

352353
@VisibleForTesting
353354
static final class ServerSecurityHandler
354-
extends InternalProtocolNegotiators.ProtocolNegotiationHandler {
355+
extends InternalProtocolNegotiators.ProtocolNegotiationHandler {
355356
private final GrpcHttp2ConnectionHandler grpcHandler;
356357
private final SslContextProviderSupplier sslContextProviderSupplier;
357358

358359
ServerSecurityHandler(
359-
GrpcHttp2ConnectionHandler grpcHandler,
360-
SslContextProviderSupplier sslContextProviderSupplier) {
360+
GrpcHttp2ConnectionHandler grpcHandler,
361+
SslContextProviderSupplier sslContextProviderSupplier) {
361362
super(
362363
// superclass (InternalProtocolNegotiators.ProtocolNegotiationHandler) expects 'next'
363364
// handler but we don't have a next handler _yet_. So we "disable" superclass's behavior
@@ -399,7 +400,8 @@ public void updateSslContextAndExtendedX509TrustManager(
399400
public void onException(Throwable throwable) {
400401
ctx.fireExceptionCaught(throwable);
401402
}
402-
});
403+
}
404+
);
403405
}
404406
}
405-
}
407+
}

‎xds/src/main/java/io/grpc/xds/internal/security/SslContextProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public UpstreamTlsContext getUpstreamTlsContext() {
114114
public abstract void addCallback(Callback callback);
115115

116116
protected final void performCallback(
117-
final SslContextGetter sslContextGetter, final Callback callback) {
117+
final SslContextGetter sslContextGetter, final Callback callback) {
118118
checkNotNull(sslContextGetter, "sslContextGetter");
119119
checkNotNull(callback, "callback");
120120
callback.executor.execute(

0 commit comments

Comments
 (0)