From 251d859d10bbcdc9353e213857c0b5a81ae62012 Mon Sep 17 00:00:00 2001 From: Chris Price Date: Fri, 17 Oct 2014 23:35:05 -0700 Subject: [PATCH] (TK-97) Expose configuration settings for protocols/cipher suites This commit adds configuration settings for the SSL protocols and cipher suites, in both the java and clojure clients. It also adds a list of default protocols which will be used if the protocols setting is not explicitly set. --- src/clj/puppetlabs/http/client/async.clj | 36 ++++-- src/clj/puppetlabs/http/client/common.clj | 22 ++-- .../http/client/RequestOptions.java | 21 ++++ .../client/impl/CoercedRequestOptions.java | 14 +++ .../http/client/impl/JavaClient.java | 21 +++- .../http/client/async_ssl_config_test.clj | 16 +-- test/puppetlabs/http/client/sync_ssl_test.clj | 116 +++++++++++++++++- 7 files changed, 213 insertions(+), 33 deletions(-) diff --git a/src/clj/puppetlabs/http/client/async.clj b/src/clj/puppetlabs/http/client/async.clj index 6bb237a..0ace1d3 100644 --- a/src/clj/puppetlabs/http/client/async.clj +++ b/src/clj/puppetlabs/http/client/async.clj @@ -12,7 +12,8 @@ ;; these methods. (ns puppetlabs.http.client.async - (:import (com.puppetlabs.http.client HttpMethod HttpClientException) + (:import (com.puppetlabs.http.client HttpMethod HttpClientException + RequestOptions) (org.apache.http.nio.client HttpAsyncClient) (org.apache.http.impl.nio.client HttpAsyncClients) (org.apache.http.client.methods HttpGet HttpHead HttpPost HttpPut HttpTrace HttpDelete HttpOptions HttpPatch) @@ -26,7 +27,8 @@ (com.puppetlabs.http.client.impl Compression) (org.apache.http.client RedirectStrategy) (org.apache.http.impl.client LaxRedirectStrategy DefaultRedirectStrategy) - (org.apache.http.nio.conn.ssl SSLIOSessionStrategy)) + (org.apache.http.nio.conn.ssl SSLIOSessionStrategy) + (org.apache.http.conn.ssl SSLContexts)) (:require [puppetlabs.certificate-authority.core :as ssl] [clojure.string :as str] [puppetlabs.kitchensink.core :as ks] @@ -64,7 +66,7 @@ [req] (initialize-ssl-context-from-ca-pem req)) -(schema/defn configure-ssl :- (schema/either {} common/SslContextOptions) +(schema/defn configure-ssl-ctxt :- (schema/either {} common/SslContextOptions) "Configures a request map to have an SSLContext. It will use an existing one (stored in :ssl-context) if already present, and will fall back to a set of PEM files (stored in :ssl-cert, :ssl-key, and :ssl-ca-cert) if those are present. @@ -263,6 +265,7 @@ (schema/defn extract-client-opts :- common/ClientOptions [opts :- common/UserRequestOptions] (select-keys opts [:ssl-context :ssl-ca-cert :ssl-cert :ssl-key + :ssl-protocols :cipher-suites :force-redirects :follow-redirects])) (schema/defn extract-ssl-opts :- common/SslOptions @@ -273,6 +276,18 @@ [opts :- common/UserRequestOptions] (select-keys opts [:url :method :headers :body :decompress-body :as :persistent :query-params])) +(schema/defn ^:always-validate ssl-strategy :- SSLIOSessionStrategy + [ssl-ctxt-opts :- common/SslContextOptions + ssl-prot-opts :- common/SslProtocolOptions] + (SSLIOSessionStrategy. + (:ssl-context ssl-ctxt-opts) + (if (contains? ssl-prot-opts :ssl-protocols) + (into-array String (:ssl-protocols ssl-prot-opts)) + RequestOptions/DEFAULT_SSL_PROTOCOLS) + (if (contains? ssl-prot-opts :cipher-suites) + (into-array String (:cipher-suites ssl-prot-opts))) + SSLIOSessionStrategy/BROWSER_COMPATIBLE_HOSTNAME_VERIFIER)) + (schema/defn ^:always-validate redirect-strategy :- RedirectStrategy [opts :- common/ClientOptions] (let [follow-redirects (:follow-redirects opts) @@ -291,17 +306,12 @@ (schema/defn ^:always-validate create-default-client :- common/Client [opts :- common/ClientOptions] - (let [configured-opts (configure-ssl (extract-ssl-opts opts)) + (let [ssl-ctxt-opts (configure-ssl-ctxt (extract-ssl-opts opts)) + ssl-prot-opts (select-keys opts [:ssl-protocols :cipher-suites]) client-builder (HttpAsyncClients/custom) - client (do (when (:ssl-context configured-opts) - (.setSSLStrategy - client-builder - (SSLIOSessionStrategy. - (:ssl-context configured-opts) - SSLIOSessionStrategy/BROWSER_COMPATIBLE_HOSTNAME_VERIFIER))) - (.setRedirectStrategy - client-builder - (redirect-strategy opts)) + client (do (when (:ssl-context ssl-ctxt-opts) + (.setSSLStrategy client-builder (ssl-strategy ssl-ctxt-opts ssl-prot-opts))) + (.setRedirectStrategy client-builder (redirect-strategy opts)) (.build client-builder))] (.start client) client)) diff --git a/src/clj/puppetlabs/http/client/common.clj b/src/clj/puppetlabs/http/client/common.clj index 322a13d..928d175 100644 --- a/src/clj/puppetlabs/http/client/common.clj +++ b/src/clj/puppetlabs/http/client/common.clj @@ -29,8 +29,8 @@ (def UrlOrString (schema/either schema/Str URL)) --;; TODO: replace this with a protocol --(def Client CloseableHttpAsyncClient) +;; TODO: replace this with a protocol +(def Client CloseableHttpAsyncClient) (def Headers {schema/Str schema/Str}) @@ -57,6 +57,8 @@ (ok :ssl-cert) UrlOrString (ok :ssl-key) UrlOrString (ok :ssl-ca-cert) UrlOrString + (ok :ssl-protocols) [schema/Str] + (ok :cipher-suites) [schema/Str] (ok :force-redirects) schema/Bool (ok :follow-redirects) schema/Bool}) @@ -99,6 +101,10 @@ (def SslOptions (schema/either {} SslContextOptions SslCertOptions SslCaCertOptions)) +(def SslProtocolOptions + {(schema/optional-key :ssl-protocols) [schema/Str] + (schema/optional-key :cipher-suites) [schema/Str]}) + (def RedirectOptions {(schema/optional-key :force-redirects) schema/Bool (schema/optional-key :follow-redirects) schema/Bool}) @@ -108,9 +114,9 @@ validating the RawUserRequestClientOptions and merging it with the defaults." (schema/either (merge RequestOptions RedirectOptions) - (merge RequestOptions SslContextOptions RedirectOptions) - (merge RequestOptions SslCaCertOptions RedirectOptions) - (merge RequestOptions SslCertOptions RedirectOptions))) + (merge RequestOptions SslContextOptions SslProtocolOptions RedirectOptions) + (merge RequestOptions SslCaCertOptions SslProtocolOptions RedirectOptions) + (merge RequestOptions SslCertOptions SslProtocolOptions RedirectOptions))) (def ClientOptions "The options from UserRequestOptions that are related to the @@ -118,9 +124,9 @@ from UserRequestOptions not included in RequestOptions." (schema/either RedirectOptions - (merge SslContextOptions RedirectOptions) - (merge SslCertOptions RedirectOptions) - (merge SslCaCertOptions RedirectOptions))) + (merge SslContextOptions SslProtocolOptions RedirectOptions) + (merge SslCertOptions SslProtocolOptions RedirectOptions) + (merge SslCaCertOptions SslProtocolOptions RedirectOptions))) (def ResponseCallbackFn (schema/maybe (schema/pred ifn?))) diff --git a/src/java/com/puppetlabs/http/client/RequestOptions.java b/src/java/com/puppetlabs/http/client/RequestOptions.java index 8a637c5..a83ecfa 100644 --- a/src/java/com/puppetlabs/http/client/RequestOptions.java +++ b/src/java/com/puppetlabs/http/client/RequestOptions.java @@ -12,6 +12,9 @@ import java.net.URISyntaxException; import java.util.Map; public class RequestOptions { + public static final String[] DEFAULT_SSL_PROTOCOLS = + new String[] {"TLSv1", "TLSv1.1", "TLSv1.2"}; + private HttpAsyncClient client = null; private URI uri; @@ -21,6 +24,8 @@ public class RequestOptions { private String sslCert; private String sslKey; private String sslCaCert; + private String[] sslProtocols; + private String[] sslCipherSuites; private boolean insecure = false; private Object body; private boolean decompressBody = true; @@ -97,6 +102,22 @@ public class RequestOptions { return this; } + public String[] getSslProtocols() { + return sslProtocols; + } + public RequestOptions setSslProtocols(String[] sslProtocols) { + this.sslProtocols = sslProtocols; + return this; + } + + public String[] getSslCipherSuites() { + return sslCipherSuites; + } + public RequestOptions setSslCipherSuites(String[] sslCipherSuites) { + this.sslCipherSuites = sslCipherSuites; + return this; + } + public boolean getInsecure() { return insecure; } diff --git a/src/java/com/puppetlabs/http/client/impl/CoercedRequestOptions.java b/src/java/com/puppetlabs/http/client/impl/CoercedRequestOptions.java index 4b4f07d..c6d5e75 100644 --- a/src/java/com/puppetlabs/http/client/impl/CoercedRequestOptions.java +++ b/src/java/com/puppetlabs/http/client/impl/CoercedRequestOptions.java @@ -13,6 +13,8 @@ public class CoercedRequestOptions { private final Header[] headers; private final HttpEntity body; private final SSLContext sslContext; + private final String[] sslProtocols; + private final String[] sslCipherSuites; private final boolean forceRedirects; private final boolean followRedirects; @@ -22,6 +24,8 @@ public class CoercedRequestOptions { Header[] headers, HttpEntity body, SSLContext sslContext, + String[] sslProtocols, + String[] sslCipherSuites, boolean forceRedirects, boolean followRedirects) { this.uri = uri; @@ -29,6 +33,8 @@ public class CoercedRequestOptions { this.headers = headers; this.body = body; this.sslContext = sslContext; + this.sslProtocols = sslProtocols; + this.sslCipherSuites = sslCipherSuites; this.forceRedirects = forceRedirects; this.followRedirects = followRedirects; } @@ -53,6 +59,14 @@ public class CoercedRequestOptions { return sslContext; } + public String[] getSslProtocols() { + return sslProtocols; + } + + public String[] getSslCipherSuites() { + return sslCipherSuites; + } + public boolean getForceRedirects() { return forceRedirects; } public boolean getFollowRedirects() { return followRedirects; } diff --git a/src/java/com/puppetlabs/http/client/impl/JavaClient.java b/src/java/com/puppetlabs/http/client/impl/JavaClient.java index f5883f6..3282b39 100644 --- a/src/java/com/puppetlabs/http/client/impl/JavaClient.java +++ b/src/java/com/puppetlabs/http/client/impl/JavaClient.java @@ -103,6 +103,18 @@ public class JavaClient { sslContext = getInsecureSslContext(); } + String[] sslProtocols = null; + if (options.getSslProtocols() != null) { + sslProtocols = options.getSslProtocols(); + } else { + sslProtocols = RequestOptions.DEFAULT_SSL_PROTOCOLS; + } + + String[] sslCipherSuites = null; + if (options.getSslCipherSuites() != null) { + sslCipherSuites = options.getSslCipherSuites(); + } + HttpMethod method = options.getMethod(); if (method == null) { method = HttpMethod.GET; @@ -136,7 +148,7 @@ public class JavaClient { boolean forceRedirects = options.getForceRedirects(); boolean followRedirects = options.getFollowRedirects(); - return new CoercedRequestOptions(uri, method, headers, body, sslContext, forceRedirects, followRedirects); + return new CoercedRequestOptions(uri, method, headers, body, sslContext, sslProtocols, sslCipherSuites, forceRedirects, followRedirects); } private static SSLContext getInsecureSslContext() { @@ -232,8 +244,11 @@ public class JavaClient { private static CloseableHttpAsyncClient createClient(CoercedRequestOptions coercedOptions) { HttpAsyncClientBuilder clientBuilder = HttpAsyncClients.custom(); if (coercedOptions.getSslContext() != null) { - clientBuilder.setSSLStrategy(new SSLIOSessionStrategy(coercedOptions.getSslContext(), - SSLIOSessionStrategy.BROWSER_COMPATIBLE_HOSTNAME_VERIFIER)); + clientBuilder.setSSLStrategy( + new SSLIOSessionStrategy(coercedOptions.getSslContext(), + coercedOptions.getSslProtocols(), + coercedOptions.getSslCipherSuites(), + SSLIOSessionStrategy.BROWSER_COMPATIBLE_HOSTNAME_VERIFIER)); } RedirectStrategy redirectStrategy; if (!coercedOptions.getFollowRedirects()) { diff --git a/test/puppetlabs/http/client/async_ssl_config_test.clj b/test/puppetlabs/http/client/async_ssl_config_test.clj index c92ef13..1620083 100644 --- a/test/puppetlabs/http/client/async_ssl_config_test.clj +++ b/test/puppetlabs/http/client/async_ssl_config_test.clj @@ -12,9 +12,9 @@ (let [opts {:ssl-cert (resource "ssl/cert.pem") :ssl-key (resource "ssl/key.pem") :ssl-ca-cert (resource "ssl/ca.pem")} - configured-opts (http/configure-ssl opts)] + configured-opts (http/configure-ssl-ctxt opts)] - (testing "configure-ssl sets up an SSLContext when given cert, key, ca-cert" + (testing "configure-ssl-ctxt sets up an SSLContext when given cert, key, ca-cert" (is (instance? SSLContext (:ssl-context configured-opts)))) (testing "removes ssl-cert, ssl-key, ssl-ca-cert" @@ -24,18 +24,18 @@ (deftest ssl-config-with-ca-file (let [opts {:ssl-ca-cert (resource "ssl/ca.pem")} - configured-opts (http/configure-ssl opts)] + configured-opts (http/configure-ssl-ctxt opts)] - (testing "configure-ssl sets up an SSLContext when given ca-cert" + (testing "configure-ssl-ctxt sets up an SSLContext when given ca-cert" (is (instance? SSLContext (:ssl-context configured-opts)))) (testing "removes ssl-ca-cert" (is (not (:ssl-ca-cert configured-opts)))))) (deftest ssl-config-without-ssl-params - (let [configured-opts (http/configure-ssl {})] + (let [configured-opts (http/configure-ssl-ctxt {})] - (testing "configure-ssl does nothing when given no ssl parameters" + (testing "configure-ssl-ctxt does nothing when given no ssl parameters" (is (= {} configured-opts))))) (deftest ssl-config-with-context @@ -43,7 +43,7 @@ (resource "ssl/cert.pem") (resource "ssl/key.pem") (resource "ssl/ca.pem"))} - configured-opts (http/configure-ssl opts)] + configured-opts (http/configure-ssl-ctxt opts)] - (testing "configure-ssl uses an existing ssl context" + (testing "configure-ssl-ctxt uses an existing ssl context" (is (instance? SSLContext (:ssl-context configured-opts)))))) \ No newline at end of file diff --git a/test/puppetlabs/http/client/sync_ssl_test.clj b/test/puppetlabs/http/client/sync_ssl_test.clj index af37b2f..243f453 100644 --- a/test/puppetlabs/http/client/sync_ssl_test.clj +++ b/test/puppetlabs/http/client/sync_ssl_test.clj @@ -2,7 +2,8 @@ (:import (com.puppetlabs.http.client SyncHttpClient RequestOptions HttpClientException) (javax.net.ssl SSLHandshakeException) - (java.net URI)) + (java.net URI) + (org.apache.http ConnectionClosedException)) (:require [clojure.test :refer :all] [puppetlabs.trapperkeeper.core :as tk] [puppetlabs.trapperkeeper.testutils.bootstrap :as testutils] @@ -94,3 +95,116 @@ (is (thrown? SSLHandshakeException (sync/get "https://localhost:10081/hello/" {:ssl-ca-cert "./dev-resources/ssl/alternate-ca.pem"}))))))) + +(defmacro with-server-with-protocols + [server-protocols server-cipher-suites & body] + `(testlogging/with-test-logging + (testutils/with-app-with-config app# + [jetty9/jetty9-service test-web-service] + {:webserver (merge + {:ssl-host "0.0.0.0" + :ssl-port 10080 + :ssl-ca-cert "./dev-resources/ssl/ca.pem" + :ssl-cert "./dev-resources/ssl/cert.pem" + :ssl-key "./dev-resources/ssl/key.pem" + :ssl-protocols ~server-protocols} + (if ~server-cipher-suites + {:cipher-suites ~server-cipher-suites}))} + ~@body))) + +(defmacro java-unsupported-protocol-exception? + [& body] + `(try + ~@body + (catch HttpClientException e# + (let [cause# (.getCause e#)] + (or + (and (instance? SSLHandshakeException cause#) + (re-find #"not supported by the client" (.getMessage cause#))) + (instance? ConnectionClosedException cause#)))))) + +(defn java-https-get-with-protocols + [client-protocols client-cipher-suites] + (let [options (.. (RequestOptions. (URI. "https://localhost:10080/hello/")) + (setSslCert "./dev-resources/ssl/cert.pem") + (setSslKey "./dev-resources/ssl/key.pem") + (setSslCaCert "./dev-resources/ssl/ca.pem"))] + (if client-protocols + (.setSslProtocols options (into-array String client-protocols))) + (if client-cipher-suites + (.setSslCipherSuites options (into-array String client-cipher-suites))) + (SyncHttpClient/get options))) + +(defn clj-https-get-with-protocols + [client-protocols client-cipher-suites] + (let [ssl-opts (merge {:ssl-cert "./dev-resources/ssl/cert.pem" + :ssl-key "./dev-resources/ssl/key.pem" + :ssl-ca-cert "./dev-resources/ssl/ca.pem"} + (if client-protocols + {:ssl-protocols client-protocols}) + (if client-cipher-suites + {:cipher-suites client-cipher-suites}))] + (sync/get "https://localhost:10080/hello/" ssl-opts))) + +(deftest sync-client-test-ssl-protocols + (testing "should be able to connect to a TLSv1.2 server by default" + (with-server-with-protocols ["TLSv1.2"] nil + (testing "java sync client" + (let [response (java-https-get-with-protocols nil nil)] + (is (= 200 (.getStatus response))) + (is (= "Hello, World!" (slurp (.getBody response)))))) + (testing "clojure sync client" + (let [response (clj-https-get-with-protocols nil nil)] + (is (= 200 (:status response))) + (is (= "Hello, World!" (slurp (:body response)))))))) + + (testing "should be able to connect to a server with non-default protocol if configured" + (with-server-with-protocols ["SSLv3"] nil + (testing "java sync client" + (let [response (java-https-get-with-protocols ["SSLv3"] nil)] + (is (= 200 (.getStatus response))) + (is (= "Hello, World!" (slurp (.getBody response)))))) + (testing "clojure sync client" + (let [response (clj-https-get-with-protocols ["SSLv3"] nil)] + (is (= 200 (:status response))) + (is (= "Hello, World!" (slurp (:body response)))))))) + + (testing "should not connect to an SSLv3 server by default" + (with-server-with-protocols ["SSLv3"] nil + (testing "java sync client" + (is (java-unsupported-protocol-exception? + (java-https-get-with-protocols nil nil)))) + (testing "clojure sync client" + (is (thrown-with-msg? + SSLHandshakeException #"not supported by the client" + (clj-https-get-with-protocols nil nil)))))) + + (testing "should not connect to a server when protocols don't overlap" + (with-server-with-protocols ["TLSv1.1"] nil + (testing "java sync client" + (is (java-unsupported-protocol-exception? + (java-https-get-with-protocols ["TLSv1.2"] nil)))) + (testing "clojure sync client" + (is (thrown-with-msg? + SSLHandshakeException #"not supported by the client" + (clj-https-get-with-protocols ["TLSv1.2"] nil))))))) + +(deftest sync-client-test-cipher-suites + (testing "should not connect to a server with no overlapping cipher suites" + (with-server-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_256_CBC_SHA256"] + (testing "java sync client" + (is (java-unsupported-protocol-exception? + (java-https-get-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_128_CBC_SHA256"])))) + (testing "clojure sync client" + (is (thrown? ConnectionClosedException + (clj-https-get-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_128_CBC_SHA256"])))))) + (testing "should connect to a server with overlapping cipher suites" + (with-server-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_256_CBC_SHA256"] + (testing "java sync client" + (let [response (java-https-get-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_256_CBC_SHA256"])] + (is (= 200 (.getStatus response))) + (is (= "Hello, World!" (slurp (.getBody response)))))) + (testing "clojure sync client" + (let [response (clj-https-get-with-protocols ["TLSv1.2"] ["TLS_RSA_WITH_AES_256_CBC_SHA256"])] + (is (= 200 (:status response))) + (is (= "Hello, World!" (slurp (:body response))))))))) \ No newline at end of file