From 7e95fc0e45ec061a3d01d43e4daea98f01b2df23 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 21 Dec 2018 15:27:22 -0800 Subject: [PATCH] Strip ports on audience check. Services might have proxies behind them so we cannot rely on them. Fixes #17 --- authority/authority.go | 10 ++------- authority/authority_test.go | 13 ++---------- authority/authorize.go | 20 ++++++++++++++---- authority/authorize_test.go | 41 +++++++++++++++++++++++++++++++++++-- authority/config.go | 6 ++++++ authority/config_test.go | 30 +++++++++++++++++++-------- 6 files changed, 87 insertions(+), 33 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index f0e5c0b7..4753f83b 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -5,11 +5,9 @@ import ( realx509 "crypto/x509" "encoding/hex" "fmt" - "net" "sync" "time" - "github.com/pkg/errors" "github.com/smallstep/cli/crypto/pemutil" "github.com/smallstep/cli/crypto/x509util" ) @@ -50,15 +48,11 @@ func New(config *Config) (*Authority, error) { } } - // Define audiences: legacy + possible urls - _, port, err := net.SplitHostPort(config.Address) - if err != nil { - return nil, errors.Wrapf(err, "error parsing %s", config.Address) - } + // Define audiences: legacy + possible urls without the ports. + // The CA might have proxies in front so we cannot rely on the port. audiences := []string{legacyAuthority} for _, name := range config.DNSNames { audiences = append(audiences, fmt.Sprintf("https://%s/sign", name), fmt.Sprintf("https://%s/1.0/sign", name)) - audiences = append(audiences, fmt.Sprintf("https://%s:%s/sign", name, port), fmt.Sprintf("https://%s:%s/1.0/sign", name, port)) } var a = &Authority{ diff --git a/authority/authority_test.go b/authority/authority_test.go index 71b51a52..ad2f4980 100644 --- a/authority/authority_test.go +++ b/authority/authority_test.go @@ -74,15 +74,6 @@ func TestAuthorityNew(t *testing.T) { err: errors.New("open foo failed: no such file or directory"), } }, - "fail bad address": func(t *testing.T) *newTest { - c, err := LoadConfiguration("../ca/testdata/ca.json") - assert.FatalError(t, err) - c.Address = "127.0.0.1" - return &newTest{ - config: c, - err: errors.New("error parsing 127.0.0.1: address 127.0.0.1: missing port in address"), - } - }, "fail bad password": func(t *testing.T) *newTest { c, err := LoadConfiguration("../ca/testdata/ca.json") assert.FatalError(t, err) @@ -137,8 +128,8 @@ func TestAuthorityNew(t *testing.T) { assert.Equals(t, auth.audiences, []string{ "step-certificate-authority", - "https://127.0.0.1:0/sign", - "https://127.0.0.1:0/1.0/sign", + "https://127.0.0.1/sign", + "https://127.0.0.1/1.0/sign", }) } } diff --git a/authority/authorize.go b/authority/authorize.go index ca753cab..1c6fc65d 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -4,6 +4,7 @@ import ( "crypto/x509" "encoding/asn1" "net/http" + "net/url" "time" "github.com/pkg/errors" @@ -15,15 +16,15 @@ type idUsed struct { Subject string `json:"sub,omitempty"` } -// matchesOne returns true if A and B share at least one element. -func matchesOne(as, bs []string) bool { +// 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 { return false } for _, b := range bs { for _, a := range as { - if b == a { + if b == a || stripPort(a) == stripPort(b) { return true } } @@ -31,6 +32,17 @@ func matchesOne(as, bs []string) bool { return false } +// stripPort attempts to strip the port from the given url. If parsing the url +// produces errors it will just return the passed argument. +func stripPort(rawurl string) string { + u, err := url.Parse(rawurl) + if err != nil { + return rawurl + } + u.Host = u.Hostname() + return u.String() +} + // Authorize authorizes a signature request by validating and authenticating // a OTT that must be sent w/ the request. func (a *Authority) Authorize(ott string) ([]interface{}, error) { @@ -91,7 +103,7 @@ func (a *Authority) Authorize(ott string) ([]interface{}, error) { } } - if !matchesOne(claims.Audience, a.audiences) { + if !matchesAudience(claims.Audience, a.audiences) { return nil, &apiError{errors.New("authorize: token audience invalid"), http.StatusUnauthorized, errContext} } diff --git a/authority/authorize_test.go b/authority/authorize_test.go index d4416a25..8ccd7a4d 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -13,7 +13,7 @@ import ( "gopkg.in/square/go-jose.v2/jwt" ) -func TestMatchesOne(t *testing.T) { +func TestMatchesAudience(t *testing.T) { type matchesTest struct { a, b []string exp bool @@ -44,10 +44,47 @@ func TestMatchesOne(t *testing.T) { b: []string{"https://127.0.0.1:0/sign", "https://test.ca.smallstep.com/sign"}, exp: true, }, + "true,portsA": { + a: []string{"step-gateway", "https://test.ca.smallstep.com:9000/sign"}, + b: []string{"https://127.0.0.1:0/sign", "https://test.ca.smallstep.com/sign"}, + exp: true, + }, + "true,portsB": { + a: []string{"step-gateway", "https://test.ca.smallstep.com/sign"}, + b: []string{"https://127.0.0.1:0/sign", "https://test.ca.smallstep.com:9000/sign"}, + exp: true, + }, + "true,portsAB": { + a: []string{"step-gateway", "https://test.ca.smallstep.com:9000/sign"}, + b: []string{"https://127.0.0.1:0/sign", "https://test.ca.smallstep.com:8000/sign"}, + exp: true, + }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { - assert.Equals(t, tc.exp, matchesOne(tc.a, tc.b)) + assert.Equals(t, tc.exp, matchesAudience(tc.a, tc.b)) + }) + } +} + +func TestStripPort(t *testing.T) { + type args struct { + rawurl string + } + tests := []struct { + name string + args args + want string + }{ + {"with port", args{"https://ca.smallstep.com:9000/sign"}, "https://ca.smallstep.com/sign"}, + {"with no port", args{"https://ca.smallstep.com/sign/"}, "https://ca.smallstep.com/sign/"}, + {"bad url", args{"https://a bad url:9000"}, "https://a bad url:9000"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := stripPort(tt.args.rawurl); got != tt.want { + t.Errorf("stripPort() = %v, want %v", got, tt.want) + } }) } } diff --git a/authority/config.go b/authority/config.go index 5f41b48f..5f9910af 100644 --- a/authority/config.go +++ b/authority/config.go @@ -2,6 +2,7 @@ package authority import ( "encoding/json" + "net" "os" "time" @@ -157,6 +158,11 @@ func (c *Config) Validate() error { return errors.New("dnsNames cannot be empty") } + // Validate address (a port is required) + if _, _, err := net.SplitHostPort(c.Address); err != nil { + return errors.Errorf("invalid address %s", c.Address) + } + if c.TLS == nil { c.TLS = &DefaultTLSOptions } else { diff --git a/authority/config_test.go b/authority/config_test.go index ad4fc651..c16b4780 100644 --- a/authority/config_test.go +++ b/authority/config_test.go @@ -50,10 +50,24 @@ func TestConfigValidate(t *testing.T) { err: errors.New("address cannot be empty"), } }, - "empty-root": func(t *testing.T) ConfigValidateTest { + "invalid-address": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ Address: "127.0.0.1", + Root: "testdata/secrets/root_ca.crt", + IntermediateCert: "testdata/secrets/intermediate_ca.crt", + IntermediateKey: "testdata/secrets/intermediate_ca_key", + DNSNames: []string{"test.smallstep.com"}, + Password: "pass", + AuthorityConfig: ac, + }, + err: errors.New("invalid address 127.0.0.1"), + } + }, + "empty-root": func(t *testing.T) ConfigValidateTest { + return ConfigValidateTest{ + config: &Config{ + Address: "127.0.0.1:443", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", DNSNames: []string{"test.smallstep.com"}, @@ -66,7 +80,7 @@ func TestConfigValidate(t *testing.T) { "empty-intermediate-cert": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", DNSNames: []string{"test.smallstep.com"}, @@ -79,7 +93,7 @@ func TestConfigValidate(t *testing.T) { "empty-intermediate-key": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", DNSNames: []string{"test.smallstep.com"}, @@ -92,7 +106,7 @@ func TestConfigValidate(t *testing.T) { "empty-dnsNames": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", @@ -105,7 +119,7 @@ func TestConfigValidate(t *testing.T) { "empty-TLS": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", @@ -119,7 +133,7 @@ func TestConfigValidate(t *testing.T) { "empty-TLS-values": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", @@ -134,7 +148,7 @@ func TestConfigValidate(t *testing.T) { "custom-tls-values": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key", @@ -163,7 +177,7 @@ func TestConfigValidate(t *testing.T) { "tls-min>max": func(t *testing.T) ConfigValidateTest { return ConfigValidateTest{ config: &Config{ - Address: "127.0.0.1", + Address: "127.0.0.1:443", Root: "testdata/secrets/root_ca.crt", IntermediateCert: "testdata/secrets/intermediate_ca.crt", IntermediateKey: "testdata/secrets/intermediate_ca_key",