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

Fix manually specified Certificate and CertificateRequest versions #4392

Merged
merged 1 commit into from Aug 20, 2021

Conversation

SgtCoDFish
Copy link
Member

@SgtCoDFish SgtCoDFish commented Aug 19, 2021

Basically all modern X.509 certs are version 3, but confusingly to specify "version 3" in an encoded cert, the version number is actually 2.

For PKCS#10 CSRs, the only valid version is 1, which again confusingly has the value "0" when encoded.

This was incorrect in many places, including one place in which the version number on a CSR was used as a certificate's version number, when the two are entirely unrelated.

Go ignores these values, so there's no functional changes here; still, it's better to be accurate.

Go ignoring CSR version and specifying 0: https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/crypto/x509/x509.go;l=1958

Go ignoring Certificate version and specifying 2: https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/crypto/x509/x509.go;l=1534

PKCS#10 CSR specification in RFC 2986 section 4.1: https://datatracker.ietf.org/doc/html/rfc2986#section-4

version is the version number, for compatibility with future revisions of this document. It shall be 0 for this version of the standard.

X.509 Cert specification in RFC 5280 section 4.1.2.1: https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1

When extensions are used, as expected in this profile, version MUST be 3 (value is 2).

/kind cleanup

Fix manually specified PKCS#10 CSR and X.509 Certificate version numbers (although these were ignored in practice)

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 19, 2021
Basically all modern X.509 certs are version 3, but confusingly to
specify "version 3" in an encoded cert, the version number is actually
2.

For PKCS#10 CSRs, the only valid version is 1, which again
confusingly has the value "0" when encoded.

This was incorrect in many places, including one place in which the
version number on a CSR was used as a certificate's version number,
when the two are entirely unrelated.

Go ignores these values, so there's no functional changes here; still,
it's better to be accurate.

Go ignoring CSR version and specifying 0:
https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/crypto/x509/x509.go;l=1958

Go ignoring Certificate version and specifying 2:
https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/crypto/x509/x509.go;l=1534

PKCS#10 CSR specification in RFC 2986 section 4.1:
https://datatracker.ietf.org/doc/html/rfc2986#section-4

X.509 Cert specification in RFC 5280 section 4.1.2.1:
https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
Copy link
Member

@jakexks jakexks left a comment

Choose a reason for hiding this comment

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

Thanks for linking the RFCs in comments!
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakexks, SgtCoDFish

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 merged commit 0ff741c into cert-manager:master Aug 20, 2021
@SgtCoDFish SgtCoDFish deleted the fixversions branch August 20, 2021 11:19
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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants