Fix improper redirect URL validation for /web/sso_confirm

lib/auth/github.go

@@ -1024,20 +1024,34 @@ func ValidateClientRedirect(clientRedirect string, ssoTestFlow bool, settings *t
 		// they're used a lot in test code
 		return nil
 	}
+
 	u, err := url.Parse(clientRedirect)
 	if err != nil {
 		return trace.Wrap(err, "parsing client redirect URL")
 	}
-	if u.Path == sso.WebMFARedirect {
-		// If this is a SSO redirect in the WebUI, allow.
-		return nil
-	}
+
 	if u.Opaque != "" {
 		return trace.BadParameter("unexpected opaque client redirect URL")
 	}
 	if u.User != nil {
 		return trace.BadParameter("unexpected userinfo in client redirect URL")
 	}
+	if u.Fragment != "" || u.RawFragment != "" {
+		return trace.BadParameter("unexpected fragment in client redirect URL")
+	}
+
+	// For Web MFA redirect, we expect a relative path. The proxy handling the SSO callback
+	// will will redirect to itself with this relative path.
+	if u.Path == sso.WebMFARedirect {
+		if u.IsAbs() {
+			return trace.BadParameter("invalid scheme in client redirect URL for SSO MFA")
+		}
+		if u.Hostname() != "" {
+			return trace.BadParameter("invalid host name in client redirect URL for SSO MFA")
+		}
+		return nil
+	}
+
 	if u.EscapedPath() != "/callback" {
 		return trace.BadParameter("invalid path in client redirect URL")
 	}
@@ -1046,9 +1060,6 @@ func ValidateClientRedirect(clientRedirect string, ssoTestFlow bool, settings *t
 	} else if len(q) != 1 || len(q["secret_key"]) != 1 {
 		return trace.BadParameter("malformed query parameters in client redirect URL")
 	}
-	if u.Fragment != "" || u.RawFragment != "" {
-		return trace.BadParameter("unexpected fragment in client redirect URL")
-	}
 
 	// we checked everything but u.Scheme and u.Host now
 	if u.Scheme != "http" && u.Scheme != "https" {

lib/auth/github_test.go

@@ -43,6 +43,7 @@ import (
 	"github.com/gravitational/teleport/lib/authz"
 	"github.com/gravitational/teleport/lib/backend"
 	"github.com/gravitational/teleport/lib/backend/memory"
+	"github.com/gravitational/teleport/lib/client/sso"
 	"github.com/gravitational/teleport/lib/events"
 	"github.com/gravitational/teleport/lib/events/eventstest"
 	"github.com/gravitational/teleport/lib/loginrule"
@@ -784,4 +785,33 @@ func TestValidateClientRedirect(t *testing.T) {
 			require.Error(t, ValidateClientRedirect(badURL+"?secret_key=", ssoTestFlowTrue, settings))
 		}
 	})
+
+	t.Run("SSOMFAWeb", func(t *testing.T) {
+		const ssoTestFlowFalse = false
+		settings := &types.SSOClientRedirectSettings{
+			AllowedHttpsHostnames: []string{
+				"allowed.domain.invalid",
+			},
+		}
+
+		// Only allow web mfa redirect as a relative path.
+		require.NoError(t, ValidateClientRedirect(sso.WebMFARedirect+"?channel_id=", ssoTestFlowFalse, settings))
+
+		for _, badURL := range []string{
+			"localhost:12345" + sso.WebMFARedirect,
+			"http://localhost:12345" + sso.WebMFARedirect,
+			"https://localhost:12345" + sso.WebMFARedirect,
+			"127.0.0.1:12345" + sso.WebMFARedirect,
+			"http://127.0.0.1:12345" + sso.WebMFARedirect,
+			"https://127.0.0.1:12345" + sso.WebMFARedirect,
+			"allowed.domain.invalid" + sso.WebMFARedirect,
+			"http://allowed.domain.invalid" + sso.WebMFARedirect,
+			"https://allowed.domain.invalid" + sso.WebMFARedirect,
+			"not.allowed.domain.invalid" + sso.WebMFARedirect,
+			"http://not.allowed.domain.invalid" + sso.WebMFARedirect,
+			"https://not.allowed.domain.invalid" + sso.WebMFARedirect,
+		} {
+			require.Error(t, ValidateClientRedirect(badURL+"?channel_id=", ssoTestFlowFalse, settings))
+		}
+	})
 }

lib/web/apiserver.go

@@ -2329,18 +2329,19 @@ func ConstructSSHResponse(response AuthParams) (*url.URL, error) {
 		return nil, trace.Wrap(err)
 	}
 
-	// Extract secret out of the request.
-	secretKey := u.Query().Get("secret_key")
-
-	// We don't use a secret key for WebUI SSO MFA redirects. The request ID itself is
-	// kept a secret on the front end to minimize the risk of a phishing attack.
-	if secretKey == "" && u.Path == sso.WebMFARedirect && response.MFAToken != "" {
+	if u.Path == sso.WebMFARedirect {
+		// Transform the web sso mfa redirectURL into a relative redirect, preserving
+		// query parameters while ignoring scheme, hostname, and other url parts.
 		q := u.Query()
 		q.Add("response", string(out))
-		u.RawQuery = q.Encode()
-		return u, nil
+		return &url.URL{
+			Path:     sso.WebMFARedirect,
+			RawQuery: q.Encode(),
+		}, nil
 	}
 
+	// Extract secret out of the request.
+	secretKey := u.Query().Get("secret_key")
 	if secretKey == "" {
 		return nil, trace.BadParameter("missing secret_key")
 	}

lib/web/apiserver_test.go

@@ -111,6 +111,7 @@ import (
 	"github.com/gravitational/teleport/lib/client"
 	"github.com/gravitational/teleport/lib/client/conntest"
 	dbrepl "github.com/gravitational/teleport/lib/client/db/repl"
+	"github.com/gravitational/teleport/lib/client/sso"
 	"github.com/gravitational/teleport/lib/cryptosuites"
 	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/events"
@@ -3228,6 +3229,35 @@ func (f byTimeAndIndex) Swap(i, j int) {
 	f[i], f[j] = f[j], f[i]
 }
 
+// ConstructSSHResponse_WebMFA tests that [sso.WebMFARedirect] is always
+// transformed into a relative URL so that the sso callback will redirect
+// back to the proxy.
+func TestConstructSSHResponse_WebMFA(t *testing.T) {
+	baseURL := sso.WebMFARedirect + "?channel_id=" + uuid.NewString()
+	for _, clientRedirectURL := range []string{
+		baseURL,
+		"http://127.0.0.1:1234" + baseURL,
+		"http://localhost:1234" + baseURL,
+		"https://proxy.example.com" + baseURL,
+		"tcp://tcp.app.com" + baseURL,
+	} {
+		t.Run("clientRedirectURL: "+clientRedirectURL, func(t *testing.T) {
+			redirectURL, err := ConstructSSHResponse(AuthParams{
+				Username:          "foo",
+				Cert:              []byte{0x00},
+				TLSCert:           []byte{0x01},
+				MFAToken:          "token",
+				ClientRedirectURL: clientRedirectURL,
+			})
+			require.NoError(t, err)
+			require.NotEmpty(t, redirectURL.Query().Get("channel_id"))
+			require.NotEmpty(t, redirectURL.Query().Get("response"))
+			withoutQueryParams, _, _ := strings.Cut(redirectURL.String(), "?")
+			require.Equal(t, sso.WebMFARedirect, withoutQueryParams)
+		})
+	}
+}
+
 // TestSearchClusterEvents makes sure web API allows querying events by type.
 func TestSearchClusterEvents(t *testing.T) {
 	t.Parallel()