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

Add remove_roots_from_chain to sign and issue pki apis #16935

Merged
merged 3 commits into from
Aug 31, 2022

Conversation

stevendpclark
Copy link
Contributor

@stevendpclark stevendpclark commented Aug 30, 2022

This adds a new flag to allow end-users to control if we return the self-signed/root CA certificates within the ca_chain field on issue and sign api calls. This was done to resolve concerns brought up by @maxb within issue #16057. The default behavior for the flag does preserve the behavior returning the root CA within the ca_chain field. The new flag is available on the following apis.

  • issue/<role>
  • issuer/<issuer ref>/issue/<role>
  • sign/<role>
  • issuer/<issuer_ref>/sign/<role>
  • sign-verbatim

Note that the change mentioned within #16057 only affected mounts that had been initialized with the root/generate/internal api. Had an intermediary mount had an intermediate/set-signed call with the root CA within the PEM bundle, it would have contained the root CA certificate within ca_chain field (tested on 1.9.8 and 1.10.5). We should have made it clearer in the original changelog that we were making the behavior consistent no matter how the mount was populated.

 - Add a new flag to allow end-users to control if we return the
   root/self-signed CA certificate within the list of certificates in
   ca_chain field on issue and sign api calls.
for _, certBlock := range parsedBundle.CAChain {
cert := certBlock.Certificate
if (len(cert.AuthorityKeyId) > 0 && !bytes.Equal(cert.AuthorityKeyId, cert.SubjectKeyId)) ||
(len(cert.AuthorityKeyId) == 0 && !bytes.Equal(cert.RawIssuer, cert.RawSubject)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

While this code was cribbed from certutil and I'm thus fine leaving it, it'd be good to also verify cert.CheckSignatureFrom(cert) == nil, i.e., that the cert is truly both self-referencing and has a valid self-signature.

With fancy chains (e.g., a rotation of the same CA cert under new key material), generated outside of vault (no AKID), you can run into the case where the issuers will line up, and you probably want to include the cross-bridge but not either self-signed roots. This will elide the cross-bridge.

@maxb
Copy link
Contributor

maxb commented Aug 30, 2022

Thanks for @-ing me - is the behaviour still compatible with previous versions of Vault if the new flag is not specified?

i.e. chain as configured by administrator via set-signed endpoint is served?

I can try to read the code later but wanted to react early as this is already approved.

@cipherboy
Copy link
Contributor

cipherboy commented Aug 30, 2022

@maxb Yes, it is opt-in behavior per request so it is compatible from 1.11->1.12.

Btw, did you see the note at the tail end of the description? The behavior change from 1.10->1.11 was only for root mounts to be consistent with intermediates. On 1.9->1.12, if you don't post the root to the intermediate mount, you won't see that behavior. But if you had posted a root with the intermediate on 1.9->1.11, you'd see the behavior you didn't like in the issue (wherein roots would be served in intermediate sign requests).

@stevendpclark
Copy link
Contributor Author

stevendpclark commented Aug 30, 2022

Thanks for @-ing me - is the behaviour still compatible with previous versions of Vault if the new flag is not specified?

i.e. chain as configured by administrator via set-signed endpoint is served?

The behaviour is the same as 1.11 Vault without the new flag set for both root and intermediary mounts. So assuming a root CA was set within the intermediate/set-signed api call, the behaviour is unchanged from all previous versions of Vault.

This flag will allow someone to return to the behaviour for root mounts that we had in 1.10 and lower and/or apply that behaviour to intermediary mounts if a root CA had been imported along with the signed intermediary

@stormshield-gt
Copy link

I wish the remove_roots_from_chain option could also be added to endpoint like
GET /pki/cert/:serial so we could get the old behavior here also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants