From f0683c2e0a5755926a7e17402b57c25fb6f99edb Mon Sep 17 00:00:00 2001 From: max furman Date: Wed, 30 Jan 2019 17:36:42 -0600 Subject: [PATCH 1/7] Enable signing certificates with custom SANs * validate against SANs in token. must be 1:1 equivalent. --- Gopkg.lock | 13 +++++------- authority/authorize.go | 38 ++++++++++++++++++++++++++++++++--- authority/claims.go | 45 ++++++++++++++++++++++++------------------ authority/tls.go | 22 +++++++++------------ ca/client.go | 14 ++++++------- 5 files changed, 81 insertions(+), 51 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 451faf0f..b8b3dcb3 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -18,12 +18,9 @@ [[projects]] branch = "master" - digest = "1:5f3371d6e432e3c4e0ceabb14b5c0679827ce8b96bbd963fa923977782e8e99b" + digest = "1:454adc7f974228ff789428b6dc098638c57a64aa0718f0bd61e53d3cd39d7a75" name = "github.com/chzyer/readline" - packages = [ - ".", - "runes", - ] + packages = ["."] pruneopts = "UT" revision = "2972be24d48e78746da79ba8e24e8b488c9880de" @@ -214,8 +211,8 @@ revision = "de77670473b5492f5d0bce155b5c01534c2d13f7" [[projects]] - branch = "master" - digest = "1:9e42b7ce7fce33796a870bbbd4e5fdf7c25f3a243ca355d7d9be52f88e3526d5" + branch = "sans" + digest = "1:84a773da390eabc9a292221bbc2c16653093f8eb805a1b16f738ef3cd88df701" name = "github.com/smallstep/cli" packages = [ "command", @@ -234,7 +231,7 @@ "utils", ] pruneopts = "UT" - revision = "a633fd7f7eff2e8ba4026189241a60968b3767d6" + revision = "a1df67d396f730ff0e7bf1916cd27ae8d5255236" [[projects]] branch = "master" diff --git a/authority/authorize.go b/authority/authorize.go index 1c6fc65d..cfd52bb7 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -3,6 +3,7 @@ package authority import ( "crypto/x509" "encoding/asn1" + "net" "net/http" "net/url" "time" @@ -16,6 +17,12 @@ type idUsed struct { Subject string `json:"sub,omitempty"` } +// Claims extends jwt.Claims with step attributes. +type Claims struct { + jwt.Claims + SANS []string `json:"sans,omitempty"` +} + // matchesAudience returns true if A and B share at least one element. func matchesAudience(as, bs []string) bool { if len(bs) == 0 || len(as) == 0 { @@ -48,7 +55,7 @@ func stripPort(rawurl string) string { func (a *Authority) Authorize(ott string) ([]interface{}, error) { var ( errContext = map[string]interface{}{"ott": ott} - claims = jwt.Claims{} + claims = Claims{} ) // Validate payload @@ -113,10 +120,15 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { http.StatusUnauthorized, errContext} } + dnsNames, ips := SplitSANS(claims.SANS) + if err != nil { + return nil, err + } + signOps := []interface{}{ &commonNameClaim{claims.Subject}, - &dnsNamesClaim{claims.Subject}, - &ipAddressesClaim{claims.Subject}, + &dnsNamesClaim{dnsNames}, + &ipAddressesClaim{ips}, p, } @@ -132,6 +144,26 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { return signOps, nil } +// SplitSANS splits a slice of Subject Alternative Names into slices of +// IP Addresses and DNS Names. If an element is not an IP address, then it +// is bucketed as a DNS Name. +func SplitSANS(sans []string) (dnsNames []string, ips []net.IP) { + dnsNames = []string{} + ips = []net.IP{} + if sans == nil { + return + } + for _, san := range sans { + if ip := net.ParseIP(san); ip != nil { + ips = append(ips, ip) + } else { + // If not IP then assume DNSName. + dnsNames = append(dnsNames, san) + } + } + return +} + // authorizeRenewal tries to locate the step provisioner extension, and checks // if for the configured provisioner, the renewal is enabled or not. If the // extra extension cannot be found, authorize the renewal by default. diff --git a/authority/claims.go b/authority/claims.go index 4dbea684..2af4d358 100644 --- a/authority/claims.go +++ b/authority/claims.go @@ -1,7 +1,10 @@ package authority import ( + "bytes" + "fmt" "net" + "sort" "time" "github.com/pkg/errors" @@ -42,43 +45,47 @@ func (c *commonNameClaim) Valid(crt *x509.Certificate) error { } type dnsNamesClaim struct { - name string + names []string } // Valid checks that certificate request common name matches the one configured. func (c *dnsNamesClaim) Valid(crt *x509.Certificate) error { - if len(crt.DNSNames) == 0 { - return nil + sort.Strings(c.names) + sort.Strings(crt.DNSNames) + if len(c.names) != len(crt.DNSNames) { + fmt.Printf("len(c.names) = %+v, len(crt.DNSNames) = %+v\n", len(c.names), len(crt.DNSNames)) + return errors.Errorf("DNS names claim failed - got %s, want %s", crt.DNSNames, c.names) } - for _, name := range crt.DNSNames { - if name != c.name { - return errors.Errorf("DNS names claim failed - got %s, want %s", name, c.name) + for i := range c.names { + if c.names[i] != crt.DNSNames[i] { + fmt.Printf("c.names[i] = %+v, crt.DNSNames[i] = %+v\n", c.names[i], crt.DNSNames[i]) + return errors.Errorf("DNS names claim failed - got %s, want %s", crt.DNSNames, c.names) } } return nil } type ipAddressesClaim struct { - name string + ips []net.IP } // Valid checks that certificate request common name matches the one configured. func (c *ipAddressesClaim) Valid(crt *x509.Certificate) error { - if len(crt.IPAddresses) == 0 { - return nil + if len(c.ips) != len(crt.IPAddresses) { + return errors.Errorf("IP Addresses claim failed - got %v, want %v", crt.IPAddresses, c.ips) } - - // If it's an IP validate that only that ip is in IP addresses - if requestedIP := net.ParseIP(c.name); requestedIP != nil { - for _, ip := range crt.IPAddresses { - if !ip.Equal(requestedIP) { - return errors.Errorf("IP addresses claim failed - got %s, want %s", ip, requestedIP) - } + sort.Slice(c.ips, func(i, j int) bool { + return bytes.Compare(c.ips[i], c.ips[j]) < 0 + }) + sort.Slice(crt.IPAddresses, func(i, j int) bool { + return bytes.Compare(crt.IPAddresses[i], crt.IPAddresses[j]) < 0 + }) + for i := range c.ips { + if !c.ips[i].Equal(crt.IPAddresses[i]) { + return errors.Errorf("IP Addresses claim failed - got %v, want %v", crt.IPAddresses[i], c.ips[i]) } - return nil } - - return errors.Errorf("IP addresses claim failed - got %v, want none", crt.IPAddresses) + return nil } // certTemporalClaim validates the certificate temporal validity settings. diff --git a/authority/tls.go b/authority/tls.go index 750ced60..6ca433ff 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -129,10 +129,6 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts SignOptions, ext return nil, nil, &apiError{errors.Wrap(err, "sign: error converting x509 csr to stepx509 csr"), http.StatusInternalServerError, errContext} } - // DNSNames and IPAddresses are validated but to avoid duplications we will - // clean them as x509util.NewLeafProfileWithCSR will set the right values. - stepCSR.DNSNames = nil - stepCSR.IPAddresses = nil issIdentity := a.intermediateIdentity leaf, err := x509util.NewLeafProfileWithCSR(stepCSR, issIdentity.Crt, @@ -201,15 +197,15 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi ExtKeyUsage: oldCert.ExtKeyUsage, UnknownExtKeyUsage: oldCert.UnknownExtKeyUsage, BasicConstraintsValid: oldCert.BasicConstraintsValid, - IsCA: oldCert.IsCA, - MaxPathLen: oldCert.MaxPathLen, - MaxPathLenZero: oldCert.MaxPathLenZero, - OCSPServer: oldCert.OCSPServer, - IssuingCertificateURL: oldCert.IssuingCertificateURL, - DNSNames: oldCert.DNSNames, - EmailAddresses: oldCert.EmailAddresses, - IPAddresses: oldCert.IPAddresses, - URIs: oldCert.URIs, + IsCA: oldCert.IsCA, + MaxPathLen: oldCert.MaxPathLen, + MaxPathLenZero: oldCert.MaxPathLenZero, + OCSPServer: oldCert.OCSPServer, + IssuingCertificateURL: oldCert.IssuingCertificateURL, + DNSNames: oldCert.DNSNames, + EmailAddresses: oldCert.EmailAddresses, + IPAddresses: oldCert.IPAddresses, + URIs: oldCert.URIs, PermittedDNSDomainsCritical: oldCert.PermittedDNSDomainsCritical, PermittedDNSDomains: oldCert.PermittedDNSDomains, ExcludedDNSDomains: oldCert.ExcludedDNSDomains, diff --git a/ca/client.go b/ca/client.go index e138698f..f6eea62d 100644 --- a/ca/client.go +++ b/ca/client.go @@ -15,7 +15,6 @@ import ( "encoding/pem" "io" "io/ioutil" - "net" "net/http" "net/url" "strconv" @@ -23,6 +22,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/api" + "github.com/smallstep/certificates/authority" "gopkg.in/square/go-jose.v2/jwt" ) @@ -442,7 +442,7 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) if err != nil { return nil, nil, errors.Wrap(err, "error parsing ott") } - var claims jwt.Claims + var claims authority.Claims if err := token.UnsafeClaimsWithoutVerification(&claims); err != nil { return nil, nil, errors.Wrap(err, "error parsing ott") } @@ -452,17 +452,15 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) return nil, nil, errors.Wrap(err, "error generating key") } + dnsNames, ips := authority.SplitSANS(claims.SANS) + template := &x509.CertificateRequest{ Subject: pkix.Name{ CommonName: claims.Subject, }, SignatureAlgorithm: x509.ECDSAWithSHA256, - } - - if ip := net.ParseIP(claims.Subject); ip != nil { - template.IPAddresses = append(template.IPAddresses, ip) - } else { - template.DNSNames = append(template.DNSNames, claims.Subject) + DNSNames: dnsNames, + IPAddresses: ips, } csr, err := x509.CreateCertificateRequest(rand.Reader, template, pk) From e6e8443f3c262a6a939fedc13afda4acb6e38289 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 31 Jan 2019 11:20:21 -0600 Subject: [PATCH 2/7] allow multiple identical SANs in cert --- Gopkg.lock | 2 +- Gopkg.toml | 2 +- authority/claims.go | 44 +++++++++++++++++----------------------- authority/claims_test.go | 42 ++++++++++++++++++++++---------------- authority/tls.go | 18 ++++++++-------- ca/bootstrap_test.go | 2 ++ ca/ca_test.go | 40 +++++++++++++++++++++++------------- ca/tls_test.go | 20 +++++++++++------- 8 files changed, 95 insertions(+), 75 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index b8b3dcb3..6293e4c3 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -231,7 +231,7 @@ "utils", ] pruneopts = "UT" - revision = "a1df67d396f730ff0e7bf1916cd27ae8d5255236" + revision = "49d4a4c26c802e83c5ed160abdd5babab1c9b5c6" [[projects]] branch = "master" diff --git a/Gopkg.toml b/Gopkg.toml index 0e564d1f..0ed06849 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -45,7 +45,7 @@ required = [ name = "github.com/go-chi/chi" [[override]] - branch = "master" + branch = "sans" name = "github.com/smallstep/cli" [prune] diff --git a/authority/claims.go b/authority/claims.go index 2af4d358..d2c45882 100644 --- a/authority/claims.go +++ b/authority/claims.go @@ -1,10 +1,8 @@ package authority import ( - "bytes" - "fmt" "net" - "sort" + "reflect" "time" "github.com/pkg/errors" @@ -50,17 +48,16 @@ type dnsNamesClaim struct { // Valid checks that certificate request common name matches the one configured. func (c *dnsNamesClaim) Valid(crt *x509.Certificate) error { - sort.Strings(c.names) - sort.Strings(crt.DNSNames) - if len(c.names) != len(crt.DNSNames) { - fmt.Printf("len(c.names) = %+v, len(crt.DNSNames) = %+v\n", len(c.names), len(crt.DNSNames)) - return errors.Errorf("DNS names claim failed - got %s, want %s", crt.DNSNames, c.names) + tokMap := make(map[string]int) + for _, e := range c.names { + tokMap[e] = 1 } - for i := range c.names { - if c.names[i] != crt.DNSNames[i] { - fmt.Printf("c.names[i] = %+v, crt.DNSNames[i] = %+v\n", c.names[i], crt.DNSNames[i]) - return errors.Errorf("DNS names claim failed - got %s, want %s", crt.DNSNames, c.names) - } + crtMap := make(map[string]int) + for _, e := range crt.DNSNames { + crtMap[e] = 1 + } + if !reflect.DeepEqual(tokMap, crtMap) { + return errors.Errorf("DNS names claim failed - got %s, want %s", crt.DNSNames, c.names) } return nil } @@ -71,19 +68,16 @@ type ipAddressesClaim struct { // Valid checks that certificate request common name matches the one configured. func (c *ipAddressesClaim) Valid(crt *x509.Certificate) error { - if len(c.ips) != len(crt.IPAddresses) { - return errors.Errorf("IP Addresses claim failed - got %v, want %v", crt.IPAddresses, c.ips) + tokMap := make(map[string]int) + for _, e := range c.ips { + tokMap[e.String()] = 1 } - sort.Slice(c.ips, func(i, j int) bool { - return bytes.Compare(c.ips[i], c.ips[j]) < 0 - }) - sort.Slice(crt.IPAddresses, func(i, j int) bool { - return bytes.Compare(crt.IPAddresses[i], crt.IPAddresses[j]) < 0 - }) - for i := range c.ips { - if !c.ips[i].Equal(crt.IPAddresses[i]) { - return errors.Errorf("IP Addresses claim failed - got %v, want %v", crt.IPAddresses[i], c.ips[i]) - } + crtMap := make(map[string]int) + for _, e := range crt.IPAddresses { + crtMap[e.String()] = 1 + } + if !reflect.DeepEqual(tokMap, crtMap) { + return errors.Errorf("IP Addresses claim failed - got %v, want %v", crt.IPAddresses, c.ips) } return nil } diff --git a/authority/claims_test.go b/authority/claims_test.go index 02a327cc..d9c9d768 100644 --- a/authority/claims_test.go +++ b/authority/claims_test.go @@ -52,23 +52,28 @@ func TestIPAddressesClaim_Valid(t *testing.T) { crt *x509.Certificate err error }{ - "unexpected-ip": { - iac: &ipAddressesClaim{name: "127.0.0.1"}, + "unexpected-ip-in-crt": { + iac: &ipAddressesClaim{ips: []net.IP{net.ParseIP("127.0.0.1")}}, crt: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("1.1.1.1")}}, - err: errors.New("IP addresses claim failed - got 1.1.1.1, want 127.0.0.1"), + err: errors.New("IP Addresses claim failed - got [127.0.0.1 1.1.1.1], want [127.0.0.1]"), + }, + "missing-ip-in-crt": { + iac: &ipAddressesClaim{ips: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("1.1.1.1")}}, + crt: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}}, + err: errors.New("IP Addresses claim failed - got [127.0.0.1], want [127.0.0.1 1.1.1.1]"), }, "invalid-matcher-nonempty-ips": { - iac: &ipAddressesClaim{name: "invalid"}, + iac: &ipAddressesClaim{ips: []net.IP{}}, crt: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}}, - err: errors.New("IP addresses claim failed - got [127.0.0.1], want none"), + err: errors.New("IP Addresses claim failed - got [127.0.0.1], want []"), }, "ok": { - iac: &ipAddressesClaim{name: "127.0.0.1"}, + iac: &ipAddressesClaim{ips: []net.IP{net.ParseIP("127.0.0.1")}}, crt: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}}, }, - "ok-empty-ips": { - iac: &ipAddressesClaim{name: "127.0.0.1"}, - crt: &x509.Certificate{IPAddresses: []net.IP{}}, + "ok-multiple-identical-ip-entries": { + iac: &ipAddressesClaim{ips: []net.IP{net.ParseIP("127.0.0.1")}}, + crt: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.1")}}, }, } @@ -92,21 +97,22 @@ func TestDNSNamesClaim_Valid(t *testing.T) { crt *x509.Certificate err error }{ - "wrong-dns-name": { - dnc: &dnsNamesClaim{name: "foo"}, + "unexpected-dns-name-in-crt": { + dnc: &dnsNamesClaim{names: []string{"foo"}}, crt: &x509.Certificate{DNSNames: []string{"foo", "bar"}}, - err: errors.New("DNS names claim failed - got bar, want foo"), + err: errors.New("DNS names claim failed - got [foo bar], want [foo]"), }, "ok": { - dnc: &dnsNamesClaim{name: "foo"}, - crt: &x509.Certificate{DNSNames: []string{"foo"}}, + dnc: &dnsNamesClaim{names: []string{"foo", "bar"}}, + crt: &x509.Certificate{DNSNames: []string{"bar", "foo"}}, }, - "ok-empty-dnsNames": { - dnc: &dnsNamesClaim{"foo"}, - crt: &x509.Certificate{}, + "missing-dns-name-in-crt": { + dnc: &dnsNamesClaim{names: []string{"foo", "bar"}}, + crt: &x509.Certificate{DNSNames: []string{"foo"}}, + err: errors.New("DNS names claim failed - got [foo], want [foo bar]"), }, "ok-multiple-identical-dns-entries": { - dnc: &dnsNamesClaim{name: "foo"}, + dnc: &dnsNamesClaim{names: []string{"foo"}}, crt: &x509.Certificate{DNSNames: []string{"foo", "foo", "foo"}}, }, } diff --git a/authority/tls.go b/authority/tls.go index 6ca433ff..bd086bee 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -197,15 +197,15 @@ func (a *Authority) Renew(ocx *x509.Certificate) (*x509.Certificate, *x509.Certi ExtKeyUsage: oldCert.ExtKeyUsage, UnknownExtKeyUsage: oldCert.UnknownExtKeyUsage, BasicConstraintsValid: oldCert.BasicConstraintsValid, - IsCA: oldCert.IsCA, - MaxPathLen: oldCert.MaxPathLen, - MaxPathLenZero: oldCert.MaxPathLenZero, - OCSPServer: oldCert.OCSPServer, - IssuingCertificateURL: oldCert.IssuingCertificateURL, - DNSNames: oldCert.DNSNames, - EmailAddresses: oldCert.EmailAddresses, - IPAddresses: oldCert.IPAddresses, - URIs: oldCert.URIs, + IsCA: oldCert.IsCA, + MaxPathLen: oldCert.MaxPathLen, + MaxPathLenZero: oldCert.MaxPathLenZero, + OCSPServer: oldCert.OCSPServer, + IssuingCertificateURL: oldCert.IssuingCertificateURL, + DNSNames: oldCert.DNSNames, + EmailAddresses: oldCert.EmailAddresses, + IPAddresses: oldCert.IPAddresses, + URIs: oldCert.URIs, PermittedDNSDomainsCritical: oldCert.PermittedDNSDomainsCritical, PermittedDNSDomains: oldCert.PermittedDNSDomains, ExcludedDNSDomains: oldCert.ExcludedDNSDomains, diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 6a0a7159..540b1b20 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -94,6 +94,7 @@ func generateBootstrapToken(ca, subject, sha string) string { cl := struct { SHA string `json:"sha"` jwt.Claims + SANS []string `json:"sans"` }{ SHA: sha, Claims: jwt.Claims{ @@ -104,6 +105,7 @@ func generateBootstrapToken(ca, subject, sha string) string { Expiry: jwt.NewNumericDate(now.Add(time.Minute)), Audience: []string{ca + "/sign"}, }, + SANS: []string{subject}, } raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() if err != nil { diff --git a/ca/ca_test.go b/ca/ca_test.go index 99714208..8bec48a1 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -154,13 +154,19 @@ ZEp7knvU2psWRw== "fail commonname-claim": func(t *testing.T) *signTest { jti, err := randutil.ASCII(32) assert.FatalError(t, err) - cl := jwt.Claims{ - Subject: "invalid", - Issuer: "step-cli", - NotBefore: jwt.NewNumericDate(now), - Expiry: jwt.NewNumericDate(now.Add(time.Minute)), - Audience: validAud, - ID: jti, + cl := struct { + jwt.Claims + SANS []string `json:"sans"` + }{ + Claims: jwt.Claims{ + Subject: "invalid", + Issuer: "step-cli", + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAud, + ID: jti, + }, + SANS: []string{"invalid"}, } raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() assert.FatalError(t, err) @@ -181,13 +187,19 @@ ZEp7knvU2psWRw== "ok": func(t *testing.T) *signTest { jti, err := randutil.ASCII(32) assert.FatalError(t, err) - cl := jwt.Claims{ - Subject: "test.smallstep.com", - Issuer: "step-cli", - NotBefore: jwt.NewNumericDate(now), - Expiry: jwt.NewNumericDate(now.Add(time.Minute)), - Audience: validAud, - ID: jti, + cl := struct { + jwt.Claims + SANS []string `json:"sans"` + }{ + Claims: jwt.Claims{ + Subject: "test.smallstep.com", + Issuer: "step-cli", + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAud, + ID: jti, + }, + SANS: []string{"test.smallstep.com"}, } raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() assert.FatalError(t, err) diff --git a/ca/tls_test.go b/ca/tls_test.go index 6c6a5291..c71e839d 100644 --- a/ca/tls_test.go +++ b/ca/tls_test.go @@ -39,13 +39,19 @@ func generateOTT(subject string) string { if err != nil { panic(err) } - cl := jwt.Claims{ - ID: id, - Subject: subject, - Issuer: "mariano", - NotBefore: jwt.NewNumericDate(now), - Expiry: jwt.NewNumericDate(now.Add(time.Minute)), - Audience: []string{"https://127.0.0.1:0/sign"}, + cl := struct { + jwt.Claims + SANS []string `json:"sans"` + }{ + Claims: jwt.Claims{ + ID: id, + Subject: subject, + Issuer: "mariano", + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: []string{"https://127.0.0.1:0/sign"}, + }, + SANS: []string{subject}, } raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() if err != nil { From fe8c8614b2695e370ab84597c894d58f563ef59e Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 1 Feb 2019 12:18:10 -0600 Subject: [PATCH 3/7] SANS backwards compat when token missing sujbect SAN --- authority/authorize.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/authority/authorize.go b/authority/authorize.go index cfd52bb7..b121d265 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -120,6 +120,12 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { http.StatusUnauthorized, errContext} } + // `step ca token` should generate tokens where the subject is also in the + // sans. It should not be necessary to add to SANS if both certificates and + // cli are up to date. However, for backwards compatibility we will add + // the subject to the SANS if it is missing. + claims.SANS = appendIfMissingString(claims.SANS, claims.Subject) + dnsNames, ips := SplitSANS(claims.SANS) if err != nil { return nil, err @@ -164,6 +170,15 @@ func SplitSANS(sans []string) (dnsNames []string, ips []net.IP) { return } +func appendIfMissingString(slice []string, s string) []string { + for _, e := range slice { + if e == s { + return slice + } + } + return append(slice, s) +} + // authorizeRenewal tries to locate the step provisioner extension, and checks // if for the configured provisioner, the renewal is enabled or not. If the // extra extension cannot be found, authorize the renewal by default. From ab78534b08d62d03e0181a34acd048a64bda7e6f Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 1 Feb 2019 12:24:21 -0600 Subject: [PATCH 4/7] add test for SAN backwards compatibility with CLI * new provisioner tokens always contain the crt.Subject.CommonName in the SANS attribute of the token claims. added tests that verifies backwards compatibility still works in cases where the token does not contain the subject as a SAN claim. --- ca/ca_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/ca/ca_test.go b/ca/ca_test.go index 8bec48a1..32701c05 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -218,6 +218,39 @@ ZEp7knvU2psWRw== status: http.StatusCreated, } }, + "ok-backwards-compat-missing-subject-SAN": func(t *testing.T) *signTest { + jti, err := randutil.ASCII(32) + assert.FatalError(t, err) + cl := struct { + jwt.Claims + SANS []string `json:"sans"` + }{ + Claims: jwt.Claims{ + Subject: "test.smallstep.com", + Issuer: "step-cli", + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAud, + ID: jti, + }, + } + raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + assert.FatalError(t, err) + csr, err := getCSR(priv) + assert.FatalError(t, err) + body, err := json.Marshal(&api.SignRequest{ + CsrPEM: api.CertificateRequest{CertificateRequest: csr}, + OTT: raw, + NotBefore: now, + NotAfter: leafExpiry, + }) + assert.FatalError(t, err) + return &signTest{ + ca: ca, + body: string(body), + status: http.StatusCreated, + } + }, } for name, genTestCase := range tests { From 93f39c64a065d0c5302ed26dc8786a422f8a6e6a Mon Sep 17 00:00:00 2001 From: max furman Date: Mon, 4 Feb 2019 20:02:56 -0800 Subject: [PATCH 5/7] backwards compat only when SANS empty --- authority/authorize.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index b121d265..1dd77c1a 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -120,12 +120,12 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { http.StatusUnauthorized, errContext} } - // `step ca token` should generate tokens where the subject is also in the - // sans. It should not be necessary to add to SANS if both certificates and - // cli are up to date. However, for backwards compatibility we will add - // the subject to the SANS if it is missing. - claims.SANS = appendIfMissingString(claims.SANS, claims.Subject) - + // NOTE: This is for backwards compatibility with older versions of cli + // and certificates. Older versions added the token subject as the only SAN + // in a CSR by default. + if len(claims.SANS) == 0 { + claims.SANS = []string{claims.Subject} + } dnsNames, ips := SplitSANS(claims.SANS) if err != nil { return nil, err @@ -170,15 +170,6 @@ func SplitSANS(sans []string) (dnsNames []string, ips []net.IP) { return } -func appendIfMissingString(slice []string, s string) []string { - for _, e := range slice { - if e == s { - return slice - } - } - return append(slice, s) -} - // authorizeRenewal tries to locate the step provisioner extension, and checks // if for the configured provisioner, the renewal is enabled or not. If the // extra extension cannot be found, authorize the renewal by default. From 6937bfea7bae55e7f3fbd3d6ef2b40cb9b043fcc Mon Sep 17 00:00:00 2001 From: max furman Date: Mon, 4 Feb 2019 20:22:02 -0800 Subject: [PATCH 6/7] claims.SANS -> claims.SANs --- authority/authorize.go | 12 ++++++------ ca/client.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index 1dd77c1a..17cd37a5 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -20,7 +20,7 @@ type idUsed struct { // Claims extends jwt.Claims with step attributes. type Claims struct { jwt.Claims - SANS []string `json:"sans,omitempty"` + SANs []string `json:"sans,omitempty"` } // matchesAudience returns true if A and B share at least one element. @@ -123,10 +123,10 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { // NOTE: This is for backwards compatibility with older versions of cli // and certificates. Older versions added the token subject as the only SAN // in a CSR by default. - if len(claims.SANS) == 0 { - claims.SANS = []string{claims.Subject} + if len(claims.SANs) == 0 { + claims.SANs = []string{claims.Subject} } - dnsNames, ips := SplitSANS(claims.SANS) + dnsNames, ips := SplitSANs(claims.SANs) if err != nil { return nil, err } @@ -150,10 +150,10 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { return signOps, nil } -// SplitSANS splits a slice of Subject Alternative Names into slices of +// SplitSANs splits a slice of Subject Alternative Names into slices of // IP Addresses and DNS Names. If an element is not an IP address, then it // is bucketed as a DNS Name. -func SplitSANS(sans []string) (dnsNames []string, ips []net.IP) { +func SplitSANs(sans []string) (dnsNames []string, ips []net.IP) { dnsNames = []string{} ips = []net.IP{} if sans == nil { diff --git a/ca/client.go b/ca/client.go index f6eea62d..5f997dea 100644 --- a/ca/client.go +++ b/ca/client.go @@ -452,7 +452,7 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) return nil, nil, errors.Wrap(err, "error generating key") } - dnsNames, ips := authority.SplitSANS(claims.SANS) + dnsNames, ips := authority.SplitSANs(claims.SANs) template := &x509.CertificateRequest{ Subject: pkix.Name{ From 3415a1fef8ed079a92d6f33470cc4d384d055e48 Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 5 Feb 2019 19:32:01 -0800 Subject: [PATCH 7/7] move SplitSANs to cli --- Gopkg.lock | 4 ++-- authority/authorize.go | 24 ++---------------------- ca/client.go | 3 ++- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 6293e4c3..e5d80fa4 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -212,7 +212,7 @@ [[projects]] branch = "sans" - digest = "1:84a773da390eabc9a292221bbc2c16653093f8eb805a1b16f738ef3cd88df701" + digest = "1:4c9e30abfe7c119eb4d40287f6c23f854f3ad71c69206d8dc6402e1fef14ac88" name = "github.com/smallstep/cli" packages = [ "command", @@ -231,7 +231,7 @@ "utils", ] pruneopts = "UT" - revision = "49d4a4c26c802e83c5ed160abdd5babab1c9b5c6" + revision = "1379a62e0cf06b164d35e20a912d017ac8bad071" [[projects]] branch = "master" diff --git a/authority/authorize.go b/authority/authorize.go index 17cd37a5..5566b17f 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -3,12 +3,12 @@ package authority import ( "crypto/x509" "encoding/asn1" - "net" "net/http" "net/url" "time" "github.com/pkg/errors" + "github.com/smallstep/cli/crypto/x509util" "gopkg.in/square/go-jose.v2/jwt" ) @@ -126,7 +126,7 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { if len(claims.SANs) == 0 { claims.SANs = []string{claims.Subject} } - dnsNames, ips := SplitSANs(claims.SANs) + dnsNames, ips := x509util.SplitSANs(claims.SANs) if err != nil { return nil, err } @@ -150,26 +150,6 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { return signOps, nil } -// SplitSANs splits a slice of Subject Alternative Names into slices of -// IP Addresses and DNS Names. If an element is not an IP address, then it -// is bucketed as a DNS Name. -func SplitSANs(sans []string) (dnsNames []string, ips []net.IP) { - dnsNames = []string{} - ips = []net.IP{} - if sans == nil { - return - } - for _, san := range sans { - if ip := net.ParseIP(san); ip != nil { - ips = append(ips, ip) - } else { - // If not IP then assume DNSName. - dnsNames = append(dnsNames, san) - } - } - return -} - // authorizeRenewal tries to locate the step provisioner extension, and checks // if for the configured provisioner, the renewal is enabled or not. If the // extra extension cannot be found, authorize the renewal by default. diff --git a/ca/client.go b/ca/client.go index 5f997dea..1ca682de 100644 --- a/ca/client.go +++ b/ca/client.go @@ -23,6 +23,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" + "github.com/smallstep/cli/crypto/x509util" "gopkg.in/square/go-jose.v2/jwt" ) @@ -452,7 +453,7 @@ func CreateSignRequest(ott string) (*api.SignRequest, crypto.PrivateKey, error) return nil, nil, errors.Wrap(err, "error generating key") } - dnsNames, ips := authority.SplitSANs(claims.SANs) + dnsNames, ips := x509util.SplitSANs(claims.SANs) template := &x509.CertificateRequest{ Subject: pkix.Name{