fix(controlplane): keep download login tokens out of URLs

app/controlplane/internal/service/auth.go

@@ -333,14 +333,20 @@ func LegacyCallbackAllowed(authConfig *conf.Auth) bool {
 	return authConfig == nil || authConfig.AllowLegacyLoginCallback == nil || authConfig.GetAllowLegacyLoginCallback()
 }
 
-// isLegacyDelivery tells if a login would put the token in an external callback URL. A relative
-// callback is the same-origin download flow, which is not part of the deprecation
+// isLegacyDelivery tells if a login would put the token in a callback URL.
 func isLegacyDelivery(callback, codeChallenge string) bool {
-	return callback != "" && codeChallenge == "" && !isRelativePath(callback)
+	return callback != "" && codeChallenge == ""
 }
 
 // checkLegacyCallback rejects a legacy delivery when the operator denied it
 func (svc *AuthService) checkLegacyCallback(callback, codeChallenge string) error {
+	if isDownloadCallback(callback) {
+		u, _ := url.Parse(callback)
+		query, err := url.ParseQuery(u.RawQuery)
+		if err != nil || query.Has(oauth.QueryParamLegacyToken) || query.Has(oauth.QueryParamHandoff) || codeChallenge == "" {
+			return errors.New("download login requires a handoff code, please open the download link again")
+		}
+	}
 	if isLegacyDelivery(callback, codeChallenge) && !LegacyCallbackAllowed(svc.authConfig) {
 		return errLegacyCallbackDenied
 	}
@@ -594,6 +600,10 @@ func (svc *AuthService) deliverLogin(ctx context.Context, w http.ResponseWriter,
 	}
 
 	callbackValue := callbackURLFromCookie.Value
+	if isDownloadCallback(callbackValue) {
+		// A download must never opt into a long-lived user session.
+		expiration = shortLivedDuration
+	}
 
 	// There is no callback, render the token in an HTML page with a copy button
 	if callbackValue == "" {
@@ -780,7 +790,11 @@ func (svc *AuthService) ExchangeHandoff(ctx context.Context, req *pb.AuthService
 		return nil, errLoginHandoffExchange
 	}
 
-	token, err := svc.userToken(h.UserID.String(), h.TokenTTL)
+	ttl := h.TokenTTL
+	if isDownloadCallback(h.Callback) {
+		ttl = shortLivedDuration
+	}
+	token, err := svc.userToken(h.UserID.String(), ttl)
 	if err != nil {
 		return nil, sl.LogAndMaskErr(ctx, fmt.Errorf("generating user token: %w", err), svc.log)
 	}

app/controlplane/internal/service/auth_handoff_test.go

@@ -43,9 +43,10 @@ import (
 )
 
 const (
-	testHMACSecret = "test-secret"
-	testCLIBack    = "http://localhost:1234/auth/callback"
-	testAppBack    = "https://app.chainloop.dev/login/callback"
+	testHMACSecret   = "test-secret"
+	testCLIBack      = "http://localhost:1234/auth/callback"
+	testAppBack      = "https://app.chainloop.dev/login/callback"
+	testDownloadBack = "/download/sha256:abc"
 	// parses with no host, but browsers resolve it off-origin
 	testMultiSlashBack = "////evil.example/cb"
 )
@@ -181,10 +182,24 @@ func TestLoginHandlerCodeChallenge(t *testing.T) {
 			wantCode:    http.StatusTemporaryRedirect,
 		},
 		{
-			name:        "download flow not affected by the legacy setting",
-			params:      map[string]string{oauth.QueryParamCallback: "/download/sha256:abc"},
+			name:          "download flow not affected by the legacy setting",
+			params:        map[string]string{oauth.QueryParamCallback: testDownloadBack, oauth.QueryParamCodeChallenge: testChallenge, oauth.QueryParamCodeChallengeMethod: oauth.CodeChallengeMethodS256},
+			wantChallenge: testChallenge,
+			allowLegacy:   proto.Bool(false),
+			wantCode:      http.StatusTemporaryRedirect,
+		},
+		{
+			name:     "download requires challenge even with legacy allowed",
+			params:   map[string]string{oauth.QueryParamCallback: testDownloadBack},
+			wantCode: http.StatusBadRequest,
+			wantErr:  "download login requires a handoff code",
+		},
+		{
+			name:        "relative callbacks no longer bypass legacy setting",
+			params:      map[string]string{oauth.QueryParamCallback: "/other/callback"},
 			allowLegacy: proto.Bool(false),
-			wantCode:    http.StatusTemporaryRedirect,
+			wantCode:    http.StatusBadRequest,
+			wantErr:     "upgrade",
 		},
 		{
 			// It parses with no host, but browsers resolve it off-origin
@@ -277,7 +292,10 @@ func TestDeliverLogin(t *testing.T) {
 		{name: "code exchange with legacy denied", callback: testCLIBack, challenge: testChallenge, allowLegacy: proto.Bool(false), wantCode: http.StatusTemporaryRedirect, wantHandoff: true},
 		{name: "legacy login", callback: testCLIBack, wantCode: http.StatusTemporaryRedirect, wantToken: true, wantLegacyInc: 1},
 		{name: "legacy login denied", callback: testCLIBack, allowLegacy: proto.Bool(false), wantCode: http.StatusBadRequest},
-		{name: "download flow", callback: "/download/sha256:abc", allowLegacy: proto.Bool(false), wantCode: http.StatusTemporaryRedirect, wantToken: true},
+		{name: "download flow", callback: testDownloadBack, challenge: testChallenge, allowLegacy: proto.Bool(false), wantCode: http.StatusTemporaryRedirect, wantHandoff: true},
+		{name: "download without challenge", callback: testDownloadBack, wantCode: http.StatusBadRequest},
+		{name: "download with token in callback", callback: "/download/sha256:abc?t=old", challenge: testChallenge, wantCode: http.StatusBadRequest},
+		{name: "download with handoff in callback", callback: "/download/sha256:abc?handoff=old", challenge: testChallenge, wantCode: http.StatusBadRequest},
 		{name: "multi-slash callback cookie", callback: testMultiSlashBack, allowLegacy: proto.Bool(false), wantCode: http.StatusBadRequest},
 		{name: "multi-slash callback cookie with legacy allowed", callback: testMultiSlashBack, wantCode: http.StatusBadRequest},
 		{name: "headless", callback: "", wantCode: http.StatusOK},
@@ -327,7 +345,11 @@ func TestDeliverLogin(t *testing.T) {
 				assert.Equal(t, "the-handoff-code", q.Get(oauth.QueryParamHandoff))
 				assert.Empty(t, q.Get(oauth.QueryParamLegacyToken))
 				assert.Equal(t, userID, handoffs.issued.userID)
-				assert.Equal(t, longLivedDuration, handoffs.issued.ttl)
+				if isDownloadCallback(tc.callback) {
+					assert.Equal(t, shortLivedDuration, handoffs.issued.ttl)
+				} else {
+					assert.Equal(t, longLivedDuration, handoffs.issued.ttl)
+				}
 				assert.Equal(t, tc.challenge, handoffs.issued.challenge)
 				assert.Equal(t, tc.callback, handoffs.issued.callback)
 			}
@@ -416,6 +438,23 @@ func TestExchangeHandoff(t *testing.T) {
 		assert.WithinDuration(t, time.Now().Add(time.Hour), claims.ExpiresAt.Time, time.Minute)
 	})
 
+	t.Run("download clamps previously stored long lifetime", func(t *testing.T) {
+		svc := newHandoffTestService(nil, &fakeLoginHandoffs{exchanged: &biz.LoginHandoff{
+			UserID: userID, TokenTTL: longLivedDuration, Callback: testDownloadBack,
+		}}, nil)
+		resp, err := svc.ExchangeHandoff(context.Background(), &pb.AuthServiceExchangeHandoffRequest{
+			Handoff: "code", CodeVerifier: "verifier", Callback: testDownloadBack,
+		})
+		require.NoError(t, err)
+		claims := &user.CustomClaims{}
+		_, err = jwt.ParseWithClaims(resp.GetToken(), claims, func(_ *jwt.Token) (any, error) {
+			return []byte(testHMACSecret), nil
+		})
+		require.NoError(t, err)
+		assert.Equal(t, userID.String(), claims.UserID)
+		assert.WithinDuration(t, time.Now().Add(shortLivedDuration), claims.ExpiresAt.Time, time.Second)
+	})
+
 	reasons := []string{
 		biz.LoginHandoffReasonUnknown,
 		biz.LoginHandoffReasonExpired,

app/controlplane/internal/service/casredirect.go

@@ -22,6 +22,7 @@
 	"net/http"
 	"net/url"
 	"strings"
+	"time"
 
 	pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1"
 	conf "github.com/chainloop-dev/chainloop/app/controlplane/internal/conf/controlplane/config/v1"
@@ -31,6 +32,7 @@
 	kerrors "github.com/go-kratos/kratos/v2/errors"
 	khttp "github.com/go-kratos/kratos/v2/transport/http"
 	"github.com/google/uuid"
+	"golang.org/x/oauth2"
 )
 
 const (
@@ -41,6 +43,9 @@
 
 	// authorizationKey holds the key used to store the JWT Token in the request tokenHeader.
 	authorizationKey string = "Authorization"
+
+	cookieDownloadVerifierPrefix = "downloadVerifier_"
+	queryParamDownloadFlow       = "download_flow"
 )
 
 type CASRedirectService struct {
@@ -50,18 +55,23 @@
 	casMappingUC    *biz.CASMappingUseCase
 	casCredsUseCase *biz.CASCredentialsUseCase
 	casServerConf   *conf.Bootstrap_CASServer
+	auth            *AuthService
 }
 
-func NewCASRedirectService(casmUC *biz.CASMappingUseCase, casCredsUC *biz.CASCredentialsUseCase, conf *conf.Bootstrap_CASServer, opts ...NewOpt) (*CASRedirectService, error) {
+func NewCASRedirectService(casmUC *biz.CASMappingUseCase, casCredsUC *biz.CASCredentialsUseCase, conf *conf.Bootstrap_CASServer, auth *AuthService, opts ...NewOpt) (*CASRedirectService, error) {
 	if conf == nil || conf.GetDownloadUrl() == "" {
 		return nil, errors.New("CASServer.downloadURL configuration is missing")
 	}
+	if auth == nil {
+		return nil, errors.New("download authentication service is missing")
+	}
 
 	return &CASRedirectService{
 		service:         newService(opts...),
 		casMappingUC:    casmUC,
 		casCredsUseCase: casCredsUC,
 		casServerConf:   conf,
+		auth:            auth,
 	}, nil
 }
 
@@ -147,7 +157,9 @@
 // 3 - redirects to the generated URL
 // NOTE: This code is similar to the one that gets autogenerated if we happened to use proto.http but we can't use it because we need custom redirection
 func (s *CASRedirectService) HTTPDownload(ctx khttp.Context) error {
-	if redirected := authenticateUser(ctx.Response(), ctx.Request()); redirected {
+	if redirected, err := s.authenticateUser(ctx.Response(), ctx.Request()); err != nil {
+		return err
+	} else if redirected {
 		return nil
 	}
 
@@ -194,31 +206,100 @@
 	return nil
 }
 
-// authenticateUser checks if the user is authenticated, if not it will redirect to the login page
-// TODO: move to middleware
-func authenticateUser(w http.ResponseWriter, r *http.Request) (redirected bool) {
-	// Check if the token is already in the header
+// isDownloadCallback recognizes the relative callback used by the browser download route.
+func isDownloadCallback(callback string) bool {
+	if !isRelativePath(callback) {
+		return false
+	}
+	u, err := url.Parse(callback)
+	if err != nil || !strings.HasPrefix(u.Path, "/download/") {
+		return false
+	}
+	digest := strings.TrimPrefix(u.Path, "/download/")
+	return digest != "" && !strings.Contains(digest, "/") && digest != "." && digest != ".."
+}
+
+// authenticateUser exchanges a browser-bound handoff internally, keeping the user JWT out
+// of URLs. Header-authenticated clients continue through the normal middleware chain.
+func (s *CASRedirectService) authenticateUser(w http.ResponseWriter, r *http.Request) (bool, error) {
+	setTokenLeakHeaders(w)
+	query, err := url.ParseQuery(r.URL.RawQuery)
+	if err != nil {
+		return false, kerrors.BadRequest("invalid query", "invalid download query")
+	}
+	if query.Has(oauth.QueryParamLegacyToken) {
+		return false, kerrors.BadRequest("legacy download token", "please open the download link again without the token")
+	}
 	if r.Header.Get(authorizationKey) != "" {
-		return
+		return false, nil
 	}
 
-	// Check if it's in the query params and add it to the header
-	// This scenario happens when the user is redirected from the oauth callback
-	// so the next middleware will pick it up
-	if userToken := r.URL.Query().Get("t"); userToken != "" {
-		// Set the token in the request, this is required to call GetDownloadURL
-		r.Header.Set(authorizationKey, fmt.Sprintf(bearerFormat, userToken))
-		return
+	callback := *r.URL
+	callback.Scheme, callback.Host, callback.Fragment = "", "", ""
+	callback.ForceQuery = false
+	query.Del(oauth.QueryParamHandoff)
+	callback.RawQuery = query.Encode()
+	if !isDownloadCallback(callback.String()) {
+		return false, kerrors.BadRequest("invalid callback", "invalid download callback")
 	}
 
-	// redirect to login
-	loginURL, _ := url.Parse(AuthLoginPath)
-	// Append local callback URL
-	q := loginURL.Query()
-	q.Set(oauth.QueryParamCallback, r.URL.String())
-	loginURL.RawQuery = q.Encode()
+	if codes, ok := r.URL.Query()[oauth.QueryParamHandoff]; ok {
+		flows := query[queryParamDownloadFlow]
+		if len(flows) != 1 {
+			return false, errLoginHandoffExchange
+		}
+		flowID, err := uuid.Parse(flows[0])
+		if err != nil || flowID.String() != flows[0] {
+			return false, errLoginHandoffExchange
+		}
+		verifier, err := r.Cookie(cookieDownloadVerifierPrefix + flows[0])
+		s.setDownloadVerifierCookie(w, flows[0], "", true)
+		if err != nil || len(codes) != 1 || codes[0] == "" {
+			return false, errLoginHandoffExchange
+		}
+		token, err := s.auth.ExchangeHandoff(r.Context(), &pb.AuthServiceExchangeHandoffRequest{
+			Handoff: codes[0], CodeVerifier: verifier.Value, Callback: callback.String(),
+		})
+		if err != nil {
+			return false, err
+		}
+		r.Header.Set(authorizationKey, fmt.Sprintf(bearerFormat, token.Token))
+		return false, nil
+	}
 
-	// Get the current user auth token and set it in the request context
+	if query.Has(queryParamDownloadFlow) {
+		return false, errLoginHandoffExchange
+	}
+	// Bind the verifier cookie and callback to this flow so overlapping downloads
+	// neither replace each other's verifier nor clear each other's cookie.
+	flowID := uuid.NewString()
+	query.Set(queryParamDownloadFlow, flowID)
+	callback.RawQuery = query.Encode()
+	verifier := oauth2.GenerateVerifier()
+	s.setDownloadVerifierCookie(w, flowID, verifier, false)
+	loginURL := url.URL{Path: AuthLoginPath}
+	loginURL.RawQuery = url.Values{
+		oauth.QueryParamCallback:            {callback.String()},
+		oauth.QueryParamCodeChallenge:       {oauth2.S256ChallengeFromVerifier(verifier)},
+		oauth.QueryParamCodeChallengeMethod: {oauth.CodeChallengeMethodS256},
+	}.Encode()
 	http.Redirect(w, r, loginURL.String(), http.StatusTemporaryRedirect)
-	return true
+	return true, nil
+}
+
+func (s *CASRedirectService) setDownloadVerifierCookie(w http.ResponseWriter, flowID, verifier string, remove bool) {
+	// #nosec G124 -- Secure is disabled only in development mode, as for the OAuth state cookies.
+	cookie := &http.Cookie{
+		Name: cookieDownloadVerifierPrefix + flowID, Value: verifier, Path: "/download/",
+		HttpOnly: true, Secure: true, SameSite: http.SameSiteLaxMode,
+		MaxAge: 600, Expires: time.Now().Add(10 * time.Minute),
+	}
+	if s.auth.devMode {
+		cookie.Secure = false
+	}
+	if remove {
+		cookie.MaxAge = -1
+		cookie.Expires = time.Unix(1, 0)
+	}
+	http.SetCookie(w, cookie)
 }

app/controlplane/internal/service/casredirect_integration_test.go

@@ -110,7 +110,7 @@ func (s *casDownloadRBACIntegrationSuite) SetupTest() {
 		Logger:         s.L,
 	})
 
-	s.redirectSvc, err = NewCASRedirectService(s.CASMapping, casUC, &conf.Bootstrap_CASServer{DownloadUrl: "https://cas.example.test/download"})
+	s.redirectSvc, err = NewCASRedirectService(s.CASMapping, casUC, &conf.Bootstrap_CASServer{DownloadUrl: "https://cas.example.test/download"}, newHandoffTestService(nil, &fakeLoginHandoffs{}, nil))
 	s.Require().NoError(err)
 
 	s.credentialsSvc = NewCASCredentialsService(casUC, s.CASMapping, s.CASBackend, authzUC)