Repository: clj-commons/aleph
Pull request #689 (2023-09-18 09:43:40)

Enable TLS endpoint identification by default (breaking)
A few review notes:

1. See commit message for overall intent
2. I would have preferred to make the option part of `:ssl-context` instead having it sit next to it since conceptually, it belongs there. The reason for not doing this is that we also support passing in an `SslContext` object. Since Netty doesn't offer an API for setting an endpoint identification algorithm on the context, this would make it impossible to reach for users doing so, forcing them to convert their `SslContext` into an options map (which for some reason or another they might not be able to do). Feel free to disagree on this tradeoff, of course.
3. In the tests, I had to give up on `netty/self-signed-ssl-context` because I needed to be able to reach the certificate to properly configure the client's trust store (which in turn allowed me to remove the `:insecure? true`). Unfortunately, `SslContext` has no API for that. I guess we could still keep `netty/self-signed-ssl-context` around since it's exposed as public API. However, perhaps we should also offer an arity for passing in a hostname now?
4. Also in the tests you'll notice that I changed the default connection options to `:http-versions [:http1]`. This is because I wanted to override (only) the `:trust-store` option in `:ssl-context` which means I don't get the "good default" `:application-protocol-config` provided in `aleph.http.client/client-ssl-context` anymore. Perhaps we could lift that default a bit higher so that it also applies when passing in an `:ssl-context` map without `:application-protocol-config`?


Patch:
diff --git a/src/aleph/http.clj b/src/aleph/http.clj
index eac8361..0746a01 100644
--- a/src/aleph/http.clj
+++ b/src/aleph/http.clj
@@ -120,6 +120,7 @@
    Param key                   | Description
    | ---                       | ---
    | `ssl-context`             | an `io.netty.handler.ssl.SslContext` object or a map of SSL context options (see `aleph.netty/ssl-client-context` for more details), only required if a custom context is required
+   | `ssl-endpoint-id-alg`     | the name of the algorithm to use for SSL endpoint identification (see https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms), defaults to \"HTTPS\". Only used for HTTPS connections. Pass `nil` to disable endpoint identification.
    | `local-address`           | an optional `java.net.SocketAddress` describing which local interface should be used
    | `bootstrap-transform`     | a function that takes an `io.netty.bootstrap.Bootstrap` object and modifies it.
    | `pipeline-transform`      | a function that takes an `io.netty.channel.ChannelPipeline` object, which represents a connection, and modifies it.
@@ -229,6 +230,7 @@
    | `raw-stream?`         | if `true`, the connection will emit raw `io.netty.buffer.ByteBuf` objects rather than strings or byte-arrays.  This will minimize copying, but means that care must be taken with Netty's buffer reference counting.  Only recommended for advanced users.
    | `insecure?`           | if `true`, the certificates for `wss://` will be ignored.
    | `ssl-context`         | an `io.netty.handler.ssl.SslContext` object, only required if a custom context is required
+   | `ssl-endpoint-id-alg` | the name of the algorithm to use for SSL endpoint identification (see https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms), defaults to \"HTTPS\". Only used for WSS connections. Pass `nil` to disable endpoint identification.
    | `extensions?`         | if `true`, the websocket extensions will be supported.
    | `sub-protocols`       | a string with a comma seperated list of supported sub-protocols.
    | `headers`             | the headers that should be included in the handshake
diff --git a/src/aleph/http/client.clj b/src/aleph/http/client.clj
index 7ed1267..66c0da4 100644
--- a/src/aleph/http/client.clj
+++ b/src/aleph/http/client.clj
@@ -683,14 +683,14 @@
 
    Can't use an ApnHandler/ApplicationProtocolNegotiationHandler here,
    because it's tricky to run Manifold code on Netty threads."
