From 30a6889d1f326e167fec1fe8e21b0dcfeeca3f3b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 20 Mar 2019 17:12:52 -0700 Subject: [PATCH 1/5] Use standard x509 instead of step one. --- authority/tls.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index c52ac1e8..64e218e8 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -14,7 +14,6 @@ import ( "github.com/smallstep/cli/crypto/pemutil" "github.com/smallstep/cli/crypto/tlsutil" "github.com/smallstep/cli/crypto/x509util" - stepx509 "github.com/smallstep/cli/pkg/x509" ) // GetTLSOptions returns the tls options configured. @@ -77,15 +76,14 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti } } - stepCSR, err := stepx509.ParseCertificateRequest(csr.Raw) + stepCSR, err := x509.ParseCertificateRequest(csr.Raw) if err != nil { return nil, nil, &apiError{errors.Wrap(err, "sign: error converting x509 csr to stepx509 csr"), http.StatusInternalServerError, errContext} } issIdentity := a.intermediateIdentity - leaf, err := x509util.NewLeafProfileWithCSR(stepCSR, issIdentity.Crt, - issIdentity.Key, mods...) + leaf, err := x509util.NewLeafProfileWithCSR(stepCSR, issIdentity.Crt, issIdentity.Key, mods...) if err != nil { return nil, nil, &apiError{errors.Wrapf(err, "sign"), http.StatusInternalServerError, errContext} } @@ -130,7 +128,7 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi issIdentity := a.intermediateIdentity // Convert a realx509.Certificate to the step x509 Certificate. - oldCert, err := stepx509.ParseCertificate(ocx.Raw) + oldCert, err := x509.ParseCertificate(ocx.Raw) if err != nil { return nil, nil, &apiError{ errors.Wrap(err, "error converting x509.Certificate to stepx509.Certificate"), @@ -140,7 +138,7 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi now := time.Now().UTC() duration := oldCert.NotAfter.Sub(oldCert.NotBefore) - newCert := &stepx509.Certificate{ + newCert := &x509.Certificate{ PublicKey: oldCert.PublicKey, Issuer: issIdentity.Crt.Subject, Subject: oldCert.Subject, From a3e2b4a552d054cdc8b951a4275635d9be3410c4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 20 Mar 2019 17:36:45 -0700 Subject: [PATCH 2/5] Move certificate check to the right place. --- authority/tls.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 64e218e8..64bd7ebe 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -59,6 +59,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti errContext = context{"csr": csr, "signOptions": signOpts} mods = []x509util.WithOption{withDefaultASN1DN(a.config.AuthorityConfig.Template)} certValidators = []provisioner.CertificateValidator{} + issIdentity = a.intermediateIdentity ) for _, op := range extraOpts { switch k := op.(type) { @@ -76,18 +77,22 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti } } - stepCSR, err := x509.ParseCertificateRequest(csr.Raw) - if err != nil { - return nil, nil, &apiError{errors.Wrap(err, "sign: error converting x509 csr to stepx509 csr"), - http.StatusInternalServerError, errContext} + if err := csr.CheckSignature(); err != nil { + return nil, nil, &apiError{errors.Wrap(err, "sign: invalid certificate request"), + http.StatusBadRequest, errContext} } - issIdentity := a.intermediateIdentity - leaf, err := x509util.NewLeafProfileWithCSR(stepCSR, issIdentity.Crt, issIdentity.Key, mods...) + leaf, err := x509util.NewLeafProfileWithCSR(csr, issIdentity.Crt, issIdentity.Key, mods...) if err != nil { return nil, nil, &apiError{errors.Wrapf(err, "sign"), http.StatusInternalServerError, errContext} } + for _, v := range certValidators { + if err := v.Valid(leaf.Subject()); err != nil { + return nil, nil, &apiError{errors.Wrap(err, "sign"), http.StatusUnauthorized, errContext} + } + } + crtBytes, err := leaf.CreateCertificate() if err != nil { return nil, nil, &apiError{errors.Wrap(err, "sign: error creating new leaf certificate"), @@ -100,13 +105,6 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti http.StatusInternalServerError, errContext} } - // FIXME: This should be before creating the certificate. - for _, v := range certValidators { - if err := v.Valid(serverCert); err != nil { - return nil, nil, &apiError{errors.Wrap(err, "sign"), http.StatusUnauthorized, errContext} - } - } - caCert, err := x509.ParseCertificate(issIdentity.Crt.Raw) if err != nil { return nil, nil, &apiError{errors.Wrap(err, "sign: error parsing intermediate certificate"), From b9530909a452d56175b41c2adbb8ac07f2627348 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 20 Mar 2019 17:41:37 -0700 Subject: [PATCH 3/5] Fix tests. --- authority/tls_test.go | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/authority/tls_test.go b/authority/tls_test.go index 47ac7966..c9f80aff 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -19,7 +19,6 @@ import ( "github.com/smallstep/cli/crypto/tlsutil" "github.com/smallstep/cli/crypto/x509util" "github.com/smallstep/cli/jose" - stepx509 "github.com/smallstep/cli/pkg/x509" ) var ( @@ -124,20 +123,6 @@ func TestSign(t *testing.T) { }, } }, - "fail convert csr to step format": func(t *testing.T) *signTest { - csr := getCSR(t, priv) - csr.Raw = []byte("foo") - return &signTest{ - auth: a, - csr: csr, - extraOpts: extraOpts, - signOpts: signOpts, - err: &apiError{errors.New("sign: error converting x509 csr to stepx509 csr"), - http.StatusInternalServerError, - context{"csr": csr, "signOptions": signOpts}, - }, - } - }, "fail merge default ASN1DN": func(t *testing.T) *signTest { _a := testAuthority(t) _a.config.AuthorityConfig.Template = nil @@ -335,13 +320,6 @@ func TestRenew(t *testing.T) { err *apiError } tests := map[string]func() (*renewTest, error){ - "fail-conversion-stepx509": func() (*renewTest, error) { - return &renewTest{ - crt: &x509.Certificate{Raw: []byte("foo")}, - err: &apiError{errors.New("error converting x509.Certificate to stepx509.Certificate"), - http.StatusInternalServerError, context{}}, - }, nil - }, "fail-create-cert": func() (*renewTest, error) { _a := testAuthority(t) _a.intermediateIdentity.Key = nil @@ -373,7 +351,7 @@ func TestRenew(t *testing.T) { assert.FatalError(t, err) newRootBytes, err := newRootProfile.CreateCertificate() assert.FatalError(t, err) - newRootCrt, err := stepx509.ParseCertificate(newRootBytes) + newRootCrt, err := x509.ParseCertificate(newRootBytes) assert.FatalError(t, err) newIntermediateProfile, err := x509util.NewIntermediateProfile("new-intermediate", @@ -381,7 +359,7 @@ func TestRenew(t *testing.T) { assert.FatalError(t, err) newIntermediateBytes, err := newIntermediateProfile.CreateCertificate() assert.FatalError(t, err) - newIntermediateCrt, err := stepx509.ParseCertificate(newIntermediateBytes) + newIntermediateCrt, err := x509.ParseCertificate(newIntermediateBytes) assert.FatalError(t, err) _a := testAuthority(t) From da7360e445a2d0e4fc8db73cc7398bb12fab09fe Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 20 Mar 2019 17:44:59 -0700 Subject: [PATCH 4/5] Use x509util-real-x509 branch of cli --- Gopkg.lock | 7 +++---- Gopkg.toml | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index a11d0292..92e72495 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -276,8 +276,8 @@ revision = "de77670473b5492f5d0bce155b5c01534c2d13f7" [[projects]] - branch = "master" - digest = "1:a11fa27b1cebc2aa3650bd2086aeadf1e2aaf1f4b646895893b80260b17a19d2" + branch = "x509util-real-x509" + digest = "1:8b36444f30009b5e124a3ac48b353558024a95c3fccdf3e6bb557a091e67342b" name = "github.com/smallstep/cli" packages = [ "command", @@ -298,7 +298,7 @@ "utils", ] pruneopts = "UT" - revision = "68ac9850f47f4cfe0045f1444f3f23404e2237ba" + revision = "de740bc707a264406a455c405238788d1f9f0666" [[projects]] branch = "master" @@ -570,7 +570,6 @@ "github.com/smallstep/cli/crypto/x509util", "github.com/smallstep/cli/errs", "github.com/smallstep/cli/jose", - "github.com/smallstep/cli/pkg/x509", "github.com/smallstep/cli/token", "github.com/smallstep/cli/token/provision", "github.com/smallstep/cli/usage", diff --git a/Gopkg.toml b/Gopkg.toml index 0e564d1f..a0e088a0 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -45,7 +45,7 @@ required = [ name = "github.com/go-chi/chi" [[override]] - branch = "master" + branch = "x509util-real-x509" name = "github.com/smallstep/cli" [prune] From 8c8547bf652c78f15ff5e9659d4e05c67e1bba59 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 20 Mar 2019 18:11:45 -0700 Subject: [PATCH 5/5] Remove unnecessary parse and improve tests. --- authority/tls.go | 13 ++----------- authority/tls_test.go | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 64bd7ebe..0b089194 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -116,24 +116,15 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti // Renew creates a new Certificate identical to the old certificate, except // with a validity window that begins 'now'. -func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certificate, error) { +func (a *Authority) Renew(oldCert *x509.Certificate) (*x509.Certificate, *x509.Certificate, error) { // Check step provisioner extensions - if err := a.authorizeRenewal(ocx); err != nil { + if err := a.authorizeRenewal(oldCert); err != nil { return nil, nil, err } // Issuer issIdentity := a.intermediateIdentity - // Convert a realx509.Certificate to the step x509 Certificate. - oldCert, err := x509.ParseCertificate(ocx.Raw) - if err != nil { - return nil, nil, &apiError{ - errors.Wrap(err, "error converting x509.Certificate to stepx509.Certificate"), - http.StatusInternalServerError, context{}, - } - } - now := time.Now().UTC() duration := oldCert.NotAfter.Sub(oldCert.NotBefore) newCert := &x509.Certificate{ diff --git a/authority/tls_test.go b/authority/tls_test.go index c9f80aff..43b35237 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -109,6 +109,20 @@ func TestSign(t *testing.T) { err *apiError } tests := map[string]func(*testing.T) *signTest{ + "fail invalid signature": func(t *testing.T) *signTest { + csr := getCSR(t, priv) + csr.Signature = []byte("foo") + return &signTest{ + auth: a, + csr: csr, + extraOpts: extraOpts, + signOpts: signOpts, + err: &apiError{errors.New("sign: invalid certificate request"), + http.StatusBadRequest, + context{"csr": csr, "signOptions": signOpts}, + }, + } + }, "fail invalid extra option": func(t *testing.T) *signTest { csr := getCSR(t, priv) csr.Raw = []byte("foo")