Skip to content

Commit

Permalink
(PE-36269) ensure CRLs are regenerated when nearing expiration
Browse files Browse the repository at this point in the history 
This adds a new behavior where once a day, puppetserver will check to
see if the crls for both the main crl, and if enabled the infra-crl
are nearing expiration.  If one is, the list of expired serial numbers
is collected from the inventory file, and used to prune the CRL list.

The CRL is then regenerated with the (potentially) smaller set of serials.

If the CRLs are not nearing expiration, nothing is done.

Tests are added to demonstrate the CRL behaviors added.
  • Loading branch information
@jonathannewman
jonathannewman committed Nov 14, 2023
1 parent 34e1853 commit 28b2114
Show file tree
Hide file tree
Showing 3 changed files with 255 additions and 3 deletions.
97 changes: 97 additions & 0 deletions src/clj/puppetlabs/puppetserver/certificate_authority.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
(java.security PrivateKey PublicKey)
(java.security.cert X509Certificate CRLException CertPathValidatorException X509CRL)
(java.text SimpleDateFormat)
(java.time LocalDateTime ZoneId)
(java.util Date)
(java.util.concurrent.locks ReentrantReadWriteLock)
(org.apache.commons.io IOUtils)
Expand Down Expand Up @@ -246,6 +247,9 @@
(def default-auto-ttl-renewal-seconds
(duration-str->sec default-auto-ttl-renewal)) ; 90 days by default

;; if the crl is going to expire in less than this number of days, it should be regenerated.
(def crl-expiration-window-days 30)

(schema/defn ^:always-validate initialize-ca-config
"Adds in default ca config keys/values, which may be overwritten if a value for
any of those keys already exists in the ca-data"
Expand Down Expand Up @@ -785,6 +789,15 @@
;; lack of an end date means we can't tell if it is expired or not, so assume it isn't.
false))

(schema/defn is-expired? :- schema/Bool
[now :- DateTime
[_serial _not-before not-after _row-subject] :- [schema/Str]]
(if-let [not-after-date (parse-date-time not-after)]
(time/after? 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 @@ -803,6 +816,25 @@
(log/debug "Unable to find inventory file {0}" cert-inventory)
false))))

(schema/defn expired-inventory-serials :- [BigInteger]
[{:keys [cert-inventory inventory-lock inventory-lock-timeout-seconds]} :- CaSettings]
(common/with-safe-read-lock inventory-lock inventory-lock-descriptor inventory-lock-timeout-seconds
(log/trace (i18n/trs "Extracting expired serials from inventory file {1}" cert-inventory))
(if (fs/exists? cert-inventory)
(with-open [inventory-reader (io/reader cert-inventory)]
(let [now (time/now)]
(doall
(->>
(line-seq inventory-reader)
(map extract-inventory-row-contents )
(filter (partial is-expired? now))
(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 "Unable to find inventory file {0}" cert-inventory)
[]))))

(schema/defn find-matching-valid-serial-numbers :- [BigInteger]
[{:keys [cert-inventory inventory-lock inventory-lock-timeout-seconds]} :- CaSettings
certname :- schema/Str]
Expand Down Expand Up @@ -2298,3 +2330,68 @@
(.getNotAfter signed-cert))))
(report-activity [cert-subject] "renewed")
signed-cert))

(schema/defn crl-expires-in-n-days?
[crl-path {:keys [crl-lock crl-lock-timeout-seconds]} :- CaSettings
days :- schema/Int]
(common/with-safe-write-lock crl-lock crl-lock-descriptor crl-lock-timeout-seconds
(let [crl-object (first (utils/pem->crls crl-path))
next-update (-> crl-object
.getNextUpdate
.toInstant
(.atZone (ZoneId/systemDefault))
.toLocalDateTime)]
(.isBefore next-update (.plusDays (LocalDateTime/now) days)))))

