fix: support longer http cookie (argoproj#2917) (argoproj#5497)

* fix: support longer cookie

Signed-off-by: kshamajain99 <kshamajain99@gmail.com>

cmd/argocd-dex/commands/argocd_dex.go

@@ -28,8 +28,8 @@ const (
 func NewCommand() *cobra.Command {
 	var command = &cobra.Command{
 		Use:               cliName,
-		Short:             "argocd-util tools used by Argo CD",
-		Long:              "argocd-util has internal utility tools used by Argo CD",
+		Short:             "argocd-dex tools used by Argo CD",
+		Long:              "argocd-dex has internal utility tools used by Argo CD",
 		DisableAutoGenTag: true,
 		Run: func(c *cobra.Command, args []string) {
 			c.HelpFunc()(c, args)

docs/operator-manual/server-commands/argocd-dex.md

@@ -1,10 +1,10 @@
 ## argocd-dex
 
-argocd-util tools used by Argo CD
+argocd-dex tools used by Argo CD
 
 ### Synopsis
 
-argocd-util has internal utility tools used by Argo CD
+argocd-dex has internal utility tools used by Argo CD
 
 ```
 argocd-dex [flags]

docs/operator-manual/server-commands/argocd-dex_gendexcfg.md

@@ -32,5 +32,5 @@ argocd-dex gendexcfg [flags]
 
 ### SEE ALSO
 
-* [argocd-dex](argocd-dex.md)	 - argocd-util tools used by Argo CD
+* [argocd-dex](argocd-dex.md)	 - argocd-dex tools used by Argo CD
 

docs/operator-manual/server-commands/argocd-dex_rundex.md

@@ -31,5 +31,5 @@ argocd-dex rundex [flags]
 
 ### SEE ALSO
 
-* [argocd-dex](argocd-dex.md)	 - argocd-util tools used by Argo CD
+* [argocd-dex](argocd-dex.md)	 - argocd-dex tools used by Argo CD
 

docs/user-guide/sync-options.md

@@ -60,8 +60,6 @@ The dry run will still be executed if the CRD is already present in the cluster.
 
 ## Selective Sync
 
->v1.8
-
 Currently when syncing using auto sync ArgoCD applies every object in the application. 
 For applications containing thousands of objects this takes quite a long time and puts undue pressure on the api server.
 Turning on selective sync option which will sync only out-of-sync resources. 

server/logout/logout.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/argoproj/argo-cd/common"
 	"github.com/argoproj/argo-cd/pkg/client/clientset/versioned"
+	httputil "github.com/argoproj/argo-cd/util/http"
 	jwtutil "github.com/argoproj/argo-cd/util/jwt"
 	"github.com/argoproj/argo-cd/util/session"
 	"github.com/argoproj/argo-cd/util/settings"
@@ -64,18 +65,25 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	logoutRedirectURL := strings.TrimRight(strings.TrimLeft(argoCDSettings.URL, "/"), "/") + strings.TrimRight(strings.TrimLeft(h.rootPath, "/"), "/")
 
-	argocdCookie, err := r.Cookie(common.AuthCookieName)
-	if err != nil {
+	cookies := r.Cookies()
+	tokenString, err = httputil.JoinCookies(common.AuthCookieName, cookies)
+	if tokenString == "" || err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		http.Error(w, "Failed to retrieve ArgoCD auth token: "+fmt.Sprintf("%s", err), http.StatusBadRequest)
 		return
 	}
 
-	tokenString = argocdCookie.Value
-
-	argocdCookie.Value = ""
-	argocdCookie.Path = fmt.Sprintf("/%s", strings.TrimRight(strings.TrimLeft(h.rootPath, "/"), "/"))
-	w.Header().Set("Set-Cookie", argocdCookie.String())
+	for _, cookie := range cookies {
+		if !strings.HasPrefix(cookie.Name, common.AuthCookieName) {
+			continue
+		}
+		argocdCookie := http.Cookie{
+			Name:  cookie.Name,
+			Value: "",
+		}
+		argocdCookie.Path = fmt.Sprintf("/%s", strings.TrimRight(strings.TrimLeft(h.rootPath, "/"), "/"))
+		w.Header().Add("Set-Cookie", argocdCookie.String())
+	}
 
 	claims, err := h.verifyToken(tokenString)
 	if err != nil {

server/server.go

@@ -610,11 +610,13 @@ func (a *ArgoCDServer) translateGrpcCookieHeader(ctx context.Context, w http.Res
 				return err
 			}
 		}
-		cookie, err := httputil.MakeCookieMetadata(common.AuthCookieName, token, flags...)
+		cookies, err := httputil.MakeCookieMetadata(common.AuthCookieName, token, flags...)
 		if err != nil {
 			return err
 		}
-		w.Header().Set("Set-Cookie", cookie)
+		for _, cookie := range cookies {
+			w.Header().Add("Set-Cookie", cookie)
+		}
 	}
 	return nil
 }
@@ -941,9 +943,9 @@ func getToken(md metadata.MD) string {
 		header := http.Header{}
 		header.Add("Cookie", t)
 		request := http.Request{Header: header}
-		token, err := request.Cookie(common.AuthCookieName)
-		if err == nil {
-			tokens = append(tokens, token.Value)
+		token, err := httputil.JoinCookies(common.AuthCookieName, request.Cookies())
+		if token != "" && err == nil {
+			tokens = append(tokens, token)
 		}
 	}
 

server/server_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -463,6 +464,17 @@ func TestTranslateGrpcCookieHeader(t *testing.T) {
 		})
 		assert.NoError(t, err)
 		assert.Equal(t, "argocd.token=xyz; path=/; SameSite=lax; httpOnly; Secure", recorder.Result().Header.Get("Set-Cookie"))
+		assert.Equal(t, 1, len(recorder.Result().Cookies()))
+	})
+
+	t.Run("TokenIsLongerThan4093", func(t *testing.T) {
+		recorder := httptest.NewRecorder()
+		err := argocd.translateGrpcCookieHeader(context.Background(), recorder, &session.SessionResponse{
+			Token: "abc.xyz." + strings.Repeat("x", 4093),
+		})
+		assert.NoError(t, err)
+		assert.Regexp(t, "argocd.token=.*; path=/; SameSite=lax; httpOnly; Secure", recorder.Result().Header.Get("Set-Cookie"))
+		assert.Equal(t, 2, len(recorder.Result().Cookies()))
 	})
 
 	t.Run("TokenIsEmpty", func(t *testing.T) {
@@ -471,7 +483,7 @@ func TestTranslateGrpcCookieHeader(t *testing.T) {
 			Token: "",
 		})
 		assert.NoError(t, err)
-		assert.Equal(t, "argocd.token=; path=/; SameSite=lax; httpOnly; Secure", recorder.Result().Header.Get("Set-Cookie"))
+		assert.Equal(t, "", recorder.Result().Header.Get("Set-Cookie"))
 	})
 
 }

util/http/http.go

@@ -2,27 +2,113 @@ package http
 
 import (
 	"fmt"
+	"math"
 	"net/http"
 	"net/http/httputil"
+	"strconv"
 	"strings"
 
 	log "github.com/sirupsen/logrus"
 )
 
+// max number of chunks a cookie can be broken into. To be compatible with
+// widest range of browsers, we shouldn't create more than 30 cookies per domain
+const maxCookieNumber = 5
+const maxCookieLength = 4093
+
 // MakeCookieMetadata generates a string representing a Web cookie.  Yum!
-func MakeCookieMetadata(key, value string, flags ...string) (string, error) {
-	components := []string{
-		fmt.Sprintf("%s=%s", key, value),
+func MakeCookieMetadata(key, value string, flags ...string) ([]string, error) {
+	attributes := strings.Join(flags, "; ")
+
+	// cookie: name=value; attributes and key: key-(i) e.g. argocd.token-1
+	maxValueLength := maxCookieValueLength(key, attributes)
+	numberOfCookies := int(math.Ceil(float64(len(value)) / float64(maxValueLength)))
+	if numberOfCookies > maxCookieNumber {
+		return nil, fmt.Errorf("invalid cookie value, at %d long it is longer than the max length of %d", len(value), maxValueLength*maxCookieNumber)
+	}
+
+	return splitCookie(key, value, attributes), nil
+}
+
+// browser has limit on size of cookie, currently 4kb. In order to
+// support cookies longer than 4kb, we split cookie into multiple 4kb chunks.
+// first chunk will be of format argocd.token=<numberOfChunks>:token; attributes
+func splitCookie(key, value, attributes string) []string {
+	var cookies []string
+	valueLength := len(value)
+	// cookie: name=value; attributes and key: key-(i) e.g. argocd.token-1
+	maxValueLength := maxCookieValueLength(key, attributes)
+	numberOfChunks := int(math.Ceil(float64(valueLength) / float64(maxValueLength)))
+
+	var end int
+	for i, j := 0, 0; i < valueLength; i, j = i+maxValueLength, j+1 {
+		end = i + maxValueLength
+		if end > valueLength {
+			end = valueLength
+		}
+
+		var cookie string
+		if j == 0 && numberOfChunks == 1 {
+			cookie = fmt.Sprintf("%s=%s", key, value[i:end])
+		} else if j == 0 {
+			cookie = fmt.Sprintf("%s=%d:%s", key, numberOfChunks, value[i:end])
+		} else {
+			cookie = fmt.Sprintf("%s-%d=%s", key, j, value[i:end])
+		}
+		if attributes != "" {
+			cookie = fmt.Sprintf("%s; %s", cookie, attributes)
+		}
+		cookies = append(cookies, cookie)
 	}
-	components = append(components, flags...)
-	header := strings.Join(components, "; ")
+	return cookies
+}
+
+// JoinCookies combines chunks of cookie based on key as prefix. It returns cookie
+// value as string. cookieString is of format key1=value1; key2=value2; key3=value3
+// first chunk will be of format argocd.token=<numberOfChunks>:token; attributes
+func JoinCookies(key string, cookieList []*http.Cookie) (string, error) {
+	cookies := make(map[string]string)
+	for _, cookie := range cookieList {
+		if !strings.HasPrefix(cookie.Name, key) {
+			continue
+		}
+		cookies[cookie.Name] = cookie.Value
+	}
+
+	var sb strings.Builder
+	var numOfChunks int
+	var err error
+	var token string
+	var ok bool
+
+	if token, ok = cookies[key]; !ok {
+		return "", fmt.Errorf("failed to retrieve cookie %s", key)
+	}
+	parts := strings.Split(token, ":")
+
+	if len(parts) == 2 {
+		if numOfChunks, err = strconv.Atoi(parts[0]); err != nil {
+			return "", err
+		}
+		sb.WriteString(parts[1])
+	} else if len(parts) == 1 {
+		numOfChunks = 1
+		sb.WriteString(parts[0])
+	} else {
+		return "", fmt.Errorf("invalid cookie for key %s", key)
+	}
+
+	for i := 1; i < numOfChunks; i++ {
+		sb.WriteString(cookies[fmt.Sprintf("%s-%d", key, i)])
+	}
+	return sb.String(), nil
+}
 
-	const maxLength = 4093
-	headerLength := len(header)
-	if headerLength > maxLength {
-		return "", fmt.Errorf("invalid cookie, at %d long it is longer than the max length of %d", headerLength, maxLength)
+func maxCookieValueLength(key, attributes string) int {
+	if len(attributes) > 0 {
+		return maxCookieLength - (len(key) + 3) - (len(attributes) + 2)
 	}
-	return header, nil
+	return maxCookieLength - (len(key) + 3)
 }
 
 // DebugTransport is a HTTP Client Transport to enable debugging

util/http/http_test.go

@@ -1,19 +1,41 @@
 package http
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestCookieMaxLength(t *testing.T) {
+	cookies, err := MakeCookieMetadata("foo", "bar")
+	assert.NoError(t, err)
+	assert.Equal(t, "foo=bar", cookies[0])
+
+	// keys will be of format foo, foo-1, foo-2 ..
+	cookies, err = MakeCookieMetadata("foo", strings.Repeat("_", (maxCookieLength-5)*maxCookieNumber))
+	assert.EqualError(t, err, "invalid cookie value, at 20440 long it is longer than the max length of 20435")
+	assert.Equal(t, 0, len(cookies))
+}
 
-	cookie, err := MakeCookieMetadata("foo", "bar")
+func TestSplitCookie(t *testing.T) {
+	cookieValue := strings.Repeat("_", (maxCookieLength-6)*4)
+	cookies, err := MakeCookieMetadata("foo", cookieValue)
 	assert.NoError(t, err)
-	assert.Equal(t, "foo=bar", cookie)
+	assert.Equal(t, 4, len(cookies))
+	assert.Equal(t, 2, len(strings.Split(cookies[0], "=")))
+	token := strings.Split(cookies[0], "=")[1]
+	assert.Equal(t, 2, len(strings.Split(token, ":")))
+	assert.Equal(t, "4", strings.Split(token, ":")[0])
 
-	cookie, err = MakeCookieMetadata("foo", strings.Repeat("_", 4093-3))
-	assert.EqualError(t, err, "invalid cookie, at 4094 long it is longer than the max length of 4093")
-	assert.Equal(t, "", cookie)
+	cookies = append(cookies, "bar=this-entry-should-be-filtered")
+	var cookieList []*http.Cookie
+	for _, cookie := range cookies {
+		parts := strings.Split(cookie, "=")
+		cookieList = append(cookieList, &http.Cookie{Name: parts[0], Value: parts[1]})
+	}
+	token, err = JoinCookies("foo", cookieList)
+	assert.NoError(t, err)
+	assert.Equal(t, cookieValue, token)
 }

util/oidc/oidc.go

@@ -330,13 +330,16 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, err.Error(), http.StatusInternalServerError)
 			return
 		}
-		cookie, err := httputil.MakeCookieMetadata(common.AuthCookieName, idTokenRAW, flags...)
+		cookies, err := httputil.MakeCookieMetadata(common.AuthCookieName, idTokenRAW, flags...)
 		if err != nil {
 			claimsJSON, _ := json.Marshal(claims)
 			http.Error(w, fmt.Sprintf("claims=%s, err=%v", claimsJSON, err), http.StatusInternalServerError)
 			return
 		}
-		w.Header().Set("Set-Cookie", cookie)
+
+		for _, cookie := range cookies {
+			w.Header().Add("Set-Cookie", cookie)
+		}
 	}
 
 	claimsJSON, _ := json.Marshal(claims)