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

crypto/x509: ParseCertificate ignores all but first value from Subject's []string{} fields #18654

Closed
joemiller opened this issue Jan 13, 2017 · 13 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@joemiller
Copy link

joemiller commented Jan 13, 2017

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

go1.7.4, go1.8rc1

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

darwin/amd64

What did you do?

When parsing a certificate with the x509.ParseCertificate() function it ignores all but the first value from attributes in the Subject (pkix.Name) struct that support multiple values.

A cert contains a Subject (pkix.Name) struct. Some of the attributes in this struct support multiple values, represented as []string{}, such as OrganizationalUnit, Locality, etc:

type Name struct {
        Country, Organization, OrganizationalUnit []string
        Locality, Province                        []string
        StreetAddress, PostalCode                 []string
        SerialNumber, CommonName                  string

        Names      []AttributeTypeAndValue
        ExtraNames []AttributeTypeAndValue
}

When x509.ParseCertificate() parses a certificate that has, for example, two OrganizationalUnit (OU) attributes only the first one is parsed and set in the returned x509.Certificate

Note that x509.CreateCertificate() will correctly create certs with multiple values.

Example code and additional commentary demonstrating this issue in this gist: https://gist.github.com/joemiller/c97a52d46cae0a4b38df841db8307fc4

I can move the info from the gist into this issue if that is desirable. I don't know the golang issue etiquette.

What did you expect to see?

x509.ParseCertificate() should return an x509.Certificate containing all of the values for the fields that are of type []string{}

What did you see instead?

I only see the first value for each field.

Discovered during this work: hashicorp/vault#2251

@joemiller joemiller changed the title crypto/x509 crypto/x509 ParseCertificate ignores all but first value from OrganizationalUnit and other []string{} fields Jan 13, 2017
@bradfitz bradfitz changed the title crypto/x509 ParseCertificate ignores all but first value from OrganizationalUnit and other []string{} fields crypto/x509: ParseCertificate ignores all but first value from OrganizationalUnit and other []string{} fields Jan 13, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Jan 13, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 13, 2017
@joemiller joemiller changed the title crypto/x509: ParseCertificate ignores all but first value from OrganizationalUnit and other []string{} fields crypto/x509: ParseCertificate ignores all but first value from Subject's []string{} fields Jan 13, 2017
@csstaub
Copy link

csstaub commented Jan 14, 2017

I tried to reproduce this on go1.8rc1 running on darwin/amd64, seems to work fine for me:

package main

import (
	"crypto/x509"
	"encoding/pem"
	"fmt"
)

const testCertificate = `
-----BEGIN CERTIFICATE-----
MIIDTzCCAjegAwIBAgIJAPd2rUd3n9vNMA0GCSqGSIb3DQEBCwUAMDExDTALBgNV
BAMTBHRlc3QxDzANBgNVBAsTBnZhbHVlMTEPMA0GA1UECxMGdmFsdWUyMB4XDTE3
MDExNDA3MTMwNloXDTE5MTAxMTA3MTMwNlowMTENMAsGA1UEAxMEdGVzdDEPMA0G
A1UECxMGdmFsdWUxMQ8wDQYDVQQLEwZ2YWx1ZTIwggEiMA0GCSqGSIb3DQEBAQUA
A4IBDwAwggEKAoIBAQDOm1c/1c/Ry5/H1WOG09bL/XuCbOhNT62yzyVfGk9dy/oB
HobuV+ow/ctUJ43dgFsJgkJ+w3f/+DLlmRFwoaSbHoVV+5hWdCtBmKCAwWYJ4m0d
bppYGU/IAfGuDSgsSTmQs/MEJ6T5jRdSNIBAQy3ci0stsZsZazGZ+i0TPTQByRvn
F3c1cU9QXKzB52BRU3qm0X+SFvn+uEoabI3Nf1AXWqXye5yybJtWffdc/N5kA7Bz
VqTU314DUxob1LoTbKsv1agGoBfIYMvhsbVkJpSRSKX/X+scYv7J4RrEhPDX+TWJ
3YYjE5dHNgZxqCf1kPKi6ms7iM6u1TBpqshwHaFTAgMBAAGjajBoMAwGA1UdEwEB
/wQCMAAwCwYDVR0PBAQDAgTwMEsGA1UdIwREMEKhNaQzMDExDTALBgNVBAMTBHRl
c3QxDzANBgNVBAsTBnZhbHVlMTEPMA0GA1UECxMGdmFsdWUyggkA93atR3ef280w
DQYJKoZIhvcNAQELBQADggEBAG8MH0plZthQBpdoGK3uyh2mErQ9IHLDi9r2ykLV
h24GdV0UExqL5LAr1l+Nd23KFfYMr3E8nm+PWkqZCOyQTVj7Jby2ETqL+DJWVcJp
ZO9HhgQ2sOvicg2GNIqSGUm1qH3BJwBo3NPGFF/QgBJeuZnJzPaWZrUoriUUDyJ6
/TRm5wRXFMlekt2g3/zpNqeHZSQm4zNijY+qJ+BNZumtPSE6i5093neNjOPhY4yI
n/GWK1XL4ET98qvNHM05gO0cWIe1ZnYEFzyylMPEwWLcluIrBy2aj9xbKR5FKWXb
LDSb0SqDg4UMbsQw3AidGXo5Vluc22KigTrJV9mf1OrUm7Q=
-----END CERTIFICATE-----`

