From 503c9f6101bafdbb3991c07181568b2a0ab10a33 Mon Sep 17 00:00:00 2001 From: Oleksandr Kovalchuk Date: Thu, 14 May 2020 13:20:55 +0300 Subject: [PATCH 1/5] Add config option to force CN Add configuration option `forceCN` to ACME provisioner. When this option is set to `true`, provisioner should generate Subject.CommonName for certificate if it was not present in the request. Default value of `false` should keep the existing behavior (do not modify CSR and certificate). Ref: https://github.com/smallstep/certificates/issues/259 --- authority/provisioner/acme.go | 1 + 1 file changed, 1 insertion(+) diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index e414410b..abeedaf1 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -15,6 +15,7 @@ type ACME struct { Type string `json:"type"` Name string `json:"name"` Claims *Claims `json:"claims,omitempty"` + ForceCN bool `json:"forceCN,omitempty"` claimer *Claimer } From 0218018cee90ae5e450b6bf8b12a3622abc09056 Mon Sep 17 00:00:00 2001 From: Oleksandr Kovalchuk Date: Thu, 14 May 2020 13:23:42 +0300 Subject: [PATCH 2/5] Generate Subject if `forceCN` and Subject is empty When `forceCN` is set in provisioner configuration and Subject.CommonName is empty, set Subject.CommonName to the first SAN from the CSR to follow the letsencrypt's boulder behavior. This is done in order to support system which require certificate's Subject field to be non-empty. N.B. certbot does not send Subject in its certificate request and relies on similar behavior of letsencrypt. Closes https://github.com/smallstep/certificates/issues/259 --- acme/order.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/acme/order.go b/acme/order.go index 27e030e9..ba0f3104 100644 --- a/acme/order.go +++ b/acme/order.go @@ -262,6 +262,13 @@ func (o *order) finalize(db nosql.DB, csr *x509.CertificateRequest, auth SignAut if csr.Subject.CommonName != "" { csr.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) } + + // Generate Subject CommonName for supporting `conservative` systems + // which does not accept certificates with empty subject + if csr.Subject.CommonName == "" && p.(*provisioner.ACME).ForceCN { + csr.Subject.CommonName = csr.DNSNames[0] + } + csr.DNSNames = uniqueLowerNames(csr.DNSNames) orderNames := make([]string, len(o.Identifiers)) for i, n := range o.Identifiers { From 322200b7dbce1f8779d0bc6bb2221128275e8ca3 Mon Sep 17 00:00:00 2001 From: Oleksandr Kovalchuk Date: Sun, 17 May 2020 20:23:13 +0300 Subject: [PATCH 3/5] Implement modifier to set CommonName Implement modifier which sets CommonName to the certificate if CommonName is empty and forceCN is set in the config. Replace previous implementation introduced in 0218018cee90ae5e450b6bf8b12a3622abc09056 with new modifier. Closes https://github.com/smallstep/certificates/issues/259 Ref: https://github.com/smallstep/certificates/pull/260#issuecomment-628961322 --- acme/order.go | 7 ------- authority/provisioner/acme.go | 1 + authority/provisioner/sign_options.go | 26 ++++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/acme/order.go b/acme/order.go index ba0f3104..27e030e9 100644 --- a/acme/order.go +++ b/acme/order.go @@ -262,13 +262,6 @@ func (o *order) finalize(db nosql.DB, csr *x509.CertificateRequest, auth SignAut if csr.Subject.CommonName != "" { csr.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) } - - // Generate Subject CommonName for supporting `conservative` systems - // which does not accept certificates with empty subject - if csr.Subject.CommonName == "" && p.(*provisioner.ACME).ForceCN { - csr.Subject.CommonName = csr.DNSNames[0] - } - csr.DNSNames = uniqueLowerNames(csr.DNSNames) orderNames := make([]string, len(o.Identifiers)) for i, n := range o.Identifiers { diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index abeedaf1..95115e6d 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -68,6 +68,7 @@ func (p *ACME) AuthorizeSign(ctx context.Context, token string) ([]SignOption, e return []SignOption{ // modifiers / withOptions newProvisionerExtensionOption(TypeACME, p.Name, ""), + newForceCNOption(p.ForceCN), profileDefaultDuration(p.claimer.DefaultTLSCertDuration()), // validators defaultPublicKeyValidator{}, diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 92572cde..1d88131e 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -316,6 +316,32 @@ type stepProvisionerASN1 struct { KeyValuePairs []string `asn1:"optional,omitempty"` } +type forceCNOption struct { + ForceCN bool +} + +func newForceCNOption(forceCN bool) *forceCNOption { + return &forceCNOption{forceCN} +} + +func (o *forceCNOption) Option(Options) x509util.WithOption { + return func(p x509util.Profile) error { + if !o.ForceCN { + // Forcing CN is disabled, do nothing to certificate + return nil + } + crt := p.Subject() + if crt.Subject.CommonName == "" { + if len(crt.DNSNames) > 0 { + crt.Subject.CommonName = crt.DNSNames[0] + } else { + return errors.New("Cannot force CN, DNSNames is empty") + } + } + return nil + } +} + type provisionerExtensionOption struct { Type int Name string From 893a53793af1883e8fa2132fc64a0955ed845d69 Mon Sep 17 00:00:00 2001 From: Oleksandr Kovalchuk Date: Sun, 17 May 2020 20:27:09 +0300 Subject: [PATCH 4/5] Modify existing tests to accept forceCNOption modifier Modify existing tests to pass with changes introduced in commit 322200b7dbce1f8779d0bc6bb2221128275e8ca3. This is safe to do as tests assert exact length of modifiers, which has changed. --- acme/order_test.go | 6 +++--- authority/provisioner/acme_test.go | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/acme/order_test.go b/acme/order_test.go index b0453754..fa08e3b7 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -1137,7 +1137,7 @@ func TestOrderFinalize(t *testing.T) { csr: csr, sa: &mockSignAuth{ sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) { - assert.Equals(t, len(signOps), 4) + assert.Equals(t, len(signOps), 5) return []*x509.Certificate{crt, inter}, nil }, }, @@ -1186,7 +1186,7 @@ func TestOrderFinalize(t *testing.T) { csr: csr, sa: &mockSignAuth{ sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) { - assert.Equals(t, len(signOps), 4) + assert.Equals(t, len(signOps), 5) return []*x509.Certificate{crt, inter}, nil }, }, @@ -1233,7 +1233,7 @@ func TestOrderFinalize(t *testing.T) { csr: csr, sa: &mockSignAuth{ sign: func(csr *x509.CertificateRequest, pops provisioner.Options, signOps ...provisioner.SignOption) ([]*x509.Certificate, error) { - assert.Equals(t, len(signOps), 4) + assert.Equals(t, len(signOps), 5) return []*x509.Certificate{crt, inter}, nil }, }, diff --git a/authority/provisioner/acme_test.go b/authority/provisioner/acme_test.go index 581f20ed..7b669d8d 100644 --- a/authority/provisioner/acme_test.go +++ b/authority/provisioner/acme_test.go @@ -168,7 +168,7 @@ func TestACME_AuthorizeSign(t *testing.T) { } } else { if assert.Nil(t, tc.err) && assert.NotNil(t, opts) { - assert.Len(t, 4, opts) + assert.Len(t, 5, opts) for _, o := range opts { switch v := o.(type) { case *provisionerExtensionOption: @@ -176,6 +176,8 @@ func TestACME_AuthorizeSign(t *testing.T) { assert.Equals(t, v.Name, tc.p.GetName()) assert.Equals(t, v.CredentialID, "") assert.Len(t, 0, v.KeyValuePairs) + case *forceCNOption: + assert.Equals(t, v.ForceCN, tc.p.ForceCN) case profileDefaultDuration: assert.Equals(t, time.Duration(v), tc.p.claimer.DefaultTLSCertDuration()) case defaultPublicKeyValidator: From 4cd01b6868f8638f5f0ed8e952aa63461b5ea77e Mon Sep 17 00:00:00 2001 From: Oleksandr Kovalchuk Date: Sun, 17 May 2020 20:29:28 +0300 Subject: [PATCH 5/5] Implement tests for forceCNOption modifier Implement unit tests which checks forceCNOption modifier (implemented in 322200b7dbce1f8779d0bc6bb2221128275e8ca3) is not broken and works correctly. Ref: https://github.com/smallstep/certificates/issues/259 --- authority/provisioner/sign_options_test.go | 82 ++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/authority/provisioner/sign_options_test.go b/authority/provisioner/sign_options_test.go index 97d34ea8..2aa6dd05 100644 --- a/authority/provisioner/sign_options_test.go +++ b/authority/provisioner/sign_options_test.go @@ -344,6 +344,88 @@ func Test_validityValidator_Valid(t *testing.T) { } } +func Test_forceCN_Option(t *testing.T) { + type test struct { + so Options + fcn forceCNOption + cert *x509.Certificate + valid func(*x509.Certificate) + err error + } + + tests := map[string]func() test{ + "ok/CN-not-forced": func() test { + return test{ + fcn: forceCNOption{false}, + so: Options{}, + cert: &x509.Certificate{ + Subject: pkix.Name{}, + DNSNames: []string{"acme.example.com", "step.example.com"}, + }, + valid: func(cert *x509.Certificate) { + assert.Equals(t, cert.Subject.CommonName, "") + }, + } + }, + "ok/CN-forced-and-set": func() test { + return test{ + fcn: forceCNOption{true}, + so: Options{}, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "Some Common Name", + }, + DNSNames: []string{"acme.example.com", "step.example.com"}, + }, + valid: func(cert *x509.Certificate) { + assert.Equals(t, cert.Subject.CommonName, "Some Common Name") + }, + } + }, + "ok/CN-forced-and-not-set": func() test { + return test{ + fcn: forceCNOption{true}, + so: Options{}, + cert: &x509.Certificate{ + Subject: pkix.Name{}, + DNSNames: []string{"acme.example.com", "step.example.com"}, + }, + valid: func(cert *x509.Certificate) { + assert.Equals(t, cert.Subject.CommonName, "acme.example.com") + }, + } + }, + "fail/CN-forced-and-empty-DNSNames": func() test { + return test{ + fcn: forceCNOption{true}, + so: Options{}, + cert: &x509.Certificate{ + Subject: pkix.Name{}, + DNSNames: []string{}, + }, + err: errors.New("Cannot force CN, DNSNames is empty"), + } + }, + } + + for name, run := range tests { + t.Run(name, func(t *testing.T) { + tt := run() + prof := &x509util.Leaf{} + prof.SetSubject(tt.cert) + if err := tt.fcn.Option(tt.so)(prof); err != nil { + if assert.NotNil(t, tt.err) { + assert.HasPrefix(t, err.Error(), tt.err.Error()) + } + } else { + if assert.Nil(t, tt.err) { + tt.valid(prof.Subject()) + } + } + }) + } +} + func Test_profileDefaultDuration_Option(t *testing.T) { type test struct { so Options