Validate Subject Common Name for Orders with Permanent Identifier

acme/order.go

@@ -165,6 +165,15 @@ func (o *Order) Finalize(ctx context.Context, db DB, csr *x509.CertificateReques
 	for i := range o.Identifiers {
 		if o.Identifiers[i].Type == PermanentIdentifier {
 			permanentIdentifier = o.Identifiers[i].Value
+			// the first (and only) Permanent Identifier that gets added to the certificate
+			// should be equal to the Subject Common Name if it's set. If not equal, the CSR
+			// is rejected, because the Common Name hasn't been challenged in that case. This
+			// could result in unauthorized access if a relying system relies on the Common
+			// Name in its authorization logic.
+			if csr.Subject.CommonName != "" && csr.Subject.CommonName != permanentIdentifier {
+				return NewError(ErrorBadCSRType, "CSR Subject Common Name does not match identifiers exactly: "+
+					"CSR Subject Common Name = %s, Order Permanent Identifier = %s", csr.Subject.CommonName, permanentIdentifier)
+			}
 			break
 		}
 	}

acme/order_test.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/asn1"
 	"encoding/json"
+	"fmt"
 	"net"
 	"net/url"
 	"reflect"
@@ -386,6 +388,41 @@ func TestOrder_Finalize(t *testing.T) {
 				err: NewErrorISE("unrecognized order status: %s", o.Status),
 			}
 		},
+		"fail/non-matching-permanent-identifier-common-name": func(t *testing.T) test {
+			now := clock.Now()
+			o := &Order{
+				ID:               "oID",
+				AccountID:        "accID",
+				Status:           StatusReady,
+				ExpiresAt:        now.Add(5 * time.Minute),
+				AuthorizationIDs: []string{"a", "b"},
+				Identifiers: []Identifier{
+					{Type: "permanent-identifier", Value: "a-permanent-identifier"},
+				},
+			}
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "a-different-identifier",
+				},
+				ExtraExtensions: []pkix.Extension{
+					{
+						Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 8, 3},
+						Value: []byte("a-permanent-identifier"),
+					},
+				},
+			}
+			return test{
+				o:   o,
+				csr: csr,
+				err: &Error{
+					Type:   "urn:ietf:params:acme:error:badCSR",
+					Detail: "The CSR is unacceptable",
+					Status: 400,
+					Err: fmt.Errorf("CSR Subject Common Name does not match identifiers exactly: "+
+						"CSR Subject Common Name = %s, Order Permanent Identifier = %s", csr.Subject.CommonName, "a-permanent-identifier"),
+				},
+			}
+		},
 		"fail/error-provisioner-auth": func(t *testing.T) test {
 			now := clock.Now()
 			o := &Order{
@@ -617,6 +654,165 @@ func TestOrder_Finalize(t *testing.T) {
 				err: NewErrorISE("error updating order oID: force"),
 			}
 		},
+		"ok/permanent-identifier": func(t *testing.T) test {
+			now := clock.Now()
+			o := &Order{
+				ID:               "oID",
+				AccountID:        "accID",
+				Status:           StatusReady,
+				ExpiresAt:        now.Add(5 * time.Minute),
+				AuthorizationIDs: []string{"a", "b"},
+				Identifiers: []Identifier{
+					{Type: "permanent-identifier", Value: "a-permanent-identifier"},
+				},
+			}
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "a-permanent-identifier",
+				},
+				ExtraExtensions: []pkix.Extension{
+					{
+						Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 8, 3},
+						Value: []byte("a-permanent-identifier"),
+					},
+				},
+			}
+
+			leaf := &x509.Certificate{
+				Subject: pkix.Name{CommonName: "a-permanent-identifier"},
+				ExtraExtensions: []pkix.Extension{
+					{
+						Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 8, 3},
+						Value: []byte("a-permanent-identifier"),
+					},
+				},
+			}
+			inter := &x509.Certificate{Subject: pkix.Name{CommonName: "inter"}}
+			root := &x509.Certificate{Subject: pkix.Name{CommonName: "root"}}
+
+			return test{
+				o:   o,
+				csr: csr,
+				prov: &MockProvisioner{
+					MauthorizeSign: func(ctx context.Context, token string) ([]provisioner.SignOption, error) {
+						assert.Equals(t, token, "")
+						return nil, nil
+					},
+					MgetOptions: func() *provisioner.Options {
+						return nil
+					},
+				},
+				ca: &mockSignAuth{
+					sign: func(_csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) {
+						assert.Equals(t, _csr, csr)
+						return []*x509.Certificate{leaf, inter, root}, nil
+					},
+				},
+				db: &MockDB{
+					MockCreateCertificate: func(ctx context.Context, cert *Certificate) error {
+						cert.ID = "certID"
+						assert.Equals(t, cert.AccountID, o.AccountID)
+						assert.Equals(t, cert.OrderID, o.ID)
+						assert.Equals(t, cert.Leaf, leaf)
+						assert.Equals(t, cert.Intermediates, []*x509.Certificate{inter, root})
+						return nil
+					},
+					MockUpdateOrder: func(ctx context.Context, updo *Order) error {
+						assert.Equals(t, updo.CertificateID, "certID")
+						assert.Equals(t, updo.Status, StatusValid)
+						assert.Equals(t, updo.ID, o.ID)
+						assert.Equals(t, updo.AccountID, o.AccountID)
+						assert.Equals(t, updo.ExpiresAt, o.ExpiresAt)
+						assert.Equals(t, updo.AuthorizationIDs, o.AuthorizationIDs)
+						assert.Equals(t, updo.Identifiers, o.Identifiers)
+						return nil
+					},
+				},
+			}
+		},
+		"ok/permanent-identifier-only": func(t *testing.T) test {
+			now := clock.Now()
+			o := &Order{
+				ID:               "oID",
+				AccountID:        "accID",
+				Status:           StatusReady,
+				ExpiresAt:        now.Add(5 * time.Minute),
+				AuthorizationIDs: []string{"a", "b"},
+				Identifiers: []Identifier{
+					{Type: "dns", Value: "foo.internal"},
+					{Type: "permanent-identifier", Value: "a-permanent-identifier"},
+				},
+			}
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "a-permanent-identifier",
+				},
+				DNSNames: []string{"foo.internal"},
+				ExtraExtensions: []pkix.Extension{
+					{
+						Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 8, 3},
+						Value: []byte("a-permanent-identifier"),
+					},
+				},
+			}
+
+			leaf := &x509.Certificate{
+				Subject: pkix.Name{CommonName: "a-permanent-identifier"},
+				ExtraExtensions: []pkix.Extension{
+					{
+						Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 8, 3},
+						Value: []byte("a-permanent-identifier"),
+					},
+				},
+			}
+			inter := &x509.Certificate{Subject: pkix.Name{CommonName: "inter"}}
+			root := &x509.Certificate{Subject: pkix.Name{CommonName: "root"}}
+
+			return test{
+				o:   o,
+				csr: csr,
+				prov: &MockProvisioner{
+					MauthorizeSign: func(ctx context.Context, token string) ([]provisioner.SignOption, error) {
+						assert.Equals(t, token, "")
+						return nil, nil
+					},
+					MgetOptions: func() *provisioner.Options {
+						return nil
+					},
+				},
+				// TODO(hs): we should work on making the mocks more realistic. Ideally, we should get rid of
+				// the mock entirely, relying on an instances of provisioner, authority and DB (possibly hardest), so
+				// that behavior of the tests is what an actual CA would do. We could gradually phase them out by
+				// using the mocking functions as a wrapper for actual test helpers generated per test case or per
+				// function that's tested.
+				ca: &mockSignAuth{
+					sign: func(_csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) {
+						assert.Equals(t, _csr, csr)
+						return []*x509.Certificate{leaf, inter, root}, nil
+					},
+				},
+				db: &MockDB{
+					MockCreateCertificate: func(ctx context.Context, cert *Certificate) error {
+						cert.ID = "certID"
+						assert.Equals(t, cert.AccountID, o.AccountID)
+						assert.Equals(t, cert.OrderID, o.ID)
+						assert.Equals(t, cert.Leaf, leaf)
+						assert.Equals(t, cert.Intermediates, []*x509.Certificate{inter, root})
+						return nil
+					},
+					MockUpdateOrder: func(ctx context.Context, updo *Order) error {
+						assert.Equals(t, updo.CertificateID, "certID")
+						assert.Equals(t, updo.Status, StatusValid)
+						assert.Equals(t, updo.ID, o.ID)
+						assert.Equals(t, updo.AccountID, o.AccountID)
+						assert.Equals(t, updo.ExpiresAt, o.ExpiresAt)
+						assert.Equals(t, updo.AuthorizationIDs, o.AuthorizationIDs)
+						assert.Equals(t, updo.Identifiers, o.Identifiers)
+						return nil
+					},
+				},
+			}
+		},
 		"ok/new-cert-dns": func(t *testing.T) test {
 			now := clock.Now()
 			o := &Order{