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: sort order of 'SET of' components during Marshal #38228

Closed
wants to merge 2 commits into from

Conversation

rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 3, 2020

Per X690 Section 11.6 sort the order of SET of components when generating
DER. This CL makes no changes to Unmarshal, meaning unordered components
will still be accepted, and won't be re-ordered during parsing.

In order to sort the components a new encoder, setEncoder, which is similar
to multiEncoder is added. The functional difference is that setEncoder
encodes each component to a [][]byte, sorts the slice using a sort.Sort
interface, and then writes it out to the destination slice. The ordering
matches the output of OpenSSL.

Fixes #24254

Per X690 Section 11.6 sort the order of SET of components when generating
DER. This CL makes no changes to Unmarshal, meaning unordered components
will still be accepted, and won't be re-ordered during parsing.

In order to sort the components a new encoder, setEncoder, which is similar
to multiEncoder is added. The functional difference is that setEncoder
encodes each component to a [][]byte, sorts the slice using a sort.Sort
interface, and then writes it out to the destination slice. The ordering
matches the output of OpenSSL.

Fixes golang#24254

Change-Id: Iff4560f0b8c2dce5aae616ba30226f39c10b972e
@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Apr 3, 2020
@gopherbot
Copy link

This PR (HEAD: 331ab1f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226984 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Roland Shoemaker:

Patch Set 2:

Not entirely sure who the best reviewer for this, assuming it's your area Filippo (Does agl still do golang reviews?).


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Filippo Valsorda:

Patch Set 2: Run-TryBot+1

(4 comments)

Thank you Roland, I had been putting this off for too long and this is an elegant fix.


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=f51379f2


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 2: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

Change-Id: Ib6a8fb2f9b20dbff5f24b18cc5a217d95066a98f
@gopherbot
Copy link

This PR (HEAD: e52fc43) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/226984 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Roland Shoemaker:

Patch Set 4:

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Josh Bleecher Snyder:

Patch Set 4:

If this is going to go into 1.15, it has to happen very soon.


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Filippo Valsorda:

Patch Set 4: Run-TryBot+1 Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ca1021d6


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 4: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/226984.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request May 8, 2020
Per X690 Section 11.6 sort the order of SET of components when generating
DER. This CL makes no changes to Unmarshal, meaning unordered components
will still be accepted, and won't be re-ordered during parsing.

In order to sort the components a new encoder, setEncoder, which is similar
to multiEncoder is added. The functional difference is that setEncoder
encodes each component to a [][]byte, sorts the slice using a sort.Sort
interface, and then writes it out to the destination slice. The ordering
matches the output of OpenSSL.

Fixes #24254

Change-Id: Iff4560f0b8c2dce5aae616ba30226f39c10b972e
GitHub-Last-Rev: e52fc43
GitHub-Pull-Request: #38228
Reviewed-on: https://go-review.googlesource.com/c/go/+/226984
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

This PR is being closed because golang.org/cl/226984 has been merged.

@defacto64
Copy link

Hello,
still related to this topic, I would like to draw attention to the attached CSR (CertificateSigning Request).

problematic-csr.txt

Its self-signature is OK when verified with GoLang, OpenSSL, and other tools, but KO with GnuTLS and BouncyCastle.

I understand that GoLang (like several other tools and run-times, to be honest) omits the DER-encoding pass prescribed by RFC2986, when verifying the CSR self-signature, wrongly assuming that the signed data are already DER-encoded -- which is not necessarily the case, as in the attached example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto/x509: multi-value RDN sequence is not properly DER-ordered
4 participants