Allow commands to be bound to the submitter's certificate

config.sample.ini

@@ -12,6 +12,11 @@ vardir = /var/lib/puppetdb
 # List of certificate names from which to allow incoming HTTPS requests:
 # certificate-allowlist = /path/to/certname/allowlist
 
+# List of certificate names permitted to submit commands on behalf of other
+# nodes, such as your OpenVox Server. Any other client may then only submit
+# commands for itself:
+# trusted-submitter-allowlist = /path/to/submitter/allowlist
+
 [database]
 
 # Subname pattern: //host:port/databaseName

documentation/configure.markdown

@@ -248,6 +248,43 @@ If not supplied, OpenVoxDB uses standard HTTPS without any additional
 authorization. All HTTPS clients must still supply valid, verifiable
 SSL client certificates.
 
+### `trusted-submitter-allowlist`
+
+Optional. This describes the path to a file that contains a list of certificate
+names, one per line, that are permitted to submit
+[commands](./api/command/v1/commands.markdown) on behalf of other nodes.
+
+Commands name the node they apply to in their `certname` parameter, and
+OpenVoxDB does not otherwise require that parameter to have anything to do with
+the certificate the command was submitted with. Any client allowed to submit
+commands can therefore replace the facts, catalog, or reports of any node in the
+fleet, or deactivate it.
+
+When this setting is supplied, a client whose certificate name does not appear in
+the file may only submit commands whose `certname` is its own certificate name,
+and is rejected with a 403 otherwise. OpenVox Server submits commands for every
+agent whose catalog it compiles, so its certname belongs in this file, as does
+the certname of anything else that submits on behalf of other nodes, such as an
+OpenVoxDB sync or migration tool.
+
+The `certname` parameter only names the queue entry, though. What actually gets
+stored is the certname inside the command's payload, so this setting also
+requires the two to agree. That comparison cannot happen when the command is
+submitted, because OpenVoxDB streams the payload to disk without reading it. A
+command whose payload names a different node is therefore accepted, queued, and
+then discarded when it is processed, with a fatal error naming both certnames.
+The discarded command is kept in the [dead letter
+office](./maintain_and_tune.markdown#clean-up-the-dead-letter-office).
+
+If not supplied, neither check applies: any client that reaches the command
+endpoint may submit commands for any certname, which is the behavior of earlier
+versions.
+
+Requests that were not authenticated with a certificate have no certificate name
+to compare against and are not restricted by this setting. Whether they are
+accepted at all is decided by
+[`certificate-allowlist`](#certificate-allowlist).
+
 ### `log-queries`
 Optional. Setting this to `true` will enable debug level logging of the internal
 AST and SQL that OpenVoxDB generates for all queries. This can be useful when

src/puppetlabs/puppetdb/command.clj

@@ -527,6 +527,32 @@
     "configure expiration" (prep-configure-expiration cmd)
     "replace catalog inputs" (prep-replace-catalog-inputs cmd)))
 
+(defn check-payload-certname
+  "Throws a fatal error if the certname in cmd's payload is not the
+  certname cmd was submitted for.  Does nothing unless the operator has
+  opted into binding commands to their submitter.
+
+  The command endpoint binds the submitted certname to the submitter's
+  certificate, but the storage functions act on the certname in the
+  payload, so the two must agree for that binding to constrain what a
+  submitter can change.  Nothing else requires them to agree, since the
+  submitted certname otherwise only names the queue entry.
+
+  The payload must already have been normalized to the latest wire
+  format, because earlier formats either name the certname differently
+  or, for deactivate node, do not carry a map at all."
+  [{:keys [certname payload] :as cmd}
+   {:keys [enforce-submitter-binding?] :as _options-config}]
+  (when enforce-submitter-binding?
+    (let [payload-certname (:certname payload)]
+      (when-not (queue/certname-matches-cmdref? cmd payload-certname)
+        (throw
+         (fatality
+          (ex-info (trs "payload names {0} but the command was submitted for {1}"
+                        (pr-str payload-certname) (pr-str certname))
+                   {:puppetlabs.puppetdb/known-error? true}))))))
+  cmd)
+
 (defn supported-command? [{:keys [command version] :as _cmd}]
   (some-> (supported-command-versions command) (get version)))
 
