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

[release-1.4] Fix check for self-signed certs in EncodeX509Chain #4238

Conversation

jetstack-bot
Copy link
Collaborator

This is an automated cherry-pick of #4237

/assign JoshVanL

Fix check for self-signed certificates in EncodeX509Chain which broke certs whose subject DN matched their issuer's subject DN

see also cert-manager#4142

EncodeX509Chain checked for self-signed certs by comparing the subject
and issuer of the cert in question, which is invalid since it's
perfectly fine for those to match.

the correct behavior is to use cert.CheckSignatureFrom(cert). this bug
was exposed in 1.4 when ParseSingleCertificateChain started using
EncodeX509Chain in the critical path of several issuers; when end-users
had leaf certificates with subjects matching their issuer's subject, the
bug was triggered.

includes newly written tests for EncodeX509Chain and a test for
ParseSingleCertificateChain

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 21, 2021
@JoshVanL
Copy link
Collaborator

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2021
@JoshVanL
Copy link
Collaborator

/cherry-pick-approve

@JoshVanL
Copy link
Collaborator

/approve

@jetstack-bot
Copy link
Collaborator Author

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jetstack-bot, JoshVanL

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

The pull request process is described here

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021
@JoshVanL
Copy link
Collaborator

/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 Jul 21, 2021
@JoshVanL JoshVanL added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager label Jul 21, 2021
@jetstack-bot jetstack-bot removed the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Jul 21, 2021
@jetstack-bot jetstack-bot merged commit f970eca into cert-manager:release-1.4 Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager 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. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants