(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

@@ -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)
+        client          (do (when (:ssl-context ssl-ctxt-opts)
-                              (.setSSLStrategy
+                              (.setSSLStrategy client-builder (ssl-strategy ssl-ctxt-opts ssl-prot-opts)))
-                                client-builder
+                            (.setRedirectStrategy client-builder (redirect-strategy opts))
-                                (SSLIOSessionStrategy.
-                                  (:ssl-context configured-opts)
-                                  SSLIOSessionStrategy/BROWSER_COMPATIBLE_HOSTNAME_VERIFIER)))
-                            (.setRedirectStrategy
-                              client-builder
-                              (redirect-strategy opts))
                             (.build client-builder))]
     (.start client)
     client))

src/clj/puppetlabs/http/client/common.clj

@@ -29,8 +29,8 @@
 
 (def UrlOrString (schema/either schema/Str URL))
 
--;; TODO: replace this with a protocol
+;; TODO: replace this with a protocol
--(def Client CloseableHttpAsyncClient)
+(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 SslContextOptions SslProtocolOptions RedirectOptions)
-    (merge RequestOptions SslCaCertOptions RedirectOptions)
+    (merge RequestOptions SslCaCertOptions SslProtocolOptions RedirectOptions)
-    (merge RequestOptions SslCertOptions 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 SslContextOptions SslProtocolOptions RedirectOptions)
-    (merge SslCertOptions RedirectOptions)
+    (merge SslCertOptions SslProtocolOptions RedirectOptions)
-    (merge SslCaCertOptions RedirectOptions)))
+    (merge SslCaCertOptions SslProtocolOptions RedirectOptions)))
 
 (def ResponseCallbackFn
   (schema/maybe (schema/pred ifn?)))

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;
     }

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; }

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(),
+            clientBuilder.setSSLStrategy(
-                    SSLIOSessionStrategy.BROWSER_COMPATIBLE_HOSTNAME_VERIFIER));
+                    new SSLIOSessionStrategy(coercedOptions.getSslContext(),
+                            coercedOptions.getSslProtocols(),
+                            coercedOptions.getSslCipherSuites(),
+                            SSLIOSessionStrategy.BROWSER_COMPATIBLE_HOSTNAME_VERIFIER));
         }
         RedirectStrategy redirectStrategy;
         if (!coercedOptions.getFollowRedirects()) {

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))))))

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)))))))))