Don't call validateSNIServerName when TLS hostname verification is disabled

[nuyawktuah]

I noticed that validateSNIServerName is called even when the TLS configuration explicitly disables hostname validation with certificateVerification set to .noHostnameVerification. I'm not too familiar with this library but this seems like a bug to me?

This pr adds a check to NIOSSLClientHandler.swift and UniversalBootstrapSupport.swift to skip validateSNIServerName() if the TLS context has certificateVerification set to .noHostnameVerification. It also adds a test case which fails without this check and now passes.

Hope this change makes sense! Please let me know if there is anything I've missed or can improve.

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[Lukasa]

Thanks for filing this!

This behaviour is not a bug: validateSNIServerName is validating that the name provided as the serverHostname argument is a valid SNI string. This is not validating that it matches the name provided in the server certificate, but instead confirming it's acceptable for us to send it in the handshake extension.

The extension is defined in RFC 6066 § 3, which contains the following constraints:

Literal IPv4 and IPv6 addresses are not permitted in "HostName".

"HostName" contains the fully qualified DNS hostname of the server, as understood by the client.

A DNS hostname forbids embedded NULs and can be no more than 255 bytes long.

[nuyawktuah]

Thanks for the explanation! That makes total sense.

It seems that the bug is in the higher level library that I'm using: https://github.com/vapor/websocket-kit/blob/main/Sources/WebSocketKit/WebSocketClient.swift#L100

I suppose the proper fix would be to check there if the host is an IP address and if so, pass nil as serverHostname.

[Lukasa]

Yup, that would work. Alternatively, the wrapper library could catch those specific errors and re-create the handler without the SNI, as the errors are localised and only thrown from there.

[nuyawktuah]

Ah good idea, catching the error would definitely be cleaner. Unfortunately the compiler is complaining:

Referencing operator function '~=' on '_ErrorCodeProtocol' requires that 'NIOSSLExtraError' conform to '_ErrorCodeProtocol'

let tlsHandler: NIOSSLClientHandler
do {
    tlsHandler = try NIOSSLClientHandler(context: context, serverHostname: host)
} catch NIOSSLExtraError.cannotUseIPAddressInSNI {
    tlsHandler = try NIOSSLClientHandler(context: context, serverHostname: nil)
}

Am I catching the error incorrectly or does NIOSSLExtraError need some work to conform to that protocol? Hope you don't mind me asking, I'm just a bit out of my depth here and could use some help if you can spare the time :)

[Lukasa]

I think you need a longer catch clause: NIOSSLExtraError isn't an enum and so you can't catch it using that nice shorthand syntax. You'll need something like:

catch let error as NIOSSLExtraError, error == .cannotUseIPAddressInSNI

[nuyawktuah]

Got it working with

catch let error as NIOSSLExtraError where error == .cannotUseIPAddressInSNI

Thanks so much for all the help!

[galtek]

Hi, what is the alternative when I need to use literal IP host name, because will make connection/handshake with an IoT device, that have this IP, but not server name?
There any alternative to this error using this library or overwrite some method or parameter?
Thanks

Snippet of the code:
`
//...
// The server's hostname and port.
let _hostname = "(NUMBER IP)"
let _port = (NUMBER PORT)
var _certbundlePath = ""
var _keybundlePath = ""

//...

        // Create an NIOSSLContext from the TLSConfiguration.
        let sslContext = try NIOSSLContext(configuration: tlsConfiguration)

        // Initialize the Bootstrap with NIOSSL.
        let bootstrap = ClientBootstrap(group: _group)
            .channelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
            .channelInitializer { channel in
                // Add the NIOSSLHandler to the pipeline.
                do {
                    let sslHandler = try NIOSSLClientHandler(context: sslContext, serverHostname: _hostname)
                    return channel.pipeline.addHandler(sslHandler).flatMap {
                        // Add your custom application handler to the pipeline.
                        // This handler will handle your application's logic.
                        do {
                            let customHandler = try CustomHandler(serverHostname: _hostname)
                            channel.pipeline.addHandler(customHandler)
                        } catch {
                            print("Error: \(error)")
                            return channel.close()
                        }
                        return channel.close()
                    }
                } catch {
                    print("Error: \(error)")
                }
                return channel.close()
            }

        // Connect to the server.
        let channel = try bootstrap.connect(host: _hostname, port: _port).wait()

//...
`

Out on Terminal:
Error: NIOSSLExtraError.cannotUseIPAddressInSNI: IP addresses cannot validly be used for Server Name Indication, got (NUMBER IP)

[Lukasa]

Set the hostname to nil. NIOSSL automatically checks for the IP address in the SAN field, so long as that field has type iPAddress.

[galtek]

Thanks @Lukasa , but I have a IP, example let _hostname = "10.0.0.200" and let _port = 1080. When do you suggest that I put the value to nil ?

You can write me an example or edit part of my snippet to visualize your suggestion?

[Lukasa]

Change:

let sslHandler = try NIOSSLClientHandler(context: sslContext, serverHostname: _hostname)

To

let sslHandler = try NIOSSLClientHandler(context: sslContext, serverHostname: nil)