From 1951669e13348e2634ad5772abd982cda03f3015 Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 23 Jun 2020 11:10:45 -0700 Subject: [PATCH 1/6] wip --- acme/order.go | 10 +++++ authority/provisioner/aws.go | 2 + authority/provisioner/azure.go | 3 ++ authority/provisioner/gcp.go | 3 ++ authority/provisioner/jwk.go | 9 +---- authority/provisioner/sign_options.go | 45 ++++++++++++++++++++++ authority/provisioner/sign_options_test.go | 36 +++++++++++++++++ authority/provisioner/x5c.go | 10 +---- authority/tls.go | 4 +- ca/client.go | 6 ++- go.mod | 3 +- go.sum | 11 ++++++ 12 files changed, 122 insertions(+), 20 deletions(-) diff --git a/acme/order.go b/acme/order.go index e3fd6d8f..8642c6d1 100644 --- a/acme/order.go +++ b/acme/order.go @@ -308,6 +308,16 @@ func (o *order) finalize(db nosql.DB, csr *x509.CertificateRequest, auth SignAut } } + if len(csr.IPAddresses) > 0 { + return nil, BadCSRErr(errors.Errorf("CSR contains IP Address SANs, but should only contain DNS Names")) + } + if len(csr.EmailAddresses) > 0 { + return nil, BadCSRErr(errors.Errorf("CSR contains Email Address SANs, but should only contain DNS Names")) + } + if len(csr.URIs) > 0 { + return nil, BadCSRErr(errors.Errorf("CSR contains URI SANs, but should only contain DNS Names")) + } + // Get authorizations from the ACME provisioner. ctx := provisioner.NewContextWithMethod(context.Background(), provisioner.SignMethod) signOps, err := p.AuthorizeSign(ctx, "") diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index 3f2c8873..e426564f 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -287,6 +287,8 @@ func (p *AWS) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er so = append(so, ipAddressesValidator([]net.IP{ net.ParseIP(doc.PrivateIP), })) + so = append(so, emailAddressesValidator(nil)) + so = append(so, urisValidator(nil)) } return append(so, diff --git a/authority/provisioner/azure.go b/authority/provisioner/azure.go index cd1b1b83..8580eced 100644 --- a/authority/provisioner/azure.go +++ b/authority/provisioner/azure.go @@ -284,6 +284,9 @@ func (p *Azure) AuthorizeSign(ctx context.Context, token string) ([]SignOption, // name will work only inside the virtual network so = append(so, commonNameValidator(name)) so = append(so, dnsNamesValidator([]string{name})) + so = append(so, ipAddressesValidator(nil)) + so = append(so, emailAddressesValidator(nil)) + so = append(so, urisValidator(nil)) } return append(so, diff --git a/authority/provisioner/gcp.go b/authority/provisioner/gcp.go index 28c7f51e..507eea1b 100644 --- a/authority/provisioner/gcp.go +++ b/authority/provisioner/gcp.go @@ -228,6 +228,9 @@ func (p *GCP) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er so = append(so, dnsNamesValidator([]string{ dnsName1, dnsName2, })) + so = append(so, ipAddressesValidator(nil)) + so = append(so, emailAddressesValidator(nil)) + so = append(so, urisValidator(nil)) } return append(so, diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 57297f78..2d2d577e 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -8,7 +8,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/cli/crypto/x509util" "github.com/smallstep/cli/jose" ) @@ -152,19 +151,15 @@ func (p *JWK) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er claims.SANs = []string{claims.Subject} } - dnsNames, ips, emails := x509util.SplitSANs(claims.SANs) - return []SignOption{ + return append([]SignOption{ // modifiers / withOptions newProvisionerExtensionOption(TypeJWK, p.Name, p.Key.KeyID), profileDefaultDuration(p.claimer.DefaultTLSCertDuration()), // validators commonNameValidator(claims.Subject), defaultPublicKeyValidator{}, - dnsNamesValidator(dnsNames), - emailAddressesValidator(emails), - ipAddressesValidator(ips), newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), - }, nil + }, sansValidators(claims.SANs)), nil } // AuthorizeRenew returns an error if the renewal is disabled. diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 55ed9d3a..d8ac3ab0 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -7,6 +7,7 @@ import ( "crypto/x509/pkix" "encoding/asn1" "net" + "net/url" "reflect" "time" @@ -195,6 +196,50 @@ func (v emailAddressesValidator) Valid(req *x509.CertificateRequest) error { return nil } +// urisValidator validates the URI SANs of a certificate request. +type urisValidator []*url.URL + +// Valid checks that certificate request IP Addresses match those configured in +// the bootstrap (token) flow. +func (v urisValidator) Valid(req *x509.CertificateRequest) error { + want := make(map[string]bool) + for _, u := range v { + want[u.String()] = true + } + got := make(map[string]bool) + for _, u := range req.URIs { + got[u.String()] = true + } + if !reflect.DeepEqual(want, got) { + return errors.Errorf("URIs claim failed - got %v, want %v", req.URIs, v) + } + return nil +} + +func sansValidators(sans []string) []SignOption { + dnsNames, ips, emails, uris := x509util.SplitSANs(sans) + return []SignOption{dnsNamesValidator(dnsNames), emailAddressesValidator(emails), + ipAddressesValidator(ips), urisValidator(uris)} +} + +// ExtraExtensionsEnforcer enforces only those extra extensions that are strictly +// managed by step-ca. All other "extra extensions" are dropped. +type ExtraExtensionsEnforcer struct{} + +// Enforce removes all extensions except the step provisioner extension, if it +// exists. If the step provisioner extension is not present, then remove all +// extra extensions from the cert. +func (eee ExtraExtensionsEnforcer) Enforce(cert *x509.Certificate) error { + for _, ext := range cert.Extensions { + if ext.Id.Equal(stepOIDProvisioner) { + cert.ExtraExtensions = []pkix.Extension{ext} + return nil + } + } + cert.ExtraExtensions = nil + return nil +} + // profileDefaultDuration is a wrapper against x509util.WithOption to conform // the SignOption interface. type profileDefaultDuration time.Duration diff --git a/authority/provisioner/sign_options_test.go b/authority/provisioner/sign_options_test.go index 829dbbba..6a8d3069 100644 --- a/authority/provisioner/sign_options_test.go +++ b/authority/provisioner/sign_options_test.go @@ -246,6 +246,42 @@ func Test_ipAddressesValidator_Valid(t *testing.T) { } } +func Test_urisValidator_Valid(t *testing.T) { + u1, err := url.Parse("https://ca.smallstep.com") + assert.FatalError(t, err) + u2, err := url.Parse("https://google.com/index.html") + assert.FatalError(t, err) + u3, err := url.Parse("https://foo.bar.baz") + assert.FatalError(t, err) + fu, err := url.Parse("https://unexpected.com") + assert.FatalError(t, err) + + type args struct { + req *x509.CertificateRequest + } + tests := []struct { + name string + v urisValidator + args args + wantErr bool + }{ + {"ok0", []*url.URL{}, args{&x509.CertificateRequest{URIs: []*url.URL{}}}, false}, + {"ok1", []*url.URL{u1}, args{&x509.CertificateRequest{URIs: []*url.URL{u1}}}, false}, + {"ok2", []*url.URL{u1, u2}, args{&x509.CertificateRequest{URIs: []*url.URL{u2, u1}}}, false}, + {"ok3", []*url.URL{u2, u1, u3}, args{&x509.CertificateRequest{URIs: []*url.URL{u3, u2, u1}}}, false}, + {"fail1", []*url.URL{u1}, args{&x509.CertificateRequest{URIs: []*url.URL{u2}}}, true}, + {"fail2", []*url.URL{u1}, args{&x509.CertificateRequest{URIs: []*url.URL{u2, u1}}}, true}, + {"fail3", []*url.URL{u1, u2}, args{&x509.CertificateRequest{URIs: []*url.URL{u1, fu}}}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.v.Valid(tt.args.req); (err != nil) != tt.wantErr { + t.Errorf("urisValidator.Valid() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + func Test_validityValidator_Valid(t *testing.T) { type test struct { cert *x509.Certificate diff --git a/authority/provisioner/x5c.go b/authority/provisioner/x5c.go index c174aa3f..b8a32206 100644 --- a/authority/provisioner/x5c.go +++ b/authority/provisioner/x5c.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - "github.com/smallstep/cli/crypto/x509util" "github.com/smallstep/cli/jose" ) @@ -194,9 +193,7 @@ func (p *X5C) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er claims.SANs = []string{claims.Subject} } - dnsNames, ips, emails := x509util.SplitSANs(claims.SANs) - - return []SignOption{ + return append([]SignOption{ // modifiers / withOptions newProvisionerExtensionOption(TypeX5C, p.Name, ""), profileLimitDuration{p.claimer.DefaultTLSCertDuration(), @@ -204,11 +201,8 @@ func (p *X5C) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er // validators commonNameValidator(claims.Subject), defaultPublicKeyValidator{}, - dnsNamesValidator(dnsNames), - emailAddressesValidator(emails), - ipAddressesValidator(ips), newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), - }, nil + }, sansValidators(claims.SANs)), nil } // AuthorizeRenew returns an error if the renewal is disabled. diff --git a/authority/tls.go b/authority/tls.go index dbfbf96a..ebe25911 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -103,8 +103,8 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti } } - // Certificate modifier after validation - for _, m := range forcedModifiers { + // Certificate modifiers after validation + for _, m := range append(forcedModifiers, provisioner.ExtraExtensionsEnforcer{}) { if err := m.Enforce(leaf.Subject()); err != nil { return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) } diff --git a/ca/client.go b/ca/client.go index ce936655..370126d6 100644 --- a/ca/client.go +++ b/ca/client.go @@ -1066,7 +1066,7 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) return nil, nil, errors.Wrap(err, "error generating key") } - dnsNames, ips, emails := x509util.SplitSANs(claims.SANs) + dnsNames, ips, emails, uris := x509util.SplitSANs(claims.SANs) if claims.Email != "" { emails = append(emails, claims.Email) } @@ -1079,6 +1079,7 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) DNSNames: dnsNames, IPAddresses: ips, EmailAddresses: emails, + URIs: uris, } csr, err := x509.CreateCertificateRequest(rand.Reader, template, pk) @@ -1137,7 +1138,7 @@ func createCertificateRequest(commonName string, sans []string, key crypto.Priva if len(sans) == 0 { sans = []string{commonName} } - dnsNames, ips, emails := x509util.SplitSANs(sans) + dnsNames, ips, emails, uris := x509util.SplitSANs(sans) template := &x509.CertificateRequest{ Subject: pkix.Name{ CommonName: commonName, @@ -1145,6 +1146,7 @@ func createCertificateRequest(commonName string, sans []string, key crypto.Priva DNSNames: dnsNames, IPAddresses: ips, EmailAddresses: emails, + URIs: uris, } csr, err := x509.CreateCertificateRequest(rand.Reader, template, key) if err != nil { diff --git a/go.mod b/go.mod index 49f6c511..eec2babc 100644 --- a/go.mod +++ b/go.mod @@ -27,5 +27,6 @@ require ( gopkg.in/square/go-jose.v2 v2.4.0 ) -//replace github.com/smallstep/cli => ../cli +replace github.com/smallstep/cli => ../cli + //replace github.com/smallstep/nosql => ../nosql diff --git a/go.sum b/go.sum index 1bd57d61..80a44d6b 100644 --- a/go.sum +++ b/go.sum @@ -21,12 +21,16 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/DataDog/zstd v1.4.1 h1:3oxKN3wbHibqx897utPC2LTQU4J+IHWWJO+glkAkpFM= github.com/DataDog/zstd v1.4.1/go.mod h1:1jcaCB/ufaK+sKp1NBhlGmpz41jOoPQ35bpF36t7BBo= +github.com/Masterminds/glide v0.13.2/go.mod h1:STyF5vcenH/rUqTEv+/hBXlSTo7KYwg2oc2f4tzPWic= github.com/Masterminds/goutils v1.1.0 h1:zukEsf/1JZwCMgHiK3GZftabmxiCw4apj3a28RPBiVg= github.com/Masterminds/goutils v1.1.0/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU= +github.com/Masterminds/semver v1.4.2 h1:WBLTQ37jOCzSLtXNdoo8bNM8876KhNqOKvrlGITgsTc= +github.com/Masterminds/semver v1.4.2/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= github.com/Masterminds/semver/v3 v3.0.1 h1:2kKm5lb7dKVrt5TYUiAavE6oFc1cFT0057UVGT+JqLk= github.com/Masterminds/semver/v3 v3.0.1/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs= github.com/Masterminds/sprig/v3 v3.0.0 h1:KSQz7Nb08/3VU9E4ns29dDxcczhOD1q7O1UfM4G3t3g= github.com/Masterminds/sprig/v3 v3.0.0/go.mod h1:NEUY/Qq8Gdm2xgYA+NwJM6wmfdRV9xkh8h/Rld20R0U= +github.com/Masterminds/vcs v1.13.0/go.mod h1:N09YCmOQr6RLxC6UNHzuVwAdodYbbnycGHSmwVJjcKA= github.com/Microsoft/go-winio v0.4.14 h1:+hMXMk01us9KgxGb7ftKQt2Xpf5hH/yky+TDA+qxleU= github.com/Microsoft/go-winio v0.4.14/go.mod h1:qXqCSQ3Xa7+6tgxaGTIe4Kpcdsi+P8jBhyzoq1bpyYA= github.com/OneOfOne/xxhash v1.2.2 h1:KMrpdQIwFcEqXDklaen+P1axHaj9BSKzvpUUfnHldSE= @@ -55,6 +59,7 @@ github.com/bgentry/speakeasy v0.1.0 h1:ByYyxL9InA1OWqxJqqp2A5pYHUrCiAL6K3J+LKSsQ github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs= github.com/bombsimon/wsl/v2 v2.0.0 h1:+Vjcn+/T5lSrO8Bjzhk4v14Un/2UyCA1E3V5j9nwTkQ= github.com/bombsimon/wsl/v2 v2.0.0/go.mod h1:mf25kr/SqFEPhhcxW1+7pxzGlW+hIl/hYTKY95VwV8U= +github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= github.com/census-instrumentation/opencensus-proto v0.2.0/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko= @@ -68,6 +73,7 @@ github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5P github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1 h1:q763qf9huN11kDQavWsoZXJNW3xEE4JJyHa5Q25/sd8= github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= +github.com/codegangsta/cli v1.20.0/go.mod h1:/qJNoX69yVSKu5o4jLyXAENLRyk1uhi7zkbQ3slBdOA= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/bbolt v1.3.3 h1:n6AiVyVRKQFNb6mJlwESEvvLoDyiTzXX7ORAUlkeBdY= github.com/coreos/bbolt v1.3.3/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= @@ -83,6 +89,7 @@ github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e h1:Wf6HqHfScWJN9 github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f h1:lBNOc5arjvs8E5mO2tbpBpLoyyu8B6e44T7hJy6potg= github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f/go.mod h1:E3G3o1h8I7cfcXa63jLwjI0eiQQMgzzUDFVpN/nH/eA= +github.com/corpix/uarand v0.1.1/go.mod h1:SFKZvkcRoLqVRFZ4u25xPmp6m9ktANfbpXZ7SJ0/FNU= github.com/cpuguy83/go-md2man v1.0.10 h1:BSKMNlYxDvnunlTymqtgONjNnaRV1sTpcovwwjF22jk= github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwcJI5acqYI6dE= github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= @@ -273,6 +280,7 @@ github.com/imdario/mergo v0.3.7 h1:Y+UAYTZ7gDEuOfhxKWy+dvb5dRQ6rJjFSdX2HZY1/gI= github.com/imdario/mergo v0.3.7/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM= github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= +github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5imkbgOkpRUYLnmbU7UEFbjtDA2hxJ1ichM= github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k= github.com/jmespath/go-jmespath v0.3.0 h1:OS12ieG61fsCg5+qLJ+SsW9NicxNkg3b25OyT2yCeUc= @@ -290,6 +298,7 @@ github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a h1:FaWFmfWdAUKbSCtOU github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a/go.mod h1:UJSiEoRfvx3hP73CvoARgeLjaIOjybY9vj8PUPPFGeU= github.com/juju/ratelimit v1.0.1/go.mod h1:qapgC/Gy+xNh9UxzV13HGGl/6UXNN+ct+vwSgWNm/qk= github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= +github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8= github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvWXihfKN4Q= github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00= github.com/kisielk/gotool v0.0.0-20161130080628-0de1eaf82fa3/go.mod h1:jxZFDH7ILpTPQTk+E2s+z4CUas9lVNjIuKR4c5/zKgM= @@ -372,6 +381,7 @@ github.com/nbutton23/zxcvbn-go v0.0.0-20180912185939-ae427f1e4c1d h1:AREM5mwr4u1 github.com/nbutton23/zxcvbn-go v0.0.0-20180912185939-ae427f1e4c1d/go.mod h1:o96djdrsSGy3AWPyBgZMAGfxZNfgntdJG+11KU4QvbU= github.com/newrelic/go-agent v2.15.0+incompatible h1:IB0Fy+dClpBq9aEoIrLyQXzU34JyI1xVTanPLB/+jvU= github.com/newrelic/go-agent v2.15.0+incompatible/go.mod h1:a8Fv1b/fYhFSReoTU6HDkTYIMZeSVNffmoS726Y0LzQ= +github.com/ngdinhtoan/glide-cleanup v0.2.0/go.mod h1:UQzsmiDOb8YV3nOsCxK/c9zPpCZVNoHScRE3EO9pVMM= github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U= github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/olekukonko/tablewriter v0.0.4 h1:vHD/YYe1Wolo78koG299f7V/VAS08c6IpCLn+Ejf/w8= @@ -792,6 +802,7 @@ honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM= honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg= +howett.net/plist v0.0.0-20200419221736-3b63eb3a43b5/go.mod h1:vMygbs4qMhSZSc4lCUl2OEE+rDiIIJAIdR4m7MiMcm0= mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed h1:WX1yoOaKQfddO/mLzdV4wptyWgoH/6hwLs7QHTixo0I= mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed/go.mod h1:Xkxe497xwlCKkIaQYRfC7CSLworTXY9RMqwhhCm+8Nc= mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b h1:DxJ5nJdkhDlLok9K6qO+5290kphDJbHOQO1DFFFTeBo= From 3636ba322897a67b105ebb7d461e6e284ad608e5 Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 23 Jun 2020 17:13:39 -0700 Subject: [PATCH 2/6] wip --- authority/provisioner/jwk.go | 5 +++-- authority/provisioner/sign_options.go | 18 ++++++++++++++---- authority/provisioner/x5c.go | 5 +++-- authority/tls.go | 4 ++-- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 2d2d577e..24630ad3 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -151,15 +151,16 @@ func (p *JWK) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er claims.SANs = []string{claims.Subject} } - return append([]SignOption{ + return []SignOption{ // modifiers / withOptions newProvisionerExtensionOption(TypeJWK, p.Name, p.Key.KeyID), profileDefaultDuration(p.claimer.DefaultTLSCertDuration()), // validators commonNameValidator(claims.Subject), + defaultSANsValidator(claims.SANs), defaultPublicKeyValidator{}, newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), - }, sansValidators(claims.SANs)), nil + }, nil } // AuthorizeRenew returns an error if the renewal is disabled. diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index d8ac3ab0..fd82470d 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -216,10 +216,20 @@ func (v urisValidator) Valid(req *x509.CertificateRequest) error { return nil } -func sansValidators(sans []string) []SignOption { - dnsNames, ips, emails, uris := x509util.SplitSANs(sans) - return []SignOption{dnsNamesValidator(dnsNames), emailAddressesValidator(emails), - ipAddressesValidator(ips), urisValidator(uris)} +type defaultSANsValidator []string + +func (v defaultSANsValidator) Valid(req *x509.CertificateRequest) (err error) { + dnsNames, ips, emails, uris := x509util.SplitSANs(v) + if err = dnsNamesValidator(dnsNames).Valid(req); err != nil { + return + } else if err = emailAddressesValidator(emails).Valid(req); err != nil { + return + } else if err = ipAddressesValidator(ips).Valid(req); err != nil { + return + } else if err = urisValidator(uris).Valid(req); err != nil { + return + } + return } // ExtraExtensionsEnforcer enforces only those extra extensions that are strictly diff --git a/authority/provisioner/x5c.go b/authority/provisioner/x5c.go index b8a32206..6f7d0a5f 100644 --- a/authority/provisioner/x5c.go +++ b/authority/provisioner/x5c.go @@ -193,16 +193,17 @@ func (p *X5C) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er claims.SANs = []string{claims.Subject} } - return append([]SignOption{ + return []SignOption{ // modifiers / withOptions newProvisionerExtensionOption(TypeX5C, p.Name, ""), profileLimitDuration{p.claimer.DefaultTLSCertDuration(), claims.chains[0][0].NotBefore, claims.chains[0][0].NotAfter}, // validators commonNameValidator(claims.Subject), + defaultSANsValidator(claims.SANs), defaultPublicKeyValidator{}, newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), - }, sansValidators(claims.SANs)), nil + }, nil } // AuthorizeRenew returns an error if the renewal is disabled. diff --git a/authority/tls.go b/authority/tls.go index ebe25911..551b7e15 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -64,7 +64,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti opts = []interface{}{errs.WithKeyVal("csr", csr), errs.WithKeyVal("signOptions", signOpts)} mods = []x509util.WithOption{withDefaultASN1DN(a.config.AuthorityConfig.Template)} certValidators = []provisioner.CertificateValidator{} - forcedModifiers = []provisioner.CertificateEnforcer{} + forcedModifiers = []provisioner.CertificateEnforcer{provisioner.ExtraExtensionsEnforcer{}} ) // Set backdate with the configured value @@ -104,7 +104,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti } // Certificate modifiers after validation - for _, m := range append(forcedModifiers, provisioner.ExtraExtensionsEnforcer{}) { + for _, m := range forcedModifiers { if err := m.Enforce(leaf.Subject()); err != nil { return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) } From d25e7f64c29433f7abcc2abfaed7fb8f2d7af1bf Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 24 Jun 2020 09:58:40 -0700 Subject: [PATCH 3/6] wip --- acme/order_test.go | 57 ++++++++++ authority/provisioner/sign_options.go | 12 ++- authority/provisioner/sign_options_test.go | 116 ++++++++++++++++++++- authority/tls.go | 2 +- 4 files changed, 181 insertions(+), 6 deletions(-) diff --git a/acme/order_test.go b/acme/order_test.go index 90c8de84..f3894a2a 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509/pkix" "encoding/json" "fmt" + "net" "net/url" "testing" "time" @@ -1056,6 +1057,62 @@ func TestOrderFinalize(t *testing.T) { err: BadCSRErr(errors.Errorf("CSR names do not match identifiers exactly")), } }, + "fail/ready/no-ipAddresses": func(t *testing.T) test { + o, err := newO() + assert.FatalError(t, err) + o.Status = StatusReady + + csr := &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "", + }, + DNSNames: []string{"acme.example.com", "step.example.com"}, + IPAddresses: []net.IP{net.ParseIP("1.1.1.1")}, + } + return test{ + o: o, + csr: csr, + err: BadCSRErr(errors.Errorf("CSR contains IP Address SANs, but should only contain DNS Names")), + } + }, + "fail/ready/no-emailAddresses": func(t *testing.T) test { + o, err := newO() + assert.FatalError(t, err) + o.Status = StatusReady + + csr := &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "", + }, + DNSNames: []string{"acme.example.com", "step.example.com"}, + EmailAddresses: []string{"max@smallstep.com", "mariano@smallstep.com"}, + } + return test{ + o: o, + csr: csr, + err: BadCSRErr(errors.Errorf("CSR contains Email Address SANs, but should only contain DNS Names")), + } + }, + "fail/ready/no-URIs": func(t *testing.T) test { + o, err := newO() + assert.FatalError(t, err) + o.Status = StatusReady + + u, err := url.Parse("https://google.com") + assert.FatalError(t, err) + csr := &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "", + }, + DNSNames: []string{"acme.example.com", "step.example.com"}, + URIs: []*url.URL{u}, + } + return test{ + o: o, + csr: csr, + err: BadCSRErr(errors.Errorf("CSR contains URI SANs, but should only contain DNS Names")), + } + }, "fail/ready/provisioner-auth-sign-error": func(t *testing.T) test { o, err := newO() assert.FatalError(t, err) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index fd82470d..dac0bfb8 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -216,8 +216,12 @@ func (v urisValidator) Valid(req *x509.CertificateRequest) error { return nil } +// defaultsSANsValidator stores a set of SANs to eventually validate 1:1 against +// the SANs in an x509 certificate request. type defaultSANsValidator []string +// Valid verifies that the SANs stored in the validator match 1:1 with those +// requested in the x509 certificate request. func (v defaultSANsValidator) Valid(req *x509.CertificateRequest) (err error) { dnsNames, ips, emails, uris := x509util.SplitSANs(v) if err = dnsNamesValidator(dnsNames).Valid(req); err != nil { @@ -232,15 +236,15 @@ func (v defaultSANsValidator) Valid(req *x509.CertificateRequest) (err error) { return } -// ExtraExtensionsEnforcer enforces only those extra extensions that are strictly +// ExtraExtsEnforcer enforces only those extra extensions that are strictly // managed by step-ca. All other "extra extensions" are dropped. -type ExtraExtensionsEnforcer struct{} +type ExtraExtsEnforcer struct{} // Enforce removes all extensions except the step provisioner extension, if it // exists. If the step provisioner extension is not present, then remove all // extra extensions from the cert. -func (eee ExtraExtensionsEnforcer) Enforce(cert *x509.Certificate) error { - for _, ext := range cert.Extensions { +func (eee ExtraExtsEnforcer) Enforce(cert *x509.Certificate) error { + for _, ext := range cert.ExtraExtensions { if ext.Id.Equal(stepOIDProvisioner) { cert.ExtraExtensions = []pkix.Extension{ext} return nil diff --git a/authority/provisioner/sign_options_test.go b/authority/provisioner/sign_options_test.go index 6a8d3069..62e285e9 100644 --- a/authority/provisioner/sign_options_test.go +++ b/authority/provisioner/sign_options_test.go @@ -251,7 +251,7 @@ func Test_urisValidator_Valid(t *testing.T) { assert.FatalError(t, err) u2, err := url.Parse("https://google.com/index.html") assert.FatalError(t, err) - u3, err := url.Parse("https://foo.bar.baz") + u3, err := url.Parse("urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959") assert.FatalError(t, err) fu, err := url.Parse("https://unexpected.com") assert.FatalError(t, err) @@ -282,6 +282,120 @@ func Test_urisValidator_Valid(t *testing.T) { } } +func Test_defaultSANsValidator_Valid(t *testing.T) { + type test struct { + csr *x509.CertificateRequest + expectedSANs []string + err error + } + tests := map[string]func() test{ + "fail/dnsNamesValidator": func() test { + return test{ + csr: &x509.CertificateRequest{DNSNames: []string{"foo", "bar"}}, + expectedSANs: []string{"foo"}, + err: errors.New("certificate request does not contain the valid DNS names"), + } + }, + "fail/emailAddressesValidator": func() test { + return test{ + csr: &x509.CertificateRequest{EmailAddresses: []string{"max@fx.com", "mariano@fx.com"}}, + expectedSANs: []string{"dcow@fx.com"}, + err: errors.New("certificate request does not contain the valid Email Addresses"), + } + }, + "fail/ipAddressesValidator": func() test { + return test{ + csr: &x509.CertificateRequest{IPAddresses: []net.IP{net.ParseIP("1.1.1.1"), net.ParseIP("127.0.0.1")}}, + expectedSANs: []string{"127.0.0.1"}, + err: errors.New("IP Addresses claim failed"), + } + }, + "fail/urisValidator": func() test { + u1, err := url.Parse("https://google.com") + assert.FatalError(t, err) + u2, err := url.Parse("urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959") + assert.FatalError(t, err) + return test{ + csr: &x509.CertificateRequest{URIs: []*url.URL{u1, u2}}, + expectedSANs: []string{"urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959"}, + err: errors.New("URIs claim failed"), + } + }, + "ok": func() test { + u1, err := url.Parse("https://google.com") + assert.FatalError(t, err) + u2, err := url.Parse("urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959") + assert.FatalError(t, err) + return test{ + csr: &x509.CertificateRequest{ + DNSNames: []string{"foo", "bar"}, + EmailAddresses: []string{"max@fx.com", "mariano@fx.com"}, + IPAddresses: []net.IP{net.ParseIP("1.1.1.1"), net.ParseIP("127.0.0.1")}, + URIs: []*url.URL{u1, u2}, + }, + expectedSANs: []string{"foo", "127.0.0.1", "max@fx.com", "mariano@fx.com", "https://google.com", "1.1.1.1", "bar", "urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959"}, + } + }, + } + for name, run := range tests { + t.Run(name, func(t *testing.T) { + tt := run() + if err := defaultSANsValidator(tt.expectedSANs).Valid(tt.csr); err != nil { + if assert.NotNil(t, tt.err, fmt.Sprintf("expected no error, but got err = %s", err.Error())) { + assert.True(t, strings.Contains(err.Error(), tt.err.Error()), + fmt.Sprintf("want err = %s, but got err = %s", tt.err.Error(), err.Error())) + } + } else { + assert.Nil(t, tt.err, fmt.Sprintf("expected err = %s, but not ", tt.err)) + } + }) + } +} + +func Test_ExtraExtsEnforcer_Enforce(t *testing.T) { + e1 := pkix.Extension{Id: []int{1, 2, 3, 4, 5}, Critical: false, Value: []byte("foo")} + e2 := pkix.Extension{Id: []int{2, 2, 2}, Critical: false, Value: []byte("bar")} + stepExt := pkix.Extension{Id: stepOIDProvisioner, Critical: false, Value: []byte("baz")} + type test struct { + cert *x509.Certificate + check func(*x509.Certificate) + } + tests := map[string]func() test{ + "ok/empty-exts": func() test { + return test{ + cert: &x509.Certificate{}, + check: func(cert *x509.Certificate) { + assert.Equals(t, len(cert.ExtraExtensions), 0) + }, + } + }, + "ok/no-step-provisioner-ext": func() test { + return test{ + cert: &x509.Certificate{ExtraExtensions: []pkix.Extension{e1, e2}}, + check: func(cert *x509.Certificate) { + assert.Equals(t, len(cert.ExtraExtensions), 0) + }, + } + }, + "ok/step-provisioner-ext": func() test { + return test{ + cert: &x509.Certificate{ExtraExtensions: []pkix.Extension{e1, stepExt, e2}}, + check: func(cert *x509.Certificate) { + assert.Equals(t, len(cert.ExtraExtensions), 1) + assert.Equals(t, cert.ExtraExtensions[0], stepExt) + }, + } + }, + } + for name, run := range tests { + t.Run(name, func(t *testing.T) { + tt := run() + ExtraExtsEnforcer{}.Enforce(tt.cert) + tt.check(tt.cert) + }) + } +} + func Test_validityValidator_Valid(t *testing.T) { type test struct { cert *x509.Certificate diff --git a/authority/tls.go b/authority/tls.go index 551b7e15..ef5aa49f 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -64,7 +64,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Opti opts = []interface{}{errs.WithKeyVal("csr", csr), errs.WithKeyVal("signOptions", signOpts)} mods = []x509util.WithOption{withDefaultASN1DN(a.config.AuthorityConfig.Template)} certValidators = []provisioner.CertificateValidator{} - forcedModifiers = []provisioner.CertificateEnforcer{provisioner.ExtraExtensionsEnforcer{}} + forcedModifiers = []provisioner.CertificateEnforcer{provisioner.ExtraExtsEnforcer{}} ) // Set backdate with the configured value From 71d87b4e61c28671f56a22c82691d695facf6152 Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 24 Jun 2020 23:25:15 -0700 Subject: [PATCH 4/6] wip --- authority/authorize_test.go | 2 +- authority/provisioner/aws_test.go | 66 +++++++++++++++------- authority/provisioner/azure_test.go | 29 +++++++++- authority/provisioner/gcp_test.go | 29 +++++++++- authority/provisioner/jwk.go | 2 +- authority/provisioner/jwk_test.go | 51 ++++++++++------- authority/provisioner/sign_options.go | 10 +++- authority/provisioner/sign_options_test.go | 44 ++++++++++++++- authority/provisioner/x5c_test.go | 41 +++++--------- authority/tls_test.go | 49 ++++++++++++---- 10 files changed, 241 insertions(+), 82 deletions(-) diff --git a/authority/authorize_test.go b/authority/authorize_test.go index e4863764..1d7e69ad 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -449,7 +449,7 @@ func TestAuthority_authorizeSign(t *testing.T) { } } else { if assert.Nil(t, tc.err) { - assert.Len(t, 8, got) + assert.Len(t, 6, got) } } }) diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 9e8fc7ad..3c4e3f86 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -10,6 +10,7 @@ import ( "encoding/hex" "encoding/pem" "fmt" + "net" "net/http" "net/url" "strings" @@ -529,7 +530,7 @@ func TestAWS_AuthorizeSign(t *testing.T) { assert.FatalError(t, err) type args struct { - token string + token, cn string } tests := []struct { name string @@ -539,24 +540,24 @@ func TestAWS_AuthorizeSign(t *testing.T) { code int wantErr bool }{ - {"ok", p1, args{t1}, 5, http.StatusOK, false}, - {"ok", p2, args{t2}, 7, http.StatusOK, false}, - {"ok", p2, args{t2Hostname}, 7, http.StatusOK, false}, - {"ok", p2, args{t2PrivateIP}, 7, http.StatusOK, false}, - {"ok", p1, args{t4}, 5, http.StatusOK, false}, - {"fail account", p3, args{t3}, 0, http.StatusUnauthorized, true}, - {"fail token", p1, args{"token"}, 0, http.StatusUnauthorized, true}, - {"fail subject", p1, args{failSubject}, 0, http.StatusUnauthorized, true}, - {"fail issuer", p1, args{failIssuer}, 0, http.StatusUnauthorized, true}, - {"fail audience", p1, args{failAudience}, 0, http.StatusUnauthorized, true}, - {"fail account", p1, args{failAccount}, 0, http.StatusUnauthorized, true}, - {"fail instanceID", p1, args{failInstanceID}, 0, http.StatusUnauthorized, true}, - {"fail privateIP", p1, args{failPrivateIP}, 0, http.StatusUnauthorized, true}, - {"fail region", p1, args{failRegion}, 0, http.StatusUnauthorized, true}, - {"fail exp", p1, args{failExp}, 0, http.StatusUnauthorized, true}, - {"fail nbf", p1, args{failNbf}, 0, http.StatusUnauthorized, true}, - {"fail key", p1, args{failKey}, 0, http.StatusUnauthorized, true}, - {"fail instance age", p2, args{failInstanceAge}, 0, http.StatusUnauthorized, true}, + {"ok", p1, args{t1, "foo.local"}, 5, http.StatusOK, false}, + {"ok", p2, args{t2, "instance-id"}, 9, http.StatusOK, false}, + {"ok", p2, args{t2Hostname, "ip-127-0-0-1.us-west-1.compute.internal"}, 9, http.StatusOK, false}, + {"ok", p2, args{t2PrivateIP, "127.0.0.1"}, 9, http.StatusOK, false}, + {"ok", p1, args{t4, "instance-id"}, 5, http.StatusOK, false}, + {"fail account", p3, args{token: t3}, 0, http.StatusUnauthorized, true}, + {"fail token", p1, args{token: "token"}, 0, http.StatusUnauthorized, true}, + {"fail subject", p1, args{token: failSubject}, 0, http.StatusUnauthorized, true}, + {"fail issuer", p1, args{token: failIssuer}, 0, http.StatusUnauthorized, true}, + {"fail audience", p1, args{token: failAudience}, 0, http.StatusUnauthorized, true}, + {"fail account", p1, args{token: failAccount}, 0, http.StatusUnauthorized, true}, + {"fail instanceID", p1, args{token: failInstanceID}, 0, http.StatusUnauthorized, true}, + {"fail privateIP", p1, args{token: failPrivateIP}, 0, http.StatusUnauthorized, true}, + {"fail region", p1, args{token: failRegion}, 0, http.StatusUnauthorized, true}, + {"fail exp", p1, args{token: failExp}, 0, http.StatusUnauthorized, true}, + {"fail nbf", p1, args{token: failNbf}, 0, http.StatusUnauthorized, true}, + {"fail key", p1, args{token: failKey}, 0, http.StatusUnauthorized, true}, + {"fail instance age", p2, args{token: failInstanceAge}, 0, http.StatusUnauthorized, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -571,6 +572,33 @@ func TestAWS_AuthorizeSign(t *testing.T) { assert.Equals(t, sc.StatusCode(), tt.code) } else { assert.Len(t, tt.wantLen, got) + for _, o := range got { + switch v := o.(type) { + case *provisionerExtensionOption: + assert.Equals(t, v.Type, int(TypeAWS)) + assert.Equals(t, v.Name, tt.aws.GetName()) + assert.Equals(t, v.CredentialID, tt.aws.Accounts[0]) + assert.Len(t, 2, v.KeyValuePairs) + case profileDefaultDuration: + assert.Equals(t, time.Duration(v), tt.aws.claimer.DefaultTLSCertDuration()) + case commonNameValidator: + assert.Equals(t, string(v), tt.args.cn) + case defaultPublicKeyValidator: + case *validityValidator: + assert.Equals(t, v.min, tt.aws.claimer.MinTLSCertDuration()) + assert.Equals(t, v.max, tt.aws.claimer.MaxTLSCertDuration()) + case ipAddressesValidator: + assert.Equals(t, []net.IP(v), []net.IP{net.ParseIP("127.0.0.1")}) + case emailAddressesValidator: + assert.Equals(t, v, nil) + case urisValidator: + assert.Equals(t, v, nil) + case dnsNamesValidator: + assert.Equals(t, []string(v), []string{"ip-127-0-0-1.us-west-1.compute.internal"}) + default: + assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) + } + } } }) } diff --git a/authority/provisioner/azure_test.go b/authority/provisioner/azure_test.go index 6a226e09..500ab2bb 100644 --- a/authority/provisioner/azure_test.go +++ b/authority/provisioner/azure_test.go @@ -432,7 +432,7 @@ func TestAzure_AuthorizeSign(t *testing.T) { wantErr bool }{ {"ok", p1, args{t1}, 4, http.StatusOK, false}, - {"ok", p2, args{t2}, 6, http.StatusOK, false}, + {"ok", p2, args{t2}, 9, http.StatusOK, false}, {"ok", p1, args{t11}, 4, http.StatusOK, false}, {"fail tenant", p3, args{t3}, 0, http.StatusUnauthorized, true}, {"fail resource group", p4, args{t4}, 0, http.StatusUnauthorized, true}, @@ -456,6 +456,33 @@ func TestAzure_AuthorizeSign(t *testing.T) { assert.Equals(t, sc.StatusCode(), tt.code) } else { assert.Len(t, tt.wantLen, got) + for _, o := range got { + switch v := o.(type) { + case *provisionerExtensionOption: + assert.Equals(t, v.Type, int(TypeAzure)) + assert.Equals(t, v.Name, tt.azure.GetName()) + assert.Equals(t, v.CredentialID, tt.azure.TenantID) + assert.Len(t, 0, v.KeyValuePairs) + case profileDefaultDuration: + assert.Equals(t, time.Duration(v), tt.azure.claimer.DefaultTLSCertDuration()) + case commonNameValidator: + assert.Equals(t, string(v), "virtualMachine") + case defaultPublicKeyValidator: + case *validityValidator: + assert.Equals(t, v.min, tt.azure.claimer.MinTLSCertDuration()) + assert.Equals(t, v.max, tt.azure.claimer.MaxTLSCertDuration()) + case ipAddressesValidator: + assert.Equals(t, v, nil) + case emailAddressesValidator: + assert.Equals(t, v, nil) + case urisValidator: + assert.Equals(t, v, nil) + case dnsNamesValidator: + assert.Equals(t, []string(v), []string{"virtualMachine"}) + default: + assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) + } + } } }) } diff --git a/authority/provisioner/gcp_test.go b/authority/provisioner/gcp_test.go index 34a09003..a9c781d2 100644 --- a/authority/provisioner/gcp_test.go +++ b/authority/provisioner/gcp_test.go @@ -516,7 +516,7 @@ func TestGCP_AuthorizeSign(t *testing.T) { wantErr bool }{ {"ok", p1, args{t1}, 4, http.StatusOK, false}, - {"ok", p2, args{t2}, 6, http.StatusOK, false}, + {"ok", p2, args{t2}, 9, http.StatusOK, false}, {"ok", p3, args{t3}, 4, http.StatusOK, false}, {"fail token", p1, args{"token"}, 0, http.StatusUnauthorized, true}, {"fail key", p1, args{failKey}, 0, http.StatusUnauthorized, true}, @@ -545,6 +545,33 @@ func TestGCP_AuthorizeSign(t *testing.T) { assert.Equals(t, sc.StatusCode(), tt.code) } else { assert.Len(t, tt.wantLen, got) + for _, o := range got { + switch v := o.(type) { + case *provisionerExtensionOption: + assert.Equals(t, v.Type, int(TypeGCP)) + assert.Equals(t, v.Name, tt.gcp.GetName()) + assert.Equals(t, v.CredentialID, tt.gcp.ServiceAccounts[0]) + assert.Len(t, 4, v.KeyValuePairs) + case profileDefaultDuration: + assert.Equals(t, time.Duration(v), tt.gcp.claimer.DefaultTLSCertDuration()) + case commonNameSliceValidator: + assert.Equals(t, []string(v), []string{"instance-name", "instance-id", "instance-name.c.project-id.internal", "instance-name.zone.c.project-id.internal"}) + case defaultPublicKeyValidator: + case *validityValidator: + assert.Equals(t, v.min, tt.gcp.claimer.MinTLSCertDuration()) + assert.Equals(t, v.max, tt.gcp.claimer.MaxTLSCertDuration()) + case ipAddressesValidator: + assert.Equals(t, v, nil) + case emailAddressesValidator: + assert.Equals(t, v, nil) + case urisValidator: + assert.Equals(t, v, nil) + case dnsNamesValidator: + assert.Equals(t, []string(v), []string{"instance-name.c.project-id.internal", "instance-name.zone.c.project-id.internal"}) + default: + assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) + } + } } }) } diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 24630ad3..cc513dc6 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -157,8 +157,8 @@ func (p *JWK) AuthorizeSign(ctx context.Context, token string) ([]SignOption, er profileDefaultDuration(p.claimer.DefaultTLSCertDuration()), // validators commonNameValidator(claims.Subject), - defaultSANsValidator(claims.SANs), defaultPublicKeyValidator{}, + defaultSANsValidator(claims.SANs), newValidityValidator(p.claimer.MinTLSCertDuration(), p.claimer.MaxTLSCertDuration()), }, nil } diff --git a/authority/provisioner/jwk_test.go b/authority/provisioner/jwk_test.go index ed97d8f1..5644f49f 100644 --- a/authority/provisioner/jwk_test.go +++ b/authority/provisioner/jwk_test.go @@ -6,7 +6,6 @@ import ( "crypto/rand" "crypto/rsa" "crypto/x509" - "net" "net/http" "strings" "testing" @@ -253,18 +252,36 @@ func TestJWK_AuthorizeSign(t *testing.T) { token string } tests := []struct { - name string - prov *JWK - args args - code int - err error - dns []string - emails []string - ips []net.IP + name string + prov *JWK + args args + code int + err error + sans []string }{ - {name: "fail-signature", prov: p1, args: args{failSig}, code: http.StatusUnauthorized, err: errors.New("jwk.AuthorizeSign: jwk.authorizeToken; error parsing jwk claims: square/go-jose: error in cryptographic primitive")}, - {"ok-sans", p1, args{t1}, http.StatusOK, nil, []string{"foo"}, []string{"max@smallstep.com"}, []net.IP{net.ParseIP("127.0.0.1")}}, - {"ok-no-sans", p1, args{t2}, http.StatusOK, nil, []string{"subject"}, []string{}, []net.IP{}}, + { + name: "fail-signature", + prov: p1, + args: args{failSig}, + code: http.StatusUnauthorized, + err: errors.New("jwk.AuthorizeSign: jwk.authorizeToken; error parsing jwk claims: square/go-jose: error in cryptographic primitive"), + }, + { + name: "ok-sans", + prov: p1, + args: args{t1}, + code: http.StatusOK, + err: nil, + sans: []string{"127.0.0.1", "max@smallstep.com", "foo"}, + }, + { + name: "ok-no-sans", + prov: p1, + args: args{t2}, + code: http.StatusOK, + err: nil, + sans: []string{"subject"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -278,7 +295,7 @@ func TestJWK_AuthorizeSign(t *testing.T) { } } else { if assert.NotNil(t, got) { - assert.Len(t, 8, got) + assert.Len(t, 6, got) for _, o := range got { switch v := o.(type) { case *provisionerExtensionOption: @@ -291,15 +308,11 @@ func TestJWK_AuthorizeSign(t *testing.T) { case commonNameValidator: assert.Equals(t, string(v), "subject") case defaultPublicKeyValidator: - case dnsNamesValidator: - assert.Equals(t, []string(v), tt.dns) - case emailAddressesValidator: - assert.Equals(t, []string(v), tt.emails) - case ipAddressesValidator: - assert.Equals(t, []net.IP(v), tt.ips) case *validityValidator: assert.Equals(t, v.min, tt.prov.claimer.MinTLSCertDuration()) assert.Equals(t, v.max, tt.prov.claimer.MaxTLSCertDuration()) + case defaultSANsValidator: + assert.Equals(t, []string(v), tt.sans) default: assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) } diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index dac0bfb8..00b5c7f4 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -426,7 +426,15 @@ func (o *provisionerExtensionOption) Option(Options) x509util.WithOption { if err != nil { return err } - crt.ExtraExtensions = append(crt.ExtraExtensions, ext) + // NOTE: HACK. + // Prepend the provisioner extension. In the auth.Sign code we will + // force the resulting certificate to only have one extension, the + // first stepOIDProvisioner that is found in the ExtraExtensions. + // A client could pass a csr containing a malicious stepOIDProvisioner + // ExtraExtension. If we were to append (rather than prepend) the correct + // stepOIDProvisioner extension, then the resulting certificate would + // contain the malicious extension, rather than the one applied by step-ca. + crt.ExtraExtensions = append([]pkix.Extension{ext}, crt.ExtraExtensions...) return nil } } diff --git a/authority/provisioner/sign_options_test.go b/authority/provisioner/sign_options_test.go index 62e285e9..3e02853a 100644 --- a/authority/provisioner/sign_options_test.go +++ b/authority/provisioner/sign_options_test.go @@ -356,6 +356,7 @@ func Test_ExtraExtsEnforcer_Enforce(t *testing.T) { e1 := pkix.Extension{Id: []int{1, 2, 3, 4, 5}, Critical: false, Value: []byte("foo")} e2 := pkix.Extension{Id: []int{2, 2, 2}, Critical: false, Value: []byte("bar")} stepExt := pkix.Extension{Id: stepOIDProvisioner, Critical: false, Value: []byte("baz")} + fakeStepExt := pkix.Extension{Id: stepOIDProvisioner, Critical: false, Value: []byte("zap")} type test struct { cert *x509.Certificate check func(*x509.Certificate) @@ -379,7 +380,7 @@ func Test_ExtraExtsEnforcer_Enforce(t *testing.T) { }, "ok/step-provisioner-ext": func() test { return test{ - cert: &x509.Certificate{ExtraExtensions: []pkix.Extension{e1, stepExt, e2}}, + cert: &x509.Certificate{ExtraExtensions: []pkix.Extension{e1, stepExt, fakeStepExt, e2}}, check: func(cert *x509.Certificate) { assert.Equals(t, len(cert.ExtraExtensions), 1) assert.Equals(t, cert.ExtraExtensions[0], stepExt) @@ -668,6 +669,47 @@ func Test_profileDefaultDuration_Option(t *testing.T) { } } +func Test_newProvisionerExtension_Option(t *testing.T) { + type test struct { + cert *x509.Certificate + valid func(*x509.Certificate) + } + tests := map[string]func() test{ + "ok/one-element": func() test { + return test{ + cert: new(x509.Certificate), + valid: func(cert *x509.Certificate) { + if assert.Len(t, 1, cert.ExtraExtensions) { + ext := cert.ExtraExtensions[0] + assert.Equals(t, ext.Id, stepOIDProvisioner) + } + }, + } + }, + "ok/prepend": func() test { + return test{ + cert: &x509.Certificate{ExtraExtensions: []pkix.Extension{{Id: stepOIDProvisioner, Critical: true}, {Id: []int{1, 2, 3}}}}, + valid: func(cert *x509.Certificate) { + if assert.Len(t, 3, cert.ExtraExtensions) { + ext := cert.ExtraExtensions[0] + assert.Equals(t, ext.Id, stepOIDProvisioner) + assert.False(t, ext.Critical) + } + }, + } + }, + } + for name, run := range tests { + t.Run(name, func(t *testing.T) { + tt := run() + prof := &x509util.Leaf{} + prof.SetSubject(tt.cert) + assert.FatalError(t, newProvisionerExtensionOption(TypeJWK, "foo", "bar", "baz", "zap").Option(Options{})(prof)) + tt.valid(prof.Subject()) + }) + } +} + func Test_profileLimitDuration_Option(t *testing.T) { n, fn := mockNow() defer fn() diff --git a/authority/provisioner/x5c_test.go b/authority/provisioner/x5c_test.go index 3ebaeb6b..24f3e71e 100644 --- a/authority/provisioner/x5c_test.go +++ b/authority/provisioner/x5c_test.go @@ -2,7 +2,6 @@ package provisioner import ( "context" - "net" "net/http" "testing" "time" @@ -407,13 +406,11 @@ func TestX5C_AuthorizeSign(t *testing.T) { assert.FatalError(t, err) type test struct { - p *X5C - token string - code int - err error - dns []string - emails []string - ips []net.IP + p *X5C + token string + code int + err error + sans []string } tests := map[string]func(*testing.T) test{ "fail/invalid-token": func(t *testing.T) test { @@ -434,11 +431,9 @@ func TestX5C_AuthorizeSign(t *testing.T) { withX5CHdr(certs)) assert.FatalError(t, err) return test{ - p: p, - token: tok, - dns: []string{"foo"}, - emails: []string{}, - ips: []net.IP{}, + p: p, + token: tok, + sans: []string{"foo"}, } }, "ok/multi-sans": func(t *testing.T) test { @@ -449,11 +444,9 @@ func TestX5C_AuthorizeSign(t *testing.T) { withX5CHdr(certs)) assert.FatalError(t, err) return test{ - p: p, - token: tok, - dns: []string{"foo"}, - emails: []string{"max@smallstep.com"}, - ips: []net.IP{net.ParseIP("127.0.0.1")}, + p: p, + token: tok, + sans: []string{"127.0.0.1", "foo", "max@smallstep.com"}, } }, } @@ -470,7 +463,7 @@ func TestX5C_AuthorizeSign(t *testing.T) { } else { if assert.Nil(t, tc.err) { if assert.NotNil(t, opts) { - tot := 0 + assert.Equals(t, len(opts), 6) for _, o := range opts { switch v := o.(type) { case *provisionerExtensionOption: @@ -487,21 +480,15 @@ func TestX5C_AuthorizeSign(t *testing.T) { case commonNameValidator: assert.Equals(t, string(v), "foo") case defaultPublicKeyValidator: - case dnsNamesValidator: - assert.Equals(t, []string(v), tc.dns) - case emailAddressesValidator: - assert.Equals(t, []string(v), tc.emails) - case ipAddressesValidator: - assert.Equals(t, []net.IP(v), tc.ips) + case defaultSANsValidator: + assert.Equals(t, []string(v), tc.sans) case *validityValidator: assert.Equals(t, v.min, tc.p.claimer.MinTLSCertDuration()) assert.Equals(t, v.max, tc.p.claimer.MaxTLSCertDuration()) default: assert.FatalError(t, errors.Errorf("unexpected sign option of type %T", v)) } - tot++ } - assert.Equals(t, tot, 8) } } } diff --git a/authority/tls_test.go b/authority/tls_test.go index 183c3083..807f970d 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -89,6 +89,17 @@ func getCSR(t *testing.T, priv interface{}, opts ...func(*x509.CertificateReques return csr } +func setExtraExtsCSR(exts []pkix.Extension) func(*x509.CertificateRequest) { + return func(csr *x509.CertificateRequest) { + csr.ExtraExtensions = exts + } +} + +type basicConstraints struct { + IsCA bool `asn1:"optional"` + MaxPathLen int `asn1:"optional,default:-1"` +} + func TestAuthority_Sign(t *testing.T) { pub, priv, err := keys.GenerateDefaultKeyPair() assert.FatalError(t, err) @@ -271,7 +282,16 @@ ZYtQ9Ot36qc= } }, "ok with enforced modifier": func(t *testing.T) *signTest { - csr := getCSR(t, priv) + bcExt := pkix.Extension{} + bcExt.Id = asn1.ObjectIdentifier{2, 5, 29, 19} + bcExt.Critical = false + bcExt.Value, err = asn1.Marshal(basicConstraints{IsCA: true, MaxPathLen: 4}) + assert.FatalError(t, err) + + csr := getCSR(t, priv, setExtraExtsCSR([]pkix.Extension{ + bcExt, + {Id: stepOIDProvisioner, Value: []byte("foo")}, + {Id: []int{1, 1, 1}, Value: []byte("bar")}})) now := time.Now().UTC() enforcedExtraOptions := append(extraOpts, &certificateDurationEnforcer{ NotBefore: now, @@ -347,19 +367,26 @@ ZYtQ9Ot36qc= // Verify Provisioner OID found := 0 for _, ext := range leaf.Extensions { - id := ext.Id.String() - if id != stepOIDProvisioner.String() { - continue + switch { + case ext.Id.Equal(stepOIDProvisioner): + found++ + val := stepProvisionerASN1{} + _, err := asn1.Unmarshal(ext.Value, &val) + assert.FatalError(t, err) + assert.Equals(t, val.Type, provisionerTypeJWK) + assert.Equals(t, val.Name, []byte(p.Name)) + assert.Equals(t, val.CredentialID, []byte(p.Key.KeyID)) + // Basic Constraints + case ext.Id.Equal(asn1.ObjectIdentifier([]int{2, 5, 29, 19})): + val := basicConstraints{} + _, err := asn1.Unmarshal(ext.Value, &val) + assert.FatalError(t, err) + assert.False(t, val.IsCA, false) + assert.Equals(t, val.MaxPathLen, 0) } - found++ - val := stepProvisionerASN1{} - _, err := asn1.Unmarshal(ext.Value, &val) - assert.FatalError(t, err) - assert.Equals(t, val.Type, provisionerTypeJWK) - assert.Equals(t, val.Name, []byte(p.Name)) - assert.Equals(t, val.CredentialID, []byte(p.Key.KeyID)) } assert.Equals(t, found, 1) + assert.Len(t, 6, leaf.Extensions) realIntermediate, err := x509.ParseCertificate(a.x509Issuer.Raw) assert.FatalError(t, err) From 7a7825cfcb5ab529e89d1b5c518d488cb21c5227 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 25 Jun 2020 14:00:25 -0700 Subject: [PATCH 5/6] wip --- go.mod | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go.mod b/go.mod index eec2babc..49f6c511 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,5 @@ require ( gopkg.in/square/go-jose.v2 v2.4.0 ) -replace github.com/smallstep/cli => ../cli - +//replace github.com/smallstep/cli => ../cli //replace github.com/smallstep/nosql => ../nosql From accf1be7e906b0149211382eec8307bc7db73492 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 25 Jun 2020 14:02:24 -0700 Subject: [PATCH 6/6] wip --- authority/provisioner/sign_options.go | 1 - 1 file changed, 1 deletion(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 00b5c7f4..3da08bec 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -426,7 +426,6 @@ func (o *provisionerExtensionOption) Option(Options) x509util.WithOption { if err != nil { return err } - // NOTE: HACK. // Prepend the provisioner extension. In the auth.Sign code we will // force the resulting certificate to only have one extension, the // first stepOIDProvisioner that is found in the ExtraExtensions.