Don't use IP addresses for SNI

[nuyawktuah]

TLS forbids the use of literal IPv4 and IPv6 addresses in server name indication. However, websocket-kit passes IP addresses to NIOSSLClientHandler as serverHostname, which triggers an error when the underlying validateSNIServerName is called. See https://github.com/apple/swift-nio-ssl/pull/380 for more context.

This PR adds a do / catch statement to pass nil for serverHostname in case of the specific cannotUseIPAddressInSNI error, which allows for secure connections to IP addresses.

[0xTim]

@olivernyc Thanks for this! Is there any way to write a test to verify the new behaviour?

[codecov-commenter]

Codecov Report

Merging #119 (eb22d5c) into main (d8230ea) will decrease coverage by 4.55%. The diff coverage is 100.00%.

:exclamation: Current head eb22d5c differs from pull request most recent head f58f890. Consider uploading reports for the commit f58f890 to get more accurate results

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
- Coverage   71.34%   66.78%   -4.56%     
==========================================
  Files           5        6       +1     
  Lines         485      584      +99     
==========================================
+ Hits          346      390      +44     
- Misses        139      194      +55     

Flag	Coverage Δ
unittests	66.78% <100.00%> (-4.56%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Sources/WebSocketKit/WebSocketClient.swift	79.62% <100.00%> (+11.66%)	:arrow_up:
...bSocketKit/Concurrency/WebSocket+Concurrency.swift	29.78% <0.00%> (ø)

[nuyawktuah]

Just tried adding a test but I'm having a bit of trouble setting up the server. connect is failing with the error OPENSSL_internal:WRONG_VERSION_NUMBER, so it seems like it needs a bit of work to handle TLS connections. However, if you set breakpoints here and here you can verify that the error is being thrown and that the fix successfully initializes tlsHandler.

[nuyawktuah]

Ok, I got it mostly working by including generateSelfSignedCert from NIOSSLTestHelpers.swift. I've verified that the connection fails without my fix and succeeds with it.

I'm running in to a small issue where the server fails to bind when the test is run in quick succession: Address already in use. I figured the call to server.close(mode: .all).wait() would prevent this but perhaps there is something else I'm missing?

[nuyawktuah]

I added a notice and updated the test to use an automatically assigned port. Seems to be working well now! Please let me know if there is anything else I should fix or improve.

[nuyawktuah]

Thanks for the help!

[VaporBot]

These changes are now available in 2.5.1