From acc7c47ce745a99b973b54a1acf996cb7dc002a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Stroi=C5=84ski?= Date: Sat, 27 Jul 2024 11:50:06 +0200 Subject: [PATCH 1/2] Test for multiple requests with cert in InputStream Both testing contexts are failing. The serial one is to demonstrate that the InputStream cannot be read twice without resetting, which obviously is not done by Netty/Aleph. This is also the case in the concurrent context, which was intended to resemble the original report in #728 and is a more likely scenario, since it doesn't disable keep-alive. IIUC, the concurrent scenario could fail in an even more unpleasant way, if the test certificate file was greater than the 8192-byte buffer used to read it, but ours is not (the fix would be the same). NB: `with-http-ssl-servers` already runs things twice, so `repeatedly` is not required to make it fail, but that would be harder to read and wouldn't cover (at some level, at least) both servers. --- test/aleph/http_test.clj | 31 +++++++++++++++++++++++++++++++ test/ca_cert.pem | 23 +++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 test/ca_cert.pem diff --git a/test/aleph/http_test.clj b/test/aleph/http_test.clj index f04b6d2a..bf14361b 100644 --- a/test/aleph/http_test.clj +++ b/test/aleph/http_test.clj @@ -452,6 +452,37 @@ :body bs/to-string)))))) +(deftest using-input-stream-as-ssl-context-trust-store + (let [num-requests 2 + file-name "test/ca_cert.pem" + client-options (fn [stream] + {:connection-options {:ssl-context {:private-key test-ssl/client-key + :certificate-chain [test-ssl/client-cert] + :trust-store stream}}}) + requests (fn [pool] + (repeatedly num-requests #(http-post "/" + {:body "hello!" + :pool pool})))] + (testing "multiple serial requests without connection reuse" + (with-open [stream (io/input-stream file-name)] + (let [client-pool (http/connection-pool (-> (client-options stream) + (assoc-in [:connection-options :keep-alive?] false)))] + (with-http-ssl-servers echo-handler {} + (is (every? + #{"hello!"} + (->> (requests client-pool) + (mapv (comp bs/to-string :body deref))))))))) + + (testing "multiple concurrent requests" + (with-open [stream (io/input-stream file-name)] + (let [client-pool (http/connection-pool (client-options stream))] + (with-http-ssl-servers echo-handler {} + (is (every? + #{"hello!"} + (->> (requests client-pool) + (doall) + (mapv (comp bs/to-string :body deref))))))))))) + (defn ssl-session-capture-handler [ssl-session-atom] (fn [req] (reset! ssl-session-atom (http.core/ring-request-ssl-session req)) diff --git a/test/ca_cert.pem b/test/ca_cert.pem new file mode 100644 index 00000000..798fa173 --- /dev/null +++ b/test/ca_cert.pem @@ -0,0 +1,23 @@ +-----BEGIN CERTIFICATE----- +MIIDyjCCArKgAwIBAgIJAPj8IfB83MXVMA0GCSqGSIb3DQEBCwUAMHIxCzAJBgNV +BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRIwEAYDVQQHDAlDdXBlcnRpbm8x +GDAWBgNVBAoMD0JGUCBDb3Jwb3JhdGlvbjEOMAwGA1UECwwFQWxlcGgxEDAOBgNV +BAMMB1Jvb3QgQ0EwHhcNMTYxMTIxMjEzMTIzWhcNMzcwMjI0MjEzMTIzWjByMQsw +CQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTESMBAGA1UEBwwJQ3VwZXJ0 +aW5vMRgwFgYDVQQKDA9CRlAgQ29ycG9yYXRpb24xDjAMBgNVBAsMBUFsZXBoMRAw +DgYDVQQDDAdSb290IENBMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA +1kKISz7cCJIU7pk+JBOH8+6UfvtR7BS1hTkWMw+IsTa9O1EJJqEtiJZTF267nLog ++jfUr8AHSTR+qtKkbs77XrOMlaa6Zyq3Z2d/p8R3oUdurg6T3JECGwilYDsEMLNL +XnqnUdkeWQJ7ea7UzgJ7ACZ61I4+Dv9xJQ+5BGMRkH+SUTDQ/um8UmrPxbDDljR7 +TbTY7WtAPbxbALrEKA5EfNS1vdcYCfguN0BUcHaHEiBDAIU7IXZigdPBnSTDHhqB +YHjmgQZ9U/ojrvmjG9lsG6X5WGj5H1SZCmpWbp+WiNEgHckzhRkCKU5V53mpqcrF +Q5WJjAHGQrBF7CD1IUj6VwIDAQABo2MwYTAdBgNVHQ4EFgQUHZFU7TsvVmLorae0 +LntY0bhIRwIwHwYDVR0jBBgwFoAUHZFU7TsvVmLorae0LntY0bhIRwIwDwYDVR0T +AQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwDQYJKoZIhvcNAQELBQADggEBACfu +Sp0gy8QI1BP6bAueT6/t7Nz2Yg2kwbIXac5sanLc9MjhjG/EjLrkwhCpEVEfFrKD +Bl/s0wdYoHcVTDlev4H3QOM4WeciaSUsEytihhey72f89ZyvQ+FGbif2BXNk4kPN +0eo3t5TXS8Fw/iBi371KZo4jTpdsB0Y3fwKtXw8ieUAlaF86yGHA9bMF7eGXorpS +hEJ8JRWWy2pV9WtkYw+tBWj7PtXQAIUx4t+J3+B9pSUyHxxArKmZUKa3GpJzBAKX +TLHddtadJLqptjZ6pq7OSiihAs3fxVF+TGDJyPyk8K48y9G2MinrYXVzKHeQWqPT +rO0jz1F4FL9LiD+HwLc= +-----END CERTIFICATE----- From 4414396373eec1107cf111888d125005523a636a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Stroi=C5=84ski?= Date: Sat, 27 Jul 2024 12:08:16 +0200 Subject: [PATCH 2/2] Lift ssl-context coercion to connection-pool fn As suggested by @DerGuteMoritz in #728. This fixes the issue and makes the test added in the previous commit pass. Keeping the `client-ssl-context` call in `http-connection` as is, even though it might seem superfluous considering the code path taken in the test, but `http-connection` is a public API, so we have to keep the call (which for us is a no-op, if we ignore the repeated ALPN check) even for our case when the protocol is https and `ssl-context` is supplied. NOTE: This highlights a difference we are introducing here. Previously, if we specified ssl-context, but the protocol wasn't https, we would just ignore the ssl-context. Currently, we are coercing it ahead-of-time, before knowing the request protocol. This could be alleviated by wrapping the coercion in a `delay`, so it wouldn't happen until needed. Yet, given how unlikely this scenario seems, I have doubts whether it'd be worth it. I slightly dislike the repetition of `[:http1]` default value, but since it serves as documentation in `http-connection`, I decided to keep it as is rather than to extract it out. Also, I slightly dislike the repetition of a pattern to call `ensure-consistent-alpn-config` and then `coerce-ssl-client-context` but it's only now in 2 places, which I think is a better alternative than adding yet another ssl-coercion layer/wrapping function. Obviously, we cannot just move `ensure-consistent-alpn-config` to `ssl-client-context`, since ALPN is only for HTTP. --- src/aleph/http.clj | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/aleph/http.clj b/src/aleph/http.clj index 08efc6a4..08ad8cf4 100644 --- a/src/aleph/http.clj +++ b/src/aleph/http.clj @@ -229,7 +229,10 @@ (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) + (let [{:keys [log-activity + ssl-context + http-versions] + :or {http-versions [:http1]}} connection-options dns-options' (if-not (and (some? dns-options) (not (or (contains? dns-options :transport) (contains? dns-options :epoll?)))) @@ -242,7 +245,13 @@ (assoc :name-resolver (netty/dns-resolver-group dns-options')) (some? log-activity) - (assoc :log-activity (netty/activity-logger "aleph-client" log-activity))) + (assoc :log-activity (netty/activity-logger "aleph-client" log-activity)) + + (some? ssl-context) + (update :ssl-context + #(-> % + (common/ensure-consistent-alpn-config http-versions) + (netty/coerce-ssl-client-context)))) p (promise) create-pool-fn (or pool-builder-fn flow/instrumented-pool)