diff --git a/CHANGELOG.md b/CHANGELOG.md index ca81f77a..c7a3c246 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- fix OAuth `redirect_uri` race with multiple `callback_urls` + Coming soon! Please document any work in progress here as part of your PR. It will be moved to the next tag when released. ## v0.50.0 diff --git a/handlers/login.go b/handlers/login.go index 34f26f58..d2297dd1 100644 --- a/handlers/login.go +++ b/handlers/login.go @@ -24,6 +24,7 @@ import ( "github.com/vouch/vouch-proxy/pkg/cfg" "github.com/vouch/vouch-proxy/pkg/cookie" "github.com/vouch/vouch-proxy/pkg/domains" + "github.com/vouch/vouch-proxy/pkg/providers/common" "github.com/vouch/vouch-proxy/pkg/responses" "golang.org/x/oauth2" ) @@ -250,28 +251,11 @@ func oauthLoginURL(r *http.Request, session sessions.Session) string { // See relevant RFC: http://tools.ietf.org/html/rfc6749#section-10.12 var state string = session.Values["state"].(string) opts := []oauth2.AuthCodeOption{} + oauthClient := common.OAuthClientForRequest(r) if cfg.GenOAuth.Provider == cfg.Providers.IndieAuth { - return cfg.OAuthClient.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id")) + return oauthClient.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id")) } - // cfg.OAuthClient.RedirectURL is set in cfg - // this checks the multiple redirect case for multiple matching domains - if len(cfg.GenOAuth.RedirectURLs) > 0 { - found := false - domain := domains.Matches(r.Host) - log.Debugf("/login looking for callback_url matching %s from host %s", domain, r.Host) - for _, v := range cfg.GenOAuth.RedirectURLs { - if strings.Contains(v, domain) { - found = true - log.Debugf("/login callback_url set to %s", v) - cfg.OAuthClient.RedirectURL = v - break - } - } - if !found { - log.Infof("/login no callback_url matched %s (is the `Host` header being passed to Vouch Proxy?)", domain) - } - } // append code challenge and code challenge method query parameters if enabled if cfg.GenOAuth.CodeChallengeMethod != "" { @@ -281,7 +265,7 @@ func oauthLoginURL(r *http.Request, session sessions.Session) string { if cfg.OAuthopts != nil { opts = append(opts, cfg.OAuthopts...) } - return cfg.OAuthClient.AuthCodeURL(state, opts...) + return oauthClient.AuthCodeURL(state, opts...) } var regExJustAlphaNum, _ = regexp.Compile("[^a-zA-Z0-9]+") diff --git a/handlers/login_test.go b/handlers/login_test.go index 71f519a9..09abf2d8 100644 --- a/handlers/login_test.go +++ b/handlers/login_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/gorilla/sessions" "github.com/stretchr/testify/assert" "github.com/vouch/vouch-proxy/pkg/cfg" ) @@ -219,6 +220,49 @@ func TestLoginHandler(t *testing.T) { }) } } + +func TestOAuthLoginURLUsesRequestCallbackURLWithoutMutatingGlobalConfig(t *testing.T) { + setUp("/config/testing/handler_login_redirecturls.yml") + startupRedirectURL := cfg.OAuthClient.RedirectURL + + firstRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + firstRequest.Host = "vouch.example.com" + firstURL, err := url.Parse(oauthLoginURL(firstRequest, loginSession("first"))) + assert.NoError(t, err) + assert.Equal(t, "http://vouch.example.com:9090/auth", firstURL.Query().Get("redirect_uri")) + + secondRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + secondRequest.Host = "vouch.other.com" + secondURL, err := url.Parse(oauthLoginURL(secondRequest, loginSession("second"))) + assert.NoError(t, err) + assert.Equal(t, "http://vouch.other.com:9090/auth", secondURL.Query().Get("redirect_uri")) + + assert.Equal(t, "http://vouch.example.com:9090/auth", firstURL.Query().Get("redirect_uri")) + assert.Equal(t, startupRedirectURL, cfg.OAuthClient.RedirectURL) +} + +func TestOAuthLoginURLUsesRequestCallbackURLForIndieAuth(t *testing.T) { + setUp("/config/testing/handler_login_redirecturls.yml") + startupRedirectURL := cfg.OAuthClient.RedirectURL + cfg.GenOAuth.Provider = cfg.Providers.IndieAuth + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + request.Host = "vouch.other.com" + oauthURL, err := url.Parse(oauthLoginURL(request, loginSession("state"))) + + assert.NoError(t, err) + assert.Equal(t, "http://vouch.other.com:9090/auth", oauthURL.Query().Get("redirect_uri")) + assert.Equal(t, "id", oauthURL.Query().Get("response_type")) + assert.Equal(t, startupRedirectURL, cfg.OAuthClient.RedirectURL) +} + +func loginSession(state string) sessions.Session { + return sessions.Session{Values: map[interface{}]interface{}{ + "state": state, + "codeChallenge": "challenge", + }} +} + func TestLoginErrTooManyRedirects(t *testing.T) { handler := http.HandlerFunc(LoginHandler) diff --git a/pkg/providers/common/common.go b/pkg/providers/common/common.go index 7a23e4e7..34dbdd2a 100644 --- a/pkg/providers/common/common.go +++ b/pkg/providers/common/common.go @@ -14,11 +14,13 @@ import ( "context" "encoding/json" "net/http" + "strings" "go.uber.org/zap" "golang.org/x/oauth2" "github.com/vouch/vouch-proxy/pkg/cfg" + "github.com/vouch/vouch-proxy/pkg/domains" "github.com/vouch/vouch-proxy/pkg/structs" ) @@ -29,9 +31,36 @@ func Configure() { log = cfg.Logging.Logger } +// OAuthClientForRequest returns a request-local OAuth config. When several +// callback URLs are configured, it selects the callback associated with the +// request Host without modifying the shared config created at startup. +func OAuthClientForRequest(r *http.Request) *oauth2.Config { + oauthClient := *cfg.OAuthClient + + if len(cfg.GenOAuth.RedirectURLs) == 0 { + return &oauthClient + } + + domain := domains.Matches(r.Host) + log.Debugf("/login looking for callback_url matching %s from host %s", domain, r.Host) + if domain != "" { + for _, callbackURL := range cfg.GenOAuth.RedirectURLs { + if strings.Contains(callbackURL, domain) { + log.Debugf("/login callback_url set to %s", callbackURL) + oauthClient.RedirectURL = callbackURL + return &oauthClient + } + } + } + + log.Infof("/login no callback_url matched %s (is the `Host` header being passed to Vouch Proxy?)", domain) + return &oauthClient +} + // PrepareTokensAndClient setup the client, usually for a UserInfo request func PrepareTokensAndClient(r *http.Request, ptokens *structs.PTokens, setProviderToken bool, opts ...oauth2.AuthCodeOption) (*http.Client, *oauth2.Token, error) { - providerToken, err := cfg.OAuthClient.Exchange(context.TODO(), r.URL.Query().Get("code"), opts...) + oauthClient := OAuthClientForRequest(r) + providerToken, err := oauthClient.Exchange(context.TODO(), r.URL.Query().Get("code"), opts...) if err != nil { return nil, nil, err } @@ -48,7 +77,7 @@ func PrepareTokensAndClient(r *http.Request, ptokens *structs.PTokens, setProvid } log.Debugf("ptokens: accessToken length: %d, IdToken length: %d", len(ptokens.PAccessToken), len(ptokens.PIdToken)) - client := cfg.OAuthClient.Client(context.TODO(), providerToken) + client := oauthClient.Client(context.TODO(), providerToken) return client, providerToken, err } diff --git a/pkg/providers/common/common_test.go b/pkg/providers/common/common_test.go new file mode 100644 index 00000000..4e3ddfae --- /dev/null +++ b/pkg/providers/common/common_test.go @@ -0,0 +1,129 @@ +/* + +Copyright 2020 The Vouch Proxy Authors. +Use of this source code is governed by The MIT License (MIT) that +can be found in the LICENSE file. Software distributed under The +MIT License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES +OR CONDITIONS OF ANY KIND, either express or implied. + +*/ + +package common + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + "golang.org/x/oauth2" + + "github.com/vouch/vouch-proxy/pkg/cfg" + "github.com/vouch/vouch-proxy/pkg/domains" + "github.com/vouch/vouch-proxy/pkg/structs" +) + +func setUpOAuthClientForRequestTest(t *testing.T) { + t.Helper() + + previousCfg := cfg.Cfg + previousOAuthClient := cfg.OAuthClient + previousRedirectURLs := cfg.GenOAuth.RedirectURLs + previousLog := log + + cfg.Cfg = &cfg.Config{Domains: []string{"example.com", "other.com"}} + cfg.OAuthClient = &oauth2.Config{RedirectURL: "https://startup.example.com/auth"} + cfg.GenOAuth.RedirectURLs = []string{ + "https://vouch.example.com/auth", + "https://vouch.other.com/auth", + } + log = zap.NewNop().Sugar() + domains.Configure() + + t.Cleanup(func() { + cfg.Cfg = previousCfg + cfg.OAuthClient = previousOAuthClient + cfg.GenOAuth.RedirectURLs = previousRedirectURLs + log = previousLog + }) +} + +func TestOAuthClientForRequest(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + firstRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + firstRequest.Host = "vouch.example.com" + firstClient := OAuthClientForRequest(firstRequest) + assert.Equal(t, "https://vouch.example.com/auth", firstClient.RedirectURL) + + secondRequest := httptest.NewRequest(http.MethodGet, "/login", nil) + secondRequest.Host = "vouch.other.com" + secondClient := OAuthClientForRequest(secondRequest) + assert.Equal(t, "https://vouch.other.com/auth", secondClient.RedirectURL) + + assert.Equal(t, "https://vouch.example.com/auth", firstClient.RedirectURL) + assert.Equal(t, "https://startup.example.com/auth", cfg.OAuthClient.RedirectURL) +} + +func TestOAuthClientForRequestFallsBackToStartupRedirectURL(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + for _, host := range []string{"", "vouch.unknown.test"} { + request.Host = host + assert.Equal(t, cfg.OAuthClient.RedirectURL, OAuthClientForRequest(request).RedirectURL) + } + + cfg.GenOAuth.RedirectURLs = nil + request.Host = "vouch.example.com" + assert.Equal(t, cfg.OAuthClient.RedirectURL, OAuthClientForRequest(request).RedirectURL) +} + +func TestPrepareTokensAndClientUsesRequestCallbackURL(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + var redirectURIs []string + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + t.Fatal(err) + } + redirectURIs = append(redirectURIs, r.Form.Get("redirect_uri")) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"token","token_type":"Bearer"}`) + })) + defer tokenServer.Close() + + cfg.OAuthClient.Endpoint.TokenURL = tokenServer.URL + cfg.OAuthClient.ClientID = "client-id" + cfg.OAuthClient.ClientSecret = "client-secret" + + for _, host := range []string{"vouch.example.com", "vouch.other.com"} { + request := httptest.NewRequest(http.MethodGet, "/auth/state/?code=code", nil) + request.Host = host + _, _, err := PrepareTokensAndClient(request, &structs.PTokens{}, false) + assert.NoError(t, err) + } + + assert.Equal(t, []string{ + "https://vouch.example.com/auth", + "https://vouch.other.com/auth", + }, redirectURIs) +} + +func TestOAuthClientForRequestPreservesSingleCallbackConfiguration(t *testing.T) { + setUpOAuthClientForRequestTest(t) + + singleCallbackURL := "https://vouch.single.example.com/auth" + cfg.OAuthClient.RedirectURL = singleCallbackURL + cfg.GenOAuth.RedirectURLs = nil + + request := httptest.NewRequest(http.MethodGet, "/login", nil) + request.Host = "vouch.example.com" + oauthClient := OAuthClientForRequest(request) + + assert.Equal(t, singleCallbackURL, oauthClient.RedirectURL) + assert.Equal(t, singleCallbackURL, cfg.OAuthClient.RedirectURL) + assert.NotSame(t, cfg.OAuthClient, oauthClient) +}