Pass URI path upstream verbatim

Closes #528
Closes #483 by superseding it as a bug fix, not an option

Author: Nuru

doc.go

@@ -385,6 +385,11 @@ type RequestScope struct {
 	AccessDenied bool
 	// Identity is the user Identity of the request
 	Identity *userContext
+	// The parsed (unescaped) value of the request path
+	Path string
+	// Preserve the original request path: KEYCLOAK-10864, KEYCLOAK-11276, KEYCLOAK-13315
+	// The exact path received in the request, if different than Path
+	RawPath string
 }
 
 // storage is used to hold the offline refresh token, assuming you don't want to use

forwarding.go

@@ -32,8 +32,9 @@ func (r *oauthProxy) proxyMiddleware(next http.Handler) http.Handler {
 
 		// @step: retrieve the request scope
 		scope := req.Context().Value(contextScopeName)
+		var sc *RequestScope
 		if scope != nil {
-			sc := scope.(*RequestScope)
+			sc = scope.(*RequestScope)
 			if sc.AccessDenied {
 				return
 			}
@@ -56,6 +57,12 @@ func (r *oauthProxy) proxyMiddleware(next http.Handler) http.Handler {
 		// @note: by default goproxy only provides a forwarding proxy, thus all requests have to be absolute and we must update the host headers
 		req.URL.Host = r.endpoint.Host
 		req.URL.Scheme = r.endpoint.Scheme
+		// Restore the unprocessed original path, so that we pass upstream exactly what we received
+		// as the resource request.
+		if sc != nil {
+			req.URL.Path = sc.Path
+			req.URL.RawPath = sc.RawPath
+		}
 		if v := req.Header.Get("Host"); v != "" {
 			req.Host = v
 			req.Header.Del("Host")

middleware.go

@@ -41,30 +41,35 @@ const (
 // entrypointMiddleware is custom filtering for incoming requests
 func entrypointMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		keep := req.URL.Path
+		// @step: create a context for the request
+		scope := &RequestScope{}
+		// Save the exact formatting of the incoming request so we can use it later
+		scope.Path = req.URL.Path
+		scope.RawPath = req.URL.RawPath
+
+		// We want to Normalize the URL so that we can more easily and accurately
+		// parse it to apply resource protection rules.
 		purell.NormalizeURL(req.URL, normalizeFlags)
 
 		// ensure we have a slash in the url
 		if !strings.HasPrefix(req.URL.Path, "/") {
 			req.URL.Path = "/" + req.URL.Path
 		}
-		req.RequestURI = req.URL.RawPath
-		req.URL.RawPath = req.URL.Path
+		req.URL.RawPath = req.URL.EscapedPath()
 
-		// @step: create a context for the request
-		scope := &RequestScope{}
 		resp := middleware.NewWrapResponseWriter(w, 1)
 		start := time.Now()
+		// All the processing, including forwarding the request upstream and getting the response,
+		// happens here in this chain.
 		next.ServeHTTP(resp, req.WithContext(context.WithValue(req.Context(), contextScopeName, scope)))
 
 		// @metric record the time taken then response code
 		latencyMetric.Observe(time.Since(start).Seconds())
 		statusMetric.WithLabelValues(fmt.Sprintf("%d", resp.Status()), req.Method).Inc()
 
-		// place back the original uri for proxying request
-		req.URL.Path = keep
-		req.URL.RawPath = keep
-		req.RequestURI = keep
+		// place back the original uri for any later consumers
+		req.URL.Path = scope.Path
+		req.URL.RawPath = scope.RawPath
 	})
 }
 
@@ -92,13 +97,24 @@ func (r *oauthProxy) loggingMiddleware(next http.Handler) http.Handler {
 		resp := w.(middleware.WrapResponseWriter)
 		next.ServeHTTP(resp, req)
 		addr := req.RemoteAddr
-		r.log.Info("client request",
-			zap.Duration("latency", time.Since(start)),
-			zap.Int("status", resp.Status()),
-			zap.Int("bytes", resp.BytesWritten()),
-			zap.String("client_ip", addr),
-			zap.String("method", req.Method),
-			zap.String("path", req.URL.Path))
+		if req.URL.Path == req.URL.RawPath || req.URL.RawPath == "" {
+			r.log.Info("client request",
+				zap.Duration("latency", time.Since(start)),
+				zap.Int("status", resp.Status()),
+				zap.Int("bytes", resp.BytesWritten()),
+				zap.String("client_ip", addr),
+				zap.String("method", req.Method),
+				zap.String("path", req.URL.Path))
+		} else {
+			r.log.Info("client request",
+				zap.Duration("latency", time.Since(start)),
+				zap.Int("status", resp.Status()),
+				zap.Int("bytes", resp.BytesWritten()),
+				zap.String("client_ip", addr),
+				zap.String("method", req.Method),
+				zap.String("path", req.URL.Path),
+				zap.String("raw path", req.URL.RawPath))
+		}
 	})
 }
 

