Improve TCP+SSL server connection handling (#608)

* Improve TCP+SSL server connection handling

When starting an TCP+SSL server, the handler would be called when the
underlying TCP connection is established but before the SSL handshake
has taken place.  At that point, the client's `:ssl-session` value is
still a placeholder (e.g. `.isValid` returns `false`, the cipher suite
is reported as `"SSL_NULL_WITH_NULL_NULL"` and the peer principal is
not available). Also, the stream is not ready for use, so any writes
would be buffered until after the handshake is complete. Since there
is no way to be notified of the handshake completion, the existing
test case worked around this by looking up the SSL session only after
having received a message for the first time which implies that the
session has been successfully established since. However, this is a
pretty awkward API and only works for protocols in which the client
actually is the first to transmit anything. There's also a bit of a
gotcha: The value of `:ssl-session` mutably changes to a different
value after the handshake, so holding on to the initial placeholder
value and only starting to use it later won't do the trick.

This patch changes the semantics so that the handler is only invoked
after the session was successfully established. While technically a
breaking change, it appears unlikely that any existing users would
rely on the original behavior since the placeholder SSL session value
is pretty much useless. The new behavior should be more in line with
user expectation and arguably is more correct (e.g. the handler
doesn't have to wait for I/O before being able to peruse the SSL
session info).

* Elaborate semantics of `ssl-context` parameter in docstring
* Add changelog entry
* Add some HTTPS server tests around SSL session handling

Author: DerGuteMoritz

CHANGES.md

@@ -1,3 +1,7 @@
+### Unreleased
+
+* Make TCP+SSL server call handler only after successful SSL handshake
+
 ### 0.5.0
 
 * Add initial clj-kondo hooks

src/aleph/tcp.clj

@@ -13,7 +13,10 @@
     [io.netty.channel
      Channel
      ChannelHandler
-     ChannelPipeline]))
+     ChannelHandlerContext
+     ChannelPipeline]
+    [io.netty.handler.ssl
+     SslHandler]))
 
 (p/def-derived-map TcpConnection [^Channel ch]
   :server-name (netty/channel-server-name ch)
@@ -25,7 +28,16 @@
 
 (defn- ^ChannelHandler server-channel-handler
   [handler {:keys [raw-stream?] :as options}]
-  (let [in (atom nil)]
+  (let [in (atom nil)
+        call-handler (fn [^ChannelHandlerContext ctx]
+                       (let [ch (.channel ctx)]
+                         (handler
+                          (doto
+                              (s/splice
+                               (netty/sink ch true netty/to-byte-buf)
+                               (reset! in (netty/source ch)))
+                            (reset-meta! {:aleph/channel ch}))
+                          (->TcpConnection ch))))]
     (netty/channel-inbound-handler
 
       :exception-caught
@@ -40,15 +52,20 @@
 
       :channel-active
       ([_ ctx]
-        (let [ch (.channel ctx)]
-          (handler
-            (doto
-              (s/splice
-                (netty/sink ch true netty/to-byte-buf)
-                (reset! in (netty/source ch)))
-              (reset-meta! {:aleph/channel ch}))
-            (->TcpConnection ch)))
-        (.fireChannelActive ctx))
+       (if-let [^SslHandler ssl-handler (-> ctx .pipeline (.get SslHandler))]
+         (-> ssl-handler
+             .handshakeFuture
+             netty/wrap-future
+             (d/on-realized (fn [_]
+                              (call-handler ctx))
+                            ;; No need to handle errors here since
+                            ;; the SSL handler will terminate the
+                            ;; whole pipeline by throwing a
+                            ;; `javax.net.ssl.SSLHandshakeException`
+                            ;; in this case anyway.
+                            (fn [_])))
+         (call-handler ctx))
+       (.fireChannelActive ctx))
 
       :channel-read
       ([_ ctx msg]
@@ -65,7 +82,7 @@
    |:---|:-----
    | `port` | the port the server will bind to.  If `0`, the server will bind to a random port.
    | `socket-address` | a `java.net.SocketAddress` specifying both the port and interface to bind to.
-   | `ssl-context` | an `io.netty.handler.ssl.SslContext` object. If a self-signed certificate is all that's required, `(aleph.netty/self-signed-ssl-context)` will suffice.
+   | `ssl-context` | an `io.netty.handler.ssl.SslContext` object. If given, the server will only accept SSL connections and call the handler once the SSL session has been successfully established. If a self-signed certificate is all that's required, `(aleph.netty/self-signed-ssl-context)` will suffice.
    | `bootstrap-transform` | a function that takes an `io.netty.bootstrap.ServerBootstrap` object, which represents the server, 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."

test/aleph/http_test.clj

@@ -2,6 +2,7 @@
   (:require
    [aleph.flow :as flow]
    [aleph.http :as http]
+   [aleph.http.core :as http.core]
    [aleph.netty :as netty]
    [aleph.ssl :as ssl]
    [aleph.tcp :as tcp]
@@ -18,7 +19,8 @@
    (io.netty.handler.codec.http HttpMessage)
    (java.io File)
    (java.util.concurrent TimeoutException)
-   (java.util.zip GZIPInputStream ZipException)))
+   (java.util.zip GZIPInputStream ZipException)
+   (javax.net.ssl SSLSession)))
 
 ;;;
 
@@ -260,6 +262,35 @@
                                 {:body (io/file "test/file.txt")
                                  :pool client-pool}))))))))
 
+(defn ssl-session-capture-handler [ssl-session-atom]
+  (fn [req]
+    (reset! ssl-session-atom (http.core/ring-request-ssl-session req))
+    {: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))))))))
+
+(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)))))
+
 (deftest test-invalid-body
   (let [client-url (str "http://localhost:" port)]
     (with-handler identity

test/aleph/tcp_ssl_test.clj

@@ -16,14 +16,8 @@
 
 (defn ssl-echo-handler [ssl-session]
   (fn [s c]
-    (s/connect
-     ;; note we need to capture the SSL session *after* we start
-     ;; reading data. Otherwise, the session might not be set up yet.
-     (s/map (fn [msg]
-              (reset! ssl-session (:ssl-session c))
-              msg)
-            s)
-     s)))
+    (reset! ssl-session (:ssl-session c))
+    (s/connect s s)))
 
 (deftest test-ssl-echo
   (let [ssl-session (atom nil)]
@@ -38,3 +32,18 @@
         (is (some? @ssl-session) "SSL session should be defined")
         (is (= (.getSubjectDN ^X509Certificate ssl/client-cert)
                (.getSubjectDN ^X509Certificate (first (.getPeerCertificates @ssl-session)))))))))
+
+(deftest test-failed-ssl-handshake
+  (let [ssl-session (atom nil)]
+    (with-server (tcp/start-server (ssl-echo-handler ssl-session)
+                                   {:port 10001
+                                    :ssl-context ssl/server-ssl-context})
+      (let [c @(tcp/client {:host "localhost"
+                            :port 10001
+                            :ssl-context (netty/ssl-client-context
+                                          ;; Note the intentionally wrong private key here
+                                          {:private-key ssl/server-key
+                                           :certificate-chain (into-array X509Certificate [ssl/client-cert])
+                                           :trust-store (into-array X509Certificate [ssl/ca-cert])})})]
+        (is (nil? @(s/take! c)))
+        (is (nil? @ssl-session) "SSL session should be undefined")))))