@@ -875,6 +901,7 @@
                                           options-config maybe-send-cmd-event!))
           :else (-> cmd
                     (prep-command options-config)
+                    (check-payload-certname options-config)
                     (process-cmd cmdref q write-dbs broadcast-pool response-chan
                                  stats maybe-send-cmd-event!
                                  shutdown-for-ex options-config))))
@@ -1003,7 +1030,13 @@
       :database
       (select-keys [:facts-blocklist
                     :facts-blocklist-type
-                    :resource-events-ttl])))
+                    :resource-events-ttl])
+      ;; Setting the allowlist is how an operator opts into binding
+      ;; commands to their submitter, which the command endpoint can
+      ;; only enforce for the submitted certname (see
+      ;; check-payload-certname).
+      (assoc :enforce-submitter-binding?
+             (boolean (conf/trusted-submitter-allowlist config)))))
 
 (defn start-command-service
   [context config {:keys [dlo] :as globals} request-shutdown]

src/puppetlabs/puppetdb/config.clj

@@ -204,6 +204,7 @@
   (all-optional
    {:certificate-whitelist s/Str
     :certificate-allowlist s/Str
+    :trusted-submitter-allowlist s/Str
     :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
+   (s/optional-key :trusted-submitter-allowlist) s/Str
    :add-agent-report-filter Boolean
    :log-queries Boolean
    :query-timeout-default s/Num
@@ -715,6 +717,10 @@
   [config]
   (get-in config [:command-processing :max-command-size]))
 
+(defn trusted-submitter-allowlist
+  [config]
+  (get-in config [:puppetdb :trusted-submitter-allowlist]))
+
 (defn stockpile-dir [config]
   (str (io/file (get-in config [:global :vardir]) "stockpile")))
 

src/puppetlabs/puppetdb/http/command.clj

@@ -96,6 +96,42 @@
 
        :else (handler request)))))
 
+(defn- build-submitter-authorizer
+  "Returns a predicate that is true for a certname permitted to submit commands
+  on behalf of other nodes. When allowlist is nil no restriction applies and
+  every submitter is permitted."
+  [allowlist]
+  (if allowlist
+    (let [trusted? (set (kitchensink/lines allowlist))]
+      (fn [submitter] (boolean (trusted? submitter))))
+    (constantly true)))
+
+(defn- wrap-with-submitter-authorization
+  "Rejects a command whose certname parameter names a node other than the
+  submitter, unless the submitter appears in the trusted submitter allowlist.
+  OpenVox Server submits commands for the agents it compiles catalogs for, so
+  its certname belongs in that allowlist.
+
+  Requests that did not authenticate with a certificate carry no certname to
+  compare against and are not restricted here; whether such a request is
+  accepted at all is decided by the certificate authentication middleware.
+  This middleware should ingest the request after parameter validation."
+  [handler submits-for-other-nodes?]
+  (fn authorize-submitter
+    [{:keys [params ssl-client-cn] :as request}]
+    (let [certname (params "certname")]
+      (if (or (nil? ssl-client-cn)
+              (= certname ssl-client-cn)
+              (submits-for-other-nodes? ssl-client-cn))
+        (handler request)
+        (do
+          (log/warn (trs "{0} rejected: not a trusted submitter, so it may only submit commands for itself, not for {1}"
+                         ssl-client-cn certname))
+          (http/denied-response
+           (tru "The client certificate name {0} may only submit commands for itself, not for {1}. Is it listed in OpenVoxDB''s trusted-submitter-allowlist file?"
+                ssl-client-cn certname)
+           HttpURLConnection/HTTP_FORBIDDEN))))))
+
 (defmacro with-chan
   "Bind chan-sym to init-chan in the scope of the body, calling async/close! in
   a finally block.
@@ -315,11 +351,14 @@
 ;; return functions that accept a ring request map
 
 (defn command-app
-  [get-shared-globals enqueue-fn reject-large-commands? max-command-size]
+  [get-shared-globals enqueue-fn reject-large-commands? max-command-size
+   trusted-submitter-allowlist]
   (-> (routes enqueue-fn
               (when reject-large-commands? max-command-size))
       mid/make-pdb-handler
       add-received-param ;; must be (temporally) after wrap-with-request-params-validation
+      (wrap-with-submitter-authorization
+       (build-submitter-authorizer trusted-submitter-allowlist))
       wrap-with-request-params-validation
       wrap-with-request-normalization
       rmc/wrap-accepts-json

src/puppetlabs/puppetdb/pdb_routing.clj

@@ -39,7 +39,8 @@
      (wrap-with-context "/meta" (meta/build-app))
      (wrap-with-context "/cmd" (cmd/command-app get-shared-globals enqueue-command-fn
                                       (conf/reject-large-commands? defaulted-config)
-                                      (conf/max-command-size defaulted-config)))
+                                      (conf/max-command-size defaulted-config)
+                                      (conf/trusted-submitter-allowlist defaulted-config)))
      (wrap-with-context "/query" (server/build-app get-shared-globals))
      (wrap-with-context "/admin" (admin/build-app enqueue-command-fn query-fn db-cfg clean-fn
                                         delete-node-fn))]))

src/puppetlabs/puppetdb/queue.clj

@@ -272,6 +272,18 @@
                       :callback identity
                       :compression compression})))
 
+(defn certname-matches-cmdref?
+  "True if certname is the certname the command described by cmdref was
+  submitted for.  A cmdref's :certname is really a certid, i.e. it may
+  be a sanitized and truncated proxy for the original certname (see
+  embeddable-certid), so certname is put through the same
+  transformation here rather than compared directly."
+  [cmdref certname]
+  (= (:certname cmdref)
+     (-> (serialize-metadata (:received cmdref) (assoc cmdref :certname certname) false)
+         parse-metadata
+         :certname)))
+
 (defn wrap-decompression-stream
   [file-extension command-stream]
   (condp = file-extension

test/puppetlabs/puppetdb/command_test.clj

@@ -1202,6 +1202,78 @@
       (let [result (query-to-vec "SELECT certname,environment_id FROM factsets")]
         (is (= result [(with-env {:certname certname})]))))))
 
+(deftest replace-facts-bound-to-submitter
+  (let [certname "foo.example.com"
+        values {"a" "1" "b" "2" "c" "3"}
+        facts {:certname certname
+               :environment "DEV"
+               :values values
+               :producer nil
+               :producer_timestamp (to-timestamp (now))}
+        ;; The submitted certname is the one the command endpoint binds to
+        ;; the submitter's certificate; the payload certname is the one
+        ;; that actually gets stored.
+        req (fn [submitted version payload]
+              (queue/create-command-req "replace facts" version submitted
+                                        (time/to-string (now)) "" identity
+                                        (tqueue/coerce-to-stream payload)))
+        bound (assoc blocklist-config :enforce-submitter-binding? true)]
+
+    (testing "a payload naming another node is discarded"
+      (with-redefs [blocklist-config bound]
+        (with-message-handler {:keys [handle-message dlo delay-pool q]}
+          (let [discards (discard-count)]
+            (handle-message
+             (queue/store-command q (req "submitter.example.com" 5 facts)))
+            (is (= (inc discards) (discard-count))))
+          (is (empty? (query-to-vec "SELECT * FROM factsets")))
+          (is (= 0 (task-count delay-pool)))
+          (is (= 2 (count (fs/list-dir (:path dlo))))))))
+
+    (testing "a payload naming another node is stored while unbound, since binding is opt-in"
+      (with-message-handler {:keys [handle-message dlo delay-pool q]}
+        (handle-message
+         (queue/store-command q (req "submitter.example.com" 5 facts)))
+        (is (= [{:certname certname :facts values}]
+               (query-factsets :certname :facts)))
+        (is (= 0 (task-count delay-pool)))
+        (is (empty? (fs/list-dir (:path dlo))))))
+
+    (testing "an agreeing payload is stored"
+      (with-redefs [blocklist-config bound]
+        (with-message-handler {:keys [handle-message dlo q]}
+          (handle-message (queue/store-command q (req certname 5 facts)))
+          (is (= [{:certname certname :facts values}]
+                 (query-factsets :certname :facts)))
+          (is (empty? (fs/list-dir (:path dlo)))))))
+
+    (testing "an agreeing payload is stored when the queue cannot record the certname verbatim"
+      (let [certname "host_0"]
+        (with-redefs [blocklist-config bound]
+          (with-message-handler {:keys [handle-message dlo q]}
+            (handle-message
+             (queue/store-command q (req certname 5 (assoc facts :certname certname))))
+            (is (= [{:certname certname :facts values}]
+                   (query-factsets :certname :facts)))
+            (is (empty? (fs/list-dir (:path dlo))))))))
+
+    (testing "an agreeing payload in an older wire format is stored"
+      ;; Those formats name the certname differently, so the check only
+      ;; works after the payload has been normalized.
+      (doseq [[version payload] {2 {:name certname
+                                    :environment "DEV"
+                                    :values values}
+                                 3 {:name certname
+                                    :environment "DEV"
+                                    :values values
+                                    :producer-timestamp (to-timestamp (now))}}]
+        (with-redefs [blocklist-config bound]
+          (with-message-handler {:keys [handle-message dlo q]}
+            (handle-message (queue/store-command q (req certname version payload)))
+            (is (= [{:certname certname :facts values}]
+                   (query-factsets :certname :facts)))
+            (is (empty? (fs/list-dir (:path dlo))))))))))
+
 (deftest replace-facts-bad-payload
   (dotestseq [_version fact-versions]
     (testing "should discard the message"

test/puppetlabs/puppetdb/http/command_test.clj

@@ -10,8 +10,10 @@
              :refer [get-request post-request
                      content-type uuid-in-response?
                      assert-success!
+                     temp-file
                      test-command-app
                      dotestseq]]
+            [puppetlabs.ssl-utils.core :refer [get-cn-from-x509-certificate]]
             [puppetlabs.kitchensink.core :as kitchensink]
             [puppetlabs.puppetdb.http :as http]
             [puppetlabs.stockpile.queue :as stock]
@@ -123,6 +125,54 @@
                                             payload))]
            (assert-success! response)))))))
 
