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: support code-constrained roots #57178

Closed
rolandshoemaker opened this issue Dec 8, 2022 · 11 comments
Closed

crypto/x509: support code-constrained roots #57178

rolandshoemaker opened this issue Dec 8, 2022 · 11 comments
Assignees
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@rolandshoemaker
Copy link
Member

CertPool and Certificate.Verify currently only support roots constrained using X509 semantics. For most cases this is fine, but with the introduction of the golang.org/x/crypto/x509roots/fallback package (#43958), which plans to use the Mozilla NSS root list, there is a need to implement additional constraints.

The Mozilla NSS list encodes additional trust constraints for specific root certificates, such as only trusting certificates issued before a specific date (e.g. what is happening with TrustCor, or what previous happened with Symantec), etc. In order to support these constraints we need a new API which allows for attaching additional checks to certificates which chain to particular roots.

The API suggested below, CertPool.AddCertWithConstraint, would allow attaching a callback to a root, which Certificate.Verify would call during chain building. There would be no other externally visible API changes, as the root->constraint mapping would be a private structure in the CertPool, accessible only by Certificate.Verify.

As always the name is up for debate. The meaning of 'Constraint' in X509 is perhaps overloaded, AddCertWithAdditionalChecks could also work, but is possibly a little verbose. Another question is if the function should accept a single callback, or a list of callbacks (i.e. should the caller compose all of their constraints into a single callback, or should we allow passing multiple callbacks that we check ourselves serially).

// AddCertWithConstraint adds a certificate to the pool with the additional constraint.
// When Certificate.Verify builds a chain which is rooted by cert, it will additionally
// pass the chain to constraint to determine its validity. If constraint returns a non-nil
// error, the chain will be discarded.
func (*CertPool) AddCertWithConstraint(cert *Certificate, constraint func([]*Certificate) error)

cc @FiloSottile @golang/security

@rolandshoemaker rolandshoemaker self-assigned this Dec 8, 2022
@gopherbot gopherbot added this to the Proposal milestone Dec 8, 2022
@seankhliao
Copy link
Member

This allows adding constraints to individual certs as they're added to the pool, but what if you're starting with the system pool and want to layer additional checks on top of existing certs/system verifier checks?

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Dec 9, 2022
@rolandshoemaker
Copy link
Member Author

I don't think we want to support adding additional arbitrary constraints to platform verifiers. Platforms have their own ways of applying constraints to trust stores (i.e. both Windows and macOS allow you to constrain certificates in complicated ways), and I think we'd prefer that the platform verifier, that can more properly understand those constraints, apply them themselves.

If users really want to apply their own constraints purely in code, that can be done by wrapping calls to Verify with the logic that excludes specific chains.

For platforms that don't allow applying additional constraints (namely Linux), the root pool can be loaded manually, and constraints can be added at that point (we've considered exposing the logic used to load Linux trust stores, which perhaps we should do to make this simpler).

@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

What is the motivation for this, and how does it affect the feasibility of the algorithms for building the chain? Do we have to build all possible chains ending at a given root to try to make the constraint happy?

And why is it only about roots? That makes the combinatorics better (but not great), but nothing in the API suggests it would only be about roots.

Is there something simpler we can do, like allowing a parent cert to disallow a direct child?

@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

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

@rolandshoemaker
Copy link
Member Author

The motivation as described in the parent comment is that there are trust stores that encode additional trust constraints outside of those that can be defined in X509. In particular with the introduction of the golang.org/x/crypto/x509roots package, there are roots in the NSS list that contain additional constraints around trust based on issuance dates. Currently we just ignore roots with these extra constraints, but it'd be more valuable for users to attempt to match behavior of NSS itself, and apply the additional constraints consistently.

This has no meaningful impact on the chain building algorithm, it can be implemented in a similar way to how EKU chaining checks are applied. We already build all chains from a leaf.

It is unlikely you'd want to apply constraints to intermediates (the only other anchor type feasibly in a CertPool), but there is not really any extra complexity to check both roots and intermediates (either during the path building process, or once it is completed).

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

Spoke to @rolandshoemaker about this. The code already generates all possible chains (paths from the leaf back to a known root), and while that is potentially exponential in the number of intermediate certs, you can't get an signed, trusted intermediate cert without essentially being a CA, so the opportunity does not really exist for a malicious peer to create a large enough graph problem to cause issues. Things are also helped by the relevant TLS message being fairly limited in size.

This new API locks us in to that decision, but some other semantics of certficate chain validation arguably do too, and since the attack surface seems not to exist, using this API seems okay to me.

@rsc
Copy link
Contributor

rsc commented Mar 8, 2023

Have this all concerns about this proposal been addressed?

@rolandshoemaker
Copy link
Member Author

I believe so.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

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

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

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: support code-constrained roots crypto/x509: support code-constrained roots Mar 29, 2023
@rsc rsc modified the milestones: Proposal, Backlog Mar 29, 2023
@gopherbot
Copy link

Change https://go.dev/cl/519315 mentions this issue: crypto/x509: implement AddCertWithConstraint

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
Projects
Status: Accepted
Development

No branches or pull requests

5 participants