diff --git a/CHANGELOG.md b/CHANGELOG.md index c205345..2fc6783 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] There are currently no unreleased changes. +## [v0.0.2] - 2021-03-25 +### Fixed +- Query parameters in request causing route matching failures when request paths are received through headers. + ## [v0.0.1] - 2020-06-12 ### Added - Support for original request path and method specification through headers (see [nginx docs](https://docs.nginx.com/nginx/admin-guide/security-controls/configuring-subrequest-authentication/)). @@ -24,5 +28,6 @@ This is the first version that includes the following functionality: - Claims-based authorization - Pure authorization server and reverse proxy modes -[Unreleased]: https://github.com/kaancfidan/bouncer/compare/v0.0.1...master +[Unreleased]: https://github.com/kaancfidan/bouncer/compare/v0.0.2...master +[v0.0.2]: https://github.com/kaancfidan/bouncer/compare/v0.0.1...v0.0.2 [v0.0.1]: https://github.com/kaancfidan/bouncer/compare/v0.0.0...v0.0.1 diff --git a/services/route_matcher.go b/services/route_matcher.go index 1927301..b63330f 100644 --- a/services/route_matcher.go +++ b/services/route_matcher.go @@ -1,6 +1,8 @@ package services import ( + "fmt" + "net/url" "strings" "github.com/gobwas/glob" @@ -29,12 +31,17 @@ func NewRouteMatcher(routePolicies []models.RoutePolicy) *RouteMatcherImpl { func (g RouteMatcherImpl) MatchRoutePolicies(path string, method string) ([]models.RoutePolicy, error) { matches := make([]models.RoutePolicy, 0) for _, rp := range g.routePolicies { - normalizedPath := "/" + strings.Trim(path, " \t\n/") + "/" + parsed, err := url.Parse(path) + if err != nil { + return nil, fmt.Errorf("could not parse path: %v", err) + } + + normalizedPath := "/" + strings.Trim(parsed.Path, " \t\n/") + "/" normalizedPolicyPath := "/" + strings.Trim(rp.Path, " \t\n/") + "/" g, err := glob.Compile(normalizedPolicyPath, '/') if err != nil { - return nil, err + return nil, fmt.Errorf("could not compile policy glob: %v", err) } // check if route matches diff --git a/services/route_matcher_test.go b/services/route_matcher_test.go index c87bb5d..5d0a7e8 100644 --- a/services/route_matcher_test.go +++ b/services/route_matcher_test.go @@ -31,6 +31,15 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) { want: nil, wantErr: true, }, + { + name: "path error", + routePolicies: []models.RoutePolicy{ + {Path: "/test", Methods: []string{"GET"}}, + }, + path: ".::this is not a valid path::.", + want: nil, + wantErr: true, + }, { name: "exact matched route", routePolicies: []models.RoutePolicy{ @@ -44,7 +53,19 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) { wantErr: false, }, { - name: "exact matched route with trailing separator", + name: "exact matched route with trailing separator in request path", + routePolicies: []models.RoutePolicy{ + {Path: "/test", Methods: []string{"GET"}}, + }, + path: "/test/", + method: "GET", + want: []models.RoutePolicy{ + {Path: "/test", Methods: []string{"GET"}}, + }, + wantErr: false, + }, + { + name: "exact matched route with trailing separator in route policy", routePolicies: []models.RoutePolicy{ {Path: "/test/", Methods: []string{"GET"}}, }, @@ -55,6 +76,18 @@ func TestRouteMatcherImpl_MatchRoutePolicies(t *testing.T) { }, wantErr: false, }, + { + name: "exact matched route with query parameters", + routePolicies: []models.RoutePolicy{ + {Path: "/test/", Methods: []string{"GET"}}, + }, + path: "/test?someBool=true&someString=test", + method: "GET", + want: []models.RoutePolicy{ + {Path: "/test/", Methods: []string{"GET"}}, + }, + wantErr: false, + }, { name: "exact matched route with spaces all around", routePolicies: []models.RoutePolicy{ diff --git a/services/server.go b/services/server.go index a35e423..ed48b3a 100644 --- a/services/server.go +++ b/services/server.go @@ -3,6 +3,7 @@ package services import ( "log" "net/http" + "net/url" "reflect" "github.com/google/uuid" @@ -50,7 +51,14 @@ func (s Server) Handle(writer http.ResponseWriter, request *http.Request) { path = request.URL.Path method = request.Method } else { - path = request.Header.Get(s.config.OriginalRequestHeaders.Path) + parsed, err := url.Parse(request.Header.Get(s.config.OriginalRequestHeaders.Path)) + if err != nil { + log.Printf("[%v] Request path read from header could not be parsed: %v", requestID, err) + writer.WriteHeader(http.StatusBadRequest) + return + } + + path = parsed.Path method = request.Header.Get(s.config.OriginalRequestHeaders.Method) } diff --git a/services/server_test.go b/services/server_test.go index 4005f80..f10dce3 100644 --- a/services/server_test.go +++ b/services/server_test.go @@ -18,6 +18,8 @@ import ( func TestServer_Handle(t *testing.T) { tests := []struct { name string + requestPath string + config models.ServerConfig proxyEnabled bool expectations func(*http.Request, *mocks.RouteMatcher, *mocks.Authenticator, *mocks.Authorizer) wantUpstreamCalled bool @@ -25,6 +27,7 @@ func TestServer_Handle(t *testing.T) { }{ { name: "route matching failed", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -38,8 +41,28 @@ func TestServer_Handle(t *testing.T) { wantUpstreamCalled: false, wantStatusCode: http.StatusInternalServerError, }, + { + name: "invalid path in original request headers", + requestPath: "**-.::invalid path::.-**", + config: models.ServerConfig{ + OriginalRequestHeaders: &models.OriginalRequestHeaders{ + Method: "X-Original-Method", + Path: "X-Original-URI", + }, + }, + proxyEnabled: false, + expectations: func( + request *http.Request, + routeMatcher *mocks.RouteMatcher, + authenticator *mocks.Authenticator, + authorizer *mocks.Authorizer) { + }, + wantUpstreamCalled: false, + wantStatusCode: http.StatusBadRequest, + }, { name: "allow anonymous", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -62,8 +85,43 @@ func TestServer_Handle(t *testing.T) { wantUpstreamCalled: false, wantStatusCode: http.StatusOK, }, + { + name: "allow anonymous through original request headers", + requestPath: "/some/path", + config: models.ServerConfig{ + OriginalRequestHeaders: &models.OriginalRequestHeaders{ + Method: "X-Original-Method", + Path: "X-Original-URI", + }, + }, + proxyEnabled: false, + expectations: func( + request *http.Request, + routeMatcher *mocks.RouteMatcher, + authenticator *mocks.Authenticator, + authorizer *mocks.Authorizer) { + + matchedRoutes := []models.RoutePolicy{ + { + Path: "/", + AllowAnonymous: true, + }, + } + + routeMatcher.On("MatchRoutePolicies", + request.Header.Get("X-Original-URI"), + request.Header.Get("X-Original-Method")).Return(matchedRoutes, nil) + + authorizer.On("IsAnonymousAllowed", + matchedRoutes, + request.Header.Get("X-Original-Method")).Return(true) + }, + wantUpstreamCalled: false, + wantStatusCode: http.StatusOK, + }, { name: "proxy enabled, allow anonymous", + requestPath: "/some/path", proxyEnabled: true, expectations: func( request *http.Request, @@ -88,6 +146,7 @@ func TestServer_Handle(t *testing.T) { }, { name: "authentication - success, authorization - success", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -122,6 +181,7 @@ func TestServer_Handle(t *testing.T) { }, { name: "proxy enabled, authentication - success, authorization - success", + requestPath: "/some/path", proxyEnabled: true, expectations: func( request *http.Request, @@ -156,6 +216,7 @@ func TestServer_Handle(t *testing.T) { }, { name: "authentication - failed", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -179,6 +240,7 @@ func TestServer_Handle(t *testing.T) { }, { name: "authentication - success, authorization - failed", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -218,6 +280,7 @@ func TestServer_Handle(t *testing.T) { }, { name: "error while authorization", + requestPath: "/some/path", proxyEnabled: false, expectations: func( request *http.Request, @@ -260,10 +323,24 @@ func TestServer_Handle(t *testing.T) { t.Run(tt.name, func(t *testing.T) { header := http.Header{} - request, err := http.NewRequest("GET", "/", nil) - if err != nil { - t.Errorf("could not be create request: %v", err) - return + var request *http.Request + var err error + + if tt.config.OriginalRequestHeaders != nil { + request, err = http.NewRequest("GET", "/", nil) + if err != nil { + t.Errorf("could not be create request: %v", err) + return + } + + request.Header.Set("X-Original-URI", tt.requestPath) + request.Header.Set("X-Original-Method", "POST") + } else { + request, err = http.NewRequest("POST", tt.requestPath, nil) + if err != nil { + t.Errorf("could not be create request: %v", err) + return + } } var upstream *mocks.Handler @@ -282,11 +359,16 @@ func TestServer_Handle(t *testing.T) { routeMatcher, authorizer, authenticator, - models.ServerConfig{}) + tt.config) tt.expectations(request, routeMatcher, authenticator, authorizer) if tt.wantUpstreamCalled { + if upstream == nil { + t.Errorf("invalid test case: proxy must be enabled to be called") + return + } + upstream.On("ServeHTTP", responseWriter, request).Return() } else { responseWriter.On("WriteHeader", tt.wantStatusCode).Return() @@ -302,7 +384,7 @@ func TestServer_Handle(t *testing.T) { assert.Equal(t, "Bearer", header["Www-Authenticate"][0]) } - if tt.proxyEnabled { + if upstream != nil { upstream.AssertExpectations(t) }