From 0a890a5c16eea184b8b35326ae157381450760de Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 28 Jan 2020 15:34:01 -0800 Subject: [PATCH] Add the commonName as a DNSName to match RFC. Normalize names and remove the use of reflection. --- acme/order.go | 49 +++++++++++++++---- acme/order_test.go | 114 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 19 deletions(-) diff --git a/acme/order.go b/acme/order.go index 8d22b7db..27e030e9 100644 --- a/acme/order.go +++ b/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 // - csrNames := make(map[string]int) - for _, n := range csr.DNSNames { - csrNames[n] = 1 + // RFC8555: The CSR MUST indicate the exact same set of requested + // identifiers as the initial newOrder request. Identifiers of type "dns" + // MUST appear either in the commonName portion of the requested subject + // 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) - for _, n := range o.Identifiers { - orderNames[n.Value] = 1 + csr.DNSNames = uniqueLowerNames(csr.DNSNames) + orderNames := make([]string, len(o.Identifiers)) + for i, n := range o.Identifiers { + orderNames[i] = n.Value } - if !reflect.DeepEqual(csrNames, orderNames) { - return nil, BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly")) + orderNames = uniqueLowerNames(orderNames) + + // 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 +} diff --git a/acme/order_test.go b/acme/order_test.go index 18a46589..77c21e24 100644 --- a/acme/order_test.go +++ b/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) {