crypto/x509: gate marshaling of Policies on a GODEBUG [freeze exception]

[mateusz834]

#60665 introduced a new OID type and a new field on the Certificate.

This change might cause a slight backwards compatibility breakage, it was not discussed there, so it might be worth discussing it here again.

Consider https://go.dev/play/p/Z92_1DOZJQi?v=gotip

func TestParsedCertificateAsTemplate(t *testing.T) {
	b, _ := pem.Decode([]byte(largeOIDPEM))
	if b == nil {
		t.Fatalf("couldn't decode test certificate")
	}
	template, err := x509.ParseCertificate(b.Bytes)
	if err != nil {
		t.Fatalf("ParseCertificate unexpected error: %v", err)
	}
	template.PublicKey = nil

	// Clear all PolicyIdentifiers from template.
	template.PolicyIdentifiers = nil

	newCertDER, err := x509.CreateCertificate(rand.Reader, template, template, rsaPrivateKey.Public(), rsaPrivateKey)
	if err != nil {
		t.Fatalf("CreateCertificate unexpected error: %v", err)
	}

	cert, err := x509.ParseCertificate(newCertDER)
	if err != nil {
		t.Fatalf("ParseCertificate unexpected error: %v", err)
	}

	if len(cert.PolicyIdentifiers) != 0 {
		t.Fatalf("PolicyIdentifiers field is not empty") // fails on gotip
	}
}

This does not fail on go 1.21, but on gotip it fails.
I am not sure whether we should be concerned about this. Not sure whether (and why) someone might be using a parsed certificate as a template, but who knows.

CC @rolandshoemaker

[mateusz834]

Also related: #63909, but for duplicates between Policies and PolicyIdentifiers.

[mateusz834]

This can be solved by introducing a new field on the Certificate, like UsePolicies:

type Certificate struct {
	PolicyIdentifiers []asn1.ObjectIdentifier

        // UsePolicies is used by [CreateCertificate], is signals that
        // [Certificate.Policies] field should be used over [Certificate.PolicyIdentifiers]
        UsePolicies bool
	Policies    []OID
}

This way we can also remove this check, introduced by CL 539297

go/src/crypto/x509/x509.go

Lines 1381 to 1384 in 5abae02

	// added is used to track OIDs which are duplicated in both Policies and PolicyIdentifiers
	// so they can be skipped. Note that this explicitly doesn't check for duplicate OIDs in
	// Policies or in PolicyIdentifiers themselves, as this would be considered breaking behavior.
	added := map[string]bool{}

[mknyszek]

CC @golang/security

Does anything need to be done here? Thanks.

[dmitshur]

This is currently a blocker for the RC 1, scheduled in about two weeks. Can someone from @golang/security take a look. Thanks.

[rolandshoemaker]

We're thinking about it, but don't have a good solution as of yet.

I'd rather not add a bool to use the new field, ideally we'd like to transition to using this new field and deprecating the old one. If we add a bool that indicates we should use the new field the migration plan becomes very complicated since we cannot reasonably flip the default while retaining any real meaning for the bool (essentially we just end up carrying a bool field with zero meaning that either has no impact regardless of value, or we invert the meaning, which is breaking for existing code that uses the bool).

One solution would be to have an enum field that selects which policies field to use, and we can flip the default when nil when we deprecate PolicyIdentifiers. I cannot think of a good name for it though.

Possibly we should just revert the marshaling behavior for Policies, only parsing into it for 1.22 (which fixes 90% of the problems people were running into with certificates in the wild being used for TLS containing these OIDs), and then address the marshaling behavior in 1.23.

[mateusz834]

One solution would be to have an enum field that selects which policies field to use, and we can flip the default when nil when we deprecate PolicyIdentifiers. I cannot think of a good name for it though.

It sounds like this would also cause the same issue.

[rolandshoemaker]

Unlike a bool, the enum can have a meaningful zero value (of default to whatever the runtime decides when unpopulated), whereas the zero value of a bool is the negative case (i.e. don't use Policies, use PolicyIdentifiers).

i.e.

type WhichPoliciesField int

const (
	UsePolicyIdentifiers WhichPoliciesField = iota + 1
	UsePolicies
)

type Certificate struct {
	...

	// WhichPoliciesField defines which of the two policy OID fields should be used
	// during marshaling. If UsePolicyIdentifiers is used, the PolicyIdentifier field
	// is used. If UsePolicies is used, the Policies field is used. If the field is
	// unset, UsePolicyIdentifiers is used by default. The default value may change
	// in the future.
	WhichPoliciesField WhichPoliciesField
}

[rolandshoemaker]

Oh sorry, when you say the same problem, do you mean the same marshaling problem, as in it would result in not nil-ing the correct field? I think that is addressed by the deprecating. We'd likely want to deprecate the field in one release, and then change the default of the enum in the next release, giving people long enough to migrate safely.

[mateusz834]

Oh sorry, when you say the same problem, do you mean the same marshaling problem, as in it would result in not nil-ing the correct field? I think that is addressed by the deprecating. We'd likely want to deprecate the field in one release, and then change the default of the enum in the next release, giving people long enough to migrate safely.

Exactly, but i would still consider this a compatibility breaking change. Also this issue does not only apply to nil-ing the field, but also filtering out some specific OIDs. When the default of WhichPoliciesField is going to be changed, then the test from #64248 (comment) would still fail, right?

[rolandshoemaker]

Yup that is still an issue. I think any approach we take here (that migrates from one field to another) is going to exhibit a degree of this problem.

@rsc suggested taking the GODEBUG approach, which I think is a viable solution. We'd gate which field is marshaled behind a GODEBUG, i.e. x509marshalpolicies, and would only use one of the two fields, depending on the value of the GODEBUG. We can then change the default value at some point in the future, and anything using the go directive in their go.mod file will retain the old behavior, and will only have to change code when they update the directive (which is the acceptable breakage path for GODEBUGs).

I'll prepare two changes, one which removes the dual marshaling behavior, and another which gates which field is marshaled using a GODEBUG. We can submit the first one to get back to the "safe" behavior before 1.22RC1, and if we get this through the proposal/release-exception review in time for the 1.22RC1, we can submit that.

[mateusz834]

But once we make the Policies the default, then all code that sets the PolicyIndetifiers field would break (all CreateCertificate templates that do not come from a parsed der certificate). I don't think that this is a good direction

I understand that this will be guarded by the go directive, but we are going to break the most common approach (template does not come from ParseCertificate)