crypto/x509: make OID have text, binary marshal methods

[rsc]

Essentially all types in x509 are all exported data, making them easy to marshal and unmarshal. #65633 points out that the new OID type is not, and adding it to x509.Certificate made that type not marshalable either. This is probably something we should fix, as I noted there.

Per #10275 (comment) and the docs at the top of package encoding, we can still add marshal/unmarshal methods to OID, since right now it is unusable with encoders.

Probably we should add both the text and binary forms together, with *OID receivers for unmarshal and OID receivers for marshal. Unless the text is too onerous to generate.

[rsc]

Over on #65633 @tycho pointed out that not having these methods is breaking Mattermost, which is more breakage than I realized. It seems like our choices are (1) remove new-in-Go1.22 x509.Policies field in a point release, potentially breaking other programs that have started using the field; (2) add the OID methods in a point release; or (3) leave Mattermost and similarly affected programs broken until Go 1.23.

Of these three bad choices, it seems like (2) is the least bad. In general we still want to avoid adding methods in point releases, but this does seem like a critical issue without a workaround, per the release policy, and now at least people can write 'go 1.22.1' in their go.mod files.

@adonovan an interesting case for your new vet checker.

Edit: @mateusz834 points out choice (4): After accepting the proposal and committing to adding these methods in Go 1.23, special-case x509.OID inside gob in a Go 1.22 point release to behave as though those methods already existed (they will just use the single unexported []byte field directly, so that's easy). That fixes the breakage in a Go 1.22 point release without needing to add methods until the Go 1.23 release.

[mateusz834]

@rsc we can also patch encoding/gob for go 1.22 point release and for go 1.23 add proper Marshal methods.

[VictorLowther]

@mateusz834 For my purposes having x509.OID implement BinaryMarshaler/BinaryUnmarshaler will suffice. I don't think a change in encoding/gob is called for at this point.

[rsc]

@VictorLowther, for your purposes, patching Go1.22 gob to special-case x509.OID to behave like it will in Go 1.23 would also suffice, right? It doesn't solve other uses of binarymarshal, but it does somewhat cut the Gordian knot I described above.

[mateusz834]

@VictorLowther I mean to hardcode the x509.OID type in encoding/gob and encode and decode it properly, so that we don't have to add APIs in patch release.

[VictorLowther]

@rsc As long as encoding/gob can handle passing an x509.Certificate in between something compiled with Go 1.22.x and Go 1.21.x in both directions, I think my needs will be met. Having x509.OID implement Binary(Unm|M)arshaler seemed like the most straightforward solution that doesn't involve time travel (and will fix it for json/xml/non-stdlib codecs), but if there are policy reasons that make it tricky I can be selfish and use a magical encoding/gob.

[rsc]

For the record, json/xml would only be fixed by TextMarshaler; they ignore BinaryMarshaler.

[mateusz834]

@rsc But at least the json encoder ignores that field without any error: https://go.dev/play/p/7cPWKSJe9ff

[VictorLowther]

@mateusz834 Having encoding/gob ignore things in a similar fashion would probably also be acceptable -- I will need to add code to detect when x509.Certificate.Policies is nil or empty after decoding it and arrange to get it filled out properly to handle the 1.21 -> 1.22 migration anyways (probably by reparsing the whole cert from the Raw field, but whatever), since I don't think the code in the x509 package will do it for me.

[rsc]

I spoke to @robpike about this yesterday. We think the simplest, least invasive change for Go 1.22.x would be to change gob to look for the x509.Certificate type and have it skip over the Policies field entirely. That seems like it would fix Mattermost and other affected users, and then we can proceed with new methods on the normal timeline and then remove the gob change.

[gopherbot]

Change https://go.dev/cl/571095 mentions this issue: encoding/gob: make x509.Certificate marshalable again

[rsc]

This proposal is no longer considering adding new methods in a point release. The proposal is to add, for Go 1.23, these methods

func (OID) MarshalText
func (OID) MarshalBinary
func (*OID) UnmarshalText
func (*OID) UnmarshalBinary

(whatever the right signatures are for the interfaces).

The text form is the dotted decimal that the String method already uses.

Also if we are going to have an UnmarshalString method, then we probably should add ParseOID rather than have people use

var oid OID
oid.UnmarshalString(s)

Does that (four methods plus ParseOID) sound reasonable to people?

[gopherbot]

Change https://go.dev/cl/571715 mentions this issue: [release-branch.go1.22] encoding/gob: make x509.Certificate marshalable again

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

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

The proposal is to add

func (OID) MarshalText
func (OID) MarshalBinary
func (*OID) UnmarshalText
func (*OID) UnmarshalBinary
func ParseOID(string) (OID, error)

[mateusz834]

@rolandshoemaker are you working on this? I would like to make a CL for this once this gets accepted.

[rolandshoemaker]

I haven't started looking at it yet, feel free to send a CL.

[gopherbot]

Change https://go.dev/cl/575295 mentions this issue: crypto/x509: add text and binary marshal methods to OID

[rsc]

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

The proposal is to add

func (OID) MarshalText
func (OID) MarshalBinary
func (*OID) UnmarshalText
func (*OID) UnmarshalBinary
func ParseOID(string) (OID, error)