proposal: crypto/x509: support Inhibit Any-policy and Policy Constraints

[satsrini]

Important

The current proposal is #68484 (comment).

Go version

go version go1.20.4 windows/amd64

Output of go env in your module/workspace:

GO111MODULE=
 GOARCH=amd64
 GOBIN=
 GOCACHE=C:\Users\ ****\AppData\Local\go-build
 GOENV=C:\Users\ ****\AppData\Roaming\go\env
 GOEXE=.exe
 GOEXPERIMENT=
 GOFLAGS=
 GOHOSTARCH=amd64
 GOHOSTOS=windows
 GOINSECURE=
 GOMODCACHE=C:\git\pkg\mod
 GONOPROXY=
 GONOSUMDB=
 GOOS=windows
 GOPATH=C:\git
 GOPRIVATE=
 GOPROXY=https://proxy.golang.org,direct
 GOROOT=C:\Program Files\Go
 GOSUMDB=sum.golang.org
 GOTMPDIR=
 GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
 GOVCS=
 GOVERSION=go1.20.4
 GCCGO=gccgo
 GOAMD64=v1
 AR=ar
 CC=gcc
 CXX=g++
 CGO_ENABLED=1
 GOMOD=c:\git\****\****\go.mod
 GOWORK=
 CGO_CFLAGS=-O2 -g
 CGO_CPPFLAGS=
 CGO_CXXFLAGS=-O2 -g
 CGO_FFLAGS=-O2 -g
 CGO_LDFLAGS=-O2 -g
 PKG_CONFIG=pkg-config
 GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\S****\AppData\Local\Temp\go-build1520036143=/tmp/go-build -gno-record-gcc-switches

What did you do?

To validate/verify the X509 certificates that contain the following X509 critical extensions using https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L753

               _a. X509v3 Policy Constraints: critical
                Require Explicit Policy:0, Inhibit Policy Mapping:0

               b. X509v3 Inhibit Any Policy: critical
                0_          

code snippet:

_,err = x509cert.Verify(x509.VerifyOptions{
		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
		Roots:         rootCertsPool,
		Intermediates: intermediatesCertPool,
	})

What did you see happen?

It throws the error saying
"x509: unhandled critical extension"

What did you expect to see?

Certificate Verification done successfully with out any error.

Additional info:

This is because of the following code.
https://github.com/golang/go/blob/master/src/crypto/x509/parser.go#L780
https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L565

as the following Extensions OIDs are not handled in the above mentioned GO Lang code.
2.5.29.54 (Require Explicit Policy:0, Inhibit Policy Mapping:0)
2.5.29.36 (X509v3 Inhibit Any Policy: critical)

[gabyhelp]

Related Issues and Documentation

• crypto/x509: invalid certificate policies #65990 (closed)
• crypto/x509: support OIDs for tcg-kp (2.23.133.8) #47620
• crypto/x509: parse SAN DirectoryName #47618 (closed)
• crypto/x509: parseCertificate fails if Cert Policy extension is critical #9964 (closed)
• crypto/x509: error parsing large ASN.1 identifiers #49678 (closed)
• crypto/x509: Certificate.Verify does not consistently return UnknownAuthorityError #58777
• crypto/x509: Verify fails to find valid path when some paths have EKU constraint violations #48869 (closed)
• crypto/x509: malformed x509 certificate is accepted since 1.17 #51369
• crypto/x509: CreateRevocationList over-enforces KeyUsage check #59669
• crypto/x509: NameConstraintsWithoutSANs when checking signing certificate #24151 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

What certificates make use of these extensions?

[satsrini]

Hi @seankhliao , DOD certificates make use of these extensions.

eg.
download https://dl.dod.cyber.mil/wp-content/uploads/pki-pke/zip/unclass-dod_approved_external_pkis_trust_chains.zip
You can check in DoD_Approved_External_PKIs_Trust_Chains_v11.0_20240513/DigiCert_Federal_SSP/Trust_Chain_1/1-Symantec_SSP_Intermediate_CA-G4.cer

[seankhliao]

cc @golang/security

[rolandshoemaker]

We don't support policy constraints because they are basically unused in the webpki, and impose somewhat complicated constraints on path building. Federal PKI is notorious for using complex/obscure RFC 5280 features, for better or worse, which are generally unused in the wider webpki, so it doesn't surprise me that we fail to parse/process some of their hierarchies.

While I'm not aware of any restrictions on using them in publicly trusted certificates, without significant usage in webpki certificates I'm not convinced that we should add the necessary path building complexity to support policy constraints.

[satsrini]

ok, thanks a lot team.

[cherrymui]

Sounds like we can close this for now. If things change in the future and there is a necessity for support this, we can reopen. Thanks.

[sgmiller]

We're seeing this in other branches of the US government as well, all Go programs can't make TLS connections due to these attributes. Would like some sort of solution or workaround, most recently IRS.

[armon]

Wanted to chime in that we see this as broadly important to the Golang ecosystem at HashiCorp. Users and customers in the Federal space have more advanced uses of PKI that are standards compliant, but beyond the typical commercial use cases. Lack of support prevents those organizations from being able to build applications using Golang, and impacts vendors like us supporting them.

[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

[rolandshoemaker]

After some discussions it looks like, due to the current prevalence of this and the indications that FPKI is going all-in on it, we are probably going to need to support it. I think the only external API changes will need to be adding fields for the parsed extensions. Internally we'll implement the validation logic as specified in https://datatracker.ietf.org/doc/html/draft-ietf-lamps-x509-policy-graph, rather than the 5280 logic, since that is vulnerable to a trivial DoS.

I'll propose a concrete API change this week.

[rolandshoemaker]

Concrete proposal below. This copies the pattern we use for MaxPathLen/MaxPathLenZero, which is necessary because the zero value is meaningful. It feels like there should be a better way to do this, but it is not obvious to me. Ideally we'd use a pointer, but requiring users to create an int and then construct a pointer to it is not exactly user friendly.

We may not need the VerifyOptions change, but it allows users to specify the initial acceptable policy, which may be useful.

type Certificate {
	...

	// InhibitAnyPolicy and InhibitAnyPolicyZero indicate the preence and value
	// of the inhibitAnyPolicy extension.
	//
	// The value of InhibitAnyPolicy indicates the number of additional
	// certificates in the path after this certificate that may use the
	// anyPolicy policy OID to indicate a match with any other policy.
	//
	// When parsing a certificate, a positive non-zero InhibitAnyPolicy means
	// that the field was specified, -1 means it was unset, and
	// InhibitAnyPolicyZero being true mean that the field was explicitly set to
	// zero. The case of InhibitAnyPolicy==0 with InhibitAnyPolicyZero==false
	// should be treated equivalent to -1 (unset).
	InhibitAnyPolicy int
	// InhibitAnyPolicyZero indicates that InhibitAnyPolicy==0 should be
	// interpreted as an actual maximum path length of zero. Otherwise, that
	// combination is interpreted as InhibitAnyPolicy not being set.
	InhibitAnyPolicyZero bool

	// InhibitPolicyMapping and InhibitPolicyMappingZero indicate the presence
	// and value of the inhibitPolicyMapping field of the policyConstraints
	// extension.
	//
	// The value of InhibitPolicyMapping indicates the number of additional
	// certificates in the path after this certificate that may use policy
	// mapping.
	//
	// When parsing a certificate, a positive non-zero InhibitPolicyMapping
	// means that the field was specified, -1 means it was unset, and
	// InhibitPolicyMappingZero being true mean that the field was explicitly
	// set to zero. The case of InhibitPolicyMapping==0 with
	// InhibitPolicyMappingZero==false should be treated equivalent to -1
	// (unset).
	InhibitPolicyMapping int
	// InhibitPolicyMappingZero indicates that InhibitPolicyMapping==0 should be
	// interpreted as an actual maximum path length of zero. Otherwise, that
	// combination is interpreted as InhibitAnyPolicy not being set.
	InhibitPolicyMappingZero bool

	// RequireExplicitPolicy and RequireExplicitPolicyZero indicate the presence
	// and value of the requireExplicitPolicy field of the policyConstraints
	// extension.
	//
	// The value of RequireExplicitPolicy indicates the number of additional
	// certificates in the path after this certificate before an explicit policy
	// is required for the rest of the path. When an explicit policy is required,
	// each subsequent certificate in the path must contain a required policy OID,
	// or a policy OID which has been declared as equivalent through the policy
	// mapping extension.
	//
	// When parsing a certificate, a positive non-zero RequireExplicitPolicy
	// means that the field was specified, -1 means it was unset, and
	// RequireExplicitPolicyZero being true mean that the field was explicitly
	// set to zero. The case of RequireExplicitPolicy==0 with
	// RequireExplicitPolicyZero==false should be treated equivalent to -1
	// (unset).
	RequireExplicitPolicy int
	// RequireExplicitPolicyZero indicates that RequireExplicitPolicy==0 should be
	// interpreted as an actual maximum path length of zero. Otherwise, that
	// combination is interpreted as InhibitAnyPolicy not being set.
	RequireExplicitPolicyZero bool

	// PolicyMappings contains a list of policy mappings included in the certificate.
	PolicyMappings []PolicyMapping
}

// PolicyMapping represents a policy mapping entry in the policyMappings extension.
type PolicyMapping struct {
	// IssuerDomainPolicy indicates policy OID the issuing certificate considers
	// equivalent to the subjects SubjectDomainPolicy.
	IssuerDomainPolicy OID
	// SubjectDomainPolicy indicates policy OID the issuing certificate considers
	// equivalent to the issuers IssuerDomainPolicy.
	SubjectDomainPolicy OID
}

type VerifyOptions struct {
	...

	// CertificatePolicies specifies which certificate policy OIDs are acceptable.
	// If set, the policy graph is checked during path building, and it must
	// satisfy one of the provided policies.
	CertificatePolicies []OID
}

const(
	...

	// NoValidChains results when there are no valid chains to return.
	NoValidChains
)

[rsc]

This seems unfortunate but probably also the best we can do. Any thoughts @FiloSottile?

[FiloSottile]

Are these extensions ever used for non-zero values? If not, we could start with just the ...Zero bool fields. Answering this question might not be trivial, but if we're going to target some level of support of FPKI certificates (alas), we should try to get a large sample to do compatibility checks on, like we do with WebPKI certificates at times.

Also, if the use case we are targeting is supporting FPKI certificates for TLS, it feels like we could do without the VerifyOptions field, at least for now. I assume they still only use EKUs for designating server auth?

(What's type PolicyMapping for? It's not referenced by any other APIs.)

[rolandshoemaker]

Are these extensions ever used for non-zero values? If not, we could start with just the ...Zero bool fields. Answering this question might not be trivial, but if we're going to target some level of support of FPKI certificates (alas), we should try to get a large sample to do compatibility checks on, like we do with WebPKI certificates at times.

A good question, the answer to which is.... unclear. The (FPKI certificate policy)[https://www.idmanagement.gov/docs/fpki-x509-cert-policy-common.pdf] seems to indicate that inhibitPolicyMapping may be non zero, but InhibitAnyPolicy and requireExplicitPolicy should only ever be zero if set. That said, I'm having a slightly painful time actually trying to verify if that is true in practice. FPKI certificates are not particularly as well mapped in the same way web PKI ones are.

I'm happy to just leave out InhibitAnyPolicy and RequireExplicitPolicy, keeping just the Zero fields for the initial implementation. There is a question of what to do when we parse a certificate containing these extensions with non-zero values though. Probably we should fail since we couldn't safely do the verification? (or at least how we treat it internally would diverge from the what the user sees) 🤷

Also, if the use case we are targeting is supporting FPKI certificates for TLS, it feels like we could do without the VerifyOptions field, at least for now. I assume they still only use EKUs for designating server auth?

Yeah I was of two minds about this. Likely all we really want is to verify basic policy (or, really, just parse the extensions), but the FPKI has it's whole own set of weird policy OIDs beyond those specified in the web PKI (in the id-fpki-certpcy tree). It's possible that users may want, or be required, to verify compliance with a specific one of these.

I think at least for the first implementation of this we should just leave the verification stuff out, but I wanted to include it in the proposal so we can be ready to implement it if it is necessary.

(What's type PolicyMapping for? It's not referenced by any other APIs.)

Bah I managed to skip that extension in the proposal. Updated the comment.

[sgmiller]

Just to add a little bit of context, our interested parties definitely want to be able to do more than just parse the extensions. They expect to verify policy. How deeply, whether basic (likely) or being able to validate particular policies isn't super clear to us at this time, but we'd probably appreciate the ability to satisfy this so as to not to be caught off guard and to not need to try to workaround or interrupt Go development to be able to provide compliance in this space.

[aclements]

The next step on this is that @rolandshoemaker is planning to try implementing the API in #68484 (comment) to "see how painful it is."

[rolandshoemaker]

Implemented in CL 628616. It turns out to not be as terrible as I expected, other than the ton of new fields on Certificate.

In order to properly implement validation we needed three more fields on VerifyOptions:

	// InhibitPolicyMapping indicates if policy mapping should be allowed
	// during path validation.
	InhibitPolicyMapping bool

	// RequireExplicitPolicy indicates if the path contain a policy listed in
	// CertificatePolicies.
	RequireExplicitPolicy bool

	// InhibitAnyPolicy indicates if the anyPolicy policy should be
	// processed if present in a certificate being validated.
	InhibitAnyPolicy bool

Updated #68484 (comment) to reflect the final API.

[gopherbot]

Change https://go.dev/cl/628616 mentions this issue: crypto/x509: implement policy validation

[rolandshoemaker]

Amazingly we need one more API change. Updated #68484 (comment).

Since we filter chains post-building, we need a new InvalidReason const for the error return, since we may return no chains for a new reason.

const(
	...

	// NoValidChains results when there are no valid chains to return.
	NoValidChains
)

[FiloSottile]

Went back and forth in review, and looked for ways to reduce either the API surface or the attack surface of "regular" chain validation, and we came up pretty much short. This is a terrible feature of X.509, and if we need to support it, I think this is the best we can do.

[rolandshoemaker]

Re-opening since the proposal is still inflight. We landed this under the assumption it would be accepted after the freeze. If the committee decides not to accept the proposal we will revert the change.

[gopherbot]

Change https://go.dev/cl/618415 mentions this issue: crypto/x509: run a subset of the NIST PKI test suite

[aclements]

Went back and forth in review

Thanks for doing the exercise! @rolandshoemaker is our only expert in this stuff on the proposal committee, so I wanted to make sure we got a second expert opinion before moving forward with this.

[rsc]

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

The proposal details are in #68484 (comment).