Allow custom SslContext without ALPN again for HTTP/1.1-only case

src/aleph/http.clj

@@ -208,11 +208,17 @@
            control-period       60000
            middleware           middleware/wrap-request
            max-queue-size       65536}}]
-  (when (and (false? (:keep-alive? connection-options))
-             (pos? (:idle-timeout connection-options 0)))
-    (throw
-      (IllegalArgumentException.
+  (let [{:keys [keep-alive?
+                idle-timeout
+                http-versions
+                force-h2c?]
+         :or {idle-timeout 0}} connection-options]
+    (when (and (false? keep-alive?) (pos? idle-timeout))
+      (throw
+       (IllegalArgumentException.
         ":idle-timeout option is not allowed when :keep-alive? is explicitly disabled")))
+    (when (and force-h2c? (not-any? #{:http2} http-versions))
+      (throw (IllegalArgumentException. "force-h2c? may only be true when HTTP/2 is enabled."))))
 
   (let [log-activity (:log-activity connection-options)
         dns-options' (if-not (and (some? dns-options)

src/aleph/http/common.clj

@@ -158,8 +158,9 @@
    If `ssl-context` is an options map and it does contain an `:application-protocol-config` key, its
    supported protocols are validated to match `desired-http-versions`.
 
-   Otherwise, if `ssl-context` is an `io.netty.handler.ssl.SslContext` instance, its supported
-   protocols are validated to match `desired-http-versions`."
+   Otherwise, if `ssl-context` is an `io.netty.handler.ssl.SslContext` instance, its ALPN config's
+   protocols are validated to match `desired-http-versions`. If it doesn't have an ALPN config,
+   `desired-http-versions` may only contain `:http1`."
   [ssl-context desired-http-versions]
   (when-let [unsupported-http-versions (seq (remove supported-http-versions desired-http-versions))]
     (throw (ex-info "Unsupported desired HTTP versions"
@@ -178,13 +179,23 @@
     nil
 
     :else
-    (do
-      (assert-consistent-alpn-config!
-       (some-> ^SslContext ssl-context
-               (.applicationProtocolNegotiator)
-               (.protocols))
-       desired-http-versions)
-      ssl-context)))
+    ;; NOTE: `SslContext` always has an `ApplicationProtocolNegotiator`, even if
+    ;; `.applicationProtocolConfig` was never invoked in the `SslContextBuilder`. In this case, its
+    ;; `.protocols` will be an empty collection, which we thus treat as if no ALPN config is
+    ;; present.
+    (if-let [protocols (some-> ^SslContext ssl-context
+                               (.applicationProtocolNegotiator)
+                               (.protocols)
+                               (seq))]
+      (do
+        (assert-consistent-alpn-config!
+         protocols
+         desired-http-versions)
+        ssl-context)
+      (if (= [:http1] desired-http-versions)
+        ssl-context
+        (throw (ex-info "Supplied SslContext with no ALPN config, but requested secure non-HTTP/1 versions that require ALPN."
+                        {:desired-http-versions desired-http-versions}))))))
 
 (defn validate-http1-pipeline-transform
   "Checks that :pipeline-transform is not being used with HTTP/2, since Netty

test/aleph/http_test.clj

@@ -1339,7 +1339,7 @@
     (catch Exception e
       e)))
 
-(deftest test-http-versions-config
+(deftest test-http-versions-server-config
   (testing "ssl-context as options map"
 
     (testing "with different HTTP versions in ALPN config"
@@ -1424,12 +1424,18 @@
                                           (netty/application-protocol-config [:http2 :http1])))})]
         (is (= :started result))))
 
-    (testing "with no ALPN config"
+    (testing "with no ALPN config but desiring HTTP/2"
       (let [result (try-start-server
                     {:http-versions [:http2 :http1]
                      :ssl-context test-ssl/server-ssl-context})]
         (is (instance? ExceptionInfo result))
-        (is (= "Some desired HTTP versions are not part of ALPN config." (ex-message result))))))
+        (is (= "Supplied SslContext with no ALPN config, but requested secure non-HTTP/1 versions that require ALPN." (ex-message result)))))
+
+    (testing "with no ALPN config but desiring only HTTP/1"
+      (let [result (try-start-server
+                    {:http-versions [:http1]
+                     :ssl-context test-ssl/server-ssl-context})]
+        (is (= :started result)))))
 
   (testing "HTTP/2 without ssl-context"
     (let [result (try-start-server
@@ -1458,6 +1464,162 @@
       (is (instance? IllegalArgumentException result))
       (is (= "use-h2c? may only be true when HTTP/2 is enabled." (ex-message result))))))
 
+(defn ex-root-cause [e]
+  (if-let [c (ex-cause e)]
+    (recur c)
+    e))
+
+(defn try-request-with-pool [options]
+  (try
+    (let [^Pool pool (http/connection-pool options)]
+      (try
+        @(http-get "/" {:pool pool})
+        :success
+        (finally
+          (.shutdown pool))))
+    (catch Exception e
+      ;; Exceptions raised during connection creation are wrapped in multiple levels of
+      ;; `RuntimeException` which we don't care about here.
+      (ex-root-cause e))))
+
+(deftest test-http-versions-client-config
+  (with-http-ssl-servers echo-handler {}
+    (testing "ssl-context as options map"
+
+      (testing "with different HTTP versions in ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2]
+                        :ssl-context (assoc test-ssl/client-ssl-context-opts
+                                            :application-protocol-config
+                                            (netty/application-protocol-config [:http1]))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "Some desired HTTP versions are not part of ALPN config." (ex-message result)))))
+
+      (testing "with different preference order in ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context (assoc test-ssl/client-ssl-context-opts
+                                            :application-protocol-config
+                                            (netty/application-protocol-config [:http1 :http2]))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "Desired HTTP version preference order differs from ALPN config." (ex-message result)))))
+
+      (testing "with extra HTTP versions in the ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http1]
+                        :ssl-context (assoc test-ssl/client-ssl-context-opts
+                                            :application-protocol-config
+                                            (netty/application-protocol-config [:http1 :http2]))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "ALPN config contains more HTTP versions than desired." (ex-message result)))))
+
+      (testing "with matching ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context (assoc test-ssl/client-ssl-context-opts
+                                            :application-protocol-config
+                                            (netty/application-protocol-config [:http2 :http1]))}})]
+          (is (= :success result))))
+
+      (testing "with no ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context test-ssl/client-ssl-context-opts}})]
+          (is (= :success result)))))
+
+    (testing "ssl-context as SslContext instance"
+
+      (testing "with different HTTP versions in ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2]
+                        :ssl-context (netty/ssl-client-context
+                                      (assoc test-ssl/client-ssl-context-opts
+                                             :application-protocol-config
+                                             (netty/application-protocol-config [:http1])))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "Some desired HTTP versions are not part of ALPN config." (ex-message result)))))
+
+      (testing "with different preference order in ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context (netty/ssl-client-context
+                                      (assoc test-ssl/client-ssl-context-opts
+                                             :application-protocol-config
+                                             (netty/application-protocol-config [:http1 :http2])))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "Desired HTTP version preference order differs from ALPN config." (ex-message result)))))
+
+      (testing "with extra HTTP versions in the ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http1]
+                        :ssl-context (netty/ssl-client-context
+                                      (assoc test-ssl/client-ssl-context-opts
+                                             :application-protocol-config
+                                             (netty/application-protocol-config [:http1 :http2])))}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "ALPN config contains more HTTP versions than desired." (ex-message result)))))
+
+      (testing "with matching ALPN config"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context (netty/ssl-client-context
+                                      (assoc test-ssl/client-ssl-context-opts
+                                             :application-protocol-config
+                                             (netty/application-protocol-config [:http2 :http1])))}})]
+          (is (= :success result))))
+
+      (testing "with no ALPN config but desiring HTTP/2"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http2 :http1]
+                        :ssl-context test-ssl/client-ssl-context}})]
+          (is (instance? ExceptionInfo result))
+          (is (= "Supplied SslContext with no ALPN config, but requested secure non-HTTP/1 versions that require ALPN." (ex-message result)))))
+
+      (testing "with no ALPN config but desiring only HTTP/1"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:http-versions [:http1]
+                        :ssl-context test-ssl/client-ssl-context}})]
+          (is (= :success result)))))
+
+    (testing "HTTP/2 without ssl-context"
+      (let [result (try-request-with-pool
+                    {:connection-options
+                     {:http-versions [:http2]
+                      ;; We only want assert that not providing `:ssl-context` is possible but since
+                      ;; the test server uses a self-signed certificate, we cannot verify it with
+                      ;; the default one. Since all we care about is ALPN here, this is fine.
+                      :insecure? true}})]
+        (is (= :success result)))))
+
+  (with-http2-server echo-handler {:use-h2c? true :ssl-context nil}
+    (with-redefs [*use-tls-requests* false]
+      (testing "h2c with HTTP/2"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:force-h2c? true
+                        :http-versions [:http2]}})]
+          (is (= :success result))))
+
+      (testing "h2c without HTTP/2"
+        (let [result (try-request-with-pool
+                      {:connection-options
+                       {:force-h2c? true
+                        :http-versions [:http1]}})]
+          (is (instance? IllegalArgumentException result))
+          (is (= "force-h2c? may only be true when HTTP/2 is enabled." (ex-message result))))))))
+
+
 (deftest test-in-flight-request-cancellation
   (let [conn-established (promise)
         conn-closed (promise)]