diff --git a/authenticate/authenticate.go b/authenticate/authenticate.go index a553a2753..76fc777b7 100644 --- a/authenticate/authenticate.go +++ b/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() == "" { - return errors.New("authenticate: 'AUTHENTICATE_SERVICE_URL' missing") + if o.AuthenticateURL == nil { + 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, diff --git a/authenticate/authenticate_test.go b/authenticate/authenticate_test.go index 3fb9fd4cb..4567fe3ff 100644 --- a/authenticate/authenticate_test.go +++ b/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 = "" diff --git a/cmd/pomerium/main_test.go b/cmd/pomerium/main_test.go index 6c6f24333..68f2a653b 100644 --- a/cmd/pomerium/main_test.go +++ b/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.AuthorizeURL = *AuthorizeURL + testOpts.AuthenticateURL = AuthenticateURL + testOpts.AuthorizeURL = AuthorizeURL testOpts.CookieSecret = "YixWi1MYh77NMECGGIJQevoonYtVF+ZPRkQZrrmeRqM=" testOpts.Services = tt.s diff --git a/internal/config/options.go b/internal/config/options.go index 26c6791b6..8b56593e3 100644 --- a/internal/config/options.go +++ b/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") diff --git a/internal/config/options_test.go b/internal/config/options_test.go index 318d58686..69f3e132e 100644 --- a/internal/config/options_test.go +++ b/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.AuthenticateURL = *authenticate + goodOptions.AuthorizeURL = authorize + goodOptions.AuthenticateURL = authenticate if err := goodOptions.Validate(); err != nil { t.Fatal(err) } diff --git a/internal/identity/providers.go b/internal/identity/providers.go index 0e18a621d..394e69566 100644 --- a/internal/identity/providers.go +++ b/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 diff --git a/internal/urlutil/url.go b/internal/urlutil/url.go index cc90c893e..06db9b72e 100644 --- a/internal/urlutil/url.go +++ b/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()) +} diff --git a/internal/urlutil/url_test.go b/internal/urlutil/url_test.go index 707ce4346..3c9706b51 100644 --- a/internal/urlutil/url_test.go +++ b/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) + } + }) + } +} diff --git a/proxy/handlers.go b/proxy/handlers.go index a5e914eca..7fe44a362 100644 --- a/proxy/handlers.go +++ b/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, ","), diff --git a/proxy/handlers_test.go b/proxy/handlers_test.go index aa7ecbe74..e68d7ddea 100644 --- a/proxy/handlers_test.go +++ b/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) } diff --git a/proxy/proxy.go b/proxy/proxy.go index 2b0abb271..d22ac1156 100755 --- a/proxy/proxy.go +++ b/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" { - return errors.New("authenticate-service-url must be a valid https url") + if _, err := urlutil.ParseAndValidateURL(o.AuthenticateURL.String()); err != nil { + 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" { - return errors.New("authorize-service-url must be a valid https url") + if _, err := urlutil.ParseAndValidateURL(o.AuthorizeURL.String()); err != nil { + 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 - AuthenticateURL *url.URL - AuthenticateClient clients.Authenticator - AuthorizeClient clients.Authorizer + SharedKey string + authenticateURL *url.URL + authenticateInternalAddr *url.URL + 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, - InternalAddr: &opts.AuthenticateInternalAddr, + Addr: p.authenticateURL, + 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, diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 7eb3cc384..46960f3f0 100644 --- a/proxy/proxy_test.go +++ b/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.AuthorizeURL = *authorizeService + opts.AuthenticateURL = authenticateService + 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.AuthorizeURL = *authorizeService + opts.AuthenticateURL = authenticateService + 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{} - authurl, _ := url.Parse("http://authenticate.corp.beyondperimeter.com") + badAuthURL.AuthenticateURL = nil + 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}, - {"authorize service url not https", authorizeBadSCheme, true}, + {"authenticate service url no scheme", authenticateBadScheme, 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},