-
Notifications
You must be signed in to change notification settings - Fork 3.9k
xds: xDS based SNI setting and SAN validation #12378
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
Changes from all commits
ed5072e
c8be933
63997fd
6263cce
5e794bf
30ffa7b
42c9df0
f12bc61
e9c4e3c
dd8fa02
a371065
a6f1bc9
a576df0
ce1f2d0
5a4f758
4076998
968d564
4cf653d
6c1898a
5be2aa2
90abe55
4c44e4c
ae74a9e
199cc69
37cd044
14a91e7
6958b4e
139805e
4417fcc
7f48afa
d2b722a
e116552
acb8fa5
13200fa
e95725d
180f373
381beb2
18f5d5a
e18d6cd
b8dba99
2ecbdb9
3845e16
825b954
0ca4f8b
92f3182
2f5ba5d
2985cc3
011a9ea
b828098
c19a24f
5ba39b3
f135943
220e428
0d5eb0a
08391fb
d6acdfc
c14a488
733f57c
967fe8c
26733ab
9a817f8
107cbd8
8fa5a78
0034d9c
7219706
962df75
30cea9a
59fe14e
1c6066e
348a7a4
c6ca2d4
3f53682
16e4bde
d8544fd
232ca14
fa4e498
a6ee4b2
1f484a2
c89d51d
d3bd6e2
0724bc0
0d5c141
cd61ddb
ab11c79
aff4584
8ba6bfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,15 +102,6 @@ final class ProtocolNegotiators { | |
private static final EnumSet<TlsServerCredentials.Feature> understoodServerTlsFeatures = | ||
EnumSet.of( | ||
TlsServerCredentials.Feature.MTLS, TlsServerCredentials.Feature.CUSTOM_MANAGERS); | ||
private static Class<?> x509ExtendedTrustManagerClass; | ||
|
||
static { | ||
try { | ||
x509ExtendedTrustManagerClass = Class.forName("javax.net.ssl.X509ExtendedTrustManager"); | ||
} catch (ClassNotFoundException e) { | ||
// Will disallow per-rpc authority override via call option. | ||
} | ||
} | ||
|
||
private ProtocolNegotiators() { | ||
} | ||
|
@@ -147,15 +138,8 @@ public static FromChannelCredentialsResult from(ChannelCredentials creds) { | |
trustManagers = Arrays.asList(tmf.getTrustManagers()); | ||
} | ||
builder.trustManager(new FixedTrustManagerFactory(trustManagers)); | ||
TrustManager x509ExtendedTrustManager = null; | ||
if (x509ExtendedTrustManagerClass != null) { | ||
for (TrustManager trustManager : trustManagers) { | ||
if (x509ExtendedTrustManagerClass.isInstance(trustManager)) { | ||
x509ExtendedTrustManager = trustManager; | ||
break; | ||
} | ||
} | ||
} | ||
TrustManager x509ExtendedTrustManager = | ||
CertificateUtils.getX509ExtendedTrustManager(trustManagers); | ||
return FromChannelCredentialsResult.negotiator(tlsClientFactory(builder.build(), | ||
(X509TrustManager) x509ExtendedTrustManager)); | ||
} catch (SSLException | GeneralSecurityException ex) { | ||
|
@@ -579,20 +563,22 @@ static final class ClientTlsProtocolNegotiator implements ProtocolNegotiator { | |
|
||
public ClientTlsProtocolNegotiator(SslContext sslContext, | ||
ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable, | ||
X509TrustManager x509ExtendedTrustManager) { | ||
X509TrustManager x509ExtendedTrustManager, String sni) { | ||
this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext"); | ||
this.executorPool = executorPool; | ||
if (this.executorPool != null) { | ||
this.executor = this.executorPool.getObject(); | ||
} | ||
this.handshakeCompleteRunnable = handshakeCompleteRunnable; | ||
this.x509ExtendedTrustManager = x509ExtendedTrustManager; | ||
this.sni = sni; | ||
} | ||
|
||
private final SslContext sslContext; | ||
private final ObjectPool<? extends Executor> executorPool; | ||
private final Optional<Runnable> handshakeCompleteRunnable; | ||
private final X509TrustManager x509ExtendedTrustManager; | ||
private final String sni; | ||
private Executor executor; | ||
|
||
@Override | ||
|
@@ -604,9 +590,17 @@ public AsciiString scheme() { | |
public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) { | ||
ChannelHandler gnh = new GrpcNegotiationHandler(grpcHandler); | ||
ChannelLogger negotiationLogger = grpcHandler.getNegotiationLogger(); | ||
ChannelHandler cth = new ClientTlsHandler(gnh, sslContext, grpcHandler.getAuthority(), | ||
this.executor, negotiationLogger, handshakeCompleteRunnable, this, | ||
x509ExtendedTrustManager); | ||
String authority; | ||
if ("".equals(sni)) { | ||
authority = null; | ||
} else if (sni != null) { | ||
authority = sni; | ||
} else { | ||
authority = grpcHandler.getAuthority(); | ||
} | ||
ChannelHandler cth = new ClientTlsHandler(gnh, sslContext, | ||
authority, this.executor, negotiationLogger, handshakeCompleteRunnable, this, | ||
x509ExtendedTrustManager); | ||
return new WaitUntilActiveHandler(cth, negotiationLogger); | ||
} | ||
|
||
|
@@ -630,28 +624,40 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler { | |
private final int port; | ||
private Executor executor; | ||
private final Optional<Runnable> handshakeCompleteRunnable; | ||
private final X509TrustManager x509ExtendedTrustManager; | ||
private final X509TrustManager x509TrustManager; | ||
ejona86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private SSLEngine sslEngine; | ||
|
||
ClientTlsHandler(ChannelHandler next, SslContext sslContext, String authority, | ||
Executor executor, ChannelLogger negotiationLogger, | ||
Optional<Runnable> handshakeCompleteRunnable, | ||
ClientTlsProtocolNegotiator clientTlsProtocolNegotiator, | ||
X509TrustManager x509ExtendedTrustManager) { | ||
X509TrustManager x509TrustManager) { | ||
super(next, negotiationLogger); | ||
this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext"); | ||
HostPort hostPort = parseAuthority(authority); | ||
this.host = hostPort.host; | ||
this.port = hostPort.port; | ||
// TODO: For empty authority and fallback flag | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Half-baked thought (I haven't double checked with the expected behavior): it seems we should just pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could either pass empty SNI here and change it to null when fallback flag is set (which is what I was going to do), or we could do that in the calling site at ClientTlsProtocolNegotiator itself and pass a null SNI in that case. The bigger question is at the time of SAN validation do we get the xDS channel authority from the SsllParameters.getServerNames(), and if so we should not use it for the SAN validation, and that would require setting some state in the |
||
// GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE present, we should parse authority | ||
// but prevent it from being used for SAN validation in the TrustManager. | ||
if (authority != null) { | ||
HostPort hostPort = parseAuthority(authority); | ||
this.host = hostPort.host; | ||
this.port = hostPort.port; | ||
} else { | ||
this.host = null; | ||
this.port = 0; | ||
} | ||
this.executor = executor; | ||
this.handshakeCompleteRunnable = handshakeCompleteRunnable; | ||
this.x509ExtendedTrustManager = x509ExtendedTrustManager; | ||
this.x509TrustManager = x509TrustManager; | ||
} | ||
|
||
@Override | ||
@IgnoreJRERequirement | ||
protected void handlerAdded0(ChannelHandlerContext ctx) { | ||
sslEngine = sslContext.newEngine(ctx.alloc(), host, port); | ||
if (host != null) { | ||
sslEngine = sslContext.newEngine(ctx.alloc(), host, port); | ||
} else { | ||
sslEngine = sslContext.newEngine(ctx.alloc()); | ||
} | ||
SSLParameters sslParams = sslEngine.getSSLParameters(); | ||
sslParams.setEndpointIdentificationAlgorithm("HTTPS"); | ||
sslEngine.setSSLParameters(sslParams); | ||
|
@@ -709,7 +715,7 @@ private void propagateTlsComplete(ChannelHandlerContext ctx, SSLSession session) | |
.set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.PRIVACY_AND_INTEGRITY) | ||
.set(Grpc.TRANSPORT_ATTR_SSL_SESSION, session) | ||
.set(GrpcAttributes.ATTR_AUTHORITY_VERIFIER, new X509AuthorityVerifier( | ||
sslEngine, x509ExtendedTrustManager)) | ||
sslEngine, x509TrustManager)) | ||
.build(); | ||
replaceProtocolNegotiationEvent(existingPne.withAttributes(attrs).withSecurity(security)); | ||
if (handshakeCompleteRunnable.isPresent()) { | ||
|
@@ -746,13 +752,14 @@ static HostPort parseAuthority(String authority) { | |
* 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. | ||
* | ||
* @param executorPool a dedicated {@link Executor} pool for time-consuming TLS tasks | ||
*/ | ||
public static ProtocolNegotiator tls(SslContext sslContext, | ||
ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable, | ||
X509TrustManager x509ExtendedTrustManager) { | ||
X509TrustManager x509ExtendedTrustManager, String sni) { | ||
return new ClientTlsProtocolNegotiator(sslContext, executorPool, handshakeCompleteRunnable, | ||
x509ExtendedTrustManager); | ||
x509ExtendedTrustManager, sni); | ||
} | ||
|
||
/** | ||
|
@@ -762,7 +769,7 @@ public static ProtocolNegotiator tls(SslContext sslContext, | |
*/ | ||
public static ProtocolNegotiator tls(SslContext sslContext, | ||
X509TrustManager x509ExtendedTrustManager) { | ||
return tls(sslContext, null, Optional.absent(), x509ExtendedTrustManager); | ||
return tls(sslContext, null, Optional.absent(), x509ExtendedTrustManager, null); | ||
} | ||
|
||
public static ProtocolNegotiator.ClientFactory tlsClientFactory(SslContext sslContext, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not an api breaking change? Should we do function overloading here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in an
Internal*
class, so we are free to make the change. Using overloads is convenient to avoid breaking usages. In this case I expect Kannan thought he was updating all usages (because the usages themselves were the cause of adding the new argument).FWIW, I also contacted security team and they're looking at deleting the unused internal copy of s2a.