Repository: grpc/grpc-swift (swift)

Issue:
Set ALPN token to h2 by default.
It might surprise people to discover that the ALPN tokens aren't set to H2 on some paths (initialising the TLS config from a `TLSConfiguration` directly). Given that we only support the ALPNs we support, we should probably just forcibly set them on all code paths.

Pull request:
Motivation:

Client and server TLS configuration have initializers which allow a
`NIOSSL.TLSConfiguration` to be provided to give users a little more
control. However, it's easy to miss setting ALPN (or indeed not know it
needs to be set).

Modifications:

- Set the 'default' ALPN tokens on these code paths if the caller didn't
  priovide any tokens.
- Add tests.

Result:

- Easier for users to correctly configure TLS
- Resolves #1124

Patch:
diff --git a/Sources/GRPC/TLSConfiguration.swift b/Sources/GRPC/TLSConfiguration.swift
index 46e28f260..f8a54a30e 100644
--- a/Sources/GRPC/TLSConfiguration.swift
+++ b/Sources/GRPC/TLSConfiguration.swift
@@ -109,9 +109,20 @@ extension ClientConnection.Configuration {
     }
 
     /// Creates a TLS Configuration using the given `NIOSSL.TLSConfiguration`.
+    ///
+    /// - Note: If no ALPN tokens are set in `configuration.applicationProtocols` then "grpc-exp"
+    ///   and "h2" will be used.
+    /// - Parameters:
+    ///   - configuration: The `NIOSSL.TLSConfiguration` to base this configuration on.
+    ///   - hostnameOverride: The hostname override to use for the TLS SNI extension.
     public init(configuration: TLSConfiguration, hostnameOverride: String? = nil) {
       self.configuration = configuration
       self.hostnameOverride = hostnameOverride
+
+      // Set the ALPN tokens if none were set.
+      if self.configuration.applicationProtocols.isEmpty {
+        self.configuration.applicationProtocols = GRPCApplicationProtocolIdentifier.client
+      }
     }
   }
 }
@@ -202,9 +213,19 @@ extension Server.Configuration {
     }
 
     /// Creates a TLS Configuration using the given `NIOSSL.TLSConfiguration`.
+    /// - Note: If no ALPN tokens are set in `configuration.applicationProtocols` then the tokens
+    ///  "grpc-exp", "h2" and "http/1.1" will be used.
+    /// - Parameters:
+    ///   - configuration: The `NIOSSL.TLSConfiguration` to base this configuration on.
+    ///   - requireALPN: Whether ALPN is required.
     public init(configuration: TLSConfiguration, requireALPN: Bool = true) {
       self.configuration = configuration
       self.requireALPN = requireALPN
+
+      // Set the ALPN tokens if none were set.
+      if self.configuration.applicationProtocols.isEmpty {
+        self.configuration.applicationProtocols = GRPCApplicationProtocolIdentifier.server
+      }
     }
   }
 }


Test patch:
diff --git a/Tests/GRPCTests/ALPNConfigurationTests.swift b/Tests/GRPCTests/ALPNConfigurationTests.swift
new file mode 100644
index 000000000..2ff0974c3
--- /dev/null
+++ b/Tests/GRPCTests/ALPNConfigurationTests.swift
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2021, gRPC Authors All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import GRPC
+import NIOSSL
+import XCTest
+
+class ALPNConfigurationTests: GRPCTestCase {
+  private func assertExpectedClientALPNTokens(in tokens: [String]) {
+    XCTAssertEqual(tokens, ["grpc-exp", "h2"])
+  }
+
+  private func assertExpectedServerALPNTokens(in tokens: [String]) {
+    XCTAssertEqual(tokens, ["grpc-exp", "h2", "http/1.1"])
+  }
+
+  func testClientDefaultALPN() {
+    let config = ClientConnection.Configuration.TLS()
+    self.assertExpectedClientALPNTokens(in: config.configuration.applicationProtocols)
+  }
+
+  func testClientAddsTokensFromEmptyNIOSSLConfig() {
+    let tlsConfig = TLSConfiguration.forClient()
+    XCTAssertTrue(tlsConfig.applicationProtocols.isEmpty)
+
+    let config = ClientConnection.Configuration.TLS(configuration: tlsConfig)
+    // Should now contain expected config.
+    self.assertExpectedClientALPNTokens(in: config.configuration.applicationProtocols)
+  }
+
+  func testClientDoesNotOverrideNonEmptyNIOSSLConfig() {
+    let tlsConfig = TLSConfiguration.forClient(applicationProtocols: ["foo"])
+
+    let config = ClientConnection.Configuration.TLS(configuration: tlsConfig)
+    // Should not be overridden.
+    XCTAssertEqual(config.configuration.applicationProtocols, ["foo"])
+  }
+
+  func testServerDefaultALPN() {
+    let config = Server.Configuration.TLS(certificateChain: [], privateKey: .file(""))
+    self.assertExpectedServerALPNTokens(in: config.configuration.applicationProtocols)
+  }
+
+  func testServerAddsTokensFromEmptyNIOSSLConfig() {
+    let tlsConfig = TLSConfiguration.forServer(certificateChain: [], privateKey: .file(""))
+    XCTAssertTrue(tlsConfig.applicationProtocols.isEmpty)
+
+    let config = Server.Configuration.TLS(configuration: tlsConfig)
+    // Should now contain expected config.
+    self.assertExpectedServerALPNTokens(in: config.configuration.applicationProtocols)
+  }
+
+  func testServerDoesNotOverrideNonEmptyNIOSSLConfig() {
+    let tlsConfig = TLSConfiguration.forServer(
+      certificateChain: [],
+      privateKey: .file(""),
+      applicationProtocols: ["foo"]
+    )
+
+    let config = ClientConnection.Configuration.TLS(configuration: tlsConfig)
+    // Should not be overridden.
+    XCTAssertEqual(config.configuration.applicationProtocols, ["foo"])
+  }
+}