From 42dcba8367c0c66511876652a041d69cd6073139 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 9 Feb 2023 11:56:52 +0100 Subject: [PATCH] refactor: replace relative canonical redirect with handler This also ensure that we clean any urls that may stem from user input (e.g. url parameter or login cookie) before performing redirects. --- cmd/wonderwall/main.go | 2 +- pkg/config/config.go | 30 +++++++------- pkg/handler/error/error.go | 9 +++-- pkg/handler/handler_default.go | 49 +++++++++++++---------- pkg/handler/handler_sso_proxy.go | 65 ++++++++++--------------------- pkg/handler/handler_sso_server.go | 10 ++++- pkg/handler/login.go | 44 +++++++++++++++++++-- pkg/handler/login_callback.go | 8 +++- pkg/openid/client/login.go | 27 +++++-------- 9 files changed, 139 insertions(+), 105 deletions(-) diff --git a/cmd/wonderwall/main.go b/cmd/wonderwall/main.go index b72d316..3fa9b99 100644 --- a/cmd/wonderwall/main.go +++ b/cmd/wonderwall/main.go @@ -96,7 +96,7 @@ func ssoServerHandler(ctx context.Context, cfg *config.Config, crypt crypto.Cryp WithPath("/"). WithDomain(cfg.SSO.Domain) - return handler.NewSSOServerHandler(h), nil + return handler.NewSSOServerHandler(h) } func ssoProxyHandler(cfg *config.Config) (*handler.SSOProxyHandler, error) { diff --git a/pkg/config/config.go b/pkg/config/config.go index 1c19826..474fb7e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -50,10 +50,11 @@ type Session struct { } type SSO struct { - Enabled bool `json:"enabled"` - Domain string `json:"domain"` - Mode SSOMode `json:"mode"` - ServerURL string `json:"server-url"` + Enabled bool `json:"enabled"` + Domain string `json:"domain"` + Mode SSOMode `json:"mode"` + ServerURL string `json:"server-url"` + ServerDefaultRedirectURL string `json:"server-default-redirect-url"` } type SSOMode string @@ -87,10 +88,11 @@ const ( LoginstatusResourceIndicator = "loginstatus.resource-indicator" LoginstatusTokenURL = "loginstatus.token-url" - SSOEnabled = "sso.enabled" - SSODomain = "sso.domain" - SSOModeFlag = "sso.mode" - SSOServerURL = "sso.server-url" + SSOEnabled = "sso.enabled" + SSODomain = "sso.domain" + SSOServerDefaultRedirectURL = "sso.server-default-redirect-url" + SSOModeFlag = "sso.mode" + SSOServerURL = "sso.server-url" ) func Initialize() (*Config, error) { @@ -122,7 +124,8 @@ func Initialize() (*Config, error) { flag.Bool(SSOEnabled, false, "Enable single sign-on mode; one server acting as the OIDC Relying Party, and N proxies. The proxies delegate most endpoint operations to the server, and only implements a reverse proxy that reads the user's session data from the shared store.") flag.String(SSODomain, "", "The domain that the session cookies should be set for, usually the second-level domain name (e.g. example.com).") flag.String(SSOModeFlag, string(SSOModeServer), "The SSO mode for this instance. Must be one of 'server' or 'proxy'.") - flag.String(SSOServerURL, "", "The URL that points to the SSO server instance.") + flag.String(SSOServerDefaultRedirectURL, "", "The URL that the SSO server should redirect to by default if the a given redirect query parameter is invalid.") + flag.String(SSOServerURL, "", "The URL used by the proxy to point to the SSO server instance.") redisFlags() openIDFlags() @@ -186,10 +189,6 @@ func (c *Config) Validate() error { if c.SSO.Enabled { switch c.SSO.Mode { case SSOModeProxy: - if len(c.SSO.ServerURL) == 0 { - return fmt.Errorf("%q cannot be empty", SSOServerURL) - } - _, err := url.ParseRequestURI(c.SSO.ServerURL) if err != nil { return fmt.Errorf("%q must be a valid url: %w", SSOServerURL, err) @@ -198,6 +197,11 @@ func (c *Config) Validate() error { if len(c.SSO.Domain) == 0 { return fmt.Errorf("%q cannot be empty", SSODomain) } + + _, err := url.ParseRequestURI(c.SSO.ServerDefaultRedirectURL) + if err != nil { + return fmt.Errorf("%q must be a valid url: %w", SSOServerDefaultRedirectURL, err) + } default: return fmt.Errorf("%q must be one of [%q, %q]", SSOModeFlag, SSOModeServer, SSOModeProxy) } diff --git a/pkg/handler/error/error.go b/pkg/handler/error/error.go index cf99a38..dfc479a 100644 --- a/pkg/handler/error/error.go +++ b/pkg/handler/error/error.go @@ -16,6 +16,7 @@ import ( "github.com/nais/wonderwall/pkg/handler/templates" mw "github.com/nais/wonderwall/pkg/middleware" "github.com/nais/wonderwall/pkg/openid" + "github.com/nais/wonderwall/pkg/redirect" "github.com/nais/wonderwall/pkg/router/paths" urlpkg "github.com/nais/wonderwall/pkg/url" ) @@ -30,6 +31,7 @@ type Source interface { GetCrypter() crypto.Crypter GetErrorPath() string GetPath(r *http.Request) string + GetRedirectHandler() redirect.Handler } type Page struct { @@ -71,13 +73,14 @@ func (h Handler) Retry(r *http.Request, loginCookie *openid.LoginCookie) string } } - redirect := urlpkg.CanonicalRedirect(r) + redirectHandler := h.GetRedirectHandler() + rd := redirectHandler.Canonical(r) if loginCookie != nil && len(loginCookie.Referer) > 0 { - redirect = loginCookie.Referer + rd = redirectHandler.Clean(r, loginCookie.Referer) } - return urlpkg.LoginRelative(ingressPath, redirect) + return urlpkg.LoginRelative(ingressPath, rd) } func (h Handler) respondError(w http.ResponseWriter, r *http.Request, statusCode int, cause error, level log.Level) { diff --git a/pkg/handler/handler_default.go b/pkg/handler/handler_default.go index 8801c1f..2b21044 100644 --- a/pkg/handler/handler_default.go +++ b/pkg/handler/handler_default.go @@ -14,6 +14,7 @@ import ( "github.com/nais/wonderwall/pkg/middleware" openidclient "github.com/nais/wonderwall/pkg/openid/client" openidconfig "github.com/nais/wonderwall/pkg/openid/config" + "github.com/nais/wonderwall/pkg/redirect" "github.com/nais/wonderwall/pkg/router" "github.com/nais/wonderwall/pkg/session" ) @@ -21,16 +22,17 @@ import ( var _ router.Source = &DefaultHandler{} type DefaultHandler struct { - AutoLogin *autologin.AutoLogin - Client *openidclient.Client - Config *config.Config - CookieOptions cookie.Options - Crypter crypto.Crypter - Ingresses *ingress.Ingresses - Loginstatus *loginstatus.Loginstatus - OpenidConfig openidconfig.Config - Sessions *session.Handler - UpstreamProxy *ReverseProxy + AutoLogin *autologin.AutoLogin + Client *openidclient.Client + Config *config.Config + CookieOptions cookie.Options + Crypter crypto.Crypter + Ingresses *ingress.Ingresses + Loginstatus *loginstatus.Loginstatus + OpenidConfig openidconfig.Config + RedirectHandler redirect.Handler + Sessions *session.Handler + UpstreamProxy *ReverseProxy } func NewDefaultHandler( @@ -64,17 +66,20 @@ func NewDefaultHandler( return nil, err } + redirectHandler := redirect.NewDefaultHandler(ingresses) + return &DefaultHandler{ - AutoLogin: autoLogin, - Client: openidClient, - Config: cfg, - CookieOptions: cookieOpts, - Crypter: crypter, - Ingresses: ingresses, - Loginstatus: loginstatusClient, - OpenidConfig: openidConfig, - Sessions: sessionHandler, - UpstreamProxy: NewReverseProxy(cfg.UpstreamHost), + AutoLogin: autoLogin, + Client: openidClient, + Config: cfg, + CookieOptions: cookieOpts, + Crypter: crypter, + Ingresses: ingresses, + Loginstatus: loginstatusClient, + OpenidConfig: openidConfig, + Sessions: sessionHandler, + UpstreamProxy: NewReverseProxy(cfg.UpstreamHost), + RedirectHandler: redirectHandler, }, nil } @@ -128,6 +133,10 @@ func (d *DefaultHandler) GetPath(r *http.Request) string { return path } +func (d *DefaultHandler) GetRedirectHandler() redirect.Handler { + return d.RedirectHandler +} + func (d *DefaultHandler) GetSessions() *session.Handler { return d.Sessions } diff --git a/pkg/handler/handler_sso_proxy.go b/pkg/handler/handler_sso_proxy.go index 05f283f..3528247 100644 --- a/pkg/handler/handler_sso_proxy.go +++ b/pkg/handler/handler_sso_proxy.go @@ -5,13 +5,10 @@ import ( "net/http" urllib "net/url" - log "github.com/sirupsen/logrus" - "github.com/nais/wonderwall/pkg/config" - "github.com/nais/wonderwall/pkg/handler/url" "github.com/nais/wonderwall/pkg/ingress" - mw "github.com/nais/wonderwall/pkg/middleware" openidclient "github.com/nais/wonderwall/pkg/openid/client" + "github.com/nais/wonderwall/pkg/redirect" "github.com/nais/wonderwall/pkg/router" "github.com/nais/wonderwall/pkg/router/paths" ) @@ -19,9 +16,10 @@ import ( var _ router.Source = &SSOProxyHandler{} type SSOProxyHandler struct { - Config *config.Config - Ingresses *ingress.Ingresses - SSOServerURL *urllib.URL + Config *config.Config + Ingresses *ingress.Ingresses + RedirectHandler redirect.Handler + SSOServerURL *urllib.URL } func NewSSOProxyHandler(cfg *config.Config) (*SSOProxyHandler, error) { @@ -30,6 +28,8 @@ func NewSSOProxyHandler(cfg *config.Config) (*SSOProxyHandler, error) { return nil, err } + redirectHandler := redirect.NewSSOProxyHandler(ingresses) + u, err := urllib.ParseRequestURI(cfg.SSO.ServerURL) if err != nil { return nil, fmt.Errorf("parsing sso server url: %w", err) @@ -48,47 +48,15 @@ func NewSSOProxyHandler(cfg *config.Config) (*SSOProxyHandler, error) { u.RawQuery = query.Encode() return &SSOProxyHandler{ - Config: cfg, - Ingresses: ingresses, - SSOServerURL: u, + Config: cfg, + Ingresses: ingresses, + SSOServerURL: u, + RedirectHandler: redirectHandler, }, nil } func (s *SSOProxyHandler) Login(w http.ResponseWriter, r *http.Request) { - target := *s.SSOServerURL - targetQuery := target.Query() - - reqQuery := r.URL.Query() - - if reqQuery.Has(openidclient.SecurityLevelURLParameter) { - targetQuery.Set(openidclient.SecurityLevelURLParameter, reqQuery.Get(openidclient.SecurityLevelURLParameter)) - } - - if reqQuery.Has(openidclient.LocaleURLParameter) { - targetQuery.Set(openidclient.LocaleURLParameter, reqQuery.Get(openidclient.LocaleURLParameter)) - } - - target.RawQuery = reqQuery.Encode() - - redirect, err := url.Ingress(r) - if err != nil { - redirect = s.Ingresses.Single().NewURL() - } - parsedRedirect, err := urllib.ParseRequestURI(reqQuery.Get(url.RedirectURLParameter)) - if err == nil { - redirect = redirect.JoinPath(parsedRedirect.Path) - } - - ssoServerLoginURL := url.Login(&target, redirect.String()) - - mw.LogEntryFrom(r). - WithFields(log.Fields{ - "redirect_to": ssoServerLoginURL, - "redirect_after_login": redirect.String(), - }). - Info("login: redirecting to sso server") - - http.Redirect(w, r, ssoServerLoginURL, http.StatusTemporaryRedirect) + LoginSSOProxy(s, w, r) } func (s *SSOProxyHandler) LoginCallback(w http.ResponseWriter, r *http.Request) { @@ -131,3 +99,12 @@ func (s *SSOProxyHandler) ReverseProxy(w http.ResponseWriter, r *http.Request) { func (s *SSOProxyHandler) GetIngresses() *ingress.Ingresses { return s.Ingresses } + +func (s *SSOProxyHandler) GetRedirectHandler() redirect.Handler { + return s.RedirectHandler +} + +func (s *SSOProxyHandler) GetSSOServerURL() *urllib.URL { + u := *s.SSOServerURL + return &u +} diff --git a/pkg/handler/handler_sso_server.go b/pkg/handler/handler_sso_server.go index 1b8c921..47722ca 100644 --- a/pkg/handler/handler_sso_server.go +++ b/pkg/handler/handler_sso_server.go @@ -3,6 +3,7 @@ package handler import ( "net/http" + "github.com/nais/wonderwall/pkg/redirect" "github.com/nais/wonderwall/pkg/router" ) @@ -12,8 +13,13 @@ type SSOServerHandler struct { DefaultHandler } -func NewSSOServerHandler(handler *DefaultHandler) *SSOServerHandler { - return &SSOServerHandler{DefaultHandler: *handler} +func NewSSOServerHandler(handler *DefaultHandler) (*SSOServerHandler, error) { + rdHandler, err := redirect.NewSSOServerHandler(handler.Config) + if err != nil { + return nil, err + } + handler.RedirectHandler = rdHandler + return &SSOServerHandler{DefaultHandler: *handler}, nil } func (s *SSOServerHandler) ReverseProxy(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/handler/login.go b/pkg/handler/login.go index 47e9270..2e975ef 100644 --- a/pkg/handler/login.go +++ b/pkg/handler/login.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "time" log "github.com/sirupsen/logrus" @@ -15,6 +16,8 @@ import ( logentry "github.com/nais/wonderwall/pkg/middleware" "github.com/nais/wonderwall/pkg/openid" openidclient "github.com/nais/wonderwall/pkg/openid/client" + "github.com/nais/wonderwall/pkg/redirect" + urlpkg "github.com/nais/wonderwall/pkg/url" ) const ( @@ -26,9 +29,11 @@ type LoginSource interface { GetCookieOptsPathAware(r *http.Request) cookie.Options GetCrypter() crypto.Crypter GetErrorHandler() errorhandler.Handler + GetRedirectHandler() redirect.Handler } func Login(src LoginSource, w http.ResponseWriter, r *http.Request) { + canonicalRedirect := src.GetRedirectHandler().Canonical(r) login, err := src.GetClient().Login(r) if err != nil { if errors.Is(err, openidclient.ErrInvalidSecurityLevel) || errors.Is(err, openidclient.ErrInvalidLocale) { @@ -40,19 +45,52 @@ func Login(src LoginSource, w http.ResponseWriter, r *http.Request) { return } - err = setLoginCookies(src, w, r, login.Cookie()) + err = setLoginCookies(src, w, r, login.Cookie(canonicalRedirect)) if err != nil { src.GetErrorHandler().InternalError(w, r, fmt.Errorf("login: setting cookie: %w", err)) return } fields := log.Fields{ - "redirect_after_login": login.CanonicalRedirect(), + "redirect_after_login": canonicalRedirect, } - logentry.LogEntryFrom(r).WithFields(fields).Debug("login: redirecting to identity provider") + logentry.LogEntryFrom(r).WithFields(fields).Info("login: redirecting to identity provider") http.Redirect(w, r, login.AuthCodeURL(), http.StatusTemporaryRedirect) } +type LoginSSOProxySource interface { + GetSSOServerURL() *url.URL + GetRedirectHandler() redirect.Handler +} + +func LoginSSOProxy(src LoginSSOProxySource, w http.ResponseWriter, r *http.Request) { + logger := logentry.LogEntryFrom(r) + + target := src.GetSSOServerURL() + targetQuery := target.Query() + + // override default query parameters + reqQuery := r.URL.Query() + if reqQuery.Has(openidclient.SecurityLevelURLParameter) { + targetQuery.Set(openidclient.SecurityLevelURLParameter, reqQuery.Get(openidclient.SecurityLevelURLParameter)) + } + if reqQuery.Has(openidclient.LocaleURLParameter) { + targetQuery.Set(openidclient.LocaleURLParameter, reqQuery.Get(openidclient.LocaleURLParameter)) + } + + target.RawQuery = reqQuery.Encode() + + canonicalRedirect := src.GetRedirectHandler().Canonical(r) + ssoServerLoginURL := urlpkg.Login(target, canonicalRedirect) + + logger.WithFields(log.Fields{ + "redirect_to": ssoServerLoginURL, + "redirect_after_login": canonicalRedirect, + }).Info("login: redirecting to sso server") + + http.Redirect(w, r, ssoServerLoginURL, http.StatusTemporaryRedirect) +} + func setLoginCookies(src LoginSource, w http.ResponseWriter, r *http.Request, loginCookie *openid.LoginCookie) error { loginCookieJson, err := json.Marshal(loginCookie) if err != nil { diff --git a/pkg/handler/login_callback.go b/pkg/handler/login_callback.go index b47f466..eba24a0 100644 --- a/pkg/handler/login_callback.go +++ b/pkg/handler/login_callback.go @@ -18,6 +18,7 @@ import ( logentry "github.com/nais/wonderwall/pkg/middleware" "github.com/nais/wonderwall/pkg/openid" openidclient "github.com/nais/wonderwall/pkg/openid/client" + "github.com/nais/wonderwall/pkg/redirect" retrypkg "github.com/nais/wonderwall/pkg/retry" "github.com/nais/wonderwall/pkg/session" ) @@ -29,6 +30,7 @@ type LoginCallbackSource interface { GetCrypter() crypto.Crypter GetErrorHandler() errorhandler.Handler GetLoginstatus() *loginstatus.Loginstatus + GetRedirectHandler() redirect.Handler GetSessions() *session.Handler GetSessionConfig() config.Session } @@ -96,9 +98,11 @@ func LoginCallback(src LoginCallbackSource, w http.ResponseWriter, r *http.Reque logentry.LogEntryFrom(r).Debug("callback: successfully fetched loginstatus token") } - logSuccessfulLogin(r, tokens, loginCookie.Referer) + rd := src.GetRedirectHandler().Clean(r, loginCookie.Referer) + + logSuccessfulLogin(r, tokens, rd) cookie.Clear(w, cookie.Retry, src.GetCookieOptsPathAware(r)) - http.Redirect(w, r, loginCookie.Referer, http.StatusTemporaryRedirect) + http.Redirect(w, r, rd, http.StatusTemporaryRedirect) } func clearLoginCookies(src LogoutCallbackSource, w http.ResponseWriter, r *http.Request) { diff --git a/pkg/openid/client/login.go b/pkg/openid/client/login.go index 37f46c7..4e4d5b8 100644 --- a/pkg/openid/client/login.go +++ b/pkg/openid/client/login.go @@ -53,32 +53,25 @@ func NewLogin(c *Client, r *http.Request) (*Login, error) { return nil, fmt.Errorf("generating auth code url: %w", err) } - referer := urlpkg.CanonicalRedirect(r) - cookie := params.cookie(referer, callbackURL) + cookie := params.cookie(callbackURL) return &Login{ - authCodeURL: url, - canonicalRedirect: referer, - cookie: cookie, - params: params, + authCodeURL: url, + cookie: cookie, + params: params, }, nil } type Login struct { - authCodeURL string - canonicalRedirect string - cookie *openid.LoginCookie - params *loginParameters + authCodeURL string + cookie *openid.LoginCookie + params *loginParameters } func (l *Login) AuthCodeURL() string { return l.authCodeURL } -func (l *Login) CanonicalRedirect() string { - return l.canonicalRedirect -} - func (l *Login) CodeChallenge() string { return l.params.CodeChallenge } @@ -87,7 +80,8 @@ func (l *Login) CodeVerifier() string { return l.params.CodeVerifier } -func (l *Login) Cookie() *openid.LoginCookie { +func (l *Login) Cookie(canonicalRedirect string) *openid.LoginCookie { + l.cookie.Referer = canonicalRedirect return l.cookie } @@ -159,12 +153,11 @@ func (in *loginParameters) authCodeURL(r *http.Request, callbackURL string, logi return authCodeUrl, nil } -func (in *loginParameters) cookie(referer, redirectURI string) *openid.LoginCookie { +func (in *loginParameters) cookie(redirectURI string) *openid.LoginCookie { return &openid.LoginCookie{ State: in.State, Nonce: in.Nonce, CodeVerifier: in.CodeVerifier, - Referer: referer, RedirectURI: redirectURI, } }