-  [{:keys [ssl? remote-address ssl-context]}]
+  [{:keys [ssl? remote-address ssl-context ssl-endpoint-id-alg]}]
   (fn pipeline-builder
     [^ChannelPipeline pipeline]
     (when ssl?
       (do
         (.addLast pipeline
                   "ssl-handler"
-                  (netty/ssl-handler (.channel pipeline) ssl-context remote-address))
+                  (netty/ssl-handler (.channel pipeline) ssl-context remote-address ssl-endpoint-id-alg))
         (.addLast pipeline
                   "pause-handler"
                   ^ChannelHandler (netty/pause-handler))))))
@@ -954,6 +954,7 @@
            keep-alive?
            insecure?
            ssl-context
+           ssl-endpoint-id-alg
            response-buffer-size
            epoll?
            transport
@@ -966,6 +967,7 @@
            bootstrap-transform  identity
            pipeline-transform   identity
            keep-alive?          true
+           ssl-endpoint-id-alg  netty/default-ssl-endpoint-id-alg
            response-buffer-size 65536
            epoll?               false
            name-resolver        :default
@@ -999,6 +1001,7 @@
                            (assoc opts
                                   :ssl? ssl?
                                   :ssl-context ssl-context
+                                  :ssl-endpoint-id-alg ssl-endpoint-id-alg
                                   :remote-address remote-address
                                   :raw-stream? raw-stream?
                                   :response-buffer-size response-buffer-size
diff --git a/src/aleph/http/websocket/client.clj b/src/aleph/http/websocket/client.clj
index f586ba6..df6b14c 100644
--- a/src/aleph/http/websocket/client.clj
+++ b/src/aleph/http/websocket/client.clj
@@ -232,6 +232,7 @@
    {:keys [raw-stream?
            insecure?
            ssl-context
+           ssl-endpoint-id-alg
            headers
            local-address
            bootstrap-transform
@@ -248,6 +249,7 @@
            pipeline-transform  identity
            raw-stream?         false
            epoll?              false
+           ssl-endpoint-id-alg netty/default-ssl-endpoint-id-alg
            sub-protocols       nil
            extensions?         false
            max-frame-payload   65536
@@ -294,7 +296,7 @@
                            (when ssl?
                              (.addLast p
                                        "ssl-handler"
-                                       (netty/ssl-handler (.channel p) ssl-context remote-address)))
+                                       (netty/ssl-handler (.channel p) ssl-context remote-address ssl-endpoint-id-alg)))
                            (.addLast p "http-client" (HttpClientCodec.))
                            (.addLast p "aggregator" (HttpObjectAggregator. 16384))
                            (.addLast p "websocket-frame-aggregator" (WebSocketFrameAggregator. max-frame-size))
diff --git a/src/aleph/netty.clj b/src/aleph/netty.clj
index 5c7f2b6..6a04aa3 100644
--- a/src/aleph/netty.clj
+++ b/src/aleph/netty.clj
@@ -1078,10 +1078,12 @@
 
 (defn self-signed-ssl-context
   "A self-signed SSL context for servers."
-  []
-  (let [cert (SelfSignedCertificate.)]
-    (ssl-server-context {:private-key       (.privateKey cert)
-                         :certificate-chain (.certificate cert)})))
+  ([]
+   (self-signed-ssl-context "localhost"))
+  ([hostname]
+   (let [cert (SelfSignedCertificate. hostname)]
+     (ssl-server-context {:private-key       (.privateKey cert)
+                          :certificate-chain (.certificate cert)}))))
 
 (defn insecure-ssl-client-context
   "An insure SSL context for servers."
@@ -1374,33 +1376,47 @@ initialize an DnsAddressResolverGroup instance.
   anyway."
   [_])
 
