-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Remove duplicated issuers in full chain #15005
Conversation
* Simple starting PKI storage api for CA rotation * Add key and issuer storage apis * Add listKeys and listIssuers storage implementations * Add simple keys and issuers configuration storage api methods
The API context will usually have a user-specified reference to the key. This is either the literal string "default" to select the default key, an identifier of the key, or a slug name for the key. Here, we wish to resolve this reference to an actual identifier that can be understood by storage. Also adds the missing Name field to keys. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This adds a method to construct a certutil.CertBundle from the specified issuer identifier, optionally loading its corresponding key for signing. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This refactors the parsing of PrivateKeys from PEM blobs into shared methods (ParsePEMKey, ParseDERKey) that can be reused by the existing Bundle parsing logic (ParsePEMBundle) or independently in the new issuers/key-based PKI storage code. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
importKey is generally preferable to the low-level writeKey for adding new entries. This takes only the contents of the private key (as a string -- so a PEM bundle or a managed key handle) and checks if it already exists in the storage. If it does, it returns the existing key instance. Otherwise, we create a new one. In the process, we detect any issuers using this key and link them back to the new key entry. The same holds for importCert over importKey, with the note that keys are not modified when importing certificates. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This adds tests for importing keys and issuers into the new storage layout, ensuring that identifiers are correctly inferred and linked. Note that directly writing entries to storage (writeKey/writeissuer) will take KeyID links from the parent entry and should not be used for import; only existing entries should be updated with this info. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
- Hook into the backend::initialize function, calling the migration on a primary only. - Migrate an existing certificate bundle to the new issuers and key layout
This allows fetchCAInfo to fetch a specified issuer, via a reference parameter provided by the user. We pass that into the storage layer and have it return a cert bundle for us. Finally, we need to validate that it truly has the key desired. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This implements the fetch operations around issuers in the PKI Secrets Engine. We implement the following operations: - LIST /issuers - returns a list of known issuers' IDs and names. - GET /issuer/:ref - returns a JSON blob with information about this issuer. - POST /issuer/:ref - allows configuring information about issuers, presently just its name. - DELETE /issuer/:ref - allows deleting the specified issuer. - GET /issuer/:ref/{der,pem} - returns a raw API response with just the DER (or PEM) of the issuer's certificate. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This adds the two core import code paths to the API: /issuers/import/cert and /issuers/import/bundle. The former differs from the latter in that the latter allows the import of keys. This allows operators to restrict importing of keys to privileged roles, while allowing more operators permission to import additional certificates (not used for signing, but instead for path/chain building). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This endpoint allows existing issuers to be used to sign intermediate CA certificates. In the process, we've updated the existing /root/sign-intermediate endpoint to be equivalent to a call to /issuer/default/sign-intermediate. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This endpoint allows existing issuers to be used to sign self-signed certificates. In the process, we've updated the existing /root/sign-self-issued endpoint to be equivalent to a call to /issuer/default/sign-self-issued. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This endpoint allows existing issuers to be used to directly sign CSRs. In the process, we've updated the existing /sign-verbatim endpoint to be equivalent to a call to /issuer/:ref/sign-verbatim. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Using the new updateDefaultIssuerId(...) from the storage migration PR allows for easy implementation of configuring the default issuer. We restrict callers from setting blank defaults and setting default to default. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
After setting a default issuer, one should be able to use the old /ca, /ca_chain, and /cert/{ca,ca_chain} endpoints to fetch the default issuer (and its chain). Update the fetchCertBySerial helper to no longer support fetching the ca and prefer fetchCAInfo for that instead (as we've already updated that to support fetching the new issuer location). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This updates the /sign and /issue endpoints, allowing them to take the default issuer (if none is provided by a role) and adding issuer-specific versions of them. Note that at this point in time, the behavior isn't yet ideal (as /sign/:role allows adding the ref=... parameter to override the default issuer); a later change adding role-based issuer specification will fix this incorrect behavior. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
- Update all new API endpoints to use the new agreed upon argument names. - issuer_ref & key_ref to refer to existing - issuer_name & key_name for new definitions - Update returned values to always user issuer_id and key_id
- Add utility methods to fetch the issuer_name, issuer_ref, key_name and key_ref arguments from data fields. - Centralize the logic to clean up these inputs and apply various validations to all of them.
- Use the buildPath convention for the function name instead of common...
…suer} methods - PR feedback, move setting up the default configuration references within the import methods instead of within the writeCaBundle method. This should now cover all use cases of us setting up the defaults properly.
- Addresses some test failures due to an incorrect refactoring of a legacy api path /sign-verbatim within PKI
The existing bundle import code will satisfy the intermediate import; use it instead of the old ca_bundle import logic. Additionally, update /config/ca to use the new import code as well. While testing, a panic was discovered: > reflect.Value.SetMapIndex: value of type string is not assignable to type pki.keyId This was caused by returning a map with type issuerId->keyId; instead switch to returning string->string maps so the audit log can properly HMAC them. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
When the default issuer and key are missing (and haven't yet been specified), we should clarify that error message. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This makes two minor changes to the existing test suite: 1. Importing partial bundles should now succeed, where they'd previously error. 2. fetchCertBySerial no longer handles CA certificates. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
The old DELETE /root code must now delete all keys and issuers for backwards compatibility. We strongly suggest calling individual delete methods (DELETE /key/:key_ref or DELETE /issuer/:issuer_ref) instead, for finer control. In the process, we detect whether the deleted key/issuers was set as the default. This will allow us to warn (from the single key/deletion issuer code) whether or not the default was deleted (while allowing the operation to succeed). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Don't set the legacy IssuingCA field on the certificate bundle, as we prefer the CAChain field over it. Additionally, building the full chain could result in duplicate certificates when the CAChain included the leaf certificate itself. When building the full chain, ensure we don't include the bundle's certificate twice. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
We wish to ensure that each desired certificate in the chain is only present once. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
c89b60f
to
7e2179e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug fix and tests look good, have question about effect on SDK
builtin/logical/pki/storage.go
Outdated
@@ -575,7 +575,6 @@ func fetchCertBundleByIssuerId(ctx context.Context, s logical.Storage, id issuer | |||
|
|||
var bundle certutil.CertBundle | |||
bundle.Certificate = issuer.Certificate | |||
bundle.IssuingCA = issuer.CAChain[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, this is used in sdk/helper/certutil/types.go, I think? Clarification that pemBlock, _ = pem.Decode([]byte(c.IssuingCA))
will still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note its gated on size tho:
https://github.com/hashicorp/vault/blob/main/sdk/helper/certutil/types.go#L210-L212
So it won't really matter, we'll skip it.
0a94ae1
to
fa6ad88
Compare
Manually merged, per out of band conversation with @kitography and @stevendpclark. Thanks! |
This fixes full chain construction so that we don't accidentally introduce duplicate issuers into the chain. This also removes our incorrect setting of the legacy
IssuingCA
field, which was used prior toCAChain
introduction.Strictly, I believe this is caused by
CAChain
containing not just the strict issuer hierarchy but also the leaf cert. I believe this makes the most sense from a bundle perspective (at the cost of a duplication), but perhaps it is worth trimming it out.Additionally, we haven't yet switched the old logic (for
certificate
field on theca_chain
endpoint) to the new full chain. I'd like to make that change, given the goals of rotation, and have it be for the default issuer only.Thoughts?