From 41f1a8c1b45ff11f677898da4f3618090b63caf6 Mon Sep 17 00:00:00 2001 From: Moritz Heidkamp Date: Thu, 13 Jun 2024 16:20:20 +0200 Subject: [PATCH 1/4] Allow custom `SslContext` without ALPN again for HTTP/1.1-only case With the introduction of HTTP/2 support, Aleph started to require a matching ALPN config to be present in custom `SslContext` objects. This needlessly broke existing HTTP/1.1-only uses. With this change, we now allow custom `SslContext` objects without ALPN config again if only HTTP/1.1 is desired (via the `http-versions` option). Since this still happens to be the default, existing uses should just work again. Fixes #727 --- src/aleph/http/common.clj | 29 ++++++++++++++++++++--------- test/aleph/http_test.clj | 12 +++++++++--- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/aleph/http/common.clj b/src/aleph/http/common.clj index d6262471..7898401f 100644 --- a/src/aleph/http/common.clj +++ b/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 "No ALPN supplied, but requested 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 diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj index 30778ef7..e5cd2173 100644 --- a/test/aleph/http_test.clj +++ b/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 (= "No ALPN supplied, but requested 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 From d024fe77e4835bcfe0790c62019bb23849013936 Mon Sep 17 00:00:00 2001 From: Moritz Heidkamp Date: Fri, 14 Jun 2024 14:31:39 +0200 Subject: [PATCH 2/4] Add `test-http-versions-client-config` This is a mirror of `test-http-versions-server-config` to test various permutations of the `http-versions` option in combination with `ssl-context` and `force-h2c?`. --- test/aleph/http_test.clj | 148 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj index e5cd2173..153b4aaa 100644 --- a/test/aleph/http_test.clj +++ b/test/aleph/http_test.clj @@ -1464,6 +1464,154 @@ (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 (= "No ALPN supplied, but requested 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))))))) + + (deftest test-in-flight-request-cancellation (let [conn-established (promise) conn-closed (promise)] From e57806d1a1c4ff201dd37c7289b0b742f6feb6b2 Mon Sep 17 00:00:00 2001 From: Moritz Heidkamp Date: Fri, 14 Jun 2024 14:33:26 +0200 Subject: [PATCH 3/4] Only allow `force-h2c?` when `:http2` is present in `http-versions` --- src/aleph/http.clj | 14 ++++++++++---- test/aleph/http_test.clj | 10 +++++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/aleph/http.clj b/src/aleph/http.clj index 06b70587..8231dc76 100644 --- a/src/aleph/http.clj +++ b/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) diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj index 153b4aaa..a0cc2133 100644 --- a/test/aleph/http_test.clj +++ b/test/aleph/http_test.clj @@ -1609,7 +1609,15 @@ {:connection-options {:force-h2c? true :http-versions [:http2]}})] - (is (= :success result))))))) + (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 From 3cf3034091923644893b5cda47e5950efed4ebb2 Mon Sep 17 00:00:00 2001 From: Moritz Heidkamp Date: Tue, 18 Jun 2024 14:31:48 +0200 Subject: [PATCH 4/4] Improve exception message --- src/aleph/http/common.clj | 2 +- test/aleph/http_test.clj | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/aleph/http/common.clj b/src/aleph/http/common.clj index 7898401f..7e2d4f93 100644 --- a/src/aleph/http/common.clj +++ b/src/aleph/http/common.clj @@ -194,7 +194,7 @@ ssl-context) (if (= [:http1] desired-http-versions) ssl-context - (throw (ex-info "No ALPN supplied, but requested non-HTTP/1 versions that require ALPN." + (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 diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj index a0cc2133..17afb727 100644 --- a/test/aleph/http_test.clj +++ b/test/aleph/http_test.clj @@ -1429,7 +1429,7 @@ {:http-versions [:http2 :http1] :ssl-context test-ssl/server-ssl-context})] (is (instance? ExceptionInfo result)) - (is (= "No ALPN supplied, but requested non-HTTP/1 versions that require ALPN." (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 @@ -1583,7 +1583,7 @@ {:http-versions [:http2 :http1] :ssl-context test-ssl/client-ssl-context}})] (is (instance? ExceptionInfo result)) - (is (= "No ALPN supplied, but requested non-HTTP/1 versions that require ALPN." (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-request-with-pool