Based on the security best practices we should not use HTTP 307 for r…

…edirects, instead HTTP 303 should be used. The option `InvalidAuthRedirectsWith303` was removed, as does not make sense to have it anymore. More details: https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9 Resolves #627
louketo · Jun 2, 2020 · d5a8563 · d5a8563
1 parent f623dee
commit d5a8563
Show file tree

Hide file tree

Showing 8 changed files with 30 additions and 37 deletions.
diff --git a/doc.go b/doc.go
@@ -306,8 +306,6 @@ type Config struct {
 	// EncryptionKey is the encryption key used to encrypt the refresh token
 	EncryptionKey string `json:"encryption-key" yaml:"encryption-key" usage:"encryption key used to encryption the session state" env:"ENCRYPTION_KEY"`
 
-	// InvalidAuthRedirectsWith303 will make requests with invalid auth headers redirect using HTTP 303 instead of HTTP 307.  See github.com/keycloak/keycloak-gatekeeper/issues/292 for context.
-	InvalidAuthRedirectsWith303 bool `json:"invalid-auth-redirects-with-303" yaml:"invalid-auth-redirects-with-303" usage:"use HTTP 303 redirects instead of 307 for invalid auth tokens"`
 	// NoRedirects informs we should hand back a 401 not a redirect
 	NoRedirects bool `json:"no-redirects" yaml:"no-redirects" usage:"do not have back redirects when no authentication is present, 401 them"`
 	// SkipTokenVerification tells the service to skipp verifying the access token - for testing purposes

diff --git a/handlers.go b/handlers.go
@@ -101,7 +101,7 @@ func (r *oauthProxy) oauthAuthorizationHandler(w http.ResponseWriter, req *http.
 		return
 	}
 
-	r.redirectToURL(authURL, w, req, http.StatusTemporaryRedirect)
+	r.redirectToURL(authURL, w, req, http.StatusSeeOther)
 }
 
 // getClientAuthMethod maps the config value CLIENT_AUTH_METHOD to valid OAuth2 auth method keys
@@ -229,7 +229,7 @@ func (r *oauthProxy) oauthCallbackHandler(w http.ResponseWriter, req *http.Reque
 		}
 	}
 
-	r.redirectToURL(redirectURI, w, req, http.StatusTemporaryRedirect)
+	r.redirectToURL(redirectURI, w, req, http.StatusSeeOther)
 }
 
 // loginHandler provide's a generic endpoint for clients to perform a user_credentials login to the provider
@@ -359,7 +359,7 @@ func (r *oauthProxy) logoutHandler(w http.ResponseWriter, req *http.Request) {
 			}
 		}
 
-		r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusTemporaryRedirect)
+		r.redirectToURL(fmt.Sprintf("%s?redirect_uri=%s", sendTo, url.QueryEscape(redirectURL)), w, req, http.StatusSeeOther)
 
 		return
 	}
@@ -411,7 +411,7 @@ func (r *oauthProxy) logoutHandler(w http.ResponseWriter, req *http.Request) {
 	}
 	// step: should we redirect the user
 	if redirectURL != "" {
-		r.redirectToURL(redirectURL, w, req, http.StatusTemporaryRedirect)
+		r.redirectToURL(redirectURL, w, req, http.StatusSeeOther)
 	}
 }
 

diff --git a/handlers_test.go b/handlers_test.go
@@ -179,7 +179,7 @@ func TestLogoutHandlerGood(t *testing.T) {
 		{
 			URI:              c.WithOAuthURI(logoutURL) + "?redirect=http://example.com",
 			HasToken:         true,
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 			ExpectedLocation: "http://example.com",
 		},
 	}
@@ -220,7 +220,7 @@ func TestServiceRedirect(t *testing.T) {
 		{
 			URI:              "/admin",
 			Redirects:        true,
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 			ExpectedLocation: "/oauth/authorize?state",
 		},
 		{
@@ -248,25 +248,25 @@ func TestAuthorizationURL(t *testing.T) {
 			URI:              "/admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/admin/test",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/help/../admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              "/admin?test=yes&test1=test",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:          "/oauth/test",
@@ -298,19 +298,19 @@ func TestCallbackURL(t *testing.T) {
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake&state=/admin",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 		{
 			URI:              cfg.WithOAuthURI(callbackURL) + "?code=fake&state=L2FkbWlu",
 			ExpectedCookies:  map[string]string{cfg.CookieAccessName: ""},
 			ExpectedLocation: "/",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(cfg).RunTests(t, requests)

diff --git a/middleware_test.go b/middleware_test.go
@@ -356,7 +356,7 @@ func TestOauthRequests(t *testing.T) {
 		{
 			URI:          "/oauth/authorize",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{
 			URI:          "/oauth/callback",
@@ -379,7 +379,7 @@ func TestOauthRequestsWithBaseURI(t *testing.T) {
 		{
 			URI:          "/base-uri/oauth/authorize",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{
 			URI:          "/base-uri/oauth/callback",
@@ -614,22 +614,22 @@ func TestNoProxyingRequests(t *testing.T) {
 		{ // check for escaping
 			URI:          "/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/etc/passwd",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/.%2e/",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/../%2e",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(c).RunTests(t, requests)
@@ -650,27 +650,27 @@ func TestStrangeAdminRequests(t *testing.T) {
 		{ // check for escaping
 			URI:          "//admin%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "///admin/../admin//%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for escaping
 			URI:          "/admin%2Ftest",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for prefix slashs
 			URI:          "/" + testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for double slashs
 			URI:          testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for double slashs no redirects
 			URI:          "/admin//test",
@@ -681,7 +681,7 @@ func TestStrangeAdminRequests(t *testing.T) {
 		{ // check for dodgy url
 			URI:          "//admin/.." + testAdminURI,
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check for it works
 			URI:           "/" + testAdminURI,
@@ -942,7 +942,7 @@ func TestRolePermissionsMiddleware(t *testing.T) {
 		{ // check for redirect
 			URI:          "/",
 			Redirects:    true,
-			ExpectedCode: http.StatusTemporaryRedirect,
+			ExpectedCode: http.StatusSeeOther,
 		},
 		{ // check with a token but not test role
 			URI:          "/",

diff --git a/misc.go b/misc.go
@@ -107,11 +107,7 @@ func (r *oauthProxy) redirectToAuthorization(w http.ResponseWriter, req *http.Re
 		w.WriteHeader(http.StatusForbidden)
 		return r.revokeProxy(w, req)
 	}
-	if r.config.InvalidAuthRedirectsWith303 {
-		r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)
-	} else {
-		r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusTemporaryRedirect)
-	}
+	r.redirectToURL(r.config.WithOAuthURI(authorizationURL+authQuery), w, req, http.StatusSeeOther)
 
 	return r.revokeProxy(w, req)
 }

diff --git a/misc_test.go b/misc_test.go
@@ -36,15 +36,14 @@ func TestRedirectToAuthorization(t *testing.T) {
 			URI:              "/admin",
 			Redirects:        true,
 			ExpectedLocation: "/oauth/authorize?state",
-			ExpectedCode:     http.StatusTemporaryRedirect,
+			ExpectedCode:     http.StatusSeeOther,
 		},
 	}
 	newFakeProxy(nil).RunTests(t, requests)
 }
 
 func TestRedirectToAuthorizationWith303Enabled(t *testing.T) {
 	cfg := newFakeKeycloakConfig()
-	cfg.InvalidAuthRedirectsWith303 = true
 
 	requests := []fakeRequest{
 		{

diff --git a/oauth_test.go b/oauth_test.go
@@ -188,7 +188,7 @@ func (r *fakeAuthServer) authHandler(w http.ResponseWriter, req *http.Request) {
 	}
 	redirectionURL := fmt.Sprintf("%s?state=%s&code=%s", redirect, state, getRandomString(32))
 
-	http.Redirect(w, req, redirectionURL, http.StatusTemporaryRedirect)
+	http.Redirect(w, req, redirectionURL, http.StatusSeeOther)
 }
 
 func (r *fakeAuthServer) logoutHandler(w http.ResponseWriter, req *http.Request) {

diff --git a/server_test.go b/server_test.go
@@ -508,7 +508,7 @@ func makeTestCodeFlowLogin(location string) (*http.Response, error) {
 		if err != nil {
 			return nil, err
 		}
-		if resp.StatusCode != http.StatusTemporaryRedirect {
+		if resp.StatusCode != http.StatusSeeOther {
 			return nil, errors.New("no redirection found in resp")
 		}
 		location = resp.Header.Get("Location")