internal/config: change internal-authenticate-addr to url (#154)

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`.

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. 

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
 

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

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=

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
+	// AuthenticateInternalAddr is used override the routable destination of
-	// or ingress that does not natively support routing gRPC.
+	// authenticate service's GRPC endpoint.
-	AuthenticateInternalAddr string `mapstructure:"authenticate_internal_url"`
+	// 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",
+		Addr:                     ":https",
-		CertFile:          filepath.Join(findPwd(), "cert.pem"),
+		CertFile:                 filepath.Join(findPwd(), "cert.pem"),
-		KeyFile:           filepath.Join(findPwd(), "privkey.pem"),
+		KeyFile:                  filepath.Join(findPwd(), "privkey.pem"),
-		ReadHeaderTimeout: 10 * time.Second,
+		ReadHeaderTimeout:        10 * time.Second,
-		ReadTimeout:       30 * time.Second,
+		ReadTimeout:              30 * time.Second,
-		WriteTimeout:      0, // support streaming by default
+		WriteTimeout:             0, // support streaming by default
-		IdleTimeout:       5 * time.Minute,
+		IdleTimeout:              5 * time.Minute,
-		AuthenticateURL:   new(url.URL),
+		AuthenticateURL:          new(url.URL),
-		AuthorizeURL:      new(url.URL),
+		AuthenticateInternalAddr: new(url.URL),
-		RefreshCooldown:   time.Duration(5 * time.Minute),
+		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 o.AuthenticateURLString != "" {
-	if err != nil {
+		AuthenticateURL, err := parseAndValidateURL(o.AuthenticateURLString)
-		return err
+		if err != nil {
-	}
+			return fmt.Errorf("internal/config: bad authenticate-url %s : %v", o.AuthenticateURLString, err)
-	AuthorizeURL, err := url.Parse(o.AuthorizeURLString)
+		}
-	if err != nil {
+		o.AuthenticateURL = AuthenticateURL
-		return err
+	}
+
+	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
 }
 

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
+		name                    string
-		authorizeURL    string
+		authorizeURL            string
-		authenticateURL string
+		authenticateURL         string
-		wantErr         bool
+		authenticateInternalURL string
+		wantErr                 bool
 	}{
-		{"good", "https://authz.mydomain.tld", "https://authn.mydomain.tld", false},
+		{"good", "https://authz.mydomain.example", "https://authn.mydomain.example", "https://internal.svc.local", false},
-		{"bad authorize", "notaurl", "https://authn.mydomain.tld", true},
+		{"bad not https scheme", "http://authz.mydomain.example", "http://authn.mydomain.example", "http://internal.svc.local", true},
-		{"bad authenticate", "https://authz.mydomain.tld", "notaurl", true},
+		{"missing scheme", "authz.mydomain.example", "authn.mydomain.example", "internal.svc.local", true},
-		{"only authn", "", "https://authn.mydomain.tld", false},
+		{"bad authorize", "notaurl", "https://authn.mydomain.example", "", true},
-		{"only authz", "https://authz.mydomain.tld", "", false},
+		{"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,
+			AuthenticateURLString:          test.authenticateURL,
-			AuthorizeURLString:    test.authorizeURL,
+			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) {
+			if diff := cmp.Diff(got, tt.want); diff != "" {
-				t.Errorf("OptionsFromViper() = %v, want %v", got, tt.want)
+				t.Errorf("OptionsFromViper() = \n%s\n, \ngot\n%v\n, want \n%v", diff, got, tt.want)
 			}
 
 		})

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,