-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: Name type does not sensibly generate RDNs #40876
Comments
In case the exact Go version is important, the actual version on the playground right now is 1.15. See https://play.golang.org/p/Ztyu2FJaajl. The text on the About page is misleading due to #40319. |
Like @thanatos stated, the docs for pkix.Name are important here:
And for pkix.Name.FillFromRDNSequence
I agree that this doesn't behave the way it's supposed to, but I'm also not sure that we are at a place to change this now. At the very least, it could break backwards compatibility for people that rely on the existing behavior (albeit not the right one). |
Indeed, this is unfortunate behavior that is now enshrined by the compatibility promise. If you have suggestions for how the docs could be clearer about it, we'd be happy to hear it. |
I sort of figured that stability would prevent anything here, and breaking existing code that could be relying on the behavior does not seem like a great idea. I thought it still worthwhile to document the bug; this also gives me a place to refer to. Thanks for at least taking the time to read it. I'll think a bit on the docs, and perhaps take another read of them to see. |
Indeed, this is a known issue we are regrettably not going to fix, and it's useful to have an issue number for it. We still welcome suggestions for improving the docs if there are any, but closing the issue as there is nothing left to do on our side. |
Thanks a lot @thanatos for the summarizing the problems of the implementation in this issue as well, very helpful information 🙏 |
What version of Go are you using (
go version
)?1.14.7, specifically, the playground.
Does this issue reproduce with the latest release?
It reproduces on the playground, which uses the "latest stable release"
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
(Playground)
What did you expect to see / what you saw instead?
First understand RDNs; LDAPwiki is a good source, particularly,
In practice, Relative Distinguished Name containing multiple name-value pairs (called "Multi-Valued RDNs") are rare, but they can be useful at times when either there is no unique attribute in the entry or you want to ensure that the entry's Distinguished Name contains some useful identifying information.
The above program uses >1 entry in the
Name.OrganizationUnit
field. Whenpkix.Name
encodes this into anRDNSequence
, either through callingToRDNSequence
or by simply printing it, go will encode all the given OUs as a single multi-valued RDN; the source code topkix
even explicity notes this,Such RDNs are always invalid, though; in an RDN, each
AttributeTypeAndValue
must have a differentAttributeType
. From X.501¹,Regardless, an RDN represents a hierarchical reference to an entity of some sort. Multiple OUs, in particular, is going to refer to one OU nested inside of another.
Further,
pkix.Name
's layout doesn't preserve ordering of the input RDN; not only will it fail to round trip an RDNSequence with two RDNs containing the same AttributeType, it also can't even round-trip the example from the standard:(Playground)
The package sort of notes this,
But it hands a most likely unaware user a loaded gun of an API. E.g.,
cert-manager
, a project to issues certificates in Kubernetes clusters, adopted the structure ofpkix.Name
nearly verbatim to represent RDN sequences. Since it usespkix.Name
nearly directly, it suffers all the same flaws: some RDN sequences are impossible to input, and some input result in invalid nonsensical RDN sequences. A high-level API should not guide the user towards code that produces incorrect results.¹Note that this is a superseded version of the standard; as X.501 is not an open standard, it is not possible to link to it.
The text was updated successfully, but these errors were encountered: