Add automatic tidy of expired issuers #17823

cipherboy · 2022-11-04T16:03:26Z

To aid PKI users like Consul, which periodically rotate intermediates, and provided a little more consistency with older versions of Vault which would silently (and dangerously!) replace the configured CA on root/intermediate generation, we introduce an automatic tidy of expired issuers.

This includes a longer safety buffer (1 year) and logging of the relevant issuer information prior to deletion (certificate contents, key ID, and issuer ID/name) to allow admins to recover this value if desired, or perform further cleanup of keys.

From my PoV, removal of the issuer is thus a relatively safe operation compared to keys (which I do not feel comfortable removing) as they can always be re-imported if desired. Additionally, this is an opt-in tidy operation, not enabled by default. Lastly, most major performance penalties comes with lots of issuers within the mount, not as much large numbers of keys (as only new issuer creation/import operations are affected, unlike LIST /issuers which is a public, unauthenticated endpoint).

Signed-off-by: Alexander Scheel <[email protected]>

This obviously needs:

Tests
Changelog
Docs

But I'm curious to get people's initial opinions on this change.

kitography

Getting somewhere!

kitography · 2022-11-07T20:49:07Z

builtin/logical/pki/fields.go

+	fields["tidy_expired_issuers"] = &framework.FieldSchema{
+		Type: framework.TypeBool,
+		Description: `Set to true to automatically remove expired issuers
+past the issuer_safety_buffer. No keys will be removed as part of this


I assume that we need to add something to remove unused keys eventually?

Nope, I'm punting on that particular problem. Issuers make the mount slow (chain building, CRL building, ...), keys are always fetched by reference and don't make the mount slow. Obviously they'll be some nominal storage costs, but operators should be able to sort this out themselves eventually, if they care about the storage costs and want to remove old keys.

But even then, the number of stored leaves should exceed the stored issuer keys by several orders of magnitude.

Will the logging provide enough information for an operator to delete the key after an auto tidy if they want to?

Yes, see the log message below:

https://github.com/hashicorp/vault/pull/17823/files#diff-4d3f13dea8cac8ec87e6f15d17a6bbbe798129d171059e492303070ce3903d9cR518

It contains the key ID from the issuer, allowing them to make a call from the log messages as to whether to remove that key or not (and the system will prevent that if its in use by other still-present issuers).

In the general case of tidying keys, we don't necessarily presently store when/how a key was created, so we don't know if there's an outstanding, say, ICA issuance going on that we don't know about, or if it was from a deleted issuer.

We could later remove now-unused keys from issuers if we wanted to, but it makes recovery much harder -- you can't presently re-import deleted keys if you didn't have an external backup, and we wouldn't want to log them (unlike issuers) for obvious reasons...

No, I don't want to automatically remove keys, but I don't want operators to get in a case where they'd like to remove an old key but are confused which issuer it once applied to.

Would you recommend something else? Another migration to associate issuers on keys, and/or a tidy step to associate a since-deleted issuer's identifier(s) back to the key?

Two way association is only beneficial in this case; in general nothing in the PKI code (presently) goes from key->issuer, except the key deletion code.

The problem is a number of issuers won't necessarily have meaningful names, so logging the full certificate is perhaps the best approach here -- unless we do want to retain the issuer's certificate in the system and associate removed issuers' certificates with the key entry. Then they could read the key and see the list of since-deleted issuers which used that key... It wouldn't impact perf anymore but still wouldn't be ideal to interact with (as they'd likely have to parse the certificate to understand which one it was).

No, I'd just be happy if the key_id was in the output of the issuer tidy, so they could grep it out and delete it by hand.

builtin/logical/pki/fields.go

builtin/logical/pki/path_tidy.go

builtin/logical/pki/path_tidy_test.go

To aid PKI users like Consul, which periodically rotate intermediates, and provided a little more consistency with older versions of Vault which would silently (and dangerously!) replace the configured CA on root/intermediate generation, we introduce an automatic tidy of expired issuers. This includes a longer safety buffer (1 year) and logging of the relevant issuer information prior to deletion (certificate contents, key ID, and issuer ID/name) to allow admins to recover this value if desired, or perform further cleanup of keys. From my PoV, removal of the issuer is thus a relatively safe operation compared to keys (which I do not feel comfortable removing) as they can always be re-imported if desired. Additionally, this is an opt-in tidy operation, not enabled by default. Lastly, most major performance penalties comes with lots of issuers within the mount, not as much large numbers of keys (as only new issuer creation/import operations are affected, unlike LIST /issuers which is a public, unauthenticated endpoint). Signed-off-by: Alexander Scheel <[email protected]>

Signed-off-by: Alexander Scheel <[email protected]>

sgmiller

Some small feedback

sgmiller · 2022-11-09T15:27:42Z

builtin/logical/pki/fields.go

+	fields["tidy_expired_issuers"] = &framework.FieldSchema{
+		Type: framework.TypeBool,
+		Description: `Set to true to automatically remove expired issuers
+past the issuer_safety_buffer. No keys will be removed as part of this


Will the logging provide enough information for an operator to delete the key after an auto tidy if they want to?

sgmiller · 2022-11-09T15:28:45Z

builtin/logical/pki/fields.go

@@ -463,6 +470,15 @@ Defaults to 72 hours.`,
 		Default: int(defaultTidyConfig.SafetyBuffer / time.Second), // TypeDurationSecond currently requires defaults to be int
 	}

+	fields["issuer_safety_buffer"] = &framework.FieldSchema{


How about issuer_tidy_grace_period? This doesn't as directly indicate safety from what, and that buffer is time related.

This mirrors naming of the present field, safety_buffer above -- do you recommend breaking that pattern?

Ah, no, didn't notice the existing field.

I'd like something named _duration ideally, but I think it'd require a soft-migration for the other field (and use say expired_issuer_retention_duration and expired_leaf_retention_duration+safety_buffer) -- this'd let the CLI parse it correctly as a duration-typed field.

We have time to revisit doing that before 1.13 GAs as this won't be backported.

We could just introduce the two new fields and switch the docs. Add a note about the previous field still existing and being deprecated. But that can wait for another PR.

builtin/logical/pki/path_tidy.go

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy added enhancement secret/pki labels Nov 4, 2022

cipherboy added this to the 1.13.0-rc1 milestone Nov 4, 2022

cipherboy requested review from kitography, sgmiller and a team November 4, 2022 16:03

cipherboy mentioned this pull request Nov 4, 2022

Optional automatic default issuer selection #17824

Merged

3 tasks

cipherboy force-pushed the cipherboy-remove-issuers-expired-tidy branch 3 times, most recently from 42725f6 to a56bc78 Compare November 7, 2022 15:23

vercel bot deployed to Preview November 7, 2022 15:35 View deployment

cipherboy force-pushed the cipherboy-remove-issuers-expired-tidy branch from a56bc78 to e329b46 Compare November 7, 2022 17:42

vercel bot deployed to Preview November 7, 2022 17:47 View deployment

kitography reviewed Nov 7, 2022

View reviewed changes

cipherboy force-pushed the cipherboy-remove-issuers-expired-tidy branch from e329b46 to 8da9ad4 Compare November 8, 2022 13:15

cipherboy requested a review from kitography November 8, 2022 13:20

vercel bot deployed to Preview November 8, 2022 13:21 View deployment

cipherboy added 4 commits November 8, 2022 11:47

Add changelog entry

a9fc45a

Signed-off-by: Alexander Scheel <[email protected]>

Add test for tidy

09f55be

Signed-off-by: Alexander Scheel <[email protected]>

Add docs on tidy of issuers

41943d9

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy force-pushed the cipherboy-remove-issuers-expired-tidy branch from 8da9ad4 to 41943d9 Compare November 8, 2022 16:47

cipherboy requested a review from a team November 8, 2022 16:48

vercel bot deployed to Preview November 8, 2022 16:52 View deployment

This was referenced Nov 8, 2022

Backport of Optional automatic default issuer selection into release/1.11.x #17853

Merged

Backport of Optional automatic default issuer selection into release/1.12.x #17854

Merged

sgmiller requested changes Nov 9, 2022

View reviewed changes

Restructure logging

484a6d3

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy requested a review from sgmiller November 10, 2022 12:47

sgmiller approved these changes Nov 10, 2022

View reviewed changes

Add missing fields to expected tidy output

2b1e7f0

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy enabled auto-merge (squash) November 10, 2022 15:33

cipherboy disabled auto-merge November 10, 2022 15:36

cipherboy enabled auto-merge (squash) November 10, 2022 15:37

cipherboy merged commit cddc578 into main Nov 10, 2022

cipherboy deleted the cipherboy-remove-issuers-expired-tidy branch December 1, 2022 14:56

jkirschner-hashicorp mentioned this pull request Dec 5, 2022

Vault 1.11 Multi-Issuer CA breaks Connect CA Intermediate CAs (<-> Vault Provider) hashicorp/consul#15217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automatic tidy of expired issuers #17823

Add automatic tidy of expired issuers #17823

cipherboy commented Nov 4, 2022 •

edited

Loading

kitography left a comment

kitography Nov 7, 2022

cipherboy Nov 7, 2022 •

edited

Loading

sgmiller Nov 9, 2022

cipherboy Nov 9, 2022 •

edited

Loading

sgmiller Nov 9, 2022

cipherboy Nov 9, 2022 •

edited

Loading

sgmiller Nov 10, 2022

sgmiller left a comment

sgmiller Nov 9, 2022

sgmiller Nov 9, 2022

cipherboy Nov 9, 2022

sgmiller Nov 9, 2022

cipherboy Nov 9, 2022

sgmiller Nov 10, 2022

Add automatic tidy of expired issuers #17823

Add automatic tidy of expired issuers #17823

Conversation

cipherboy commented Nov 4, 2022 • edited Loading

kitography left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented Nov 4, 2022 •

edited

Loading

cipherboy Nov 7, 2022 •

edited

Loading

cipherboy Nov 9, 2022 •

edited

Loading

cipherboy Nov 9, 2022 •

edited

Loading