Skip to content

Commit

Permalink
Merge pull request #18 from smallstep/mariano/audience
Browse files Browse the repository at this point in the history 
Do not require the port in the audience check.
  • Loading branch information
@maraino
maraino authored Dec 21, 2018
2 parents e5dff95 + 7e95fc0 commit 9e2fce2
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 36 deletions.
14 changes: 3 additions & 11 deletions authority/authority.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -50,17 +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 {
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:%s/sign", name, port), fmt.Sprintf("https://%s:%s/1.0/sign", name, port))
audiences = append(audiences, fmt.Sprintf("https://%s/sign", name), fmt.Sprintf("https://%s/1.0/sign", name))
}

var a = &Authority{
Expand Down
13 changes: 2 additions & 11 deletions authority/authority_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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",
})
}
}
Expand Down
20 changes: 16 additions & 4 deletions authority/authorize.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"crypto/x509"
"encoding/asn1"
"net/http"
"net/url"
"time"

"github.com/pkg/errors"
Expand All @@ -15,22 +16,33 @@ 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
}
}
}
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) {
Expand Down Expand Up @@ -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}
}
Expand Down
41 changes: 39 additions & 2 deletions authority/authorize_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
})
}
}
Expand Down
6 changes: 6 additions & 0 deletions authority/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package authority

import (
"encoding/json"
"net"
"os"
"time"

Expand Down Expand Up @@ -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 {
Expand Down
30 changes: 22 additions & 8 deletions authority/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 9e2fce2

Please sign in to comment.