+(def ^:const ^:no-doc default-ssl-endpoint-id-alg "HTTPS")
+
 (defn ssl-handler
   "Generates a new SslHandler for the given SslContext.
 
-   The 2-arity version is for the server.
-   The 3-arity version is for the client. The `remote-address` must be provided"
-  (^ChannelHandler
+   The 2-ary version is for servers.
+
+   The 3- and 4-ary version are for clients.
+   For these, the `remote-address` must be provided.
+   The `ssl-endpoint-id-alg` is the name of the algorithm to use for endpoint identification (see https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms). It defaults to \"HTTPS\" in the 3-ary version which is a reasonable default for non-HTTPS uses, too. Pass `nil` to disable endpoint identification."
+  (^SslHandler
    [^Channel ch ^SslContext ssl-ctx]
    (.newHandler ssl-ctx
                 (.alloc ch)))
-  (^ChannelHandler
+  (^SslHandler
    [^Channel ch ^SslContext ssl-ctx ^InetSocketAddress remote-address]
-   (.newHandler ssl-ctx
-                (.alloc ch)
-                (.getHostName ^InetSocketAddress remote-address)
-                (.getPort ^InetSocketAddress remote-address))))
+   (ssl-handler ch ssl-ctx remote-address default-ssl-endpoint-id-alg))
+  (^SslHandler
+   [^Channel ch ^SslContext ssl-ctx ^InetSocketAddress remote-address ssl-endpoint-id-alg]
+   (let [ssl-handler (.newHandler ssl-ctx
+                                  (.alloc ch)
+                                  (.getHostName ^InetSocketAddress remote-address)
+                                  (.getPort ^InetSocketAddress remote-address))]
+     (when ssl-endpoint-id-alg
+       (let [ssl-engine (.engine ssl-handler)
+             ssl-params (.getSSLParameters ssl-engine)]
+         (.setEndpointIdentificationAlgorithm ssl-params ssl-endpoint-id-alg)
+         (.setSSLParameters ssl-engine ssl-params)))
+     ssl-handler)))
 
 (defn- add-ssl-handler-to-pipeline-builder
   "Adds an `SslHandler` to the pipeline builder."
   ([pipeline-builder ssl-ctx]
-   (add-ssl-handler-to-pipeline-builder pipeline-builder ssl-ctx nil))
-  ([pipeline-builder ssl-ctx remote-address]
+   (add-ssl-handler-to-pipeline-builder pipeline-builder ssl-ctx nil nil))
+  ([pipeline-builder ssl-ctx remote-address ssl-endpoint-id-alg]
    (fn [^ChannelPipeline p]
      (.addFirst p
                 "ssl-handler"
                 (let [ch (.channel p)]
                   (if remote-address
-                    (ssl-handler ch ssl-ctx remote-address)
+                    (ssl-handler ch ssl-ctx remote-address ssl-endpoint-id-alg)
                     (ssl-handler ch ssl-ctx))))
      (pipeline-builder p))))
 