func main() {
	block, _ := pem.Decode([]byte(testCertificate))
	cert, _ := x509.ParseCertificate(block.Bytes)

	fmt.Printf("CN = %v\n", cert.Subject.CommonName)
	fmt.Printf("OU = %v\n", cert.Subject.OrganizationalUnit)
}

Parses and populates both OU values in the embedded certificate:

$ go version
go version go1.8rc1 darwin/amd64
$ go build -o dump main.go && ./dump
CN = test
OU = [value1 value2]

OpenSSL for comparison:

$ openssl x509 -in test-x509v3.crt -noout -text | grep Subject:
        Subject: CN=test, OU=value1, OU=value2

The linked foo.go program (from gist) also seems to parse multiple values correctly:

$ go build -o foo foo.go && ./foo | grep 'ou[12]'
  (string) (len=3) "ou1",
  (string) (len=3) "ou2"
   Value: (string) (len=3) "ou1"
   Value: (string) (len=3) "ou2"

Used grep ou[12] to condense output for readability, but works for all multi-value fields equally.

@joemiller
Copy link
Author

With your code I get identical results on 1.7 and 1.8rc:

$ go version
go version go1.8rc1 darwin/amd64
$ go build -o dump f.go && ./dump
CN = test
OU = [value1 value2]

$ go version
go version go1.7.4 darwin/amd64
$ go build -o dump f.go && ./dump
CN = test
OU = [value1 value2]

Using my code from the gist, I get differing results between 1.7 and 1.8:

$ go version
go version go1.7.4 darwin/amd64
$  go build -o foo foo.go && ./foo | grep 'ou[12]'
  (string) (len=3) "ou1"
   Value: (string) (len=3) "ou1"

$ go version
go version go1.8rc1 darwin/amd64
$ go build -o foo foo.go && ./foo | grep 'ou[12]'
  (string) (len=3) "ou1",
  (string) (len=3) "ou2"
   Value: (string) (len=3) "ou1"
   Value: (string) (len=3) "ou2"

I'm a bit confused at this point, but I'll keep poking. Thanks for the feedback.

@joemiller
Copy link
Author

Trying a slight variation:

Using your code, but with a cert.pem created by the code in my (foo.go) gist. I get differing behavior on 1.7.4 -vs- 1.8rc1, even though I had identical behavior using your code with your original cert fixture:

package main

import (
        "crypto/x509"
        "encoding/pem"
        "fmt"
)

const testCertificate = `
-----BEGIN CERTIFICATE-----
MIIEVDCCAzygAwIBAgIRALYZF1aMGVFSHyIZJmWmSAQwDQYJKoZIhvcNAQELBQAw
gckxFjAJBgNVBAYTAmMxMAkGA1UEBhMCYzIxHDAMBgNVBAgTBXByb3YxMAwGA1UE
CBMFcHJvdjIxFjAJBgNVBAcTAmwxMAkGA1UEBxMCbDIxHDAMBgNVBAkTBWFkZHIx
MAwGA1UECRMFYWRkcjIxHDAMBgNVBBETBXBvc3QxMAwGA1UEERMFcG9zdDIxIzAO
BgNVBAoTB0FjbWUgQ28wEQYDVQQKEwpzZWNvbmQgb3JnMRgwCgYDVQQLEwNvdTEw
CgYDVQQLEwNvdTIwHhcNMTcwMTE0MTc1NTM3WhcNMTcwMTE0MjA1NTM3WjCByTEW
MAkGA1UEBhMCYzEwCQYDVQQGEwJjMjEcMAwGA1UECBMFcHJvdjEwDAYDVQQIEwVw
cm92MjEWMAkGA1UEBxMCbDEwCQYDVQQHEwJsMjEcMAwGA1UECRMFYWRkcjEwDAYD
VQQJEwVhZGRyMjEcMAwGA1UEERMFcG9zdDEwDAYDVQQREwVwb3N0MjEjMA4GA1UE
ChMHQWNtZSBDbzARBgNVBAoTCnNlY29uZCBvcmcxGDAKBgNVBAsTA291MTAKBgNV
BAsTA291MjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALwmq1o2NSYI
IaR5pqCu6y0PyCrX6pm7V2goeFM44Ydj3fAh9b+oA8gXIdCQcML4nKVkVIg+isLR
KTC8eN/F8zMPm01Gzzog1I4mvahk10HojFaxUEkFD99fg4uWyZDxQWGfGPQA4IKJ
X6vHEgN7CpqnafAjeqoICrc2pigAaHca4Of3Vq6a1lZzMT2eIi3ESxVNukc/vEUo
4azJJ3uUjXBhG2GK6wGKsP5ponZw0jQHj8lny/nAi7XT2NYJ2Q6U1I+7zhHB0Xjz
EEz62ZwrbvxGwGXkrlfBJ+3hdi3ggKyDeIRfgBDU4xqHh6+zfwRKI4D0mARDQhF8
k6VgygbUJO8CAwEAAaM1MDMwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsG
AQUFBwMBMAwGA1UdEwEB/wQCMAAwDQYJKoZIhvcNAQELBQADggEBAHj1EFJ6Jav2
Xs1KUsDbBQlcbbh4ebTJhSOjEiC1Jg31JQ9A/0WHtSlAWh7KfV51eG2xNFBwUD5x
y53XNR9wuEZARZhgY5ovhr+4fmZHODPTQrAlq/KCc4rf+ecPtYR8Wu75DJZkyWSW
4td4O0+xD/aexqQsqYm4sRyf6d/XzUCgj4wjUCw4hcOT8mnARL8BSnJpbevDOxjL
iiTel4LSrYsoYdtjd2SHNJ5rlQ7fl7ccVN7NBSgxdzskGJZUvllC6phwOIfM141R
3n9tiO6Wb043WYO3exrptKTIz3HxZWhEYZUbkwQ+yU3rt6PQOOTN0HEe7xb1PG2W
VICti8IoCn4=
-----END CERTIFICATE-----
`

func main() {
        block, _ := pem.Decode([]byte(testCertificate))
        cert, _ := x509.ParseCertificate(block.Bytes)

        fmt.Printf("CN = %v\n", cert.Subject.CommonName)
        fmt.Printf("OU = %v\n", cert.Subject.OrganizationalUnit)
}
$ go version
go version go1.7.4 darwin/amd64
$ go build -o dump main.go && ./dump
CN =
OU = [ou1]

$ go version
go version go1.8rc1 darwin/amd64
$ go build -o dump main.go && ./dump
CN =
OU = [ou1 ou2]

I'm confused as to why the cert fixture in your code works as expected in both 1.7.4 and 1.8 for me, while the other cert only works as expected in 1.8. In any case, I'm getting better results from 1.8 in general at this time, so possibly a non-issue in 1.8+ which is good.

@csstaub
Copy link

csstaub commented Jan 14, 2017

Can confirm, seeing the same difference in behavior (using your test cert, but not mine) in 1.7.1 vs. 1.8rc1.

@csstaub
Copy link

csstaub commented Jan 14, 2017

It appears there's a difference in the way the subjects are encoded.

From the working cert (output from dumpasn1):

 56  15:       SET {
 58  13:         SEQUENCE {
 60   3:           OBJECT IDENTIFIER organizationalUnitName (2 5 4 11)
 65   6:           PrintableString 'value1'
       :           }
       :         }
 73  15:       SET {
 75  13:         SEQUENCE {
 77   3:           OBJECT IDENTIFIER organizationalUnitName (2 5 4 11)
 82   6:           PrintableString 'value2'
       :           }
       :         }
       :       }

From the broken cert:

 461   24:       SET {
 463   10:         SEQUENCE {
 465    3:           OBJECT IDENTIFIER organizationalUnitName (2 5 4 11)
 470    3:           PrintableString 'ou1'
         :           }
 475   10:         SEQUENCE {
 477    3:           OBJECT IDENTIFIER organizationalUnitName (2 5 4 11)
 482    3:           PrintableString 'ou2'
         :           }
         :         }

For reference, my test cert was generated with OpenSSL 0.9.8zh (which ships in macOS Sierra).

@agl
Copy link
Contributor

agl commented Jan 14, 2017

One of the sharp edges of X.509 is that the name structure isn't a mapping from OIDs to a list of values, it's a sequence of sets of pairs. I.e. something like [][]struct{key OID, value string}. (I think the original motivation was to try and identify which values of the multiset were alternatives, and which were needed for a unique identification. These days, nobody cares and that over-engineering is now just a source of confusion.)

There was a bug, fixed in 809a1de, that meant that code previously only looked at first element of these sublists. So the difference between the certificates that work and fail with 1.7 is that the multiple values are either duplicated at the top-level, or the sublevel. You can see this with openssl asn1parse -i.

@agl
Copy link
Contributor

agl commented Jan 14, 2017

(Since this should be fixed in 1.8, I think this can be closed.)

@agl agl closed this as completed Jan 14, 2017
@jefferai
Copy link

@agl Thanks for the clarification, it's much appreciated. Glad it's already fixed!

@joemiller
Copy link
Author

@agl thank you!

@seh
Copy link
Contributor

seh commented Apr 19, 2017

This defect report addresses the parsing behavior of crypto/x509, but I noticed a problem on the generation side of this too: Given a certificate generated via x509.CreateCertificate in Go 1.8, x509.ParseCertificate in Go 1.7.3 retains only the first AVA in an RDN, but the same x509.ParseCertificate invocation in Go 1.7.3 parses a certificate generated by openssl x509 correctly. That is, there is something different between openssl and Go (even in Go 1.8) about the way that they generate their subjects, such that Go's technique made it parse the subject incorrectly in Go 1.7.

Go version Generated by
openssl x509
Generated by
x509.CreateCertificate
1.7.3
x509.ParseCertificate

All AVAs
⚠️
Only first AVA
1.8.1
x509.ParseCertificate

All AVAs

All AVAs

I'm not saying that Go is generating certificates incorrectly; I'm just noting that it's doing something different from openssl x509. If the certificate consumer is built with Go 1.7, the certificate producer can't use x509.CreateCertificate and expect compatibility.

@bradfitz
Copy link
Contributor

@seh, we don't track closed bugs, so if you're filing a bug report, please open a new bug. You can cross-reference the new one and this one.

@jefferai
Copy link

@seh Please do cross-reference if you file a new bug, I'd want to keep track of it.

@seh
Copy link
Contributor

seh commented Apr 19, 2017

As I ponder whether there's anything here deserving a defect report against Go—given that this problem is solved in Go 1.8—here I'll share the rather distasteful workaround that I concocted, mostly by poring over the documentation for pkix.Name and snooping around in the package's implementation.

In my program that creates certificates that I need a consumer built with Go 1.7 to interpret properly, I am specifically interested in conveying only two RDNs in my certificate subject:

  • CN, or common name
  • O, or a set of organizations

First, a couple of helper functions:

func appendSingletonAVAStringSets(name *pkix.Name, oid asn1.ObjectIdentifier, s []string) *pkix.Name {
	for _, v := range s {
		name.ExtraNames = append(name.ExtraNames, pkix.AttributeTypeAndValue{
			Type:  oid,
			Value: v,
		})
	}
	return name
}

func forceOrganizationAsSingletonAVASets(name *pkix.Name) *pkix.Name {
	// Per RFC 5328 appendix A.1:
	oidOrganization := asn1.ObjectIdentifier{2, 5, 4, 10}
	orgs := name.Organization
	// Not strictly necessary:
	name.Organization = nil
	return appendSingletonAVAStringSets(name, oidOrganization, orgs)
}

Then, while preparing for the coming call to x509.CreateCertificate:

signeeTemplate.Subject = csr.Subject
// Work around Go issue 18654 (https://github.com/golang/go/issues/18654):
//
// Force the ASN.1 marshaller to emit a sequence of singleton sets (sets with a single sequence
// with a single element), as opposed to the more compact sequence of a single set with multiple
// sequences, each of a single element.
//
// (Sample output from "openssl asn1parse -i" follows.)
//
// Compact form that Go 1.7 does not parse correctly:
//   SEQUENCE
//    SET
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org1
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org2
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org3
//    SET
//     SEQUENCE
//      OBJECT            :commonName
//      UTF8STRING        :someone@somewhere.org
//
// Expanded form that Go 1.7 parses well enough:
//   SEQUENCE
//    SET
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org1
//    SET
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org2
//    SET
//     SEQUENCE
//      OBJECT            :organizationName
//      PRINTABLESTRING   :org3
//    SET
//     SEQUENCE
//      OBJECT            :commonName
//      UTF8STRING        :someone@somewhere.org
forceOrganizationAsSingletonAVASets(&signeeTemplate.Subject)

I'm not sure if all of my X.509- and ANS.1-related terms are correct here.

If there's a better way to achieve this goal, I'm open to suggestions.

@golang golang locked and limited conversation to collaborators Apr 19, 2018
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants