Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(PE-36952) ensure infra-serial is up to date #2781

Merged
merged 2 commits into from
Oct 6, 2023
Merged

(PE-36952) ensure infra-serial is up to date #2781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfbd43f
(PE-36952) ensure infra-serials are up to date
jonathannewman Oct 5, 2023
83bc33b
(PE-36952) ensure all infrastructure serials are written
jonathannewman Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 136 additions & 60 deletions src/clj/puppetlabs/puppetserver/certificate_authority.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,6 @@
{}
auth-exts)))

(defn seq-contains? [coll target] (some #(= target %) coll))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't used, so I removed it.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Writing various SSL objects safely
;; These versions all encode writing atomically and knowledge of file permissions
Expand Down Expand Up @@ -658,15 +656,64 @@
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Inventory File

(def inventory-date-formatter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the formatter once and reusing it saves some time and memory churn.

(time-format/formatter "YYY-MM-dd'T'HH:mm:ssz"))

(schema/defn format-date-time :- schema/Str
"Formats a date-time into the format expected by the ruby puppet code."
[date-time :- Date]
(time-format/unparse
(time-format/formatter "YYY-MM-dd'T'HH:mm:ssz")
inventory-date-formatter
(time-coerce/from-date date-time)))

(schema/defn parse-date-time :- DateTime
"parses a date-time string into a DateTime instance"
[date-time :- schema/Str]
(time-format/parse inventory-date-formatter date-time))

(def buffer-copy-size (* 64 1024))

(schema/defn read-infra-nodes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved up from below.

"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.
Expand All @@ -682,31 +729,33 @@
* $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))
not-after (-> cert
(.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)]
(try
(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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was incorrectly handling the 0 result case.

(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
Expand All @@ -717,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
Expand All @@ -727,6 +777,14 @@
(= (subs row-subject 1) cn-subject)
false))

(schema/defn is-not-expired? :- schema/Bool
jonathannewman marked this conversation as resolved.
Show resolved Hide resolved
[now :- DateTime
[_serial _not-before not-after _row-subject] :- [schema/Str]]
(if-let [not-after-date (parse-date-time not-after)]
(time/before? now not-after-date)
;; lack of an end date means we can't tell if it is expired or not, so assume it isn't.
false))

(defn extract-inventory-row-contents
[row]
(str/split row #" "))
Expand All @@ -736,26 +794,36 @@
certname :- schema/Str]
(common/with-safe-read-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds
(log/trace (i18n/trs "Looking for \"{0}\" in inventory file {1}" certname cert-inventory))
(with-open [inventory-reader (io/reader cert-inventory)]
(let [inventory-rows (map extract-inventory-row-contents (line-seq inventory-reader))
cn-subject (utils/cn certname)]
(some? (some (partial is-subject-in-inventory-row? cn-subject) inventory-rows))))))
(if (fs/exists? cert-inventory)
(with-open [inventory-reader (io/reader cert-inventory)]
(let [inventory-rows (map extract-inventory-row-contents (line-seq inventory-reader))
cn-subject (utils/cn certname)]
(some? (some (partial is-subject-in-inventory-row? cn-subject) inventory-rows))))
(do
(log/debug "Unable to find inventory file {0}" cert-inventory)
false))))

(schema/defn find-matching-valid-serial-numbers :- [BigInteger]
[{:keys [cert-inventory inventory-lock inventory-lock-timeout-seconds]} :- CaSettings
certname :- schema/Str]
(common/with-safe-read-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds
(log/trace (i18n/trs "Looking for serial numbers for \"{0}\" in inventory file {1}" certname cert-inventory))
(with-open [inventory-reader (io/reader cert-inventory)]
(let [inventory-rows (map extract-inventory-row-contents (line-seq inventory-reader))
cn-subject (utils/cn certname)]
(doall
(->> inventory-rows
(filter (partial is-subject-in-inventory-row? cn-subject))
;; serial number is first entry in the array
(map first)
;; assume serials are base 16 strings, drop the `0x` as BigInteger won't deal with them.
(map #(BigInteger. ^String (subs % 2) 16))))))))
(if (fs/exists? cert-inventory)
(with-open [inventory-reader (io/reader cert-inventory)]
(let [inventory-rows (map extract-inventory-row-contents (line-seq inventory-reader))
cn-subject (utils/cn certname)
now (time/now)]
(doall
(->> inventory-rows
(filter (partial is-subject-in-inventory-row? cn-subject))
(filter (partial is-not-expired? now))
;; serial number is first entry in the array
(map first)
;; assume serials are base 16 strings, drop the `0x` as BigInteger won't deal with them.
(map #(BigInteger. ^String (subs % 2) 16))))))
(do
(log/debug (i18n/trs "Unable to find inventory file {0}" cert-inventory))
[]))))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Initialization
Expand Down Expand Up @@ -818,41 +886,49 @@
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))

(defn- write-infra-serials-to-writer
[writer infra-nodes-path signeddir]
(try
(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,
Also check the filesystem for a signed cert for that node. Return a sorted unique
set of serial numbers for nodes in the infra file"
[{:keys [infra-nodes-path signeddir] :as settings} :- CaSettings]
(if (fs/exists? infra-nodes-path)
(with-open [infra-nodes-reader (io/reader infra-nodes-path)]
(let [infra-nodes (read-infra-nodes infra-nodes-reader)]
(doseq [infra-node infra-nodes]
(try
(let [infra-serial (-> (path-to-cert signeddir infra-node)
(utils/pem->cert)
(utils/get-serial))]
(.write writer (str infra-serial))
(.newLine writer))
(catch FileNotFoundException _
(log/warn
(i18n/trs
(str
"Failed to find/load certificate for Puppet Infrastructure Node:"
infra-node))))))))
(catch FileNotFoundException _
(log/warn (i18n/trs (str infra-nodes-path " does not exist"))))))

(schema/defn generate-infra-serials
(loop [local-infra-nodes infra-nodes
result []]
(if-let [infra-node (first local-infra-nodes)]
(let [augmented-results (concat result (find-matching-valid-serial-numbers settings infra-node))
cert-path (path-to-cert signeddir infra-node)]
(if (fs/exists? cert-path)
(let [serial (-> cert-path
utils/pem->cert
utils/get-serial)]
(recur (rest local-infra-nodes)
(conj augmented-results serial)))
(do
(log/warn (i18n/trs "Failed to find certificate for Puppet Infrastructure Node: {0}" infra-node))
(recur (rest local-infra-nodes)
augmented-results))))
;; ensure they are unique and ordered so the end result is stable
(sort (set result))))))
[]))

(schema/defn write-infra-serials-to-writer
[writer :- BufferedWriter
settings :- CaSettings]
(let [infra-serials (extract-active-infra-serials settings)]
(doseq [infra-serial infra-serials]
(.write writer (str infra-serial))
(.newLine writer))))

(schema/defn generate-infra-serials!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this by adding the ! suffix to indicate that it makes changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a sanity check here:
When a cert is renewed the new serial will be appended to the infra-serials file, but when the CA restarts, reloads, or the infra node certname file is updated the whole infra-serials file will be rewritten with the latest serial for each infra node. That's the intended behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the startup case, with these changes to generate-infra-serials! we are now looking in the inventory as well for matching serial numbers and adding them. So at startup there should be technically no work to do, unless someone manually changed the files. The idea is that the infra-serial should always be more correct now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to answer your question, the intent is that we append the infra-serial on demand when a new one is generated, and when we restart, we update the state based on the current state on the filesystem, including all known valid serial numbers for the infra-nodes, not just the most recent one. This helps at revocation time to ensure the infra-crl is updated with all possible certs for the host being revoked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that.

"Given a list of infra nodes it will create a file containing
serial numbers of their certificates (listed on separate lines).
It is expected have at least one entry (MoM)"
[{:keys [infra-nodes-path infra-node-serials-path signeddir]} :- CaSettings]
[{:keys [infra-node-serials-path] :as settings} :- CaSettings]
(ks-file/atomic-write infra-node-serials-path
#(write-infra-serials-to-writer %
infra-nodes-path
signeddir)
#(write-infra-serials-to-writer % settings)
public-key-perms))

(defn symlink-cadir
Expand Down Expand Up @@ -888,7 +964,7 @@
(-> ca-settings :signeddir fs/file ks/mkdirs!)
(initialize-serial-file! ca-settings)
(-> ca-settings :infra-nodes-path fs/file fs/create)
(generate-infra-serials ca-settings)
(generate-infra-serials! ca-settings)
(let [keypair (utils/generate-key-pair (:keylength ca-settings))
public-key (utils/get-public-key keypair)
private-key (utils/get-private-key keypair)
Expand Down Expand Up @@ -1835,7 +1911,7 @@
(do
(log/info (i18n/trs "CA already initialized for SSL"))
(when (:enable-infra-crl settings)
(generate-infra-serials settings))
(generate-infra-serials! settings))
(when (:manage-internal-file-permissions settings)
(ensure-ca-file-perms! settings)))
(let [{found true
Expand Down
2 changes: 1 addition & 1 deletion src/clj/puppetlabs/services/ca/certificate_authority_service.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
(= (.getCanonicalPath (:changed-path %))
infra-nodes-file))
events)
(ca/generate-infra-serials settings))))
(ca/generate-infra-serials! settings))))
(register-status
"ca"
(status-core/get-artifact-version "puppetlabs" "puppetserver")
Expand Down
69 changes: 68 additions & 1 deletion test/unit/puppetlabs/puppetserver/certificate_authority_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
Loading
Loading