From 7d8a587b724191a3441729754be5e0e5d354fe1c Mon Sep 17 00:00:00 2001 From: Steven Pritchard Date: Tue, 4 Aug 2026 18:40:05 +0000 Subject: [PATCH 1/2] Authenticate every request by client certificate OpenVoxDB accepted any request that reached it whenever [puppetdb] certificate-allowlist was unset, because wrap-cert-authn returned the handler unwrapped in that case. The HTTPS listener is configured with client-auth = want rather than need, so a client presenting no certificate at all could read the whole database, submit commands for any certname, and delete node data. The allowlist was also bypassed for cleartext requests, since kitchensink's cn-whitelist->authorizer authorizes any request whose scheme is :http. An operator who configured an allowlist got no enforcement on the cleartext port, and no indication of that. Authentication is now always applied. A request must present a certificate signed by OpenVoxDB's CA, and where an allowlist is configured its CN must appear there, with the allowlist checked locally rather than through the scheme-sensitive kitchensink helper. A cleartext connection cannot present a certificate, so cleartext requests are exempt only when they come from a loopback address, which is what the shipped configuration expects: [jetty] host defaults to localhost and the port exists for local administration and the performance dashboard. Operators who terminate authentication elsewhere, such as behind a reverse proxy on another host, can set the new [puppetdb] allow-unauthenticated-cleartext to exempt the port entirely; that logs a warning at startup. This is a breaking change for deployments that rely on unauthenticated access: anonymous HTTPS clients and remote cleartext clients are now rejected with a 403 rather than served. Co-Authored-By: Claude Fable 5 Signed-off-by: Steven Pritchard --- config.sample.ini | 7 +- documentation/configure.markdown | 33 +++++++- documentation/maintain_and_tune.markdown | 12 ++- src/puppetlabs/puppetdb/config.clj | 2 + src/puppetlabs/puppetdb/middleware.clj | 84 ++++++++++++++++---- src/puppetlabs/puppetdb/pdb_routing.clj | 8 +- test/puppetlabs/puppetdb/middleware_test.clj | 72 ++++++++++++++--- 7 files changed, 180 insertions(+), 38 deletions(-) diff --git a/config.sample.ini b/config.sample.ini index 67c1419ce..15d97f9b4 100644 --- a/config.sample.ini +++ b/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 diff --git a/documentation/configure.markdown b/documentation/configure.markdown index a92c261dd..dbcfbfba9 100644 --- a/documentation/configure.markdown +++ b/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 diff --git a/documentation/maintain_and_tune.markdown b/documentation/maintain_and_tune.markdown index d9cb276dc..232baf3f1 100644 --- a/documentation/maintain_and_tune.markdown +++ b/documentation/maintain_and_tune.markdown @@ -28,9 +28,15 @@ example, ssh -L 8080:localhost:8080 root@ 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://: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://: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. diff --git a/src/puppetlabs/puppetdb/config.clj b/src/puppetlabs/puppetdb/config.clj index 02b221ae9..f336aaecd 100644 --- a/src/puppetlabs/puppetdb/config.clj +++ b/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 diff --git a/src/puppetlabs/puppetdb/middleware.clj b/src/puppetlabs/puppetdb/middleware.clj index 2b6aa5f19..4ec29dd08 100644 --- a/src/puppetlabs/puppetdb/middleware.clj +++ b/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 diff --git a/src/puppetlabs/puppetdb/pdb_routing.clj b/src/puppetlabs/puppetdb/pdb_routing.clj index 243aec495..ca95ba35d 100644 --- a/src/puppetlabs/puppetdb/pdb_routing.clj +++ b/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) diff --git a/test/puppetlabs/puppetdb/middleware_test.clj b/test/puppetlabs/puppetdb/middleware_test.clj index e219e4e07..be893052b 100644 --- a/test/puppetlabs/puppetdb/middleware_test.clj +++ b/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" From 29e6c85f66bb4762075186128c6e824f7f28fd21 Mon Sep 17 00:00:00 2001 From: Steven Pritchard Date: Tue, 4 Aug 2026 19:56:39 +0000 Subject: [PATCH 2/2] Address review feedback on certificate authentication Make reject-unauthenticated-request and loopback-request? private, as both are only used within middleware.clj, and cover the defaulting and boolean coercion of allow-unauthenticated-cleartext in config-test. Co-Authored-By: Claude Fable 5 Signed-off-by: Steven Pritchard --- src/puppetlabs/puppetdb/middleware.clj | 4 ++-- test/puppetlabs/puppetdb/config_test.clj | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/puppetlabs/puppetdb/middleware.clj b/src/puppetlabs/puppetdb/middleware.clj index 4ec29dd08..ad723ca67 100644 --- a/src/puppetlabs/puppetdb/middleware.clj +++ b/src/puppetlabs/puppetdb/middleware.clj @@ -46,7 +46,7 @@ (tru "OpenVoxDB requires clients to present a certificate signed by its CA, and this request presented none.") HttpURLConnection/HTTP_FORBIDDEN)) -(defn reject-unauthenticated-request +(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]}] @@ -72,7 +72,7 @@ (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? +(defn- loopback-request? "True if req arrived from a loopback address." [{:keys [remote-addr]}] (boolean diff --git a/test/puppetlabs/puppetdb/config_test.clj b/test/puppetlabs/puppetdb/config_test.clj index 4d76441fe..4c000ff80 100644 --- a/test/puppetlabs/puppetdb/config_test.clj +++ b/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"