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)

[FiloSottile]

I'm not sure ParseCertificate -> CreateCertificate loops are a tenable backwards compatibility target. Anything new we start parsing from a certificate might end up serialized.

This is arguably even documented in CreateCertificate (emphasis mine):

The following members of template are currently used:

If we did want to mitigate this, one option would be the Extensions / ExtraExtensions pattern, where parsing populates the former and marshaling uses the latter, but that's already complex enough without the legacy Policies field interacting.

I like the suggestion of making it a GODEBUG. I think that'd be the best solution.

[rolandshoemaker]

Re-purposing this issue to propose the GODEBUG behavior, hopefully we can push this through before 1.22RC1, otherwise we'll be stuck with this until 1.23.

[gopherbot]

Change https://go.dev/cl/546916 mentions this issue: crypto/x509: gate Policies marshaling with GODEBUG

[gopherbot]

Change https://go.dev/cl/546915 mentions this issue: crypto/x509: revert Policies marshaling behavior

[mateusz834]

@rolandshoemaker Does this proposal also include adding a new field? #64248 (comment)

Will the GODEBUG be enabled by default when the go directive is >= go1.22? Or is it going to be opt-in?

[rolandshoemaker]

It won't add a new field, the GODEBUG alone chooses which field will be marshaled (either PolicyIdentifiers or Policies). The current implementation (https://go.dev/cl/546916) doesn't change the value based on the Go version, at some point in the future (maybe 1.23?) we will make that change so that we give people a while to understand how the new field works.

[mateusz834]

at some point in the future (maybe 1.23?) we will make that change

Have we ever done something like that with GODEBUGs? Most GODEBUGs have negligible effect for users. This would make all code that uses PolicyIdentifiers broken (only guarded by a go directive) and as a consequence every user of PolicyIdentifiers would have to migrate to using Policies.

[rolandshoemaker]

GODEBUGs being used this way is a somewhat new idea, so there aren't a huge number of examples, but we discussed this idea in the proposal committee and considered it acceptable. There was at least on previous example of a type system change being made this way (I cannot for the life of me remember the exact issue though).

I expect flipping the default value will require going through the proposal committee again when we decide to make that change though, so we'll have another change to decide if we actually find this acceptable at that point.

[rolandshoemaker]

(I cannot for the life of me remember the exact issue though).

Probably it was #63223.

[rsc]

The GODEBUG for go/types's alias transition is #63223.

[rsc]

We've already added the new Policies field here:

type Certificate struct {
	PolicyIdentifiers []asn1.ObjectIdentifier
	Policies    []OID
}

Code right now unmarshals into both PolicyIdentifiers and Policies, but we only Marshal from PolicyIdentifiers for backwards compatibility, because old code does not know to update Policies.

The proposal is to add GODEBUG=x509marshalpolicies=0 (default) to mean the current behavior, and =1 means marshal the Policies field instead of PolicyIdentifiers. Go 1.22 would keep the =0 default, but then Go 1.23 would change to =1. At the point where Go 1.23 is released, Go 1.21 is unsupported, so it is OK to assume the existence of Policies in any code.

This seems fine.

[mateusz834]

@rsc There is no UsePolicies field now.

[rsc]

@mateusz834 Thanks, removed UsePolicies. I was referring only to Policies.

[rsc]

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

Add GODEBUG=x509marshalpolicies=0 (default) to mean the current behavior (marshal PolicyIdentifiers not Policies), and =1 means marshal the Policies field instead of PolicyIdentifiers. Go 1.22 would keep the =0 default, but then Go 1.23 would change to =1. At the moment where Go 1.23 is released, Go 1.21 is unsupported, so it will be OK to assume the existence of Policies in any code that needs updating (specifically by clearing Policies when it clears PolicyIdentifiers).

[mateusz834]

I wonder whether it would be better to change how x509marshalpolicies=1 works a bit, so that we don't unnecessarily break code that currently sets only the PolicyIdentifiers field.

When x509marshalpolicies=0, marshal only the PolicyIdentifiers field.
When x509marshalpolicies=1, marshal PolicyIdentifiers when len(Policies) == 0 otherwise marshal only the Policies field.

This way we don't break code that does something like:

template := x509.Certificate{
    // (....)
    PolicyIdentifiers: []asn1.ObjectIdentifier{[]int{1, 2, 3}},
}
x509.CreateCertificate(rand.Reader, &template, caCert, pub, caPriv)

EDIT: This has a drawback that if someone uses a parsed certificate as a template and wants to remove all x509 Policy Identifiers, then it is required to clear both PolicyIdentifiers and Policies, so it might be a bit annoying. Additionally, someone might filter out an element from the Policies (especially the last element), potentially causing the marshaling of the PolicyIdentifiers field. Using Policies == nil instead of len(Policies) == 0 (for x509marshalpolicies=1) might improve the situation a bit. Considering that using a parsed certificate as a template is not super common, maybe it is worth it?

[rolandshoemaker]

I think having the field be marshaled depending on certain constraints ends up being a lot harder to reason about than just explicitly picking one field or the other. RC1 happens on the 12th, so we need to either decide if this is acceptable now, or push any change until 1.23.

That said, since this is a GODEBUG feature, if we see people significantly misusing this functionality we can decide not to flip the default value, and revisit the design of the feature with those concerns in mind.

cc @golang/release can we get a freeze exception for this? Understandable if this is a bit too late though. https://go.dev/cl/546916 implements the functionality described in this comment.

[prattmic]

From discussion with @dmitshur and @cagedmantis, a freeze exception is accepted.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Add GODEBUG=x509marshalpolicies=0 (default) to mean the current behavior (marshal PolicyIdentifiers not Policies), and =1 means marshal the Policies field instead of PolicyIdentifiers. Go 1.22 would keep the =0 default, but then Go 1.23 would change to =1. At the moment where Go 1.23 is released, Go 1.21 is unsupported, so it will be OK to assume the existence of Policies in any code that needs updating (specifically by clearing Policies when it clears PolicyIdentifiers).

[mateusz834]

Openned #67620, so that we don't forget about this.