Skip to content

Commit

Permalink
feat: flag to disable hop-by-hop defenses (#1120)
Browse files Browse the repository at this point in the history
Ory Oathkeeper v0.44.4 uses the new Rewrite feature of Golang's reverse proxy. This will strip any X-Forwarded headers from upstream requests. This however is not always desirable which is why a new config flag `serve.proxy.trust_forwarded_headers` was introduced to optionally enable the forwarding of X-Forwarded headers.
  • Loading branch information
aeneasr authored Jul 17, 2023
1 parent 07c1e3c commit fffe8ef
Show file tree
Hide file tree
Showing 8 changed files with 293 additions and 235 deletions.
432 changes: 201 additions & 231 deletions .schema/version.schema.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions driver/configuration/config_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const (
ProxyIdleTimeout Key = "serve.proxy.timeout.idle"
ProxyServeAddressHost Key = "serve.proxy.host"
ProxyServeAddressPort Key = "serve.proxy.port"
ProxyTrustForwardedHeaders Key = "serve.proxy.trust_forwarded_headers"
APIServeAddressHost Key = "serve.api.host"
APIServeAddressPort Key = "serve.api.port"
APIReadTimeout Key = "serve.api.timeout.read"
Expand Down
2 changes: 2 additions & 0 deletions driver/configuration/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ type Provider interface {
CORSOptions(iface string) cors.Options
CORS(iface string) (cors.Options, bool)

ProxyTrustForwardedHeaders() bool

ProviderAuthenticators
ProviderErrorHandlers
ProviderAuthorizers
Expand Down
4 changes: 4 additions & 0 deletions driver/configuration/provider_koanf.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,10 @@ func (v *KoanfProvider) AuthenticatorIsEnabled(id string) bool {
return v.pipelineIsEnabled("authenticators", id)
}

func (v *KoanfProvider) ProxyTrustForwardedHeaders() bool {
return v.source.Bool(ProxyTrustForwardedHeaders)
}

func (v *KoanfProvider) AuthenticatorConfig(id string, override json.RawMessage, dest interface{}) error {
return v.PipelineConfig("authenticators", id, override, dest)
}
Expand Down
2 changes: 1 addition & 1 deletion driver/registry_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ func (r *RegistryMemory) AvailablePipelineMutators() (available []string) {

func (r *RegistryMemory) Proxy() *proxy.Proxy {
if r.proxyProxy == nil {
r.proxyProxy = proxy.NewProxy(r)
r.proxyProxy = proxy.NewProxy(r, r.c)
}

return r.proxyProxy
Expand Down
12 changes: 9 additions & 3 deletions proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"net/url"
"strings"

"github.com/ory/oathkeeper/driver/configuration"

"github.com/ory/oathkeeper/pipeline/authn"
"github.com/ory/oathkeeper/x"

Expand All @@ -22,17 +24,17 @@ import (
type proxyRegistry interface {
x.RegistryLogger
x.RegistryWriter

ProxyRequestHandler() RequestHandler
RuleMatcher() rule.Matcher
}

func NewProxy(r proxyRegistry) *Proxy {
return &Proxy{r: r}
func NewProxy(r proxyRegistry, c configuration.Provider) *Proxy {
return &Proxy{r: r, c: c}
}

type Proxy struct {
r proxyRegistry
c configuration.Provider
}

type key int
Expand Down Expand Up @@ -107,6 +109,10 @@ func (d *Proxy) RoundTrip(r *http.Request) (*http.Response, error) {
}

func (d *Proxy) Rewrite(r *httputil.ProxyRequest) {
if d.c.ProxyTrustForwardedHeaders() {
r.SetXForwarded()
}

EnrichRequestedURL(r)
rl, err := d.r.RuleMatcher().Match(r.Out.Context(), r.Out.Method, r.Out.URL, rule.ProtocolHTTP)
if err != nil {
Expand Down
69 changes: 69 additions & 0 deletions proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,12 @@ func TestProxy(t *testing.T) {
url string
code int
messages []string
messagesNot []string
rulesRegexp []rule.Rule
rulesGlob []rule.Rule
transform func(r *http.Request)
d string
prep func(t *testing.T)
}{
{
d: "should fail because url does not exist in rule set",
Expand Down Expand Up @@ -201,6 +203,62 @@ func TestProxy(t *testing.T) {
"host=" + x.ParseURLOrPanic(backend.URL).Host,
},
},
{
d: "should pass and set x-forwarded headers",
prep: func(t *testing.T) {
conf.SetForTest(t, configuration.ProxyTrustForwardedHeaders, true)
},
transform: func(r *http.Request) {
r.Header.Set("X-Forwarded-For", "foobar.com")
},
url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234",
rulesRegexp: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, 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: backend.URL},
}},
rulesGlob: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, 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: backend.URL},
}},
code: http.StatusOK,
messages: []string{
"authorization=",
"url=/authn-anon/authz-allow/cred-noop/1234",
"host=" + x.ParseURLOrPanic(backend.URL).Host,
"header X-Forwarded-Proto=http",
},
},
{
d: "should pass and remove x-forwarded headers",
transform: func(r *http.Request) {
r.Header.Set("X-Forwarded-For", "foobar.com")
},
url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234",
rulesRegexp: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, 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: backend.URL},
}},
rulesGlob: []rule.Rule{{
Match: &rule.Match{Methods: []string{"GET"}, 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: backend.URL},
}},
code: http.StatusOK,
messagesNot: []string{
"header X-Forwarded-Proto=http",
},
},
{
d: "should fail when authorizer fails",
url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234",
Expand Down Expand Up @@ -339,6 +397,10 @@ func TestProxy(t *testing.T) {
} {
t.Run(fmt.Sprintf("description=%s", tc.d), func(t *testing.T) {
testFunc := func(t *testing.T, strategy configuration.MatchingStrategy, rules []rule.Rule) {
if tc.prep != nil {
tc.prep(t)
}

reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules)
require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), strategy))

Expand All @@ -361,6 +423,13 @@ func TestProxy(t *testing.T) {
%s
proxy_url=%s
backend_url=%s
`, m, greeting, ts.URL, backend.URL)
}
for _, m := range tc.messagesNot {
assert.False(t, strings.Contains(string(greeting), m), `Value "%s" found in message but not expected:
%s
proxy_url=%s
backend_url=%s
`, m, greeting, ts.URL, backend.URL)
}

Expand Down
6 changes: 6 additions & 0 deletions spec/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,12 @@
"title": "Host",
"description": "The network interface to listen on. Leave empty to listen on all interfaces."
},
"trust_forwarded_headers": {
"type": "boolean",
"default": false,
"title": "Trust X-Forwarded Headers",
"description": "Trust the X-Forwarded-* headers from the reverse proxy. This is useful when running behind a load balancer or similar. Set this to false if you are not running behind a reverse proxy that prevents Hop-by-Hop attacks."
},
"timeout": {
"$ref": "#/definitions/serverTimeout"
},
Expand Down

0 comments on commit fffe8ef

Please sign in to comment.