diff --git a/internal/cloud/auth/auth.go b/internal/cloud/auth/auth.go index 2d15b655..a663ed14 100644 --- a/internal/cloud/auth/auth.go +++ b/internal/cloud/auth/auth.go @@ -250,7 +250,7 @@ func (s *Service) Authorize(r *http.Request) error { if token == "" { return fmt.Errorf("bearer token is required") } - if token != s.expectedToken { + if !hmac.Equal([]byte(token), []byte(s.expectedToken)) { return fmt.Errorf("invalid bearer token") } return nil diff --git a/internal/cloud/auth/auth_test.go b/internal/cloud/auth/auth_test.go index 4e40cd75..a7b51820 100644 --- a/internal/cloud/auth/auth_test.go +++ b/internal/cloud/auth/auth_test.go @@ -252,6 +252,44 @@ func TestDashboardSessionTokenRejectsWhenConfiguredBearerChanges(t *testing.T) { } } +// TestAuthorizeBearerTokenConstantTimeComparison is a correctness guard for the +// constant-time token comparison at auth.go:253. A correct token must be accepted +// and any wrong token must be rejected, preserving behavior after the +// timing-safe hmac.Equal replacement (security issue #350). +func TestAuthorizeBearerTokenConstantTimeComparison(t *testing.T) { + svc, err := NewService(&cloudstore.CloudStore{}, strings.Repeat("x", 32)) + if err != nil { + t.Fatalf("new service: %v", err) + } + svc.SetBearerToken("correct-token") + + correct := httptest.NewRequest("GET", "/sync/pull", nil) + correct.Header.Set("Authorization", "Bearer correct-token") + if err := svc.Authorize(correct); err != nil { + t.Fatalf("correct token must be accepted, got %v", err) + } + + wrong := httptest.NewRequest("GET", "/sync/pull", nil) + wrong.Header.Set("Authorization", "Bearer wrong-token") + if err := svc.Authorize(wrong); err == nil { + t.Fatal("wrong token must be rejected") + } + + // A token that is a prefix of the correct one must also be rejected. + prefix := httptest.NewRequest("GET", "/sync/pull", nil) + prefix.Header.Set("Authorization", "Bearer correct-toke") + if err := svc.Authorize(prefix); err == nil { + t.Fatal("prefix of correct token must be rejected") + } + + // A token that is a superset of the correct one must also be rejected. + super := httptest.NewRequest("GET", "/sync/pull", nil) + super.Header.Set("Authorization", "Bearer correct-token-extra") + if err := svc.Authorize(super); err == nil { + t.Fatal("superset of correct token must be rejected") + } +} + func TestDashboardSessionTokenSupportsAdditionalDashboardCredential(t *testing.T) { svc, err := NewService(&cloudstore.CloudStore{}, strings.Repeat("x", 32)) if err != nil { diff --git a/internal/cloud/cloudserver/cloudserver.go b/internal/cloud/cloudserver/cloudserver.go index 360b1411..6c4c8ca2 100644 --- a/internal/cloud/cloudserver/cloudserver.go +++ b/internal/cloud/cloudserver/cloudserver.go @@ -2,6 +2,7 @@ package cloudserver import ( "context" + "crypto/hmac" "encoding/json" "errors" "fmt" @@ -157,7 +158,7 @@ func (s *CloudServer) routes() { if token == "" { return fmt.Errorf("bearer token is required") } - if adminToken := strings.TrimSpace(s.dashboardAdmin); adminToken != "" && token == adminToken { + if adminToken := strings.TrimSpace(s.dashboardAdmin); adminToken != "" && hmac.Equal([]byte(token), []byte(adminToken)) { return nil } if s.auth == nil { @@ -258,7 +259,7 @@ func (s *CloudServer) authorizeDashboardRequest(r *http.Request) error { if strings.TrimSpace(bearerToken) == "" { return fmt.Errorf("dashboard session token is empty") } - if adminToken := strings.TrimSpace(s.dashboardAdmin); adminToken != "" && bearerToken == adminToken { + if adminToken := strings.TrimSpace(s.dashboardAdmin); adminToken != "" && hmac.Equal([]byte(bearerToken), []byte(adminToken)) { return nil } req, _ := http.NewRequest(http.MethodGet, "/dashboard", nil) @@ -317,7 +318,7 @@ func (s *CloudServer) isDashboardAdmin(r *http.Request) bool { if err != nil { return false } - return token == adminToken + return hmac.Equal([]byte(token), []byte(adminToken)) } func (s *CloudServer) handlePullManifest(w http.ResponseWriter, r *http.Request) { diff --git a/internal/cloud/cloudserver/cloudserver_test.go b/internal/cloud/cloudserver/cloudserver_test.go index cdc84dba..a1d79339 100644 --- a/internal/cloud/cloudserver/cloudserver_test.go +++ b/internal/cloud/cloudserver/cloudserver_test.go @@ -1497,6 +1497,105 @@ func TestAuditLogE2E_MutationPushPausedThenListRendered(t *testing.T) { } } +// TestValidateLoginTokenAdminComparisonGuard is a correctness guard for the +// constant-time admin token comparison inside validateLoginToken (cloudserver.go:160). +// A correct admin token must be accepted and a wrong one rejected, preserving +// behavior after the timing-safe hmac.Equal replacement (security issue #350). +func TestValidateLoginTokenAdminComparisonGuard(t *testing.T) { + authSvc, err := cloudauth.NewService(&cloudstore.CloudStore{}, strings.Repeat("x", 32)) + if err != nil { + t.Fatalf("new auth service: %v", err) + } + authSvc.SetBearerToken("sync-token") + authSvc.SetDashboardSessionTokens([]string{"admin-token"}) + srv := New(&fakeStore{}, authSvc, 0, WithDashboardAdminToken("admin-token")) + + // Correct admin token must authenticate and redirect to dashboard. + goodLogin := httptest.NewRecorder() + goodReq := httptest.NewRequest(http.MethodPost, "/dashboard/login", strings.NewReader("token=admin-token")) + goodReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + srv.Handler().ServeHTTP(goodLogin, goodReq) + if goodLogin.Code != http.StatusSeeOther { + t.Fatalf("correct admin token must authenticate, got %d body=%q", goodLogin.Code, goodLogin.Body.String()) + } + + // Wrong token must be rejected (re-render login form). + badLogin := httptest.NewRecorder() + badReq := httptest.NewRequest(http.MethodPost, "/dashboard/login", strings.NewReader("token=wrong-token")) + badReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + srv.Handler().ServeHTTP(badLogin, badReq) + if badLogin.Code == http.StatusSeeOther { + t.Fatalf("wrong admin token must be rejected, got %d", badLogin.Code) + } + for _, cookie := range badLogin.Result().Cookies() { + if cookie.Name == dashboardSessionCookieName { + t.Fatal("wrong admin token must not set session cookie") + } + } +} + +// TestIsDashboardAdminComparisonGuard is a correctness guard for the +// constant-time admin token comparison inside isDashboardAdmin (cloudserver.go:320). +// A valid admin session cookie must grant admin access and a wrong one must not, +// preserving behavior after the timing-safe hmac.Equal replacement (security issue #350). +func TestIsDashboardAdminComparisonGuard(t *testing.T) { + authSvc, err := cloudauth.NewService(&cloudstore.CloudStore{}, strings.Repeat("x", 32)) + if err != nil { + t.Fatalf("new auth service: %v", err) + } + authSvc.SetBearerToken("sync-token") + authSvc.SetDashboardSessionTokens([]string{"admin-token"}) + srv := New(&fakeStore{}, authSvc, 0, WithDashboardAdminToken("admin-token")) + + // Establish a valid admin session cookie via login. + loginRec := httptest.NewRecorder() + loginReq := httptest.NewRequest(http.MethodPost, "/dashboard/login", strings.NewReader("token=admin-token")) + loginReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + srv.Handler().ServeHTTP(loginRec, loginReq) + if loginRec.Code != http.StatusSeeOther { + t.Fatalf("setup: expected admin login to succeed, got %d body=%q", loginRec.Code, loginRec.Body.String()) + } + adminCookies := loginRec.Result().Cookies() + + // Admin session must grant access to /dashboard/admin. + adminRec := httptest.NewRecorder() + adminReq := httptest.NewRequest(http.MethodGet, "/dashboard/admin", nil) + for _, c := range adminCookies { + adminReq.AddCookie(c) + } + srv.Handler().ServeHTTP(adminRec, adminReq) + if adminRec.Code != http.StatusOK { + t.Fatalf("valid admin session must access /dashboard/admin, got %d body=%q", adminRec.Code, adminRec.Body.String()) + } + + // Non-admin session (sync-token login) must not access /dashboard/admin. + authSvc2, err := cloudauth.NewService(&cloudstore.CloudStore{}, strings.Repeat("x", 32)) + if err != nil { + t.Fatalf("new auth service 2: %v", err) + } + authSvc2.SetBearerToken("sync-token") + authSvc2.SetDashboardSessionTokens([]string{"admin-token"}) + srv2 := New(&fakeStore{}, authSvc2, 0, WithDashboardAdminToken("admin-token")) + + syncLoginRec := httptest.NewRecorder() + syncLoginReq := httptest.NewRequest(http.MethodPost, "/dashboard/login", strings.NewReader("token=sync-token")) + syncLoginReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + srv2.Handler().ServeHTTP(syncLoginRec, syncLoginReq) + if syncLoginRec.Code != http.StatusSeeOther { + t.Fatalf("setup: sync-token login must succeed, got %d body=%q", syncLoginRec.Code, syncLoginRec.Body.String()) + } + + nonAdminRec := httptest.NewRecorder() + nonAdminReq := httptest.NewRequest(http.MethodGet, "/dashboard/admin", nil) + for _, c := range syncLoginRec.Result().Cookies() { + nonAdminReq.AddCookie(c) + } + srv2.Handler().ServeHTTP(nonAdminRec, nonAdminReq) + if nonAdminRec.Code == http.StatusOK { + t.Fatalf("non-admin session must not access /dashboard/admin, got %d", nonAdminRec.Code) + } +} + // TestInsecureModeLoginRedirects asserts that GET /dashboard/login with auth==nil // returns 303 to /dashboard/ (login is a no-op in insecure mode). Satisfies REQ-110. func TestInsecureModeLoginRedirects(t *testing.T) {