grpc-java icon indicating copy to clipboard operation
grpc-java copied to clipboard

Support SslMasterKeyHandler in NettyServerBuilder

Open nucz opened this issue 5 years ago • 15 comments

I have a problem with adding SslMasterKeyHandler#newWireSharkSslMasterKeyHandler on the server side. I tried to do this with channelFactory:

builder.channelFactory(() -> {
          final ServerChannel ch = GrpcUtil.getDefaultServerChannelFactory().newChannel();
          final ChannelPipeline pipeline = ch.pipeline();
          pipeline.addLast(SslMasterKeyHandler.newWireSharkSslMasterKeyHandler());
          return ch;
        })

and later with ProtocolNegotiator:

final SslContext sslContext = GrpcSslContexts.configure(sslClientContextBuilder).build();
final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.serverTls(sslContext);
builder.protocolNegotiator(tlsNegotiator(negotiator);

In the first case, the setting had no effect. In the second case, the server worked incorrectly (it stopped accepting connections, even without a handler, just with a negotiator wrapper).

What should be the proper implementation?

nucz avatar Jul 10 '20 12:07 nucz

I don't think this is the correct usage of WireSharkSslMasterKeyHandler. You would only need to enable it by setting the system property -Dio.netty.ssl.masterKeyHandler=true, no modification for the pipeline is needed. See details in its Javadoc.

voidzcy avatar Jul 11 '20 00:07 voidzcy

@voidzcy, I don't think that system property would work as-is. That only works if gRPC included SslMasterKeyHandler in the pipeline already.

Changing the channelFactory() didn't work because that's the listening channel factory. It produces children for each connection and those are what you are wanting to modify. The protocolNegotiator() snippet looks pretty mangled, such that it is unclear what it would look like once you actually got it to compile.

I don't think there is a proper implementation currently. You could maybe hack something with the channelFactory(), but it would be a big hack. It isn't really safe for you to change the pipeline without gRPC being aware; we've found Netty pipelines to be very brittle.

ejona86 avatar Jul 11 '20 20:07 ejona86

@nucz, would it be fair to call this an enhancement request asking for SslMasterKeyHandler support?

It seems we could check the system property io.netty.ssl.masterKeyHandler just like Netty and if it is true we add the handler to the pipeline. I'd prefer not having it on the pipeline all the time, to avoid any potential performance/behavior confusion. I'm disappointed it doesn't remove itself from the pipeline after the handshake completes; I wonder if it doesn't to allow supporting rekeying (I don't remember what is supported there).

ejona86 avatar Jul 20 '20 21:07 ejona86

@ejona86 Yes, that would be great, to have SslMasterKeyHandler support. I found the solution using negotiators, but it is far from ideal and is version sensitive.

nucz avatar Jul 24 '20 20:07 nucz

@ejona86 I would like to take a stab at this, but off the top of your head, please what's your estimate for this unit of work?

Nnadozie avatar Oct 14 '20 21:10 Nnadozie

@Nnadozie, should be like 3 lines of code. You'd basically check the system property, and if set, create and add the additional handler within ServerTlsHandler.

ejona86 avatar Nov 03 '20 21:11 ejona86

Thanks @ejona86! Working on a PR now.

Nnadozie avatar Nov 04 '20 21:11 Nnadozie

@ejona86 @Nnadozie I suggest that this feature can also be used on the client side, because sometimes you can only debug on the client side (because the server is provided by others).

In addition, I recommend to extend .sslContext(SslContext sslContext) to .sslContext(SslContext sslContext, boolean allowExportMasterKey). To do pipeline.addLast(SslMasterKeyHandler.newWireSharkSslMasterKeyHandler()), only while both the allowExportMasterKe and the system property "io.netty.ssl.masterKeyHandler" are true. Because sometimes there are several grpc clients working in one program, we may only want to track/debug on the traffic one of them.

huangqiangxiong avatar Nov 28 '20 06:11 huangqiangxiong

Hi, @Nnadozie Are you still working on this feature? If you don't have time, please tell me, I may make a try.

huangqiangxiong avatar Dec 10 '20 16:12 huangqiangxiong

@huangqiangxiong I haven't had time lately so please go ahead.

Nnadozie avatar Dec 10 '20 16:12 Nnadozie

@ejona86 I submit a PR according to your early idea (checking the system property, and if set, create and add the additional handler within ServerTlsHandler) because I find the ClientTlsHandler/ServerTlsHandler are not only created through NettyChannelBuilder or NettyServerBuilder. So I give up my original idea. Only changing ClientTlsHandler/ServerTlsHandler is more simple and enough for debug.

huangqiangxiong avatar Dec 12 '20 09:12 huangqiangxiong

Reopening since #7724 was reverted by #7792.

ejona86 avatar Jan 20 '21 18:01 ejona86

I find an alternative way to export TLS key materials in Java that is using jsslkeylog and replacing boringssl sslProvider with JDK built-in security Provider.

I think some people may get help from it, so record it here https://gitlab.com/wireshark/wireshark/-/wikis/How-to-Export-TLS-Master-keys-of-gRPC#using-jsslkeylog

huangqiangxiong avatar Feb 26 '22 09:02 huangqiangxiong