Merge pull request #18 from smallstep/mariano/audience

Do not require the port in the audience check.

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,17 +48,11 @@ func New(config *Config) (*Authority, error) {
 		}
 	}
 
-	// Define audiences: legacy + possible urls
+	// Define audiences: legacy + possible urls without the ports.
-	_, port, err := net.SplitHostPort(config.Address)
+	// The CA might have proxies in front so we cannot rely on the port.
-	if err != nil {
-		return nil, errors.Wrapf(err, "error parsing %s", config.Address)
-	}
 	audiences := []string{legacyAuthority}
 	for _, name := range config.DNSNames {
-		if port == "443" {
+		audiences = append(audiences, fmt.Sprintf("https://%s/sign", name), fmt.Sprintf("https://%s/1.0/sign", name))
-			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{

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/sign",
-						"https://127.0.0.1:0/1.0/sign",
+						"https://127.0.0.1/1.0/sign",
 					})
 				}
 			}

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.
+// matchesAudience returns true if A and B share at least one element.
-func matchesOne(as, bs []string) bool {
+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}
 	}

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)
+			}
 		})
 	}
 }

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 {

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",