middleware_test.go

@@ -333,18 +333,19 @@ func TestMetricsMiddleware(t *testing.T) {
 	cfg.EnableMetrics = true
 	cfg.LocalhostMetrics = true
 	requests := []fakeRequest{
-		{
-			URI:                     cfg.WithOAuthURI(metricsURL),
-			ExpectedCode:            http.StatusOK,
-			ExpectedContentContains: "proxy_request_status_total",
-		},
 		{
 			URI: cfg.WithOAuthURI(metricsURL),
 			Headers: map[string]string{
 				"X-Forwarded-For": "10.0.0.1",
 			},
 			ExpectedCode: http.StatusForbidden,
 		},
+		// Some request must run before this one to generate request status numbers
+		{
+			URI:                     cfg.WithOAuthURI(metricsURL),
+			ExpectedCode:            http.StatusOK,
+			ExpectedContentContains: "proxy_request_status_total",
+		},
 	}
 	newFakeProxy(cfg).RunTests(t, requests)
 }
@@ -433,6 +434,100 @@ func TestMethodExclusions(t *testing.T) {
 	newFakeProxy(cfg).RunTests(t, requests)
 }
 
+func TestPreserveURLEncoding(t *testing.T) {
+	cfg := newFakeKeycloakConfig()
+	cfg.EnableLogging = true
+	cfg.Resources = []*Resource{
+		{
+			URL:     "/api/v2/*",
+			Methods: allHTTPMethods,
+			Roles:   []string{"dev"},
+		},
+		{
+			URL:     "/api/v1/auth*",
+			Methods: allHTTPMethods,
+			Roles:   []string{"admin"},
+		},
+		{
+			URL:         "/api/v1/*",
+			Methods:     allHTTPMethods,
+			WhiteListed: true,
+		},
+		{
+			URL:     "/*",
+			Methods: allHTTPMethods,
+			Roles:   []string{"user"},
+		},
+	}
+	requests := []fakeRequest{
+		{
+			URI:          "/test",
+			HasToken:     true,
+			Roles:        []string{"nothing"},
+			ExpectedCode: http.StatusForbidden,
+		},
+		{
+			URI:          "/",
+			ExpectedCode: http.StatusUnauthorized,
+		},
+		{ // See KEYCLOAK-10864
+			URI:                     "/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https%3A%2F%2Flocalhost%3A6001%2Fmanage/",
+			ExpectedContentContains: `"uri":"/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https%3A%2F%2Flocalhost%3A6001%2Fmanage/"`,
+			HasToken:                true,
+			Roles:                   []string{"user"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+		},
+		{ // See KEYCLOAK-11276
+			URI:                     "/iiif/2/edepot_local:ST%2F00001%2FST00005_00001.jpg/full/1000,/0/default.png",
+			ExpectedContentContains: `"uri":"/iiif/2/edepot_local:ST%2F00001%2FST00005_00001.jpg/full/1000,/0/default.png"`,
+			HasToken:                true,
+			Roles:                   []string{"user"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+		},
+		{ // See KEYCLOAK-13315
+			URI:                     "/rabbitmqui/%2F/replicate-to-central",
+			ExpectedContentContains: `"uri":"/rabbitmqui/%2F/replicate-to-central"`,
+			HasToken:                true,
+			Roles:                   []string{"user"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+		},
+		{ // should work
+			URI:           "/api/v1/auth",
+			HasToken:      true,
+			Roles:         []string{"admin"},
+			ExpectedProxy: true,
+			ExpectedCode:  http.StatusOK,
+		},
+		{ // should work
+			URI:                     "/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth",
+			ExpectedContentContains: `"uri":"/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth"`,
+			HasToken:                true,
+			Roles:                   []string{"admin"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+		},
+		{
+			URI:          "/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth",
+			HasToken:     true,
+			Roles:        []string{"user"},
+			ExpectedCode: http.StatusForbidden,
+		},
+		{ // should work
+			URI:                     "/api/v3/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth",
+			ExpectedContentContains: `"uri":"/api/v3/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth"`,
+			HasToken:                true,
+			Roles:                   []string{"user"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+		},
+	}
+
+	newFakeProxy(cfg).RunTests(t, requests)
+}
+
 func TestStrangeRoutingError(t *testing.T) {
 	cfg := newFakeKeycloakConfig()
 	cfg.Resources = []*Resource{
@@ -459,12 +554,13 @@ func TestStrangeRoutingError(t *testing.T) {
 	}
 	requests := []fakeRequest{
 		{ // should work
-			URI:           "/api/v1/events/123456789",
-			HasToken:      true,
-			Redirects:     true,
-			Roles:         []string{"user"},
-			ExpectedProxy: true,
-			ExpectedCode:  http.StatusOK,
+			URI:                     "/api/v1/events/123456789",
+			HasToken:                true,
+			Redirects:               true,
+			Roles:                   []string{"user"},
+			ExpectedProxy:           true,
+			ExpectedCode:            http.StatusOK,
+			ExpectedContentContains: `"uri":"/api/v1/events/123456789"`,
 		},
 		{ // should break with bad role
 			URI:          "/api/v1/events/123456789",

server.go

@@ -32,6 +32,8 @@ import (
 	"strings"
 	"time"
 
+	"go.uber.org/zap/zapcore"
+
 	"golang.org/x/crypto/acme/autocert"
 
 	httplog "log"
@@ -137,6 +139,8 @@ func createLogger(config *Config) (*zap.Logger, error) {
 	c := zap.NewProductionConfig()
 	c.DisableStacktrace = true
 	c.DisableCaller = true
+	// Use human-readable timestamps in the logs until KEYCLOAK-12100 is fixed
+	c.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
 	// are we enabling json logging?
 	if !config.EnableJSONLogging {
 		c.Encoding = "console"

server_test.go

@@ -86,9 +86,10 @@ func TestReverseProxyHeaders(t *testing.T) {
 	token := newTestToken(p.idp.getLocation())
 	token.addRealmRoles([]string{fakeAdminRole})
 	signed, _ := p.idp.signToken(token.claims)
+	uri := "/auth_all/test"
 	requests := []fakeRequest{
 		{
-			URI:           "/auth_all/test",
+			URI:           uri,
 			RawToken:      signed.Encode(),
 			ExpectedProxy: true,
 			ExpectedProxyHeaders: map[string]string{
@@ -99,7 +100,8 @@ func TestReverseProxyHeaders(t *testing.T) {
 				"X-Auth-Userid":   "rjayawardene",
 				"X-Auth-Username": "rjayawardene",
 			},
-			ExpectedCode: http.StatusOK,
+			ExpectedCode:            http.StatusOK,
+			ExpectedContentContains: `"uri":"` + uri + `"`,
 		},
 	}
 	p.RunTests(t, requests)
@@ -553,7 +555,10 @@ func (f *fakeUpstreamService) ServeHTTP(w http.ResponseWriter, r *http.Request)
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusOK)
 		content, _ := json.Marshal(&fakeUpstreamResponse{
-			URI:     r.RequestURI,
+			// r.RequestURI is what was received by the proxy.
+			// r.URL.String() is what is actually sent to the upstream service.
+			// KEYCLOAK-10864, KEYCLOAK-11276, KEYCLOAK-13315
+			URI:     r.URL.String(),
 			Method:  r.Method,
 			Address: r.RemoteAddr,
 			Headers: r.Header,