[go] crypto/x509: parse all names in an RDN.

[Adam Langley (Gerrit)]

Adam Langley uploaded a change:
https://go-review.googlesource.com/30810

crypto/x509: parse all names in an RDN.

The Subject and Issuer names in a certificate look like they should be a
list of key-value pairs. However, they're actually a list of lists of
key-value pairs. Previously we only looked at the first element of each
sublist and the vast majority of certificates only have one element per
sublist.

However, it's possible to have multiple elements and some 360
certificates from the “Pilot” log are so constructed.

This change causes all elements of the sublists to be processed.

Fixes #16836.

Change-Id: Ie0a5159135b08226ec517fcf251aa17aada37857
---
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509_test.go
2 files changed, 81 insertions(+), 27 deletions(-)



diff --git a/src/crypto/x509/pkix/pkix.go b/src/crypto/x509/pkix/pkix.go
index faad406..39fd78d 100644
--- a/src/crypto/x509/pkix/pkix.go
+++ b/src/crypto/x509/pkix/pkix.go
@@ -64,34 +64,36 @@
if len(rdn) == 0 {
continue
}
- atv := rdn[0]
- n.Names = append(n.Names, atv)
- value, ok := atv.Value.(string)
- if !ok {
- continue
- }

- t := atv.Type
- if len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 {
- switch t[3] {
- case 3:
- n.CommonName = value
- case 5:
- n.SerialNumber = value
- case 6:
- n.Country = append(n.Country, value)
- case 7:
- n.Locality = append(n.Locality, value)
- case 8:
- n.Province = append(n.Province, value)
- case 9:
- n.StreetAddress = append(n.StreetAddress, value)
- case 10:
- n.Organization = append(n.Organization, value)
- case 11:
- n.OrganizationalUnit = append(n.OrganizationalUnit, value)
- case 17:
- n.PostalCode = append(n.PostalCode, value)
+ for _, atv := range rdn {
+ n.Names = append(n.Names, atv)
+ value, ok := atv.Value.(string)
+ if !ok {
+ continue
+ }
+
+ t := atv.Type
+ if len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 {
+ switch t[3] {
+ case 3:
+ n.CommonName = value
+ case 5:
+ n.SerialNumber = value
+ case 6:
+ n.Country = append(n.Country, value)
+ case 7:
+ n.Locality = append(n.Locality, value)
+ case 8:
+ n.Province = append(n.Province, value)
+ case 9:
+ n.StreetAddress = append(n.StreetAddress, value)
+ case 10:
+ n.Organization = append(n.Organization, value)
+ case 11:
+ n.OrganizationalUnit = append(n.OrganizationalUnit, value)
+ case 17:
+ n.PostalCode = append(n.PostalCode, value)
+ }
}
}
}
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 36c2b91..c5b0269 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -1405,3 +1405,55 @@
t.Errorf("ISO OID not recognised in certificate")
}
}
+
+// certMultipleRDN contains a RelativeDistinguishedName with two elements
(the
+// common name and serial number). This particular certificate was the
first
+// such certificate in the “Pilot” Certificate Transparency log.
+const certMultipleRDN = `
+-----BEGIN CERTIFICATE-----
+MIIFRzCCBC+gAwIBAgIEOl59NTANBgkqhkiG9w0BAQUFADA9MQswCQYDVQQGEwJz
+aTEbMBkGA1UEChMSc3RhdGUtaW5zdGl0dXRpb25zMREwDwYDVQQLEwhzaWdvdi1j
+YTAeFw0xMjExMTYxMDUyNTdaFw0xNzExMTYxMjQ5MDVaMIGLMQswCQYDVQQGEwJz
+aTEbMBkGA1UEChMSc3RhdGUtaW5zdGl0dXRpb25zMRkwFwYDVQQLExB3ZWItY2Vy
+dGlmaWNhdGVzMRAwDgYDVQQLEwdTZXJ2ZXJzMTIwFAYDVQQFEw0xMjM2NDg0MDEw
+MDEwMBoGA1UEAxMTZXBvcnRhbC5tc3MuZWR1cy5zaTCCASIwDQYJKoZIhvcNAQEB
+BQADggEPADCCAQoCggEBAMrNkZH9MPuBTjMGNk3sJX8V+CkFx/4ru7RTlLS6dlYM
+098dtSfJ3s2w0p/1NB9UmR8j0yS0Kg6yoZ3ShsSO4DWBtcQD8820a6BYwqxxQTNf
+HSRZOc+N/4TQrvmK6t4k9Aw+YEYTMrWOU4UTeyhDeCcUsBdh7HjfWsVaqNky+2sv
+oic3zP5gF+2QfPkvOoHT3FLR8olNhViIE6Kk3eFIEs4dkq/ZzlYdLb8pHQoj/sGI
+zFmA5AFvm1HURqOmJriFjBwaCtn8AVEYOtQrnUCzJYu1ex8azyS2ZgYMX0u8A5Z/
+y2aMS/B2W+H79WcgLpK28vPwe7vam0oFrVytAd+u65ECAwEAAaOCAf4wggH6MA4G
+A1UdDwEB/wQEAwIFoDBABgNVHSAEOTA3MDUGCisGAQQBr1kBAwMwJzAlBggrBgEF
+BQcCARYZaHR0cDovL3d3dy5jYS5nb3Yuc2kvY3BzLzAfBgNVHREEGDAWgRRwb2Rw
+b3JhLm1pemtzQGdvdi5zaTCB8QYDVR0fBIHpMIHmMFWgU6BRpE8wTTELMAkGA1UE
+BhMCc2kxGzAZBgNVBAoTEnN0YXRlLWluc3RpdHV0aW9uczERMA8GA1UECxMIc2ln
+b3YtY2ExDjAMBgNVBAMTBUNSTDM5MIGMoIGJoIGGhldsZGFwOi8veDUwMC5nb3Yu
+c2kvb3U9c2lnb3YtY2Esbz1zdGF0ZS1pbnN0aXR1dGlvbnMsYz1zaT9jZXJ0aWZp
+Y2F0ZVJldm9jYXRpb25MaXN0P2Jhc2WGK2h0dHA6Ly93d3cuc2lnb3YtY2EuZ292
+LnNpL2NybC9zaWdvdi1jYS5jcmwwKwYDVR0QBCQwIoAPMjAxMjExMTYxMDUyNTda
+gQ8yMDE3MTExNjEyNDkwNVowHwYDVR0jBBgwFoAUHvjUU2uzgwbpBAZXAvmlv8ZY
+PHIwHQYDVR0OBBYEFGI1Duuu+wTGDZka/xHNbwcbM69ZMAkGA1UdEwQCMAAwGQYJ
+KoZIhvZ9B0EABAwwChsEVjcuMQMCA6gwDQYJKoZIhvcNAQEFBQADggEBAHny0K1y
+BQznrzDu3DDpBcGYguKU0dvU9rqsV1ua4nxkriSMWjgsX6XJFDdDW60I3P4VWab5
+ag5fZzbGqi8kva/CzGgZh+CES0aWCPy+4Gb8lwOTt+854/laaJvd6kgKTER7z7U9
+9C86Ch2y4sXNwwwPJ1A9dmrZJZOcJjS/WYZgwaafY2Hdxub5jqPE5nehwYUPVu9R
+uH6/skk4OEKcfOtN0hCnISOVuKYyS4ANARWRG5VGHIH06z3lGUVARFRJ61gtAprd
+La+fgSS+LVZ+kU2TkeoWAKvGq8MAgDq4D4Xqwekg7WKFeuyusi/NI5rm40XgjBMF
+DF72IUofoVt7wo0=
+-----END CERTIFICATE-----`
+
+func TestMultipleRDN(t *testing.T) {
+ block, _ := pem.Decode([]byte(certMultipleRDN))
+ cert, err := ParseCertificate(block.Bytes)
+ if err != nil {
+ t.Fatalf("certificate with two elements in an RDN failed to parse: %s",
err)
+ }
+
+ if expected := "eportal.mss.edus.si"; cert.Subject.CommonName != expected
{
+ t.Errorf("expected common name of %q, but got %q", expected,
cert.Subject.CommonName)
+ }
+
+ if expected := "1236484010010"; cert.Subject.SerialNumber != expected {
+ t.Errorf("expected serial number of %q, but got %q", expected,
cert.Subject.SerialNumber)
+ }
+}

--
https://go-review.googlesource.com/30810

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/x509: parse all names in an RDN.

Patch Set 1: Run-TryBot+1

--
https://go-review.googlesource.com/30810
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: parse all names in an RDN.

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=b2c67ad7

--
https://go-review.googlesource.com/30810
Gerrit-Reviewer: Adam Langley <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: parse all names in an RDN.

Patch Set 1: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/30810
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-HasComments: No

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: parse all names in an RDN.

Patch Set 1: Code-Review+2

(4 comments)

https://go-review.googlesource.com/#/c/30810/1/src/crypto/x509/x509_test.go
File src/crypto/x509/x509_test.go:

Line 1449: t.Fatalf("certificate with two elements in an RDN failed to
parse: %s", err)
we usually use %v in formats for type error. doesn't really matter, though.


Line 1452: if expected := "eportal.mss.edus.si";
cert.Subject.CommonName != expected {
Go style would be s/expected/want/


Line 1453: t.Errorf("expected common name of %q, but got %q", expected,
cert.Subject.CommonName)
got before want:

"common name = %q; want %q"


Line 1456: if expected := "1236484010010"; cert.Subject.SerialNumber !=
expected {
etc

--
https://go-review.googlesource.com/30810
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: Yes

[Adam Langley (Gerrit)]

Reviewers: Gobot Gobot, Brad Fitzpatrick

Adam Langley uploaded a new patch set:

https://go-review.googlesource.com/30810

crypto/x509: parse all names in an RDN.

The Subject and Issuer names in a certificate look like they should be a
list of key-value pairs. However, they're actually a list of lists of
key-value pairs. Previously we only looked at the first element of each
sublist and the vast majority of certificates only have one element per
sublist.

However, it's possible to have multiple elements and some 360
certificates from the “Pilot” log are so constructed.

This change causes all elements of the sublists to be processed.

Fixes #16836.

Change-Id: Ie0a5159135b08226ec517fcf251aa17aada37857
---
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509_test.go
2 files changed, 81 insertions(+), 27 deletions(-)

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/x509: parse all names in an RDN.

Patch Set 1:

(4 comments)

https://go-review.googlesource.com/#/c/30810/1/src/crypto/x509/x509_test.go
File src/crypto/x509/x509_test.go:

Line 1449: t.Fatalf("certificate with two elements in an RDN failed to
parse: %s", err)

> we usually use %v in formats for type error. doesn't really matter,
> though.

Done


Line 1452: if expected := "eportal.mss.edus.si";
cert.Subject.CommonName != expected {

> Go style would be s/expected/want/

Done


Line 1453: t.Errorf("expected common name of %q, but got %q", expected,
cert.Subject.CommonName)
> got before want:
Done


Line 1456: if expected := "1236484010010"; cert.Subject.SerialNumber !=
expected {
> etc
Done

--
https://go-review.googlesource.com/30810
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: Yes

[Adam Langley (Gerrit)]

Adam Langley has submitted this change and it was merged.

crypto/x509: parse all names in an RDN.

The Subject and Issuer names in a certificate look like they should be a
list of key-value pairs. However, they're actually a list of lists of
key-value pairs. Previously we only looked at the first element of each
sublist and the vast majority of certificates only have one element per
sublist.

However, it's possible to have multiple elements and some 360
certificates from the “Pilot” log are so constructed.

This change causes all elements of the sublists to be processed.

Fixes #16836.

Change-Id: Ie0a5159135b08226ec517fcf251aa17aada37857

Reviewed-on: https://go-review.googlesource.com/30810
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>

---
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509_test.go
2 files changed, 81 insertions(+), 27 deletions(-)

Approvals:
Brad Fitzpatrick: Looks good to me, approved

[Omar Shafie (Gerrit)]

Omar Shafie posted comments on crypto/x509: parse all names in an RDN..

View Change

Patch Set 3: (1 comment) As preferred, putting my GitHub comment (for Issue #12342) in the original Gerrit CR.

• File src/crypto/x509/pkix/pkix.go:

  • Patch Set #3, Line 68:

    		for _, atv := range rdn {
    			n.Names = append(n.Names, atv)
    			value, ok := atv.Value.(string)
    			if !ok {
    				continue
    			}
    

    While this code successfully parses a multi-value RDN, by merely appending to Names, it strips the set grouping. That is, ToRDNSequence after parsing such a certificate will not result in a multi-value RDN. As noted previously, without a breaking change to pkix.Name or the introduction of new fields/methods, I do not see an obvious way to preserve the multi-value nature of the RDN. While multi-valued RDNs may not be common, they are clearly part of the RDN standard in RFC 5280, not erroneous certificate constructions like Bug #16836 seems to suggest. If you decide to keep the parsing logic, I would at least add a note documenting the loss of information about the multi-value nature of an RDN.

To view, visit this change. To unsubscribe, visit settings.

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0a5159135b08226ec517fcf251aa17aada37857
Gerrit-PatchSet: 3
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Owner: Adam Langley <a...@golang.org>Gerrit-Reviewer: Adam Langley <a...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: Yes