search

buzzfeed / sso

sso, aka S.S.Octopus, aka octoboi, is a single sign-on solution for securing internal services
MIT License
3.1k stars 186 forks source link

sso_*: proxy path-components with %-escaped characters in tact. #284

Closed katzdm closed 4 years ago

katzdm commented 4 years ago

Problem

When proxying to a path with a %-encoded / character (i.e. %2F), the Golang http.ServeMux class auto-unwraps the %-encoding. It then uses path.Clean() to "helpfully" normalize successive / characters (e.g. /a/b//c to /a/b/c, /a/b/../c to /a, etc).

Though admittedly an edge-case, the unintended side-effect is that a URL whose path contains a %-encoded URL will be proxied incorrectly. Fo instance, the URL https://example.com/path/http:%2F%2Ffoo.com/ will be proxied to https://example.com/path/http:/foo.com/.

Solution

Replace use of http.ServeMux with the mux.Router class from the popular https://github.com/gorilla/mux library, which allows use of URL.EscapedPath() in lieu of directly reading URL.Path. This preserves the %-wrapping of path-components, which in turn prevents path.Clean() from errantly rewriting the path.

Notes

The motivating example derives from the popular open-source Jenkins project, which uses URLs in such a form to check the health of a reverse-proxy - Hence this bug causes a Jenkins instance behind an SSO deployment to report a "broken" reverse-proxy configuration.

codecov[bot] commented 4 years ago

Codecov Report

Merging #284 into master will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #284   +/-   ##
=======================================
  Coverage   61.85%   61.85%           
=======================================
  Files          57       57           
  Lines        4637     4638    +1     
=======================================
+ Hits         2868     2869    +1     
  Misses       1556     1556           
  Partials      213      213

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f070dc5...de0300b. Read the comment docs.

katzdm commented 4 years ago

This looks good -- but I'd like to see a test added to the proxy side.

Great idea - Done; I've added one to oauthproxy_test.go.