diff --git a/docs/docs/CHANGELOG.md b/docs/docs/CHANGELOG.md index 1516df913..54d343ad5 100644 --- a/docs/docs/CHANGELOG.md +++ b/docs/docs/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed an issue where requests handled by forward-auth would not be redirected back to the underlying route after successful authentication and authorization. [GH-363] +- Fixed an issue where requests handled by forward-auth would add an extraneous query-param following sign-in causing issues in some configurations. [GH-366] ## v0.4.0 diff --git a/docs/docs/reference/reference.md b/docs/docs/reference/reference.md index 7ce762bdf..6d2ee86f5 100644 --- a/docs/docs/reference/reference.md +++ b/docs/docs/reference/reference.md @@ -303,7 +303,7 @@ Forward authentication creates an endpoint that can be used with third-party pro #### NGINX Ingress -Some reverse-proxies, such as nginx split access control flow into two parts: verification and sign-in redirection. Notice the additional the additional `?no_redirect=true` query param in `auth-rul` which tells Pomerium to return a `401` instead of redirecting and starting the sign-in process. +Some reverse-proxies, such as nginx split access control flow into two parts: verification and sign-in redirection. Notice the additional path `/verify` used for `auth-url` indicating to Pomerium that it should return a `401` instead of redirecting and starting the sign-in process. ```yaml apiVersion: extensions/v1beta1 @@ -313,8 +313,8 @@ metadata: annotations: kubernetes.io/ingress.class: "nginx" certmanager.k8s.io/issuer: "letsencrypt-prod" - nginx.ingress.kubernetes.io/auth-url: https://fwdauth.corp.example.com/.pomerium/verify?uri=$scheme://$host$request_uri&x-pomerium-no-auth-redirect=true - nginx.ingress.kubernetes.io/auth-signin: "https://fwdauth.corp.example.com/.pomerium/verify?uri=$scheme://$host$request_uri" + nginx.ingress.kubernetes.io/auth-url: https://fwdauth.corp.example.com?uri=$scheme://$host$request_uri + nginx.ingress.kubernetes.io/auth-signin: "https://fwdauth.corp.example.com/verify?uri=$scheme://$host$request_uri" spec: tls: - hosts: @@ -356,7 +356,7 @@ services: - "traefik.http.routers.httpbin.rule=Host(`httpbin.corp.example.com`)" # Create a middleware named `foo-add-prefix` - "traefik.http.middlewares.test-auth.forwardauth.authResponseHeaders=X-Pomerium-Authenticated-User-Email,x-pomerium-authenticated-user-id,x-pomerium-authenticated-user-groups,x-pomerium-jwt-assertion" - - "traefik.http.middlewares.test-auth.forwardauth.address=http://fwdauth.corp.example.com/.pomerium/verify?uri=https://httpbin.corp.example.com" + - "traefik.http.middlewares.test-auth.forwardauth.address=http://fwdauth.corp.example.com/?uri=https://httpbin.corp.example.com" - "traefik.http.routers.httpbin.middlewares=test-auth@docker" ``` diff --git a/docs/docs/upgrading.md b/docs/docs/upgrading.md index b3f0e3076..8c293894e 100644 --- a/docs/docs/upgrading.md +++ b/docs/docs/upgrading.md @@ -13,13 +13,15 @@ description: >- Previously, routes were verified by taking the downstream applications hostname in the form of a path `(e.g. ${fwdauth}/.pomerium/verify/httpbin.some.example`) variable. The new method for verifying a route using forward authentication is to pass the entire requested url in the form of a query string `(e.g. ${fwdauth}/.pomerium/verify?url=https://httpbin.some.example)` where the routed domain is the value of the `uri` key. +Note that the verification URL is no longer nested under the `.pomerium` endpoint. + For example, in nginx this would look like: ```diff - nginx.ingress.kubernetes.io/auth-url: https://fwdauth.corp.example.com/.pomerium/verify/httpbin.corp.example.com?no_redirect=true - nginx.ingress.kubernetes.io/auth-signin: https://fwdauth.corp.example.com/.pomerium/verify/httpbin.corp.example.com -+ nginx.ingress.kubernetes.io/auth-url: https://fwdauth.corp.example.com/.pomerium/verify?uri=$scheme://$host$request_uri&x-pomerium-no-auth-redirect=true -+ nginx.ingress.kubernetes.io/auth-signin: https://fwdauth.corp.example.com/.pomerium/verify?uri=$scheme://$host$request_uri ++ nginx.ingress.kubernetes.io/auth-url: https://fwdauth.corp.example.com/verify?uri=$scheme://$host$request_uri ++ nginx.ingress.kubernetes.io/auth-signin: https://fwdauth.corp.example.com?uri=$scheme://$host$request_uri ``` diff --git a/proxy/handlers.go b/proxy/handlers.go index 2e6443fd1..d09a997a4 100644 --- a/proxy/handlers.go +++ b/proxy/handlers.go @@ -16,8 +16,9 @@ import ( "github.com/pomerium/pomerium/internal/urlutil" ) -// registerHelperHandlers returns the proxy service's ServeMux -func (p *Proxy) registerHelperHandlers(r *mux.Router) *mux.Router { +// registerDashboardHandlers returns the proxy service's ServeMux +func (p *Proxy) registerDashboardHandlers(r *mux.Router) *mux.Router { + // dashboard subrouter h := r.PathPrefix(dashboardURL).Subrouter() // 1. Retrieve the user session and add it to the request context h.Use(sessions.RetrieveSession(p.sessionStore)) @@ -35,7 +36,6 @@ func (p *Proxy) registerHelperHandlers(r *mux.Router) *mux.Router { h.HandleFunc("/impersonate", p.Impersonate).Methods(http.MethodPost) h.HandleFunc("/sign_out", p.SignOut).Methods(http.MethodGet, http.MethodPost) h.HandleFunc("/refresh", p.ForceRefresh).Methods(http.MethodPost) - h.HandleFunc("/verify", p.Verify).Queries("uri", "{uri}").Methods(http.MethodGet) return r } @@ -152,49 +152,87 @@ func (p *Proxy) Impersonate(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, dashboardURL, http.StatusFound) } -// Verify checks a user's credentials for an arbitrary host. If the user is -// properly authenticated and is authorized to access the supplied host, -// a 200 http status code is returned. If the user is not authenticated, they +func (p *Proxy) registerFwdAuthHandlers() http.Handler { + r := httputil.NewRouter() + r.StrictSlash(true) + r.Use(sessions.RetrieveSession(p.sessionStore)) + r.HandleFunc("/", p.VerifyAndSignin).Queries("uri", "{uri}").Methods(http.MethodGet) + r.HandleFunc("/verify", p.VerifyOnly).Queries("uri", "{uri}").Methods(http.MethodGet) + return r +} + +// VerifyAndSignin checks a user's credentials for an arbitrary host. If the user +// is properly authenticated and is authorized to access the supplied host, +// a `200` http status code is returned. If the user is not authenticated, they // will be redirected to the authenticate service to sign in with their identity -// provider. If the user is unauthorized, they will be given a 403 http status. -func (p *Proxy) Verify(w http.ResponseWriter, r *http.Request) { +// provider. If the user is unauthorized, a `401` error is returned. +func (p *Proxy) VerifyAndSignin(w http.ResponseWriter, r *http.Request) { uri, err := urlutil.ParseAndValidateURL(r.FormValue("uri")) if err != nil || uri.String() == "" { httputil.ErrorResponse(w, r, httputil.Error("bad verification uri given", http.StatusBadRequest, nil)) return } + if err := p.authenticate(w, r); err != nil { + uri := urlutil.SignedRedirectURL(p.SharedKey, p.authenticateSigninURL, urlutil.GetAbsoluteURL(r)) + http.Redirect(w, r, uri.String(), http.StatusFound) + } + if err := p.authorize(r, uri); err != nil { + httputil.ErrorResponse(w, r, httputil.Error("", http.StatusUnauthorized, err)) + return + } + // check the queryparams to see if this check immediately followed + // authentication. If so, redirect back to the originally requested hostname. + if isCallback := r.URL.Query().Get(callbackQueryParam); isCallback == "true" { + q := uri.Query() + q.Del(callbackQueryParam) + uri.RawQuery = q.Encode() + http.Redirect(w, r, uri.String(), http.StatusFound) + return + } + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.WriteHeader(http.StatusOK) + +} + +// VerifyOnly checks a user's credentials for an arbitrary host. If the user +// is properly authenticated and is authorized to access the supplied host, +// a `200` http status code is returned otherwise a `401` error is returned. +func (p *Proxy) VerifyOnly(w http.ResponseWriter, r *http.Request) { + uri, err := urlutil.ParseAndValidateURL(r.FormValue("uri")) + if err != nil || uri.String() == "" { + httputil.ErrorResponse(w, r, httputil.Error("bad verification uri given", http.StatusBadRequest, nil)) + return + } + if err := p.authenticate(w, r); err != nil { + httputil.ErrorResponse(w, r, httputil.Error("", http.StatusUnauthorized, err)) + return + } + if err := p.authorize(r, uri); err != nil { + httputil.ErrorResponse(w, r, httputil.Error("", http.StatusUnauthorized, err)) + return + } + + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.WriteHeader(http.StatusOK) +} + +func (p *Proxy) authorize(r *http.Request, uri *url.URL) error { // attempt to retrieve the user session from the request context, validity // of the identity session is asserted by the middleware chain s, err := sessions.FromContext(r.Context()) if err != nil { - httputil.ErrorResponse(w, r, err) - return + return err } // query the authorization service to see if the session's user has // the appropriate authorization to access the given hostname authorized, err := p.AuthorizeClient.Authorize(r.Context(), uri.Host, s) if err != nil { - httputil.ErrorResponse(w, r, err) - return + return err } else if !authorized { - errMsg := fmt.Sprintf("%s is not authorized for this route", s.RequestEmail()) - httputil.ErrorResponse(w, r, httputil.Error(errMsg, http.StatusForbidden, nil)) - return + return fmt.Errorf("%s is not authorized for %s", s.RequestEmail(), uri.String()) } - // check the queryparams to see if this check immediately followed - // authentication. If so, redirect back to the originally requested hostname. - if isCallback := r.URL.Query().Get("pomerium-auth-callback"); isCallback == "true" { - http.Redirect(w, r, uri.String(), http.StatusFound) - return - } - - // User is authenticated and authorized for the given host. - w.Header().Set("Content-Type", "text/plain; charset=utf-8") - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set(HeaderUserID, s.User) - w.Header().Set(HeaderEmail, s.RequestEmail()) - w.Header().Set(HeaderGroups, s.RequestGroups()) - w.WriteHeader(http.StatusOK) - fmt.Fprintf(w, "%s is authorized for %s.", s.Email, uri.String()) + return nil } diff --git a/proxy/handlers_test.go b/proxy/handlers_test.go index 40ea6f45e..83e824d88 100644 --- a/proxy/handlers_test.go +++ b/proxy/handlers_test.go @@ -11,8 +11,7 @@ import ( "testing" "time" - "github.com/gorilla/mux" - + "github.com/google/go-cmp/cmp" "github.com/pomerium/pomerium/internal/config" "github.com/pomerium/pomerium/internal/cryptutil" "github.com/pomerium/pomerium/internal/sessions" @@ -56,27 +55,6 @@ func TestProxy_Signout(t *testing.T) { } } -func TestProxy_authenticate(t *testing.T) { - proxy, err := New(testOptions(t)) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/oauth-start", nil) - - rr := httptest.NewRecorder() - proxy.reqNeedsAuthentication(rr, req) - // expect oauth redirect - if status := rr.Code; status != http.StatusFound { - t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusFound) - } - // expected url - expected := `Found.\n\n"}, + {"not authorized", opts, nil, http.MethodGet, "", "/", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(10 * time.Second)}}, clients.MockAuthorize{AuthorizeResponse: false}, http.StatusUnauthorized, "{\"error\":\"Unauthorized\"}\n"}, + {"not authorized verify endpoint", opts, nil, http.MethodGet, "", "/verify", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(10 * time.Second)}}, clients.MockAuthorize{AuthorizeResponse: false}, http.StatusUnauthorized, "{\"error\":\"Unauthorized\"}\n"}, + {"not authorized expired, redirect to auth", opts, nil, http.MethodGet, "", "/", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(-10 * time.Second)}}, clients.MockAuthorize{AuthorizeResponse: false}, http.StatusFound, ""}, + {"not authorized expired, don't redirect!", opts, nil, http.MethodGet, "", "/verify", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(-10 * time.Second)}}, clients.MockAuthorize{AuthorizeResponse: false}, http.StatusUnauthorized, "{\"error\":\"Unauthorized\"}\n"}, + {"not authorized because of error", opts, nil, http.MethodGet, "", "/", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(10 * time.Second)}}, clients.MockAuthorize{AuthorizeError: errors.New("authz error")}, http.StatusUnauthorized, "{\"error\":\"Unauthorized\"}\n"}, + {"not authorized expired, do not redirect to auth", opts, nil, http.MethodGet, "", "/verify", "https://some.domain.example", &cryptutil.MockEncoder{}, &sessions.MockSessionStore{Session: &sessions.State{Email: "user@test.example", RefreshDeadline: time.Now().Add(-10 * time.Second)}}, clients.MockAuthorize{AuthorizeResponse: false}, http.StatusUnauthorized, "{\"error\":\"Unauthorized\"}\n"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -325,12 +308,14 @@ func TestProxy_VerifyWithMiddleware(t *testing.T) { p.UpdateOptions(tt.options) uri := &url.URL{Path: tt.path} queryString := uri.Query() + queryString.Set("donstrip", "ok") if tt.qp != "" { queryString.Set(tt.qp, "true") } if tt.verifyURI != "" { queryString.Set("uri", tt.verifyURI) } + uri.RawQuery = queryString.Encode() r := httptest.NewRequest(tt.method, uri.String(), nil) @@ -345,14 +330,19 @@ func TestProxy_VerifyWithMiddleware(t *testing.T) { r.Header.Set("Accept", "application/json") w := httptest.NewRecorder() - router := mux.NewRouter() - router.StrictSlash(true) - router = p.registerHelperHandlers(router) + router := p.registerFwdAuthHandlers() router.ServeHTTP(w, r) if status := w.Code; status != tt.wantStatus { t.Errorf("status code: got %v want %v", status, tt.wantStatus) t.Errorf("\n%+v", w.Body.String()) } + + if tt.wantBody != "" { + body := w.Body.String() + if diff := cmp.Diff(body, tt.wantBody); diff != "" { + t.Errorf("wrong body\n%s", diff) + } + } }) } } diff --git a/proxy/middleware.go b/proxy/middleware.go index 9b05af131..44d89f2cc 100644 --- a/proxy/middleware.go +++ b/proxy/middleware.go @@ -22,42 +22,52 @@ const ( HeaderEmail = "x-pomerium-authenticated-user-email" // HeaderGroups is the header key containing the user's groups. HeaderGroups = "x-pomerium-authenticated-user-groups" - - // HeaderNoAuthRedirect is the header / query param key used to disable - // redirecting unauthenticated request by default but instead return a 401. - HeaderNoAuthRedirect = "x-pomerium-no-auth-redirect" ) // AuthenticateSession is middleware to enforce a valid authentication // session state is retrieved from the users's request context. func (p *Proxy) AuthenticateSession(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ctx, span := trace.StartSpan(r.Context(), "middleware.AuthenticateSession") + ctx, span := trace.StartSpan(r.Context(), "proxy.AuthenticateSession") defer span.End() - s, err := sessions.FromContext(r.Context()) - if err != nil || s == nil { - log.Debug().Msg("proxy: re-authenticating due to session state error") - p.reqNeedsAuthentication(w, r) + if err := p.authenticate(w, r); err != nil { + log.FromRequest(r).Debug().Err(err).Msg("proxy: authenticate session") + uri := urlutil.SignedRedirectURL(p.SharedKey, p.authenticateSigninURL, urlutil.GetAbsoluteURL(r)) + http.Redirect(w, r, uri.String(), http.StatusFound) return } - if err := s.Valid(); err != nil { - log.Debug().Str("cause", err.Error()).Msg("proxy: re-authenticating due to invalid session") - p.reqNeedsAuthentication(w, r) - return - } - // add pomerium's headers to the downstream request - r.Header.Set(HeaderUserID, s.User) - r.Header.Set(HeaderEmail, s.RequestEmail()) - r.Header.Set(HeaderGroups, s.RequestGroups()) + next.ServeHTTP(w, r.WithContext(ctx)) }) } +func (p *Proxy) authenticate(w http.ResponseWriter, r *http.Request) error { + s, err := sessions.FromContext(r.Context()) + if err != nil { + return err + } + if s == nil { + return fmt.Errorf("empty session state") + } + if err := s.Valid(); err != nil { + return err + } + // add pomerium's headers to the downstream request + r.Header.Set(HeaderUserID, s.User) + r.Header.Set(HeaderEmail, s.RequestEmail()) + r.Header.Set(HeaderGroups, s.RequestGroups()) + // and upstream + w.Header().Set(HeaderUserID, s.User) + w.Header().Set(HeaderEmail, s.RequestEmail()) + w.Header().Set(HeaderGroups, s.RequestGroups()) + return nil +} + // AuthorizeSession is middleware to enforce a user is authorized for a request // session state is retrieved from the users's request context. func (p *Proxy) AuthorizeSession(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ctx, span := trace.StartSpan(r.Context(), "middleware.AuthorizeSession") + ctx, span := trace.StartSpan(r.Context(), "proxy.AuthorizeSession") defer span.End() s, err := sessions.FromContext(r.Context()) if err != nil || s == nil { @@ -82,7 +92,7 @@ func (p *Proxy) AuthorizeSession(next http.Handler) http.Handler { func (p *Proxy) SignRequest(signer cryptutil.JWTSigner) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ctx, span := trace.StartSpan(r.Context(), "middleware.SignRequest") + ctx, span := trace.StartSpan(r.Context(), "proxy.SignRequest") defer span.End() s, err := sessions.FromContext(r.Context()) if err != nil { @@ -91,7 +101,7 @@ func (p *Proxy) SignRequest(signer cryptutil.JWTSigner) func(next http.Handler) } jwt, err := signer.SignJWT(s.User, s.Email, strings.Join(s.Groups, ",")) if err != nil { - log.Warn().Err(err).Msg("proxy: failed signing jwt") + log.FromRequest(r).Warn().Err(err).Msg("proxy: failed signing jwt") } else { r.Header.Set(HeaderJWT, jwt) w.Header().Set(HeaderJWT, jwt) @@ -101,26 +111,11 @@ func (p *Proxy) SignRequest(signer cryptutil.JWTSigner) func(next http.Handler) } } -// reqNeedsAuthentication begins the authenticate flow, encrypting the -// redirect url in a request to the provider's sign in endpoint. -func (p *Proxy) reqNeedsAuthentication(w http.ResponseWriter, r *http.Request) { - // some proxies like nginx won't follow redirects, and treat any - // non 2xx or 4xx status as an internal service error. - // https://nginx.org/en/docs/http/ngx_http_auth_request_module.html - redirectHeader := r.Header.Get(HeaderNoAuthRedirect) - if _, ok := r.URL.Query()[HeaderNoAuthRedirect]; ok || redirectHeader == "true" { - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) - } - r.Header.Get(HeaderNoAuthRedirect) - uri := urlutil.SignedRedirectURL(p.SharedKey, p.authenticateSigninURL, urlutil.GetAbsoluteURL(r)) - http.Redirect(w, r, uri.String(), http.StatusFound) -} - // SetResponseHeaders sets a map of response headers. func SetResponseHeaders(headers map[string]string) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ctx, span := trace.StartSpan(r.Context(), "middleware.SetResponseHeaders") + ctx, span := trace.StartSpan(r.Context(), "proxy.SetResponseHeaders") defer span.End() for key, val := range headers { r.Header.Set(key, val) diff --git a/proxy/proxy.go b/proxy/proxy.go index aa795c0c1..9d0b7387d 100755 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -30,6 +30,8 @@ const ( signinURL = "/.pomerium/sign_in" // signoutURL is the path to authenticate's sign out endpoint signoutURL = "/.pomerium/sign_out" + + callbackQueryParam = "pomerium-auth-callback" ) // ValidateOptions checks that proper configuration settings are set to create @@ -179,11 +181,13 @@ func (p *Proxy) UpdatePolicies(opts *config.Options) error { r.SkipClean(true) r.StrictSlash(true) r.HandleFunc("/robots.txt", p.RobotsTxt).Methods(http.MethodGet) - r = p.registerHelperHandlers(r) + // dashboard handlers are registered to all routes + r = p.registerDashboardHandlers(r) if opts.ForwardAuthURL != nil { - // create a route to handle forward auth requests - r.Host(opts.ForwardAuthURL.Host).Subrouter().PathPrefix("/") + // if a forward auth endpoint is set, register its handlers + h := r.Host(opts.ForwardAuthURL.Host).Subrouter() + h.PathPrefix("/").Handler(p.registerFwdAuthHandlers()) } for _, policy := range opts.Policies {