diff --git a/forwarding.go b/forwarding.go index df0ff2655..f93c7b3b8 100644 --- a/forwarding.go +++ b/forwarding.go @@ -29,6 +29,9 @@ import ( // reverseProxyMiddleware is responsible for handles reverse proxy request to the upstream endpoint func (r *oauthProxy) reverseProxyMiddleware() gin.HandlerFunc { return func(cx *gin.Context) { + // step: continue the flow + cx.Next() + // step: check its cool to continue if cx.IsAborted() { return } diff --git a/handlers_test.go b/handlers_test.go index 3d46d2470..6e52a5fb7 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -262,8 +262,8 @@ func TestAuthorizationURL(t *testing.T) { ExpectedCode: http.StatusTemporaryRedirect, }, { - URL: "/admin/../", - ExpectedURL: "/oauth/authorize?state=L2FkbWluLy4uLw==", + URL: "/help/../admin", + ExpectedURL: "/oauth/authorize?state=L2FkbWlu", ExpectedCode: http.StatusTemporaryRedirect, }, { diff --git a/middleware.go b/middleware.go index 2a0fa21db..36ccbeff1 100644 --- a/middleware.go +++ b/middleware.go @@ -16,12 +16,12 @@ limitations under the License. package main import ( - "bytes" "fmt" "regexp" "strings" "time" + "github.com/PuerkitoBio/purell" log "github.com/Sirupsen/logrus" "github.com/coreos/go-oidc/jose" "github.com/gin-gonic/gin" @@ -34,19 +34,19 @@ const ( cxEnforce = "Enforcing" ) +const normalizeFlags purell.NormalizationFlags = purell.FlagRemoveDotSegments | purell.FlagRemoveDuplicateSlashes + // filterMiddleware is custom filtering for incoming requests func (r *oauthProxy) filterMiddleware() gin.HandlerFunc { return func(cx *gin.Context) { - var p rune - var b bytes.Buffer - for _, c := range cx.Request.URL.Path { - if c == '/' && p == '/' { - continue - } - p = c - b.WriteRune(c) - } - cx.Request.URL.Path = b.String() + // step: keep a copy of the original + orig := *cx.Request.URL + // step: normalize the url + purell.NormalizeURL(cx.Request.URL, normalizeFlags) + // step: continue the flow + cx.Next() + // step: place back the original + cx.Request.URL = &orig } } diff --git a/middleware_test.go b/middleware_test.go index 2a8e93c76..d257738e3 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -26,60 +26,62 @@ import ( "github.com/stretchr/testify/assert" ) -func TestRolePermissionsMiddleware(t *testing.T) { - cfg := newFakeKeycloakConfig() +type fakeRequest struct { + URI string + Method string + Redirects bool + HasToken bool + NotSigned bool + Expires time.Duration + Roles []string + Expects int +} + +func makeFakesRequests(t *testing.T, reqs []fakeRequest, cfg *Config) { cfg.SkipTokenVerification = false - cfg.Resources = []*Resource{ - { - URL: fakeAdminRoleURL, - Methods: []string{"ANY"}, - Roles: []string{fakeAdminRole}, - }, - { - URL: fakeTestRoleURL, - Methods: []string{"GET"}, - Roles: []string{fakeTestRole}, - }, - { - URL: fakeTestAdminRolesURL, - Methods: []string{"GET"}, - Roles: []string{fakeAdminRole, fakeTestRole}, - }, - { - URL: fakeTestWhitelistedURL, - WhiteListed: true, - Methods: []string{}, - Roles: []string{}, - }, - { - URL: "/", - Methods: []string{"ANY"}, - Roles: []string{fakeTestRole}, - }, - } px, idp, svc := newTestProxyService(cfg) + for i, c := range reqs { + px.config.NoRedirects = !c.Redirects + // step: make the client + hc := resty.New().SetRedirectPolicy(resty.NoRedirectPolicy()) + if c.HasToken { + token := newTestToken(idp.getLocation()) + if len(c.Roles) > 0 { + token.setRealmsRoles(c.Roles) + } + if c.Expires > 0 { + token.setExpiration(time.Now().Add(c.Expires)) + } + if !c.NotSigned { + signed, err := idp.signToken(token.claims) + if !assert.NoError(t, err, "case %d, unable to sign the token, error: %s", i, err) { + continue + } + hc.SetAuthToken(signed.Encode()) + } else { + jwt := token.getToken() + hc.SetAuthToken(jwt.Encode()) + } + } + // step: make the request + resp, err := hc.R().Execute(c.Method, svc+c.URI) + if err != nil { + if !strings.Contains(err.Error(), "Auto redirect is disable") { + assert.NoError(t, err, "case %d, unable to make request, error: %s", i, err) + continue + } + } + // step: check against the expected + assert.Equal(t, c.Expects, resp.StatusCode(), "case %d, uri: %s, expected: %d, got: %d", + i, c.URI, c.Expects, resp.StatusCode()) + } +} - // test cases - cs := []struct { - URI string - Method string - Redirects bool - HasToken bool - NotSigned bool - Expires time.Duration - Roles []string - Expects int - }{ +func TestOauthRequests(t *testing.T) { + cfg := newFakeKeycloakConfig() + requests := []fakeRequest{ { - URI: "/", - Expects: http.StatusUnauthorized, - }, - { // check whitelisted is passed - URI: fakeTestWhitelistedURL, - Expects: http.StatusOK, - }, - { // check for redirect - URI: "/", + URI: "/oauth/authorize", Redirects: true, Expects: http.StatusTemporaryRedirect, }, @@ -93,6 +95,20 @@ func TestRolePermissionsMiddleware(t *testing.T) { Redirects: true, Expects: http.StatusOK, }, + } + makeFakesRequests(t, requests, cfg) +} + +func TestStrangeAdminRequests(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.Resources = []*Resource{ + { + URL: "/admin", + Methods: []string{"ANY"}, + Roles: []string{fakeAdminRole}, + }, + } + requests := []fakeRequest{ { // check for escaping URI: "//admin%2Ftest", Redirects: true, @@ -124,11 +140,6 @@ func TestRolePermissionsMiddleware(t *testing.T) { Redirects: true, Expects: http.StatusTemporaryRedirect, }, - { // check for dodgy url - URI: "/help/../admin/test", - Redirects: true, - Expects: http.StatusTemporaryRedirect, - }, { // check for it works URI: "//admin/test", HasToken: true, @@ -141,6 +152,94 @@ func TestRolePermissionsMiddleware(t *testing.T) { Roles: []string{fakeAdminRole}, Expects: http.StatusOK, }, + { + URI: "/help/../admin/test/21", + Redirects: false, + Expects: http.StatusUnauthorized, + }, + } + makeFakesRequests(t, requests, cfg) +} + +func TestWhiteListedRequests(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.Resources = []*Resource{ + { + URL: "/whitelist", + WhiteListed: true, + Methods: []string{"GET"}, + Roles: []string{}, + }, + { + URL: "/", + Methods: []string{"ANY"}, + Roles: []string{fakeTestRole}, + }, + { + URL: "/whitelisted", + WhiteListed: true, + Methods: []string{"ANY"}, + Roles: []string{fakeTestRole}, + }, + } + requests := []fakeRequest{ + { // check whitelisted is passed + URI: "/whitelist", + Expects: http.StatusOK, + }, + { // check whitelisted is passed + URI: "/whitelist/test", + Expects: http.StatusOK, + }, + { + URI: "/", + Expects: http.StatusUnauthorized, + }, + } + makeFakesRequests(t, requests, cfg) +} + +func TestRolePermissionsMiddleware(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.Resources = []*Resource{ + { + URL: "/admin", + Methods: []string{"ANY"}, + Roles: []string{fakeAdminRole}, + }, + { + URL: "/test", + Methods: []string{"GET"}, + Roles: []string{fakeTestRole}, + }, + { + URL: "/test_admin_role", + Methods: []string{"GET"}, + Roles: []string{fakeAdminRole, fakeTestRole}, + }, + { + URL: "/whitelist", + WhiteListed: true, + Methods: []string{"GET"}, + Roles: []string{}, + }, + { + URL: "/", + Methods: []string{"ANY"}, + Roles: []string{fakeTestRole}, + }, + } + // test cases + requests := []fakeRequest{ + { + URI: "/", + Expects: http.StatusUnauthorized, + }, + { // check for redirect + URI: "/", + Redirects: true, + Expects: http.StatusTemporaryRedirect, + }, { // check with a token URI: "/", Redirects: false, @@ -155,20 +254,27 @@ func TestRolePermissionsMiddleware(t *testing.T) { Expects: http.StatusForbidden, }, { // token, wrong roles - URI: fakeTestRoleURL, + URI: "/test", Redirects: false, HasToken: true, Roles: []string{"bad_role"}, Expects: http.StatusForbidden, }, { // token, wrong roles, no 'get' method - URI: fakeTestRoleURL, + URI: "/test", Method: http.MethodPost, Redirects: false, HasToken: true, Roles: []string{"bad_role"}, Expects: http.StatusOK, }, + { // check with correct token + URI: "/test", + Redirects: false, + HasToken: true, + Roles: []string{fakeTestRole}, + Expects: http.StatusOK, + }, { // check with correct token URI: "/", Redirects: false, @@ -185,7 +291,7 @@ func TestRolePermissionsMiddleware(t *testing.T) { Expects: http.StatusForbidden, }, { // check with correct token, signed - URI: fakeAdminRoleURL, + URI: "/admin/page", Method: http.MethodPost, Redirects: false, HasToken: true, @@ -193,26 +299,26 @@ func TestRolePermissionsMiddleware(t *testing.T) { Expects: http.StatusForbidden, }, { // check with correct token, signed, wrong roles - URI: fakeAdminRoleURL, + URI: "/admin/page", Redirects: false, HasToken: true, Roles: []string{fakeTestRole}, Expects: http.StatusForbidden, }, { // check with correct token, signed, wrong roles - URI: fakeAdminRoleURL, + URI: "/admin/page", Redirects: false, HasToken: true, Roles: []string{fakeTestRole, fakeAdminRole}, Expects: http.StatusOK, }, { // strange url - URI: fakeAdminRoleURL + "/.." + fakeAdminRoleURL, + URI: "/admin/..//admin/page", Redirects: false, Expects: http.StatusUnauthorized, }, { // strange url, token - URI: fakeAdminRoleURL + "/.." + fakeAdminRoleURL, + URI: "/admin/../admin", Redirects: false, HasToken: true, Roles: []string{"hehe"}, @@ -229,51 +335,24 @@ func TestRolePermissionsMiddleware(t *testing.T) { Redirects: false, HasToken: true, Roles: []string{fakeAdminRole}, - Expects: http.StatusForbidden, + Expects: http.StatusOK, }, { // strange url, token, wrong roles - URI: "/test/.." + fakeTestAdminRolesURL, + URI: "/test/../admin", Redirects: false, HasToken: true, Roles: []string{fakeAdminRole}, + Expects: http.StatusOK, + }, + { // strange url, token, wrong roles + URI: "/test/../admin", + Redirects: false, + HasToken: true, + Roles: []string{fakeTestRole}, Expects: http.StatusForbidden, }, } - for i, c := range cs { - px.config.NoRedirects = !c.Redirects - // step: make the client - hc := resty.New().SetRedirectPolicy(resty.NoRedirectPolicy()) - if c.HasToken { - token := newTestToken(idp.getLocation()) - if len(c.Roles) > 0 { - token.setRealmsRoles(c.Roles) - } - if c.Expires > 0 { - token.setExpiration(time.Now().Add(c.Expires)) - } - if !c.NotSigned { - signed, err := idp.signToken(token.claims) - if !assert.NoError(t, err, "case %d, unable to sign the token, error: %s", i, err) { - continue - } - hc.SetAuthToken(signed.Encode()) - } else { - jwt := token.getToken() - hc.SetAuthToken(jwt.Encode()) - } - } - // step: make the request - resp, err := hc.R().Execute(c.Method, svc+c.URI) - if err != nil { - if !strings.Contains(err.Error(), "Auto redirect is disable") { - assert.NoError(t, err, "case %d, unable to make request, error: %s", i, err) - continue - } - } - // step: check against the expected - assert.Equal(t, c.Expects, resp.StatusCode(), "case %d, uri: %s, expected: %d, got: %d", - i, c.URI, c.Expects, resp.StatusCode()) - } + makeFakesRequests(t, requests, cfg) } func TestCrossSiteHandler(t *testing.T) { diff --git a/server.go b/server.go index 9dda7bfc0..8f0f37461 100644 --- a/server.go +++ b/server.go @@ -155,7 +155,7 @@ func (r *oauthProxy) createReverseProxy() error { engine := gin.New() engine.Use(gin.Recovery()) // step: custom filtering - engine.Use(r.filterMiddleware()) + engine.Use(r.filterMiddleware(), r.reverseProxyMiddleware()) // step: is profiling enabled? if r.config.EnableProfiling { @@ -205,8 +205,10 @@ func (r *oauthProxy) createReverseProxy() error { } // step: add the middleware - engine.Use(r.entrypointMiddleware(), r.authenticationMiddleware(), r.admissionMiddleware(), - r.headersMiddleware(r.config.AddClaims), r.reverseProxyMiddleware()) + engine.Use(r.entrypointMiddleware(), + r.authenticationMiddleware(), + r.admissionMiddleware(), + r.headersMiddleware(r.config.AddClaims)) // step: set the handler r.router = engine