Merge pull request #254 from desimone/bug/253

config: fix url type regression (#253)

authenticate/authenticate.go

@@ -12,14 +12,18 @@ import (
 	"github.com/pomerium/pomerium/internal/identity"
 	"github.com/pomerium/pomerium/internal/sessions"
 	"github.com/pomerium/pomerium/internal/templates"
+	"github.com/pomerium/pomerium/internal/urlutil"
 )
 
 // ValidateOptions checks to see if configuration values are valid for the authenticate service.
 // The checks do not modify the internal state of the Option structure. Returns
 // on first error found.
 func ValidateOptions(o config.Options) error {
-	if o.AuthenticateURL.String() == "" {
+	if o.AuthenticateURL == nil {
-		return errors.New("authenticate: 'AUTHENTICATE_SERVICE_URL' missing")
+		return errors.New("authenticate: missing setting: authenticate-service-url")
+	}
+	if _, err := urlutil.ParseAndValidateURL(o.AuthenticateURL.String()); err != nil {
+		return fmt.Errorf("authenticate: error parsing authenticate url: %v", err)
 	}
 	if o.ClientID == "" {
 		return errors.New("authenticate: 'IDP_CLIENT_ID' missing")
@@ -75,7 +79,7 @@ func New(opts config.Options) (*Authenticate, error) {
 	if err != nil {
 		return nil, err
 	}
-	redirectURL := opts.AuthenticateURL
+	redirectURL, _ := urlutil.DeepCopy(opts.AuthenticateURL)
 	redirectURL.Path = "/oauth2/callback"
 	provider, err := identity.New(
 		opts.Provider,
@@ -97,7 +101,7 @@ func New(opts config.Options) (*Authenticate, error) {
 	}
 	return &Authenticate{
 		SharedKey:    opts.SharedKey,
-		RedirectURL:  &redirectURL,
+		RedirectURL:  redirectURL,
 		templates:    templates.New(),
 		csrfStore:    cookieStore,
 		sessionStore: cookieStore,

authenticate/authenticate_test.go

@@ -1,7 +1,6 @@
 package authenticate
 
 import (
-	"net/url"
 	"testing"
 
 	"github.com/pomerium/pomerium/internal/config"
@@ -22,7 +21,9 @@ func newTestOptions(t *testing.T) *config.Options {
 func TestOptions_Validate(t *testing.T) {
 	good := newTestOptions(t)
 	badRedirectURL := newTestOptions(t)
-	badRedirectURL.AuthenticateURL = url.URL{}
+	badRedirectURL.AuthenticateURL = nil
+	badScheme := newTestOptions(t)
+	badScheme.AuthenticateURL.Scheme = ""
 	emptyClientID := newTestOptions(t)
 	emptyClientID.ClientID = ""
 	emptyClientSecret := newTestOptions(t)
@@ -36,7 +37,7 @@ func TestOptions_Validate(t *testing.T) {
 	badSharedKey := newTestOptions(t)
 	badSharedKey.SharedKey = ""
 	badAuthenticateURL := newTestOptions(t)
-	badAuthenticateURL.AuthenticateURL = url.URL{}
+	badAuthenticateURL.AuthenticateURL = nil
 
 	tests := []struct {
 		name    string
@@ -46,6 +47,7 @@ func TestOptions_Validate(t *testing.T) {
 		{"minimum options", good, false},
 		{"nil options", &config.Options{}, true},
 		{"bad redirect  url", badRedirectURL, true},
+		{"bad scheme", badScheme, true},
 		{"no cookie secret", emptyCookieSecret, true},
 		{"invalid cookie secret", invalidCookieSecret, true},
 		{"short cookie secret", shortCookieLength, true},
@@ -67,7 +69,7 @@ func TestNew(t *testing.T) {
 	good := newTestOptions(t)
 
 	badRedirectURL := newTestOptions(t)
-	badRedirectURL.AuthenticateURL = url.URL{}
+	badRedirectURL.AuthenticateURL = nil
 
 	badCookieName := newTestOptions(t)
 	badCookieName.CookieName = ""

cmd/pomerium/main_test.go

@@ -146,8 +146,8 @@ func Test_newProxyeService(t *testing.T) {
 			AuthenticateURL, _ := url.Parse("https://authenticate.example.com")
 			AuthorizeURL, _ := url.Parse("https://authorize.example.com")
 
-			testOpts.AuthenticateURL = *AuthenticateURL
+			testOpts.AuthenticateURL = AuthenticateURL
-			testOpts.AuthorizeURL = *AuthorizeURL
+			testOpts.AuthorizeURL = AuthorizeURL
 			testOpts.CookieSecret = "YixWi1MYh77NMECGGIJQevoonYtVF+ZPRkQZrrmeRqM="
 			testOpts.Services = tt.s
 

internal/config/options.go

@@ -73,7 +73,7 @@ type Options struct {
 	// AuthenticateURL represents the externally accessible http endpoints
 	// used for authentication requests and callbacks
 	AuthenticateURLString string `mapstructure:"authenticate_service_url"`
-	AuthenticateURL       url.URL
+	AuthenticateURL       *url.URL
 
 	// Session/Cookie management
 	// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
@@ -103,13 +103,13 @@ type Options struct {
 	// 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
+	AuthenticateInternalAddr       *url.URL
 
 	// AuthorizeURL is the routable destination of the authorize service's
 	// 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
+	AuthorizeURL       *url.URL
 
 	// Settings to enable custom behind-the-ingress service communication
 	OverrideCertificateName string `mapstructure:"override_certificate_name"`
@@ -230,7 +230,7 @@ func (o *Options) Validate() error {
 		if err != nil {
 			return fmt.Errorf("bad authenticate-url %s : %v", o.AuthenticateURLString, err)
 		}
-		o.AuthenticateURL = *u
+		o.AuthenticateURL = u
 	}
 
 	if o.AuthorizeURLString != "" {
@@ -238,7 +238,7 @@ func (o *Options) Validate() error {
 		if err != nil {
 			return fmt.Errorf("bad authorize-url %s : %v", o.AuthorizeURLString, err)
 		}
-		o.AuthorizeURL = *u
+		o.AuthorizeURL = u
 	}
 
 	if o.AuthenticateInternalAddrString != "" {
@@ -246,7 +246,7 @@ func (o *Options) Validate() error {
 		if err != nil {
 			return fmt.Errorf("bad authenticate-internal-addr %s : %v", o.AuthenticateInternalAddrString, err)
 		}
-		o.AuthenticateInternalAddr = *u
+		o.AuthenticateInternalAddr = u
 	}
 	if o.PolicyFile != "" {
 		return errors.New("policy file setting is deprecated")

internal/config/options_test.go

@@ -150,8 +150,8 @@ func Test_OptionsFromViper(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	goodOptions.AuthorizeURL = *authorize
+	goodOptions.AuthorizeURL = authorize
-	goodOptions.AuthenticateURL = *authenticate
+	goodOptions.AuthenticateURL = authenticate
 	if err := goodOptions.Validate(); err != nil {
 		t.Fatal(err)
 	}

internal/identity/providers.go

@@ -84,7 +84,7 @@ func New(providerName string, p *Provider) (a Authenticator, err error) {
 type Provider struct {
 	ProviderName string
 
-	RedirectURL  url.URL
+	RedirectURL  *url.URL
 	ClientID     string
 	ClientSecret string
 	ProviderURL  string

internal/urlutil/url.go

@@ -25,6 +25,9 @@ func StripPort(hostport string) string {
 // ParseAndValidateURL wraps standard library's default url.Parse because
 // it's much more lenient about what type of urls it accepts than pomerium.
 func ParseAndValidateURL(rawurl string) (*url.URL, error) {
+	if rawurl == "" {
+		return nil, fmt.Errorf("url cannot be empty")
+	}
 	u, err := url.Parse(rawurl)
 	if err != nil {
 		return nil, err
@@ -37,3 +40,10 @@ func ParseAndValidateURL(rawurl string) (*url.URL, error) {
 	}
 	return u, nil
 }
+
+func DeepCopy(u *url.URL) (*url.URL, error) {
+	if u == nil {
+		return nil, nil
+	}
+	return ParseAndValidateURL(u.String())
+}

internal/urlutil/url_test.go

@@ -2,6 +2,7 @@ package urlutil
 
 import (
 	"net/url"
+	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -45,6 +46,7 @@ func TestParseAndValidateURL(t *testing.T) {
 		{"bad schema", "//some.example", nil, true},
 		{"bad hostname", "https://", nil, true},
 		{"bad parse", "https://^", nil, true},
+		{"empty string error", "", nil, true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -59,3 +61,29 @@ func TestParseAndValidateURL(t *testing.T) {
 		})
 	}
 }
+
+func TestDeepCopy(t *testing.T) {
+
+	tests := []struct {
+		name    string
+		u       *url.URL
+		want    *url.URL
+		wantErr bool
+	}{
+		{"nil", nil, nil, false},
+		{"good", &url.URL{Scheme: "https", Host: "some.example"}, &url.URL{Scheme: "https", Host: "some.example"}, false},
+		{"bad no scheme", &url.URL{Host: "some.example"}, nil, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := DeepCopy(tt.u)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("DeepCopy() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("DeepCopy() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

proxy/handlers.go

@@ -70,7 +70,7 @@ func (p *Proxy) SignOut(w http.ResponseWriter, r *http.Request) {
 			redirectURL = uri
 		}
 	}
-	http.Redirect(w, r, p.GetSignOutURL(p.AuthenticateURL, redirectURL).String(), http.StatusFound)
+	http.Redirect(w, r, p.GetSignOutURL(p.authenticateURL, redirectURL).String(), http.StatusFound)
 }
 
 // OAuthStart begins the authenticate flow, encrypting the redirect url
@@ -116,7 +116,7 @@ func (p *Proxy) OAuthStart(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	signinURL := p.GetSignInURL(p.AuthenticateURL, p.GetRedirectURL(r.Host), remoteState)
+	signinURL := p.GetSignInURL(p.authenticateURL, p.GetRedirectURL(r.Host), remoteState)
 	log.FromRequest(r).Debug().Str("SigninURL", signinURL.String()).Msg("proxy: oauth start")
 
 	// Redirect the user to the authenticate service along with the encrypted
@@ -336,7 +336,7 @@ func (p *Proxy) UserDashboard(w http.ResponseWriter, r *http.Request) {
 		User:             session.User,
 		Groups:           session.Groups,
 		RefreshDeadline:  time.Until(session.RefreshDeadline).Round(time.Second).String(),
-		SignoutURL:       p.GetSignOutURL(p.AuthenticateURL, redirectURL).String(),
+		SignoutURL:       p.GetSignOutURL(p.authenticateURL, redirectURL).String(),
 		IsAdmin:          isAdmin,
 		ImpersonateEmail: session.ImpersonateEmail,
 		ImpersonateGroup: strings.Join(session.ImpersonateGroups, ","),

proxy/handlers_test.go

@@ -166,7 +166,7 @@ func TestProxy_Signout(t *testing.T) {
 		t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusFound)
 	}
 	body := rr.Body.String()
-	want := (proxy.AuthenticateURL.String())
+	want := (proxy.authenticateURL.String())
 	if !strings.Contains(body, want) {
 		t.Errorf("handler returned unexpected body: got %v want %s ", body, want)
 	}

proxy/proxy.go

@@ -4,7 +4,6 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
-	"errors"
 	"fmt"
 	"html/template"
 	stdlog "log"
@@ -23,6 +22,7 @@ import (
 	"github.com/pomerium/pomerium/internal/telemetry/trace"
 	"github.com/pomerium/pomerium/internal/templates"
 	"github.com/pomerium/pomerium/internal/tripper"
+	"github.com/pomerium/pomerium/internal/urlutil"
 	"github.com/pomerium/pomerium/proxy/clients"
 )
 
@@ -47,36 +47,44 @@ func ValidateOptions(o config.Options) error {
 	if len(decoded) != 32 {
 		return fmt.Errorf("`SHARED_SECRET` want 32 but got %d bytes", len(decoded))
 	}
-	if o.AuthenticateURL.String() == "" {
+
-		return errors.New("missing setting: authenticate-service-url")
+	if o.AuthenticateURL == nil {
+		return fmt.Errorf("proxy: missing setting: authenticate-service-url")
 	}
-	if o.AuthenticateURL.Scheme != "https" {
+	if _, err := urlutil.ParseAndValidateURL(o.AuthenticateURL.String()); err != nil {
-		return errors.New("authenticate-service-url must be a valid https url")
+		return fmt.Errorf("proxy: error parsing authenticate url: %v", err)
 	}
-	if o.AuthorizeURL.String() == "" {
+
-		return errors.New("missing setting: authorize-service-url")
+	if o.AuthorizeURL == nil {
+		return fmt.Errorf("proxy: missing setting: authenticate-service-url")
 	}
-	if o.AuthorizeURL.Scheme != "https" {
+	if _, err := urlutil.ParseAndValidateURL(o.AuthorizeURL.String()); err != nil {
-		return errors.New("authorize-service-url must be a valid https url")
+		return fmt.Errorf("proxy: error parsing authorize url: %v", err)
 	}
+	if o.AuthenticateInternalAddr != nil {
+		if _, err := urlutil.ParseAndValidateURL(o.AuthenticateInternalAddr.String()); err != nil {
+			return fmt.Errorf("proxy: error parsing authorize url: %v", err)
+		}
+	}
+
 	if o.CookieSecret == "" {
-		return errors.New("missing setting: cookie-secret")
+		return fmt.Errorf("proxy: missing setting: cookie-secret")
 	}
 	decodedCookieSecret, err := base64.StdEncoding.DecodeString(o.CookieSecret)
 	if err != nil {
-		return fmt.Errorf("cookie secret is invalid base64: %v", err)
+		return fmt.Errorf("proxy: cookie secret is invalid base64: %v", err)
 	}
 	if len(decodedCookieSecret) != 32 {
-		return fmt.Errorf("cookie secret expects 32 bytes but got %d", len(decodedCookieSecret))
+		return fmt.Errorf("proxy: cookie secret expects 32 bytes but got %d", len(decodedCookieSecret))
 	}
 	if len(o.SigningKey) != 0 {
 		decodedSigningKey, err := base64.StdEncoding.DecodeString(o.SigningKey)
 		if err != nil {
-			return fmt.Errorf("signing key is invalid base64: %v", err)
+			return fmt.Errorf("proxy: signing key is invalid base64: %v", err)
 		}
 		_, err = cryptutil.NewES256Signer(decodedSigningKey, "localhost")
 		if err != nil {
-			return fmt.Errorf("invalid signing key is : %v", err)
+			return fmt.Errorf("proxy: invalid signing key is : %v", err)
 		}
 	}
 	return nil
@@ -85,10 +93,12 @@ func ValidateOptions(o config.Options) error {
 // Proxy stores all the information associated with proxying a request.
 type Proxy struct {
 	// SharedKey used to mutually authenticate service communication
-	SharedKey          string
+	SharedKey                string
-	AuthenticateURL    *url.URL
+	authenticateURL          *url.URL
-	AuthenticateClient clients.Authenticator
+	authenticateInternalAddr *url.URL
-	AuthorizeClient    clients.Authorizer
+	authorizeURL             *url.URL
+	AuthenticateClient       clients.Authenticator
+	AuthorizeClient          clients.Authorizer
 
 	cipher                 cryptutil.Cipher
 	cookieName             string
@@ -142,8 +152,6 @@ func New(opts config.Options) (*Proxy, error) {
 		SharedKey: opts.SharedKey,
 
 		routeConfigs: make(map[string]*routeConfig),
-		// services
-		AuthenticateURL: &opts.AuthenticateURL,
 
 		cipher:                 cipher,
 		cookieName:             opts.CookieName,
@@ -156,6 +164,10 @@ func New(opts config.Options) (*Proxy, error) {
 		signingKey:             opts.SigningKey,
 		templates:              templates.New(),
 	}
+	// DeepCopy urls to avoid accidental mutation, err checked in validate func
+	p.authenticateURL, _ = urlutil.DeepCopy(opts.AuthenticateURL)
+	p.authorizeURL, _ = urlutil.DeepCopy(opts.AuthorizeURL)
+	p.authenticateInternalAddr, _ = urlutil.DeepCopy(opts.AuthenticateInternalAddr)
 
 	if err := p.UpdatePolicies(&opts); err != nil {
 		return nil, err
@@ -165,8 +177,8 @@ func New(opts config.Options) (*Proxy, error) {
 	})
 	p.AuthenticateClient, err = clients.NewAuthenticateClient("grpc",
 		&clients.Options{
-			Addr:                    &opts.AuthenticateURL,
+			Addr:                    p.authenticateURL,
-			InternalAddr:            &opts.AuthenticateInternalAddr,
+			InternalAddr:            p.authenticateInternalAddr,
 			OverrideCertificateName: opts.OverrideCertificateName,
 			SharedSecret:            opts.SharedKey,
 			CA:                      opts.CA,
@@ -177,7 +189,7 @@ func New(opts config.Options) (*Proxy, error) {
 	}
 	p.AuthorizeClient, err = clients.NewAuthorizeClient("grpc",
 		&clients.Options{
-			Addr:                    &opts.AuthorizeURL,
+			Addr:                    p.authorizeURL,
 			OverrideCertificateName: opts.OverrideCertificateName,
 			SharedSecret:            opts.SharedKey,
 			CA:                      opts.CA,

proxy/proxy_test.go

@@ -96,8 +96,8 @@ func testOptions(t *testing.T) config.Options {
 	opts := newTestOptions(t)
 	testPolicy := config.Policy{From: "https://corp.example.example", To: "https://example.example"}
 	opts.Policies = []config.Policy{testPolicy}
-	opts.AuthenticateURL = *authenticateService
+	opts.AuthenticateURL = authenticateService
-	opts.AuthorizeURL = *authorizeService
+	opts.AuthorizeURL = authorizeService
 	opts.SharedKey = "80ldlrU2d7w+wVpKNfevk6fmb8otEx6CqOfshj2LwhQ="
 	opts.CookieSecret = "OromP1gurwGWjQPYb1nNgSxtbVB5NnLzX6z5WOKr0Yw="
 	opts.CookieName = "pomerium"
@@ -120,8 +120,8 @@ func testOptionsTestServer(t *testing.T, uri string) config.Options {
 	}
 	opts := newTestOptions(t)
 	opts.Policies = []config.Policy{testPolicy}
-	opts.AuthenticateURL = *authenticateService
+	opts.AuthenticateURL = authenticateService
-	opts.AuthorizeURL = *authorizeService
+	opts.AuthorizeURL = authorizeService
 	opts.SharedKey = "80ldlrU2d7w+wVpKNfevk6fmb8otEx6CqOfshj2LwhQ="
 	opts.CookieSecret = "OromP1gurwGWjQPYb1nNgSxtbVB5NnLzX6z5WOKr0Yw="
 	opts.CookieName = "pomerium"
@@ -165,14 +165,17 @@ func testOptionsWithEmptyPolicies(t *testing.T, uri string) config.Options {
 func TestOptions_Validate(t *testing.T) {
 	good := testOptions(t)
 	badAuthURL := testOptions(t)
-	badAuthURL.AuthenticateURL = url.URL{}
+	badAuthURL.AuthenticateURL = nil
-	authurl, _ := url.Parse("http://authenticate.corp.beyondperimeter.com")
+	authurl, _ := url.Parse("authenticate.corp.beyondperimeter.com")
 	authenticateBadScheme := testOptions(t)
-	authenticateBadScheme.AuthenticateURL = *authurl
+	authenticateBadScheme.AuthenticateURL = authurl
+	authenticateInternalBadScheme := testOptions(t)
+	authenticateInternalBadScheme.AuthenticateInternalAddr = authurl
+
 	authorizeBadSCheme := testOptions(t)
-	authorizeBadSCheme.AuthorizeURL = *authurl
+	authorizeBadSCheme.AuthorizeURL = authurl
 	authorizeNil := testOptions(t)
-	authorizeNil.AuthorizeURL = url.URL{}
+	authorizeNil.AuthorizeURL = nil
 	emptyCookieSecret := testOptions(t)
 	emptyCookieSecret.CookieSecret = ""
 	invalidCookieSecret := testOptions(t)
@@ -196,8 +199,9 @@ func TestOptions_Validate(t *testing.T) {
 		{"good - minimum options", good, false},
 		{"nil options", config.Options{}, true},
 		{"authenticate service url", badAuthURL, true},
-		{"authenticate service url not https", authenticateBadScheme, true},
+		{"authenticate service url no scheme", authenticateBadScheme, true},
-		{"authorize service url not https", authorizeBadSCheme, true},
+		{"internal authenticate service url no scheme", authenticateInternalBadScheme, true},
+		{"authorize service url no scheme", authorizeBadSCheme, true},
 		{"authorize service cannot be nil", authorizeNil, true},
 		{"no cookie secret", emptyCookieSecret, true},
 		{"invalid cookie secret", invalidCookieSecret, true},