Merge pull request #167 from smallstep/acme-commonname-2

Allow DNS names in CommonName for ACME

acme/order.go

@@ -4,7 +4,8 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/json"
-	"reflect"
+	"sort"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -253,17 +254,29 @@ func (o *order) finalize(db nosql.DB, csr *x509.CertificateRequest, auth SignAut
 		return nil, ServerInternalErr(errors.Errorf("unexpected status %s for order %s", o.Status, o.ID))
 	}
 
-	// Validate identifier names against CSR alternative names //
+	// RFC8555: The CSR MUST indicate the exact same set of requested
-	csrNames := make(map[string]int)
+	// identifiers as the initial newOrder request. Identifiers of type "dns"
-	for _, n := range csr.DNSNames {
+	// MUST appear either in the commonName portion of the requested subject
-		csrNames[n] = 1
+	// name or in an extensionRequest attribute [RFC2985] requesting a
+	// subjectAltName extension, or both.
+	if csr.Subject.CommonName != "" {
+		csr.DNSNames = append(csr.DNSNames, csr.Subject.CommonName)
 	}
-	orderNames := make(map[string]int)
+	csr.DNSNames = uniqueLowerNames(csr.DNSNames)
-	for _, n := range o.Identifiers {
+	orderNames := make([]string, len(o.Identifiers))
-		orderNames[n.Value] = 1
+	for i, n := range o.Identifiers {
+		orderNames[i] = n.Value
 	}
-	if !reflect.DeepEqual(csrNames, orderNames) {
+	orderNames = uniqueLowerNames(orderNames)
-		return nil, BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly"))
+
+	// Validate identifier names against CSR alternative names.
+	if len(csr.DNSNames) != len(orderNames) {
+		return nil, BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly: CSR names = %v, Order names = %v", csr.DNSNames, orderNames))
+	}
+	for i := range csr.DNSNames {
+		if csr.DNSNames[i] != orderNames[i] {
+			return nil, BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly: CSR names = %v, Order names = %v", csr.DNSNames, orderNames))
+		}
 	}
 
 	// Get authorizations from the ACME provisioner.
@@ -340,3 +353,19 @@ func (o *order) toACME(db nosql.DB, dir *directory, p provisioner.Interface) (*O
 	}
 	return ao, nil
 }
+
+// uniqueLowerNames returns the set of all unique names in the input after all
+// of them are lowercased. The returned names will be in their lowercased form
+// and sorted alphabetically.
+func uniqueLowerNames(names []string) (unique []string) {
+	nameMap := make(map[string]int, len(names))
+	for _, name := range names {
+		nameMap[strings.ToLower(name)] = 1
+	}
+	unique = make([]string, 0, len(nameMap))
+	for name := range nameMap {
+		unique = append(unique, name)
+	}
+	sort.Strings(unique)
+	return
+}

acme/order_test.go

@@ -906,9 +906,9 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "foo",
+					CommonName: "acme.example.com",
 				},
-				DNSNames: []string{"bar", "baz"},
+				DNSNames: []string{"acme.example.com", "fail.smallstep.com"},
 			}
 			return test{
 				o:   o,
@@ -923,9 +923,9 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "acme.example.com",
+					CommonName: "",
 				},
-				DNSNames: []string{"step.example.com"},
+				DNSNames: []string{"acme.example.com"},
 			}
 			return test{
 				o:   o,
@@ -940,7 +940,7 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "foo",
+					CommonName: "acme.example.com",
 				},
 				DNSNames: []string{"step.example.com", "acme.example.com"},
 			}
@@ -962,7 +962,7 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "foo",
+					CommonName: "acme.example.com",
 				},
 				DNSNames: []string{"step.example.com", "acme.example.com"},
 			}
@@ -982,7 +982,7 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "foo",
+					CommonName: "acme.example.com",
 				},
 				DNSNames: []string{"step.example.com", "acme.example.com"},
 			}
@@ -1017,7 +1017,7 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "acme",
+					CommonName: "acme.example.com",
 				},
 				DNSNames: []string{"acme.example.com", "step.example.com"},
 			}
@@ -1057,7 +1057,7 @@ func TestOrderFinalize(t *testing.T) {
 
 			csr := &x509.CertificateRequest{
 				Subject: pkix.Name{
-					CommonName: "foo",
+					CommonName: "acme.example.com",
 				},
 				DNSNames: []string{"acme.example.com", "step.example.com"},
 			}
@@ -1098,6 +1098,102 @@ func TestOrderFinalize(t *testing.T) {
 				},
 			}
 		},
+		"ok/ready/no-sans": func(t *testing.T) test {
+			o, err := newO()
+			assert.FatalError(t, err)
+			o.Status = StatusReady
+			o.Identifiers = []Identifier{
+				{Type: "dns", Value: "step.example.com"},
+			}
+
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "step.example.com",
+				},
+			}
+			crt := &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "step.example.com",
+				},
+				DNSNames: []string{"step.example.com"},
+			}
+			inter := &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "intermediate",
+				},
+			}
+
+			clone := *o
+			clone.Status = StatusValid
+			count := 0
+			return test{
+				o:   o,
+				res: &clone,
+				csr: csr,
+				sa: &mockSignAuth{
+					sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) {
+						assert.Equals(t, len(signOps), 4)
+						return []*x509.Certificate{crt, inter}, nil
+					},
+				},
+				db: &db.MockNoSQLDB{
+					MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) {
+						if count == 0 {
+							clone.Certificate = string(key)
+						}
+						count++
+						return nil, true, nil
+					},
+				},
+			}
+		},
+		"ok/ready/sans-and-name": func(t *testing.T) test {
+			o, err := newO()
+			assert.FatalError(t, err)
+			o.Status = StatusReady
+
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "acme.example.com",
+				},
+				DNSNames: []string{"step.example.com"},
+			}
+			crt := &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "acme.example.com",
+				},
+				DNSNames: []string{"acme.example.com", "step.example.com"},
+			}
+			inter := &x509.Certificate{
+				Subject: pkix.Name{
+					CommonName: "intermediate",
+				},
+			}
+
+			clone := *o
+			clone.Status = StatusValid
+			count := 0
+			return test{
+				o:   o,
+				res: &clone,
+				csr: csr,
+				sa: &mockSignAuth{
+					sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) {
+						assert.Equals(t, len(signOps), 4)
+						return []*x509.Certificate{crt, inter}, nil
+					},
+				},
+				db: &db.MockNoSQLDB{
+					MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) {
+						if count == 0 {
+							clone.Certificate = string(key)
+						}
+						count++
+						return nil, true, nil
+					},
+				},
+			}
+		},
 	}
 	for name, run := range tests {
 		t.Run(name, func(t *testing.T) {