Authenticate every request by client certificate

config.sample.ini

@@ -9,9 +9,14 @@ vardir = /var/lib/puppetdb
 # logging-config = /path/to/logback.xml
 
 [puppetdb]
-# List of certificate names from which to allow incoming HTTPS requests:
+# List of certificate names from which to allow incoming requests:
 # certificate-allowlist = /path/to/certname/allowlist
 
+# Accept requests on the cleartext HTTP port without authenticating them, even
+# when they do not come from a loopback address. Only set this when that port is
+# restricted to trusted clients:
+# allow-unauthenticated-cleartext = false
+
 [database]
 
 # Subname pattern: //host:port/databaseName

documentation/configure.markdown

@@ -238,15 +238,40 @@ Note that this maximum does not apply to queries with
 ### `certificate-allowlist`
 
 Optional. This describes the path to a file that contains a list of
-certificate names, one per line. Incoming HTTPS requests will have
+certificate names, one per line. Incoming requests will have
 their certificates validated against this list of names and only those
 with an **exact** matching entry will be allowed through. (For an OpenVox
 Server, this compares against the value of the `certname` setting,
 rather than the `dns_alt_names` setting.)
 
-If not supplied, OpenVoxDB uses standard HTTPS without any additional
-authorization. All HTTPS clients must still supply valid, verifiable
-SSL client certificates.
+The allowlist applies to every request that is subject to certificate
+authentication, including requests that arrive on the cleartext HTTP port. A
+cleartext client cannot present a certificate, so it can never satisfy the
+allowlist; see [`allow-unauthenticated-cleartext`](#allow-unauthenticated-cleartext)
+for the exemptions that apply to that port.
+
+If not supplied, OpenVoxDB requires every client to present a certificate
+signed by its CA, but does not restrict which certnames may connect.
+
+### `allow-unauthenticated-cleartext`
+
+Optional, defaults to `false`. Requests that arrive on the cleartext HTTP port
+(`[jetty] port`) cannot present a client certificate, so they cannot be
+authenticated. By default they are accepted only when they come from a loopback
+address, which is what the shipped configuration expects: the cleartext port is
+bound to `localhost` and is there for local administration and for the
+performance dashboard.
+
+Set this to `true` if you terminate authentication somewhere else, for example
+when OpenVoxDB sits behind a reverse proxy running on another host, and you have
+restricted the cleartext port to that proxy. Every cleartext request is then
+accepted without authentication, and OpenVoxDB logs a warning at startup saying
+so. Anything that can reach the port can read the whole database, submit
+commands for any node, and delete node data, so do not enable this on a port
+that untrusted clients can reach.
+
+Requests on the HTTPS port are always authenticated, regardless of this
+setting.
 
 ### `log-queries`
 Optional. Setting this to `true` will enable debug level logging of the internal

documentation/maintain_and_tune.markdown

@@ -28,9 +28,15 @@ example,
     ssh -L 8080:localhost:8080 root@<puppetdb server>
 
 and then visit `http://localhost:8080` in the browser. If OpenVoxDB is running
-locally, or on a remote host that is listening for external cleartext
-connections from your machine, you can skip the ssh tunnel and visit either
-`http://localhost:8080` or `http://<puppetdb server>:8080` directly.
+locally you can skip the ssh tunnel and visit `http://localhost:8080` directly.
+
+Cleartext requests that do not come from a loopback address are rejected,
+because a cleartext client has no way to present a certificate and so cannot be
+authenticated. If OpenVoxDB is listening for external cleartext connections and
+you want to reach the dashboard at `http://<puppetdb server>:8080` without a
+tunnel, you have to set
+[`allow-unauthenticated-cleartext`](./configure.markdown#allow-unauthenticated-cleartext),
+which turns off authentication for that port entirely.
 
 OpenVoxDB uses this page to display a web-based dashboard with performance information and metrics, including its memory use, queue depth, command processing metrics, duplication rate, and query stats. It displays min/max/median of each metric over a configurable duration, as well as an animated SVG "sparkline" (a simple line chart that shows general variation). It also displays the current version of OpenVoxDB.
 

src/puppetlabs/puppetdb/config.clj

@@ -204,6 +204,7 @@
   (all-optional
    {:certificate-whitelist s/Str
     :certificate-allowlist s/Str
+    :allow-unauthenticated-cleartext (pls/defaulted-maybe String "false")
     :add-agent-report-filter (pls/defaulted-maybe String "true")
     :log-queries (pls/defaulted-maybe String "false")
     :query-timeout-default (pls/defaulted-maybe String "600")
@@ -212,6 +213,7 @@
 (def puppetdb-config-out
   "Schema for validating the parsed/processed [puppetdb] block"
   {(s/optional-key :certificate-allowlist) s/Str
+   :allow-unauthenticated-cleartext Boolean
    :add-agent-report-filter Boolean
    :log-queries Boolean
    :query-timeout-default s/Num

src/puppetlabs/puppetdb/middleware.clj

@@ -26,7 +26,7 @@
             [puppetlabs.puppetdb.command.constants :as const])
   (:import
    (clojure.lang ExceptionInfo)
-   (java.net HttpURLConnection)
+   (java.net HttpURLConnection InetAddress UnknownHostException)
    (java.sql SQLException)))
 
 (def handler-schema (s/=> s/Any {s/Any s/Any}))
@@ -41,30 +41,80 @@
     (log/debug (trs "Processing HTTP request to URI: ''{0}''" (:uri req)))
     (app req)))
 
+(defn- no-certificate-response []
+  (http/denied-response
+   (tru "OpenVoxDB requires clients to present a certificate signed by its CA, and this request presented none.")
+   HttpURLConnection/HTTP_FORBIDDEN))
+
+(defn- reject-unauthenticated-request
+  "Returns a ring response rejecting a request that did not present a client
+  certificate, or nil if it did."
+  [{:keys [ssl-client-cn]}]
+  (when-not ssl-client-cn
+    (log/warn (trs "Request without a client certificate rejected"))
+    (no-certificate-response)))
+
 (defn build-allowlist-authorizer
   "Build a function that will authorize requests based on the supplied
-  certificate allowlist (see `cn-whitelist->authorizer` for more
-  details). Returns :authorized if the request is allowed, otherwise a
-  string describing the reason not."
+  certificate allowlist, a file of certnames, one per line. Returns nil if the
+  request is allowed, otherwise a ring response describing the reason not.
+
+  The allowlist is checked for every request, whatever scheme it arrived over,
+  so a request that presented no client certificate can never satisfy it."
   [allowlist]
   {:pre  [(string? allowlist)]
    :post [(fn? %)]}
-  (let [allowed? (kitchensink/cn-whitelist->authorizer allowlist)]
+  (let [allowed? (set (kitchensink/lines allowlist))]
     (fn [{:keys [ssl-client-cn] :as req}]
-      (when-not (allowed? req)
-        (when ssl-client-cn
-          (log/warn (trs "{0} rejected by certificate allowlist {1}" ssl-client-cn allowlist)))
-        (http/denied-response (tru "The client certificate name {0} doesn't appear in the certificate allowlist. Is your master''s (or other OpenVoxDB client''s) certname listed in OpenVoxDB''s certificate-allowlist file?" ssl-client-cn)
-                              HttpURLConnection/HTTP_FORBIDDEN)))))
+      (or (reject-unauthenticated-request req)
+          (when-not (allowed? ssl-client-cn)
+            (log/warn (trs "{0} rejected by certificate allowlist {1}" ssl-client-cn allowlist))
+            (http/denied-response (tru "The client certificate name {0} doesn't appear in the certificate allowlist. Is your master''s (or other OpenVoxDB client''s) certname listed in OpenVoxDB''s certificate-allowlist file?" ssl-client-cn)
+                                  HttpURLConnection/HTTP_FORBIDDEN))))))
+
+(defn- loopback-request?
+  "True if req arrived from a loopback address."
+  [{:keys [remote-addr]}]
+  (boolean
+   (when remote-addr
+     ;; Jetty reports the peer address as a literal, so this does not
+     ;; perform a name lookup.
+     (try
+       (.isLoopbackAddress (InetAddress/getByName remote-addr))
+       (catch UnknownHostException _ false)))))
 
 (defn wrap-cert-authn
-  [app cert-allowlist]
-  (if-let [cert-authorize-fn (some-> cert-allowlist build-allowlist-authorizer)]
-    (fn [req]
-      (if-let [cert-auth-result (cert-authorize-fn req)]
-        cert-auth-result
-        (app req)))
-    app))
+  "Ring middleware that authenticates requests by client certificate. A
+  request must present a certificate signed by OpenVoxDB's CA, and when
+  cert-allowlist is the path to a certname allowlist, the certificate's CN
+  must appear in it.
+
+  Cleartext HTTP requests cannot present a certificate. They are allowed only
+  when they arrive from a loopback address, or from anywhere when
+  allow-unauthenticated-cleartext? is set."
+  [app cert-allowlist allow-unauthenticated-cleartext?]
+  (let [authorize (if cert-allowlist
+                    (build-allowlist-authorizer cert-allowlist)
+                    reject-unauthenticated-request)]
+    (fn [{:keys [remote-addr scheme] :as req}]
+      (let [cleartext? (= :http scheme)]
+        (cond
+          (and cleartext? (or allow-unauthenticated-cleartext?
+                              (loopback-request? req)))
+          (app req)
+
+          ;; A cleartext connection cannot present a client certificate, so it
+          ;; can never authenticate, whatever the request claims.
+          cleartext?
+          (do
+            (log/warn (trs "Cleartext request from {0} rejected because it cannot present a client certificate"
+                           remote-addr))
+            (no-certificate-response))
+
+          :else
+          (if-let [denied (authorize req)]
+            denied
+            (app req)))))))
 
 (defn wrap-with-certificate-cn
   "Ring middleware that will annotate the request with an

src/puppetlabs/puppetdb/pdb_routing.clj

@@ -103,9 +103,13 @@
         augmented-globals #(-> (shared-globals)
                                (assoc :url-prefix query-prefix
                                       :warn-experimental true))
