Merge pull request #191 from grimradical/client-cert-restriction

Client whitelisting

README.md

@@ -693,6 +693,17 @@ your puppet infrastructure.
 
 Passphrase to use to unlock the truststore file.
 
+`certificate-whitelist`
+
+Optional. Path to a file that contains a list of hostnames, one per
+line.  Incoming HTTPS requests will have their certificates validated
+against this list of hostnames, and only those with an _exact_,
+matching entry will be allowed through.
+
+If not supplied, we'll perform standard HTTPS without any additional
+authorization. We'll still make sure that all HTTPS clients supply
+valid, verifiable SSL client certificates.
+
 **[repl]**
 
 Enabling a remote

acceptance/helper.rb

@@ -11,6 +11,14 @@ def self.test_mode()
     @test_mode
   end
 
+  def puppetdb_confdir(host)
+    if host.is_pe?
+      "/etc/puppetlabs/puppetdb"
+    else
+      "/etc/puppetdb"
+    end
+  end
+
   def start_puppetdb(host)
     on host, "service puppetdb start"
 
@@ -30,6 +38,11 @@ def stop_puppetdb(host)
     on host, "service puppetdb stop"
   end
 
+  def restart_puppetdb(host)
+    stop_puppetdb(host)
+    start_puppetdb(host)
+  end
+
   def sleep_until_queue_empty(host, timeout=nil)
     metric = "org.apache.activemq:BrokerName=localhost,Type=Queue,Destination=com.puppetlabs.puppetdb.commands"
     queue_size = nil

acceptance/tests/security/cert_whitelist.rb

@@ -0,0 +1,44 @@
+test_name "certificate whitelisting" do
+
+  confd = "#{puppetdb_confdir(database)}/conf.d"
+  dbname = database.node_name
+
+  on database, puppet_agent("--configprint ssldir")
+  ssldir = stdout.chomp
+
+  step "reconfigure PuppetDB to use certificate whitelist" do
+    on database, "cp #{confd}/jetty.ini #{confd}/jetty.ini.bak"
+    on database, "grep -q ^certificate-whitelist #{confd}/jetty.ini || echo 'certificate-whitelist = #{confd}/whitelist' >> #{confd}/jetty.ini"
+  end
+
+  # Execute a curl from the database to itself using HTTPS, using the
+  # supplied whitelist, and verify the response is what we expect
+  curl_against_whitelist = proc do |whitelist, expected_status_code|
+    create_remote_file database, "#{confd}/whitelist", whitelist
+    on database, "chmod 644 #{confd}/whitelist"
+    restart_puppetdb database
+    on database, "curl -sL -w '%{http_code}\\n' -H 'Accept: application/json' " +
+                 "--cacert #{ssldir}/certs/ca.pem " +
+                 "--cert #{ssldir}/certs/#{dbname}.pem " +
+                 "--key #{ssldir}/private_keys/#{dbname}.pem "+
+                 "https://#{dbname}:8081/metrics/mbeans " +
+                 "-o /dev/null"
+    actual_status_code = stdout.chomp
+    assert_equal expected_status_code.to_s, actual_status_code, "Reqs from #{dbname} with whitelist '#{whitelist}' should return #{expected_status_code}, not #{actual_status_code}"
+  end
+
+  step "hosts in whitelist should be allowed in" do
+    curl_against_whitelist.call "#{dbname}\n", 200
+  end
+
+  step "hosts not in whitelist should be forbidden" do
+    curl_against_whitelist.call "", 403
+  end
+
+  step "restore original jetty.ini" do
+    on database, "mv #{confd}/jetty.ini.bak #{confd}/jetty.ini"
+    on database, "chmod 644 #{confd}/jetty.ini"
+    on database, "rm #{confd}/whitelist"
+    restart_puppetdb database
+  end
+end

project.clj

@@ -37,7 +37,7 @@
                  [org.clojure/tools.nrepl "0.2.0-beta2"]
                  [swank-clojure "1.4.0"]
                  [clj-stacktrace "0.2.4"]
-                 [metrics-clojure "0.7.0" :exclusions [org.clojure/clojure]]
+                 [metrics-clojure "0.7.0" :exclusions [org.clojure/clojure org.slf4j/slf4j-api]]
                  [clj-time "0.3.7"]
                  [org.clojure/java.jmx "0.1"]
                  ;; Filesystem utilities
@@ -52,20 +52,20 @@
                                               com.sun.jdmk/jmxtools
                                               com.sun.jmx/jmxri]]
                  ;; Database connectivity
-                 [com.jolbox/bonecp "0.7.1.RELEASE"]
-                 [org.slf4j/slf4j-log4j12 "1.5.6"]
+                 [com.jolbox/bonecp "0.7.1.RELEASE" :exclusions [org.slf4j/slf4j-api]]
+                 [org.slf4j/slf4j-log4j12 "1.6.4"]
                  [org.clojure/java.jdbc "0.1.1"]
                  [org.hsqldb/hsqldb "2.2.8"]
                  [postgresql/postgresql "9.0-801.jdbc4"]
                  [clojureql "1.0.3"]
                  ;; MQ connectivity
-                 [clamq/clamq-activemq "0.4"]
-                 [org.apache.activemq/activemq-core "5.5.1"]
+                 [clamq/clamq-activemq "0.4" :exclusions [org.slf4j/slf4j-api]]
+                 [org.apache.activemq/activemq-core "5.5.1" :exclusions [org.slf4j/slf4j-api]]
                  ;; WebAPI support libraries.
                  [net.cgrand/moustache "1.1.0"]
                  [clj-http "0.3.1"]
-                 [ring/ring-core "1.0.2"]
-                 [ring/ring-jetty-adapter "1.0.2"]]
+                 [ring/ring-core "1.1.1"]
+                 [ring/ring-jetty-adapter "1.1.1"]]
 
   :dev-dependencies [[lein-marginalia "0.7.0"]
                      ;; WebAPI support libraries.

src/com/puppetlabs/jetty.clj

@@ -1,9 +1,8 @@
 ;; ## Monkey patches for Ring's Jetty adapter
 ;;
 (ns com.puppetlabs.jetty
-  (:import (org.mortbay.jetty Server)
-           (org.mortbay.jetty.bio SocketConnector)
-           (org.mortbay.jetty.security SslSocketConnector))
+  (:import (org.eclipse.jetty.server Server)
+           (org.eclipse.jetty.server.nio SelectChannelConnector))
   (:require [ring.adapter.jetty :as jetty])
   (:use [clojure.tools.logging :as log]))
 
@@ -27,46 +26,27 @@
     (catch Throwable e
       (log/error e "Could not remove security providers; HTTPS may not work!"))))
 
-(defn add-ssl-connector!
-  "Add an SslSocketConnector to a Jetty Server instance."
-  [^Server server options]
-  (let [ssl-connector (SslSocketConnector.)]
-    (doto ssl-connector
-      (.setPort        (options :ssl-port 443))
-      (.setHost        (options :ssl-host "localhost"))
-      (.setKeystore    (options :keystore))
-      (.setKeyPassword (options :key-password)))
-    (when (options :truststore)
-      (.setTruststore ssl-connector (options :truststore)))
-    (when (options :trust-password)
-      (.setTrustPassword ssl-connector (options :trust-password)))
-    (when (options :need-client-auth)
-      (.setNeedClientAuth ssl-connector true))
-    (when (options :want-client-auth)
-      (.setWantClientAuth ssl-connector true))
-    (.addConnector server ssl-connector)))
-
-(defn add-connector!
-  "Add a plain SocketConnector to a Jetty Server instance."
-  [^Server server options]
-  (let [connector (SocketConnector.)]
-    (doto connector
-      (.setPort (options :port))
-      (.setHost (options :host "localhost")))
-    (.addConnector server connector)))
-
 ;; Monkey-patched version of `create-server` that will only create a
 ;; non-SSL connector if the options specifically dictate it.
 
