NIOSSL(Client|Server)Handler's init is throwing but that's not actionable

[weissi]

NIOSSLServerHandler's init is throwing and that has two issues:

1. it's not really actionable so everybody writes try! NIOSSLServerHandler(context: sslContext). If that's what we want, we can just do that in the init.
2. it's almost always used in the clientChannelInitializer where you have to return a future...

[weissi]

@Lukasa wdyt?

[wlisac]

I think I hit a similar challenge using the NIOSSLClientHandler.

This is how I solved it, but having an API that works nicer with the channelInitializer would be helpful 🙂

.channelInitializer { channel in
    channel.pipeline.addSSLHandlerIfNeeded(configuration: tlsConfiguration,
                                           serverHostname: host)
}

extension ChannelPipeline {
    fileprivate func addSSLHandlerIfNeeded(configuration: TLSConfiguration?, serverHostname: String) -> EventLoopFuture<Void> {
        guard let configuration = configuration else {
            return eventLoop.makeSucceededFuture(())
        }
        
        do {
            let sslContext = try NIOSSLContext(configuration: configuration)
            let handler = try NIOSSLClientHandler(context: sslContext, serverHostname: serverHostname)
            return addHandler(handler)
        } catch {
            return eventLoop.makeFailedFuture(error)
        }
    }
}

[weissi]

Thanks @wlisac! I think there also other such APIs that we should improve on.

[Lukasa]

The server handler can be made to stop throwing, as its only failure mode is an inability to allocate the BoringSSL object. The client handler is harder: it takes an SNI name and that can potentially be invalid. We can solve that by giving that idea a type that isn’t String, which would also allow us to introduce a non-throwing initializer.

Do we know what the API ramifications are of removing throws from a method signature? Is it backward compatible? I know it’ll introduce a warning, but that just requires us to have a semver minor.

[weissi]

The server handler can be made to stop throwing, as its only failure mode is an inability to allocate the BoringSSL object.

Awesome.

The client handler is harder: it takes an SNI name and that can potentially be invalid. We can solve that by giving that idea a type that isn’t String, which would also allow us to introduce a non-throwing initializer.

What happens if we allowed everything? In HTTP for example, there are also clearly invalid values for for example headers and we don't handle them in any special way. You could for example add the following as a header name: \nfoo\nbar::: and we're just allowing you to screw up at the moment.

I'm not really sure what to do for the SNI name, what does BoringSSL do if we pass total garbage? Couldn't we surface that as an error in the pipeline?

Do we know what the API ramifications are of removing throws from a method signature? Is it backward compatible? I know it’ll introduce a warning, but that just requires us to have a semver minor.

I have no idea, need to check.

[Lukasa]

Couldn't we surface that as an error in the pipeline?

In my view surfacing this as an error in the pipeline is strictly worse than forcing a throwable initializer on a type: we knew well in advance that the name was bad, and we allowed it through anyway.

Failing to set a correct server name leads to a guaranteed TLS handshake failure. Exactly what that will look like is a bit more difficult though. BoringSSL will accept nearly anything and will send it out, so the nature of the failure mode is dependent on the server. The most common failure mode will be a fatal alert in the handshake, followed closely in frequency by a certificate verification failure caused by the server guessing that you wanted the default host and serving that certificate. In either case, the outcome is pretty bad.

[weissi]

Couldn't we surface that as an error in the pipeline?

In my view surfacing this as an error in the pipeline is strictly worse than forcing a throwable initializer on a type: we knew well in advance that the name was bad, and we allowed it through anyway.

Failing to set a correct server name leads to a guaranteed TLS handshake failure. Exactly what that will look like is a bit more difficult though. BoringSSL will accept nearly anything and will send it out, so the nature of the failure mode is dependent on the server. The most common failure mode will be a fatal alert in the handshake, followed closely in frequency by a certificate verification failure caused by the server guessing that you wanted the default host and serving that certificate. In either case, the outcome is pretty bad.

