Skip to content

Validate that the "/web/sso_confirm" redirect URL is only used for MFA ceremonies #56006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+354 −306
Open

Validate that the "/web/sso_confirm" redirect URL is only used for MFA ceremonies #56006

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
913f71e
* Move ValidateClientRedirect from lib/auth/github.go to lib/client/s…
Joerger Jun 23, 2025
a3af11a
Address comments.
Joerger Jun 23, 2025
9c485e7
Add warning comment; Update error message.
Joerger Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 11 additions & 101 deletions lib/auth/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ import (
"io"
"log/slog"
"net/http"
"net/netip"
"net/url"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -153,7 +151,12 @@ func (a *Server) CreateGithubAuthRequest(ctx context.Context, req types.GithubAu
// checked, as they will point the browser away from the IdP or the web UI
// after the authentication is done
if !req.CreateWebSession {
if err := ValidateClientRedirect(req.ClientRedirectURL, req.SSOTestFlow, connector.GetClientRedirectSettings()); err != nil {
ceremonyType := sso.CeremonyTypeLogin
if req.SSOTestFlow {
ceremonyType = sso.CeremonyTypeTest
}

if err := sso.ValidateClientRedirect(req.ClientRedirectURL, ceremonyType, connector.GetClientRedirectSettings()); err != nil {
return nil, trace.Wrap(err, InvalidClientRedirectErrorMessage)
}
}
Expand Down Expand Up @@ -1005,8 +1008,6 @@ func (a *Server) createGithubUser(ctx context.Context, p *CreateUserParams, dryR
return user, nil
}

const unknownRedirectHostnameErrMsg = "unknown custom client redirect URL hostname"

// ValidateClientRedirect checks a desktop client redirect URL for SSO logins
// against some (potentially nil) settings from an auth connector; in the
// current implementation, that means either "http" schema with a hostname of
Expand All @@ -1018,105 +1019,14 @@ const unknownRedirectHostnameErrMsg = "unknown custom client redirect URL hostna
// list is non-empty URLs in both the "http" and "https" schema are allowed
// if the hostname is an IP address that is contained in a specified CIDR
// range on any port.
//
// TODO(Joerger): Replaced by [sso.ValidateClientRedirect], remove once /e no longer depends on it
func ValidateClientRedirect(clientRedirect string, ssoTestFlow bool, settings *types.SSOClientRedirectSettings) error {
if clientRedirect == "" {
// empty redirects are non-functional and harmless, so we allow them as
// 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.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")
}
if q, err := url.ParseQuery(u.RawQuery); err != nil {
return trace.Wrap(err, "parsing query in client redirect URL")
} else if len(q) != 1 || len(q["secret_key"]) != 1 {
return trace.BadParameter("malformed query parameters in client redirect URL")
}

// we checked everything but u.Scheme and u.Host now
if u.Scheme != "http" && u.Scheme != "https" {
return trace.BadParameter("invalid scheme in client redirect URL")
}

// allow HTTP redirects to local addresses
allowedHTTPLocalAddrs := []string{"localhost", "127.0.0.1", "::1"}
if u.Scheme == "http" && slices.Contains(allowedHTTPLocalAddrs, u.Hostname()) {
return nil
}

ceremonyType := sso.CeremonyTypeLogin
if ssoTestFlow {
return trace.AccessDenied("custom client redirect URLs are not allowed in SSO test")
ceremonyType = sso.CeremonyTypeTest
}

if settings == nil {
return trace.AccessDenied("%s", unknownRedirectHostnameErrMsg)
}

// allow HTTP or HTTPS redirects from IPs in specified CIDR ranges
hostIP, err := netip.ParseAddr(u.Hostname())
if err == nil {
hostIP = hostIP.Unmap()

for _, cidrStr := range settings.InsecureAllowedCidrRanges {
cidr, err := netip.ParsePrefix(cidrStr)
if err != nil {
slog.WarnContext(context.Background(), "error parsing OIDC connector CIDR prefix", "cidr", cidrStr, "err", err)
continue
}
if cidr.Contains(hostIP) {
return nil
}
}
}

if u.Scheme == "https" {
switch u.Port() {
default:
return trace.BadParameter("invalid port in client redirect URL")
case "", "443":
}

for _, expression := range settings.AllowedHttpsHostnames {
ok, err := utils.MatchString(u.Hostname(), expression)
if err != nil {
slog.WarnContext(context.Background(), "error compiling OIDC connector allowed HTTPS hostname regex", "regex", expression, "err", err)
continue
}
if ok {
return nil
}
}
}

return trace.AccessDenied("%s", unknownRedirectHostnameErrMsg)
return sso.ValidateClientRedirect(clientRedirect, ceremonyType, settings)
}

// populateGithubClaims builds a GithubClaims using queried
Expand Down
32 changes: 11 additions & 21 deletions lib/auth/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,9 +622,8 @@ func TestValidateClientRedirect(t *testing.T) {
"http://localhost/callback",
"http://localhost:1234/callback",
} {
const ssoTestFlowFalse = false
var defaultSettings *types.SSOClientRedirectSettings
require.NoError(t, ValidateClientRedirect(goodURL+"?secret_key=", ssoTestFlowFalse, defaultSettings))
require.NoError(t, sso.ValidateClientRedirect(goodURL+"?secret_key=", sso.CeremonyTypeLogin, defaultSettings))
}
})

Expand All @@ -650,9 +649,8 @@ func TestValidateClientRedirect(t *testing.T) {
"ftp://127.0.0.1/callback",
"ftp://[::1]/callback",
} {
const ssoTestFlowFalse = false
var defaultSettings *types.SSOClientRedirectSettings
require.Error(t, ValidateClientRedirect(badURL+"?secret_key=", ssoTestFlowFalse, defaultSettings))
require.Error(t, sso.ValidateClientRedirect(badURL+"?secret_key=", sso.CeremonyTypeLogin, defaultSettings))
}
})

Expand All @@ -662,9 +660,8 @@ func TestValidateClientRedirect(t *testing.T) {
"http://127.0.0.1:12345/callback?secret=a",
"http://127.0.0.1:12345/callback?secret_key=a&foo=b",
} {
const ssoTestFlowFalse = false
var defaultSettings *types.SSOClientRedirectSettings
require.Error(t, ValidateClientRedirect(badURL, ssoTestFlowFalse, defaultSettings))
require.Error(t, sso.ValidateClientRedirect(badURL, sso.CeremonyTypeLogin, defaultSettings))
}
})

Expand All @@ -674,15 +671,14 @@ func TestValidateClientRedirect(t *testing.T) {
"https://foo.allowed.with.subdomain.invalid/callback",
"https://but.no.subsubdomain.invalid/callback",
} {
const ssoTestFlowFalse = false
settings := &types.SSOClientRedirectSettings{
AllowedHttpsHostnames: []string{
"allowed.domain.invalid",
"*.allowed.with.subdomain.invalid",
"^[-a-zA-Z0-9]+.no.subsubdomain.invalid$",
},
}
require.NoError(t, ValidateClientRedirect(goodURL+"?secret_key=", ssoTestFlowFalse, settings))
require.NoError(t, sso.ValidateClientRedirect(goodURL+"?secret_key=", sso.CeremonyTypeLogin, settings))
}

for _, badURL := range []string{
Expand All @@ -693,15 +689,14 @@ func TestValidateClientRedirect(t *testing.T) {
"https://allowed.with.subdomain.invalid/callback",
"https://i.said.no.subsubdomain.invalid/callback",
} {
const ssoTestFlowFalse = false
settings := &types.SSOClientRedirectSettings{
AllowedHttpsHostnames: []string{
"allowed.domain.invalid",
"*.allowed.with.subdomain.invalid",
"^[-a-zA-Z0-9]+.no.subsubdomain.invalid",
},
}
require.Error(t, ValidateClientRedirect(badURL+"?secret_key=", ssoTestFlowFalse, settings))
require.Error(t, sso.ValidateClientRedirect(badURL+"?secret_key=", sso.CeremonyTypeLogin, settings))
}
})

Expand All @@ -720,14 +715,13 @@ func TestValidateClientRedirect(t *testing.T) {
"http://[2001:db8::1]:1337/callback",
"https://[2001:db8::1]:1337/callback",
} {
const ssoTestFlowFalse = false
settings := &types.SSOClientRedirectSettings{
InsecureAllowedCidrRanges: []string{
"192.168.0.0/24",
"2001:db8::/96",
},
}
require.NoError(t, ValidateClientRedirect(goodURL+"?secret_key=", ssoTestFlowFalse, settings))
require.NoError(t, sso.ValidateClientRedirect(goodURL+"?secret_key=", sso.CeremonyTypeLogin, settings))
}

for _, badURL := range []string{
Expand All @@ -748,54 +742,50 @@ func TestValidateClientRedirect(t *testing.T) {
"http://[2001:db8::1]/notacallback",
"https://[2001:db8::1]/notacallback",
} {
const ssoTestFlowFalse = false
settings := &types.SSOClientRedirectSettings{
InsecureAllowedCidrRanges: []string{
"192.168.0.0/24",
"2001:db8::/96",
},
}
require.Error(t, ValidateClientRedirect(badURL+"?secret_key=", ssoTestFlowFalse, settings))
require.Error(t, sso.ValidateClientRedirect(badURL+"?secret_key=", sso.CeremonyTypeLogin, settings))
}
})

t.Run("SSOTestFlow", func(t *testing.T) {
for _, goodURL := range []string{
"http://127.0.0.1:12345/callback",
} {
const ssoTestFlowTrue = true
settings := &types.SSOClientRedirectSettings{
AllowedHttpsHostnames: []string{
"allowed.domain.invalid",
},
}
require.NoError(t, ValidateClientRedirect(goodURL+"?secret_key=", ssoTestFlowTrue, settings))
require.NoError(t, sso.ValidateClientRedirect(goodURL+"?secret_key=", sso.CeremonyTypeLogin, settings))
}

for _, badURL := range []string{
"https://allowed.domain.invalid/callback",
"http://allowed.domain.invalid/callback",
} {
const ssoTestFlowTrue = true
settings := &types.SSOClientRedirectSettings{
AllowedHttpsHostnames: []string{
"allowed.domain.invalid",
},
}
require.Error(t, ValidateClientRedirect(badURL+"?secret_key=", ssoTestFlowTrue, settings))
require.Error(t, sso.ValidateClientRedirect(badURL+"?secret_key=", sso.CeremonyTypeLogin, 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))
require.NoError(t, sso.ValidateClientRedirect(sso.WebMFARedirect+"?channel_id=", sso.CeremonyTypeLogin, settings))

for _, badURL := range []string{
"localhost:12345" + sso.WebMFARedirect,
Expand All @@ -811,7 +801,7 @@ func TestValidateClientRedirect(t *testing.T) {
"http://not.allowed.domain.invalid" + sso.WebMFARedirect,
"https://not.allowed.domain.invalid" + sso.WebMFARedirect,
} {
require.Error(t, ValidateClientRedirect(badURL+"?channel_id=", ssoTestFlowFalse, settings))
require.Error(t, sso.ValidateClientRedirect(badURL+"?channel_id=", sso.CeremonyTypeLogin, settings))
}
})
}
Loading
Loading