From 4328d25f22412f8c8553063fd4dee9061ec1545f Mon Sep 17 00:00:00 2001 From: Tyler Koenig Date: Fri, 12 Jun 2026 12:45:16 -0400 Subject: [PATCH] fix(security): API import no longer replaces user accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cluster-secret holder could POST a backup with their own admin key to /api/backup/import, replacing all users — privilege escalation from cluster-auth to admin. Also, Kuma imports produced zero users but ImportWipe unconditionally deleted the users table — locking out all accounts until restart reseeded UPTOP_ADMIN_KEY. - Server handlers strip data.Users (set nil) before calling ImportData - ImportData only wipes+replaces users when data.Users != nil - New ImportWipeUsers dialect method separates user wipe from data wipe - CLI restore (main.go) unchanged — full import still replaces users --- internal/server/server.go | 5 ++++- internal/store/dialect.go | 1 + internal/store/postgres.go | 9 ++++++--- internal/store/sqlite.go | 15 +++++++++------ internal/store/sqlstore.go | 11 ++++++++--- internal/store/sqlstore_test.go | 25 +++++++++++++++++++++++++ 6 files changed, 53 insertions(+), 13 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index cac166e..8f7cbd8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -205,12 +205,15 @@ func (s *Server) handleImport(w http.ResponseWriter, r *http.Request) { http.Error(w, "Invalid JSON", http.StatusBadRequest) return } + // API import never modifies users — cluster-secret holder shouldn't be + // able to replace admin accounts. CLI restore still does full import. + data.Users = nil if err := s.store.ImportData(r.Context(), data); err != nil { slog.Error("import failed", "err", err) http.Error(w, "Import failed", http.StatusInternalServerError) return } - _, _ = w.Write([]byte("Import Successful")) + _, _ = w.Write([]byte("Import Successful (users excluded — manage via CLI or UPTOP_KEYS)")) } func (s *Server) handleKumaImport(w http.ResponseWriter, r *http.Request) { diff --git a/internal/store/dialect.go b/internal/store/dialect.go index 81aa157..9205345 100644 --- a/internal/store/dialect.go +++ b/internal/store/dialect.go @@ -18,6 +18,7 @@ type Dialect interface { BoolFalse() string ResetSequenceOnEmpty(db *sql.DB, table string) ImportWipe(tx *sql.Tx) + ImportWipeUsers(tx *sql.Tx) ImportResetSequences(tx *sql.Tx) UpsertNodeSQL() string UpsertAlertHealthSQL() string diff --git a/internal/store/postgres.go b/internal/store/postgres.go index 125d3ea..665bbf0 100644 --- a/internal/store/postgres.go +++ b/internal/store/postgres.go @@ -138,9 +138,6 @@ func (d *PostgresDialect) ImportWipe(tx *sql.Tx) { if _, err := tx.Exec("TRUNCATE TABLE alerts RESTART IDENTITY CASCADE"); err != nil { slog.Debug("import wipe failed", "table", "alerts", "err", err) } - if _, err := tx.Exec("TRUNCATE TABLE users RESTART IDENTITY CASCADE"); err != nil { - slog.Debug("import wipe failed", "table", "users", "err", err) - } if _, err := tx.Exec("TRUNCATE TABLE maintenance_windows RESTART IDENTITY CASCADE"); err != nil { slog.Debug("import wipe failed", "table", "maintenance_windows", "err", err) } @@ -155,6 +152,12 @@ func (d *PostgresDialect) ImportWipe(tx *sql.Tx) { } } +func (d *PostgresDialect) ImportWipeUsers(tx *sql.Tx) { + if _, err := tx.Exec("TRUNCATE TABLE users RESTART IDENTITY CASCADE"); err != nil { + slog.Debug("import wipe failed", "table", "users", "err", err) + } +} + func (d *PostgresDialect) ImportResetSequences(tx *sql.Tx) { if _, err := tx.Exec("SELECT setval('sites_id_seq', (SELECT COALESCE(MAX(id), 1) FROM sites))"); err != nil { slog.Debug("sequence reset failed", "table", "sites", "err", err) diff --git a/internal/store/sqlite.go b/internal/store/sqlite.go index 24c7498..7aa8057 100644 --- a/internal/store/sqlite.go +++ b/internal/store/sqlite.go @@ -167,12 +167,6 @@ func (d *SQLiteDialect) ImportWipe(tx *sql.Tx) { if _, err := tx.Exec("DELETE FROM sqlite_sequence WHERE name='alerts'"); err != nil { slog.Debug("import wipe failed", "table", "sqlite_sequence(alerts)", "err", err) } - if _, err := tx.Exec("DELETE FROM users"); err != nil { - slog.Debug("import wipe failed", "table", "users", "err", err) - } - if _, err := tx.Exec("DELETE FROM sqlite_sequence WHERE name='users'"); err != nil { - slog.Debug("import wipe failed", "table", "sqlite_sequence(users)", "err", err) - } if _, err := tx.Exec("DELETE FROM maintenance_windows"); err != nil { slog.Debug("import wipe failed", "table", "maintenance_windows", "err", err) } @@ -190,4 +184,13 @@ func (d *SQLiteDialect) ImportWipe(tx *sql.Tx) { } } +func (d *SQLiteDialect) ImportWipeUsers(tx *sql.Tx) { + if _, err := tx.Exec("DELETE FROM users"); err != nil { + slog.Debug("import wipe failed", "table", "users", "err", err) + } + if _, err := tx.Exec("DELETE FROM sqlite_sequence WHERE name='users'"); err != nil { + slog.Debug("import wipe failed", "table", "sqlite_sequence(users)", "err", err) + } +} + func (d *SQLiteDialect) ImportResetSequences(tx *sql.Tx) {} diff --git a/internal/store/sqlstore.go b/internal/store/sqlstore.go index e322710..0440bee 100644 --- a/internal/store/sqlstore.go +++ b/internal/store/sqlstore.go @@ -742,9 +742,14 @@ func (s *SQLStore) ImportData(ctx context.Context, data models.Backup) error { s.dialect.ImportWipe(tx) - for _, u := range data.Users { - if _, err := tx.ExecContext(ctx, s.q("INSERT INTO users (username, public_key, role) VALUES (?, ?, ?)"), u.Username, u.PublicKey, u.Role); err != nil { - return err + // Only wipe+replace users when callers explicitly provide them (CLI + // full restore). API/Kuma imports pass nil — existing users preserved. + if data.Users != nil { + s.dialect.ImportWipeUsers(tx) + for _, u := range data.Users { + if _, err := tx.ExecContext(ctx, s.q("INSERT INTO users (username, public_key, role) VALUES (?, ?, ?)"), u.Username, u.PublicKey, u.Role); err != nil { + return err + } } } for _, a := range data.Alerts { diff --git a/internal/store/sqlstore_test.go b/internal/store/sqlstore_test.go index 87b5355..01dbffe 100644 --- a/internal/store/sqlstore_test.go +++ b/internal/store/sqlstore_test.go @@ -276,6 +276,31 @@ func TestImportData_WipesHistory(t *testing.T) { } } +func TestImportData_NilUsersPreservesExisting(t *testing.T) { + s := newTestStore(t) + + if err := s.AddUser(context.Background(), "admin", "ssh-ed25519 ADMINKEY", "admin"); err != nil { + t.Fatalf("AddUser: %v", err) + } + + backup := models.Backup{ + Sites: []models.SiteConfig{{ID: 1, Name: "New", URL: "https://new.com", Type: "http", Interval: 30}}, + Alerts: []models.AlertConfig{{ID: 1, Name: "a", Type: "webhook", Settings: map[string]string{"url": "https://h.com"}}}, + Users: nil, + } + if err := s.ImportData(context.Background(), backup); err != nil { + t.Fatalf("ImportData: %v", err) + } + + users, err := s.GetAllUsers(context.Background()) + if err != nil { + t.Fatalf("GetAllUsers: %v", err) + } + if len(users) != 1 || users[0].Username != "admin" { + t.Errorf("expected existing admin user preserved, got %d users", len(users)) + } +} + func TestCheckHistory(t *testing.T) { s := newTestStore(t)