Skip to content

Commit

Permalink
pipe/err: Improve IP and MIME matching (#323)
Browse files Browse the repository at this point in the history 
Previously, MIME matching respected the request's wildcards which lead to multiple handlers feeling responsible for a particular request. Now, wildcards coming from the HTTP Request itself are interpreted literally.

Additionally, ORY Oathkeeper respected the X-Forwarded-For HTTP Header for matching remote IP addresses. This behavior is now turned off by default because clients were able to fake this header otherwise. It can explicitly be turned on by setting `config.when.#.request.remote_ip.RespectForwardedForHeader: true`.

Signed-off-by: aeneasr <[email protected]>
  • Loading branch information
@aeneasr
aeneasr authored Dec 26, 2019
1 parent 2764758 commit 7e6f636
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 26 deletions.
40 changes: 24 additions & 16 deletions pipeline/errors/when.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@ type (
}

WhenRequest struct {
CIDR []string `json:"cidr"`
Header *WhenRequestHeader `json:"header"`
RemoteIP *WhenRequestRemoteIP `json:"remote_ip"`
Header *WhenRequestHeader `json:"header"`
}

WhenRequestRemoteIP struct {
Match []string `json:"match"`
RespectForwardedForHeader bool `json:"respect_forwarded_for_header"`
}

WhenRequestHeader struct {
Expand Down Expand Up @@ -104,20 +109,20 @@ func matchesAcceptMIME(request string, handlers []string) bool {
for _, match := range hspec {
m := match.Value
switch {
case a == "*/*":
return true
case m == "*/*":
return true
case strings.HasSuffix(m, "/*") && strings.TrimSuffix(m, "/*") == strings.Split(a, "/")[0]:
return true

case a == m:
return true
case strings.HasSuffix(m, "/*"):
if strings.TrimSuffix(m, "/*") == strings.Split(a, "/")[0] {
return true
}
case strings.HasSuffix(a, "/*"):
if strings.TrimSuffix(a, "/*") == strings.Split(m, "/")[0] {
return true
}

// If the request contains wildcards, we expect the handler / search value to have an exact match! Otherwise
// we will get a lot of conflicts.
case a == "*/*" && a == m:
return true
case strings.HasSuffix(a, "/*") && a == m:
return true
}
}
}
Expand Down Expand Up @@ -161,18 +166,21 @@ func matchesRequest(when When, r *http.Request) error {
}
}

if len(when.Request.CIDR) > 0 {
if when.Request.RemoteIP != nil && len(when.Request.RemoteIP.Match) > 0 {
remoteIP, _, err := net.SplitHostPort(r.RemoteAddr)
if err != nil {
return errors.WithStack(err)
}

check := []string{remoteIP}
for _, fwd := range stringsx.Splitx(r.Header.Get("X-Forwarded-For"), ",") {
check = append(check, strings.TrimSpace(fwd))

if when.Request.RemoteIP.RespectForwardedForHeader {
for _, fwd := range stringsx.Splitx(r.Header.Get("X-Forwarded-For"), ",") {
check = append(check, strings.TrimSpace(fwd))
}
}

for _, rn := range when.Request.CIDR {
for _, rn := range when.Request.RemoteIP.Match {
_, cidr, err := net.ParseCIDR(rn)
if err != nil {
return errors.WithStack(err)
Expand Down
74 changes: 64 additions & 10 deletions pipeline/errors/when_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@ func TestMatchesMIME(t *testing.T) {
assert.False(t, matchesAcceptMIME("application/json", []string{"application/xml"}))
assert.True(t, matchesAcceptMIME("application/json", []string{"application/xml", "application/json"}))

assert.True(t, matchesAcceptMIME("application/*", []string{"application/json"}))
assert.True(t, matchesAcceptMIME("*/*", []string{"application/json"}))
assert.False(t, matchesAcceptMIME("application/*", []string{"application/json"}))
assert.False(t, matchesAcceptMIME("*/*", []string{"application/json"}))

assert.True(t, matchesAcceptMIME("application/*", []string{"application/*"}))
assert.True(t, matchesAcceptMIME("*/*", []string{"*/*"}))

assert.True(t, matchesAcceptMIME("text/html;q=0.9, application/xml;q=0.8, application/json;q=0.7", []string{"application/xml", "application/json"}))
assert.True(t, matchesAcceptMIME("application/xml", []string{"application/xml", "application/json"}))
Expand All @@ -39,6 +42,14 @@ func TestMatchesMIME(t *testing.T) {
func TestMatchesWhen(t *testing.T) {
mixedAccept := func(t *testing.T, r *http.Request) { r.Header.Set("Accept", "application/json,text/html") }
jsonAccept := func(t *testing.T, r *http.Request) { r.Header.Set("Accept", "application/json") }

chromeAccept := func(t *testing.T, r *http.Request) {
r.Header.Set("Accept", "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9")
}
firefoxAccept := func(t *testing.T, r *http.Request) {
r.Header.Set("Accept", "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8")
}

jsonContentType := func(t *testing.T, r *http.Request) { r.Header.Set("Content-Type", "application/json") }
withIPs := func(remote string, forwarded ...string) func(t *testing.T, r *http.Request) {
return func(t *testing.T, r *http.Request) {
Expand Down Expand Up @@ -128,7 +139,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand Down Expand Up @@ -241,7 +252,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -262,7 +273,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -274,7 +285,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -287,7 +298,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -300,7 +311,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -313,7 +324,20 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{Match: []string{"192.168.1.0/24"}},
},
},
},
in: errors.WithStack(&herodot.ErrNotFound),
ee: ErrDoesNotMatchWhen,
},
{
init: combine(jsonAccept, jsonContentType, withIPs("127.0.0.1:123", "127.0.0.2", "127.0.0.3", "192.168.1.2")),
w: Whens{
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
RemoteIP: &WhenRequestRemoteIP{RespectForwardedForHeader: true, Match: []string{"192.168.1.0/24"}},
},
},
},
Expand All @@ -325,7 +349,7 @@ func TestMatchesWhen(t *testing.T) {
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
CIDR: []string{"192.168.1.0/24"},
RemoteIP: &WhenRequestRemoteIP{RespectForwardedForHeader: true, Match: []string{"192.168.1.0/24"}},
Header: &WhenRequestHeader{
ContentType: []string{"application/json"},
Accept: []string{"application/xml", "application/json"},
Expand Down Expand Up @@ -365,6 +389,36 @@ func TestMatchesWhen(t *testing.T) {
},
in: errors.WithStack(&herodot.ErrNotFound),
},
{
init: combine(chromeAccept),
w: Whens{
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
Header: &WhenRequestHeader{
Accept: []string{"application/json"},
},
},
},
},
in: errors.WithStack(&herodot.ErrNotFound),
ee: ErrDoesNotMatchWhen,
},
{
init: combine(firefoxAccept),
w: Whens{
When{
Error: []string{statusText(http.StatusNotFound)},
Request: &WhenRequest{
Header: &WhenRequestHeader{
Accept: []string{"application/json"},
},
},
},
},
in: errors.WithStack(&herodot.ErrNotFound),
ee: ErrDoesNotMatchWhen,
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
r := httptest.NewRequest("GET", "/test", nil)
Expand Down

0 comments on commit 7e6f636

Please sign in to comment.