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: surface ReasonCode inside x509.RevocationList entries #53573

Closed
aarongable opened this issue Jun 27, 2022 · 9 comments
Closed

crypto/x509: surface ReasonCode inside x509.RevocationList entries #53573

aarongable opened this issue Jun 27, 2022 · 9 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Milestone

Comments

@aarongable
Copy link
Contributor

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
Copy link
Contributor Author

aarongable commented Jun 27, 2022

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

// 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 ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jun 28, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 28, 2022
@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jun 29, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

rsc commented Jul 1, 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

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 1, 2022
@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jul 13, 2022
@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

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

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 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: surface ReasonCode inside x509.RevocationList entries crypto/x509: surface ReasonCode inside x509.RevocationList entries Jul 20, 2022
@rsc rsc modified the milestones: Proposal, Backlog Jul 20, 2022
@aarongable
Copy link
Contributor Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Projects
No open projects
Development

No branches or pull requests

6 participants