Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cloud/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions internal/cloud/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Comment on lines +256 to +258
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 {
Expand Down
7 changes: 4 additions & 3 deletions internal/cloud/cloudserver/cloudserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cloudserver

import (
"context"
"crypto/hmac"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down
99 changes: 99 additions & 0 deletions internal/cloud/cloudserver/cloudserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down