+(deftest submitter-authorization
+  (let [allowlist (doto (.getAbsolutePath (temp-file "trusted-submitters"))
+                    (spit "puppetserver.example"))
+        payload (form-command "replace facts"
+                             (get min-supported-commands "replace facts")
+                             {:foo 1})
+        request (fn [submitter certname]
+                  (cond-> (post-request* "/v1"
+                                         {"version" (str (get min-supported-commands
+                                                              "replace facts"))
+                                          "certname" certname
+                                          "command" "replace facts"}
+                                         payload)
+                    submitter (assoc :ssl-client-cert {:cn submitter})))]
+    (with-redefs [get-cn-from-x509-certificate :cn]
+      (testing "with a trusted submitter allowlist"
+        (tqueue/with-stockpile q
+          (let [app (test-command-app q (async/chan 4) allowlist)]
+            (testing "an allowlisted submitter may submit for any certname"
+              (assert-success! (app (request "puppetserver.example" "agent.example"))))
+
+            (testing "any other submitter may only submit for itself"
+              (assert-success! (app (request "agent.example" "agent.example")))
+              (let [response (app (request "agent.example" "other.example"))]
+                (is (= HttpURLConnection/HTTP_FORBIDDEN (:status response)))
+                (is (re-find #"may only submit commands for itself"
+                             (:body response)))))
+
+            (testing "a request that presented no certificate is not restricted"
+              (assert-success! (app (request nil "other.example"))))
+
+            (testing "the certname is also checked for commands posted without
+                      query parameters, where it comes from the payload"
+              (let [response (app (-> (post-request*
+                                       "/v1" nil
+                                       (json/generate-string
+                                        {"command" "replace facts"
+                                         "version" (get min-supported-commands
+                                                        "replace facts")
+                                         "payload" {"certname" "other.example"}}))
+                                      (assoc :ssl-client-cert {:cn "agent.example"})))]
+                (is (= HttpURLConnection/HTTP_FORBIDDEN (:status response))))))))
+
+      (testing "without a trusted submitter allowlist any submitter may submit for any certname"
+        (tqueue/with-stockpile q
+          (let [app (test-command-app q (async/chan 4))]
+            (assert-success! (app (request "agent.example" "other.example")))))))))
+
 (def endpoint-error-specs
   [{:title "should 400 when missing payload"
     :params {}