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 full CA Chain to /pki/cert/ca_chain response #13935

Merged
merged 5 commits into from
Feb 7, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Feb 7, 2022

As mentioned in #13489, calls to read /pki/cert/ca_chain would return a partial chain, excluding the root authority. By adding a new field, ca_chain to the response, we can include the entire chain without breaking backwards compatibility.

We test three main scenarios:

  1. A root-only CA's /cert/ca_chain's .data.ca_chain field should contain only the root,
  2. An intermediate CA (with root provide) should contain both the root and the intermediate.
  3. An external (e.g., /config/ca-provided) CA with both root and intermediate should contain both certs.

n.b.: I think the existing note around ca_chain in the documentation is wrong, hence its removal.

Additional note: this is only the full chain as known to Vault. If the entire (from a TLS client's PoV) chain wasn't provided during either /intermediate/set-signed or /config/ca, Vault wouldn't know the entire chain and thus couldn't provided. That'd be an enhancement for the future.

This allows callers to get the full chain (including issuing
certificates) from a call to /cert/ca_chain. Previously, most endpoints
(including during issuance) do not include the root authority, requiring
an explicit call to /cert/ca to fetch. This allows full chains to be
constructed without without needing multiple calls to the API.

Resolves: #13489

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 17:39 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 7, 2022 17:39 Inactive
We test three main scenarios:

 1. A root-only CA's `/cert/ca_chain`'s `.data.ca_chain` field should
    contain only the root,
 2. An intermediate CA (with root provide) should contain both the root
    and the intermediate.
 3. An external (e.g., `/config/ca`-provided) CA with both root and
    intermediate should contain both certs.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – vault February 7, 2022 17:41 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 17:41 Inactive
@cipherboy cipherboy requested a review from a team February 7, 2022 17:41
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@kitography kitography self-requested a review February 7, 2022 18:17
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

Actually approving this time. Looks good to me :)

Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

Code looks great 👍

I would just like to see an update to the "Read CA Certificate Chain" api docs to mention if they want the full CA Certificate chain it is available within the "Read Certificate" api with the ca_chain parameter...

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy
Copy link
Contributor Author

Thanks @stevendpclark, I've added a note -- let me know if that seems right?

@cipherboy
Copy link
Contributor Author

Thanks all for the reviews! Going ahead and merging. :-)

@cipherboy cipherboy merged commit 46c5238 into main Feb 7, 2022
@cipherboy cipherboy deleted the cipherboy-add-ca-chain branch February 9, 2022 21:41
fairclothjm pushed a commit that referenced this pull request Feb 12, 2022
* Include full chain in /cert/ca_chain response

This allows callers to get the full chain (including issuing
certificates) from a call to /cert/ca_chain. Previously, most endpoints
(including during issuance) do not include the root authority, requiring
an explicit call to /cert/ca to fetch. This allows full chains to be
constructed without without needing multiple calls to the API.

Resolves: #13489

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add test case for full CA issuance

We test three main scenarios:

 1. A root-only CA's `/cert/ca_chain`'s `.data.ca_chain` field should
    contain only the root,
 2. An intermediate CA (with root provide) should contain both the root
    and the intermediate.
 3. An external (e.g., `/config/ca`-provided) CA with both root and
    intermediate should contain both certs.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add documentation for new ca_chain field

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add note about where to find the entire chain

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy added this to the 1.10 milestone Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants