mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat: support multiple OIDC redirect URIs (#25408)
This PR adds a new opt-in setting, `CODER_OIDC_REDIRECT_ALLOWED_HOSTS`, that lets a single Coder deployment complete OIDC login on more than one hostname. When the allowlist is non-empty, Coder picks the OIDC `redirect_uri` based on the incoming request's Host header (validated against the list) instead of always using the static URL derived from `CODER_ACCESS_URL`. When unset, the (default) behavior is identical to today. The motivation is that a single Coder deployment is frequently reachable via multiple hostnames - for example, an internal hostname for users on a corporate VPN and a different hostname routed through a zero-trust gateway for users off-VPN - but OIDC login today only works on whichever single hostname `CODER_ACCESS_URL` points to, because the `redirect_uri` sent to the IdP is fixed at server startup. Users who reach the deployment on any other valid hostname can see the login page but fail the OIDC callback, since the IdP redirects them back to a hostname they can't reach (or whose cookies they don't have).
This commit is contained in:
Generated
+7
@@ -21377,6 +21377,13 @@ const docTemplate = `{
|
||||
"organization_mapping": {
|
||||
"type": "object"
|
||||
},
|
||||
"redirect_allowed_hosts": {
|
||||
"description": "RedirectAllowedHosts is an allowlist of hostnames that may be used as\nthe host of the OIDC redirect_uri. When non-empty, the redirect_uri is\nconstructed from the incoming request's Host header (validated against\nthis list) instead of from AccessURL. Every listed host must also be\nregistered as a valid redirect URI in the OIDC provider. This setting\nis mutually exclusive with RedirectURL: if RedirectURL is set, this\nallowlist is ignored.",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"redirect_url": {
|
||||
"description": "RedirectURL is optional, defaulting to 'ACCESS_URL'. Only useful in niche\nsituations where the OIDC callback domain is different from the ACCESS_URL\ndomain.",
|
||||
"allOf": [
|
||||
|
||||
Generated
+7
@@ -19505,6 +19505,13 @@
|
||||
"organization_mapping": {
|
||||
"type": "object"
|
||||
},
|
||||
"redirect_allowed_hosts": {
|
||||
"description": "RedirectAllowedHosts is an allowlist of hostnames that may be used as\nthe host of the OIDC redirect_uri. When non-empty, the redirect_uri is\nconstructed from the incoming request's Host header (validated against\nthis list) instead of from AccessURL. Every listed host must also be\nregistered as a valid redirect URI in the OIDC provider. This setting\nis mutually exclusive with RedirectURL: if RedirectURL is set, this\nallowlist is ignored.",
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"redirect_url": {
|
||||
"description": "RedirectURL is optional, defaulting to 'ACCESS_URL'. Only useful in niche\nsituations where the OIDC callback domain is different from the ACCESS_URL\ndomain.",
|
||||
"allOf": [
|
||||
|
||||
+7
-3
@@ -777,8 +777,12 @@ func New(options *Options) *API {
|
||||
}
|
||||
|
||||
var oidcAuthURLParams map[string]string
|
||||
var oidcRedirectAllowedHosts []string
|
||||
var oidcRedirectDefaultScheme string
|
||||
if options.OIDCConfig != nil {
|
||||
oidcAuthURLParams = options.OIDCConfig.AuthURLParams
|
||||
oidcRedirectAllowedHosts = options.OIDCConfig.RedirectAllowedHosts
|
||||
oidcRedirectDefaultScheme = options.OIDCConfig.RedirectDefaultScheme
|
||||
}
|
||||
|
||||
api.Auditor.Store(&options.Auditor)
|
||||
@@ -1115,7 +1119,7 @@ func New(options *Options) *API {
|
||||
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
|
||||
r.Use(
|
||||
apiKeyMiddlewareRedirect,
|
||||
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, externalAuthConfig.CodeChallengeMethodsSupported),
|
||||
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, externalAuthConfig.CodeChallengeMethodsSupported, nil, ""),
|
||||
)
|
||||
r.Get("/", api.externalAuthCallback(externalAuthConfig))
|
||||
})
|
||||
@@ -1665,14 +1669,14 @@ func New(options *Options) *API {
|
||||
r.Route("/github", func(r chi.Router) {
|
||||
r.Use(
|
||||
// Github supports PKCE S256
|
||||
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, options.GithubOAuth2Config.PKCESupported()),
|
||||
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil, options.GithubOAuth2Config.PKCESupported(), nil, ""),
|
||||
)
|
||||
r.Get("/callback", api.userOAuth2Github)
|
||||
})
|
||||
})
|
||||
r.Route("/oidc/callback", func(r chi.Router) {
|
||||
r.Use(
|
||||
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCESupported()),
|
||||
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams, options.OIDCConfig.PKCESupported(), oidcRedirectAllowedHosts, oidcRedirectDefaultScheme),
|
||||
)
|
||||
r.Get("/", api.userOIDC)
|
||||
})
|
||||
|
||||
+141
-1
@@ -3,17 +3,21 @@ package httpmw
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"reflect"
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/google/uuid"
|
||||
"golang.org/x/oauth2"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw/loggermw"
|
||||
"github.com/coder/coder/v2/coderd/promoauth"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
"github.com/coder/coder/v2/cryptorand"
|
||||
@@ -45,13 +49,42 @@ func OAuth2(r *http.Request) OAuth2State {
|
||||
// pkceMethods should be a list like ['S256', 'plain'] indicating
|
||||
// which PKCE methods are supported by the OAuth2 provider. If empty,
|
||||
// PKCE will not be used.
|
||||
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string, pkceMethods []promoauth.Oauth2PKCEChallengeMethod) func(http.Handler) http.Handler {
|
||||
//
|
||||
// redirectAllowedHosts, when non-empty, enables dynamic redirect_uri
|
||||
// construction from the request Host header. The request Host must match
|
||||
// (case-insensitive, ignoring port) one of the listed hostnames. The
|
||||
// dynamic redirect_uri is cached in a cookie so the same value is reused
|
||||
// for the token exchange, as required by RFC 6749 section 4.1.3. Pass nil
|
||||
// to preserve the legacy behavior of using the redirect_uri baked into
|
||||
// config at startup.
|
||||
//
|
||||
// redirectDefaultScheme is the scheme used when constructing the dynamic
|
||||
// redirect_uri. It is populated from the configured AccessURL and takes
|
||||
// precedence over r.TLS / X-Forwarded-Proto because some reverse proxies
|
||||
// report the inner-hop scheme (e.g. "http") rather than the original
|
||||
// client-facing scheme, which would produce a redirect_uri the IdP
|
||||
// rejects. Callers must always supply this when redirectAllowedHosts is
|
||||
// non-empty; an empty value would yield an invalid redirect_uri without
|
||||
// a scheme.
|
||||
func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg codersdk.HTTPCookieConfig, authURLOpts map[string]string, pkceMethods []promoauth.Oauth2PKCEChallengeMethod, redirectAllowedHosts []string, redirectDefaultScheme string) func(http.Handler) http.Handler {
|
||||
opts := make([]oauth2.AuthCodeOption, 0, len(authURLOpts)+1)
|
||||
opts = append(opts, oauth2.AccessTypeOffline)
|
||||
for k, v := range authURLOpts {
|
||||
opts = append(opts, oauth2.SetAuthURLParam(k, v))
|
||||
}
|
||||
|
||||
// Pre-normalize the allowlist once so the per-request check is a plain
|
||||
// case-insensitive compare and we do not re-allocate on every login.
|
||||
normalizedAllowedHosts := make([]string, 0, len(redirectAllowedHosts))
|
||||
for _, h := range redirectAllowedHosts {
|
||||
h = strings.TrimSpace(h)
|
||||
if h == "" {
|
||||
continue
|
||||
}
|
||||
normalizedAllowedHosts = append(normalizedAllowedHosts, strings.ToLower(h))
|
||||
}
|
||||
dynamicRedirectEnabled := len(normalizedAllowedHosts) > 0
|
||||
|
||||
// Only S256 PKCE is currently supported.
|
||||
sha256PKCESupported := slices.Contains(pkceMethods, promoauth.PKCEChallengeMethodSha256)
|
||||
return func(next http.Handler) http.Handler {
|
||||
@@ -103,6 +136,32 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
|
||||
redirect = uriFromURL(redirect)
|
||||
}
|
||||
|
||||
// When dynamic redirect URIs are enabled, validate the request Host
|
||||
// against the allowlist regardless of whether we are initiating the
|
||||
// flow or handling the callback. Doing this upfront avoids burning
|
||||
// state and lets us reject obviously-bad requests with a clear error.
|
||||
var dynamicRedirectURI string
|
||||
if dynamicRedirectEnabled {
|
||||
hostname := r.Host
|
||||
if h, _, splitErr := net.SplitHostPort(r.Host); splitErr == nil {
|
||||
hostname = h
|
||||
}
|
||||
if !slices.Contains(normalizedAllowedHosts, strings.ToLower(hostname)) {
|
||||
if rlogger := loggermw.RequestLoggerFromContext(ctx); rlogger != nil {
|
||||
rlogger.WithFields(
|
||||
slog.F("oidc_rejected_reason", "host_not_in_allowlist"),
|
||||
slog.F("oidc_rejected_host", hostname),
|
||||
)
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: "OIDC login is not permitted from this host.",
|
||||
Detail: fmt.Sprintf("Host %q is not in the OIDC redirect allowlist. Configure CODER_OIDC_REDIRECT_ALLOWED_HOSTS to include it.", hostname),
|
||||
})
|
||||
return
|
||||
}
|
||||
dynamicRedirectURI = buildDynamicRedirectURI(r, redirectDefaultScheme)
|
||||
}
|
||||
|
||||
if code == "" {
|
||||
// If the code isn't provided, we'll redirect!
|
||||
var state string
|
||||
@@ -153,6 +212,19 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
|
||||
}))
|
||||
}
|
||||
|
||||
// Persist and inject the dynamic redirect_uri so the IdP
|
||||
// sends the user back to the same domain they started on,
|
||||
// and so the token exchange below uses the matching value.
|
||||
if dynamicRedirectURI != "" {
|
||||
http.SetCookie(rw, cookieCfg.Apply(&http.Cookie{
|
||||
Name: codersdk.OAuth2RedirectURICookie,
|
||||
Value: dynamicRedirectURI,
|
||||
Path: "/",
|
||||
HttpOnly: true,
|
||||
}))
|
||||
authOpts = append(authOpts, oauth2.SetAuthURLParam("redirect_uri", dynamicRedirectURI))
|
||||
}
|
||||
|
||||
http.Redirect(rw, r, config.AuthCodeURL(state, authOpts...), http.StatusTemporaryRedirect)
|
||||
return
|
||||
}
|
||||
@@ -195,6 +267,50 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg
|
||||
exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(pkceVerifier.Value))
|
||||
}
|
||||
|
||||
// RFC 6749 section 4.1.3: the redirect_uri included in the token
|
||||
// exchange must match the one sent in the authorization request.
|
||||
// When the dynamic-redirect path is in use, the original value was
|
||||
// stashed in a cookie; replay it here.
|
||||
//
|
||||
// Defense in depth: we do not blindly forward the cookie value to
|
||||
// the IdP. We recompute the expected redirect_uri from the (already
|
||||
// allowlist-validated) request Host, then require the cookie to
|
||||
// match. This guards against:
|
||||
// - The cookie going missing (e.g. third-party cookie blocking)
|
||||
// and silently falling back to the static redirect_uri, which
|
||||
// would mismatch the authorization request and produce a
|
||||
// confusing IdP rejection. Fail loudly here instead.
|
||||
// - A tampered cookie pointing at a host the user did not
|
||||
// authenticate on. The IdP allowlist would normally catch this,
|
||||
// but we should not depend on it.
|
||||
if dynamicRedirectEnabled {
|
||||
redirectCookie, err := r.Cookie(codersdk.OAuth2RedirectURICookie)
|
||||
if err != nil || redirectCookie.Value == "" {
|
||||
if rlogger := loggermw.RequestLoggerFromContext(ctx); rlogger != nil {
|
||||
rlogger.WithFields(slog.F("oidc_rejected_reason", "missing_redirect_uri_cookie"))
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: fmt.Sprintf("Cookie %q must be provided for the OIDC callback when CODER_OIDC_REDIRECT_ALLOWED_HOSTS is configured.", codersdk.OAuth2RedirectURICookie),
|
||||
})
|
||||
return
|
||||
}
|
||||
expectedRedirectURI := buildDynamicRedirectURI(r, redirectDefaultScheme)
|
||||
if redirectCookie.Value != expectedRedirectURI {
|
||||
if rlogger := loggermw.RequestLoggerFromContext(ctx); rlogger != nil {
|
||||
rlogger.WithFields(
|
||||
slog.F("oidc_rejected_reason", "redirect_uri_cookie_mismatch"),
|
||||
slog.F("oidc_cookie_redirect_uri", redirectCookie.Value),
|
||||
slog.F("oidc_expected_redirect_uri", expectedRedirectURI),
|
||||
)
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: "OIDC redirect_uri cookie does not match the current request host.",
|
||||
})
|
||||
return
|
||||
}
|
||||
exchangeOpts = append(exchangeOpts, oauth2.SetAuthURLParam("redirect_uri", redirectCookie.Value))
|
||||
}
|
||||
|
||||
oauthToken, err := config.Exchange(ctx, code, exchangeOpts...)
|
||||
if err != nil {
|
||||
errorCode := http.StatusInternalServerError
|
||||
@@ -424,3 +540,27 @@ func uriFromURL(u string) string {
|
||||
|
||||
return uri.RequestURI()
|
||||
}
|
||||
|
||||
// buildDynamicRedirectURI constructs the OIDC redirect_uri from the incoming
|
||||
// request, used when CODER_OIDC_REDIRECT_ALLOWED_HOSTS is configured.
|
||||
//
|
||||
// The scheme is taken from the configured AccessURL (passed in as
|
||||
// defaultScheme by the caller) rather than from the request itself. Real
|
||||
// deployments that use this feature always sit behind a TLS-terminating
|
||||
// proxy, and some such proxies set X-Forwarded-Proto to the inner-hop
|
||||
// scheme (e.g. "http" between proxy and coderd) instead of the original
|
||||
// client-facing scheme. Trusting the request for scheme would produce a
|
||||
// redirect_uri the IdP rejects. AccessURL is the operator-defined source
|
||||
// of truth and is the same value the static OIDC path uses, so reusing
|
||||
// it keeps the dynamic and static paths byte-for-byte consistent.
|
||||
//
|
||||
// The callback path is whatever path the middleware is mounted at, which
|
||||
// today is /api/v2/users/oidc/callback for OIDC.
|
||||
func buildDynamicRedirectURI(r *http.Request, defaultScheme string) string {
|
||||
u := url.URL{
|
||||
Scheme: defaultScheme,
|
||||
Host: r.Host,
|
||||
Path: r.URL.Path,
|
||||
}
|
||||
return u.String()
|
||||
}
|
||||
|
||||
@@ -50,7 +50,7 @@ func TestOAuth2(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", "/", nil)
|
||||
res := httptest.NewRecorder()
|
||||
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(nil, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
})
|
||||
t.Run("RedirectWithoutCode", func(t *testing.T) {
|
||||
@@ -58,7 +58,7 @@ func TestOAuth2(t *testing.T) {
|
||||
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape("/dashboard"), nil)
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
location := res.Header().Get("Location")
|
||||
if !assert.NotEmpty(t, location) {
|
||||
return
|
||||
@@ -82,7 +82,7 @@ func TestOAuth2(t *testing.T) {
|
||||
req := httptest.NewRequest("GET", "/?redirect="+url.QueryEscape(uri.String()), nil)
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
location := res.Header().Get("Location")
|
||||
if !assert.NotEmpty(t, location) {
|
||||
return
|
||||
@@ -97,7 +97,7 @@ func TestOAuth2(t *testing.T) {
|
||||
req := httptest.NewRequest("GET", "/?code=something", nil)
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
})
|
||||
t.Run("NoStateCookie", func(t *testing.T) {
|
||||
@@ -105,7 +105,7 @@ func TestOAuth2(t *testing.T) {
|
||||
req := httptest.NewRequest("GET", "/?code=something&state=test", nil)
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
|
||||
})
|
||||
t.Run("MismatchedState", func(t *testing.T) {
|
||||
@@ -117,7 +117,7 @@ func TestOAuth2(t *testing.T) {
|
||||
})
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
require.Equal(t, http.StatusUnauthorized, res.Result().StatusCode)
|
||||
})
|
||||
t.Run("ExchangeCodeAndState", func(t *testing.T) {
|
||||
@@ -133,7 +133,7 @@ func TestOAuth2(t *testing.T) {
|
||||
})
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
state := httpmw.OAuth2(r)
|
||||
require.Equal(t, "/dashboard", state.Redirect)
|
||||
})).ServeHTTP(res, req)
|
||||
@@ -144,7 +144,7 @@ func TestOAuth2(t *testing.T) {
|
||||
res := httptest.NewRecorder()
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("foo", "bar"))
|
||||
authOpts := map[string]string{"foo": "bar"}
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts, nil)(nil).ServeHTTP(res, req)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, authOpts, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
location := res.Header().Get("Location")
|
||||
// Ideally we would also assert that the location contains the query params
|
||||
// we set in the auth URL but this would essentially be testing the oauth2 package.
|
||||
@@ -160,7 +160,7 @@ func TestOAuth2(t *testing.T) {
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{
|
||||
Secure: true,
|
||||
SameSite: "none",
|
||||
}, nil, nil)(nil).ServeHTTP(res, req)
|
||||
}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
|
||||
found := false
|
||||
for _, cookie := range res.Result().Cookies() {
|
||||
@@ -174,3 +174,198 @@ func TestOAuth2(t *testing.T) {
|
||||
require.True(t, found, "expected state cookie")
|
||||
})
|
||||
}
|
||||
|
||||
// nolint:bodyclose
|
||||
func TestOAuth2DynamicRedirect(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const callbackPath = "/api/v2/users/oidc/callback"
|
||||
const primaryHost = "coder.test.netflix.net"
|
||||
const altHost = "dev-workspaces.test.netflix.net"
|
||||
const wantPrimaryURI = "https://" + primaryHost + callbackPath
|
||||
const wantAltURI = "https://" + altHost + callbackPath
|
||||
|
||||
t.Run("InitOnAllowedHostSetsCookieAndOverridesRedirect", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", callbackPath+"?redirect="+url.QueryEscape("/dashboard"), nil)
|
||||
req.Host = altHost
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
tp := newTestOAuth2Provider(t,
|
||||
oauth2.AccessTypeOffline,
|
||||
oauth2.SetAuthURLParam("redirect_uri", wantAltURI),
|
||||
)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost, altHost}, "https")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusTemporaryRedirect, res.Result().StatusCode)
|
||||
|
||||
var redirectCookie *http.Cookie
|
||||
for _, c := range res.Result().Cookies() {
|
||||
if c.Name == codersdk.OAuth2RedirectURICookie {
|
||||
redirectCookie = c
|
||||
break
|
||||
}
|
||||
}
|
||||
require.NotNil(t, redirectCookie, "expected %s cookie", codersdk.OAuth2RedirectURICookie)
|
||||
require.Equal(t, wantAltURI, redirectCookie.Value)
|
||||
})
|
||||
|
||||
t.Run("InitOnDisallowedHostReturnsBadRequest", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", callbackPath, nil)
|
||||
req.Host = "evil.example.com"
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
// authOpts must not be asserted: the request should be rejected
|
||||
// before AuthCodeURL is called.
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost, altHost}, "")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
for _, c := range res.Result().Cookies() {
|
||||
require.NotEqual(t, codersdk.OAuth2RedirectURICookie, c.Name, "must not set redirect_uri cookie when host is rejected")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("AllowlistHostMatchIsCaseInsensitiveAndIgnoresPort", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", callbackPath, nil)
|
||||
req.Host = "DEV-WORKSPACES.test.netflix.net:8443"
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
// Host is preserved verbatim in the constructed redirect_uri so the
|
||||
// IdP sees exactly what the user typed (case is preserved but the
|
||||
// allowlist match is insensitive). The scheme comes from the caller-
|
||||
// supplied defaultScheme; real callers populate this from AccessURL.
|
||||
expectedURI := "https://DEV-WORKSPACES.test.netflix.net:8443" + callbackPath
|
||||
tp := newTestOAuth2Provider(t,
|
||||
oauth2.AccessTypeOffline,
|
||||
oauth2.SetAuthURLParam("redirect_uri", expectedURI),
|
||||
)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{altHost}, "https")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusTemporaryRedirect, res.Result().StatusCode)
|
||||
})
|
||||
|
||||
t.Run("ExchangeReusesRedirectURIFromCookie", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", callbackPath+"?code=test&state=something", nil)
|
||||
req.Host = altHost
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2StateCookie, Value: "something"})
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2RedirectCookie, Value: "/dashboard"})
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2RedirectURICookie, Value: wantAltURI})
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
exchangeCalled := false
|
||||
tp := &exchangeAssertingProvider{
|
||||
t: t,
|
||||
onExchange: func(opts []oauth2.AuthCodeOption) {
|
||||
exchangeCalled = true
|
||||
require.Contains(t, opts, oauth2.SetAuthURLParam("redirect_uri", wantAltURI))
|
||||
},
|
||||
}
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost, altHost}, "https")(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
state := httpmw.OAuth2(r)
|
||||
require.Equal(t, "/dashboard", state.Redirect)
|
||||
})).ServeHTTP(res, req)
|
||||
require.True(t, exchangeCalled, "expected Exchange to be invoked")
|
||||
})
|
||||
|
||||
t.Run("CallbackWithMissingRedirectURICookieReturnsBadRequest", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Same shape as ExchangeReusesRedirectURIFromCookie but without the
|
||||
// redirect_uri cookie. Must fail loudly rather than silently sending
|
||||
// the static config redirect_uri (which would mismatch what was used
|
||||
// in the original authorization request).
|
||||
req := httptest.NewRequest("GET", callbackPath+"?code=test&state=something", nil)
|
||||
req.Host = altHost
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2StateCookie, Value: "something"})
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2RedirectCookie, Value: "/dashboard"})
|
||||
// Intentionally NO OAuth2RedirectURICookie.
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost, altHost}, "https")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
})
|
||||
|
||||
t.Run("CallbackWithMismatchedRedirectURICookieReturnsBadRequest", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Cookie was set when the user initiated on altHost, but the callback
|
||||
// is somehow arriving from primaryHost (or the cookie was tampered).
|
||||
// Defense in depth: reject the exchange instead of forwarding a
|
||||
// stale/mismatched value to the IdP.
|
||||
req := httptest.NewRequest("GET", callbackPath+"?code=test&state=something", nil)
|
||||
req.Host = primaryHost
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2StateCookie, Value: "something"})
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2RedirectURICookie, Value: wantAltURI})
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost, altHost}, "https")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
})
|
||||
|
||||
t.Run("CallbackOnDisallowedHostReturnsBadRequest", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Even with a valid state cookie and code, the host must be allowed.
|
||||
req := httptest.NewRequest("GET", callbackPath+"?code=test&state=something", nil)
|
||||
req.Host = "evil.example.com"
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2StateCookie, Value: "something"})
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.OAuth2RedirectURICookie, Value: wantPrimaryURI})
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil,
|
||||
[]string{primaryHost}, "")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, res.Result().StatusCode)
|
||||
})
|
||||
|
||||
t.Run("AllowlistDisabledLeavesBehaviorUnchanged", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest("GET", callbackPath+"?redirect="+url.QueryEscape("/dashboard"), nil)
|
||||
req.Host = "anything.example.com"
|
||||
res := httptest.NewRecorder()
|
||||
|
||||
// With no allowlist, AuthCodeURL must be invoked with only the base
|
||||
// AccessTypeOffline option; no redirect_uri override should be added.
|
||||
tp := newTestOAuth2Provider(t, oauth2.AccessTypeOffline)
|
||||
httpmw.ExtractOAuth2(tp, nil, codersdk.HTTPCookieConfig{}, nil, nil, nil, "")(nil).ServeHTTP(res, req)
|
||||
|
||||
require.Equal(t, http.StatusTemporaryRedirect, res.Result().StatusCode)
|
||||
for _, c := range res.Result().Cookies() {
|
||||
require.NotEqual(t, codersdk.OAuth2RedirectURICookie, c.Name)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// exchangeAssertingProvider is a test OAuth2 provider that captures the
|
||||
// options passed to Exchange so the test can assert on them.
|
||||
type exchangeAssertingProvider struct {
|
||||
t testing.TB
|
||||
onExchange func(opts []oauth2.AuthCodeOption)
|
||||
}
|
||||
|
||||
func (*exchangeAssertingProvider) AuthCodeURL(state string, _ ...oauth2.AuthCodeOption) string {
|
||||
return "?state=" + url.QueryEscape(state)
|
||||
}
|
||||
|
||||
func (p *exchangeAssertingProvider) Exchange(_ context.Context, _ string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) {
|
||||
if p.onExchange != nil {
|
||||
p.onExchange(opts)
|
||||
}
|
||||
return &oauth2.Token{AccessToken: "hello"}, nil
|
||||
}
|
||||
|
||||
func (*exchangeAssertingProvider) TokenSource(_ context.Context, _ *oauth2.Token) oauth2.TokenSource {
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -1193,6 +1193,19 @@ type OIDCConfig struct {
|
||||
// check. Used for IdP brokers that do not issue a stable `sub` for the
|
||||
// same user across connections.
|
||||
EmailFallback bool
|
||||
// RedirectAllowedHosts, when non-empty, enables dynamic redirect_uri
|
||||
// construction from the request Host header. The request Host must match
|
||||
// (case-insensitive, ignoring port) one of the hostnames in this list,
|
||||
// otherwise the OIDC flow is rejected.
|
||||
RedirectAllowedHosts []string
|
||||
// RedirectDefaultScheme is the scheme to use in the dynamically built
|
||||
// redirect_uri. It is populated from the configured AccessURL (or
|
||||
// OIDC.RedirectURL if explicitly overridden) so that the dynamic path
|
||||
// uses the same scheme as the static path. It takes precedence over
|
||||
// X-Forwarded-Proto because some reverse proxies report the inner-hop
|
||||
// scheme (e.g. "http") rather than the original client-facing scheme,
|
||||
// which would produce a redirect_uri the IdP rejects.
|
||||
RedirectDefaultScheme string
|
||||
}
|
||||
|
||||
// PKCESupported is to prevent nil pointer dereference.
|
||||
|
||||
Reference in New Issue
Block a user