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 | diff --git a/oauthproxy.go b/oauthproxy.go index 0ef5060c..e74dcabc 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 @@ -121,8 +122,16 @@ 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, + Debug: opts.Templates.Debug, + } + + upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler) if err != nil { return nil, fmt.Errorf("error initialising upstream proxy: %v", err) } @@ -224,6 +233,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr sessionChain: sessionChain, headersChain: headersChain, preAuthChain: preAuthChain, + errorPage: errorPage, }, nil } @@ -507,14 +517,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, messages ...interface{}) { redirectURL, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) @@ -523,32 +533,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, messages...) } // IsAllowedRequest is used to check if auth should be skipped for this request @@ -593,7 +578,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 +586,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 +619,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 +647,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 +657,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 +696,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 +705,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 +723,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 +746,36 @@ 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) + 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 Server Error", "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 Server Error", "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, "Internal Server Error", "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] @@ -796,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, "Permission Denied", 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, "Permission Denied", "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 } @@ -820,13 +807,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 session: unauthorized") } } @@ -908,13 +895,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.StatusForbidden, "The session failed authorization checks") 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, 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 new file mode 100644 index 00000000..56d1c6af --- /dev/null +++ b/pkg/app/error_page.go @@ -0,0 +1,97 @@ +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. + 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 + + // 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, messages ...interface{}) { + 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: e.getMessage(status, appError, messages...), + 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) + } +} + +// 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, "", 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 new file mode 100644 index 00000000..5c4f78fa --- /dev/null +++ b/pkg/app/error_page_test.go @@ -0,0 +1,101 @@ +package app + +import ( + "errors" + "html/template" + "io/ioutil" + "net/http/httptest" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +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() { + 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 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")) + }) + }) + + 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 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")) + }) + }) + }) +}) 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 }}