From ebb5711c328f04a0f6a2032864e76ff9656b3682 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 27 Jan 2022 18:14:02 +0100 Subject: [PATCH] providers/proxy: add support for X-Original-URI in nginx, better handle missing headers and report errors to authentik Signed-off-by: Jens Langhammer --- .../proxyv2/application/application.go | 2 + .../proxyv2/application/mode_common.go | 39 ++++++++++++++----- .../proxyv2/application/mode_forward.go | 27 ++++++++++++- .../application/mode_forward_nginx_test.go | 22 ++++++++--- .../application/mode_forward_traefik_test.go | 19 +++------ internal/outpost/proxyv2/application/utils.go | 10 +++++ internal/utils/web/middleware.go | 18 ++++----- 7 files changed, 98 insertions(+), 39 deletions(-) diff --git a/internal/outpost/proxyv2/application/application.go b/internal/outpost/proxyv2/application/application.go index 31224c3bd..6b9c9f02f 100644 --- a/internal/outpost/proxyv2/application/application.go +++ b/internal/outpost/proxyv2/application/application.go @@ -46,6 +46,7 @@ type Application struct { log *log.Entry mux *mux.Router + ak *ak.APIController errorTemplates *template.Template } @@ -93,6 +94,7 @@ func NewApplication(p api.ProxyOutpostConfig, c *http.Client, cs *ak.CryptoStore httpClient: c, mux: mux, errorTemplates: templates.GetTemplates(), + ak: ak, } a.sessions = a.getStore(p) mux.Use(web.NewLoggingHandler(muxLogger, func(l *log.Entry, r *http.Request) *log.Entry { diff --git a/internal/outpost/proxyv2/application/mode_common.go b/internal/outpost/proxyv2/application/mode_common.go index 6496808d9..8db273f46 100644 --- a/internal/outpost/proxyv2/application/mode_common.go +++ b/internal/outpost/proxyv2/application/mode_common.go @@ -1,6 +1,7 @@ package application import ( + "context" "encoding/base64" "errors" "fmt" @@ -59,7 +60,7 @@ func (a *Application) addHeaders(headers http.Header, c *Claims) { } // getTraefikForwardUrl See https://doc.traefik.io/traefik/middlewares/forwardauth/ -func (a *Application) getTraefikForwardUrl(r *http.Request) *url.URL { +func (a *Application) getTraefikForwardUrl(r *http.Request) (*url.URL, error) { u, err := url.Parse(fmt.Sprintf( "%s://%s%s", r.Header.Get("X-Forwarded-Proto"), @@ -67,27 +68,47 @@ func (a *Application) getTraefikForwardUrl(r *http.Request) *url.URL { r.Header.Get("X-Forwarded-Uri"), )) if err != nil { - a.log.WithError(err).Warning("Failed to parse URL from traefik") - return r.URL + return nil, err } a.log.WithField("url", u.String()).Trace("traefik forwarded url") - return u + return u, nil } // getNginxForwardUrl See https://github.com/kubernetes/ingress-nginx/blob/main/rootfs/etc/nginx/template/nginx.tmpl -func (a *Application) getNginxForwardUrl(r *http.Request) *url.URL { +func (a *Application) getNginxForwardUrl(r *http.Request) (*url.URL, error) { + ou := r.Header.Get("X-Original-URI") + if ou != "" { + u, _ := url.Parse(r.URL.String()) + u.Path = ou + a.log.WithField("url", u.String()).Info("building forward URL from X-Original-URI") + return u, nil + } h := r.Header.Get("X-Original-URL") if len(h) < 1 { - a.log.WithError(errors.New("blank URL")).Warning("blank URL") - return r.URL + return nil, errors.New("no forward URL found") } u, err := url.Parse(h) if err != nil { a.log.WithError(err).Warning("failed to parse URL from nginx") - return r.URL + return nil, err } a.log.WithField("url", u.String()).Trace("nginx forwarded url") - return u + return u, nil +} + +func (a *Application) ReportMisconfiguration(r *http.Request, msg string, fields map[string]interface{}) { + fields["message"] = msg + a.log.WithFields(fields).Error("Reporting configuration error") + req := api.EventRequest{ + Action: api.EVENTACTIONS_CONFIGURATION_ERROR, + App: "authentik.providers.proxy", // must match python apps.py name + ClientIp: *api.NewNullableString(api.PtrString(r.RemoteAddr)), + Context: &fields, + } + _, _, err := a.ak.Client.EventsApi.EventsEventsCreate(context.Background()).EventRequest(req).Execute() + if err != nil { + a.log.WithError(err).Warning("failed to report configuration error") + } } func (a *Application) IsAllowlisted(u *url.URL) bool { diff --git a/internal/outpost/proxyv2/application/mode_forward.go b/internal/outpost/proxyv2/application/mode_forward.go index 57558a30a..35460dd9c 100644 --- a/internal/outpost/proxyv2/application/mode_forward.go +++ b/internal/outpost/proxyv2/application/mode_forward.go @@ -26,7 +26,19 @@ func (a *Application) configureForward() error { func (a *Application) forwardHandleTraefik(rw http.ResponseWriter, r *http.Request) { a.log.WithField("header", r.Header).Trace("tracing headers for debug") - fwd := a.getTraefikForwardUrl(r) + // First check if we've got everything we need + fwd, err := a.getTraefikForwardUrl(r) + if err != nil { + a.ReportMisconfiguration(r, fmt.Sprintf("Outpost %s (Provider %s) failed to detect a forward URL from Traefik", a.outpostName, a.proxyConfig.Name), map[string]interface{}{ + "provider": a.proxyConfig.Name, + "outpost": a.outpostName, + "url": r.URL.String(), + "headers": cleanseHeaders(r.Header), + }) + http.Error(rw, "configuration error", http.StatusInternalServerError) + return + } + claims, err := a.getClaims(r) if claims != nil && err == nil { a.addHeaders(rw.Header(), claims) @@ -75,7 +87,18 @@ func (a *Application) forwardHandleTraefik(rw http.ResponseWriter, r *http.Reque func (a *Application) forwardHandleNginx(rw http.ResponseWriter, r *http.Request) { a.log.WithField("header", r.Header).Trace("tracing headers for debug") - fwd := a.getNginxForwardUrl(r) + fwd, err := a.getNginxForwardUrl(r) + if err != nil { + a.ReportMisconfiguration(r, fmt.Sprintf("Outpost %s (Provider %s) failed to detect a forward URL from nginx", a.outpostName, a.proxyConfig.Name), map[string]interface{}{ + "provider": a.proxyConfig.Name, + "outpost": a.outpostName, + "url": r.URL.String(), + "headers": cleanseHeaders(r.Header), + }) + http.Error(rw, "configuration error", http.StatusInternalServerError) + return + } + claims, err := a.getClaims(r) if claims != nil && err == nil { a.addHeaders(rw.Header(), claims) diff --git a/internal/outpost/proxyv2/application/mode_forward_nginx_test.go b/internal/outpost/proxyv2/application/mode_forward_nginx_test.go index 3842a49bc..815bd53a2 100644 --- a/internal/outpost/proxyv2/application/mode_forward_nginx_test.go +++ b/internal/outpost/proxyv2/application/mode_forward_nginx_test.go @@ -17,7 +17,7 @@ func TestForwardHandleNginx_Single_Blank(t *testing.T) { rr := httptest.NewRecorder() a.forwardHandleNginx(rr, req) - assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.Equal(t, http.StatusInternalServerError, rr.Code) } func TestForwardHandleNginx_Single_Skip(t *testing.T) { @@ -45,9 +45,24 @@ func TestForwardHandleNginx_Single_Headers(t *testing.T) { assert.Equal(t, "http://test.goauthentik.io/app", s.Values[constants.SessionRedirect]) } +func TestForwardHandleNginx_Single_URI(t *testing.T) { + a := newTestApplication() + req, _ := http.NewRequest("GET", "https://foo.bar/akprox/auth/nginx", nil) + req.Header.Set("X-Original-URI", "/app") + + rr := httptest.NewRecorder() + a.forwardHandleNginx(rr, req) + + assert.Equal(t, rr.Code, http.StatusUnauthorized) + + s, _ := a.sessions.Get(req, constants.SeesionName) + assert.Equal(t, "https://foo.bar/app", s.Values[constants.SessionRedirect]) +} + func TestForwardHandleNginx_Single_Claims(t *testing.T) { a := newTestApplication() req, _ := http.NewRequest("GET", "/akprox/auth/nginx", nil) + req.Header.Set("X-Original-URI", "/") rr := httptest.NewRecorder() a.forwardHandleNginx(rr, req) @@ -98,10 +113,7 @@ func TestForwardHandleNginx_Domain_Blank(t *testing.T) { rr := httptest.NewRecorder() a.forwardHandleNginx(rr, req) - assert.Equal(t, http.StatusUnauthorized, rr.Code) - - s, _ := a.sessions.Get(req, constants.SeesionName) - assert.Equal(t, "/akprox/auth/nginx", s.Values[constants.SessionRedirect]) + assert.Equal(t, http.StatusInternalServerError, rr.Code) } func TestForwardHandleNginx_Domain_Header(t *testing.T) { diff --git a/internal/outpost/proxyv2/application/mode_forward_traefik_test.go b/internal/outpost/proxyv2/application/mode_forward_traefik_test.go index 9e06c8fb1..fbca244be 100644 --- a/internal/outpost/proxyv2/application/mode_forward_traefik_test.go +++ b/internal/outpost/proxyv2/application/mode_forward_traefik_test.go @@ -17,13 +17,7 @@ func TestForwardHandleTraefik_Single_Blank(t *testing.T) { rr := httptest.NewRecorder() a.forwardHandleTraefik(rr, req) - assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) - loc, _ := rr.Result().Location() - assert.Equal(t, "/akprox/start", loc.String()) - - s, _ := a.sessions.Get(req, constants.SeesionName) - // Since we're not setting the traefik specific headers, expect it to redirect to the auth URL - assert.Equal(t, "/akprox/auth/traefik", s.Values[constants.SessionRedirect]) + assert.Equal(t, http.StatusInternalServerError, rr.Code) } func TestForwardHandleTraefik_Single_Skip(t *testing.T) { @@ -60,6 +54,9 @@ func TestForwardHandleTraefik_Single_Headers(t *testing.T) { func TestForwardHandleTraefik_Single_Claims(t *testing.T) { a := newTestApplication() req, _ := http.NewRequest("GET", "/akprox/auth/traefik", nil) + req.Header.Set("X-Forwarded-Proto", "http") + req.Header.Set("X-Forwarded-Host", "test.goauthentik.io") + req.Header.Set("X-Forwarded-Uri", "/app") rr := httptest.NewRecorder() a.forwardHandleTraefik(rr, req) @@ -110,13 +107,7 @@ func TestForwardHandleTraefik_Domain_Blank(t *testing.T) { rr := httptest.NewRecorder() a.forwardHandleTraefik(rr, req) - assert.Equal(t, http.StatusTemporaryRedirect, rr.Code) - loc, _ := rr.Result().Location() - assert.Equal(t, "/akprox/start", loc.String()) - - s, _ := a.sessions.Get(req, constants.SeesionName) - // Since we're not setting the traefik specific headers, expect it to redirect to the auth URL - assert.Equal(t, "/akprox/auth/traefik", s.Values[constants.SessionRedirect]) + assert.Equal(t, http.StatusInternalServerError, rr.Code) } func TestForwardHandleTraefik_Domain_Header(t *testing.T) { diff --git a/internal/outpost/proxyv2/application/utils.go b/internal/outpost/proxyv2/application/utils.go index eacbad6e0..3fb5b44e3 100644 --- a/internal/outpost/proxyv2/application/utils.go +++ b/internal/outpost/proxyv2/application/utils.go @@ -87,3 +87,13 @@ func contains(s []string, e string) bool { } return false } + +func cleanseHeaders(headers http.Header) map[string]string { + h := make(map[string]string) + for hk, hv := range headers { + if len(hv) > 0 { + h[hk] = hv[0] + } + } + return h +} diff --git a/internal/utils/web/middleware.go b/internal/utils/web/middleware.go index 2b66e0d05..8688e6c73 100644 --- a/internal/utils/web/middleware.go +++ b/internal/utils/web/middleware.go @@ -99,14 +99,14 @@ func (h loggingHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { h.handler.ServeHTTP(responseLogger, req) duration := float64(time.Since(t)) / float64(time.Millisecond) h.afterHandler(h.logger.WithFields(log.Fields{ - "remote": req.RemoteAddr, - "host": GetHost(req), - "request_protocol": req.Proto, - "runtime": fmt.Sprintf("%0.3f", duration), - "method": req.Method, - "size": responseLogger.Size(), - "status": responseLogger.Status(), - "upstream": responseLogger.upstream, - "request_useragent": req.UserAgent(), + "remote": req.RemoteAddr, + "host": GetHost(req), + "runtime": fmt.Sprintf("%0.3f", duration), + "method": req.Method, + "scheme": req.URL.Scheme, + "size": responseLogger.Size(), + "status": responseLogger.Status(), + "upstream": responseLogger.upstream, + "user_agent": req.UserAgent(), }), req).Info(url.RequestURI()) }