Fix OAuth redirect_uri race with multiple callback URLs

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

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]+")

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)

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
 }
 

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)
+}