crypto/x509/pkix: print non-standard parsed Names at the end

This doesn't change how ExtraNames are printed, so as not to cause
unnecessary churn of current outputs. Switched the ExtraNames check to a
nil check as we are checking for just-parsed values.

Fixes #39924
Fixes #39873

Change-Id: Ifa07cfc1a057d73643710a774ef8a154222db187
Reviewed-on: https://go-review.googlesource.com/c/go/+/240543
Run-TryBot: Filippo Valsorda <[email protected]>
Reviewed-by: Katie Hockman <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: FiloSottile

doc/go1.15.html

@@ -553,11 +553,11 @@ <h3 id="minor_library_changes">Minor changes to the library</h3>
 
 <dl id="crypto/x509/pkix"><dt><a href="/pkg/crypto/x509/pkix/">crypto/x509/pkix</a></dt>
   <dd>
-    <p><!-- CL 229864 -->
+    <p><!-- CL 229864, CL 240543 -->
       <a href="/pkg/crypto/x509/pkix/#Name.String"><code>Name.String</code></a>
       now prints non-standard attributes from
       <a href="/pkg/crypto/x509/pkix/#Name.Names"><code>Names</code></a> if
-      <a href="/pkg/crypto/x509/pkix/#Name.ExtraNames"><code>ExtraNames</code></a> is empty.
+      <a href="/pkg/crypto/x509/pkix/#Name.ExtraNames"><code>ExtraNames</code></a> is nil.
     </p>
   </dd>
 </dl><!-- crypto/x509/pkix -->

src/crypto/x509/pkix/pkix.go

@@ -247,20 +247,26 @@ func (n Name) ToRDNSequence() (ret RDNSequence) {
 // String returns the string form of n, roughly following
 // the RFC 2253 Distinguished Names syntax.
 func (n Name) String() string {
-	if len(n.ExtraNames) == 0 {
+	var rdns RDNSequence
+	// If there are no ExtraNames, surface the parsed value (all entries in
+	// Names) instead.
+	if n.ExtraNames == nil {
 		for _, atv := range n.Names {
 			t := atv.Type
 			if len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 {
 				switch t[3] {
 				case 3, 5, 6, 7, 8, 9, 10, 11, 17:
-					// These attributes are already parsed into named fields.
+					// These attributes were already parsed into named fields.
 					continue
 				}
 			}
-			n.ExtraNames = append(n.ExtraNames, atv)
+			// Place non-standard parsed values at the beginning of the sequence
+			// so they will be at the end of the string. See Issue 39924.
+			rdns = append(rdns, []AttributeTypeAndValue{atv})
 		}
 	}
-	return n.ToRDNSequence().String()
+	rdns = append(rdns, n.ToRDNSequence()...)
+	return rdns.String()
 }
 
 // oidInAttributeTypeAndValue reports whether a type with the given OID exists

src/crypto/x509/x509_test.go

@@ -2076,10 +2076,31 @@ func TestPKIXNameString(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Check that parsed non-standard attributes are printed.
+	rdns := pkix.Name{
+		Locality: []string{"Gophertown"},
+		ExtraNames: []pkix.AttributeTypeAndValue{
+			{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "golang.org"}},
+	}.ToRDNSequence()
+	nn := pkix.Name{}
+	nn.FillFromRDNSequence(&rdns)
+
+	// Check that zero-length non-nil ExtraNames hide Names.
+	extra := []pkix.AttributeTypeAndValue{
+		{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "backing array"}}
+	extraNotNil := pkix.Name{
+		Locality:   []string{"Gophertown"},
+		ExtraNames: extra[:0],
+		Names: []pkix.AttributeTypeAndValue{
+			{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "golang.org"}},
+	}
+
 	tests := []struct {
 		dn   pkix.Name
 		want string
 	}{
+		{nn, "L=Gophertown,1.2.3.4.5=#130a676f6c616e672e6f7267"},
+		{extraNotNil, "L=Gophertown"},
 		{pkix.Name{
 			CommonName:         "Steve Kille",
 			Organization:       []string{"Isode Limited"},
@@ -2108,13 +2129,31 @@ func TestPKIXNameString(t *testing.T) {
 			ExtraNames: []pkix.AttributeTypeAndValue{
 				{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "golang.org"}},
 		}, "1.2.3.4.5=#130a676f6c616e672e6f7267,L=Gophertown"},
+		// If there are no ExtraNames, the Names are printed instead.
+		{pkix.Name{
+			Locality: []string{"Gophertown"},
+			Names: []pkix.AttributeTypeAndValue{
+				{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "golang.org"}},
+		}, "L=Gophertown,1.2.3.4.5=#130a676f6c616e672e6f7267"},
+		// If there are both, print only the ExtraNames.
+		{pkix.Name{
+			Locality: []string{"Gophertown"},
+			ExtraNames: []pkix.AttributeTypeAndValue{
+				{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "golang.org"}},
+			Names: []pkix.AttributeTypeAndValue{
+				{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 6}), Value: "example.com"}},
+		}, "1.2.3.4.5=#130a676f6c616e672e6f7267,L=Gophertown"},
 	}
 
 	for i, test := range tests {
 		if got := test.dn.String(); got != test.want {
 			t.Errorf("#%d: String() = \n%s\n, want \n%s", i, got, test.want)
 		}
 	}
+
+	if extra[0].Value != "backing array" {
+		t.Errorf("the backing array of an empty ExtraNames got modified by String")
+	}
 }
 
 func TestRDNSequenceString(t *testing.T) {