Ensure port isn't included in SNI hostname (#76)

Motivation:

gRPC derives the authority from various sources and uses this value for
the SNI server hostname. The authority should include non-standard ports
and the SNI hostname must not include the port. In some cases this
wasn't being respected and the port was being used in SNI this results
in handshake failures.

Modifications:

- Have the Connection sanitize the authority so that it's suitable for
SNI.

Result:

- Fewer handshake failures
- Resolves #71

Author: glbrntt

Sources/GRPCNIOTransportCore/Client/Connection/Connection.swift

@@ -89,6 +89,9 @@ package final class Connection: Sendable {
   /// being connected to.
   private let authority: String?
 
+  /// The name of the server used for the TLS SNI extension, if applicable.
+  private let sniServerHostname: String?
+
   /// The default compression algorithm used for requests.
   private let defaultCompression: CompressionAlgorithm
 
@@ -111,6 +114,20 @@ package final class Connection: Sendable {
     self.event.stream
   }
 
+  private static func sanitizeAuthorityForSNI(_ authority: String) -> String {
+    // Strip off a trailing ":{PORT}". Look for the last non-digit byte, if it's
+    // a colon then keep everything up to that index.
+    let index = authority.utf8.lastIndex { byte in
+      return byte < UInt8(ascii: "0") || byte > UInt8(ascii: "9")
+    }
+
+    if let index = index, authority.utf8[index] == UInt8(ascii: ":") {
+      return String(authority.utf8[..<index])!
+    } else {
+      return authority
+    }
+  }
+
   package init(
     address: SocketAddress,
     authority: String?,
@@ -120,6 +137,7 @@ package final class Connection: Sendable {
   ) {
     self.address = address
     self.authority = authority
+    self.sniServerHostname = authority.map { Self.sanitizeAuthorityForSNI($0) }
     self.defaultCompression = defaultCompression
     self.enabledCompression = enabledCompression
     self.http2Connector = http2Connector
@@ -140,7 +158,7 @@ package final class Connection: Sendable {
           // The authority here is used for the SNI hostname in the TLS handshake (if applicable)
           // where a raw IP address isn't permitted, so fallback to 'address.sniHostname' rather
           // than 'address.authority'.
-          authority: self.authority ?? self.address.sniHostname
+          sniServerHostname: self.sniServerHostname ?? self.address.sniHostname
         )
       } catch let error as RPCError {
         throw error

Sources/GRPCNIOTransportCore/Client/Connection/ConnectionFactory.swift

@@ -23,10 +23,10 @@ package protocol HTTP2Connector: Sendable {
   ///
   /// - Parameters:
   ///   - address: The address to connect to.
-  ///   - authority: The authority as used for the TLS SNI extension (if applicable).
+  ///   - sniServerHostname: The name of the server used for the TLS SNI extension (if applicable).
   func establishConnection(
     to address: SocketAddress,
-    authority: String?
+    sniServerHostname: String?
   ) async throws -> HTTP2Connection
 }
 

Sources/GRPCNIOTransportHTTP2Posix/HTTP2ClientTransport+Posix.swift

@@ -165,7 +165,7 @@ extension HTTP2ClientTransport.Posix {
 
     func establishConnection(
       to address: GRPCNIOTransportCore.SocketAddress,
-      authority: String?
+      sniServerHostname: String?
     ) async throws -> HTTP2Connection {
       let (channel, multiplexer) = try await ClientBootstrap(
         group: self.eventLoopGroup
@@ -175,7 +175,7 @@ extension HTTP2ClientTransport.Posix {
             try channel.pipeline.syncOperations.addHandler(
               NIOSSLClientHandler(
                 context: sslContext,
-                serverHostname: authority
+                serverHostname: sniServerHostname
               )
             )
           }

Sources/GRPCNIOTransportHTTP2TransportServices/HTTP2ClientTransport+TransportServices.swift

@@ -149,7 +149,7 @@ extension HTTP2ClientTransport.TransportServices {
 
     func establishConnection(
       to address: GRPCNIOTransportCore.SocketAddress,
-      authority: String?
+      sniServerHostname: String?
     ) async throws -> HTTP2Connection {
       let bootstrap: NIOTSConnectionBootstrap
       let isPlainText: Bool
@@ -162,7 +162,7 @@ extension HTTP2ClientTransport.TransportServices {
       case .tls(let tlsConfig):
         isPlainText = false
         do {
-          let options = try NWProtocolTLS.Options(tlsConfig, authority: authority)
+          let options = try NWProtocolTLS.Options(tlsConfig, authority: sniServerHostname)
           bootstrap = NIOTSConnectionBootstrap(group: self.eventLoopGroup)
             .channelOption(NIOTSChannelOptions.waitForActivity, value: false)
             .tlsOptions(options)

Tests/GRPCNIOTransportCoreTests/Client/Connection/ConnectionTests.swift

@@ -21,6 +21,7 @@ import NIOCore
 import NIOHPACK
 import NIOHTTP2
 import NIOPosix
+import Synchronization
 import XCTest
 
 final class ConnectionTests: XCTestCase {
@@ -199,6 +200,44 @@ final class ConnectionTests: XCTestCase {
       XCTAssertEqual(error.code, .unavailable)
     }
   }
+
+  private func testAuthorityIsSanitized(authority: String, expected: String) async throws {
+    let recorder = SNIRecordingConnector()
+    let connection = Connection(
+      address: .ipv4(host: "ignored", port: 0),
+      authority: authority,
+      http2Connector: recorder,
+      defaultCompression: .none,
+      enabledCompression: .none
+    )
+
+    // The connect attempt will fail, but as a side effect the SNI hostname
+    // will be recorded.
+    await connection.run()
+    XCTAssertEqual(recorder.sniHostnames, [expected])
+  }
+
+  func testAuthorityIsSanitized() async throws {
+    try await self.testAuthorityIsSanitized(
+      authority: "foo.example.com",
+      expected: "foo.example.com"
+    )
+
+    try await self.testAuthorityIsSanitized(
+      authority: "foo.example.com:31415",
+      expected: "foo.example.com"
+    )
+
+    try await self.testAuthorityIsSanitized(
+      authority: "foo.example-31415",
+      expected: "foo.example-31415"
+    )
+
+    try await self.testAuthorityIsSanitized(
+      authority: "foo.example.com:abc123",
+      expected: "foo.example.com:abc123"
+    )
+  }
 }
 
 extension ClientBootstrap {
@@ -252,3 +291,25 @@ extension Metadata {
     self = metadata
   }
 }
+
+final class SNIRecordingConnector: HTTP2Connector {
+  private let _sniHostnames: Mutex<[String?]>
+
+  var sniHostnames: [String?] {
+    self._sniHostnames.withLock { $0 }
+  }
+
+  init() {
+    self._sniHostnames = Mutex([])
+  }
+
+  func establishConnection(
+    to address: GRPCNIOTransportCore.SocketAddress,
+    sniServerHostname: String?
+  ) async throws -> GRPCNIOTransportCore.HTTP2Connection {
+    self._sniHostnames.withLock {
+      $0.append(sniServerHostname)
+    }
+    throw RPCError(code: .unavailable, message: "Test is expected to throw.")
+  }
+}

Tests/GRPCNIOTransportCoreTests/Client/Connection/Utilities/HTTP2Connectors.swift

@@ -62,7 +62,7 @@ struct ThrowingConnector: HTTP2Connector {
 
   func establishConnection(
     to address: GRPCNIOTransportCore.SocketAddress,
-    authority: String?
+    sniServerHostname: String?
   ) async throws -> HTTP2Connection {
     throw self.error
   }
@@ -71,7 +71,7 @@ struct ThrowingConnector: HTTP2Connector {
 struct NeverConnector: HTTP2Connector {
   func establishConnection(
     to address: GRPCNIOTransportCore.SocketAddress,
-    authority: String?
+    sniServerHostname: String?
   ) async throws -> HTTP2Connection {
     fatalError("\(#function) called unexpectedly")
   }
@@ -103,7 +103,7 @@ struct NIOPosixConnector: HTTP2Connector {
 
   func establishConnection(
     to address: GRPCNIOTransportCore.SocketAddress,
-    authority: String?
+    sniServerHostname: String?
   ) async throws -> HTTP2Connection {
     return try await ClientBootstrap(group: self.eventLoopGroup).connect(to: address) { channel in
       channel.eventLoop.makeCompletedFuture {