x/crypto/cryptobyte: ReadOptionalASN1Boolean doesn't work as expected

[rolandshoemaker]

ReadOptionalASN1Boolean doesn't work like the other ReadOptionalASN1* methods, nor does it work in the way it seems to be intended.

ReadOptionalASN1Boolean seems to expect to support reading a example BOOLEAN OPTIONAL field that does not use context-specific tags, unlike the other ReadOptionalASN1* methods, but will in fact only work when reading a structure containing two subsequent BOOLEAN fields, i.e.

example-a BOOLEAN OPTIONAL
example-b BOOLEAN

as it continues reading from the source cryptobyte.String, rather than the optional field.

There are two possible solutions for this. (1) fix the method to support optional fields without context-specific tags (which are rather rare), or (2) fix the method to support optional fields with context-specific tags (which matches the other ReadOptionalASN1* methods). I think (2) makes the most sense since it aligns with the other methods in the package and is more likely to be used in the real world, but it does require breaking the existing API by adding an argument for the expected context-specific tag.

As far as I can tell there are no uses of this method in the standard library, and after a quick search of GitHub I was unable to find any third-party packages using this (if any are, they are likely quite broken since this method should never really work).

[gopherbot]

Change https://golang.org/cl/274242 mentions this issue: cryptobyte: fix ReadOptionalASN1Boolean

[FiloSottile]

Indeed, ReadOptionalASN1Boolean is completely broken at the moment, and it seems to be fully unused.

I support fixing it before tagging v1 of x/crypto, despite considering x/crypto stable already, because 1) nothing will break as far as I can tell and 2) anything that breaks was very broken already.

The proposal is to change it to this

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

Note that adding the tag asn1.Tag argument and breaking the API is not actually necessary to fix the bug, but 1) it makes ReadOptionalASN1Boolean actually useful since OPTIONALs are commonly explicitly tagged, 2) it makes it consistent with other ReadOptionalASN1, and 3) it prevents confusion with the previous ReadOptionalASN1Boolean implementation.

Over to @golang/proposal-review since it's an API change.

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

Given that the function fails today (does not do what it should) and cannot do what it should without a new argument and for that reason appears to be completely unused, it seems OK to make an API breaking change here.

We did this in the past for a function in syscall that was impossible to call because it took an argument of unexported type. This is not quite that strictly impossible but still seems reasonable.

[rsc]

Have all remaining concerns about this proposal been addressed?

[rolandshoemaker]

I believe so.

[rsc]

I still had some hesitation about making a breaking change, so I scanned the latest version of each module available on the module proxy for ReadOptionalASN1Boolean (about a million modules). I found 779 copies of the code but zero calls to it. This does seem to be the exceptional case where we can reasonably redefine the existing API.

[rsc]

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

In package cryptobyte, change the (*String).ReadOptionalASN1Boolean method signature to:

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

This adds a new “tag” parameter so that the code can tell whether the optional boolean is present or not. The current function signature is unfixably broken and impossible to use correctly.

Even unfixably broken and impossible to use correctly is not enough to merit an actual breaking API change. We also scanned all public Go code in the module proxy and found not a single use of this method, suggesting that it really is safe to redefine.

(It is not surprising no one uses it, since if they did, the code wouldn’t work! But that doesn’t always stop people. In this case, the method is also obscure enough that we seem to have gotten lucky.)

[rsc]

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

In package cryptobyte, change the (*String).ReadOptionalASN1Boolean method signature to:

(s *String) ReadOptionalASN1Boolean(out *bool, tag asn1.Tag, defaultValue bool) bool

This adds a new “tag” parameter so that the code can tell whether the optional boolean is present or not. The current function signature is unfixably broken and impossible to use correctly.

Even unfixably broken and impossible to use correctly is not enough to merit an actual breaking API change. We also scanned all public Go code in the module proxy and found not a single use of this method, suggesting that it really is safe to redefine.

(It is not surprising no one uses it, since if they did, the code wouldn’t work! But that doesn’t always stop people. In this case, the method is also obscure enough that we seem to have gotten lucky.)