x/crypto/x509roots: new module

[rolandshoemaker]

#43958 is the accepted proposal for introducing the x509.SetFallbackRoots API, and had a rough outline of the bundle module. We want to have a slightly different API for the bundle package than was originally outlined there, so instead of continuing the discussion in the previous issue, I'm opening a new proposal that is tightly focused on x/crypto/x509roots (the name was previously decided on, this proposal will not rehash that discussion).

x/crypto/x509roots will be a submodule of x/crypto, containing the root package which contains the NSS certdata.txt parser, and a package x/crypto/x509roots/fallback, which registers the fallbacks on import.

API for x/crypto/x509roots:

// Package x509roots provides functionality for parsing NSS certdata.txt
// formatted certificate lists and extracting serverAuth roots. The parser
// provided by this package is very opinionated, only returning roots that are
// currently trusted for serverAuth. As such roots returned by this package
// should only be used for making trust decisions about serverAuth certificates,
// as the trust status for other uses is not considered. Using the roots
// returned by this package for trust decisions should be done carefully.
//
// Some roots returned by the parser may include additional constraints
// (currently only DistrustAfter) which need to be considered when verifying
// certificates which chain to them.
package x509roots

// NSSConstraint is a constraint to be applied to a certificate or
// certificate chain.
type NSSConstraint any

// NSSDistrustAfter is a NSSConstraint that indicates a certificate has a
// CKA_NSS_SERVER_DISTRUST_AFTER constraint. This constraint defines a date
// after which any certificate issued which is rooted by the constrained
// certificate should be distrusted.
type NSSDistrustAfter time.Time

// NSSCert represents a single trusted serverAuth certificate in the NSS
// certdata.txt list and any constraints that should be applied to chains
// rooted by it.
type NSSCert struct {
	// Certificate is the parsed certificate
	Certificate   *x509.Certificate
	// Constraints contains a list of additional constraints that should be
	// applied to any certificates that chain to Certificate. If there are
	// any unknown constraints in the slice, Certificate should not be
	// trusted.
	Constraints []NSSConstraint
}

// ParseNSSCertData parses a NSS certdata.txt formatted file, returning only
// trusted serverAuth roots, as well as any additional constraints.
func ParseNSSCertData(r io.Reader) ([]NSSCert, error)

