feat: Use unique name for csrf cookie

When multiple tabs attempt an oidc auth flow, they will
the oidc_state_csrf cookie will get overriden, causing
a race condition where one tab will succeed and all others
will fail horribly.

This feature aims to create a unique oidc_state_csrf cookie
per tab. This means each tab will complete its auth flow.

the authservice_session cookie may be overriden, but that
should be ok.

README.md

@@ -63,6 +63,7 @@ Following environment variables are used by the software.
 | `VERIFY_AUTH_URL` | `AUTHSERVICE_URL_PREFIX/verify` | Path to the `/verify` endpoint. This endpoint examines a subrequest and returns `204` if the user is authenticated and authorized to perform such a request, otherwise it will return `401` if the user cannot be authenticated or `403` if the user is authenticated but they are not authorized to perform this request. |
 | `AUTH_HEADER` | `Authorization` | When the AuthService logs in a user, it creates a session for them and saves it in its database. The session secret value is saved in a cookie in the user's browser. However, for programmatic access to endpoints, it is better to use headers to authenticate. The AuthService also accepts credentials in a header configured by the `AUTH_HEADER` setting. |
 | `ID_TOKEN_HEADER` | `Authorization` | When id token is carried in this header, OIDC Authservice verifies the id token and uses the `USERID_CLAIM` inside the id token. If the `USERID_CLAIM` doesn't exist, the authentication would fail.|
+| `DYNAMIC_CSRF_COOKIE_NAME` | `false` | Make the name of the oidc csrf cookie dynamic by adding a random suffix. If there are multiple browser tabs attempting an auth flow (eg: when the main session cookie times out) this can lead to a frustrating loss of context where the original tab ends up in a non-sensical auth url. This setting ensures each tab has a unique csrf cookie, which means they should all be able to complete an auth flow if required. |
 | `LOG_LEVEL` | "INFO" | Set the log level to one of "FATAL", "ERROR", "WARN", "INFO", or "DEBUG" to specify the verbosity of the OIDC-Authservice logs. |
 
 The AuthService provides a web server with some defaults pages for a `homepage`

common/settings.go

