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

Is there a way to configure NettyServerBuilder with SniHandler? #7397

Closed
theangrydev opened this issue Sep 4, 2020 · 19 comments
Closed

Is there a way to configure NettyServerBuilder with SniHandler? #7397

theangrydev opened this issue Sep 4, 2020 · 19 comments
Labels

Comments

@theangrydev
Copy link

Is it possible to configure io.grpc.netty.NettyServerBuilder with io.netty.handler.ssl.SniHandler for Server Name Indication capabilities of a gRPC server?

I would like to use SNI to switch the server certificates used based on the host name.

@theangrydev
Copy link
Author

As far as I can tell, this is not possible at the moment.

Here are my notes on what I think would need to be implemented to support SNI:

  • io.grpc.netty.ProtocolNegotiators.ServerTlsHandler to allow configuring io.netty.handler.ssl.SniHandler instead of io.netty.handler.ssl.SslHandler
  • io.grpc.netty.ProtocolNegotiators.ClientTlsProtocolNegotiator to set the override authority host sslParams.setServerNames(Collections.singletonList(new SNIHostName(host)));

@dapengzhang0
Copy link
Member

Here are my notes on what I think would need to be implemented to support SNI:
io.grpc.netty.ProtocolNegotiators.ServerTlsHandler to allow configuring io.netty.handler.ssl.SniHandler instead of io.netty.handler.ssl.SslHandler
io.grpc.netty.ProtocolNegotiators.ClientTlsProtocolNegotiator to set the override authority host sslParams.setServerNames(Collections.singletonList(new SNIHostName(host)));

That's right, you would need the @Internal API NettyServerBuilder.protocolNegotiator() (Unfortunately we didn't have InternalNettyServerBuilder.setProtocolNegotiatorFactory() API for the server) and the @Internal API InternalNettyChannelBuilder.setProtocolNegotiatorFactory().

Want to contribute?

@theangrydev
Copy link
Author

Want to contribute?

Possibly, I have a hacky patch sufficient for my needs working locally.

I think I would like to see the NettyServerBuilder have an option sniContexts(Mapping<String, SslContext> sslContextByHost) that would supply the certificate mappings and enable server SNI.

Similarly, NettyChannelBuilder would have an option enableSni() or maybe overrideAuthority(String authority, boolean sendSniHost) to enable client SNI.

@dapengzhang0 let me know what you think about some appropriate builder API changes and I'll contribute the changes at some point.

If someone else finds they need this, just leave a comment here and I would be happy to contribute it sooner.

@dapengzhang0
Copy link
Member

cc @ejona86

@ejona86
Copy link
Member

ejona86 commented Oct 29, 2020

I'm not all that impressed with SniHandler. I understand the need, but I don't think we'd want to use it. It is really most useful if you need to asynchronously determine the server certificate, like if you were going to generate them on-the-fly. But that's not all that common and based on the API you suggested, isn't something you are looking for.

In the normal case, this is really the job of an X509ExtendedKeyManager, as the SSLEngine has the SSLParameters which has the SNIMatchers. So I suggest creating a SslContext whose KeyManager is configured observe SNI.

I agree there are ways to make that easier. It could deserve a convenience implementation. But "this is just how Java does it" and it's really a separate issue than the API we provide.

@theangrydev
Copy link
Author

@ejona86 thanks for taking the time to look into this.

My understanding is that your suggestion would work as follows.

The server side SslContext would have an SSLEngine configured with a X509ExtendedKeyManager that would inspect the ExtendedSSLSession.getRequestedServerNames() to find the SNIServerName requested by the client. This would be used to determine which alias to respond with from the keystore.

The client side SslContext would have an SSLEngine configured with the SSLParameters.setServerNames(SNIHostName) that would be sent to the server.

A major limitation of this approach is that the server must choose an alias from a single SslContext.

My use case involves starting with the equivalent of a Map<SNIHostName, SslContext> known at startup time of the server. It sounds like I would need to collapse this map into a single SslContext that uses aliases to distinguish by host name.

I can't see an obvious way to achieve the last point, where I need to construct a single SslContext that has multiple aliases.

Do you have any words of advice for this last point?

@theangrydev
Copy link
Author

theangrydev commented Oct 30, 2020

Given that both io.netty.handler.ssl.SslContext and io.netty.handler.ssl.SniHandler live in the same Netty package, it looks to me that the Netty folks are using the SniHandler rather than customizing the SSLEngine to support SNI and they do not seem to provide a default way to configure an SslContext with multiple aliases.

Since grpc-netty is using SslContext, it looks like it is going against the grain to try to avoid using an SniHandler since that is the approach they provide support for.

@theangrydev
Copy link
Author

theangrydev commented Oct 30, 2020

I think the current conclusion is that grpc-netty does not support SNI at all since it is not possible to configure multiple aliases.

@ejona86
Copy link
Member

ejona86 commented Oct 31, 2020

The client side SslContext would have an SSLEngine configured with the SSLParameters.setServerNames(SNIHostName) that would be sent to the server.

That's already handled. Lots of modern Internet infrastructure would break without the client providing SNI.

I think the current conclusion is that grpc-netty does not support SNI at all since it is not possible to configure multiple aliases.

I'd accept "it is a huge pain to configure multiple aliases," but it is possible. The alias lookup is part of the KeyManager, so you can totally return them. Java's keytool can be used to create a keystore with multiple aliases, either JKS or PKCS12.

