crypto/x509/pkix: pkix.Name.FillFromRDNSequence ignores attributes other than the first

[eliasnaur]

pkix.Name.FillFromRDNSequence ignores attributes other than the first in the RDN sets passed as argument. As demonstrated by http://play.golang.org/p/GPZdVHzZVb, this in turn makes x509.ParseCertificate(s) fail to locate the subject serial number for certificates where it is included in set containing the common name.

The fix is simple, also demonstrated by the playground program, but I'm unsure if the example certificate is broken and that multiple attributes in one RDN set should be ignored. And even if so, a workaround might still be desirable: The example certificate is issued by the national Danish ID provider, "NemID" mandatory for digital banking and digital public services.

I raised the issue on golang-nuts:

https://groups.google.com/forum/#!topic/golang-nuts/1Whb4ko4zfc

but so far I've received no replies and so this issue serves as a reminder.

[ianlancetaylor]

CC @agl

[agl]

The spec for this is section 9.3 of X.501, but if you can figure out anything from that then you're a smarter man than I.

I'm guessing that it's supposed to express the fact that either the CN or "PID" value is sufficient to identify the object, but why that's information that needs to be reflected in the structure I've no idea.

The Go code tries to hammer that into a simpler model that accounts for 95% of cases: each common identifier OID is allowed a single value. I think that trying to reflect the full complexity of X.501 in Go would be a mistake so hacks to handle cases like this are done based on sufficient real-world need.

[rapropos]

I ran into what I believe is this same problem when attempting to parse certificates generated by PKI.js, which packs multiple attributes into a single ASN.1 SET. I raised it as an issue over there and was told by the author of that library that the problem is on the Go side. I can confirm that PKI.js's certificates do parse properly by OpenSSL, in that all attributes in a set are read.

I believe (at least for my situation) the fix would not require relaxing the constraint that each common identifier OID is allowed a single value. Rather, instead of FillFromRDNSequence reading only rdn[0], have an inner loop across all values of rdn that does exactly what the single pass does now. You wouldn't have to worry about collisions of multiple values for the same OID any more than you do now, and we would gain the ability to properly parse SETs that contain more than one distinct attribute.

[ramoas]

I recently ran into the lack of multi-valued RDN support in GO too.

@agl, As @rapropos noted, it's not the same OID that is being mapped to multiple values. Rather, a single RDN can have more than one AttributeTypeAndValue. From RFC 5280, the RDN is defined to be a set with a size from 1 to MAX:

RDNSequence ::= SEQUENCE OF RelativeDistinguishedName
RelativeDistinguishedName ::= SET SIZE (1..MAX) OF AttributeTypeAndValue

GO is currently assuming MAX is always = 1, which is not a correct assumption:
https://golang.org/src/crypto/x509/pkix/pkix.go?h=FillFromRDNSequence#L67

The canonical example for multi-valued RDNs is where CN is a person's name, say "Alice Smith," where there can be lots of duplicates. To disambiguate, organizations usually then also use some unique identifier, like UID. This is rather common in LDAP and other user directories. It sounds like the example from @eliasnaur is along those lines, but using serial number instead of UID.

As noted, OpenSSL (and others, like Bouncy Castle) support both the generation and parsing of certificates with multi-valued RDNs (just search for "multivalue-rdn"):
https://www.openssl.org/docs/manmaster/apps/req.html

Being a set, the order of the AttributeTypeAndValue is not defined, so arbitrarily picking the first one can lead to different results depending on how the certificate gets generated.

Ideally, GO would parse the DN completely or at least indicate that there is one or more multi-valued RDNs so the caller knows to parse the Issuer / Subject manually if interested in all the values. Right now, calling code cannot rely on the results if it's possible for there to be multi-valued RDNs. And if you're writing code that will run in environments that you do not control, running into this seems like an inevitability.

Unfortunately, looking at the existing structure of pkix.Name, I do not see an obvious way to account for multi-valued RDNs that preserves compatibility. It seems like either new fields and/or new methods must be added or perhaps a new pkix.Name type entirely.

I understand that X509 is a beast, and that GO is still relatively young, and that there are lots of competing priorities for precious resources, but cherry-picking which parts of the X509 standard get implemented and which do not makes it more difficult and error-prone to migrate to GO from other languages / technologies that provide greater coverage. Hopefully, GO can still go further in this regard.

[rsc]

It sounds like maybe if someone can propose a concrete addition (actual code), then @agl can evaluate it. But it's a bit late for Go 1.8.

[agl]

I think this is fixed by 809a1de, but I had it linked with a different bug (#16836).

[ramoas]

@agl Reiterating my comment just added to the changeset 809a1de to widen visibility: While this code successfully parses a multi-value RDN, 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.

[joneskoo]

@ramoas I just noticed this comment; is there an open issue about this? Should this issue be reopened? Otherwise your comment will be lost.

[ramoas]

@joneskoo, Sorry about the response delay. I never opened a separate issue for my comment, but it remains relevant. I had placed my comment in multiple places at the time: this issue, the GH changeset (809a1de), and the original Gerrit CR (https://go-review.googlesource.com/c/go/+/30810#message-a29d12cfb3550dc2ce212ec60ddf32bf86363f13). I assumed that my comment was simply dismissed given that you were the only one to ever acknowledge its existence anywhere. The current GO implementation does not preserve the original structure of a certificate with multi-value RDNs; I still think that's worth fixing or at least documenting at a minimum, but no one else seems to think so.

[joneskoo]

Hi Omar, Does the implementation normally preserve the structure in other cases, is that something someone would rely on? As it sounds like a distinct problem, please open a new issue and collect the details to that single thread, so it’s easier to follow. At least then it’s not lost. If it can be reproduced and the problem is clear, at least it won’t be forgotten. I’m sorry I didn’t read the nasty details re: ASN.1 structures, so I can’t comment what is normal. I won’t be able to help.

[ramoas]

@joneskoo , Yes, generally, structure is preserved. I created #23069 with a clear reproduction. Thanks for following up.