ory / oathkeeper

A cloud native Identity & Access Proxy / API (IAP) and Access Control Decision API that authenticates, authorizes, and mutates incoming HTTP(s) requests. Inspired by the BeyondCorp / Zero Trust white paper. Written in Go.
https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=hydra
Apache License 2.0
3.2k stars 349 forks source link

fix: avoid unescaping slashes when proxying URLs #1134

Open refi64 opened 10 months ago

refi64 commented 10 months ago

Doing all the path operations on URL.Path means that they're being performed on an unescaped URL. Unfortunately, this URL is not guaranteed to be the directly unescaped version of URL.EscapedPath(); in particular, slashes will be unescaped in URL.Path, e.g. given:

/abc%2Fdef

URL.Path will be:

/abc/def

In these cases, URL.RawPath is also set with the escaped version of the path. However, once URL.Path is modified, URL.RawPath is ignored, and URL.EscapedPath() returns the path-escaped version of URL.Path...which, in this case, is now /abc/def, not the correct /abc%2Fdef.

As a result, when performing any path modifications during proxying (i.e. proxying to an upstream URL with a path component, or applying StripPath), this results in any escaped slashes in the proxied URL being unescaped.

In order to work around this, rewriting operations need to take place on the escaped path, not the unescaped one, and then an intermediate URL can be used to determine the Path and RawPath values to set on the forward URL.

This incurs a small breaking change in that StripPath is now applied to the escaped URL, not the unescaped URL. These semantics arguably make more sense, since StripPath otherwise also cannot distinguish between unencoded slashes within a path segment vs those that are separating path segments, but it's still a breaking change nonetheless.

BREAKING CHANGES: This patch changes the behavior of strip_path to be applied to the *escaped* URL as returned by Go's `URL.EscapePath()`, which means that slashes inside path segments, carets, unicode characters, and others that are need to be percent-encoded in strip_path.

Checklist

Further Comments

I debating trying to preserve the current strip_path semantics, but in addition to being unable to distinguish / and %2F, it's also just incredibly ugly: I need to put the value of the attribute in a stub URL and parse that to get the escaped path, which also introduces a large amount of weird edge cases that come from quirks in URL parsers.

CLAassistant commented 10 months ago

CLA assistant check
All committers have signed the CLA.

codecov[bot] commented 10 months ago

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.74%. Comparing base (8fc9b7a) to head (b16e209). Report is 2 commits behind head on master.

Files Patch % Lines
proxy/proxy.go 80.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1134 +/- ## ========================================== - Coverage 77.90% 77.74% -0.17% ========================================== Files 80 79 -1 Lines 3929 4040 +111 ========================================== + Hits 3061 3141 +80 - Misses 595 618 +23 - Partials 273 281 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

em- commented 2 months ago

Any chance to get this reviewed? Thank you!