From 3a2ed118217198c0d6057bea419b07b93c72faba Mon Sep 17 00:00:00 2001 From: "gcp-cherry-pick-bot[bot]" <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Date: Thu, 12 Sep 2024 18:58:47 +0200 Subject: [PATCH] providers/proxy: fix URL path getting lost when partial URL is given to rd= (cherry-pick #11354) (#11355) providers/proxy: fix URL path getting lost when partial URL is given to rd= (#11354) * providers/proxy: fix URL path getting lost when partial URL is given to rd= * better fallback + tests --------- Signed-off-by: Jens Langhammer Co-authored-by: Jens L. --- .../proxyv2/application/application.go | 12 ++++- internal/outpost/proxyv2/application/oauth.go | 30 ----------- .../proxyv2/application/oauth_state.go | 52 +++++++++++++++---- .../outpost/proxyv2/application/oauth_test.go | 30 +++++++++-- 4 files changed, 77 insertions(+), 47 deletions(-) diff --git a/internal/outpost/proxyv2/application/application.go b/internal/outpost/proxyv2/application/application.go index 45451bf36d..baeea30cf0 100644 --- a/internal/outpost/proxyv2/application/application.go +++ b/internal/outpost/proxyv2/application/application.go @@ -193,7 +193,17 @@ func NewApplication(p api.ProxyOutpostConfig, c *http.Client, server Server) (*A }) mux.HandleFunc("/outpost.goauthentik.io/start", func(w http.ResponseWriter, r *http.Request) { - a.handleAuthStart(w, r, "") + fwd := "" + // This should only really be hit for nginx forward_auth + // as for that the auth start redirect URL is generated by the + // reverse proxy, and as such we won't have a request we just + // denied to reference for final URL + rd, ok := a.checkRedirectParam(r) + if ok { + a.log.WithField("rd", rd).Trace("Setting redirect") + fwd = rd + } + a.handleAuthStart(w, r, fwd) }) mux.HandleFunc("/outpost.goauthentik.io/callback", a.handleAuthCallback) mux.HandleFunc("/outpost.goauthentik.io/sign_out", a.handleSignOut) diff --git a/internal/outpost/proxyv2/application/oauth.go b/internal/outpost/proxyv2/application/oauth.go index f2dc93319d..774bf0625a 100644 --- a/internal/outpost/proxyv2/application/oauth.go +++ b/internal/outpost/proxyv2/application/oauth.go @@ -15,36 +15,6 @@ const ( LogoutSignature = "X-authentik-logout" ) -func (a *Application) checkRedirectParam(r *http.Request) (string, bool) { - rd := r.URL.Query().Get(redirectParam) - if rd == "" { - return "", false - } - u, err := url.Parse(rd) - if err != nil { - a.log.WithError(err).Warning("Failed to parse redirect URL") - return "", false - } - // Check to make sure we only redirect to allowed places - if a.Mode() == api.PROXYMODE_PROXY || a.Mode() == api.PROXYMODE_FORWARD_SINGLE { - ext, err := url.Parse(a.proxyConfig.ExternalHost) - if err != nil { - return "", false - } - ext.Scheme = "" - if !strings.Contains(u.String(), ext.String()) { - a.log.WithField("url", u.String()).WithField("ext", ext.String()).Warning("redirect URI did not contain external host") - return "", false - } - } else { - if !strings.HasSuffix(u.Host, *a.proxyConfig.CookieDomain) { - a.log.WithField("host", u.Host).WithField("dom", *a.proxyConfig.CookieDomain).Warning("redirect URI Host was not included in cookie domain") - return "", false - } - } - return u.String(), true -} - func (a *Application) handleAuthStart(rw http.ResponseWriter, r *http.Request, fwd string) { state, err := a.createState(r, fwd) if err != nil { diff --git a/internal/outpost/proxyv2/application/oauth_state.go b/internal/outpost/proxyv2/application/oauth_state.go index 798e658396..b2cdec52cf 100644 --- a/internal/outpost/proxyv2/application/oauth_state.go +++ b/internal/outpost/proxyv2/application/oauth_state.go @@ -5,10 +5,13 @@ import ( "encoding/base64" "fmt" "net/http" + "net/url" + "strings" "github.com/golang-jwt/jwt/v5" "github.com/gorilla/securecookie" "github.com/mitchellh/mapstructure" + "goauthentik.io/api/v3" ) type OAuthState struct { @@ -27,6 +30,44 @@ func (oas *OAuthState) GetAudience() (jwt.ClaimStrings, error) { return ni var base32RawStdEncoding = base32.StdEncoding.WithPadding(base32.NoPadding) +// Validate that the given redirect parameter (?rd=...) is valid and can be used +// For proxy/forward_single this checks that if the `rd` param has a Hostname (and is a full URL) +// the hostname matches what's configured, or no hostname must be given +// For forward_domain this checks if the domain of the URL in `rd` ends with the configured domain +func (a *Application) checkRedirectParam(r *http.Request) (string, bool) { + rd := r.URL.Query().Get(redirectParam) + if rd == "" { + return "", false + } + u, err := url.Parse(rd) + if err != nil { + a.log.WithError(err).Warning("Failed to parse redirect URL") + return "", false + } + // Check to make sure we only redirect to allowed places + if a.Mode() == api.PROXYMODE_PROXY || a.Mode() == api.PROXYMODE_FORWARD_SINGLE { + ext, err := url.Parse(a.proxyConfig.ExternalHost) + if err != nil { + return "", false + } + // Either hostname needs to match the configured domain, or host name must be empty for just a path + if u.Host == "" { + u.Host = ext.Host + u.Scheme = ext.Scheme + } + if u.Host != ext.Host { + a.log.WithField("url", u.String()).WithField("ext", ext.String()).Warning("redirect URI did not contain external host") + return "", false + } + } else { + if !strings.HasSuffix(u.Host, *a.proxyConfig.CookieDomain) { + a.log.WithField("host", u.Host).WithField("dom", *a.proxyConfig.CookieDomain).Warning("redirect URI Host was not included in cookie domain") + return "", false + } + } + return u.String(), true +} + func (a *Application) createState(r *http.Request, fwd string) (string, error) { s, _ := a.sessions.Get(r, a.SessionName()) if s.ID == "" { @@ -39,17 +80,6 @@ func (a *Application) createState(r *http.Request, fwd string) (string, error) { SessionID: s.ID, Redirect: fwd, } - if fwd == "" { - // This should only really be hit for nginx forward_auth - // as for that the auth start redirect URL is generated by the - // reverse proxy, and as such we won't have a request we just - // denied to reference for final URL - rd, ok := a.checkRedirectParam(r) - if ok { - a.log.WithField("rd", rd).Trace("Setting redirect") - st.Redirect = rd - } - } token := jwt.NewWithClaims(jwt.SigningMethodHS256, st) tokenString, err := token.SignedString([]byte(a.proxyConfig.GetCookieSecret())) if err != nil { diff --git a/internal/outpost/proxyv2/application/oauth_test.go b/internal/outpost/proxyv2/application/oauth_test.go index 79e84382d9..e96c3c958b 100644 --- a/internal/outpost/proxyv2/application/oauth_test.go +++ b/internal/outpost/proxyv2/application/oauth_test.go @@ -8,25 +8,45 @@ import ( "goauthentik.io/api/v3" ) -func TestCheckRedirectParam(t *testing.T) { +func TestCheckRedirectParam_None(t *testing.T) { a := newTestApplication() + // Test no rd param req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start", nil) rd, ok := a.checkRedirectParam(req) assert.Equal(t, false, ok) assert.Equal(t, "", rd) +} - req, _ = http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://google.com", nil) +func TestCheckRedirectParam_Invalid(t *testing.T) { + a := newTestApplication() + // Test invalid rd param + req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://google.com", nil) - rd, ok = a.checkRedirectParam(req) + rd, ok := a.checkRedirectParam(req) assert.Equal(t, false, ok) assert.Equal(t, "", rd) +} - req, _ = http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://ext.t.goauthentik.io/test?foo", nil) +func TestCheckRedirectParam_ValidFull(t *testing.T) { + a := newTestApplication() + // Test valid full rd param + req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://ext.t.goauthentik.io/test?foo", nil) - rd, ok = a.checkRedirectParam(req) + rd, ok := a.checkRedirectParam(req) + + assert.Equal(t, true, ok) + assert.Equal(t, "https://ext.t.goauthentik.io/test?foo", rd) +} + +func TestCheckRedirectParam_ValidPartial(t *testing.T) { + a := newTestApplication() + // Test valid partial rd param + req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=/test?foo", nil) + + rd, ok := a.checkRedirectParam(req) assert.Equal(t, true, ok) assert.Equal(t, "https://ext.t.goauthentik.io/test?foo", rd)