-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the raw url path when proxying upstream requests #997
Conversation
I think this is actually a reasonable fix, did you ever look further into possible requests that it would break? |
I've deployed this change into our production which proxies various admin tools / frontends and had no issue with any of them. |
Ok well that seems reasonable, I've had a bit of a think about this and I think it's probably the right way to go, would like to get @NickMeves opinion as well though, Nick can you think of any negative affects of this change? |
I've definitely run into this in the past (with Apache & python webservers if I recall). This auto My only concern is the breaking aspect of this change -- us flipping this over would break upstream systems that potentially accounted for this issue in their logic? Is this minor enough of a breaking change to squeeze into a minor release? |
To rely on the current behaviour you'd have to explicitly link to sth like /%2F/ and expect the backend to just get / |
I ran into this a lot build security threat indicator systems. The threat indicator (IP, email, domain, URL) would be the last dynamic part of a path in a REST API on the graph database structure (e.g. a route in my backend like this - Where If this change happened under my nose without being marked as breaking - I would've needed to add URL decoding logic on the Now the question - is this situation minor enough of a breaking change to warrant this sneaking into a minor release instead of waiting for the next major? |
This got me curious for why all these webserver implementations do this by default and don't allow encoded slashes. I came across this blurb:
I'll look into those CVEs and think about the threat vectors |
Hm. Let me check my patch again. My intention was not to decode the %2F in
the url but pass it through as is. I believe the proxy should not touch the
incoming request at all.
Nick Meves <notifications@github.com> schrieb am Sa. 20. Feb. 2021 um 18:12:
… This got me curious for why all these webserver implementations do this by
default and don't allow encoded slashes. I came across this blurb:
Both Apache and Tomcat intentionally reject URIs with an encoded slash
(%2F for / and %5C for ) to prevent possible security vulnerabilities such
as CVE-2007-0450 and CVE-2007-0450 related attacks.
I'll look into those CVEs and think about the threat vectors
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#997 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEJ46WOGKYEGHZK4QPBZVLS77UQHANCNFSM4WHC5EMA>
.
|
Yeah, I think that's the attack vector various web servers have in mind when they refuse to proxy You can see that attack behavior a ton in our open redirect attack test cases: https://github.com/oauth2-proxy/oauth2-proxy/blob/master/testdata/openredirects.txt |
I think we need to look at the underlying Go HTTP server and see how it turns raw HTTP protocol bytes into its Then if we went down this route - I think we would want a flag (e.g. something like And finally, we would likely need our own function that only operates on encoded slashes -- I think I'm thinking since Go opted to turning |
It looks like |
This seems to be Go's implementation that we want to be mindful of - its comment has the description of our scenario: https://github.com/golang/go/blob/d4b26382342c98a95b85140b2863bc30c48edd68/src/net/url/url.go#L676 |
pkg/upstream/proxy.go
Outdated
@@ -51,6 +51,7 @@ type multiUpstreamProxy struct { | |||
|
|||
// ServerHTTP handles HTTP requests. | |||
func (m *multiUpstreamProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { | |||
req.URL.Path = req.URL.EscapedPath() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the implementation here: https://github.com/golang/go/blob/d4b26382342c98a95b85140b2863bc30c48edd68/src/net/url/url.go#L700
If we only want to be aware of force encoded slashes resulting in a difference during the building of URL.Path
and URL.RawPath
we should do this I think:
req.URL.Path = req.URL.EscapedPath() | |
if req.URL.RawPath != "" { | |
req.URL.Path = req.URL.EscapedPath() | |
} |
Otherwise EscapedPath()
goes down another codepath where it will do escaping on req.URL.Path
even though the original request builder didn't make a RawPath
because it wasn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about a more generic flag switching between the sanitized go Path and the escaped raw path instead of just allowEncodedSlashes?
And finally, we would likely need our own function that only operates on encoded slashes -- I think URL.EscapedPath() will be mutating a lot of other characters looking at the source code.
that's what i was trying to avoid. EscapedPath already considers a few edge cases and validates it. i don't wanna copy/reimplement this (and add new/other bugs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about a more generic flag switching between the sanitized go Path and the escaped raw path instead of just allowEncodedSlashes?
That seems good to me - since looking at the Go implementation, we are getting forced into more than just %2F
auto-decoding.
interestingly i looked into adding this config flag per upstream and found the following comment: @JoelSpeed do you remember what this referred to? this looks to me like it tried to fix the same thing? |
I didn't actually add that comment, just moved it from one package to another, it was originally introduced in bitly/oauth2_proxy#17 which does seem to be trying to resolve the same issue, but maybe is now broken? |
maybe back then the proxy did not allow for several upstreams / static file handlers and was not using a mux at all but proxying straight through? |
Rather than relying on the flag, implement a new option on the |
Sorry for the late reply. I was on vacation and offline for the last 2 weeks :)
Hmm. changing it to a struct would actually change the existing config file format. There might be more mux related options in the future so maybe a seperate config struct UpstreamOptions for general settings might make migration easier? i can set these easily via the test (and already do with the simple flag i added for now). My problem was more that the test framework sets everything up first (including all the options) and then runs all the tests on the same options. What i would like would be to run a different set of tests with different options and i'm not sure how to do this ginkgo. Do i put another Describe("Proxy Suite - flagX") in the same test file? |
The structured config is currently in alpha stage, so we can make breaking changes and that's ok! (I put a big red warning at the top of the docs about it! :D) We could do something like
Not sure about how to name this tbh, @NickMeves any thoughts on this change? Another option is
Yeah i think in this case, if it is going to affect all of the upstreams, then you would want it to. I guess the alternative to all of this, is whether this should be an option per upstream, that would then make it not a breaking change and would be easier to set up a new backend for the test suite. Does this actually effect file or static paths? Or is it really just that we need to set something in the proxy director of the http proxy? I guess we need to work out exactly where the 301 is coming from 🤔 |
Thinking about this, I was recently investigating replacing the mux with gorilla/mux, I did some work on this here #1060, I wonder if it might be easier to resolve this issue there. I notice they have a |
Looked into this a bit today and while it would work with gorilla mux, we can only set it for every upstream, we can't set it for individual upstreams at the moment. Though, there is a stale PR in place that would allow this to work gorilla/mux#463. I'll see if there's anything I can do to help get that PR merged |
changed the upstream config to a struct with the upstream list inside. the current change has no effect yet and all tests succeed (not sure why the github ci fails). If this is ok i can readd the rawpath change and tests at least as a setting for all upstreams. let me know if i can proceed. just wanted to check before adjusting the rest. regarding the tests: there really should be a gomega wildcard/regex matcher used instead of generic errors that need to fully match. otherwise quite a few tests will keep breaking all the time (e.g. the main_test check for a broken yaml that references a specific test config line number) |
hm... weird... i've readded the escapedpath line and the unit tests work fine but i still get the 301 redirect from the oauth proxy.... EDIT: Forget what i wrote... something about my test was broken (or my browser had the redirect cached). The flag works as expected |
Before we proceed with this, I want to spend a little time working out how this will interact with #1128 and #1060. We are basically ripping out and replacing the mux's in the regular and upstream cases, so I don't want to introduce this change now unless we can keep the behaviour when we merge these. Apologies for the delays in this, will hopefully get to fixing those up in the next couple of weeks |
the actual change to the upstream proxy is just a single line so i don't think there will be much of an impact but we can wait for the others to merge and i can rebase this one |
One of my concerns/thoughts here is that gorilla mux behaves very differently to the standard mux. I think it may even "just work" and not return the 301 redirects when it finds encoded characters, so we may not need this. But as I say, will keep this in mind and double check what we are doing |
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
ok. i have pushed a new version against current master. |
9c24bc2
to
13d9709
Compare
ok, this should fix the checked in docs. i had no idea these were generated automatically... |
Hm :/ |
Absolutely no idea, it shouldn't have been |
anything i can do to help this along? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay in reviewing this, I've been unable to get much time for this project lately due to other commitments and holidays
@@ -230,7 +236,7 @@ redirect_url="http://localhost:4180/oauth2/callback" | |||
configContent: testCoreConfig, | |||
alphaConfigContent: testAlphaConfig + ":", | |||
expectedOptions: func() *options.Options { return nil }, | |||
expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 49: did not find expected key"), | |||
expectedErr: fmt.Errorf("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line %d: did not find expected key", strings.Count(testAlphaConfig, "\n")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix, thanks! :D
pkg/apis/options/upstreams.go
Outdated
// Upstream represents the configuration for an upstream server. | ||
// Requests will be proxied to this upstream if the path matches the request path. | ||
Configs []Upstream `json:"configs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should start with the name of the field, so in this case Configs
instead of upstreams.
One quick question, what's a better name here in terms of UX, the three possibilities I can think of are configs
, backends
or servers
, did you have any thoughts about why configs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we call the surrounding struct UpstreamConfig and this one Upstreams?
Easy enough to change.
pkg/apis/options/upstreams.go
Outdated
@@ -8,7 +8,15 @@ const ( | |||
) | |||
|
|||
// Upstreams is a collection of definitions for upstream servers. | |||
type Upstreams []Upstream | |||
type Upstreams struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit weird to be called Upstreams
now right? I keep reading upstreams.configs
and I think it would be better if we had it as upstream.configs
.
I wonder if we should rename this to Upstream
and the current Upstream
struct to be something like UpstreamConfig
, this could be a follow up but should be done before we release I think 🤔
// Allows for specifying settings and even individual upstreams for specific tests and uses the default upstreams/configs otherwise | ||
upstreams := in.upstreams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach here!
pkg/upstream/proxy_test.go
Outdated
//upstreamServer.ServeHTTP(rw, req) | ||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these are needed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. i'll remove these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your review :)
I will push an updated version tomorrow i think
pkg/upstream/proxy_test.go
Outdated
//upstreamServer.ServeHTTP(rw, req) | ||
var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. i'll remove these
i just pushed a new version leaving the fixup commits in place and adding a rename commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think we are nearly there, please see my comment about the user facing naming as well. Once you've fixed that, if you can make sure we have a changelog entry and the commits are squashed into logical commits then I'm happy to get this merged, thanks for working with me on this one :)
pkg/apis/options/alpha_options.go
Outdated
@@ -12,7 +12,7 @@ type AlphaOptions struct { | |||
// Upstreams is used to configure upstream servers. | |||
// Once a user is authenticated, requests to the server will be proxied to | |||
// these upstream servers based on the path mappings defined in this list. | |||
Upstreams Upstreams `json:"upstreams,omitempty"` | |||
Upstreams UpstreamConfig `json:"upstreams,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming would be good to keep consistent. My last comment was primarily a UX based thing, so I'm thinking that in the yaml it would be good to be clear
usptreamConfig:
proxyRawPath: true
upstreams:
- type: ...
Does that make sense?
Upstreams UpstreamConfig `json:"upstreams,omitempty"` | |
UpstreamConfig UpstreamConfig `json:"upstreamConfig,omitempty"` |
pkg/apis/options/upstreams.go
Outdated
|
||
// Upstreams represents the configuration for the upstream servers. | ||
// Requests will be proxied to this upstream if the path matches the request path. | ||
Upstreams []Upstream `json:"configs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with my suggestion above this would stay as upstreams
in the json tag
Upstreams []Upstream `json:"configs,omitempty"` | |
Upstreams []Upstream `json:"upstreams,omitempty"` |
This allows urls with encoded characters (e.g.: /%2F/) to pass to the upstream mux instead of triggering a HTTP 301 from the frontend. Otherwise a /%2F/test/ will result in a HTTP 301 -> /test/
This commit changes Upstreams from []Upstream to a struct{} moving the previous []Upstream into .Configs and adjusts all uses of it.
Adding a new option to the yaml alpha config will result in failed tests unless you manually increment the line count. This commit computes this dynamically.
Setting this flag will configure the upstream proxy to pass encoded urls as-is.
Refactor proxy_test to set mux/upstream options for each test individually and add tests for encoded urls with ProxyRawPath set and unset.
ok, you are right. i missed these... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FStelzer, looks great!
Thanks a lot for your support on this! |
This way escaped char's like /%2F/ will still work instead of returning
301. The proxy should ideally touch the request as little as possible /
needed.
Description
Adds a new configuration flag to enable passing of the raw path to upstreams
Motivation and Context
#844
How Has This Been Tested?
a couple of new tests and running in our production setup for ~6month now.
Checklist: