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/pkix: pkix.Name.FillFromRDNSequence ignores attributes other than the first #12342

Closed
eliasnaur opened this issue Aug 26, 2015 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@eliasnaur
Copy link
Contributor

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 ianlancetaylor added this to the Unplanned milestone Aug 26, 2015
@ianlancetaylor
Copy link
Contributor

CC @agl

@agl agl self-assigned this Aug 26, 2015
@agl
Copy link
Contributor

agl commented Aug 30, 2015

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
Copy link

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
Copy link

ramoas commented Jul 17, 2016

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.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned Jul 17, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 20, 2016

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.

@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 20, 2016
@agl
Copy link
Contributor

agl commented Oct 20, 2016

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

@agl agl closed this as completed Oct 20, 2016
@mikioh mikioh modified the milestones: Go1.8Maybe, Go1.9, Go1.8 Oct 30, 2016
@ramoas
Copy link

ramoas commented Nov 17, 2016

@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
Copy link
Contributor

@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
Copy link

ramoas commented Dec 4, 2017

@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
Copy link
Contributor

joneskoo commented Dec 4, 2017 via email

@ramoas
Copy link

ramoas commented Dec 9, 2017

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

@golang golang locked and limited conversation to collaborators Dec 9, 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 NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests