Skip to content

Add intermediate cert to P12 chain if ca.crt is empty#3146

Closed
meyskens wants to merge 1 commit into
cert-manager:masterfrom
meyskens:p12-intermed-cert
Closed

Add intermediate cert to P12 chain if ca.crt is empty#3146
meyskens wants to merge 1 commit into
cert-manager:masterfrom
meyskens:p12-intermed-cert

Conversation

@meyskens
Copy link
Copy Markdown
Contributor

@meyskens meyskens commented Aug 4, 2020

What this PR does / why we need it:
If ACME (or any other that has no CA.crt) issues an intermediate certificate it is dropped from our pkcs12 keystore.
This was due to the logic being inside an if statement that checked the CA.crt secret.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes #3039

Release note:

Fix issue where intermediate certificates are not in the pkcs12 keystore

@meyskens
Add intermediate cert to P12 chain if ca.crt is empty
43e8e96 
Signed-off-by: Maartje Eyskens <maartje@eyskens.me>
@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 4, 2020
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Aug 4, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meyskens

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot requested a review from JoshVanL August 4, 2020 14:26
@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2020
@meyskens
Copy link
Copy Markdown
Contributor Author

meyskens commented Aug 4, 2020

/kind bug

@jetstack-bot jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 4, 2020
@james-w
Copy link
Copy Markdown
Contributor

james-w commented Aug 6, 2020

There's a keystore_test.go, so is it possible to test this case?

@meyskens meyskens added this to the v1.0 milestone Aug 7, 2020
@wallrj
Copy link
Copy Markdown
Member

wallrj commented Aug 7, 2020 •
edited
Loading

There's a keystore_test.go, so is it possible to test this case?

I had a quick look. Yes it should be possible, but the tests currently generate a single self-sigend certificate as the CA.
And the test is, understandably, not ideal: https://github.com/jetstack/cert-manager/blob/497e64de4b6920c3b967aff855e1fb2af2b463b4/pkg/controller/certificates/internal/secretsmanager/keystore_test.go#L205-L215

But it looks like this is a known issue: golang/go#14015 and the work around is to use ToPem instead of Decode

Or update to latest version of the package from SSLMate which has a DecodeChain function https://godoc.org/software.sslmate.com/src/go-pkcs12#DecodeChain

@wallrj wallrj mentioned this pull request Aug 7, 2020
Merged
wallrj added a commit to wallrj/cert-manager that referenced this pull request Aug 7, 2020
@wallrj
Update to latest version of https://github.com/SSLMate/go-pkcs12
b2ed0a2 
This new version includes a
[pkcs12.DecodeChain](https://godoc.org/software.sslmate.com/src/go-pkcs12#DecodeChain)
which will help in testing cert-manager#3146

Signed-off-by: Richard Wall <richard.wall@jetstack.io>
@meyskens
Copy link
Copy Markdown
Contributor Author

meyskens commented Aug 12, 2020

/close

@wallrj will submit a new one with added testing

@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Aug 12, 2020

@meyskens: Closed this PR.

Details

In response to this:

/close

@wallrj will submit a new one with added testing

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wallrj wallrj mentioned this pull request Aug 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoshVanL JoshVanL Awaiting requested review from JoshVanL

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v1.0

Development

Successfully merging this pull request may close these issues.

The generated pkcs12 file doesn't include the compete certificate chain

4 participants

@meyskens @jetstack-bot @james-w @wallrj