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.

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

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

Conversation

jonathannewman
Copy link
Contributor

Please see individual commit comments.

This updates the code that initializes and updates the infra-serial
file to ensure that the content contains all possible valid matches
for a given certname. Expired certs are removed as well.

Some extra checks were added to prevent trying to read files when
they don't exist.
@jonathannewman jonathannewman requested a review from a team as a code owner October 5, 2023 23:49
@@ -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.

@@ -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.

(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.

(.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.

@@ -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")
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 a typo.

(.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.

Copy link
Member

@justinstoller justinstoller left a comment

Choose a reason for hiding this comment

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

Looks good overall.

src/clj/puppetlabs/puppetserver/certificate_authority.clj Outdated Show resolved Hide resolved
(.write writer (str infra-serial))
(.newLine writer))))

(schema/defn generate-infra-serials!
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?

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.
@jonathannewman jonathannewman force-pushed the PE-36952/main/infra-inventory-updates branch from 82305eb to 83bc33b Compare October 6, 2023 17:47
@jonathannewman
Copy link
Contributor Author

Updated to address the two issues mentioned.

@jonathannewman jonathannewman merged commit fa5a05c into puppetlabs:main Oct 6, 2023
11 checks passed
@jonathannewman jonathannewman deleted the PE-36952/main/infra-inventory-updates branch October 6, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants