diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index 9da2a1733..afc16d9ae 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -289,7 +289,7 @@ func wireApp(contextContext context.Context, bootstrap *conf.Bootstrap, readerWr integrationsService := service.NewIntegrationsService(integrationUseCase, workflowUseCase, availablePlugins, v5...) organizationService := service.NewOrganizationService(membershipUseCase, organizationUseCase, v5...) casBackendService := service.NewCASBackendService(casBackendUseCase, providers, v5...) - casRedirectService, err := service.NewCASRedirectService(casMappingUseCase, casCredentialsUseCase, bootstrap_CASServer, v5...) + casRedirectService, err := service.NewCASRedirectService(casMappingUseCase, casCredentialsUseCase, bootstrap_CASServer, authService, v5...) if err != nil { cleanup3() cleanup2() diff --git a/app/controlplane/internal/service/auth.go b/app/controlplane/internal/service/auth.go index 3217e7c3c..810c9b274 100644 --- a/app/controlplane/internal/service/auth.go +++ b/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) } diff --git a/app/controlplane/internal/service/auth_handoff_test.go b/app/controlplane/internal/service/auth_handoff_test.go index 646c59f8a..788e21b95 100644 --- a/app/controlplane/internal/service/auth_handoff_test.go +++ b/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, diff --git a/app/controlplane/internal/service/casredirect.go b/app/controlplane/internal/service/casredirect.go index bc751411f..45cc4ad0d 100644 --- a/app/controlplane/internal/service/casredirect.go +++ b/app/controlplane/internal/service/casredirect.go @@ -22,6 +22,7 @@ import ( "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 @@ import ( 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 @@ const ( // 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 @@ type CASRedirectService struct { 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 @@ func (s *CASRedirectService) GetDownloadURL(ctx context.Context, req *pb.GetDown // 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 @@ func (s *CASRedirectService) HTTPDownload(ctx khttp.Context) error { 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) } diff --git a/app/controlplane/internal/service/casredirect_integration_test.go b/app/controlplane/internal/service/casredirect_integration_test.go index 805d5e072..8ee8fe647 100644 --- a/app/controlplane/internal/service/casredirect_integration_test.go +++ b/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) diff --git a/app/controlplane/internal/service/casredirect_test.go b/app/controlplane/internal/service/casredirect_test.go new file mode 100644 index 000000000..81e52ed3c --- /dev/null +++ b/app/controlplane/internal/service/casredirect_test.go @@ -0,0 +1,344 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package service + +import ( + "context" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/jwt/user" + "github.com/chainloop-dev/chainloop/internal/oauth" + "github.com/go-kratos/kratos/v2/middleware" + "github.com/go-kratos/kratos/v2/transport" + khttp "github.com/go-kratos/kratos/v2/transport/http" + "github.com/golang-jwt/jwt/v5" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" +) + +// Exercise the real handoff use case without a database or an upstream OIDC provider. +type downloadHandoffRepo struct { + row *biz.LoginHandoff + rows map[string]*biz.LoginHandoff + used map[string]bool +} + +func (r *downloadHandoffRepo) Create(_ context.Context, hash string, h *biz.LoginHandoff) error { + if r.rows == nil { + r.rows = make(map[string]*biz.LoginHandoff) + r.used = make(map[string]bool) + } + r.row = h + r.rows[hash] = h + return nil +} + +func (r *downloadHandoffRepo) Consume(_ context.Context, hash string, _ time.Time) (*biz.LoginHandoff, bool, error) { + h, ok := r.rows[hash] + if !ok { + return nil, false, biz.NewErrNotFound("handoff") + } + consumed := !r.used[hash] + r.used[hash] = true + return h, consumed, nil +} + +func (*downloadHandoffRepo) DeleteExpired(context.Context, time.Time) (int, error) { + return 0, nil +} + +func newDownloadTestService() (*CASRedirectService, *downloadHandoffRepo) { + repo := &downloadHandoffRepo{} + auth := newHandoffTestService(proto.Bool(false), nil, nil) + auth.loginHandoffs = biz.NewLoginHandoffUseCase(repo, auth.log.Logger()) + return &CASRedirectService{auth: auth}, repo +} + +// Starts a browser download, follows /auth/login, and delivers a validated OIDC login. +func startDownloadLogin(t *testing.T, svc *CASRedirectService) (string, *http.Cookie, string) { + t.Helper() + r := httptest.NewRequest(http.MethodGet, "/download/sha256:abc?z=2&filename=my%20file", nil) + w := httptest.NewRecorder() + redirected, err := svc.authenticateUser(w, r) + require.NoError(t, err) + require.True(t, redirected) + require.Equal(t, http.StatusTemporaryRedirect, w.Code) + assert.Empty(t, r.Header.Get(authorizationKey)) + assert.Equal(t, "no-store", w.Header().Get("Cache-Control")) + assert.Equal(t, "no-referrer", w.Header().Get("Referrer-Policy")) + + login, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + assert.Equal(t, AuthLoginPath, login.Path) + assert.Empty(t, login.Query().Get(oauth.QueryParamLegacyToken)) + assert.Empty(t, login.Query().Get(oauth.QueryParamLongLived)) + require.Len(t, w.Result().Cookies(), 1) + verifier := w.Result().Cookies()[0] + assert.True(t, verifier.Secure) + assert.True(t, verifier.HttpOnly) + assert.Equal(t, http.SameSiteLaxMode, verifier.SameSite) + assert.Equal(t, "/download/", verifier.Path) + assert.Positive(t, verifier.MaxAge) + assert.True(t, oauth.VerifyS256(verifier.Value, login.Query().Get(oauth.QueryParamCodeChallenge))) + assert.NotContains(t, login.String(), verifier.Value) + + // Even a login page that requests a long session cannot lengthen a download session. + query := login.Query() + query.Set(oauth.QueryParamLongLived, "true") + login.RawQuery = query.Encode() + loginResponse := httptest.NewRecorder() + require.Nil(t, loginHandler(svc.auth, loginResponse, httptest.NewRequest(http.MethodGet, login.String(), nil))) + callbackRequest := httptest.NewRequest(http.MethodGet, AuthCallbackPath, nil) + for _, cookie := range loginResponse.Result().Cookies() { + callbackRequest.AddCookie(cookie) + } + userID := uuid.NewString() + callbackResponse := httptest.NewRecorder() + require.Nil(t, svc.auth.deliverLogin(context.Background(), callbackResponse, callbackRequest, userID)) + callback, err := url.Parse(callbackResponse.Header().Get("Location")) + require.NoError(t, err) + assert.Empty(t, callback.Query().Get(oauth.QueryParamLegacyToken)) + assert.NotEmpty(t, callback.Query().Get(oauth.QueryParamHandoff)) + assert.NotEmpty(t, callback.Query().Get(queryParamDownloadFlow)) + return callback.String(), verifier, userID +} + +func TestBrowserDownloadHandoff(t *testing.T) { + svc, repo := newDownloadTestService() + callback, verifier, userID := startDownloadLogin(t, svc) + assert.Equal(t, shortLivedDuration, repo.row.TokenTTL) + assert.WithinDuration(t, time.Now().Add(biz.LoginHandoffTTL), repo.row.ExpiresAt, time.Second) + + // Verify that the existing middleware chain sees the internally exchanged user token. + var internalToken string + srv := khttp.NewServer(khttp.Middleware(func(_ middleware.Handler) middleware.Handler { + return func(ctx context.Context, _ interface{}) (interface{}, error) { + tr, ok := transport.FromServerContext(ctx) + require.True(t, ok) + assert.Equal(t, getDownloadURLOperation, tr.Operation()) + internalToken = strings.TrimPrefix(tr.RequestHeader().Get(authorizationKey), "Bearer ") + claims := &user.CustomClaims{} + _, err := jwt.ParseWithClaims(internalToken, claims, func(*jwt.Token) (interface{}, error) { + return []byte(testHMACSecret), nil + }) + require.NoError(t, err) + assert.Equal(t, userID, claims.UserID) + assert.WithinDuration(t, time.Now().Add(shortLivedDuration), claims.ExpiresAt.Time, time.Second) + return &pb.GetDownloadURLResponse{Result: &pb.GetDownloadURLResponse_Result{ + Url: "https://cas.example/download/sha256:abc?t=cas-download-token", + }}, nil + } + })) + srv.Route("/").GET("/download/{digest}", svc.HTTPDownload) + r := httptest.NewRequest(http.MethodGet, callback, nil) + r.AddCookie(verifier) + w := httptest.NewRecorder() + srv.ServeHTTP(w, r) + require.Equal(t, http.StatusFound, w.Code) + assert.NotEmpty(t, internalToken) + assert.NotContains(t, w.Body.String(), internalToken) + assert.NotContains(t, w.Header().Get("Location"), internalToken) + assert.Equal(t, "https://cas.example/download/sha256:abc?t=cas-download-token", w.Header().Get("Location")) + assert.Equal(t, "no-store", w.Header().Get("Cache-Control")) + assert.Equal(t, "no-referrer", w.Header().Get("Referrer-Policy")) + require.Len(t, w.Result().Cookies(), 1) + assert.Negative(t, w.Result().Cookies()[0].MaxAge) + + // Replaying the URL with the original verifier still fails. + replay := httptest.NewRequest(http.MethodGet, callback, nil) + replay.AddCookie(verifier) + redirected, err := svc.authenticateUser(httptest.NewRecorder(), replay) + assert.False(t, redirected) + assert.ErrorIs(t, err, errLoginHandoffExchange) + assert.Empty(t, replay.Header.Get(authorizationKey)) +} + +func TestBrowserDownloadHandoffRejected(t *testing.T) { + for _, name := range []string{"missing cookie", "wrong verifier", "wrong digest", "wrong query", "expired", "unknown", "duplicate code", "empty code", "missing flow", "duplicate flow", "invalid flow", "wrong flow"} { + t.Run(name, func(t *testing.T) { + svc, repo := newDownloadTestService() + callback, cookie, _ := startDownloadLogin(t, svc) + u, err := url.Parse(callback) + require.NoError(t, err) + q := u.Query() + clearsCookie := true + switch name { + case "missing cookie": + cookie = nil + case "wrong verifier": + cookie.Value = strings.Repeat("x", 43) + case "wrong digest": + u.Path = "/download/sha256:other" + case "wrong query": + q.Set("filename", "other") + case "expired": + repo.row.ExpiresAt = time.Now().Add(-time.Second) + case "unknown": + q.Set(oauth.QueryParamHandoff, "unknown") + case "duplicate code": + q.Add(oauth.QueryParamHandoff, "other") + case "empty code": + q.Set(oauth.QueryParamHandoff, "") + case "missing flow": + q.Del(queryParamDownloadFlow) + clearsCookie = false + case "duplicate flow": + q.Add(queryParamDownloadFlow, uuid.NewString()) + clearsCookie = false + case "invalid flow": + q.Set(queryParamDownloadFlow, "invalid") + clearsCookie = false + case "wrong flow": + q.Set(queryParamDownloadFlow, uuid.NewString()) + } + u.RawQuery = q.Encode() + r := httptest.NewRequest(http.MethodGet, u.String(), nil) + if cookie != nil { + r.AddCookie(cookie) + } + w := httptest.NewRecorder() + redirected, err := svc.authenticateUser(w, r) + assert.False(t, redirected) + assert.ErrorIs(t, err, errLoginHandoffExchange) + assert.Empty(t, r.Header.Get(authorizationKey)) + assert.Empty(t, w.Header().Get("Location")) + if code := q.Get(oauth.QueryParamHandoff); code != "" { + assert.NotContains(t, err.Error(), code) + } + if !clearsCookie { + // An invalid flow cannot select a cookie to clear. + assert.Empty(t, w.Result().Cookies()) + } else { + require.Len(t, w.Result().Cookies(), 1) + assert.Negative(t, w.Result().Cookies()[0].MaxAge) + } + }) + } +} + +func TestDownloadHeaderAndLegacyToken(t *testing.T) { + svc, _ := newDownloadTestService() + r := httptest.NewRequest(http.MethodGet, testDownloadBack, nil) + r.Header.Set(authorizationKey, "Bearer header-token") + w := httptest.NewRecorder() + redirected, err := svc.authenticateUser(w, r) + assert.NoError(t, err) + assert.False(t, redirected) + assert.Equal(t, "Bearer header-token", r.Header.Get(authorizationKey)) + assert.Empty(t, w.Result().Cookies()) + + for _, query := range []string{"t=user-token", "t=", "t=user-token&handoff=code", "bad=%zz"} { + r := httptest.NewRequest(http.MethodGet, "/download/sha256:abc?"+query, nil) + w := httptest.NewRecorder() + redirected, err := svc.authenticateUser(w, r) + assert.Error(t, err) + assert.False(t, redirected) + assert.Empty(t, r.Header.Get(authorizationKey)) + assert.Empty(t, w.Header().Get("Location")) + } +} + +func TestDownloadVerifierCookieSecurity(t *testing.T) { + for _, tc := range []struct { + name string + devMode bool + secure bool + }{ + {name: "production", secure: true}, + {name: "local HTTP development", devMode: true}, + } { + t.Run(tc.name, func(t *testing.T) { + svc, _ := newDownloadTestService() + svc.auth.SetDevMode(tc.devMode) + w := httptest.NewRecorder() + _, err := svc.authenticateUser(w, httptest.NewRequest(http.MethodGet, testDownloadBack, nil)) + require.NoError(t, err) + require.Len(t, w.Result().Cookies(), 1) + assert.Equal(t, tc.secure, w.Result().Cookies()[0].Secure) + assert.True(t, w.Result().Cookies()[0].HttpOnly) + }) + } +} + +func TestOverlappingDownloadHandoffs(t *testing.T) { + for _, tc := range []struct { + name string + order [2]int + failFirst bool + }{ + {name: "first callback first", order: [2]int{0, 1}}, + {name: "second callback first", order: [2]int{1, 0}}, + {name: "failed callback leaves other flow intact", order: [2]int{0, 1}, failFirst: true}, + } { + t.Run(tc.name, func(t *testing.T) { + svc, _ := newDownloadTestService() + jar, err := cookiejar.New(nil) + require.NoError(t, err) + origin, err := url.Parse("https://controlplane.example/download/") + require.NoError(t, err) + var callbacks [2]string + var cookies [2]*http.Cookie + var users [2]string + for i := range callbacks { + callbacks[i], cookies[i], users[i] = startDownloadLogin(t, svc) + if i == 0 && tc.failFirst { + cookies[i] = &http.Cookie{ + Name: cookies[i].Name, Value: strings.Repeat("x", 43), Path: cookies[i].Path, + Secure: true, HttpOnly: true, SameSite: http.SameSiteLaxMode, + } + } + jar.SetCookies(origin, []*http.Cookie{cookies[i]}) + } + require.NotEqual(t, cookies[0].Name, cookies[1].Name) + require.Len(t, jar.Cookies(origin), 2) + for _, i := range tc.order { + r := httptest.NewRequest(http.MethodGet, callbacks[i], nil) + for _, cookie := range jar.Cookies(origin) { + r.AddCookie(cookie) + } + w := httptest.NewRecorder() + redirected, err := svc.authenticateUser(w, r) + assert.False(t, redirected) + if i == 0 && tc.failFirst { + require.ErrorIs(t, err, errLoginHandoffExchange) + assert.Empty(t, r.Header.Get(authorizationKey)) + } else { + require.NoError(t, err) + claims := &user.CustomClaims{} + _, err := jwt.ParseWithClaims(strings.TrimPrefix(r.Header.Get(authorizationKey), "Bearer "), claims, func(*jwt.Token) (interface{}, error) { + return []byte(testHMACSecret), nil + }) + require.NoError(t, err) + assert.Equal(t, users[i], claims.UserID) + } + require.Len(t, w.Result().Cookies(), 1) + assert.Equal(t, cookies[i].Name, w.Result().Cookies()[0].Name) + jar.SetCookies(origin, w.Result().Cookies()) + } + assert.Empty(t, jar.Cookies(origin)) + }) + } +}