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

Adopt cryptobyte's updated ReadOptionalASN1Boolean #7225

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Dec 19, 2023

Version 0.17.0 of the golang.org/x/crypto/cryptobyte/asn1 package contains https://go-review.googlesource.com/c/crypto/+/274242, which fixes golang/go#43019. This should enable us to use this library method instead of using our own custom lints.ReadOptionalASN1BooleanWithTag.

@aarongable aarongable requested a review from a team as a code owner December 19, 2023 23:22
@aarongable aarongable requested review from pgporada and removed request for a team December 19, 2023 23:22
@aarongable
Copy link
Contributor Author

Ah. This doesn't work because cryptobytes ReadOptionalASN1Boolean method only works on explicitly tagged optional booleans. It even says this (although its easy to miss) in the documentation (emphasis added):

ReadOptionalASN1Boolean attempts to read an optional ASN.1 BOOLEAN explicitly tagged with tag into out and advances

This means that it expects

onlyContainsUserCerts  [1]  BOOLEAN DEFAULT FALSE,

with value "true" to be encoded as

81 03 01 01 FF

that is: context-specific tag 1 with length 3, explicitly wrapping global tag 1 (boolean) with length 1, and value TRUE.

But the IssuingDistributionPoint extension expects implicit encoding. Where its defined in both X.509 itself and in RFC 5280, it's in an ASN.1 module that sets DEFINITIONS IMPLICIT TAGS, so all optional (or default) fields are expected to be encoded implicitly unless stated otherwise. The definition of IssuingDistributionPoint doesn't state otherwise.

This means that ReadOptionalASN1Boolean expects the inner value of the context-specific tag to be three bytes (another tag, length, value triplet), but it's actually only one byte (just the value from the implicit tag it already unwrapped), and so it reports that "the read was unsuccessful" (i.e. it returns false).

I'm going to close this PR. We can't simply use this nicely-updated library method. I think the action items from here are:

  1. Consider renaming our "ReadOptionalASN1BooleanWithTag" to "ReadImplicitOptionalASN1Boolean", since the implicit/explicit distinction is now more salient that the "with tag" distinction.
  2. File a bug against cryptobyte to consider exposing Implicit versions of these read functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant