From f00e097d501973125cfe298a0da556f887a6a946 Mon Sep 17 00:00:00 2001 From: jonathannewman Date: Fri, 9 Feb 2024 09:42:47 -0800 Subject: [PATCH] (PE-37634) ensure sign-all with empty list succeeds Prior to this change, if the sign-all behavior was invoked for the CA service, an exception would be thrown because the function returns a result that wasn't expected. This adds a test that demonstrates the failure, and adds a fix to get the test to pass. --- .../puppetserver/certificate_authority.clj | 62 +++++++++---------- .../certificate_authority_int_test.clj | 13 ++++ 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/src/clj/puppetlabs/puppetserver/certificate_authority.clj b/src/clj/puppetlabs/puppetserver/certificate_authority.clj index c8a8b2b994..40cacc1f13 100644 --- a/src/clj/puppetlabs/puppetserver/certificate_authority.clj +++ b/src/clj/puppetlabs/puppetserver/certificate_authority.clj @@ -2483,35 +2483,35 @@ serial-lock serial-lock-timeout-seconds] :as ca-settings} :- CaSettings report-activity] (let [;; if part of a CA bundle, the intermediate CA will be first in the chain - cacert (utils/pem->ca-cert cacert cakey) + cacert (utils/pem->ca-cert cacert cakey) casubject (utils/get-subject-from-x509-certificate cacert) - ca-private-key (utils/pem->private-key cakey)] - (when-not (empty? subjects) - ;; since we are going to be manipulating the serial file and the inventory file for multiple entries, - ;; acquire the locks to prevent lock thrashing - (common/with-safe-write-lock serial-lock serial-lock-descriptor serial-lock-timeout-seconds - (common/with-safe-write-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds - (let [results - ;; loop through the subjects, one at a time, and collect the results for success or failure. - (loop [s subjects - result {:signed [] - :no-csr [] - :signing-errors[]}] - (if-not (empty? s) - (let [subject (first s) - csr-path (path-to-cert-request csrdir subject)] - (if (fs/exists? csr-path) - (let [_ (log/trace (i18n/trs "File exists at {0}" csr-path)) - one-result (maybe-sign-one subject csr-path cacert casubject ca-private-key ca-settings)] - ;; one-result is either :signed or :signing-errors - (recur (rest s) - (update result one-result conj subject))) - (do - (log/trace (i18n/trs "File does not exist at {0}" csr-path)) - (recur (rest s) - (update result :no-csr conj subject))))) - result))] - ;; submit the signing activity as one entry for all the hosts. - (when-not (empty? (:signed results)) - (report-activity (:signed results) "signed")) - results)))))) \ No newline at end of file + ca-private-key (utils/pem->private-key cakey)] + + ;; since we are going to be manipulating the serial file and the inventory file for multiple entries, + ;; acquire the locks to prevent lock thrashing + (common/with-safe-write-lock serial-lock serial-lock-descriptor serial-lock-timeout-seconds + (common/with-safe-write-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds + (let [results + ;; loop through the subjects, one at a time, and collect the results for success or failure. + (loop [s subjects + result {:signed [] + :no-csr [] + :signing-errors []}] + (if-not (empty? s) + (let [subject (first s) + csr-path (path-to-cert-request csrdir subject)] + (if (fs/exists? csr-path) + (let [_ (log/trace (i18n/trs "File exists at {0}" csr-path)) + one-result (maybe-sign-one subject csr-path cacert casubject ca-private-key ca-settings)] + ;; one-result is either :signed or :signing-errors + (recur (rest s) + (update result one-result conj subject))) + (do + (log/trace (i18n/trs "File does not exist at {0}" csr-path)) + (recur (rest s) + (update result :no-csr conj subject))))) + result))] + ;; submit the signing activity as one entry for all the hosts. + (when-not (empty? (:signed results)) + (report-activity (:signed results) "signed")) + results))))) \ No newline at end of file diff --git a/test/integration/puppetlabs/services/certificate_authority/certificate_authority_int_test.clj b/test/integration/puppetlabs/services/certificate_authority/certificate_authority_int_test.clj index 4c2474df1d..ab26e4f603 100644 --- a/test/integration/puppetlabs/services/certificate_authority/certificate_authority_int_test.clj +++ b/test/integration/puppetlabs/services/certificate_authority/certificate_authority_int_test.clj @@ -1227,6 +1227,19 @@ :ssl-key (str bootstrap/server-conf-dir "/ssl/private_keys/localhost.pem") :ssl-ca-cert (str bootstrap/server-conf-dir "/ca/ca_crt.pem") :ssl-crl-path (str bootstrap/server-conf-dir "/ssl/crl.pem")}} + (testing "PE-37634 PE-sign-all with no pending certs returns 200 with expected payload" + (let [response (http-client/post + "https://localhost:8140/puppet-ca/v1/sign/all" + {:ssl-cert (str bootstrap/server-conf-dir "/ca/ca_crt.pem") + :ssl-key (str bootstrap/server-conf-dir "/ca/ca_key.pem") + :ssl-ca-cert (str bootstrap/server-conf-dir "/ca/ca_crt.pem") + :as :text + :headers {"Accept" "application/json"}})] + (is (= 200 (:status response))) + (is (= {:signed [] + :no-csr [] + :signing-errors []} + (json/parse-string (:body response) true))))) (testing "returns 200 with valid payload" ;; note- more extensive testing of the behavior is done with the testing in sign-multiple-certificate-signing-requests!-test (let [certname (ks/rand-str :alpha-lower 16)