diff --git a/lib/auth/github.go b/lib/auth/github.go index d055ee8650b19..c2b92bbffc75d 100644 --- a/lib/auth/github.go +++ b/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" { diff --git a/lib/auth/github_test.go b/lib/auth/github_test.go index abeba08850046..ad5ab2f35bf11 100644 --- a/lib/auth/github_test.go +++ b/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)) + } + }) } diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 1c9bf46f54378..341546f09fc31 100644 --- a/lib/web/apiserver.go +++ b/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") } diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 460e8243f88a3..1ec72d5c1844d 100644 --- a/lib/web/apiserver_test.go +++ b/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()