From 77f3933560e08553d0d2fac79a8eb99f4fc5e593 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 31 May 2019 17:53:58 -0700 Subject: [PATCH] internal/config: change internal-authenticate-addr to url (#154) --- CHANGELOG.md | 2 +- docs/docs/upgrading.md | 4 +- docs/reference/readme.md | 8 ++-- go.mod | 1 + go.sum | 2 + internal/config/options.go | 79 +++++++++++++++++++++++---------- internal/config/options_test.go | 77 ++++++++++++++++++++++++-------- proxy/proxy.go | 2 +- 8 files changed, 127 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cf2d1bcf..b05c1947a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ - Add support for public, unauthenticated routes. [GH-129] ### CHANGED - +- Changed config `AUTHENTICATE_INTERNAL_URL` to be a URL containing both a valid hostname and schema. [GH-153] - User state is now maintained and scoped at the domain level vs at the route level. [GH-128] - Error pages contain a link to sign out from the current user session. [GH-100] - Removed `LifetimeDeadline` from `sessions.SessionState`. diff --git a/docs/docs/upgrading.md b/docs/docs/upgrading.md index f00552b64..b9957b566 100644 --- a/docs/docs/upgrading.md +++ b/docs/docs/upgrading.md @@ -46,4 +46,6 @@ Usage of the POLICY_FILE envvar is no longer supported. Support for file based timeout: 30s ``` -### Z +### Authenticate Internal Service Address + +The configuration variable [Authenticate Internal Service URL](https://www.pomerium.io/reference/#authenticate-internal-service-url) must now be a valid [URL](https://golang.org/pkg/net/url/#URL) type and contain both a hostname and valid `https` schema. diff --git a/docs/reference/readme.md b/docs/reference/readme.md index 97f709a03..485bcd4b1 100644 --- a/docs/reference/readme.md +++ b/docs/reference/readme.md @@ -323,9 +323,9 @@ Authenticate Service URL is the externally accessible URL for the authenticate s - Environmental Variable: `AUTHENTICATE_INTERNAL_URL` - Config File Key: `authenticate_internal_url` -- Type: `string` +- Type: `URL` - Optional -- Example: `pomerium-authenticate-service.pomerium.svc.cluster.local` +- Example: `https://pomerium-authenticate-service.pomerium.svc.cluster.local` Authenticate Internal Service URL is the internally routed dns name of the authenticate service. This setting is typically used with load balancers that do not gRPC, thus allowing you to specify an internally accessible name. @@ -335,11 +335,11 @@ Authenticate Internal Service URL is the internally routed dns name of the authe - Config File Key: `authorize_service_url` - Type: `URL` - Required -- Example: `https://access.corp.example.com` or `pomerium-authorize-service.pomerium.svc.cluster.local` +- Example: `https://access.corp.example.com` or `https://pomerium-authorize-service.pomerium.svc.cluster.local` Authorize Service URL is the location of the internally accessible authorize service. NOTE: Unlike authenticate, authorize has no publicly accessible http handlers so this setting is purely for gRPC communication. -If your load balancer does not support gRPC pass-through you'll need to set this value to an internally routable location (`pomerium-authorize-service.pomerium.svc.cluster.local`) instead of an externally routable one (`https://access.corp.example.com`). +If your load balancer does not support gRPC pass-through you'll need to set this value to an internally routable location (`https://pomerium-authorize-service.pomerium.svc.cluster.local`) instead of an externally routable one (`https://access.corp.example.com`). ### Override Certificate Name diff --git a/go.mod b/go.mod index 536b174b4..855cf76d5 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.12 require ( github.com/golang/mock v1.2.0 github.com/golang/protobuf v1.3.1 + github.com/google/go-cmp v0.3.0 github.com/magiconair/properties v1.8.1 // indirect github.com/pomerium/go-oidc v2.0.0+incompatible github.com/pquerna/cachecontrol v0.0.0-20180517163645-1555304b9b35 // indirect diff --git a/go.sum b/go.sum index 7aaaf2669..afb9b527d 100644 --- a/go.sum +++ b/go.sum @@ -28,6 +28,8 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M= +github.com/google/go-cmp v0.3.0 h1:crn/baboCvb5fXaQ0IJ1SGTsTVrWpDsCWC8EGETZijY= +github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/grpc-ecosystem/grpc-gateway v1.5.0/go.mod h1:RSKVYQBd5MCa4OVpNdGskqpgL2+G+NZTnrVHpWWfpdw= github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= diff --git a/internal/config/options.go b/internal/config/options.go index a121fb9c3..69fe84ce5 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -98,12 +98,15 @@ type Options struct { // (sudo) access including the ability to impersonate other users' access Administrators []string `mapstructure:"administrators"` - // AuthenticateInternalAddr is used as an override when using a load balancer - // or ingress that does not natively support routing gRPC. - AuthenticateInternalAddr string `mapstructure:"authenticate_internal_url"` + // AuthenticateInternalAddr is used override the routable destination of + // authenticate service's GRPC endpoint. + // NOTE: As many load balancers do not support externally routed gRPC so + // this may be an internal location. + AuthenticateInternalAddrString string `mapstructure:"authenticate_internal_url"` + AuthenticateInternalAddr *url.URL // AuthorizeURL is the routable destination of the authorize service's - // gRPC endpoint. NOTE: As above, many load balancers do not support + // gRPC endpoint. NOTE: As many load balancers do not support // externally routed gRPC so this may be an internal location. AuthorizeURLString string `mapstructure:"authorize_service_url"` AuthorizeURL *url.URL @@ -146,16 +149,17 @@ func NewOptions() *Options { "X-XSS-Protection": "1; mode=block", "Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload", }, - Addr: ":https", - CertFile: filepath.Join(findPwd(), "cert.pem"), - KeyFile: filepath.Join(findPwd(), "privkey.pem"), - ReadHeaderTimeout: 10 * time.Second, - ReadTimeout: 30 * time.Second, - WriteTimeout: 0, // support streaming by default - IdleTimeout: 5 * time.Minute, - AuthenticateURL: new(url.URL), - AuthorizeURL: new(url.URL), - RefreshCooldown: time.Duration(5 * time.Minute), + Addr: ":https", + CertFile: filepath.Join(findPwd(), "cert.pem"), + KeyFile: filepath.Join(findPwd(), "privkey.pem"), + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 0, // support streaming by default + IdleTimeout: 5 * time.Minute, + AuthenticateURL: new(url.URL), + AuthenticateInternalAddr: new(url.URL), + AuthorizeURL: new(url.URL), + RefreshCooldown: time.Duration(5 * time.Minute), } return o } @@ -264,19 +268,48 @@ func (o *Options) parsePolicy() error { return nil } +// parseAndValidateURL wraps standard library's default url.Parse because it's much more +// lenient about what type of urls it accepts than pomerium can be. +func parseAndValidateURL(rawurl string) (*url.URL, error) { + u, err := url.Parse(rawurl) + if err != nil { + return nil, err + } + if u.Host == "" { + return nil, fmt.Errorf("%s does have a valid hostname", rawurl) + } + if u.Scheme == "" || u.Scheme != "https" { + return nil, fmt.Errorf("%s does have a valid https scheme", rawurl) + } + return u, nil +} + // parseURLs parses URL strings into actual URL pointers func (o *Options) parseURLs() error { - AuthenticateURL, err := url.Parse(o.AuthenticateURLString) - if err != nil { - return err - } - AuthorizeURL, err := url.Parse(o.AuthorizeURLString) - if err != nil { - return err + if o.AuthenticateURLString != "" { + AuthenticateURL, err := parseAndValidateURL(o.AuthenticateURLString) + if err != nil { + return fmt.Errorf("internal/config: bad authenticate-url %s : %v", o.AuthenticateURLString, err) + } + o.AuthenticateURL = AuthenticateURL + } + + if o.AuthorizeURLString != "" { + AuthorizeURL, err := parseAndValidateURL(o.AuthorizeURLString) + if err != nil { + return fmt.Errorf("internal/config: bad authorize-url %s : %v", o.AuthorizeURLString, err) + } + o.AuthorizeURL = AuthorizeURL + } + + if o.AuthenticateInternalAddrString != "" { + AuthenticateInternalAddr, err := parseAndValidateURL(o.AuthenticateInternalAddrString) + if err != nil { + return fmt.Errorf("internal/config: bad authenticate-internal-addr %s : %v", o.AuthenticateInternalAddrString, err) + } + o.AuthenticateInternalAddr = AuthenticateInternalAddr } - o.AuthenticateURL = AuthenticateURL - o.AuthorizeURL = AuthorizeURL return nil } diff --git a/internal/config/options_test.go b/internal/config/options_test.go index 9a6f15e3d..43caafd5a 100644 --- a/internal/config/options_test.go +++ b/internal/config/options_test.go @@ -9,6 +9,7 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" "github.com/pomerium/pomerium/internal/policy" "github.com/spf13/viper" ) @@ -120,6 +121,28 @@ func Test_isAuthorize(t *testing.T) { }) } } +func Test_IsProxy(t *testing.T) { + tests := []struct { + name string + service string + want bool + }{ + {"proxy", "proxy", true}, + {"all", "all", true}, + {"authorize", "authorize", false}, + {"proxy bad case", "PrOxY", false}, + {"jiberish", "xd23", false}, + } + for _, tt := range tests { + + t.Run(tt.name, func(t *testing.T) { + if got := IsProxy(tt.service); got != tt.want { + t.Errorf("IsProxy() = %v, want %v", got, tt.want) + } + }) + } +} + func Test_bindEnvs(t *testing.T) { o := &Options{} os.Clearenv() @@ -145,30 +168,36 @@ func Test_bindEnvs(t *testing.T) { func Test_parseURLs(t *testing.T) { tests := []struct { - name string - authorizeURL string - authenticateURL string - wantErr bool + name string + authorizeURL string + authenticateURL string + authenticateInternalURL string + wantErr bool }{ - {"good", "https://authz.mydomain.tld", "https://authn.mydomain.tld", false}, - {"bad authorize", "notaurl", "https://authn.mydomain.tld", true}, - {"bad authenticate", "https://authz.mydomain.tld", "notaurl", true}, - {"only authn", "", "https://authn.mydomain.tld", false}, - {"only authz", "https://authz.mydomain.tld", "", false}, + {"good", "https://authz.mydomain.example", "https://authn.mydomain.example", "https://internal.svc.local", false}, + {"bad not https scheme", "http://authz.mydomain.example", "http://authn.mydomain.example", "http://internal.svc.local", true}, + {"missing scheme", "authz.mydomain.example", "authn.mydomain.example", "internal.svc.local", true}, + {"bad authorize", "notaurl", "https://authn.mydomain.example", "", true}, + {"bad authenticate", "https://authz.mydomain.example", "notaurl", "", true}, + {"bad authenticate internal", "", "", "just.some.naked.domain.example", true}, + {"only authn", "", "https://authn.mydomain.example", "", false}, + {"only authz", "https://authz.mydomain.example", "", "", false}, + {"malformed", "http://a b.com/", "", "", true}, } for _, test := range tests { o := &Options{ - AuthenticateURLString: test.authenticateURL, - AuthorizeURLString: test.authorizeURL, + AuthenticateURLString: test.authenticateURL, + AuthorizeURLString: test.authorizeURL, + AuthenticateInternalAddrString: test.authenticateInternalURL, } err := o.parseURLs() - if !test.wantErr && err != nil { + if (err != nil) != test.wantErr { t.Errorf("Failed to parse URLs %v: %s", test, err) } - if o.AuthenticateURL.String() != test.authenticateURL { + if o.AuthenticateURL != nil && o.AuthenticateURL.String() != test.authenticateURL { t.Errorf("Failed to update AuthenticateURL: %v", test) } - if o.AuthorizeURL.String() != test.authorizeURL { + if o.AuthorizeURL != nil && o.AuthorizeURL.String() != test.authorizeURL { t.Errorf("Failed to update AuthorizeURL: %v", test) } } @@ -185,12 +214,24 @@ func Test_OptionsFromViper(t *testing.T) { testPolicy, } - goodConfigBytes := []byte(`{"shared_secret":"Setec Astronomy","service":"all","policy":[{"from":"https://pomerium.io","to":"https://httpbin.org"}]}`) + goodConfigBytes := []byte(`{"authorize_service_url":"https://authorize.corp.example","authenticate_service_url":"https://authenticate.corp.example","shared_secret":"Setec Astronomy","service":"all","policy":[{"from":"https://pomerium.io","to":"https://httpbin.org"}]}`) goodOptions := NewOptions() goodOptions.SharedKey = "Setec Astronomy" goodOptions.Services = "all" goodOptions.Policies = testPolicies goodOptions.CookieName = "oatmeal" + goodOptions.AuthorizeURLString = "https://authorize.corp.example" + goodOptions.AuthenticateURLString = "https://authenticate.corp.example" + authorize, err := url.Parse(goodOptions.AuthorizeURLString) + if err != nil { + t.Fatal(err) + } + authenticate, err := url.Parse(goodOptions.AuthenticateURLString) + if err != nil { + t.Fatal(err) + } + goodOptions.AuthorizeURL = authorize + goodOptions.AuthenticateURL = authenticate badConfigBytes := []byte("badjson!") badUnmarshalConfigBytes := []byte(`"debug": "blue"`) @@ -217,10 +258,10 @@ func Test_OptionsFromViper(t *testing.T) { tempFile.Write(tt.configBytes) got, err := OptionsFromViper(tempFile.Name()) if (err != nil) != tt.wantErr { - t.Errorf("OptionsFromViper() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("OptionsFromViper() error = \n%v, wantErr \n%v", err, tt.wantErr) } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("OptionsFromViper() = %v, want %v", got, tt.want) + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("OptionsFromViper() = \n%s\n, \ngot\n%v\n, want \n%v", diff, got, tt.want) } }) diff --git a/proxy/proxy.go b/proxy/proxy.go index 916eead35..9a632fc38 100755 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -161,7 +161,7 @@ func New(opts *config.Options) (*Proxy, error) { p.AuthenticateClient, err = clients.NewAuthenticateClient("grpc", &clients.Options{ Addr: opts.AuthenticateURL.Host, - InternalAddr: opts.AuthenticateInternalAddr, + InternalAddr: opts.AuthenticateInternalAddr.String(), OverrideCertificateName: opts.OverrideCertificateName, SharedSecret: opts.SharedKey, CA: opts.CA,