After poking around a bit, it looks like this actually works out-of-the-box (in the Java sense, not gRPC), where you don't need to write any SNI lookup code, as long as you have multiple entries in your keystore. Java's "New" X509KeyManager searches through all the aliases looking for a certificate that matches the SNI name. You just have to make sure to use the newer X509 key manager and not the older SunX509 key manager.

To make the keystore, I used openssl to create p12 files of the cert+key. Then I used keytool to combine them into a single file. (I used "changeit" as all my passwords)

openssl pkcs12 -export -out server0.p12 -inkey server0.key -in server0.pem
openssl pkcs12 -export -out server1.p12 -inkey server1.key -in server1.pem
keytool -importkeystore -srckeystore server0.p12 -destkeystore server.p12 -deststoretype pkcs12
keytool -importkeystore -srckeystore server1.p12 -destkeystore server.p12
# type "no"
# type any name

And then to create the Netty SslContext:

KeyStore store = KeyStore.getInstance("PKCS12");
store.load(TestUtils.class.getResourceAsStream("/certs/server.p12"),
    "changeit".toCharArray());
KeyManagerFactory factory = KeyManagerFactory.getInstance("NewSunX509");
factory.init(store, "changeit".toCharArray());
sslContext =
    GrpcSslContexts.configure(SslContextBuilder.forServer(factory))
        .build();

It is possible to build the KeyStore at runtime, but converting a file-based private key to the Java PrivateKey class is needlessly annoying. Normally Netty's utilities do that for you, but that's not available here to my knowledge.

@theangrydev
Copy link
Author

Thanks, I missed the SslContextBuilder.forServer(KeyManagerFactory) method.

The KeyManagerFactory ends up here converted to a OpenSslKeyMaterialProvider.

The OpenSslKeyMaterialProvider converts an alias based PrivateKey into an OpenSslKeyMaterial.

The OpenSslKeyMaterialProvider is then used in OpenSslKeyMaterialManager to choose based on alias via X509ExtendedKeyManager.

This looks good for the server side behaviour.

My understanding is that the client side still needs an SSLEngine configured with SSLParameters.setServerNames(SNIHostName) which I need to ensure is set by this point.

This is the last piece, if there is a way to change the client SSLParameters that are used in ProtocolNegotiators then I am happy to close this issue off.

@theangrydev
Copy link
Author

I think I can do this with a DelegatingSslContext which exposes the SSLEngine. I'll give all of this a go and close this issue once I have seen it working once.

Thanks again for your help digging into this.

@ejona86
Copy link
Member

ejona86 commented Nov 2, 2020

My understanding is that the client side still needs an SSLEngine configured with SSLParameters.setServerNames(SNIHostName) which I need to ensure is set by this point.

SNI is already handled. I think via passing "host" to newEngine(). In any case, lots of stuff would break if it was broken, and I did actually test that code snippet with an unmodified client and the client-provided hostname did change the certificate returned. I also saw the SNI name show up in Wireguard.

So there should be no need to change anything on client-side.

@theangrydev
Copy link
Author

Thanks, I will close this once I have verified for myself. During my previous testing I did not see the client side work out of the box and had to make the change mentioned above, so not sure what was going on there.

@theangrydev
Copy link
Author

Did your testing include the case that the client host is for an authority different than the actual host? I think that might have been where the issue was on the client side.

@ejona86
Copy link
Member

ejona86 commented Nov 2, 2020

Did your testing include the case that the client host is for an authority different than the actual host? I think that might have been where the issue was on the client side.

That's actually the only case I tested, since I was contacting localhost I used channelBuilder.overrideAuthority() to provide the name to be used for TLS/virtual hosting.

@theangrydev
Copy link
Author

With the setup I was using previously involving SniHandler on the server side, this line results in a null SNI hostname resolved unless I set SSLParameters.setServerNames(SNIHostName) manually.

This seems to indicate that the SSL client hello packet does not contain the SNI information for some reason.

I'm not sure how this differs with your approach, but it definitely looks like the hello packet in my test using a standard NettyChannelBuilder.overrideAuthority does not contain the SNI information.

@ejona86
Copy link
Member

ejona86 commented Nov 2, 2020

I made the Wireshare trace: tls trace - with sni.pcapng.gz. It shows SNI working just fine. Below is a screenshot. I used the interop client and server with ./bin/test-client --server_host_override=foo.test.google.fr --use_test_ca=true and ./bin/test-server. I built the client/server locally, but you can reproduce my results with the release build (grab the zip or tar).

SNI is handled by the actual TLS implementation. It is possible there's some issue there. I know different implementations behave differently when the hostname ends in a dot (e.g., example.com.). gRPC supports multiple TLS providers: JDK, tcnative-static-boringssl, tcnative dynamic linking against openssl, Conscrypt. You may be using a different one than me. If any of those is broken though, it needs to be fixed and we'd love to know a situation that triggers SNI to be absent.

Screenshot from 2020-11-02 11-35-39

@ejona86 ejona86 closed this as completed Nov 2, 2020
@ejona86 ejona86 reopened this Nov 2, 2020
@theangrydev
Copy link
Author

theangrydev commented Nov 2, 2020

I am using:

  • netty-tcnative-boringssl-static version 2.0.30.Final
  • grpc-java version 1.31.1
  • netty version 4.1.48.Final

I see the following in my WireShark trace.

Note that there are no is no Extension: server_name (len=23) line present.

Screenshot from 2020-11-02 19-54-12

@theangrydev
Copy link
Author

I tracked it down to SslUtils.isValidHostNameForSNI, the name I was providing did not meet the criteria of that check.

Ok I think I am good here now, thanks again for your help.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants