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.

Sign up for GitHub

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

Jump to bottom

Bundle the CA public key in issued certificate #317

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Bundle the CA public key in issued certificate #317

merged 1 commit into from
Feb 21, 2018

Conversation

radhus
Copy link
Contributor

@radhus radhus commented Feb 18, 2018 •
edited by munnerz

What this PR does / why we need it:

If the CA used is only an intermediate CA, and the root CA is trusted by the client, the client needs help verifying the certificate chain.

This also makes the CA present in the certificate even if it's the root CA.

Which issue this PR fixes:

Trusting certs issued by intermediate CAs used by cert-manager.

Special notes for your reviewer:

I have tested this locally with my own intermediate CA used by cert-manager, issued by my own root CA trusted by my macOS client. The whole certificate chain is now presented in the browser.

The idea to just append the certificates is based on cfssl's mkbundle:
https://github.com/cloudflare/cfssl/blob/1.3.0/cmd/mkbundle/mkbundle.go#L97

Release note:

CA Issuer: bundle CA certificate with issued certificates

@radhus
Bundle the CA public key in issued certificate
6ff1746 
If the CA used is only an intermediate CA, and the root CA is trusted by
the client, the client needs help verifying the certificate chain.
@jetstack-bot jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 18, 2018
@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 18, 2018
@munnerz
Copy link
Member

munnerz commented Feb 18, 2018

/test all

@radhus
Copy link
Contributor Author

radhus commented Feb 19, 2018

@munnerz I haven't had time yet to run with ACME and Let's Encrypt, but shouldn't that present the same issue as I have here? So I'm thinking if I have done this too quickly...
Is the certificate received from the ACME client the chain including Let's encrypts' intermediate CAs?

@munnerz
Copy link
Member

munnerz commented Feb 21, 2018

@radhus yep you are correct, we don't do this for ACME right now. As per this comment however, ACME v2 will begin returning the intermediate certificate automatically: #292 (comment)

As a result, I think I'm happy to accept this change now. v0.3 should include ACMEv2 support, and so the 0.3 release should at least be consistent (and just the :canary won't be for now).

Thanks very much for this PR! 😄

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 21, 2018
@munnerz
Copy link
Member

munnerz commented Feb 21, 2018

/ok-to-test
/lgtm
/approve

@jetstack-bot jetstack-bot added lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 21, 2018
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2018
@jetstack-ci-bot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@jetstack-ci-bot
Copy link
Contributor

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@jetstack-ci-bot jetstack-ci-bot merged commit 9e0e333 into cert-manager:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@munnerz munnerz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@radhus @munnerz @jetstack-bot @jetstack-ci-bot