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: Name type does not sensibly generate RDNs #40876

Closed
thanatos opened this issue Aug 18, 2020 · 7 comments
Closed

crypto/x509/pkix: Name type does not sensibly generate RDNs #40876

thanatos opened this issue Aug 18, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Milestone

Comments

@thanatos
Copy link

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

$ 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 Output
$ go env
N/A, the playground

What did you do?

package main

import (
	"fmt"
	"crypto/x509/pkix" 
)

func main() {
	// Adding >1 item to any of the arrays in pkix.Name results in golang representing them as a single multi-valued RDN,
	// which is not valid. X.501 states,
	// > a given attribute type cannot appear twice in the same RDN
	// but that's exactly what Golang will always emit, for any array with >1 entry.
	var name pkix.Name = pkix.Name {
		OrganizationalUnit: []string{"foo", "bar"},
	};
	fmt.Println("OUs of {\"foo\", \"bar\"}, encoded:", name.ToRDNSequence(), " // this is a malformed multi-valued RDN");

	var input_rdn_seq pkix.RDNSequence = []pkix.RelativeDistinguishedNameSET{
		[]pkix.AttributeTypeAndValue { pkix.AttributeTypeAndValue { Type: []int{2, 5, 4, 11} /* OU */, Value: "foo" }},
		[]pkix.AttributeTypeAndValue { pkix.AttributeTypeAndValue { Type: []int{2, 5, 4, 11} /* OU */, Value: "foo" }},
	};
	fmt.Println("RDN seq. representing OU=foo,OU=bar:", input_rdn_seq);
	
	decoded_name := pkix.Name {};
	decoded_name.FillFromRDNSequence(&input_rdn_seq);
	fmt.Println("                  same seq, decoded:", decoded_name, " // wrong");
	fmt.Println("               same seq, re-encoded:", decoded_name.ToRDNSequence(), " // wrong")
}

(Playground)

What did you expect to see / what you saw instead?

First understand RDNs; LDAPwiki is a good source, particularly,

Relative Distinguished Name is comprised of one or more name-value pairs, in which the name and the value are separated by an equal sign (e.g., for an RDN of "uid=ann", the name is "uid" and the value is "ann"), and if there are multiple name-value pairs then they should be separated by plus signs (e.g., for an RDN of "cn=Jon Doe+employeeNumber=12345", the name-value pairs are "cn=John Doe" and "employeeNumber=12345").

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. When pkix.Name encodes this into an RDNSequence, either through calling ToRDNSequence or by simply printing it, go will encode all the given OUs as a single multi-valued RDN; the source code to pkix even explicity notes this,

// ToRDNSequence converts n into a single RDNSequence. The following
// attributes are encoded as multi-value RDNs:
//
//  - Country
//  - Organization
//  - OrganizationalUnit
//  - Locality
//  - Province
//  - StreetAddress
//  - PostalCode

Such RDNs are always invalid, though; in an RDN, each AttributeTypeAndValue must have a different AttributeType. From X.501¹,

The set that forms an RDN contains exactly one AttributeTypeAndValue for each attribute which contains distinguished values in the entry; that is, a given attribute type cannot appear twice in the same RDN.

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:

A diagram of an RDN hierarchy

(Playground)

              Example from the standard: CN=Smith,OU=Sales+L=Ipswitch,O=Telecom,C=GB
Example from the standard, as pkix.Name: CN=Smith,OU=Sales,O=Telecom,L=Ipswitch,C=GB

The package sort of notes this,

Name is only an approximation of the X.509 structure. If an accurate representation is needed, asn1.Unmarshal the raw subject or issuer as an RDNSequence.

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 of pkix.Name nearly verbatim to represent RDN sequences. Since it uses pkix.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.

@dmitshur dmitshur changed the title crypto/x509/pkix's Name type does not sensibly generate RDNs crypto/x509/pkix: Name type does not sensibly generate RDNs Aug 19, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 19, 2020
@dmitshur dmitshur added this to the Backlog milestone Aug 19, 2020
@dmitshur
Copy link
Contributor

/cc @FiloSottile @katiehockman

@dmitshur
Copy link
Contributor

dmitshur commented Aug 19, 2020

It reproduces on the playground, which uses the "latest stable release"

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.

@katiehockman
Copy link
Contributor

Like @thanatos stated, the docs for pkix.Name are important here:

Note that Name is only an approximation of the X.509 structure. If an accurate representation is needed, asn1.Unmarshal the raw subject or issuer as an RDNSequence.

And for pkix.Name.FillFromRDNSequence

Multi-entry RDNs are flattened, all entries are added to the relevant n fields, and the grouping is not preserved.

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).

@FiloSottile
Copy link
Contributor

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.

@thanatos
Copy link
Author

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.

@FiloSottile
Copy link
Contributor

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.

@wallyqs
Copy link

wallyqs commented Oct 5, 2020

Thanks a lot @thanatos for the summarizing the problems of the implementation in this issue as well, very helpful information 🙏

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. Unfortunate
Projects
None yet
Development

No branches or pull requests

6 participants