-
Notifications
You must be signed in to change notification settings - Fork 3.8k
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
grpclb with TLS sends a malformed SNI host name to the balancer #4912
Comments
Doesn't the SNI hostname need to match the authority exactly? |
My off the cuff guess is that the authority needs to change to drop the trailing period as well. Just note per RFC 6066 on SNI, "The hostname is represented as a byte string using ASCII encoding without a trailing dot." |
For completeness, do we know where the trailing dot is coming from? This seems like a bug in either Java or netty-tcnative, since the SNI spec clearly dictates it shouldn't include the trailing dot, whereas normal hostnames may include the dot. Technically, I'd expect the HTTP/2 authority is likely to continue to still have the dot. The SNI name needs to be fully-qualified while the HTTP/2 authority may be abbreviated. HTTP/2 says to use 'authority' from RFC 3986. In §3.2.2:
But I'd also expect many similar incompatibilities in this realm of things, so maybe in practice it shouldn't have the trailing dot. Although a bit tangential, HTTP/1.1's RFC 7230 §5.4 says that "Host" should be "identical" to the URL's authority:
|
I see now. The dot comes from DNS itself in the SRV record. |
Yes for comment above. Also FWIW, I suspect this may be related to the fact that Go/C++ clients are able to open top-level SSL channels to targets with trailing periods, but Java can't (experimentally, opening a Java interop client SSL channel to a target with a trailing period also fails in the same way). |
I've confirmed that Go includes the trailing dot in tcnative: sends faulty SNI with trailing dot So we should strip the trailing dot just before passing it to SSL. And we may want to strip the trailing dot from the SRV response. |
The trailing dot denotes the hostname to be absolute. It is fine to leave, but removing it makes the authority match the more common form and hopefully reduces confusion. This happens to works around SNI failures caused when using gRPC-LB, since SNI prohibits the trailing dot. However, that is not the reason for this change as we have to support users directly providing a hostname with the trailing dot anyway (and doing so is not hard). See grpc#4912
The trailing dot denotes the hostname to be absolute. It is fine to leave, but removing it makes the authority match the more common form and hopefully reduces confusion. This happens to works around SNI failures caused when using gRPC-LB, since SNI prohibits the trailing dot. However, that is not the reason for this change as we have to support users directly providing a hostname with the trailing dot anyway (and doing so is not hard). See #4912
In grpc/grpc#16727, there are several scenarios in which there is a grpclb balancer using plain TLS creds. The client is expected to find the balancer, get a backend, and complete an RPC to the backend all over plain TLS. Go and C++ pass this test but Java doesn't.
It appears that the root cause is that grpc-java includes the trailing period in it's host name passed to SNI. I dumped the SSL handshake between the java client and the balancer, and the java client sends an initial
ClientHello
with an SNI extension having a host name ofbalancer.test.google.fr.
(note the trailing period). This is unlike C++ and Go, which don't include the trailing period in the SNI hostname. The balancer SSL handshaker responds by aborting the handshake with anssl_unexpected_message
alert. Since trailing periods are invalid by SNI spec, I think that the java client needs to strip any trailing periods of resolved balancer names in SRV records. For example, java passes the "grpclb with plain TLS" test with this hackThe text was updated successfully, but these errors were encountered: