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 API to create RFC-compliant CRLs #35428

Closed
jefferai opened this issue Nov 7, 2019 · 17 comments
Closed

crypto/x509: add API to create RFC-compliant CRLs #35428

jefferai opened this issue Nov 7, 2019 · 17 comments

Comments

@jefferai
Copy link

@jefferai jefferai commented Nov 7, 2019

What version of Go are you using (go version)?

$ 1.13.4

This is a bit of a redux of #13931, but while the fix there changed the serialization on the wire of the default value, it didn't address the underlying problem, which is that the X.509 RFC pretty clearly states (https://tools.ietf.org/html/rfc5280#section-5):

   Conforming CAs are not required to issue CRLs if other revocation or
   certificate status mechanisms are provided.  When CRLs are issued,
   the CRLs MUST be version 2 CRLs, include the date by which the next
   CRL will be issued in the nextUpdate field (Section 5.1.2.5), include
   the CRL number extension (Section 5.2.3), and include the authority
   key identifier extension (Section 5.2.1).

This means that (as specified a bit after that) although any conforming client should be able to parse and understand a CRL created with this function, technically every CRL created via this function is non-conforming, because even if the root is self-signed it's still a CA. There's an argument to be made that a third party could be signing and creating the CRLs, but the behavior there isn't a distinction that is made clear in the RFC and it seems reasonable to assume that the intention is for it to behave the same way.

This would probably require a new function to create v2 CRLs so that the parameters required for the extensions can be provided.

Obviously clients are free to then ignore this extra information, but that's up to the client; it seems pretty clear that anything acting as a CA is required to issue v2 CRLs.

@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Nov 7, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 7, 2019

@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Backlog Dec 5, 2019
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jan 28, 2020

Currently it seems the only divergence from 5280 is the lack of the CRLNumber extension, which does unfortunately make the CRLs unusable for a conforming CA.

A simple solution here would be to either introduce a CreateCRLWithExtensions method with essentially the identical signature except for the addition of a []pkix.Extension or a CreateCRLWithNumber with the addition of an int for the number. The former is obviously more flexible for extensibility purposes down the road, but the later obviously significantly presents a significantly narrower API.

@jefferai
Copy link
Author

@jefferai jefferai commented Jan 28, 2020

Or, CreateCRLWithNumberAndExtensions (name TBD :-) ) that makes it easy to set the number but also allows for extensions.

As a side note, it's a shame this got bumped down to backlog, considering that it's a public stdlib API function that always does the wrong thing.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jan 28, 2020

I propose the following new API. This is akin to the existing Certificate.CreateCRL in that it only create X509 v2 CRLs, but would be conformant, and provides extensibility down the road (i.e. other version could be supported by adding a Version field to CRLTemplate and requiring/disallowing specific fields if the version is set to a specific value).

// CreateCRL creates a new x509 v2 Certificate Revocation List.
//
// The CRL is signed by priv.
//
// revokedCerts may be nil, in which case an empty CRL will be created.
//
// The issuer distinguished name CRL field and authority key identifier extension
// are populated using the issuer certificate.
//
// The CRL number extension is populated using the Number field of template.
//
// The template fields NextUpdate must be greater than ThisUpdate.
//
// Any extensions in the Extensions field of template will be copied directly into
// the CRL.
//
// This method is differentiated from the Certificate.CreateCRL method as
// it creates X509 v2 conformant CRLs as defined by the RFC 5280 CRL profile.
// This method should be used if created CRLs need to be standards compliant.
func CreateCRL(rand io.Reader, issuer *x509.Certificate, priv crypto.Signer, template CRLTemplate) ([]byte, error)

// CRLTemplate contains the fields used to create an X.509 v2 Certificate
// Revocation list.
type CRLTemplate struct {
    RevokedCertificates []pkix.RevokedCertificate
    Number              int
	ThisUpdate          time.Time
	NextUpdate          time.Time
	Extensions          []pkix.Extension
}

There is obviously significant room here for bike shedding over function/type naming (and whether it should actually be a method on Certificate), but I'm not strongly opined on what is 'best' here. Personally it feels cleaner to use a top level function, rather than a method, but I could see it being a bit confusing having a top level function and a Certificate method using the exact same name... 😕

letsencrypt/boulder needs this functionality downstream so if this proposal looks acceptable to golang folks I'm happy to implement it and throw up a CL for the changes.

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 29, 2020

@jefferai if you retitle this as "proposal: crypto/x509: add API to create CRL with [etc]" it will be addressed by the proposal review committee, and probably milestoned to 1.15 if accepted.

rolandshoemaker pushed a commit to rolandshoemaker/go that referenced this issue Jan 30, 2020
The existing Certificate.CreateCRL method generates non-conformant CRLs and as
such cannot be used for implementations that require standards compliance. This
change implements a new top level method, CreateCRL, which generates compliant
CRLs, and offers an extensible API if any extensions/fields need to be
supported in the future.

Fixes golang#35428

Change-Id: I06ef833cb860077b2d42c1bb262a72c3e918aa0d
rolandshoemaker pushed a commit to rolandshoemaker/go that referenced this issue Jan 30, 2020
The existing Certificate.CreateCRL method generates non-conformant CRLs and as
such cannot be used for implementations that require standards compliance. This
change implements a new top level method, CreateCRL, which generates compliant
CRLs, and offers an extensible API if any extensions/fields need to be
supported in the future.

Fixes golang#35428

Change-Id: I06ef833cb860077b2d42c1bb262a72c3e918aa0d
rolandshoemaker pushed a commit to rolandshoemaker/go that referenced this issue Jan 31, 2020
The existing Certificate.CreateCRL method generates non-conformant CRLs and as
such cannot be used for implementations that require standards compliance. This
change implements a new top level method, CreateCRL, which generates compliant
CRLs, and offers an extensible API if any extensions/fields need to be
supported in the future.

Fixes golang#35428

Change-Id: I06ef833cb860077b2d42c1bb262a72c3e918aa0d
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 31, 2020

Change https://golang.org/cl/217298 mentions this issue: crypto/x509: add RFC 5280/X509v2 compliant CRL generation function

@jefferai
Copy link
Author

@jefferai jefferai commented Jan 31, 2020

I had been thinking of closing this and opening a new one as a proposal given the above comment, but with the submitted patch set I'm not sure if I should touch anything :-)

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 1, 2020

Just edit the title as I suggested and include the API name from the CL. The CL refs this issue by number.

No action can be taken on the CL until a proposal is reviewed and accepted here.

@jefferai jefferai changed the title crypto/x509: CRL generated by Certificate.CreateCRL is non-conforming and should be version 2 proposal: crypto/x509: add API to create RFC-compliant CRLs Feb 2, 2020
@gopherbot gopherbot added the Proposal label Feb 2, 2020
@chauncyc
Copy link
Contributor

@chauncyc chauncyc commented Feb 11, 2020

I have a use case where I want to be able to specify SignatureAlgorithm when creating a CRL (currently it uses the default (0): https://github.com/golang/go/blob/master/src/crypto/x509/x509.go#L2222). This could be included in the CRLTemplate proposed by rolandshoemaker@.

Instead of CRLTemplate, this struct could be named CertificateRevocationList; so that:

  1. The function can be named CreateCertificateRevocationList to avoid collision with CreateCRL.
  2. Eventually, there could be a ParseCertificateRevocationList to be consistent with ParseCertificate and ParseCertificateRequest.
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 11, 2020

I like the symmetry with Certificate and CertificateRequest and the option to add a parser later. CertificateRevocationList is a mouthful though. Just like we shortened CSR to CertificateRequest, how about shortening CRL to RevocationList?

Here's a suggested API. Note the following changes:

  • CreateRevocationList and RevocationList names
  • order of arguments to CreateRevocationList matching CreateCertificate
  • order of fields resembling Certificate and CertificateRequest
  • *big.Int for Number because it can be up to 20 bytes
  • ExtraExtensions instead of Extensions like Certificate and CertificateRequest (which I'm not a fan of, but we might as well keep it all consistent)
func CreateRevocationList(rand io.Reader, template *RevocationList,
	issuer *x509.Certificate, priv crypto.Signer) ([]byte, error)

type RevocationList struct {
	SignatureAlgorithm SignatureAlgorithm

	RevokedCertificates []pkix.RevokedCertificate

	Number          *big.Int
	ThisUpdate      time.Time
	NextUpdate      time.Time
	ExtraExtensions []pkix.Extension
}

Edit: removed Version int field.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 12, 2020

Adding to proposal minutes for wider visibility. Seems OK.

@rsc rsc added this to Active in Proposals Feb 12, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Feb 12, 2020

@FiloSottile these changes look good to me.

One thing I'd suggest against is including Version, as far as I know there is no real call for creating anything other than X509v2 CRLs. I believe the field in Certificate only exists for parsing, as CreateCertificate only creates X509v3 certs. I think it's probably best to leave it out and then if a parsing method is added down the line to include it then in order to avoid confusion.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 12, 2020

One thing I'd suggest against is including Version, as far as I know there is no real call for creating anything other than X509v2 CRLs. I believe the field in Certificate only exists for parsing, as CreateCertificate only creates X509v3 certs. I think it's probably best to leave it out and then if a parsing method is added down the line to include it then in order to avoid confusion.

Agreed, we can introduce it along with a parsing function if needed.

@jefferai
Copy link
Author

@jefferai jefferai commented Feb 14, 2020

The proposed struct looks good to me too!

@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Feb 26, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Feb 27, 2020

Assuming there are no other major comments I'll look to update my existing CL sometime next week to reflect the proposal by @FiloSottile.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 4, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 4, 2020
@rsc rsc changed the title proposal: crypto/x509: add API to create RFC-compliant CRLs crypto/x509: add API to create RFC-compliant CRLs Mar 4, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Mar 4, 2020
Bobobo-bo-Bo-bobo pushed a commit to Bobobo-bo-Bo-bobo/pkidb that referenced this issue Mar 7, 2020
@gopherbot gopherbot closed this in 5d47f87 Mar 23, 2020
@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
133 of 133 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants
You can’t perform that action at this time.