From 82305eb66aaafad5a38b0dc16ea385c0f22dad1d Mon Sep 17 00:00:00 2001 From: jonathannewman Date: Thu, 5 Oct 2023 16:26:57 -0700 Subject: [PATCH] (PE-36952) ensure all infrastructure serials are written This adds a new behavior such that when a renewal or signing is done and the host in question is in the infrastructure list, the serial number for that signed cert is immediately written to the infra-serial file. This has the benefit of ensuring that all certs will be correctly revoked in the infra-crl when it is updated on revocation. --- .../puppetserver/certificate_authority.clj | 73 ++++++++++++++----- .../certificate_authority_test.clj | 69 +++++++++++++++++- .../puppetlabs/services/ca/ca_testutils.clj | 2 +- 3 files changed, 125 insertions(+), 19 deletions(-) diff --git a/src/clj/puppetlabs/puppetserver/certificate_authority.clj b/src/clj/puppetlabs/puppetserver/certificate_authority.clj index 06dd69384..b547ea6a5 100644 --- a/src/clj/puppetlabs/puppetserver/certificate_authority.clj +++ b/src/clj/puppetlabs/puppetserver/certificate_authority.clj @@ -673,6 +673,47 @@ (def buffer-copy-size (* 64 1024)) +(schema/defn read-infra-nodes + "Returns a list of infra nodes or infra node serials from the specified file organized as one item per line." + [infra-file-reader :- Reader] + (line-seq infra-file-reader)) + +(schema/defn maybe-write-to-infra-serial! + "Determine if the host in question is an infra host, and if it is, add the provided serial number to the + infra-serials file" + [serial :- BigInteger + certname :- schema/Str + {:keys [infra-nodes-path infra-node-serials-path]} :- CaSettings] + (when (fs/exists? infra-nodes-path) + (with-open [infra-nodes-reader (io/reader infra-nodes-path)] + (let [infra-nodes (read-infra-nodes infra-nodes-reader)] + (when (seq (filter #(= certname %) infra-nodes)) + (log/debug (i18n/trs "Appending serial number {0} for {1} " serial certname)) + (let [stream-content-fn + (fn [^BufferedWriter writer] + (log/trace (i18n/trs "Begin append to serial file.")) + (let [copy-buffer (CharBuffer/allocate buffer-copy-size)] + (try + (with-open [^BufferedReader reader (io/reader infra-node-serials-path)] + ;; copy all the existing content + (loop [read-length (.read reader copy-buffer)] + ;; theoretically read can return 0, which means try again + (when (<= 0 read-length) + (when (pos? read-length) + (.write writer (.array copy-buffer) 0 read-length)) + (.clear copy-buffer) + (recur (.read reader copy-buffer))))) + (catch FileNotFoundException _e + (log/trace (i18n/trs "Inventory serial file not found. Assume empty."))) + (catch Throwable e + (log/error e (i18n/trs "Error while appending to serial file.")) + (throw e)))) + (.write writer (str serial)) + (.newLine writer) + (.flush writer) + (log/trace (i18n/trs "Finish append to serial file. ")))] + (ks-file/atomic-write infra-node-serials-path stream-content-fn))))))) + (schema/defn ^:always-validate write-cert-to-inventory! "Writes an entry into Puppet's inventory file for a given certificate. @@ -688,11 +729,11 @@ * $NA = The 'not after' field of the cert, as a date/timestamp in UTC. * $S = The distinguished name of the cert's subject." [cert :- Certificate - {:keys [cert-inventory inventory-lock inventory-lock-timeout-seconds]} :- CaSettings] - (let [serial-number (->> cert - (.getSerialNumber) - (format-serial-number) - (str "0x")) + {:keys [cert-inventory inventory-lock inventory-lock-timeout-seconds] :as settings} :- CaSettings] + (let [serial-number (.getSerialNumber cert) + formatted-serial-number (->> serial-number + (format-serial-number) + (str "0x")) not-before (-> cert (.getNotBefore) (format-date-time)) @@ -700,7 +741,8 @@ (.getNotAfter) (format-date-time)) subject (utils/get-subject-from-x509-certificate cert) - entry (str serial-number " " not-before " " not-after " /" subject "\n") + cert-name (utils/x500-name->CN subject) + entry (str formatted-serial-number " " not-before " " not-after " /" subject "\n") stream-content-fn (fn [^BufferedWriter writer] (log/trace (i18n/trs "Begin append to inventory file.")) (let [copy-buffer (CharBuffer/allocate buffer-copy-size)] @@ -708,11 +750,12 @@ (with-open [^BufferedReader reader (io/reader cert-inventory)] ;; copy all the existing content (loop [read-length (.read reader copy-buffer)] - (when (< 0 read-length) - (when (pos? read-length) - (.write writer (.array copy-buffer) 0 read-length)) - (.clear copy-buffer) - (recur (.read reader copy-buffer))))) + ;; theoretically read can return 0, which means try again + (when (<= 0 read-length) + (when (pos? read-length) + (.write writer (.array copy-buffer) 0 read-length)) + (.clear copy-buffer) + (recur (.read reader copy-buffer))))) (catch FileNotFoundException _e (log/trace (i18n/trs "Inventory file not found. Assume empty."))) (catch Throwable e @@ -723,7 +766,8 @@ (log/trace (i18n/trs "Finish append to inventory file. ")))] (common/with-safe-write-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds (log/debug (i18n/trs "Append \"{1}\" to inventory file {0}" cert-inventory entry)) - (ks-file/atomic-write cert-inventory stream-content-fn)))) + (ks-file/atomic-write cert-inventory stream-content-fn)) + (maybe-write-to-infra-serial! serial-number cert-name settings))) (schema/defn is-subject-in-inventory-row? :- schema/Bool [cn-subject :- utils/ValidX500Name @@ -841,11 +885,6 @@ netscape-comment-value) (utils/create-ca-extensions issuer-public-key ca-public-key)))) -(schema/defn read-infra-nodes - "Returns a list of infra nodes or infra node serials from the specified file organized as one item per line." - [infra-file-reader :- Reader] - (line-seq infra-file-reader)) - (schema/defn extract-active-infra-serials :- [BigInteger] "Read the infra nodes file to determine which nodes are infrastructure nodes. For each node, check the inventory file for any serial numbers for that node, diff --git a/test/unit/puppetlabs/puppetserver/certificate_authority_test.clj b/test/unit/puppetlabs/puppetserver/certificate_authority_test.clj index 71a264d16..9caa16f93 100644 --- a/test/unit/puppetlabs/puppetserver/certificate_authority_test.clj +++ b/test/unit/puppetlabs/puppetserver/certificate_authority_test.clj @@ -2033,7 +2033,74 @@ ;; there are four fields, serial number, not before, not after, and subject ;; for ease of testing, just test the first and last (is (= "0x0006" (first last-entry-fields))) - (is (= "/CN=foo" (last last-entry-fields))))))))) + (is (= "/CN=foo" (last last-entry-fields))))) + (testing "the new entry is not in the infra-serial file" + (is (= "" (slurp (:infra-node-serials-path settings)))))))) + (testing "infra inventory correctly writes files" + (let [settings (testutils/ca-sandbox! cadir) + ;; auto-renewal-cert-ttl is expected to be an int + ;; unit tests skip some of the conversion flow so + ;; transform the duration here + converted-auto-renewal-cert-ttl (ca/duration-str->sec (:auto-renewal-cert-ttl settings)) + updated-settings (assoc settings :auto-renewal-cert-ttl converted-auto-renewal-cert-ttl) + ;; simulate the node being in the infra inventory file + _ (spit (:infra-nodes-path settings) "bar\n") + ca-cert (create-ca-cert "ca1" 1) + keypair (utils/generate-key-pair) + subject (utils/cn "bar") + csr (utils/generate-certificate-request keypair subject) + validity (ca/cert-validity-dates 3600) + signed-cert (utils/sign-certificate + (utils/get-subject-from-x509-certificate (:cert ca-cert)) + (:private-key ca-cert) + (ca/next-serial-number! settings) + (:not-before validity) + (:not-after validity) + subject + (utils/get-public-key csr) + (ca/create-agent-extensions csr (:cert ca-cert))) + expected-cert-path (ca/path-to-cert (:signeddir settings) "bar")] + (testing "simulate the cert being written" + (ca/write-cert signed-cert expected-cert-path) + (is (fs/exists? expected-cert-path))) + (Thread/sleep 1000) ;; ensure there is some time elapsed between the two + (let [renewed-cert (ca/renew-certificate! signed-cert updated-settings (constantly nil))] + (is (some? renewed-cert)) + (testing "serial number has increased" + (is (< (.getSerialNumber signed-cert) (.getSerialNumber renewed-cert))) + (is (= 6 (.getSerialNumber renewed-cert)))) + (testing "not before time stamps have changed" + (is (= -1 (.compareTo (.getNotBefore signed-cert) (.getNotBefore renewed-cert))))) + (testing "new not-after is later than before" + (is (= -1 (.compareTo (.getNotAfter signed-cert) (.getNotAfter renewed-cert))))) + (testing "new not-after should be 89 days (and some faction) away" + (let [diff (- (.getTime (.getNotAfter renewed-cert)) (.getTime (Date.))) + days (.convert TimeUnit/DAYS diff TimeUnit/MILLISECONDS)] + (is (= 89 days)))) + (testing "certificate should have been replaced" + (is (fs/exists? expected-cert-path)) + (testing "updated cert on disk matches renewed cert" + (let [updated-cert (utils/pem->cert expected-cert-path)] + (is (= 6 (.getSerialNumber updated-cert))) + (is (zero? (.compareTo (.getNotBefore updated-cert) (.getNotBefore renewed-cert)))) + (is (zero? (.compareTo (.getNotAfter updated-cert) (.getNotAfter renewed-cert))))))) + (testing "extensions are preserved" + (let [extensions-before (utils/get-extensions signed-cert) + extensions-after (utils/get-extensions signed-cert)] + ;; ordering may be different so use an unordered comparison + (is (= (set extensions-before) + (set extensions-after))))) + (testing "the new entry is written to the inventory file" + (let [entries (string/split (slurp (:cert-inventory settings)) #"\n") + last-entry-fields (string/split (last entries) #" ")] + ;; since the content of the inventory is well established (because of the sandbox), we can + ;; just assert that the last entry is there, and makes sense + ;; there are four fields, serial number, not before, not after, and subject + ;; for ease of testing, just test the first and last + (is (= "0x0006" (first last-entry-fields))) + (is (= "/CN=bar" (last last-entry-fields))))) + (testing "the new entry is in the infra-serial file" + (is (= "6\n" (slurp (:infra-node-serials-path settings))))))))) (deftest supports-auto-renewal?-test (let [keypair (utils/generate-key-pair) subject (utils/cn "foo")] diff --git a/test/unit/puppetlabs/services/ca/ca_testutils.clj b/test/unit/puppetlabs/services/ca/ca_testutils.clj index 6032e63de..37c458db8 100644 --- a/test/unit/puppetlabs/services/ca/ca_testutils.clj +++ b/test/unit/puppetlabs/services/ca/ca_testutils.clj @@ -73,7 +73,7 @@ :serial (str cadir "/serial") :ruby-load-path jruby-testutils/ruby-load-path :gem-path jruby-testutils/gem-path - :infra-nodes-path (str cadir "/ca/infra_inventory.txt") + :infra-nodes-path (str cadir "/infra_inventory.txt") :infra-node-serials-path (str cadir "/infra_serials") :infra-crl-path (str cadir "/infra_crl.pem") :enable-infra-crl false