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

encoding/asn1: add explicit support for ASN.1 NULL #19446

Closed
andrewmbenton opened this issue Mar 7, 2017 · 5 comments
Closed

encoding/asn1: add explicit support for ASN.1 NULL #19446

andrewmbenton opened this issue Mar 7, 2017 · 5 comments
Assignees
Milestone

Comments

@andrewmbenton
Copy link
Contributor

@andrewmbenton andrewmbenton commented Mar 7, 2017

I've been working on cleaning this up but wanted to start a discussion here about the proper approach before I get too deep.

There are a couple of places in crypto/x509/x509.go that use hardcoded byte slices representing the DER-encoding of ASN.1 NULL. See x509.go#L421 and x509.go#L932.

Additionally, there are four places in crypto/x509/x509.go where identical asn1.RawValue structs representing the unmarshaled ASN.1 NULL type are used. See x509.go#L72 and x509.go#L359 for instance.

My thought was to consolidate these usages around two new funcs in the asn1 package that would return the desired explicit values, so signatures would be:

func Null() RawValue
func NullBytes() []byte

I think this approach makes sense and I already have it written up with tests, but I'm interested in feedback. I was trying to figure a way to create an asn1.Null type as an alias of asn1.RawValue but it didn't seem to make much sense since the NULL type in practice can only ever have one value.

Are there other approaches that make more sense?

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
@agl
Copy link
Contributor

@agl agl commented Mar 24, 2017

This seems like a reasonable cleanup. I'm not too keen on adding more to encoding/asn1 (which I consider to have been a mistake), but this seems small enough.

@andrewmbenton
Copy link
Contributor Author

@andrewmbenton andrewmbenton commented Mar 25, 2017

I'll submit the very simple patch I have and reference this discussion.

EDIT: Patch submitted here: https://go-review.googlesource.com/38660

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2017

CL https://golang.org/cl/38660 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 24, 2017
…NULL

There were a number of places in crypto/x509 that used hardcoded
representations of the ASN.1 NULL type, in both byte slice and
RawValue struct forms. This change adds two new exported vars to
the asn1 package for working with ASN.1 NULL in both its forms, and
converts all usages from the x509 package.

In addition, tests were added to exercise Marshal and Unmarshal on
both vars.

See #19446 for discussion.

Change-Id: I63dbd0835841ccbc810bd6ec794360a84e933f1e
Reviewed-on: https://go-review.googlesource.com/38660
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Jun 5, 2017

The commit seems to have forgotten the magic word Fixes.

@rsc rsc closed this Jun 5, 2017
@andrewmbenton
Copy link
Contributor Author

@andrewmbenton andrewmbenton commented Jun 5, 2017

@golang golang locked and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.