Add NIOTransportServices TLS Configuration options #321
Conversation
|
Can one of the admins verify this patch? |
| @@ -628,6 +628,10 @@ public class HTTPClient { | |||
| public struct Configuration { | |||
| /// TLS configuration, defaults to `TLSConfiguration.forClient()`. | |||
| public var tlsConfiguration: Optional<TLSConfiguration> | |||
| #if canImport(Network) | |||
| /// TLS configuration, defaults to `TLSConfiguration.forClient()`. | |||
There was a problem hiding this comment.
I'm curious as to why we need a different data structure. It seems like we could shim this under the existing
TLSConfigurationstructure from NIOSSL, couldn't we?
We can shim most of it. SecCertificate can be created from the DER data of a NIOSSLCertificate. SecIdentity is the problem case. It requires a p12 or a private key already in the keychain and currently there is no way to generate this from the combination of NIOSSLPrivateKey and NIOSSLCertificate unless I missed something.
There was a problem hiding this comment.
Ah yeah, SecIdentity is rough.
Perhaps this is the moment we encourage AHC to create its own representation of a TLSConfiguration that can abstract over some of these better.
There was a problem hiding this comment.
Can we expose BoringSSL pkcs#12 generation code in NIOSSL? If we have this, we can probably generate a SecIdentity from a TLSConfiguration.
There was a problem hiding this comment.
We could, though I don't love doing it.
There was a problem hiding this comment.
If we want a common structure for both NIOSSL and NIOTransportServices TLS setup I can't see any other way around supplying a p12 generation.
There was a problem hiding this comment.
Yes, I think I agree. I'd like to know what some other folks prefer here: @artemredkin @ktoso?
There was a problem hiding this comment.
I think common structure would be preferable over implementing an additional abstraction over NIOSSL and NIOTransportServices. I wonder, can that structure live in NIOTS?
There was a problem hiding this comment.
@artemredkin I believe with p12 generation from a NIOSSL private key and certificate chain we can use TLSConfiguration to cover most situations.
There was a problem hiding this comment.
I wonder, can that structure live in NIOTS?
I don't really want it to at this early stage.
|
Anymore thoughts on this? I can implement the non |
|
@adam-fowler What's your opinion on having AHC define its own TLS configuration structure that abstracts over this mess? |
I think that is something we could aspire to. But that'd be a breaking change and would have to wait for v2.0. I'm not sure what is the full list of parameters from In the immediate future we can provide additional support for trust root certificates, application protocols via the TLSConfiguration we already have and if you supply the pkcs#12 generation we can get SecIdentity equivalent of a certificate chain and private key working. |
|
Hmm, ok. Let's get going with everything but the identity stuff, and we can schedule the PKCS#12 work for SwiftNIO SSL. |
|
Closing this as it has been usurped by #350 |
This PR is a result of another #321. In that PR I provided an alternative structure to TLSConfiguration for when connecting with Transport Services. In this one I construct the NWProtocolTLS.Options from TLSConfiguration. It does mean a little more work for whenever we make a connection, but having spoken to @weissi he doesn't seem to think that is an issue. Also there is no method to create a SecIdentity at the moment. We need to generate a pkcs#12 from the certificate chain and private key, which can then be used to create the SecIdentity. This should resolve #292
This PR comes from a discussion had on the swift-server with @Lukasa
I implemented NIOTS TLS support for client certificates, trust roots and applicationProtocols in my mqtt-nio project https://github.com/adam-fowler/mqtt-nio/blob/main/Sources/MQTTNIO/TSTLSConfiguration.swift. I ended up implementing a separate structure from the nio-ssl
TLSConfigurationas there aren't easy translations to NIOTS structures fromTLSConfiguration, specifically converting client certificates to aSecIdentity.@Lukasa was not keen to have NIOTS build an abstraction over the
Network.frameworkTLS implementation but he mentioned AHC could make use of something like this.Changes include
TSTLSConfigurationHTTPClient.Configurationhas a newinitthat takes aTSTLSConfigurationTSTLSConfigurationis provided then TLS setup falls back to what we had alreadyHTTPClientNIOTSTests.testHTTPS. Doesn't look likeHTTPBinprovides support for testing trust roots or client certificates. I have thoroughly tested this code in the MQTTNIO client but don't have any tests for AHC.