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: make OID have text, binary marshal methods #66249

Open
rsc opened this issue Mar 11, 2024 · 19 comments · May be fixed by #66599
Open

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

rsc opened this issue Mar 11, 2024 · 19 comments · May be fixed by #66599

Comments

@rsc
Copy link
Contributor

rsc commented Mar 11, 2024

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
Copy link
Contributor Author

rsc commented Mar 11, 2024

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
Copy link
Member

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

@VictorLowther
Copy link

@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
Copy link
Contributor Author

rsc commented Mar 11, 2024

@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
Copy link
Member

@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
Copy link

@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
Copy link
Contributor Author

rsc commented Mar 11, 2024

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

@mateusz834
Copy link
Member

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

@VictorLowther
Copy link

@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
Copy link
Contributor Author

rsc commented Mar 12, 2024

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
Copy link

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

@rsc
Copy link
Contributor Author

rsc commented Mar 13, 2024

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 pushed a commit that referenced this issue Mar 14, 2024
The OID type is not exported data like most of the other x509 structs.
Using it in x509.Certificate made Certificate not gob-compatible anymore,
which breaks real-world code. As a temporary fix, make gob ignore
that field, making it work as well as it did in Go 1.21.

For Go 1.23, we anticipate adding a proper fix and removing the gob
workaround. See #65633 and #66249 for more details.

For #66249.
Fixes #65633.

Change-Id: Idd1431d15063b3009e15d0565cd3120b9fa13f61
Reviewed-on: https://go-review.googlesource.com/c/go/+/571095
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@gopherbot
Copy link

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

@rsc
Copy link
Contributor Author

rsc commented Mar 15, 2024

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
Copy link
Contributor Author

rsc commented Mar 27, 2024

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)

gopherbot pushed a commit that referenced this issue Mar 27, 2024
…le again

The OID type is not exported data like most of the other x509 structs.
Using it in x509.Certificate made Certificate not gob-compatible anymore,
which breaks real-world code. As a temporary fix, make gob ignore
that field, making it work as well as it did in Go 1.21.

For Go 1.23, we anticipate adding a proper fix and removing the gob
workaround. See #65633 and #66249 for more details.

For #66249.
For #65633.
Fixes #66273.

Change-Id: Idd1431d15063b3009e15d0565cd3120b9fa13f61
Reviewed-on: https://go-review.googlesource.com/c/go/+/571095
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/571715
Reviewed-by: David Chase <drchase@google.com>
@mateusz834
Copy link
Member

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

@rolandshoemaker
Copy link
Member

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

@mateusz834 mateusz834 self-assigned this Mar 29, 2024
@gopherbot
Copy link

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

@rsc
Copy link
Contributor Author

rsc commented Apr 4, 2024

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)

@rsc rsc changed the title proposal: crypto/x509: make OID have text, binary marshal methods crypto/x509: make OID have text, binary marshal methods Apr 4, 2024
@rsc rsc removed this from the Proposal milestone Apr 4, 2024
@rsc rsc added this to the Backlog milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

5 participants