I totally see where you're coming from and agree with most of it. The API issue however is: The user gets an error thrown in a place where they have to produce a future. I think there are two options that fit nicely into the API

1. making it non-fallible (having it handled in the pipeline)
2. returning a future

There is a third option (which is what we have today) which doesn't fit into the shape of the channelInitializer API. If we leave the client handler the way it is, the user essentially has two options of handling that:

a. try! and crash
b. do/catch and convert to future.

Neither of which are really that great. WDYT is the best way forward? I'm still leaning towards (1) because the user has to handle handshake failure and other errors anyway.

[Lukasa]

The API issue however is: The user gets an error thrown in a place where they have to produce a future

Sure, but why don't we just make that experience better?

extension EventLoop {
    func `try`<T>(_ body: () throws -> T) -> EventLoopFuture<T> {
        do {
            return self.makeSucceededFuture(try body())
        } catch {
            return self.makeFailedFuture(error)
        }
    }
}

[Lukasa]

Hmm, also, how does failing in the pipeline work?

We can't fail on handlerAdded when the channel isn't active because it's highly likely we're being added in the channelInitializer and the channel is being built from front-to-back. We could try to fail fast, but the matrix for doing that is:

1. error in handlerAdded if channel is active; else
2. error on the first of connect or channelActive.

Otherwise we could just try to move forward, but then we are burning all of the CPU required to fully establish a pipeline and a TCP connection and part of a TLS handshake only to kill the connection. This failure mode also plays havoc with connection pooling, which will have seen a connection actually succeed before being apparently killed and so will likely retry a doomed connection.

I just don't see this behaving well at all.

[weissi]

Hmm, also, how does failing in the pipeline work?

We can't fail on handlerAdded when the channel isn't active because it's highly likely we're being added in the channelInitializer and the channel is being built from front-to-back. We could try to fail fast, but the matrix for doing that is:

1. error in handlerAdded if channel is active; else
2. error on the first of connect or channelActive.

Otherwise we could just try to move forward, but then we are burning all of the CPU required to fully establish a pipeline and a TCP connection and part of a TLS handshake only to kill the connection. This failure mode also plays havoc with connection pooling, which will have seen a connection actually succeed before being apparently killed and so will likely retry a doomed connection.

I just don't see this behaving well at all.

Agreed. You're totally right that failing through the pipeline won't actually work (unless we delay it to handlerAdded/channelActive which sucks).

Don't really have a good option. Only way out I now see is to switch from using an init to a static func ... -> EventLoopFuture<SSLHandler> maybe?

[Lukasa]

I'm certainly open to adding that static func and encouraging its use, as that will keep things cleaner. Do you want the EventLoopFuture extension above as well, or just the static func?

[weissi]

@Lukasa I think just adding that static func is good enough. Of course, we could add the try too but I'm not sure how many APIs (that aren't blocking) this would be useful for. But if you think try is useful we can add that too (to swift-nio I guess).

[Lukasa]

Ok, a quick test with the NIO API breakage tools suggests that removing throws from a function is safe from an API breakage perspective, so we should be good to go. That means the plan of action is:

1. Update NIOSSLClientHandler.init to preconditionFailure if we fail to allocate a BoringSSL object, and then remove throws from that function.
2. Update NIOSSLServerHandler.init to preconditionFailure if we fail to allocate a BoringSSL object.
3. Add static func create(context: NIOSSLContext, verificationCallback: NIOSSLVerificationCallback? = nil) -> EventLoopFuture<NIOSSLServerHandler>.

Agreed?

[weissi]

Wow, that is surprising but good :). Did the API breakage checker work with NIOSSL or did you check in a separate repo?

@Lukasa I think that sounds like a plan!

[Lukasa]

I checked with the NIO repository, just tried it on a separate throwing function there. We should get it hooked up here too really.

[weissi]

Awesome, yeah. I need to find time to properly debug what’s wrong with conplicated C targets and breakage checker

[weissi]

@Lukasa actually, the factory should be a make*