(schema/defn overwrite-existing-crl!
[crl :- X509CRL
rest-of-full-chain
capub :- schema/Str
cakey :- schema/Str
cacert :- X509Certificate
valid-serials :- [BigInteger]
crl-path :- schema/Str]
(let [^BigInteger crl-number (utils/get-crl-number crl)
public-key (utils/pem->public-key capub)
private-key (utils/pem->private-key cakey)
;; because we are purging existing serials, we need a whole new CRL
;; the "next update" and "crl-number" will get updated by the process that
;; adds the serial numbers to the crl
new-full-crl (utils/generate-crl
(.getIssuerX500Principal cacert)
private-key
public-key
(.getThisUpdate crl)
(.getNextUpdate crl)
crl-number
;; original crl is generated with no extensions, continue the precedence
nil)
;; create a new CRL with incremented "next update" and crl-number, and all the serials
new-crl-with-revoked (utils/revoke-multiple new-full-crl
(utils/pem->private-key cakey)
(.getPublicKey cacert)
valid-serials)
new-full-chain (cons new-crl-with-revoked (vec rest-of-full-chain))]
(write-crls new-full-chain crl-path)))
(schema/defn update-and-sign-crl!
"Given a path to a CRL, and the ca-settings, update the CRl with all known valid serials that have been revoked"
[path-to-crl {:keys [crl-lock crl-lock-timeout-seconds cacert cakey capub] :as settings} :- CaSettings]
;; read in the existing crl, and extract the serial numbers that have expired so we can remove them from the CRL set.
(let [expired-serials (set (expired-inventory-serials settings))]
(common/with-safe-write-lock crl-lock crl-lock-descriptor crl-lock-timeout-seconds
(let [[our-full-crl & rest-of-full-chain] (utils/pem->crls path-to-crl)
crl-serials (set (map #(.getSerialNumber %) (.getRevokedCertificates our-full-crl)))
valid-crl-serials (set/difference crl-serials expired-serials)
cacert (utils/pem->ca-cert cacert cakey)]
(overwrite-existing-crl! our-full-crl rest-of-full-chain capub cakey cacert (vec valid-crl-serials) path-to-crl)))))

(schema/defn maybe-update-crls-for-expiration
[{:keys [cacrl enable-infra-crl infra-crl-path] :as settings} :- CaSettings]
;; check the age of the main crl
(when (crl-expires-in-n-days? cacrl settings crl-expiration-window-days)
(log/info (i18n/trs "CA CRL expiring within 30 days, updating."))
(update-and-sign-crl! cacrl settings))
(when (and enable-infra-crl (fs/exists? infra-crl-path))
(when (crl-expires-in-n-days? infra-crl-path settings crl-expiration-window-days)
(log/info (i18n/trs "infra crl expiring within 30 days, updating."))
(update-and-sign-crl! infra-crl-path settings))))
29 changes: 27 additions & 2 deletions src/clj/puppetlabs/services/ca/certificate_authority_service.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,29 @@
[puppetlabs.trapperkeeper.services.status.status-core :as status-core]
[puppetlabs.rbac-client.protocols.activity :refer [ActivityReportingService] :as activity-proto]))

(def one-day-ms
(* 24 60 60 1000))

(def ca-scheduled-job-group-id
:ca-scheduled-job-group-id)

(defn evaluate-crls-for-expiration
[ca-settings]
(try
;; don't allow exceptions to escape
(ca/maybe-update-crls-for-expiration ca-settings)
(catch Exception e
(log/error e (i18n/trs "Failed to evaluate crls for expiration")))))

(tk/defservice certificate-authority-service
CaService
{:required
[[:PuppetServerConfigService get-config get-in-config]
[:WebroutingService add-ring-handler get-route]
[:AuthorizationService wrap-with-authorization-check]
[:FilesystemWatchService create-watcher]
[:StatusService register-status]]
[:StatusService register-status]
[:SchedulerService interspaced stop-jobs]]
:optional [ActivityReportingService]}
(init
[this context]
Expand Down Expand Up @@ -86,7 +101,17 @@
1
core/v1-status)
(assoc context :auth-handler auth-handler
:watcher watcher)))
:watcher watcher
:ca-settings settings)))
(start [this context]
(log/info (i18n/trs "Starting CA service"))
(interspaced one-day-ms #(evaluate-crls-for-expiration (:ca-settings context)) ca-scheduled-job-group-id)
context)

(stop [this context]
(log/info (i18n/trs "Stopping CA service"))
(stop-jobs ca-scheduled-job-group-id)
(dissoc context :ca-settings))

(initialize-master-ssl!
[this master-settings certname]
Expand Down
132 changes: 131 additions & 1 deletion test/unit/puppetlabs/puppetserver/certificate_authority_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
ByteArrayOutputStream)
(com.puppetlabs.ssl_utils SSLUtils)
(java.security PublicKey MessageDigest)
(java.time LocalDateTime ZoneOffset)
(java.util Date)
(java.util.concurrent TimeUnit)
(java.util.concurrent.locks ReentrantReadWriteLock)
(org.joda.time DateTime Period)
(org.bouncycastle.asn1.x509 SubjectPublicKeyInfo))
(:require [puppetlabs.puppetserver.certificate-authority :as ca]
(:require [clojure.set :as set]
[puppetlabs.puppetserver.certificate-authority :as ca]
[puppetlabs.trapperkeeper.testutils.logging :refer [logged?] :as logutils]
[puppetlabs.ssl-utils.core :as utils]
[puppetlabs.ssl-utils.simple :as simple]
Expand Down Expand Up @@ -2118,3 +2120,131 @@
subject (utils/cn "foo")
csr (utils/generate-certificate-request keypair subject [] [{:oid "1.3.6.1.4.1.34380.1.3.2" :value true}])]
(is (= [{:oid "1.3.6.1.4.1.34380.1.3.2", :values ["true"]}] (ca/get-csr-attributes csr))))))

(deftest crl-expires-in-n-days?-test
(let [settings (testutils/ca-sandbox! cadir)]
;; by default the crl expires in 5 years
(testing "CRL with long expiration don't expire soon"
(is (false? (ca/crl-expires-in-n-days? (:cacrl settings) settings 5))))
(testing "CRL with long expiration expires in ~6 years"
(is (ca/crl-expires-in-n-days? (:cacrl settings) settings (* 365 6))))
(testing "created crl expires in 5 days"
(let [[crl & rest-of-full-chain] (utils/pem->crls (:cacrl settings))
public-key (utils/pem->public-key (:capub settings))
private-key (utils/pem->private-key (:cakey settings))
new-crl (utils/generate-crl
(.getIssuerX500Principal (utils/pem->ca-cert (:cacert settings) (:cakey settings)))
private-key
public-key
(.getThisUpdate crl)
;; create a date 5 days from now using java.time
(-> (LocalDateTime/now)
(.plusDays 5)
(.toInstant (ZoneOffset/UTC))
(Date/from))
(biginteger 1)
nil)
new-full-chain (cons new-crl (vec rest-of-full-chain))
temp-path (fs/temp-file "crl" "pem")
path-as-string (.getCanonicalPath temp-path)]
(ca/write-crls new-full-chain (.getCanonicalPath temp-path))
(doseq [i (range 0 4)]
(is (false? (ca/crl-expires-in-n-days? path-as-string settings i))))
(is (ca/crl-expires-in-n-days? path-as-string settings 5))
(is (ca/crl-expires-in-n-days? path-as-string settings 6))))))

(deftest overwrite-existing-crl!-test
(let [settings (testutils/ca-sandbox! cadir)
[crl & rest-of-full-chain] (utils/pem->crls (:cacrl settings))
crl-serials (set (map #(.getSerialNumber %) (.getRevokedCertificates crl)))
all-serials (set/union crl-serials (set (map biginteger (range 1000 2000))))
ca-cert (utils/pem->ca-cert (:cacert settings) (:cakey settings))
before-next-update (.getNextUpdate crl)
crl-number (utils/get-crl-number crl)]
(ca/overwrite-existing-crl! crl rest-of-full-chain (:capub settings) (:cakey settings) ca-cert (vec all-serials) (:cacrl settings))
(testing "overwritten crl has updated properties"
(let [[updated-crl & _rest-of-full-chain] (utils/pem->crls (:cacrl settings))]
(is (.after (.getNextUpdate updated-crl) before-next-update))
(is (< crl-number (utils/get-crl-number updated-crl)))
(is (not= (set (map #(.getSerialNumber %) (.getRevokedCertificates updated-crl)))
crl-serials))
(is (= (set (map #(.getSerialNumber %) (.getRevokedCertificates updated-crl)))
all-serials))))))
(deftest expired-inventory-serials-test
(let [settings (testutils/ca-sandbox! cadir)
keypair (utils/generate-key-pair)
subject (utils/cn "foo")
csr (utils/generate-certificate-request keypair subject)
serial-number (ca/next-serial-number! settings)
ca-cert (utils/pem->ca-cert (:cacert settings) (:cakey settings))
private-key (utils/pem->private-key (:cakey settings))

signed-cert (utils/sign-certificate
(utils/get-subject-from-x509-certificate ca-cert)
private-key
serial-number
;; valid from 100 days ago until yesterday
(-> (LocalDateTime/now)
(.minusDays 100)
(.toInstant ZoneOffset/UTC)
(Date/from))
(-> (LocalDateTime/now)
(.minusDays 1)
(.toInstant ZoneOffset/UTC)
(Date/from))
subject
(utils/get-public-key csr)
(ca/create-agent-extensions csr ca-cert))]
(ca/write-cert-to-inventory! signed-cert settings)
;; there are multiple entries in the inventory file that aren't expired by default
(is (= [(biginteger serial-number)] (ca/expired-inventory-serials settings)))))

(deftest update-and-sign-crl!-test
(let [settings (testutils/ca-sandbox! cadir)
[crl & rest-of-full-chain] (utils/pem->crls (:cacrl settings))
crl-serials (set (map #(.getSerialNumber %) (.getRevokedCertificates crl)))
crl-number (utils/get-crl-number crl)
serial-number (ca/next-serial-number! settings)
keypair (utils/generate-key-pair)
subject (utils/cn "foo")
csr (utils/generate-certificate-request keypair subject)
ca-cert (utils/pem->ca-cert (:cacert settings) (:cakey settings))
private-key (utils/pem->private-key (:cakey settings))
;; create an add an expired cert to add to the inventory after we update the crl with
;; the serial number of the cert
signed-cert (utils/sign-certificate
(utils/get-subject-from-x509-certificate ca-cert)
private-key
serial-number
;; valid from 100 days ago until yesterday
(-> (LocalDateTime/now)
(.minusDays 100)
(.toInstant ZoneOffset/UTC)
(Date/from))
(-> (LocalDateTime/now)
(.minusDays 1)
(.toInstant ZoneOffset/UTC)
(Date/from))
subject
(utils/get-public-key csr)
(ca/create-agent-extensions csr ca-cert))
extra-serials (set (map biginteger (range 1000 2000)))
all-serials (set/union crl-serials #{(biginteger serial-number)} extra-serials)]
(ca/overwrite-existing-crl! crl rest-of-full-chain (:capub settings) (:cakey settings) ca-cert (vec all-serials) (:cacrl settings))
(let [[updated-crl & _rest-of-full-chain] (utils/pem->crls (:cacrl settings))
updated-crl-number (utils/get-crl-number updated-crl)]
(is (< crl-number updated-crl-number))
(ca/write-cert-to-inventory! signed-cert settings)
(ca/update-and-sign-crl! (:cacrl settings) settings)
(let [[final-updated-crl & _rest-of-full-chain] (utils/pem->crls (:cacrl settings))
final-crl-number (utils/get-crl-number final-updated-crl)
final-crl-serials (set (map #(.getSerialNumber %) (.getRevokedCertificates final-updated-crl)))]
(is (< updated-crl-number final-crl-number))
;; should not contain the serial that was expired
(is (= (set/union crl-serials extra-serials) final-crl-serials))))))






0 comments on commit 28b2114

Please sign in to comment.