API for x/crypto/x509roots/fallback (taken verbatim from #43958, other than package name changes):

// Package fallback embeds a set of fallback X.509 trusted roots in the
// application by automatically invoking [x509.SetFallbackRoots]. This allows
// the application to work correctly even if the operating system does not
// provide a verifier or system roots pool.
//
// To use it, import the package like
//
//	import _ "golang.org/x/crypto/x509roots/fallback"
//
// It's recommended that only binaries, and not libraries, import this package.
//
// This package must be kept up to date for security and compatibility reasons.
// Use govulncheck to be notified of when new versions of the package are
// available.
package fallback

cc @FiloSottile

[FiloSottile]

LGTM. Only returning serverAuth makes sense per #26624.

One thing I'm worried about is that if we ever add a new constraint to NSSCert, code written against an old version will consider them unconstrained. Is it worth making the field a []NSSConstraint so code can panic on unknown values?

If we ever switch to a different root store, we can add new functions to x509roots and replace the one we use in fallback.

nit: do we usually use nil *time.Time or zero time.Time as the missing value sentinel?

Not necessary to ship the package, but we should soon after implement automation to notice changes to the pool, and mail CLs to update the submodule and file a govulncheck entry.

[rolandshoemaker]

One thing I'm worried about is that if we ever add a new constraint to NSSCert, code written against an old version will consider them unconstrained. Is it worth making the field a []NSSConstraint so code can panic on unknown values?

Oh hm, good point. Do you imagine the NSSConstraint would be something like type NSSConstraint any and then we'd have type NSSDistrustDate time.Time etc, with a comment along the lines of "callers should not trust any certificate which contains constraints which they do not understand".

[FiloSottile]

I couldn't come up with anything more elegant than that, so yeah :)

[ianlancetaylor]

Note that the 1.20 release notes mention this package, which does not yet exist, so it would be good to prioritize this. Or change the release notes.

[rolandshoemaker]

We have an inflight CL for this, I'd like to get this in ASAP rather than updating the release notes, but at some point that may be prudent.

[gopherbot]

Change https://go.dev/cl/462036 mentions this issue: x509roots: add new module

[rolandshoemaker]

(I realized I forgot the golang/go prefix for the Fixes line 🤦)

[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]

Does the NSS prefix need to be on every function, or is that implied by the x509roots package name?

[rolandshoemaker]

I think we previously discussed making the name of the package somewhat generic, in part in case we wanted to provide multiple different trust stores in the future (hence it's x509roots, not nssroots). Currently all of these functions and types are extremely specific to the NSS list, so if we strip the prefix some context is lost if we want to add non-NSS things in the future. It's not particularly pretty, but I think it somewhat future-proofs the package.

[rsc]

We need x/crypto/x509roots/fallback, which has no API.
Do we need to export a parser for NSS files?
Or maybe the NSS parser should be x509roots/nss?
Do we want to enable others to use the parser?
If we just need it for the fallback, it can be internal.

[rsc]

Spoke to @rolandshoemaker about this. We don't need to expose the parser in order to deliver the fallback package, but people had been asking for the NSS parser anyway, so it probably makes sense to export it. Making the parser x509roots/nss makes sense and then the API from above is:

package nss

// Constraint is a constraint to be applied to a certificate or
// certificate chain.
type Constraint any

// DistrustAfter is a Constraint that indicates a certificate has a
// CKA_NSS_SERVER_DISTRUST_AFTER constraint. This constraint defines a date
// after which any certificate issued which is rooted by the constrained
// certificate should be distrusted.
type DistrustAfter time.Time

// Cert represents a single trusted serverAuth certificate in the NSS
// certdata.txt list and any constraints that should be applied to chains
// rooted by it.
type Cert struct {
	// Certificate is the parsed certificate
	Certificate   *x509.Certificate
	// Constraints contains a list of additional constraints that should be
	// applied to any certificates that chain to Certificate. If there are
	// any unknown constraints in the slice, Certificate should not be
	// trusted.
	Constraints []Constraint
}

// Parse parses a NSS certdata.txt formatted file, returning only
// trusted serverAuth roots, as well as any additional constraints.
func Parse(r io.Reader) ([]Cert, error)

I think we probably should spell out Certificate to match x509, so users don't have to remember which package uses which spelling. Similarly the field in Cert should be X509 *x509.Certificate not Certificate *x509.Certificate.

I wonder if Constraint should have actual methods instead of being 'any'. There is a mention of CKA_NSS_SERVER_DISTRUST_AFTER in the comment. If that's an enumeration maybe a type Kind int and a method Kind() Kind would be useful and also serve to make not everything in the world a valid nss.Constraint.

Above it was assumed that x/crypto/x509roots would be its own module. I thought the point of the module boundary was to separate the high-churn generated fallback package from manually maintained, low-churn code, so that (for example) code using x/crypto doesn't pull down the entire history of the x509roots. If that's still the rationale, it doesn't make sense for x509roots/nss to be in the high-churn half: it would fit the rationale better for x/crypto/x509roots/fallback to be its own module while the rest stays in x/crypto. Perhaps I am misunderstanding the rationale though.

[rsc]

Any thoughts on the suggestions in my previous comment, especially about x509roots not being the module boundary?

[rolandshoemaker]

Oh sorry, I completely missed your last comment after we talked.

Switching to Certificate definitely makes sense and adding Kind on Constraint seems reasonable, and I like that it prevents everything from being a valid Constraint.

I think moving the module boundary probably does make sense. The initial rationale is as you described, along with giving us the ability to use entries in the vulnerability database as a way to signal to users that they should update the module when the baked in list changes.

One of the reasons we initially put the parser and fallback packages in the same module was that any changes to the former were likely to require an update to the latter, so bundling them together would allow us to avoid having to do chained tagging/dependency updates, but since we want to allow users to use just the parser, I think the argument you present does make sense (splitting the fallbacks into their own isolated module also means that if we do use vulnerability database entries as an update signal, we won't create noise in symbol/package unaware vulnerability scanners, which only operate at the module level, for users of just the parser code).

If we do make this split I guess the question is where do we put the module boundary. Should x/crypto/x509roots/fallback be the separate module, or should x/crypto/x509roots be the module, and we move the nss package somewhere else? I think the former probably makes more sense in terms of organizational clarity.

[rsc]

I agree that x/crypto/x509roots/fallback should be its own module; everything else is part of x/crypto.

[rsc]

Summarizing from above, the proposed API for golang.org/x/crypto/x509roots/nss is:

package nss

// Constraint is a constraint to be applied to a certificate or
// certificate chain.
type Constraint interface {
    Kind() Kind
}

// Kind is the constraint kind, using the NSS enumeration.
type Kind int

const (
    CKA_NSS_SERVER_DISTRUST_AFTER Kind = ...
)


// DistrustAfter is a Constraint that indicates a certificate has a
// CKA_NSS_SERVER_DISTRUST_AFTER constraint. This constraint defines a date
// after which any certificate issued which is rooted by the constrained
// certificate should be distrusted.
type DistrustAfter time.Time

func (DistrustAfter) Kind() Kind

// A Certificate represents a single trusted serverAuth certificate in the NSS
// certdata.txt list and any constraints that should be applied to chains
// rooted by it.
type Certificate struct {
	// X509 is the parsed certificate
	X509   *x509.Certificate

	// Constraints contains a list of additional constraints that should be
	// applied to any certificates that chain to Certificate. If there are
	// any unknown constraints in the slice, Certificate should not be
	// trusted.
	Constraints []Constraint
}

// Parse parses a NSS certdata.txt formatted file, returning only
// trusted serverAuth roots, as well as any additional constraints.
func Parse(r io.Reader) ([]*Certificate, error)

Note that I changed Parse to return []*Certificate not []Certificate. You almost always want slices of pointers to structs for that kind of thing to avoid copying structs and getting confused.

And then also golang.org/x/crypto/x509roots/fallback:

// Package fallback embeds a set of fallback X.509 trusted roots in the
// application by automatically invoking [x509.SetFallbackRoots]. This allows
// the application to work correctly even if the operating system does not
// provide a verifier or system roots pool.
//
// To use it, import the package like
//
//	import _ "golang.org/x/crypto/x509roots/fallback"
//
// It's recommended that only binaries, and not libraries, import this package.
//
// This package must be kept up to date for security and compatibility reasons.
// Use govulncheck to be notified of when new versions of the package are
// available.
package fallback

golang.org/x/crypto/x509roots/fallback is its own module; everything else is in x/crypto.

[rolandshoemaker]

Sounds good to me.

[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

[gopherbot]

Change https://go.dev/cl/505578 mentions this issue: x509roots/fallback: add //go:build go1.20 to bundle.go