-        cert-allowlist (get-in config [:puppetdb :certificate-allowlist])]
+        cert-allowlist (get-in config [:puppetdb :certificate-allowlist])
+        allow-unauthenticated-cleartext? (get-in config [:puppetdb :allow-unauthenticated-cleartext])]
     (set-url-prefix query-prefix)
 
+    (when allow-unauthenticated-cleartext?
+      (log/warn (trs "allow-unauthenticated-cleartext is enabled, so requests arriving on the cleartext HTTP port will not be authenticated. Only enable this when that port is restricted to trusted clients.")))
+
     (log/info (trs "Starting OpenVoxDB, entering maintenance mode"))
     (add-ring-handler
      service
@@ -117,7 +121,7 @@
                                    query
                                    clean
                                    delete-node))
-         (mid/wrap-cert-authn cert-allowlist)
+         (mid/wrap-cert-authn cert-allowlist allow-unauthenticated-cleartext?)
          mid/wrap-with-puppetdb-middleware))
 
     (enable-maint-mode)

test/puppetlabs/puppetdb/config_test.clj

@@ -52,7 +52,23 @@
       (let [config (configure-puppetdb {:puppetdb {:log-queries "some-string"}})]
         (is (= false (get-in config [:puppetdb :log-queries]))))
       (is (thrown? clojure.lang.ExceptionInfo
-                   (configure-puppetdb {:puppetdb {:log-queries 1337}}))))))
+                   (configure-puppetdb {:puppetdb {:log-queries 1337}}))))
+
+    (testing "should default :allow-unauthenticated-cleartext to false"
+      (let [config (configure-puppetdb {})]
+        (is (= false (get-in config [:puppetdb :allow-unauthenticated-cleartext])))))
+
+    (testing "should allow allow-unauthenticated-cleartext boolean"
+      (let [config (configure-puppetdb {:puppetdb {:allow-unauthenticated-cleartext "true"}})]
+        (is (= true (get-in config [:puppetdb :allow-unauthenticated-cleartext]))))
+      (let [config (configure-puppetdb {:puppetdb {:allow-unauthenticated-cleartext "false"}})]
+        (is (= false (get-in config [:puppetdb :allow-unauthenticated-cleartext]))))
+      ;; Anything that isn't recognized as true leaves authentication on, which
+      ;; is the safe way for this setting to fail.
+      (let [config (configure-puppetdb {:puppetdb {:allow-unauthenticated-cleartext "some-string"}})]
+        (is (= false (get-in config [:puppetdb :allow-unauthenticated-cleartext]))))
+      (is (thrown? clojure.lang.ExceptionInfo
+                   (configure-puppetdb {:puppetdb {:allow-unauthenticated-cleartext 1337}}))))))
 
 (deftest commandproc-configuration
   (testing "should use the thread value specified"

test/puppetlabs/puppetdb/middleware_test.clj

@@ -68,24 +68,71 @@
   {:scheme :https
    :ssl-client-cn hostname})
 
+(def ^:private ok-handler
+  (fn [_req] (-> (rr/response nil)
+                 (rr/status HttpURLConnection/HTTP_OK))))
+
+(defn- status-for [app req]
+  (:status (app req)))
+
 (deftest wrapping-authorization
   (testing "Should only allow authorized requests"
-    ;; Setup an app that only lets through odd numbers
     (let [wl (.getAbsolutePath (temp-file "allowlist-log-reject"))
           _ (spit wl "foobar")
-          handler     (fn [_req] (-> (rr/response nil)
-                                     (rr/status HttpURLConnection/HTTP_OK)))
-
-          message     "The client certificate name"
-          app (wrap-cert-authn handler wl)]
-      ;; Even numbers should trigger an unauthorized response
+          message "The client certificate name"
+          app (wrap-cert-authn ok-handler wl false)]
+      ;; A cn that isn't in the allowlist should trigger an unauthorized response
       (is (= HttpURLConnection/HTTP_FORBIDDEN
-             (:status (app (create-authorizing-request "baz")))))
+             (status-for app (create-authorizing-request "baz"))))
       ;; The failure reason should be shown to the user
       (is (.contains (:body (app (create-authorizing-request "baz"))) message))
-      ;; Odd numbers should get through fine
+      ;; A cn that is in the allowlist should get through fine
+      (is (= HttpURLConnection/HTTP_OK
+             (status-for app (create-authorizing-request "foobar"))))))
+
+  (testing "Should reject an unauthenticated request when there is no allowlist"
+    (let [app (wrap-cert-authn ok-handler nil false)]
+      (is (= HttpURLConnection/HTTP_OK
+             (status-for app (create-authorizing-request "anybody"))))
+      (is (= HttpURLConnection/HTTP_FORBIDDEN
+             (status-for app {:scheme :https})))
+      (is (.contains (:body (app {:scheme :https}))
+                     "requires clients to present a certificate"))))
+
+  (testing "Should apply the allowlist to cleartext requests"
+    (let [wl (.getAbsolutePath (temp-file "allowlist-cleartext"))
+          _ (spit wl "foobar")
+          app (wrap-cert-authn ok-handler wl false)]
+      ;; A cleartext request from elsewhere can't satisfy the allowlist, even
+      ;; when it claims a cn that appears in it.
+      (is (= HttpURLConnection/HTTP_FORBIDDEN
+             (status-for app {:scheme :http :remote-addr "192.0.2.1"})))
+      (is (= HttpURLConnection/HTTP_FORBIDDEN
+             (status-for app {:scheme :http
+                              :remote-addr "192.0.2.1"
+                              :ssl-client-cn "foobar"})))))
+
+  (testing "Should exempt loopback cleartext requests"
+    (doseq [allowlist [nil (doto (.getAbsolutePath (temp-file "allowlist-loopback"))
+                             (spit "foobar"))]]
+      (let [app (wrap-cert-authn ok-handler allowlist false)]
+        (doseq [addr ["127.0.0.1" "127.0.1.1" "::1"]]
+          (is (= HttpURLConnection/HTTP_OK
+                 (status-for app {:scheme :http :remote-addr addr}))))
+        (is (= HttpURLConnection/HTTP_FORBIDDEN
+               (status-for app {:scheme :http :remote-addr "192.0.2.1"})))
+        ;; An https request from loopback is not exempt; the exemption exists
+        ;; because a cleartext client has no way to present a certificate.
+        (is (= HttpURLConnection/HTTP_FORBIDDEN
+               (status-for app {:scheme :https :remote-addr "127.0.0.1"}))))))
+
+  (testing "Should exempt all cleartext requests when configured to"
+    (let [app (wrap-cert-authn ok-handler nil true)]
       (is (= HttpURLConnection/HTTP_OK
-             (:status (app (create-authorizing-request "foobar"))))))))
+             (status-for app {:scheme :http :remote-addr "192.0.2.1"})))
+      ;; Only cleartext is exempted
+      (is (= HttpURLConnection/HTTP_FORBIDDEN
+             (status-for app {:scheme :https :remote-addr "192.0.2.1"}))))))
 
 (deftest wrapping-cert-cn-extraction
   (with-redefs [get-cn-from-x509-certificate :cn]
@@ -158,7 +205,10 @@
         (is (nil? (authorizer-fn {:ssl-client-cn "foobar"})))
         (with-log-output logz
           (is (= 403 (:status (authorizer-fn {:ssl-client-cn "badguy"}))))
-          (is (= 1 (count (logs-matching #"^badguy rejected by certificate allowlist " @logz)))))))))
+          (is (= 1 (count (logs-matching #"^badguy rejected by certificate allowlist " @logz)))))
+        (testing "and reject a request with no client certificate"
+          (is (= 403 (:status (authorizer-fn {}))))
+          (is (= 403 (:status (authorizer-fn {:scheme :http})))))))))
 
 (deftest test-fail-when-payload-too-large
   (testing "max-command-size-fail disabled should allow commands of any size"