@@ -23,18 +23,20 @@ type Config struct {
 	OIDCStateStorePath      string   `split_words:"true" default:"/var/lib/authservice/data.db"`
 
 	// General
-	AuthserviceURLPrefix *url.URL `required:"true" split_words:"true"`
-	SkipAuthURLs         []string `split_words:"true" envconfig:"SKIP_AUTH_URLS"`
-	AuthHeader           string   `split_words:"true" default:"Authorization"`
-	AuthMethodHeader     string   `split_words:"true" default:"Auth-Method"`
-	SchemeDefault        string   `split_words:"true" default:"https"`
-	SchemeHeader         string   `split_words:"true" default:"X-Forwarded-Proto"`
-	Audiences            []string `default:"istio-ingressgateway.istio-system.svc.cluster.local"`
-	HomepageURL          *url.URL `split_words:"true"`
-	AfterLoginURL        *url.URL `split_words:"true"`
-	AfterLogoutURL       *url.URL `split_words:"true"`
-	VerifyAuthURL        *url.URL `split_words:"true"`
-	LogLevel             string   `split_words:"true" default:"INFO"`
+	AuthserviceURLPrefix  *url.URL `required:"true" split_words:"true"`
+	SkipAuthURLs          []string `split_words:"true" envconfig:"SKIP_AUTH_URLS"`
+	AuthHeader            string   `split_words:"true" default:"Authorization"`
+	AuthMethodHeader      string   `split_words:"true" default:"Auth-Method"`
+	SchemeDefault         string   `split_words:"true" default:"https"`
+	SchemeHeader          string   `split_words:"true" default:"X-Forwarded-Proto"`
+	Audiences             []string `default:"istio-ingressgateway.istio-system.svc.cluster.local"`
+	HomepageURL           *url.URL `split_words:"true"`
+	AfterLoginURL         *url.URL `split_words:"true"`
+	AfterLogoutURL        *url.URL `split_words:"true"`
+	VerifyAuthURL         *url.URL `split_words:"true"`
+	LogLevel              string   `split_words:"true" default:"INFO"`
+	DynamicCsrfCookieName bool     `split_words:"true"`
+
 
 	// Identity Headers
 	UserIDHeader      string            `split_words:"true" envconfig:"USERID_HEADER"`

main.go

@@ -232,6 +232,7 @@ func main() {
 		cacheEnabled:           c.CacheEnabled,
 		cacheExpirationMinutes: c.CacheExpirationMinutes,
 		jwtCookie:              c.JWTCookie,
+		dynamicCsrfCookieName:  c.DynamicCsrfCookieName,
 
 		IDTokenAuthnEnabled:         c.IDTokenAuthnEnabled,
 		KubernetesAuthnEnabled:      c.KubernetesAuthnEnabled,

server.go

@@ -47,6 +47,7 @@ type server struct {
 	verifyAuthURL          string
 	sessionMaxAgeSeconds   int
 	jwtCookie              string
+	dynamicCsrfCookieName  bool
 
 	// Cache Configurations
 	cacheEnabled           bool
@@ -365,7 +366,8 @@ func (s *server) authCodeFlowAuthenticationRequest(w http.ResponseWriter, r *htt
 	logger := common.RequestLogger(r, logModuleInfo)
 
 	// Initiate OIDC Flow with Authorization Request.
-	state, err := sessions.CreateState(r, w, s.oidcStateStore, s.sessionDomain, s.newState)
+	state, err := sessions.CreateState(r, w, s.oidcStateStore, s.sessionDomain,
+		s.newState, s.dynamicCsrfCookieName)
 	if err != nil {
 		logger.Errorf("Failed to save state in store: %v", err)
 		common.ReturnMessage(w, http.StatusInternalServerError, "Failed to save state in store.")
@@ -403,7 +405,7 @@ func (s *server) callback(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// If state is loaded, then it's correct, as it is saved by its id.
-	state, err := sessions.VerifyState(r, w, s.oidcStateStore)
+	state, err := sessions.VerifyState(r, w, s.oidcStateStore, s.dynamicCsrfCookieName)
 	if err != nil {
 		logger.Errorf("Failed to verify state parameter: %v", err)
 		common.ReturnMessage(w, http.StatusBadRequest, "CSRF check failed."+

sessions/state.go

@@ -4,6 +4,7 @@ package sessions
 
 import (
 	"encoding/gob"
+	"math/rand"
 	"net/http"
 	"net/url"
 	"strings"
@@ -17,8 +18,12 @@ import (
 const (
 	oidcStateCookie   = "oidc_state_csrf"
 	sessionValueState = "state"
+	charset           = "abcdefghijklmnopqrstuvwxyz"
 )
 
+var seededRand *rand.Rand = rand.New(
+	rand.NewSource(time.Now().UnixNano()))
+
 func init() {
 	gob.Register(State{})
 }
@@ -79,14 +84,33 @@ func newSchemeAndHost(config *Config) StateFunc {
 	}
 }
 
+// stringWithCharset creates a random string of length from charset
+func stringWithCharset(length int, charset string) string {
+	b := make([]byte, length)
+	for i := range b {
+	  b[i] = charset[seededRand.Intn(len(charset))]
+	}
+	return string(b)
+  }
+
+// randString returns a random string of given length
+func randString(length int) string {
+	return stringWithCharset(length, charset)
+}
+
 // CreateState creates the state parameter from the incoming request, stores
 // it in the session store and sets a cookie with the session key.
 // It returns the session key, which can be used as the state value to start
 // an OIDC authentication request.
-func CreateState(r *http.Request, w http.ResponseWriter,
-	store sessions.Store, sessionDomain string, fn StateFunc) (string, error) {
+func CreateState(r *http.Request, w http.ResponseWriter, store sessions.Store,
+	sessionDomain string, fn StateFunc, dynamicOidcStateCookieName bool) (string, error) {
+	nonce := randString(8)
+	oidcStateCookieName := oidcStateCookie
+	if (dynamicOidcStateCookieName) {
+		oidcStateCookieName += "_" + nonce
+	}
 	s := fn(r)
-	session := sessions.NewSession(store, oidcStateCookie)
+	session := sessions.NewSession(store, oidcStateCookieName)
 	session.Options.MaxAge = int((20 * time.Minute).Seconds())
 	session.Options.Path = "/"
 	session.Options.Domain = sessionDomain
@@ -100,11 +124,15 @@ func CreateState(r *http.Request, w http.ResponseWriter,
 	// Cookie is persisted in ResponseWriter, make a request to parse it.
 	tempReq := &http.Request{Header: make(http.Header)}
 	tempReq.Header.Set("Cookie", w.Header().Get("Set-Cookie"))
-	c, err := tempReq.Cookie(oidcStateCookie)
+	c, err := tempReq.Cookie(oidcStateCookieName)
 	if err != nil {
 		return "", errors.Wrap(err, "error trying to save session")
 	}
-	return c.Value, nil
+	stateValue := c.Value
+	if (dynamicOidcStateCookieName) {
+		stateValue += "." + nonce
+	}
+	return stateValue, nil
 }
 
 // VerifyState gets the state from the cookie 'initState' saved. It also gets
@@ -117,30 +145,40 @@ func CreateState(r *http.Request, w http.ResponseWriter,
 // Finally, it returns a State struct, which contains information associated
 // with the particular OIDC flow.
 func VerifyState(r *http.Request, w http.ResponseWriter,
-	store sessions.Store) (*State, error) {
+	store sessions.Store, dynamicOidcStateCookieName bool) (*State, error) {
 
 	// Get the state from the HTTP param.
 	var stateParam = r.FormValue("state")
 	if len(stateParam) == 0 {
 		return nil, errors.New("Missing url parameter: state")
 	}
 
+	oidcStateCookieName := oidcStateCookie
+	stateValue := stateParam
+	nonce := ""
+	if (dynamicOidcStateCookieName) {
+		stateParamParts := strings.Split(stateParam, ".")
+		stateValue = stateParamParts[0]
+		nonce = stateParamParts[1]
+		oidcStateCookieName += "_" + nonce
+	}
+
 	// Get the state from the cookie the user-agent sent.
-	stateCookie, err := r.Cookie(oidcStateCookie)
+	stateCookie, err := r.Cookie(oidcStateCookieName)
 	if err != nil {
-		return nil, errors.Errorf("Missing cookie: '%s'", oidcStateCookie)
+		return nil, errors.Errorf("Missing cookie: '%s'", oidcStateCookieName)
 	}
 
 	// Confirm the two values match.
-	if stateParam != stateCookie.Value {
+	if stateValue != stateCookie.Value {
 		return nil, errors.New("State value from http params doesn't match " +
 			"value in cookie. Possible reasons for this error include " +
 			"opening the login form in more than 1 browser tabs OR a CSRF " +
 			"attack.")
 	}
 
 	// Retrieve session from store. If it doesn't exist, it may have expired.
-	session, err := store.Get(r, oidcStateCookie)
+	session, err := store.Get(r, oidcStateCookieName)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}