From 01a6b169469465742dfe04f7d9a639c66348a93b Mon Sep 17 00:00:00 2001 From: jrester Date: Wed, 28 Dec 2022 12:34:20 +0100 Subject: [PATCH 1/4] fix: HEAD is now handled correctly by decision API --- api/decision.go | 23 ++++++++++++++++------- api/decision_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/api/decision.go b/api/decision.go index 72cb9b50ac..9e77192d5a 100644 --- a/api/decision.go +++ b/api/decision.go @@ -5,6 +5,7 @@ package api import ( "net/http" + "net/url" "strings" "github.com/ory/oathkeeper/pipeline/authn" @@ -41,13 +42,21 @@ func NewJudgeHandler(r decisionHandlerRegistry) *DecisionHandler { func (h *DecisionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { if len(r.URL.Path) >= len(DecisionPath) && r.URL.Path[:len(DecisionPath)] == DecisionPath { - r.Method = x.OrDefaultString(r.Header.Get(xForwardedMethod), r.Method) - r.URL.Scheme = x.OrDefaultString(r.Header.Get(xForwardedProto), - x.IfThenElseString(r.TLS != nil, "https", "http")) - r.URL.Host = x.OrDefaultString(r.Header.Get(xForwardedHost), r.Host) - r.URL.Path = x.OrDefaultString(strings.SplitN(r.Header.Get(xForwardedUri), "?", 2)[0], r.URL.Path[len(DecisionPath):]) - - h.decisions(w, r) + // Copy the request information, instead of modifing the incoming request directly. + // This is nevessary because the middleware would otherwise use e.g. the method from "X-Forwarded-Method" for the response + // although the original request had another method, which leads to problem with the HEAD method. + // For more information see: https://github.com/thomseddon/traefik-forward-auth/issues/156 + forwardedReq := &http.Request{ + Method: x.OrDefaultString(r.Header.Get(xForwardedMethod), r.Method), + URL: &url.URL{ + Scheme: x.OrDefaultString(r.Header.Get(xForwardedProto), + x.IfThenElseString(r.TLS != nil, "https", "http")), + Host: x.OrDefaultString(r.Header.Get(xForwardedHost), r.Host), + Path: x.OrDefaultString(strings.SplitN(r.Header.Get(xForwardedUri), "?", 2)[0], r.URL.Path[len(DecisionPath):]), + }, + Header: r.Header, + } + h.decisions(w, forwardedReq) } else { next(w, r) } diff --git a/api/decision_test.go b/api/decision_test.go index 0609d74b63..054e5011e6 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -14,6 +14,7 @@ import ( "net/url" "strconv" "testing" + "time" "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" @@ -299,6 +300,29 @@ func TestDecisionAPI(t *testing.T) { code: http.StatusOK, authz: "", }, + { + d: "HEAD request should not result in an read timeout", + url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234", + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"HEAD"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"HEAD"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + transform: func(r *http.Request) { + r.Header.Add("X-Forwarded-Method", "HEAD") + }, + code: http.StatusOK, + authz: "", + }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { testFunc := func(strategy configuration.MatchingStrategy) { @@ -309,7 +333,10 @@ func TestDecisionAPI(t *testing.T) { tc.transform(req) } - res, err := http.DefaultClient.Do(req) + httpClient := http.Client{ + Timeout: 2 * time.Second, + } + res, err := httpClient.Do(req) require.NoError(t, err) entireBody, err := io.ReadAll(res.Body) From 1d1c6f0d94f3cd3fa2324221e9a0f7057ff6bdc2 Mon Sep 17 00:00:00 2001 From: jrester Date: Sat, 18 Mar 2023 16:31:54 +0100 Subject: [PATCH 2/4] fix: clone request --- api/decision.go | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/api/decision.go b/api/decision.go index 52bdb9fe48..7c3b55d7f4 100644 --- a/api/decision.go +++ b/api/decision.go @@ -5,7 +5,6 @@ package api import ( "net/http" - "net/url" "strings" "github.com/ory/oathkeeper/pipeline/authn" @@ -42,20 +41,15 @@ func NewJudgeHandler(r decisionHandlerRegistry) *DecisionHandler { func (h *DecisionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { if len(r.URL.Path) >= len(DecisionPath) && r.URL.Path[:len(DecisionPath)] == DecisionPath { - // Copy the request information, instead of modifing the incoming request directly. - // This is nevessary because the middleware would otherwise use e.g. the method from "X-Forwarded-Method" for the response - // although the original request had another method, which leads to problem with the HEAD method. + // Clone the request, instead of modifing the incoming request directly. + // This is nevessary because the middleware would otherwise use the method from "X-Forwarded-Method" for the response + // although the original request had another method, which leads to problems with the HEAD method. // For more information see: https://github.com/thomseddon/traefik-forward-auth/issues/156 - forwardedReq := &http.Request{ - Method: x.OrDefaultString(r.Header.Get(xForwardedMethod), r.Method), - URL: &url.URL{ - Scheme: x.OrDefaultString(r.Header.Get(xForwardedProto), - x.IfThenElseString(r.TLS != nil, "https", "http")), - Host: x.OrDefaultString(r.Header.Get(xForwardedHost), r.Host), - Path: x.OrDefaultString(strings.SplitN(r.Header.Get(xForwardedUri), "?", 2)[0], r.URL.Path[len(DecisionPath):]), - }, - Header: r.Header, - } + forwardedReq := r.Clone(r.Context()) + forwardedReq.Method = x.OrDefaultString(r.Header.Get(xForwardedMethod), r.Method) + forwardedReq.URL.Scheme = x.OrDefaultString(r.Header.Get(xForwardedProto), x.IfThenElseString(r.TLS != nil, "https", "http")) + forwardedReq.URL.Host = x.OrDefaultString(r.Header.Get(xForwardedHost), r.Host) + forwardedReq.URL.Path = x.OrDefaultString(strings.SplitN(r.Header.Get(xForwardedUri), "?", 2)[0], r.URL.Path[len(DecisionPath):]) h.decisions(w, forwardedReq) } else { next(w, r) From d28b8dace5ec71745ad3f1423694d43c3110c6a0 Mon Sep 17 00:00:00 2001 From: Jan-Niklas Weghorn Date: Sat, 10 Jun 2023 13:16:49 +0200 Subject: [PATCH 3/4] fix: merge upstream --- api/decision_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/decision_test.go b/api/decision_test.go index 7f43cb289e..17d2779d48 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -24,6 +24,7 @@ import ( "github.com/ory/herodot" "github.com/ory/x/logrusx" + "github.com/ory/x/configx" "github.com/ory/oathkeeper/api" "github.com/ory/oathkeeper/driver/configuration" @@ -34,7 +35,7 @@ import ( ) func TestDecisionAPI(t *testing.T) { - conf := internal.NewConfigurationWithDefaults() + conf := internal.NewConfigurationWithDefaults(configx.SkipValidation()) conf.SetForTest(t, configuration.AuthenticatorNoopIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorUnauthorizedIsEnabled, true) conf.SetForTest(t, configuration.AuthenticatorAnonymousIsEnabled, true) From 004b778c39c5f2db80d252a6543a256fc33a26d8 Mon Sep 17 00:00:00 2001 From: Jan-Niklas Weghorn Date: Sun, 15 Oct 2023 13:49:52 +0200 Subject: [PATCH 4/4] format --- api/decision_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/decision_test.go b/api/decision_test.go index 17d2779d48..bf514bb20d 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -23,8 +23,8 @@ import ( "github.com/urfave/negroni" "github.com/ory/herodot" - "github.com/ory/x/logrusx" "github.com/ory/x/configx" + "github.com/ory/x/logrusx" "github.com/ory/oathkeeper/api" "github.com/ory/oathkeeper/driver/configuration"