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

Concern about change to ca_chain behaviour in 1.11.0-rc1 - confusing changelog entry for PR that was not merged, unclear if changes were introduced elsewhere #16057

Closed
maxb opened this issue Jun 18, 2022 · 5 comments

Comments

@maxb
Copy link
Contributor

maxb commented Jun 18, 2022

This caught my eye in the 1.11.0-rc1 changelog:

secrets/pki: The ca_chain response field within issuing (/pki/issue/:role) and signing APIs will now include the root CA certificate if the mount is aware of it. [/~https://github.com//pull/15155]

It is frustrating that the linked PR contains no explanation for the reason for the change. Also frustrating that actual linked PR was never merged - I guess the changelog has the wrong number?

There is a solid technical reason for not including the root CA certificate in ca_chain fields. The chain purpose is to build the chain of trust through intermediate CAs. Including root CAs in the chain (i.e. CAs where issuer == subject) has no role in chain building, and just wastes bytes on the network during TLS handshakes.

@maxb
Copy link
Contributor Author

maxb commented Jun 18, 2022

It appears that this changelog entry arrived via #15500, even though #15155 was never actually merged.

It's difficult to understand the meaning of this changelog entry, since it was originally written for a PR which wasn't merged.

As a result, I'm not sure if there's an issue here or not. (Other than the changelog being confusing - that's definitely an issue.)

@maxb maxb changed the title Concern about change to ca_chain behaviour in 1.11.0-rc1 - may need reverting Concern about change to ca_chain behaviour in 1.11.0-rc1 - confusing changelog entry for PR that was not merged, unclear if changes were introduced elsewhree Jun 19, 2022
@maxb maxb changed the title Concern about change to ca_chain behaviour in 1.11.0-rc1 - confusing changelog entry for PR that was not merged, unclear if changes were introduced elsewhree Concern about change to ca_chain behaviour in 1.11.0-rc1 - confusing changelog entry for PR that was not merged, unclear if changes were introduced elsewhere Jun 19, 2022
@cipherboy
Copy link
Contributor

cipherboy commented Jun 20, 2022

\o Hey there @maxb

The linked PR was manually merged into the pki-pod-rotation branch (not main). At the time, we weren't sure if this feature was going to make it for 1.11.0 and thus we worked on a separate branch. When #15277 was opened, it contained the relevant commits from the rotation label and you should find the commits from #15155 there.


There are actually several reasons for this change.

Firstly, RFC 8466 notes that the root MAY be omitted. MAY is relatively weak in RFC language. For maximum compatibility, the root should not be omitted. Notably, it is much easier to remove the root (on the server) once presented (by Vault), than it is to guess at the location of a root (in Vault) if it isn't present. I personally wouldn't give end-users (consumers of the intermediate/signing endpoints) access to the root mount anyways, though the raw /ca path is unauthenticated if they can guess the mount point. This is why #13489 and #13935 were opened and merged back in v1.10 -- customers do generally want this behavior. Since this root is an internal root, and isn't a widely-distributed (core OS/browser trust store) root, it isn't unlikely that the client may not have it, so sending it is generally recommended as a best practice.

Secondly, note that if the root is reissued (i.e., same key material and Subject but different expiration), the new root may not be present in browsers' trust stores, but will be validated through the legacy root (if they don't check root expiration -- see Let's Encrypt's legacy Android support threads for a great example of that). If servers are properly configured to serve the entire chain, this type of temporal rotation (without re-keying) is possible without impact to clients that have been offline since the new root was generated (and thus haven't updated their trust stores yet). This type of reissuance is now possible in Vault 1.11 and thus it makes sense to support building those chains.

Finally, note that the manual_chain allows you to restrict what gets served on both signing and /ca_chain requests. So if you don't like this behavior, you can opt all consumers of the Vault API. :-)

@maxb
Copy link
Contributor Author

maxb commented Jun 21, 2022

Interesting, I learnt something new, I had thought this would be a hard failure:

Secondly, note that if the root is reissued (i.e., same key material and Subject but different expiration), the new root may not be present in browsers' trust stores, but will be validated through the legacy root (if they don't check root expiration -- see Let's Encrypt's legacy Android support threads for a great example of that).

Thanks.

@maxb
Copy link
Contributor Author

maxb commented Jun 23, 2022

Hi @cipherboy I'd like to re-open this for further discussion please.

it isn't unlikely that the client may not have it, so sending it is generally recommended as a best practice.

I don't follow... why do you say it's a best practice? Either they already have the root CA in their trust store, and don't need another copy of it - or they do not, in which case sending a self-signed certificate in the TLS handshake is not going to induce them to start trusting it.

Secondly, note that if the root is reissued (i.e., same key material and Subject but different expiration), the new root may not be present in browsers' trust stores, but will be validated through the legacy root (if they don't check root expiration -- see Let's Encrypt's legacy Android support threads for a great example of that).

Now that I have had time to review the linked document, I realise it does not match what you are asserting here. Let's Encrypt's description of the situation, is that rather than referring to a "temporally renewed" root in the certificate chain, Android simply does not care at all about expiration dates of roots.

The specific part of your linked document which I'm referencing in support of this is:

But isn’t DST Root CA X3 expiring? The self-signed certificate which represents the DST Root CA X3 keypair is expiring. But browser and OS root stores don’t contain certificates per se, they contain “trust anchors”, and the standards for verifying certificates allow implementations to choose whether or not to use fields on trust anchors. Android has intentionally chosen not to use the notAfter field of trust anchors.

@stevendpclark
Copy link
Contributor

With the new argument introduced in #16935 for Vault 1.12, end-users should be able to get back the old behavior for mounts that were initialized with the root/generate/internal api. On top of that they can now have it applied to intermediary mounts that have imported the associated root CA if they so wish, something that was not technically possible in the past.

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

No branches or pull requests

3 participants