From ef457b17653c3d5d390c7856f3953f3e6bd2b96c Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sat, 6 Feb 2021 22:05:45 +0000 Subject: [PATCH 1/4] Move Error page rendering to app package --- oauthproxy.go | 81 +++++++++++++++----------------------- pkg/app/error_page.go | 56 ++++++++++++++++++++++++++ pkg/app/error_page_test.go | 35 ++++++++++++++++ 3 files changed, 122 insertions(+), 50 deletions(-) create mode 100644 pkg/app/error_page.go create mode 100644 pkg/app/error_page_test.go diff --git a/oauthproxy.go b/oauthproxy.go index 0ef5060c..e3fcf0f2 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -108,6 +108,7 @@ type OAuthProxy struct { sessionChain alice.Chain headersChain alice.Chain preAuthChain alice.Chain + errorPage *app.ErrorPage } // NewOAuthProxy creates a new instance of OAuthProxy from the options provided @@ -224,6 +225,12 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr sessionChain: sessionChain, headersChain: headersChain, preAuthChain: preAuthChain, + errorPage: &app.ErrorPage{ + Template: templates.Lookup("error.html"), + ProxyPrefix: opts.ProxyPrefix, + Footer: opts.Templates.Footer, + Version: VERSION, + }, }, nil } @@ -507,14 +514,14 @@ func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter, req *http.Request) { _, err := fmt.Fprintf(rw, "User-agent: *\nDisallow: /") if err != nil { logger.Printf("Error writing robots.txt: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } rw.WriteHeader(http.StatusOK) } // ErrorPage writes an error response -func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, title string, message string) { +func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string) { redirectURL, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) @@ -523,32 +530,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i redirectURL = "/" } - rw.WriteHeader(code) - - // We allow unescaped template.HTML since it is user configured options - /* #nosec G203 */ - t := struct { - Title string - Message string - ProxyPrefix string - StatusCode int - Redirect string - Footer template.HTML - Version string - }{ - Title: title, - Message: message, - ProxyPrefix: p.ProxyPrefix, - StatusCode: code, - Redirect: redirectURL, - Footer: template.HTML(p.Footer), - Version: VERSION, - } - - if err := p.templates.ExecuteTemplate(rw, "error.html", t); err != nil { - logger.Printf("Error rendering error.html template: %v", err) - http.Error(rw, "Internal Server Error", http.StatusInternalServerError) - } + p.errorPage.Render(rw, code, redirectURL, appError) } // IsAllowedRequest is used to check if auth should be skipped for this request @@ -593,7 +575,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code err := p.ClearSessionCookie(rw, req) if err != nil { logger.Printf("Error clearing session cookie: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } rw.WriteHeader(code) @@ -601,7 +583,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code redirectURL, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } @@ -634,7 +616,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code err = p.templates.ExecuteTemplate(rw, "sign_in.html", t) if err != nil { logger.Printf("Error rendering sign_in.html template: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) } } @@ -662,7 +644,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { redirect, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } @@ -672,7 +654,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { err = p.SaveSession(rw, req, session) if err != nil { logger.Printf("Error saving session: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } http.Redirect(rw, req, redirect, http.StatusFound) @@ -711,7 +693,7 @@ func (p *OAuthProxy) UserInfo(rw http.ResponseWriter, req *http.Request) { err = json.NewEncoder(rw).Encode(userInfo) if err != nil { logger.Printf("Error encoding user info: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) } } @@ -720,13 +702,13 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) { redirect, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } err = p.ClearSessionCookie(rw, req) if err != nil { logger.Errorf("Error clearing session cookie: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } http.Redirect(rw, req, redirect, http.StatusFound) @@ -738,14 +720,14 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) { nonce, err := encryption.Nonce() if err != nil { logger.Errorf("Error obtaining nonce: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } p.SetCSRFCookie(rw, req, nonce) redirect, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } redirectURI := p.getOAuthRedirectURI(req) @@ -761,34 +743,34 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { err := req.ParseForm() if err != nil { logger.Errorf("Error while parsing OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } errorString := req.Form.Get("error") if errorString != "" { logger.Errorf("Error while parsing OAuth2 callback: %s", errorString) - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", errorString) + p.ErrorPage(rw, req, http.StatusForbidden, errorString) return } session, err := p.redeemCode(req) if err != nil { logger.Errorf("Error redeeming code during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") return } err = p.enrichSessionState(req.Context(), session) if err != nil { logger.Errorf("Error creating session during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") return } state := strings.SplitN(req.Form.Get("state"), ":", 2) if len(state) != 2 { logger.Error("Error while parsing OAuth2 state: invalid length") - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", "Invalid State") + p.ErrorPage(rw, req, http.StatusInternalServerError, "Invalid State") return } nonce := state[0] @@ -796,13 +778,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { c, err := req.Cookie(p.CSRFCookieName) if err != nil { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie") - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", err.Error()) + p.ErrorPage(rw, req, http.StatusForbidden, err.Error()) return } p.ClearCSRFCookie(rw, req) if c.Value != nonce { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack") - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", "CSRF Failed") + p.ErrorPage(rw, req, http.StatusForbidden, "CSRF Failed") return } @@ -820,13 +802,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { err := p.SaveSession(rw, req, session) if err != nil { logger.Errorf("Error saving session state for %s: %v", remoteAddr, err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error()) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } http.Redirect(rw, req, redirect, http.StatusFound) } else { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized") - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", "Invalid Account") + p.ErrorPage(rw, req, http.StatusForbidden, "Invalid Account") } } @@ -908,13 +890,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { } case ErrAccessDenied: - p.ErrorPage(rw, req, http.StatusUnauthorized, "Permission Denied", "Unauthorized") + p.ErrorPage(rw, req, http.StatusUnauthorized, "Unauthorized") default: // unknown error logger.Errorf("Unexpected internal error: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, - "Internal Error", "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") } } diff --git a/pkg/app/error_page.go b/pkg/app/error_page.go new file mode 100644 index 00000000..aa58f077 --- /dev/null +++ b/pkg/app/error_page.go @@ -0,0 +1,56 @@ +package app + +import ( + "html/template" + "net/http" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" +) + +// ErrorPage is used to render error pages. +type ErrorPage struct { + // Template is the error page HTML template. + Template *template.Template + + // ProxyPrefix is the prefix under which OAuth2 Proxy pages are served. + ProxyPrefix string + + // Footer is the footer to be displayed at the bottom of the page. + // If not set, a default footer will be used. + Footer string + + // Version is the OAuth2 Proxy version to be used in the default footer. + Version string +} + +// Render writes an error page to the given response writer. +// It uses the passed redirectURL to give users the option to go back to where +// they originally came from or try signing in again. +func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string) { + rw.WriteHeader(status) + + // We allow unescaped template.HTML since it is user configured options + /* #nosec G203 */ + data := struct { + Title string + Message string + ProxyPrefix string + StatusCode int + Redirect string + Footer template.HTML + Version string + }{ + Title: http.StatusText(status), + Message: appError, + ProxyPrefix: e.ProxyPrefix, + StatusCode: status, + Redirect: redirectURL, + Footer: template.HTML(e.Footer), + Version: e.Version, + } + + if err := e.Template.Execute(rw, data); err != nil { + logger.Printf("Error rendering error template: %v", err) + http.Error(rw, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + } +} diff --git a/pkg/app/error_page_test.go b/pkg/app/error_page_test.go new file mode 100644 index 00000000..f80b0c78 --- /dev/null +++ b/pkg/app/error_page_test.go @@ -0,0 +1,35 @@ +package app + +import ( + "html/template" + "io/ioutil" + "net/http/httptest" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Error Page", func() { + + Context("Render", func() { + It("Writes the template to the response writer", func() { + tmpl, err := template.New("").Parse("{{.Title}} {{.Message}} {{.ProxyPrefix}} {{.StatusCode}} {{.Redirect}} {{.Footer}} {{.Version}}") + Expect(err).ToNot(HaveOccurred()) + + errorPage := &ErrorPage{ + Template: tmpl, + ProxyPrefix: "/prefix/", + Footer: "Custom Footer Text", + Version: "v0.0.0-test", + } + + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 403, "/redirect", "Access Denied") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Forbidden Access Denied /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) + }) + }) + +}) From a63ed0225c39a893d9fc8fcddf672b7e5fde7711 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sat, 6 Feb 2021 22:17:59 +0000 Subject: [PATCH 2/4] Use ErrorPage to render proxy error page --- oauthproxy.go | 18 ++++++++++-------- pkg/app/error_page.go | 8 ++++++++ pkg/app/error_page_test.go | 35 +++++++++++++++++++++++++---------- pkg/app/templates.go | 2 ++ pkg/upstream/proxy.go | 22 ---------------------- pkg/upstream/proxy_test.go | 11 ++++++----- 6 files changed, 51 insertions(+), 45 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index e3fcf0f2..ca921efb 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -122,8 +122,15 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr if err != nil { return nil, fmt.Errorf("error loading templates: %v", err) } - proxyErrorHandler := upstream.NewProxyErrorHandler(templates.Lookup("error.html"), opts.ProxyPrefix) - upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), proxyErrorHandler) + + errorPage := &app.ErrorPage{ + Template: templates.Lookup("error.html"), + ProxyPrefix: opts.ProxyPrefix, + Footer: opts.Templates.Footer, + Version: VERSION, + } + + upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler) if err != nil { return nil, fmt.Errorf("error initialising upstream proxy: %v", err) } @@ -225,12 +232,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr sessionChain: sessionChain, headersChain: headersChain, preAuthChain: preAuthChain, - errorPage: &app.ErrorPage{ - Template: templates.Lookup("error.html"), - ProxyPrefix: opts.ProxyPrefix, - Footer: opts.Templates.Footer, - Version: VERSION, - }, + errorPage: errorPage, }, nil } diff --git a/pkg/app/error_page.go b/pkg/app/error_page.go index aa58f077..d26fed63 100644 --- a/pkg/app/error_page.go +++ b/pkg/app/error_page.go @@ -54,3 +54,11 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin http.Error(rw, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) } } + +// ProxyErrorHandler is used by the upstream ReverseProxy to render error pages +// when there are issues with upstream servers. +// It is expected to always render a bad gateway error. +func (e *ErrorPage) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) { + logger.Errorf("Error proxying to upstream server: %v", proxyErr) + e.Render(rw, http.StatusBadGateway, "", "Error proxying to upstream server") +} diff --git a/pkg/app/error_page_test.go b/pkg/app/error_page_test.go index f80b0c78..f2265431 100644 --- a/pkg/app/error_page_test.go +++ b/pkg/app/error_page_test.go @@ -1,6 +1,7 @@ package app import ( + "errors" "html/template" "io/ioutil" "net/http/httptest" @@ -10,19 +11,22 @@ import ( ) var _ = Describe("Error Page", func() { + var errorPage *ErrorPage + + BeforeEach(func() { + tmpl, err := template.New("").Parse("{{.Title}} {{.Message}} {{.ProxyPrefix}} {{.StatusCode}} {{.Redirect}} {{.Footer}} {{.Version}}") + Expect(err).ToNot(HaveOccurred()) + + errorPage = &ErrorPage{ + Template: tmpl, + ProxyPrefix: "/prefix/", + Footer: "Custom Footer Text", + Version: "v0.0.0-test", + } + }) Context("Render", func() { It("Writes the template to the response writer", func() { - tmpl, err := template.New("").Parse("{{.Title}} {{.Message}} {{.ProxyPrefix}} {{.StatusCode}} {{.Redirect}} {{.Footer}} {{.Version}}") - Expect(err).ToNot(HaveOccurred()) - - errorPage := &ErrorPage{ - Template: tmpl, - ProxyPrefix: "/prefix/", - Footer: "Custom Footer Text", - Version: "v0.0.0-test", - } - recorder := httptest.NewRecorder() errorPage.Render(recorder, 403, "/redirect", "Access Denied") @@ -32,4 +36,15 @@ var _ = Describe("Error Page", func() { }) }) + Context("ProxyErrorHandler", func() { + It("Writes a bad gateway error the response writer", func() { + req := httptest.NewRequest("", "/bad-gateway", nil) + recorder := httptest.NewRecorder() + errorPage.ProxyErrorHandler(recorder, req, errors.New("some upstream error")) + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Bad Gateway Error proxying to upstream server /prefix/ 502 Custom Footer Text v0.0.0-test")) + }) + }) }) diff --git a/pkg/app/templates.go b/pkg/app/templates.go index ef38c902..aa286fc9 100644 --- a/pkg/app/templates.go +++ b/pkg/app/templates.go @@ -79,6 +79,7 @@ const ( {{ end }} + {{ if .Redirect }}
@@ -94,6 +95,7 @@ const (
+ {{ end }} diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index 80d4b4d5..f345158b 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -2,7 +2,6 @@ package upstream import ( "fmt" - "html/template" "net/http" "net/url" @@ -71,24 +70,3 @@ func (m *multiUpstreamProxy) registerHTTPUpstreamProxy(upstream options.Upstream logger.Printf("mapping path %q => upstream %q", upstream.Path, upstream.URI) m.serveMux.Handle(upstream.Path, newHTTPUpstreamProxy(upstream, u, sigData, errorHandler)) } - -// NewProxyErrorHandler creates a ProxyErrorHandler using the template given. -func NewProxyErrorHandler(errorTemplate *template.Template, proxyPrefix string) ProxyErrorHandler { - return func(rw http.ResponseWriter, req *http.Request, proxyErr error) { - logger.Errorf("Error proxying to upstream server: %v", proxyErr) - rw.WriteHeader(http.StatusBadGateway) - data := struct { - Title string - Message string - ProxyPrefix string - }{ - Title: "Bad Gateway", - Message: "Error proxying to upstream server", - ProxyPrefix: proxyPrefix, - } - err := errorTemplate.Execute(rw, data) - if err != nil { - http.Error(rw, "Internal Server Error", http.StatusInternalServerError) - } - } -} diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index 31b7bfa6..e834bc60 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -4,7 +4,6 @@ import ( "crypto" "encoding/json" "fmt" - "html/template" "net/http" "net/http/httptest" @@ -20,9 +19,10 @@ var _ = Describe("Proxy Suite", func() { BeforeEach(func() { sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} - tmpl, err := template.New("").Parse("{{ .Title }}\n{{ .Message }}\n{{ .ProxyPrefix }}") - Expect(err).ToNot(HaveOccurred()) - errorHandler := NewProxyErrorHandler(tmpl, "prefix") + errorHandler := func(rw http.ResponseWriter, _ *http.Request, _ error) { + rw.WriteHeader(502) + rw.Write([]byte("Proxy Error")) + } ok := http.StatusOK @@ -56,6 +56,7 @@ var _ = Describe("Proxy Suite", func() { }, } + var err error upstreamServer, err = NewProxy(upstreams, sigData, errorHandler) Expect(err).ToNot(HaveOccurred()) }) @@ -143,7 +144,7 @@ var _ = Describe("Proxy Suite", func() { gapUpstream: {"bad-http-backend"}, }, // This tests the error handler - raw: "Bad Gateway\nError proxying to upstream server\nprefix", + raw: "Proxy Error", }, }), Entry("with a request to the to an unregistered path", &proxyTableInput{ From 6ecbc7bc4e1aba4a7db5157433d0f318366c880f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 10 Feb 2021 19:34:19 +0000 Subject: [PATCH 3/4] Allow users to choose detailed error messages on error pages --- oauthproxy.go | 25 +++++++++-------- oauthproxy_test.go | 2 +- pkg/apis/options/app.go | 7 +++++ pkg/app/error_page.go | 39 ++++++++++++++++++++++++--- pkg/app/error_page_test.go | 55 ++++++++++++++++++++++++++++++++++++-- 5 files changed, 111 insertions(+), 17 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index ca921efb..e74dcabc 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -128,6 +128,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr ProxyPrefix: opts.ProxyPrefix, Footer: opts.Templates.Footer, Version: VERSION, + Debug: opts.Templates.Debug, } upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler) @@ -523,7 +524,7 @@ func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter, req *http.Request) { } // ErrorPage writes an error response -func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string) { +func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string, messages ...interface{}) { redirectURL, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) @@ -532,7 +533,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i redirectURL = "/" } - p.errorPage.Render(rw, code, redirectURL, appError) + p.errorPage.Render(rw, code, redirectURL, appError, messages...) } // IsAllowedRequest is used to check if auth should be skipped for this request @@ -751,28 +752,30 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { errorString := req.Form.Get("error") if errorString != "" { logger.Errorf("Error while parsing OAuth2 callback: %s", errorString) - p.ErrorPage(rw, req, http.StatusForbidden, errorString) + message := fmt.Sprintf("Login Failed: The upstream identity provider returned an error: %s", errorString) + // Set the debug message and override the non debug message to be the same for this case + p.ErrorPage(rw, req, http.StatusForbidden, message, message) return } session, err := p.redeemCode(req) if err != nil { logger.Errorf("Error redeeming code during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } err = p.enrichSessionState(req.Context(), session) if err != nil { logger.Errorf("Error creating session during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } state := strings.SplitN(req.Form.Get("state"), ":", 2) if len(state) != 2 { logger.Error("Error while parsing OAuth2 state: invalid length") - p.ErrorPage(rw, req, http.StatusInternalServerError, "Invalid State") + p.ErrorPage(rw, req, http.StatusInternalServerError, "State paremeter did not have expected length", "Login Failed: Invalid State after login.") return } nonce := state[0] @@ -780,13 +783,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { c, err := req.Cookie(p.CSRFCookieName) if err != nil { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie") - p.ErrorPage(rw, req, http.StatusForbidden, err.Error()) + p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.") return } p.ClearCSRFCookie(rw, req) if c.Value != nonce { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack") - p.ErrorPage(rw, req, http.StatusForbidden, "CSRF Failed") + p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.") return } @@ -810,7 +813,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { http.Redirect(rw, req, redirect, http.StatusFound) } else { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized") - p.ErrorPage(rw, req, http.StatusForbidden, "Invalid Account") + p.ErrorPage(rw, req, http.StatusForbidden, "Invalid session: unauthorized") } } @@ -892,12 +895,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { } case ErrAccessDenied: - p.ErrorPage(rw, req, http.StatusUnauthorized, "Unauthorized") + p.ErrorPage(rw, req, http.StatusForbidden, "The session failed authorization checks") default: // unknown error logger.Errorf("Unexpected internal error: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 06b77ca8..06014c9b 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -2815,7 +2815,7 @@ func TestProxyAllowedGroups(t *testing.T) { test.proxy.ServeHTTP(test.rw, test.req) if tt.expectUnauthorized { - assert.Equal(t, http.StatusUnauthorized, test.rw.Code) + assert.Equal(t, http.StatusForbidden, test.rw.Code) } else { assert.Equal(t, http.StatusOK, test.rw.Code) } diff --git a/pkg/apis/options/app.go b/pkg/apis/options/app.go index 1574ac97..76c4f84a 100644 --- a/pkg/apis/options/app.go +++ b/pkg/apis/options/app.go @@ -22,6 +22,12 @@ type Templates struct { // password form if a static passwords file (htpasswd file) has been // configured. DisplayLoginForm bool `flag:"display-htpasswd-form" cfg:"display_htpasswd_form"` + + // Debug renders detailed errors when an error page is shown. + // It is not advised to use this in production as errors may contain sensitive + // information. + // Use only for diagnosing backend errors. + Debug bool `flag:"show-debug-on-error" cfg:"show-debug-on-error"` } func templatesFlagSet() *pflag.FlagSet { @@ -31,6 +37,7 @@ func templatesFlagSet() *pflag.FlagSet { flagSet.String("banner", "", "custom banner string. Use \"-\" to disable default banner.") flagSet.String("footer", "", "custom footer string. Use \"-\" to disable default footer.") flagSet.Bool("display-htpasswd-form", true, "display username / password login form if an htpasswd file is provided") + flagSet.Bool("show-debug-on-error", false, "show detailed error information on error pages (WARNING: this may contain sensitive information - do not use in production)") return flagSet } diff --git a/pkg/app/error_page.go b/pkg/app/error_page.go index d26fed63..56d1c6af 100644 --- a/pkg/app/error_page.go +++ b/pkg/app/error_page.go @@ -1,12 +1,22 @@ package app import ( + "fmt" "html/template" "net/http" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" ) +// errorMessages are default error messages for each of the the different +// http status codes expected to be rendered in the error page. +var errorMessages = map[int]string{ + http.StatusInternalServerError: "Oops! Something went wrong. For more information contact your server administrator.", + http.StatusNotFound: "We could not find the resource you were looking for.", + http.StatusForbidden: "You do not have permission to access this resource.", + http.StatusUnauthorized: "You need to be logged in to access this resource.", +} + // ErrorPage is used to render error pages. type ErrorPage struct { // Template is the error page HTML template. @@ -21,12 +31,16 @@ type ErrorPage struct { // Version is the OAuth2 Proxy version to be used in the default footer. Version string + + // Debug determines whether errors pages should be rendered with detailed + // errors. + Debug bool } // Render writes an error page to the given response writer. // It uses the passed redirectURL to give users the option to go back to where // they originally came from or try signing in again. -func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string) { +func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string, messages ...interface{}) { rw.WriteHeader(status) // We allow unescaped template.HTML since it is user configured options @@ -41,7 +55,7 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin Version string }{ Title: http.StatusText(status), - Message: appError, + Message: e.getMessage(status, appError, messages...), ProxyPrefix: e.ProxyPrefix, StatusCode: status, Redirect: redirectURL, @@ -60,5 +74,24 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin // It is expected to always render a bad gateway error. func (e *ErrorPage) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) { logger.Errorf("Error proxying to upstream server: %v", proxyErr) - e.Render(rw, http.StatusBadGateway, "", "Error proxying to upstream server") + e.Render(rw, http.StatusBadGateway, "", proxyErr.Error(), "There was a problem connecting to the upstream server.") +} + +// getMessage creates the message for the template parameters. +// If the ErrorPage.Debug is enabled, the application error takes precedence. +// Otherwise, any messages will be used. +// The first message is expected to be a format string. +// If no messages are supplied, a default error message will be used. +func (e *ErrorPage) getMessage(status int, appError string, messages ...interface{}) string { + if e.Debug { + return appError + } + if len(messages) > 0 { + format := fmt.Sprintf("%v", messages[0]) + return fmt.Sprintf(format, messages[1:]...) + } + if msg, ok := errorMessages[status]; ok { + return msg + } + return "Unknown error" } diff --git a/pkg/app/error_page_test.go b/pkg/app/error_page_test.go index f2265431..5c4f78fa 100644 --- a/pkg/app/error_page_test.go +++ b/pkg/app/error_page_test.go @@ -32,7 +32,25 @@ var _ = Describe("Error Page", func() { body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(string(body)).To(Equal("Forbidden Access Denied /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) + Expect(string(body)).To(Equal("Forbidden You do not have permission to access this resource. /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) + }) + + It("With a different code, uses the stock message for the correct code", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 500, "/redirect", "Access Denied") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Internal Server Error Oops! Something went wrong. For more information contact your server administrator. /prefix/ 500 /redirect Custom Footer Text v0.0.0-test")) + }) + + It("With a message override, uses the message", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 403, "/redirect", "Access Denied", "An extra message: %s", "with more context.") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Forbidden An extra message: with more context. /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) }) }) @@ -44,7 +62,40 @@ var _ = Describe("Error Page", func() { body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(string(body)).To(Equal("Bad Gateway Error proxying to upstream server /prefix/ 502 Custom Footer Text v0.0.0-test")) + Expect(string(body)).To(Equal("Bad Gateway There was a problem connecting to the upstream server. /prefix/ 502 Custom Footer Text v0.0.0-test")) + }) + }) + + Context("With Debug enabled", func() { + BeforeEach(func() { + tmpl, err := template.New("").Parse("{{.Message}}") + Expect(err).ToNot(HaveOccurred()) + + errorPage.Template = tmpl + errorPage.Debug = true + }) + + Context("Render", func() { + It("Writes the detailed error in place of the message", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 403, "/redirect", "Debug error") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Debug error")) + }) + }) + + Context("ProxyErrorHandler", func() { + It("Writes a bad gateway error the response writer", func() { + req := httptest.NewRequest("", "/bad-gateway", nil) + recorder := httptest.NewRecorder() + errorPage.ProxyErrorHandler(recorder, req, errors.New("some upstream error")) + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("some upstream error")) + }) }) }) }) From 9e8c2af86bd38e5840f68db5fb948bc9d345d25f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sat, 13 Feb 2021 10:48:03 +0000 Subject: [PATCH 4/4] Update docs for new show-debug-on-error option --- CHANGELOG.md | 1 + docs/docs/configuration/overview.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7a499f7..10af30fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.0.1 +- [#1029](https://github.com/oauth2-proxy/oauth2-proxy/pull/1029) Refactor error page rendering and allow debug messages on error (@JoelSpeed) - [#1028](https://github.com/oauth2-proxy/oauth2-proxy/pull/1028) Refactor templates, update theme and provide styled error pages (@JoelSpeed) - [#1039](https://github.com/oauth2-proxy/oauth2-proxy/pull/1039) Ensure errors in tests are logged to the GinkgoWriter (@JoelSpeed) diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 920b96ce..feafeda5 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -115,6 +115,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--set-xauthrequest` | bool | set X-Auth-Request-User, X-Auth-Request-Groups, X-Auth-Request-Email and X-Auth-Request-Preferred-Username response headers (useful in Nginx auth_request mode). When used with `--pass-access-token`, X-Auth-Request-Access-Token is added to response headers. | false | | `--set-authorization-header` | bool | set Authorization Bearer response header (useful in Nginx auth_request mode) | false | | `--set-basic-auth` | bool | set HTTP Basic Auth information in response (useful in Nginx auth_request mode) | false | +| `--show-debug-on-error` | bool | show detailed error information on error pages (WARNING: this may contain sensitive information - do not use in production) | false | | `--signature-key` | string | GAP-Signature request signature key (algorithm:secretkey) | | | `--silence-ping-logging` | bool | disable logging of requests to ping endpoint | false | | `--skip-auth-preflight` | bool | will skip authentication for OPTIONS requests | false |