+(defn plaintext-connector
+  [options]
+  (doto (SelectChannelConnector.)
+    (.setPort (options :port 80))
+    (.setHost (options :host "localhost"))))
+
 (defn- create-server
   "Construct a Jetty Server instance."
   [options]
   (let [server (doto (Server.)
                  (.setSendDateHeader true))]
     (when (options :port)
-      (add-connector! server options))
+      (.addConnector server (plaintext-connector options)))
+
     (when (or (options :ssl?) (options :ssl-port))
-      (add-ssl-connector! server options))
+      (let [ssl-host (options :ssl-host (options :host "localhost"))
+            options  (assoc options :host ssl-host)]
+        (.addConnector server (#'jetty/ssl-connector options))))
     server))
 
 (defn run-jetty
@@ -75,6 +55,5 @@
   [handler options]
   (when (empty? (select-keys options [:port :ssl? :ssl-port]))
     (throw (IllegalArgumentException. "No ports were specified to bind")))
-  (with-redefs [jetty/add-ssl-connector! add-ssl-connector!
-                jetty/create-server      create-server]
+  (with-redefs [jetty/create-server create-server]
     (jetty/run-jetty handler options)))

src/com/puppetlabs/middleware.clj

@@ -1,9 +1,39 @@
 ;; ## Ring middleware
 
 (ns com.puppetlabs.middleware
+  (:require [com.puppetlabs.utils :as utils]
+            [ring.util.response :as rr])
   (:use [metrics.timers :only (timer time!)]
         [metrics.meters :only (meter mark!)]))
 
+(defn wrap-with-authorization
+  "Ring middleware that will only pass through a request if the
+  supplied authorization function allows it. Otherwise an HTTP 403 is
+  returned to the client.
+
+  `authorized?` is expected to take a single argument, the current
+  request. The request is allowed only if the return value of
+  `authorized?` is truthy."
+  [app authorized?]
+  (fn [req]
+    (if (authorized? req)
+      (app req)
+      (-> "You shall not pass!"
+          (rr/response)
+          (rr/status 403)))))
+
+(defn wrap-with-certificate-cn
+  "Ring middleware that will annotate the request with an
+  :ssl-client-cn key representing the CN contained in the client
+  certificate of the request. If no client certificate is present,
+  the key's value is set to nil."
+  [app]
+  (fn [{:keys [ssl-client-cert] :as req}]
+    (let [cn  (if ssl-client-cert
+                (utils/cn-for-cert ssl-client-cert))
+          req (assoc req :ssl-client-cn cn)]
+      (app req))))
+
 (defn wrap-with-globals
   "Ring middleware that will add to each request a :globals attribute:
   a map containing various global settings"

src/com/puppetlabs/puppetdb/cli/services.clj

@@ -126,7 +126,7 @@
   [config]
   {:pre  [(map? config)]
    :post [(map? config)]}
-  (assoc-in config [:jetty :need-client-auth] true))
+  (assoc-in config [:jetty :client-auth] :need))
 
 (defn configure-database
   "Update the supplied config map with information about the database. Adds a
@@ -206,8 +206,7 @@
         discard-dir                                (file mq-dir "discarded")
         globals                                    {:scf-db     db
                                                     :command-mq {:connection-string mq-addr
-                                                                 :endpoint          mq-endpoint}}
-        ring-app                                   (server/build-app globals)]
+                                                                 :endpoint          mq-endpoint}}]
 
     (when version
       (log/info (format "PuppetDB version %s" version)))
@@ -231,10 +230,13 @@
                           (vec (for [n (range nthreads)]
                                  (future (with-error-delivery error
                                            (load-from-mq mq-addr mq-endpoint discard-dir db))))))
-          web-app       (do
+          web-app       (let [authorized? (if-let [wl (jetty :certificate-whitelist)]
+                                            (pl-utils/cn-whitelist->authorizer wl)
+                                            (constantly true))
+                              app         (server/build-app :globals globals :authorized? authorized?)]
                           (log/info "Starting query server")
                           (future (with-error-delivery error
-                                    (jetty/run-jetty ring-app jetty))))
+                                    (jetty/run-jetty app jetty))))
           db-gc         (do
                           (log/info (format "Starting database compactor (%d minute interval)" db-gc-minutes))
                           (future (with-error-delivery error

src/com/puppetlabs/puppetdb/http/server.clj

@@ -11,7 +11,8 @@
         [com.puppetlabs.puppetdb.http.node :only (node-app)]
         [com.puppetlabs.puppetdb.http.status :only (status-app)]
         [com.puppetlabs.puppetdb.http.experimental :only (experimental-app)]
-        [com.puppetlabs.middleware :only (wrap-with-globals wrap-with-metrics)]
+        [com.puppetlabs.middleware :only
+         (wrap-with-authorization wrap-with-certificate-cn wrap-with-globals wrap-with-metrics)]
         [com.puppetlabs.utils :only (uri-segments)]
         [net.cgrand.moustache :only (app)]
         [ring.middleware.resource :only (wrap-resource)]
@@ -42,11 +43,21 @@
    {:get metrics-app}))
 
 (defn build-app
-  "Given an attribute map representing connectivity to the SCF
-  database, generate a Ring application that handles queries"
-  [globals]
-  (-> routes
-      (wrap-resource "public")
-      (wrap-params)
-      (wrap-with-metrics (atom {}) #(first (uri-segments %)))
-      (wrap-with-globals globals)))
+  "Generate a Ring application that handles PuppetDB requests
+
+  `options` is a list of keys and values where keys can be the following:
+
+  * `globals` - a map containing global state useful to request handlers.
+
+  * `authorized?` - a function that takes a request and returns a
+    truthy value if the request is authorized. If not supplied, we default
+    to authorizing all requests."
+  [& options]
+  (let [opts (apply hash-map options)]
+    (-> routes
+        (wrap-resource "public")
+        (wrap-params)
+        (wrap-with-authorization (opts :authorized? (constantly true)))
+        (wrap-with-certificate-cn)
+        (wrap-with-metrics (atom {}) #(first (uri-segments %)))
+        (wrap-with-globals (opts :globals)))))