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,32 @@ 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. 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.
+
+If not supplied, 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/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))]))

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

test/puppetlabs/puppetdb/testutils.clj

@@ -410,22 +410,25 @@
 
 (defn test-command-app
   "A fixture to build a Command app and make it available as
-  *command-app* within tests."
-  [q command-chan]
-  (wrap-with-puppetdb-middleware
-   (command-app
-    (fn [] {})
-    (fn [command version certname producer-ts stream compression callback]
-      (let [maybe-send-cmd-event! (constantly true)]
-        (dispatch/do-enqueue-command
-         q
-         command-chan
-         (Semaphore. 100)
-         (queue/create-command-req command version certname producer-ts compression callback stream)
-         maybe-send-cmd-event!)))
-
-    false
-    nil)))
+  *command-app* within tests.  Restricts which certnames may submit commands
+  for other nodes when given the path to a trusted submitter allowlist."
+  ([q command-chan] (test-command-app q command-chan nil))
+  ([q command-chan trusted-submitter-allowlist]
+   (wrap-with-puppetdb-middleware
+    (command-app
+     (fn [] {})
+     (fn [command version certname producer-ts stream compression callback]
+       (let [maybe-send-cmd-event! (constantly true)]
+         (dispatch/do-enqueue-command
+          q
+          command-chan
+          (Semaphore. 100)
+          (queue/create-command-req command version certname producer-ts compression callback stream)
+          maybe-send-cmd-event!)))
+
+     false
+     nil
+     trusted-submitter-allowlist))))
 
 (def default-timeout-ms
   (* 1000 60 5))