@@ -1471,6 +1487,22 @@ initialize an DnsAddressResolverGroup instance.
   ([pipeline-builder
     ssl-context
     bootstrap-transform
+    remote-address
+    local-address
+    epoll?
+    name-resolver]
+   (create-client pipeline-builder
+                  ssl-context
+                  default-ssl-endpoint-id-alg
+                  bootstrap-transform
+                  remote-address
+                  local-address
+                  epoll?
+                  name-resolver))
+  ([pipeline-builder
+    ssl-context
+    ssl-endpoint-id-alg
+    bootstrap-transform
     ^SocketAddress remote-address
     ^SocketAddress local-address
     epoll?
@@ -1480,7 +1512,7 @@ initialize an DnsAddressResolverGroup instance.
                        (coerce-ssl-client-context ssl-context))
 
          pipeline-builder (if ssl-context
-                            (add-ssl-handler-to-pipeline-builder pipeline-builder ssl-context remote-address)
+                            (add-ssl-handler-to-pipeline-builder pipeline-builder ssl-context remote-address ssl-endpoint-id-alg)
                             pipeline-builder)]
      (create-client-chan {:pipeline-builder    pipeline-builder
                           :bootstrap-transform bootstrap-transform
diff --git a/src/aleph/tcp.clj b/src/aleph/tcp.clj
index 9273062..c944faa 100644
--- a/src/aleph/tcp.clj
+++ b/src/aleph/tcp.clj
@@ -162,14 +162,16 @@
    | `remote-address`      | a `java.net.SocketAddress` specifying the server's address.
    | `local-address`       | a `java.net.SocketAddress` specifying the local network interface to use.
    | `ssl-context`         | an explicit `io.netty.handler.ssl.SslHandler` or a map of SSL context options (see `aleph.netty/ssl-client-context` for more details) to use. Defers to `ssl?` and `insecure?` configuration if omitted.
+   | `ssl-endpoint-id-alg` | the name of the algorithm to use for SSL endpoint identification (see https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms), defaults to \"HTTPS\" which is a reasonable default for non-HTTPS uses, too. Only used by SSL connections. Pass `nil` to disable endpoint identification.
    | `ssl?`                | if true, the client attempts to establish a secure connection with the server.
    | `insecure?`           | if true, the client will ignore the server's certificate.
    | `bootstrap-transform` | a function that takes an `io.netty.bootstrap.Bootstrap` object, which represents the client, and modifies it.
    | `pipeline-transform`  | a function that takes an `io.netty.channel.ChannelPipeline` object, which represents a connection, and modifies it.
    | `raw-stream?`         | if true, messages from the stream will be `io.netty.buffer.ByteBuf` objects rather than byte-arrays.  This will minimize copying, but means that care must be taken with Netty's buffer reference counting.  Only recommended for advanced users.
    | `transport`           | the transport to use, one of `:nio`, `:epoll`, `:kqueue` or `:io-uring` (defaults to `:nio`)."
-  [{:keys [host port remote-address local-address ssl-context ssl? insecure? pipeline-transform bootstrap-transform epoll? transport]
-    :or {bootstrap-transform identity
+  [{:keys [host port remote-address local-address ssl-context ssl-endpoint-id-alg ssl? insecure? pipeline-transform bootstrap-transform epoll? transport]
+    :or {ssl-endpoint-id-alg netty/default-ssl-endpoint-id-alg
+         bootstrap-transform identity
          epoll? false}
     :as options}]
   (let [[s ^ChannelHandler handler] (client-channel-handler options)
@@ -185,7 +187,7 @@
                            (when ssl?
                              (.addLast pipeline
                                        "ssl-handler"
-                                       (netty/ssl-handler (.channel pipeline) ssl-context remote-address)))
+                                       (netty/ssl-handler (.channel pipeline) ssl-context remote-address ssl-endpoint-id-alg)))
                            (.addLast pipeline "handler" handler)
                            (when pipeline-transform
                              (pipeline-transform pipeline)))]


Test patch:
diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj
index 574df24..c1842da 100644
--- a/test/aleph/http_test.clj
+++ b/test/aleph/http_test.clj
@@ -186,9 +186,16 @@
     (apply concat)
     (partition 2)))
 
+(def default-ssl-server-options
+  {:port port
+   :ssl-context ssl/server-ssl-context})
+
+(def default-ssl-client-options
+  {:ssl-context ssl/client-ssl-context-opts})
+
 (defmacro with-server [server & body]
   `(let [server# ~server]
-     (binding [*pool* (http/connection-pool {:connection-options (merge *connection-options* {:insecure? true})})]
+     (binding [*pool* (http/connection-pool {:connection-options *connection-options*})]
        (try
          ~@body
          (finally
@@ -207,7 +214,7 @@
      (with-server (http/start-server ~handler {:port port :compression-level 3 :shutdown-timeout 0})
        ~@body)))
 
-(def default-ssl-options {:port port, :ssl-context (netty/self-signed-ssl-context)})
+
 
 (defmacro with-handler-options
   [handler options & body]
@@ -247,14 +254,15 @@
 
 
 (deftest test-ssl-response-formats
-  (with-handler-options basic-handler default-ssl-options
-    (doseq [[path result] expected-results]
-      (is
-       (= result
-          (bs/to-string
-           (:body
-            @(http-get (str "https://localhost:" port "/" path)))))
-       (str path "path failed")))))
+  (binding [*connection-options* default-ssl-client-options]
+    (with-handler-options basic-handler default-ssl-server-options
+      (doseq [[path result] expected-results]
+        (is
+         (= result
+            (bs/to-string
+             (:body
+              @(http-get (str "https://localhost:" port "/" path)))))
+         (str path "path failed"))))))
 
 (deftest test-files
   (let [client-url (str "http://localhost:" port)]
@@ -269,20 +277,17 @@
                                 {:body (io/file "test/file.txt")}))))))))
 
 (deftest test-ssl-files
-  (let [client-url (str "https://localhost:" port)
-        client-options {:connection-options {:ssl-context ssl/client-ssl-context}}
-        client-pool    (http/connection-pool client-options)]
-    (with-handler-options identity (merge default-ssl-options {:ssl-context ssl/server-ssl-context})
-      (is (str/blank?
-           (bs/to-string
-            (:body @(http-put client-url
-                              {:body (io/file "test/empty.txt")
-                               :pool client-pool})))))
-      (is (= (slurp "test/file.txt" :encoding "UTF-8")
+  (binding [*connection-options* default-ssl-client-options]
+    (let [client-url (str "https://localhost:" port)]
+      (with-handler-options identity default-ssl-server-options
+        (is (str/blank?
              (bs/to-string
               (:body @(http-put client-url
-                                {:body (io/file "test/file.txt")
-                                 :pool client-pool}))))))))
+                                {:body (io/file "test/empty.txt")})))))
+        (is (= (slurp "test/file.txt" :encoding "UTF-8")
+               (bs/to-string
+                (:body @(http-put client-url
+                                  {:body (io/file "test/file.txt")})))))))))
 
 (defn ssl-session-capture-handler [ssl-session-atom]
   (fn [req]
@@ -290,28 +295,51 @@
     {:status 200 :body "ok"}))
 
 (deftest test-ssl-session-access
-  (let [ssl-session (atom nil)]
-    (with-handler-options
-      (ssl-session-capture-handler ssl-session)
-      default-ssl-options
-      (is (= 200 (:status @(http-get (str "https://localhost:" port)))))
-      (is (some? @ssl-session))
-      (when-let [^SSLSession s @ssl-session]
-        (is (.isValid s))
-        (is (not (str/includes? "NULL" (.getCipherSuite s))))))))
+  (binding [*connection-options* default-ssl-client-options]
+    (let [ssl-session (atom nil)]
+      (with-handler-options
+        (ssl-session-capture-handler ssl-session)
+        default-ssl-server-options
+        (is (= 200 (:status @(http-get (str "https://localhost:" port)))))
+        (is (some? @ssl-session))
+        (when-let [^SSLSession s @ssl-session]
+          (is (.isValid s))
+          (is (not (str/includes? "NULL" (.getCipherSuite s)))))))))
 
 (deftest test-ssl-with-plain-client-request
-  (let [ssl-session (atom nil)]
-    (with-handler-options
-      (ssl-session-capture-handler ssl-session)
-      default-ssl-options
-      ;; Note the intentionally wrong "http" scheme here
-      (is (some-> (http-get (str "http://localhost:" port))
-                  (d/catch identity)
-                  deref
-                  ex-message
-                  (str/includes? "connection was closed")))
-      (is (nil? @ssl-session)))))
+  (binding [*connection-options* default-ssl-client-options]
+    (let [ssl-session (atom nil)]
+      (with-handler-options
+        (ssl-session-capture-handler ssl-session)
+        default-ssl-server-options
+        ;; Note the intentionally wrong "http" scheme here
+        (is (some-> (http-get (str "http://localhost:" port))
+                    (d/catch identity)
+                    deref
+                    ex-message
+                    (str/includes? "connection was closed")))
+        (is (nil? @ssl-session))))))
+
+(deftest test-ssl-endpoint-identification
+  (binding [*connection-options* {:ssl-context ssl/wrong-hostname-client-ssl-context-opts}]
+    (let [ssl-session (atom nil)]
+      (with-handler-options
+        (ssl-session-capture-handler ssl-session)
+        (assoc default-ssl-server-options :ssl-context ssl/wrong-hostname-server-ssl-context-opts)
+        (is (thrown-with-msg? javax.net.ssl.SSLHandshakeException
+                              #"^No name matching localhost found$"
+                              @(http-get (str "https://localhost:" port))))
+        (is (nil? @ssl-session))))))
+
+(deftest test-disabling-ssl-endpoint-identification
+  (binding [*connection-options* {:ssl-context ssl/wrong-hostname-client-ssl-context-opts
+                                  :ssl-endpoint-id-alg nil}]
+    (let [ssl-session (atom nil)]
+      (with-handler-options
+        (ssl-session-capture-handler ssl-session)
+        (assoc default-ssl-server-options :ssl-context ssl/wrong-hostname-server-ssl-context-opts)
+        (is (= 200 (:status @(http-get (str "https://localhost:" port)))))
+        (is (some? @ssl-session))))))
 
 (deftest test-invalid-body
   (let [client-url (str "http://localhost:" port)]
diff --git a/test/aleph/ssl.clj b/test/aleph/ssl.clj
index c678f7a..7c357ae 100644
--- a/test/aleph/ssl.clj
+++ b/test/aleph/ssl.clj
@@ -8,6 +8,7 @@
       ApplicationProtocolConfig$SelectedListenerFailureBehavior
       ApplicationProtocolConfig$SelectorFailureBehavior
       ApplicationProtocolNames)
+    (io.netty.handler.ssl.util SelfSignedCertificate)
     (java.io ByteArrayInputStream)
     (java.security KeyFactory PrivateKey)
     (java.security.cert CertificateFactory X509Certificate)
@@ -75,3 +76,14 @@
 
 (def client-ssl-context
   (netty/ssl-client-context client-ssl-context-opts))
+
+(def wrong-hostname-cert
+  (SelfSignedCertificate. "some-random-hostname"))
+
+(def wrong-hostname-server-ssl-context-opts
+  {:private-key       (.privateKey wrong-hostname-cert)
+   :certificate-chain (.certificate wrong-hostname-cert)})
+
+(def wrong-hostname-client-ssl-context-opts
+  (assoc client-ssl-context-opts
+         :trust-store (.certificate wrong-hostname-cert)))
diff --git a/test/aleph/tcp_ssl_test.clj b/test/aleph/tcp_ssl_test.clj
index cc2ebad..ed410ea 100644
--- a/test/aleph/tcp_ssl_test.clj
+++ b/test/aleph/tcp_ssl_test.clj
@@ -69,6 +69,19 @@
         (is (nil? @(s/take! c)))
         (is (nil? @ssl-session) "SSL session should be undefined")))))
 
+(deftest test-failed-endpoint-identification
+  (let [ssl-session (atom nil)]
+    (with-server (tcp/start-server (ssl-echo-handler ssl-session)
+                                   {:port 10001
+                                    :shutdown-timeout 0
+                                    :ssl-context ssl/wrong-hostname-server-ssl-context-opts})
+      (is (thrown-with-msg? javax.net.ssl.SSLHandshakeException
+                            #"^No name matching localhost found$"
+                            @(tcp/client {:host "localhost"
+                                          :port 10001
+                                          :ssl-context ssl/wrong-hostname-client-ssl-context-opts})))
+      (is (nil? @ssl-session) "SSL session should be undefined"))))
+
 (deftest test-connection-close-during-ssl-handshake
   (let [ssl-session (atom nil)
         connection-closed (promise)