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: unmarshal of sequence of oid into slice of RawValue #17321

Closed
thsnr opened this issue Oct 3, 2016 · 10 comments
Closed

encoding/asn1: unmarshal of sequence of oid into slice of RawValue #17321

thsnr opened this issue Oct 3, 2016 · 10 comments

Comments

@thsnr
Copy link

@thsnr thsnr commented Oct 3, 2016

What version of Go are you using (go version)?

go version go1.7.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOOS="linux"
GOARCH="amd64"

What did you do?

Trying to ASN.1 unmarshal a sequence of object identifiers into a slice of asn1.RawValue:
https://play.golang.org/p/gWzGulJAow

What did you expect to see?

Expected the variable bad to contain a single asn1.RawValue element with the raw encoding of the object identifier.

What did you see instead?

asn1: structure error: sequence tag mismatch

@bradfitz bradfitz changed the title ASN.1 unmarshal of sequence of oid into slice of RawValue encoding/asn1: unmarshal of sequence of oid into slice of RawValue Oct 3, 2016
@bradfitz bradfitz added this to the Unplanned milestone Oct 3, 2016
@jcmturner
Copy link

@jcmturner jcmturner commented Jan 21, 2017

Hi, Is there any update on this issue as I'm also having a problem where I need to unmarshal into an []asn1.RawValue

Thanks

@m4ns0ur
Copy link
Contributor

@m4ns0ur m4ns0ur commented Nov 28, 2017

@thsnr / @jcmturner

func main() {
	var good asn1.RawValue
	asn1.Unmarshal([]byte(der), &good)
	fmt.Println(good)
}
@thsnr
Copy link
Author

@thsnr thsnr commented Dec 1, 2017

var good asn1.RawValue

This results in good containing the entire ASN.1 SEQUENCE. We wish to unmarshal the SEQUENCE, just not individual elements in it.

@m4ns0ur
Copy link
Contributor

@m4ns0ur m4ns0ur commented Dec 1, 2017

@thsnr the struct has un-marshaled SEQUENCE (tag/value):

func main() {
	var good asn1.RawValue
	asn1.Unmarshal([]byte(der), &good)

        fmt.Println(good.Tag)
        fmt.Println(good.Bytes)
}

// 16
// [6 1 42]

so what are you expecting to get?

@thsnr
Copy link
Author

@thsnr thsnr commented Dec 12, 2017

I expect to get a slice of asn1.RawValues with a single element that has Tag 6 and Bytes 42.

Perhaps a Playground with a sequence of two elements gives a better example:
https://play.golang.org/p/QcF5Fw9LvG

Here I would expect to get a slice with two elements, the asn1.RawValue for OBJECT IDENTIFIER {1 2} and the asn1.RawValue for OBJECT IDENTIFIER {3 4}.

@m4ns0ur
Copy link
Contributor

@m4ns0ur m4ns0ur commented Dec 13, 2017

@thsnr I see your point now.

From godoc:

An ASN.1 SEQUENCE OF x or SET OF x can be written to a slice if an x can be written to the slice's element type.

So you can easily have a slice of OBJECT IDENTIFIER ([]asn1.ObjectIdentifier) like this:
https://play.golang.org/p/77B7jLWeRb

But about asn1.RawValue it's kind of confusing for Unmarshal, while it points to SEQUENCE itself:
https://play.golang.org/p/Zngp7rc4o3

Using []asn1.RawValue means, having SEQUENCE of SEQUENCEs (I changed your DER):
https://play.golang.org/p/WLf8eQIywc

IMO it's a correct behavior. I leave it to @bradfitz.

@thsnr
Copy link
Author

@thsnr thsnr commented Dec 14, 2017

tl;dr: We actually worked around this even before the bug was reported, so I am fine with leaving the current behavior, but it seems to go against the documentation.

I will add a little background. While in any sane situation I would just use []asn1.ObjectIdenfitier, we were implementing Cryptographic Message Syntax (CMS). One of the types there is Attribute (https://tools.ietf.org/html/rfc5652#page-14):

Attribute ::= SEQUENCE {
attrType OBJECT IDENTIFIER,
attrValues SET OF AttributeValue }

AttributeValue ::= ANY

So we have to unmarshal a SET OF ANY and one of the possible AttributeValues, Content Type (https://tools.ietf.org/html/rfc5652#section-11.1), was an OBJECT IDENTIFIER, which failed.

We worked around this back last year before this issue was reported (also by just unmarshaling into a single asn1.RawValue and going from there), but I still thought to report it as it seems to go against the same godoc snippet that @m4ns0ur quoted:

An ASN.1 SEQUENCE OF x or SET OF x can be written to a slice if an x can be written to the slice's element type.

Since we can unmarshal an OBJECT IDENTIFIER into an asn1.RawValue, this sentence implies that we can unmarshal SEQUENCE OF OBJECT IDENTIFIER into []asn1.RawValue.

Also as @m4ns0ur noted, unmarshaling a SEQUENCE OF SEQUENCE into []asn1.RawValue seems to work: we use it for unmarshaling a SET OF X.509 Certificates so that their FullBytes can be passed to x509.ParseCertificate. So this documentation sentence with regards to asn1.RawValue sometimes holds and sometimes not. We have not tested any other non-SEQUENCE types.

As I said, we have already worked around this situation, so it fine by me to leave the behavior as-is. :)

@m4ns0ur
Copy link
Contributor

@m4ns0ur m4ns0ur commented Dec 14, 2017

@thsnr sounds reasonable, let me look more.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 14, 2017

Change https://golang.org/cl/84095 mentions this issue: testing: add testing on Unmarshal slice of RawValue

@m4ns0ur
Copy link
Contributor

@m4ns0ur m4ns0ur commented Dec 14, 2017

@thsnr so it's already fixed at a82ee9c, and you're gonna have it in Go1.10.
Just added a test case too. Thanks for reporting.

@thsnr thsnr closed this Dec 6, 2018
d1egoaz added a commit to Shopify/sarama that referenced this issue Aug 21, 2019
fixes #1438

`jcmturner/gokrb5` needs to use the `jcmturner/gofork` in order to have
some workarounds about some stuff not implemented or not working as
expected.

For the specific case of github.com/jcmturner/gofork/encoding/asn1 there
is no reason right now to keep using `jcmturner/gofork` as the issue
golang/go#17321 was solved/closed in Dec 15, 2016 and released in go
1.10

`jcmturner/gokrb5` still uses internally `jcmturner/gofork`, it seems
the `gofork` `asn1` part could be removed and use the standar library,
but that will need some more work and PRs on the `jcmturner/gokrb5`
side.
@golang golang locked and limited conversation to collaborators Dec 6, 2019
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.