twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.78k stars 1.45k forks source link

Netty4 clients hide crucial TLS config parameters #580

Closed olix0r closed 7 years ago

olix0r commented 7 years ago

Netty4SslHandler does not support configuration of client-side protocol negotiation, which is necessary for, for instance, HTTP/2 clients that communicate with servers that support both http and h2.

The Transport.Tls param and TlsConfig expose a nextProtocol configuration for Servers but not for Clients. Though, I'll argue that things like protocols should not be configured by by the TlsConfig -- clients & servers need to configure their supported protocols independently from their configuration about, for instance, which keys/certs to use and whether validation should be performed.

I put together a workaround for this in https://github.com/BuoyantIO/linkerd/pull/917. I'd like to port something like this into finagle, but want to discuss that approach here before doing so.

Furthermore, I'd like to be able to configure cipher suites, session timeouts, etc (basically, everything that netty's SslContextBuilder supports).

Thoughts? @mosesn @vkostyukov @ryanoneill etc

ryanoneill commented 7 years ago

@olix0r You should be able to do this once all my stuff has shipped (which it's started too, for preliminaries check out util-security).

The next commit will include SslClientEngineFactory in finagle-core. That is a generic trait which can be extended, of which one extension is Netty4ClientEngineFactory, which uses Netty 4's SslContextBuilder and can pass in the ApplicationProtocols specified in the SslClientConfiguration into the engine factory, which in turn passes them into the SslContextBuilder.

CipherSuites will be similarly supported for all engines. Timeouts are not at the moment, but we could revisit soon afterwards, and I would also like to close this long outstanding issue.

When my changes are complete, TlsConfig will go away, and the params will be much closer to how Finagle running Netty3 works today. There will be a ClientEngine param which requires an instance of SslClientEngineFactory, a ClientConfiguration param which requires an instance of SslClientConfiguration, and a ClientVerifier param, which will allow the replacement and the eventual removal of SessionVerifier.

If our ClientEngineFactory didn't work for you at that point, then you'd also be able to construct your own and use that instead. Thanks for your patience on this. Sorry for the headaches.

olix0r commented 7 years ago

Furthermore, the only way to configure a CA cert with a client is to build a java SSLContext, which can't be used in conjunction with netty's SslContext...

Are there concrete plans to address this already? The current state of things is pretty unusable for anything but trivial client tls configurations. I think we'd be willing to take this on if it's worthwhile for us to do so.

olix0r commented 7 years ago

@ryanoneill that's great news! thanks. let us know if there's anything we can do to help it along.

ryanoneill commented 7 years ago

Just to clarify, when you say a CA cert with a client, are you talking about using the CA cert for trusting a certificate from the server, or in using a chain of certificates for configuration for key management.

Example: Root CA -> Intermediate CA -> Client Cert / Server Cert

Are you talking about configuring the client with ([Root CA,] Intermediate CA, Client Cert) so that multiple certs are sent by the client for client authentication or using the Intermediate cert for trust management?

olix0r commented 7 years ago

Out of curiosity: will this new type unify Netty3 and Netty4 tls configuration? or is the plan to just let Netty3 drift off to sea?

olix0r commented 7 years ago

@ryanoneill My initial goal is only trust management.

We'd like to support client certs as well, but that's secondary.

ryanoneill commented 7 years ago

You would then want an SslClientConfiguration that uses TrustCredentials.CertCollection.

That file should contain multiple certificates that you would like to trust. With the Netty4ClientEngineFactory for instance, that file gets sent to the builder.trustManager for the client.

For what will be our current 'default' engine factory, that file is used by X509TrustManagerFactory to create a TrustManager which is used by a Java SSLContext.

Hope that gives a clearer picture of the direction where things are headed.

Client certs will be supported too as part of this. It's one of the reasons we are making large changes here. So a Finagle server will be able to want/need a client certificate, and the client will be able to send it.

ryanoneill commented 7 years ago

Just saw your previous question. These changes will unify Netty 3 and Netty 4 SSL/TLS configuration. You would also be able to use the Netty4ClientEngineFactory or Netty4ServerEngineFactory with a Finagle client/server running Netty3. You would want to do that if you're still using Netty 3, want client authentication, and you want to use netty-tcnative.

My work is independent of our move to get everything in Finagle working on Netty 4, hence the ability to use this piece of Netty 4 with Finagle running Netty 3.

olix0r commented 7 years ago

Thanks for the clarification. Some more questions:

Re: SslClientConfiguration, how do you foresee configuration being composed from multiple sources. I have some protocol-specific client that knows what protocols need to be supported independently of the details of verification. It seems desirable to just simply do something like:

params = clientParams +
  ApplicationProtocols("h2")

(like any other protocol-specific configuration). hostname and trustCredentials, on the other hand, is likely to be configured separately in some endpoint-specific way (and not protocol-specific). It seems a bit awkward for these bound into one type (though I haven't read enough of the code to see how these things are actually used). Any thoughts on how to set something like ApplicationProtocols as its own parameter (i.e. I'd want to set that on our H2 client, and i don't have enough of the other details to fill out a full SslClientConfiguration statically).

It is important that we be able to use an alternate crypto provider (boringssl). The netty docs indicate that the TrustManagerFactory api can only be used to configure the JDK provider. I suspect that you have similar needs. Is this all copacetic?

ryanoneill commented 7 years ago

I think I understand your first question. My current thinking is that it might be best for you to refactor our Netty4ClientEngineFactory to work also for your needs, and have the "h2" part stick with the engine factory in this case, and leave the applicationProtocols section of the SslClientConfiguration as Unspecified. I'm open to further ideas once you can see the items in place.

Yeah, the TrustManagerFactory/TrustManager code will only be used by us with a different SslClientEngineFactory, not the Netty4ClientEngineFactory. The Netty4ClientEngineFactory will use a File, specifically for the reason you mentioned.

ryanoneill commented 7 years ago

If you can elaborate further on endpoint-specific vs not protocol-specific that might help.

olix0r commented 7 years ago

I think there are several distinct components of TLS configuration that can change independently and are typically set by different things:

Application protocols are basically always to be set the same for all clients in a protocol, and should be able to be configured statically for the protocol (for example, here). Local credentialing may, for instance, be configured for each process, and remote verification configured for each client.

I think it would be simpler for people like me (who have configuration frameworks over finagle) if these can be changed independently. With a monolithic config object, levels of indirection have to be introduced to augment configured policy.

ryanoneill commented 7 years ago

Within another week or so you should be able to see what this code looks like and then we can talk again about what does and doesn't work for you.

koshelev commented 7 years ago

@ryanoneill any progress on this issue? Let me know if there is something I can help with

ryanoneill commented 7 years ago

We are in the process of doing the OSS release for Finagle 6.42.0 now.

My changes will all be public in 6.43.0. You can see what some of them look like by looking on the develop branch now. The stack parameters for Netty 3 and Netty 4 both will be unified and be changed internally (and on develop) hopefully next week. The additional parameter for verification (which is what would allow setting the other parameters that Oliver was talking about) should be added soon after.

vkostyukov commented 7 years ago

I think it's no longer an issue (given our current SSL API). Please, let us know if you still experiencing troubles configuring TLS/SSL clients/servers.