crypto/x509: surface ReasonCode inside x509.RevocationList entries

[aarongable]

Background

The crypto/x509 package contains a variety of types representing objects which make up the PKI ecosystem, such as certificates, OCSP requests and responses, and CRLs. Most of these types represent both the underlying ASN.1 structure (e.g. the Certificate type has a RawTBSCertificate field) and a collection of ergonomic helpers which abstract away ASN.1 details (e.g. the Certificate type has the DNSNames field, which corresponds with a subset of the contents of the SubjectAltNames extension).

The crypto/x509/pkix sub-package contains a collection of types which are much more closely tied to their raw ASN.1 definitions, without the addtional ergonomics provided by the crypto/x509 types. These types are very useful for directly (un)marshalling ASN.1 data.

Currently, CRLs exist in a sort of in-between state. The x509.RevocationList type has a Number field which abstracts away the underlying ASN.1 crlNumber extension. However, the actual list of entries inside an x509.RevocationList is a []pkix.RevokedCertificate. This means that we do not have a ReasonCode field to abstract away the underlying ASN.1 reasonCode extension.

Objective

Provide a way for the crypto/x509 package to expose the revocation reason for every entry in a CRL, without requiring the user to directly interface with ASN.1 extensions.

Proposal

Add a new type to the x509 package which represents a single CRL entry with extra ergonommics added on top:

// CRLEntry represents a single entry in a Certificate Revocation List.
type CRLEntry struct {
	SerialNumber     *big.Int
	RevocationDate   time.Time
	RevocationReason int
	ExtraExtensions  []pkix.Extension
}

The RevocationReason field would take the same values as the crypto/ocsp.Response.RevocationReason field. It is unfortunate that the revocation reason constants are all defined in the crypto/ocsp package (e.g. ocsp.KeyCompromise) despite being originally defined in RFC 5280; perhaps moving those into the crypto/x509 package and giving them their own type would be part of this proposal as well.

Add a new field to x509.RevocationList which is a list of the above new type. Deprecate the old RevokedCertificates field.

type RevocationList struct {
	...
	// Entries is used to populate the revokedCertificates
	// sequence in the CRL, or is populated from the revokedCertificates
	// sequence when parsing a CRL. It is an error for both CRLEntries
	// and RevokedCertificates to be non-nil when passed as a template
	// to CreateRevocationList.
	Entries []x509.CRLEntry

	// RevokedCertificates is used to populate the revokedCertificates
	// sequence in the CRL, if Entries is nil.
	// DEPRECATED: use Entries instead.
	RevokedCertificates []pkix.RevokedCertificate
	...
}

[aarongable]

I'll also note that the current code on master indicates that pkix.RevokedCertificate is deprecated:

go/src/crypto/x509/pkix/pkix.go

Lines 312 to 320 in 3af5280

	// RevokedCertificate represents the ASN.1 structure of the same name. See RFC
	// 5280, section 5.1.
	//
	// Deprecated: x509.RevocationList should be used instead.
	type RevokedCertificate struct {
	SerialNumber *big.Int
	RevocationTime time.Time
	Extensions []Extension `asn1:"optional"`
	}

even though it is still used by the current x509.RevocationList on master:

go/src/crypto/x509/x509.go

Lines 2113 to 2116 in 3af5280

	// RevokedCertificates is used to populate the revokedCertificates
	// sequence in the CRL, it may be empty. RevokedCertificates may be nil,
	// in which case an empty CRL will be created.
	RevokedCertificates []pkix.RevokedCertificate

Edit: This is fixed by https://go-review.googlesource.com/c/go/+/414754

[ianlancetaylor]

CC @golang/security

[rolandshoemaker]

Seems reasonable, unfortunate that we need to add a whole new type just for a single field, but I guess it also opens up adding more convenience fields further down the road if we really need to. Generally making pkix less user facing is good.

The relation with RevocationList may be clearer if the new type was named RevocationListEntry or something. Also implementation needs to take care to ignore the zero value of RevocationReason so we aren't adding an unnecessary unspecified extension to every entry.

[aarongable]

Yeah, I don't have any particular feelings about the name of the new type or the new field.

I've implemented a non-backwards-compatible version of this, including handling of the zero-value ReasonCode, if taking a look at a potential implementation is of interest: letsencrypt/boulder@6c93e9d

[rsc]

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

[rsc]

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

[rsc]

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

[aarongable]

My current implementation has become more concretely fleshed out since I linked it above. It now treats RevokedCertificateEntries as essentially equals to Certificates and RevocationLists, with fully-fledged one-way Extensions (used during parsing only) and ExtraExtensions (used during creation only) fields.

The current version looks like this:

// RevokedCertificate represents an entry in the revokedCertificates sequence of
// a CRL.
type RevokedCertificate struct {
	// Raw contains the raw bytes of the revokedCertificates entry. It is set when
	// parsing a CRL; it is ignored when generating a CRL.
	Raw []byte

	// SerialNumber represents the serial number of a revoked certificate. It is
	// both used when creating a CRL and populated when parsing a CRL. It MUST NOT
	// be nil.
	SerialNumber *big.Int
	// RevocationTime represents the time at which the certificate was revoked. It
	// is both used when creating a CRL and populated when parsing a CRL. It MUST
	// NOT be nil.
	RevocationTime time.Time
	// ReasonCode represents the reason for revocation, using the integer enum
	// values specified in RFC 5280 Section 5.3.1. When creating a CRL, a value of
	// nil or zero will result in the reasonCode extension being omitted. When
	// parsing a CRL, a value of nil represents a no reasonCode extension, while a
	// value of 0 represents a reasonCode extension containing enum value 0 (this
	// SHOULD NOT happen, but can and does).
	ReasonCode *int

	// Extensions contains raw X.509 extensions. When creating a CRL, the
	// Extensions field is ignored, see ExtraExtensions.
	Extensions []pkix.Extension
	// ExtraExtensions contains any additional extensions to add directly to the
	// revokedCertificate entry. It is up to the caller to ensure that this field
	// does not contain any extensions which duplicate extensions created by this
	// package (currently, the reasonCode extension). The ExtraExtensions field is
	// not populated when parsing a CRL, see Extensions.
	ExtraExtensions []pkix.Extension
}

I'm happy to work on upstreaming this directly, but I also want to make sure I don't step on your toes @rolandshoemaker since I know this is an area you're doing a lot of work in.

[gopherbot]

Change https://go.dev/cl/468875 mentions this issue: x509: Improve RevocationList API