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

crypto/x509: add ParseRevocationList, deprecate ParseCRL & ParseDERCRL #50674

Closed
rolandshoemaker opened this issue Jan 18, 2022 · 7 comments
Closed
Labels
Milestone

Comments

@rolandshoemaker
Copy link
Member

The current implementation of CRLs in crypto/x509 and crypto/x509/pkix is somewhat confusing, easy to misuse, and does not match the design of the rest of the package. In particular it doesn't expose the necessary information to do safe issuer verification (see grpc/grpc-go#5130 for an example of how this can go wrong.)

We could try to provide the extra information required to do safe comparisons, but unfortunately due to the design of pkix.CertificateList, which is intended to be a direct ASN.1 analog, it is not possible to add new fields, since it will inherently change how encoding/asn1 encodes/decodes related data.

In go1.15 we introduced RevocationList, a type used as input to CreateRevocationList. I propose that we introduce ParseRevocationList, and convert RevocationList into a Go representation of a CRL, similar to how Certificate is used. This results in an API much more in line with the rest of the package, and would allow us more leeway to update the representative CRL structure without having to worry about its direct ASN.1 encoding.

For the sake of slowly moving away from reliance on encoding/asn1, the new ParseRevocationList function should employ a x/crypto/cryptobyte parser.

This would deprecate the pkix.CertificateList type (and associated types), the ParseCRL and ParseDERCRL functions, and the Certificate.CheckCRLSignature method (the latter being replaced with a method on RevocationList.)

// Added (crypto/x509)
func ParseRevocationList(der []byte) (*RevocationList, error)

func (rl *RevocationList) CheckSignatureFrom(issuer *Certificate) error

type RevocationList struct {
  ...
  // New fields
  RawIssuer      []byte
  Signature      []byte
  AuthorityKeyId []byte
  Extensions     []pkix.Extension
}

// Deprecated (crypto/x509)
func ParseCRL(crlBytes []byte) (*pkix.CertificateList, error)
func ParseDERCRL(derBytes []byte) (*pkix.CertificateList, error)
func (c *Certificate) CheckCRLSignature(crl *pkix.CertificateList) error

// Deprecated (crypto/x509/pkix)
type CertificateList ...
type TBSCertificateList ...

cc @golang/security

@gopherbot gopherbot added this to the Proposal milestone Jan 18, 2022
@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jan 18, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 19, 2022
@rsc rsc moved this from Incoming to Active in Proposals (old) Jan 19, 2022
@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@FiloSottile FiloSottile changed the title proposal: crypto/x509: add CreateRevocationList, deprecate ParseCRL & ParseDERCRL proposal: crypto/x509: add ParseRevocationList, deprecate ParseCRL & ParseDERCRL Jan 20, 2022
@rsc
Copy link
Contributor

rsc commented Feb 9, 2022

Does anyone object to the API in the top comment?

@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 16, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Feb 23, 2022
@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/x509: add ParseRevocationList, deprecate ParseCRL & ParseDERCRL crypto/x509: add ParseRevocationList, deprecate ParseCRL & ParseDERCRL Feb 23, 2022
@rsc rsc modified the milestones: Proposal, Backlog Feb 23, 2022
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.19 Mar 2, 2022
@gopherbot
Copy link

Change https://go.dev/cl/390834 mentions this issue: crypto/x509: add new CRL parser, deprecate old one

@rsc
Copy link
Contributor

rsc commented Jun 27, 2022

CL 390834 added three additional fields beyond what was in this proposal:

Raw                  []byte
RawTBSRevocationList []byte
Issuer         pkix.Name

Noting here in case anyone objects to them, although I doubt it given the vigor of the earlier discussion.

@gopherbot
Copy link

Change https://go.dev/cl/414635 mentions this issue: crypto/x509: improve RevocationList documentation

gopherbot pushed a commit that referenced this issue Jun 29, 2022
Adds documentation for a handful of RevocationList fields.

Updates #50674

Change-Id: I26b838553d870b631deaf8b9a5b4d0b251fdef20
Reviewed-on: https://go-review.googlesource.com/c/go/+/414635
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
Adds documentation for a handful of RevocationList fields.

Updates golang#50674

Change-Id: I26b838553d870b631deaf8b9a5b4d0b251fdef20
Reviewed-on: https://go-review.googlesource.com/c/go/+/414635
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

No branches or pull requests

5 participants