From 0358efe95b0288b8854193ee29f15a15fce057ad Mon Sep 17 00:00:00 2001 From: Jon Chery Date: Mon, 10 Aug 2026 12:27:05 +0000 Subject: [PATCH] =?UTF-8?q?fix(P07):=20concurrency=20safety=20=E2=80=94=20?= =?UTF-8?q?SQLite,=20flock,=20cache,=20atomic=20writes=20(REQ-156)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SQLite busy_timeout(5000) + SetMaxOpenConns(1) on all 4 DSNs - secrets file flock (concurrent set on same ns no longer loses data) - upgrade lock file (refuse concurrent orca upgrade) - backup lock file (refuse concurrent backup) - cache invalidation by writes (read-after-write consistency) - Executor.Run mutex scope fix (hold only for DB inserts) - ns create/inherit/set-constraint atomic writeNSMdAtomic - writeCurrentLead + rotateSSHKeys atomic - consolidate 3 writeAtomic impls onto security.WriteAtomic - WebAuthn session stores guarded with sync.Mutex Tests: concurrent secrets set, upgrade lock rejection, cache read-after-write, WebAuthn session thread-safety (pass under -race). ---ci--- project: orca phase: 7 milestone: v0.13 status: complete requirements: covered: [156] ---/ci--- --- internal/cache/cache.go | 9 +- internal/cli/acl.go | 33 +- internal/cli/backup.go | 35 ++ internal/cli/cache.go | 21 + internal/cli/concurrency_test.go | 422 ++++++++++++++++++ internal/cli/job.go | 10 + internal/cli/node.go | 9 + internal/cli/ns.go | 25 +- internal/cli/recovery.go | 7 +- internal/cli/rotate_lead.go | 15 +- internal/cli/secrets.go | 57 ++- internal/cli/upgrade.go | 47 +- internal/engine/executor.go | 33 +- internal/identity/oidc.go | 20 +- internal/store/store.go | 5 + internal/webauthn/connector.go | 67 ++- .../webauthn/connector_concurrency_test.go | 179 ++++++++ internal/webauthn/store.go | 6 +- 18 files changed, 932 insertions(+), 68 deletions(-) create mode 100644 internal/cli/concurrency_test.go create mode 100644 internal/webauthn/connector_concurrency_test.go diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 5fe94dc..f22e98e 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -58,10 +58,17 @@ func Open(path string) (*Cache, error) { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return nil, fmt.Errorf("create cache db dir: %w", err) } - db, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)") + // REQ-156 / P07 T1: busy_timeout(5000) so concurrent cache opens + // (e.g. two `orca node list` invocations racing on the same shell) + // wait up to 5s for the writer instead of failing immediately with + // SQLITE_BUSY. SetMaxOpenConns(1) serializes the connections so the + // busy_timeout is rarely needed but keeps the cache durable under + // contention. + db, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)") if err != nil { return nil, fmt.Errorf("open cache sqlite: %w", err) } + db.SetMaxOpenConns(1) if err := db.Ping(); err != nil { _ = db.Close() return nil, fmt.Errorf("ping cache sqlite: %w", err) diff --git a/internal/cli/acl.go b/internal/cli/acl.go index 2bd4e08..deba309 100644 --- a/internal/cli/acl.go +++ b/internal/cli/acl.go @@ -175,32 +175,15 @@ func lockACL() (func(), error) { return security.Flock(paths.ACLPath() + ".lock") } -// writeAtomicFile writes data to a temp file in dir(path) and renames -// it into place, matching the security.WriteAtomic pattern (P02 keeps -// a local copy to avoid importing internal/security into the CLI). +// writeAtomicFile writes data atomically (REQ-156, P07 T9). +// Previously a local copy of the temp+chmod+rename pattern (P02 kept a +// local copy to avoid importing internal/security); it lacked fsync, +// so a crash between write and rename could promote a partially-durable +// file. Now a thin wrapper around the canonical security.WriteAtomic +// (temp + chmod + fsync + rename) so all CLI atomic writes share one +// fsync-correct implementation. func writeAtomicFile(path string, data []byte, mode os.FileMode) error { - dir := filepath.Dir(path) - tmp, err := os.CreateTemp(dir, ".acl-tmp-*") - if err != nil { - return fmt.Errorf("create temp: %w", err) - } - tmpName := tmp.Name() - defer func() { _ = os.Remove(tmpName) }() - if _, err := tmp.Write(data); err != nil { - _ = tmp.Close() - return fmt.Errorf("write temp: %w", err) - } - if err := tmp.Chmod(mode); err != nil { - _ = tmp.Close() - return fmt.Errorf("chmod temp: %w", err) - } - if err := tmp.Close(); err != nil { - return fmt.Errorf("close temp: %w", err) - } - if err := os.Rename(tmpName, path); err != nil { - return fmt.Errorf("rename temp: %w", err) - } - return nil + return security.WriteAtomic(path, mode, data) } var aclGrantCmd = &cobra.Command{ diff --git a/internal/cli/backup.go b/internal/cli/backup.go index 687221a..339a5a6 100644 --- a/internal/cli/backup.go +++ b/internal/cli/backup.go @@ -12,6 +12,8 @@ package cli import ( "fmt" + "os" + "path/filepath" "time" "github.com/spf13/cobra" @@ -29,6 +31,31 @@ var ( restoreDryRun bool ) +// acquireBackupLock atomically creates an exclusive lock file at +// paths.ClusterDir()/backup.lock (REQ-156, P07 T4). Returns a release +// function that MUST be deferred (it removes the lock file). If the +// lock file already exists, returns an error "backup already in +// progress" — preventing two concurrent `orca backup` invocations +// from racing on the same ORCA_HOME (two tarballs being written from +// the same source tree could produce inconsistent archives). O_CREATE +// |O_EXCL is atomic under POSIX. +func acquireBackupLock() (func(), error) { + lockPath := filepath.Join(paths.ClusterDir(), "backup.lock") + if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + return nil, fmt.Errorf("create cluster dir for backup lock: %w", err) + } + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + if os.IsExist(err) { + return nil, fmt.Errorf("backup already in progress (lock file %s exists; remove it if stale)", lockPath) + } + return nil, fmt.Errorf("acquire backup lock: %w", err) + } + _, _ = f.WriteString(fmt.Sprintf("pid=%d started=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339))) + _ = f.Close() + return func() { _ = os.Remove(lockPath) }, nil +} + var backupCmd = &cobra.Command{ Use: "backup", Short: "Create a signed tar.gz backup of ORCA_HOME", @@ -44,6 +71,14 @@ written to --out; the hex-encoded signature to --out + ".sig".`, if err != nil { return fmt.Errorf("load master key: %w", err) } + // REQ-156 / P07 T4: acquire an exclusive backup lock so two + // concurrent `orca backup` invocations don't race on the same + // ORCA_HOME (producing interleaved / inconsistent archives). + backupRelease, err := acquireBackupLock() + if err != nil { + return err + } + defer backupRelease() out := backupOutPath if out == "" { ts := time.Now().UTC().Format("20060102-150405") diff --git a/internal/cli/cache.go b/internal/cli/cache.go index 5e31ed5..36ade65 100644 --- a/internal/cli/cache.go +++ b/internal/cli/cache.go @@ -101,6 +101,27 @@ func cachePutList(class, key string, list any, ttl time.Duration) { cachePopulate(class, key, val, ttl) } +// cacheInvalidate drops all entries for the given cache class +// (REQ-156, P07 T5). It is called after write operations (node +// join/leave, ns create/delete, job run/stop) so the very next read +// does not surface a stale cached list. Errors are logged but never +// returned — a failed invalidation must not break the write command +// (the cache entry will simply expire at its TTL). +func cacheInvalidate(class string) { + if !cacheAvailable() { + return + } + c, err := cache.Open(paths.CacheDB()) + if err != nil { + slog.Warn("cache: open failed during invalidate", "class", class, "err", err) + return + } + defer c.Close() + if err := c.Invalidate(class); err != nil { + slog.Warn("cache: invalidate failed", "class", class, "err", err) + } +} + // Per-class TTLs (P00-T2). const ( cacheNodeTTL = 30 * time.Second diff --git a/internal/cli/concurrency_test.go b/internal/cli/concurrency_test.go new file mode 100644 index 0000000..308b1a2 --- /dev/null +++ b/internal/cli/concurrency_test.go @@ -0,0 +1,422 @@ +package cli + +// concurrency_test.go covers the REQ-156 / P07 concurrency-safety +// fixes: +// +// - T11: concurrent `secrets set` on the same namespace preserves all +// keys (the flock serializes the read-modify-write so no key is +// lost to a clobbering second writer). +// - T12: a second `orca upgrade` invoked while the first is running +// is rejected with "upgrade already in progress". +// - T13: cache invalidation read-after-write - `node join` followed +// by an immediate `node list` (with a populated stale cache) shows +// the new node, not the stale cached list. +// - T14: (in internal/webauthn) concurrent BeginRegistration does +// not panic / race on the session map. +// +// These tests complement the per-fix unit tests in the relevant +// _test.go files; they specifically exercise the cross-cutting +// concurrency invariants the milestone hardens. + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "git.cloudinit.dev/coreci/orca/internal/cache" + "git.cloudinit.dev/coreci/orca/internal/paths" + "git.cloudinit.dev/coreci/orca/internal/secrets" +) + +// runCLI is a helper that resets root flags, wires a fresh output +// buffer, sets the given args, and runs rootCmd. Returns the captured +// output. The buffer must be wired AFTER resetRootFlags (which sets +// its own buffer). +func runCLI(t *testing.T, args ...string) (string, error) { + t.Helper() + resetRootFlags(t) + var buf bytes.Buffer + rootCmd.SetOut(&buf) + rootCmd.SetErr(&buf) + rootCmd.SetArgs(args) + err := rootCmd.Execute() + return buf.String(), err +} + +// --------------------------------------------------------------------------- +// T11: concurrent secrets set preserves all keys +// --------------------------------------------------------------------------- + +// TestSecretsConcurrentSetPreservesAllKeys runs 5 concurrent +// `orca secrets set` invocations against the SAME namespace, each +// setting a distinct key. Without the flock (P07 T2) the second writer +// would load-then-save and clobber the first, losing a key. With the +// flock all 5 keys must be present afterward. +// +// The cobra rootCmd is a package global and is NOT goroutine-safe +// (shared flag state), so we drive the secrets-set RunE body directly +// under real concurrency. This exercises the lockNSSecrets flock + +// loadMasterAndNSSecrets + saveNSSecrets path that the RunE uses. +func TestSecretsConcurrentSetPreservesAllKeys(t *testing.T) { + ns := "concsetns" + setupSecretsTestEnv(t, ns) + + const n = 5 + keys := make([]string, n) + for i := 0; i < n; i++ { + keys[i] = fmt.Sprintf("KEY_%d", i) + } + + var wg sync.WaitGroup + errs := make([]error, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + // Replicate the secretsSetCmd RunE body under real + // concurrency: lock -> load -> mutate -> save. The lock + // serializes the read-modify-write so concurrent sets do + // not clobber each other. + release, err := lockNSSecrets(ns) + if err != nil { + errs[idx] = fmt.Errorf("lock: %w", err) + return + } + defer release() + nsKey, lines, err := loadMasterAndNSSecrets(ns) + if err != nil { + errs[idx] = err + return + } + defer secrets.ZeroKey(nsKey) + key := keys[idx] + value := fmt.Sprintf("value_%d", idx) + newLine := key + "=" + value + j := findKeyIndex(lines, key) + if j >= 0 { + lines[j] = newLine + } else { + lines = append(lines, newLine) + } + errs[idx] = saveNSSecrets(ns, nsKey, lines) + }(i) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("goroutine %d: %v", i, err) + } + } + + // All 5 keys must be present. + out, err := runCLI(t, "secrets", "list", ns) + if err != nil { + t.Fatalf("secrets list: %v", err) + } + for _, k := range keys { + if !strings.Contains(out, k) { + t.Errorf("key %q missing after concurrent set (flock did not serialize): %s", k, out) + } + } +} + +// TestSecretsConcurrentSetViaCLI is the cobra-driven variant. cobra's +// rootCmd is not goroutine-safe (shared flag globals), so we serialize +// the Execute() calls. This still exercises the flock because the +// load+save happens inside RunE. Confirms the CLI path itself (with +// flock) does not lose keys under repeated serial sets. +func TestSecretsConcurrentSetViaCLI(t *testing.T) { + ns := "conccli" + setupSecretsTestEnv(t, ns) + + const n = 5 + for i := 0; i < n; i++ { + if _, err := runCLI(t, "secrets", "set", ns, fmt.Sprintf("K_%d=v_%d", i, i)); err != nil { + t.Fatalf("secrets set %d: %v", i, err) + } + } + out, err := runCLI(t, "secrets", "list", ns) + if err != nil { + t.Fatalf("secrets list: %v", err) + } + for i := 0; i < n; i++ { + k := fmt.Sprintf("K_%d", i) + if !strings.Contains(out, k) { + t.Errorf("key %q missing after serial CLI sets: %s", k, out) + } + } +} + +// --------------------------------------------------------------------------- +// T12: concurrent upgrade rejection +// --------------------------------------------------------------------------- + +// TestUpgradeConcurrentLockRejected verifies that a second upgrade +// invocation while the first holds the upgrade.lock is rejected with +// "upgrade already in progress". +func TestUpgradeConcurrentLockRejected(t *testing.T) { + setupUpgradeTest(t) + resetUpgradeFlags() + + // Manually create the upgrade.lock as if a first upgrade is in + // progress (the lock file content is just diagnostic; its + // EXISTENCE is what blocks the second caller via O_CREATE|O_EXCL). + lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock") + if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + t.Fatalf("mkdir cluster: %v", err) + } + if err := os.WriteFile(lockPath, []byte("pid=999 started=2026-01-01T00:00:00Z\n"), 0o600); err != nil { + t.Fatalf("write lock: %v", err) + } + defer os.Remove(lockPath) + + // A dry-run upgrade must now be rejected because the lock exists. + _, err := runCLI(t, "upgrade", "--to", "v0.11.0", "--dry-run") + if err == nil { + t.Fatal("upgrade with stale lock should fail, got nil") + } + if !strings.Contains(err.Error(), "upgrade already in progress") { + t.Errorf("unexpected error: %v", err) + } +} + +// TestUpgradeLockReleasedOnSuccess verifies the upgrade.lock is +// removed after a successful (dry-run) upgrade so a subsequent upgrade +// is not blocked by a stale lock. +func TestUpgradeLockReleasedOnSuccess(t *testing.T) { + setupUpgradeTest(t) + setupUpgradeTestWithMocks(t) + rootCmd.SetArgs([]string{"upgrade", "--to", "v0.11.0", "--dry-run"}) + if err := rootCmd.Execute(); err != nil { + t.Fatalf("upgrade dry-run: %v", err) + } + lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock") + if _, err := os.Stat(lockPath); err == nil { + t.Errorf("upgrade.lock still exists after successful dry-run (not released): %s", lockPath) + } +} + +// TestUpgradeLockReleasedOnError verifies the lock is released even +// when the upgrade fails mid-run (the defer in runUpgrade covers the +// error path). +func TestUpgradeLockReleasedOnError(t *testing.T) { + setupUpgradeTest(t) + setupUpgradeTestWithMocks(t) + // Force a failure: --to with a version that triggers a cutover + // whose verification fails. The runner reports :443 (cutover + // needed) and the http check returns 502 (verification fail). + runner := &mockUpgradeRunner{ + outputs: map[string][]byte{ + "ss -tlnp": []byte(":443"), + }, + } + upgradeRunnerOverride = runner + httpClientOverride = func(url string) (int, error) { return 502, nil } + rootCmd.SetArgs([]string{"upgrade", "--to", "v0.11.0"}) + _ = rootCmd.Execute() // expected to fail + lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock") + if _, err := os.Stat(lockPath); err == nil { + t.Errorf("upgrade.lock still exists after failed upgrade (not released on error): %s", lockPath) + } +} + +// --------------------------------------------------------------------------- +// T13: cache invalidation read-after-write +// --------------------------------------------------------------------------- + +// TestCacheInvalidationNodeJoinReadAfterWrite verifies that after +// `node join` invalidates the `nodes` cache class, an immediate +// `node list` (which would otherwise serve a STALE cached list) shows +// the just-joined node. +// +// Setup: populate the cache with a stale nodes list (missing the new +// node). Without T5's invalidation, the second `node list` would serve +// the stale list and the new node would be invisible until the TTL +// expired. With T5, the join invalidates the class and the list +// re-reads from the DB. +func TestCacheInvalidationNodeJoinReadAfterWrite(t *testing.T) { + _, cleanup := initTestEnv(t) + defer cleanup() + + // Seed the cache with a stale nodes list (a sentinel node that + // does NOT exist in the DB). The TTL is long so it would be + // served on a subsequent list without invalidation. + c, err := cache.Open(paths.CacheDB()) + if err != nil { + t.Fatalf("open cache: %v", err) + } + stale := `[{"id":"stale-id","name":"stale-node","address":"10.0.0.99:8443","state":"ready"}]` + if err := c.Set(cacheNodeClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil { + t.Fatalf("set stale cache: %v", err) + } + c.Close() + + // Confirm the stale entry is served by a fresh list (proving the + // cache is populated and would be hit). + staleOut, err := runCLI(t, "node", "list") + if err != nil { + t.Fatalf("stale node list: %v", err) + } + if !strings.Contains(staleOut, "stale-node") { + t.Fatalf("precondition: stale cache not served: %s", staleOut) + } + + // Join a real node. T5 invalidates the `nodes` cache class. + if _, err := runCLI(t, "node", "join", "--name", "freshnode", "--addr", "10.0.0.42:8443"); err != nil { + t.Fatalf("node join: %v", err) + } + + // Immediate list: the stale sentinel must be GONE (invalidated) + // and the real fresh node must be present (read from the DB). + out, err := runCLI(t, "node", "list") + if err != nil { + t.Fatalf("node list after join: %v", err) + } + if strings.Contains(out, "stale-node") { + t.Errorf("stale cache still served after join (invalidation missing): %s", out) + } + if !strings.Contains(out, "freshnode") { + t.Errorf("fresh node missing from list after join (cache not re-read): %s", out) + } +} + +// TestCacheInvalidationNSCreateReadAfterWrite is the ns variant: a +// stale `namespaces` cache is invalidated by `ns create` so the next +// `ns list` shows the new namespace. +func TestCacheInvalidationNSCreateReadAfterWrite(t *testing.T) { + root := t.TempDir() + t.Setenv("ORCA_HOME", root) + writeDefaultsNS(t, root) + + // Seed a stale namespaces cache containing only _defaults. + c, err := cache.Open(paths.CacheDB()) + if err != nil { + t.Fatalf("open cache: %v", err) + } + stale := `[{"name":"_defaults","path":"` + filepath.Join(root, "_defaults") + `","default":true}]` + if err := c.Set(cacheNamespaceClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil { + t.Fatalf("set stale: %v", err) + } + c.Close() + + // Confirm stale served. + resetRootFlags(t) + resetNSFlags() + staleOut, err := runCLI(t, "ns", "list") + if err != nil { + t.Fatalf("stale ns list: %v", err) + } + if !strings.Contains(staleOut, "_defaults") { + t.Fatalf("precondition: stale ns cache not served: %s", staleOut) + } + + // Create a new namespace. T5 invalidates the `namespaces` cache. + resetRootFlags(t) + resetNSFlags() + if _, err := runCLI(t, "ns", "create", "newns"); err != nil { + t.Fatalf("ns create: %v", err) + } + + // Immediate list: must show the new namespace (read from disk, + // not the stale cache). + resetRootFlags(t) + resetNSFlags() + out, err := runCLI(t, "ns", "list") + if err != nil { + t.Fatalf("ns list after create: %v", err) + } + if !strings.Contains(out, "newns") { + t.Errorf("new namespace missing from list after create (cache not invalidated/re-read): %s", out) + } +} + +// TestCacheInvalidationJobRunReadAfterWrite verifies `job run` +// invalidates the `jobs` cache so a stale cached job list is not +// served after a new job runs. +func TestCacheInvalidationJobRunReadAfterWrite(t *testing.T) { + _, cleanup := initTestEnv(t) + defer cleanup() + + // Seed a stale jobs cache (a sentinel job that does not exist). + c, err := cache.Open(paths.CacheDB()) + if err != nil { + t.Fatalf("open cache: %v", err) + } + stale := `[{"id":"stale-job","name":"stale","status":"complete","exit_code":0}]` + if err := c.Set(cacheJobClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil { + t.Fatalf("set stale: %v", err) + } + c.Close() + + // Confirm stale served. + staleOut, err := runCLI(t, "job", "list") + if err != nil { + t.Fatalf("stale job list: %v", err) + } + if !strings.Contains(staleOut, "stale") { + t.Fatalf("precondition: stale job cache not served: %s", staleOut) + } + + // Write a job spec and run it. T5 invalidates the `jobs` cache. + specDir := t.TempDir() + specPath := filepath.Join(specDir, "job.md") + specBody := "---\n" + + "kind: Job\n" + + "name: cacheinv-job\n" + + "runtime:\n" + + " one_of: process\n" + + " command: /bin/true\n" + + "---\n# cacheinv\n\nRuns /bin/true.\n" + if err := os.WriteFile(specPath, []byte(specBody), 0o644); err != nil { + t.Fatalf("write spec: %v", err) + } + if _, err := runCLI(t, "job", "run", specPath); err != nil { + t.Fatalf("job run: %v", err) + } + + // Immediate list: the stale sentinel must be gone; the real job + // must be present (read from the DB). + out, err := runCLI(t, "job", "list") + if err != nil { + t.Fatalf("job list after run: %v", err) + } + if strings.Contains(out, "stale-job") { + t.Errorf("stale job cache still served after run (invalidation missing): %s", out) + } + if !strings.Contains(out, "cacheinv-job") { + t.Errorf("new job missing from list after run (cache not re-read): %s", out) + } +} + +// TestCacheInvalidateHelperDirectly is a small unit test for the +// cacheInvalidate helper itself: it confirms a populated class is +// empty after the helper runs. +func TestCacheInvalidateHelperDirectly(t *testing.T) { + _, cleanup := initTestEnv(t) + defer cleanup() + c, err := cache.Open(paths.CacheDB()) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := c.Set(cacheNodeClass, cacheListKey, []byte("x"), 0); err != nil { + t.Fatalf("set: %v", err) + } + c.Close() + + cacheInvalidate(cacheNodeClass) + + c2, err := cache.Open(paths.CacheDB()) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer c2.Close() + if _, _, err := c2.Get(cacheNodeClass, cacheListKey); err == nil { + t.Errorf("nodes/list still present after cacheInvalidate") + } +} diff --git a/internal/cli/job.go b/internal/cli/job.go index ab84af6..b0fba22 100644 --- a/internal/cli/job.go +++ b/internal/cli/job.go @@ -123,6 +123,9 @@ var jobRunCmd = &cobra.Command{ return derr } res.unitPaths = unitPaths + // REQ-156 / P07 T5: invalidate the jobs cache (the + // dispatch decision records a local job entry). + cacheInvalidate(cacheJobClass) if jsonOutput { return printJSON(map[string]any{ "status": "deployed", @@ -149,6 +152,10 @@ var jobRunCmd = &cobra.Command{ } runErr := exec.Run(ctx, job, workloadToTaskSpecs(spec)) logDispatch(res, runErr) + // REQ-156 / P07 T5: invalidate the jobs cache so the next + // `orca job list` reflects the just-run (or just-failed) + // job instead of a stale cached list. + cacheInvalidate(cacheJobClass) if runErr != nil { if jsonOutput { _ = printJSON(map[string]any{"id": job.ID, "status": "failed", "error": runErr.Error()}) @@ -316,6 +323,9 @@ var jobStopCmd = &cobra.Command{ if err := repo.UpdateStatus(ctx, id, model.JobStatusStopped, 130); err != nil { return err } + // REQ-156 / P07 T5: invalidate the jobs cache so the next + // `orca job list` reflects the just-stopped job. + cacheInvalidate(cacheJobClass) if jsonOutput { return printJSON(map[string]any{"id": id, "status": "stopped", "previous_status": job.Status}) } diff --git a/internal/cli/node.go b/internal/cli/node.go index 57dd326..6b0f709 100644 --- a/internal/cli/node.go +++ b/internal/cli/node.go @@ -140,6 +140,10 @@ func joinLocal(cmd *cobra.Command) error { if err := registry.Join(ctx, node); err != nil { return err } + // REQ-156 / P07 T5: invalidate the nodes cache so the next + // `orca node list` does not surface a stale list missing the + // just-joined node. + cacheInvalidate(cacheNodeClass) if jsonOutput { return printJSON(node) } @@ -203,6 +207,8 @@ func joinProxmox(cmd *cobra.Command) error { if err := registry.Join(regCtx, node); err != nil { return fmt.Errorf("register proxmox node: %w", err) } + // REQ-156 / P07 T5: invalidate the nodes cache. + cacheInvalidate(cacheNodeClass) if jsonOutput { return printJSON(node) } @@ -236,6 +242,9 @@ var nodeLeaveCmd = &cobra.Command{ if err := registry.Leave(ctx, id); err != nil { return err } + // REQ-156 / P07 T5: invalidate the nodes cache so the next + // `orca node list` does not surface the just-left node. + cacheInvalidate(cacheNodeClass) if jsonOutput { return printJSON(map[string]string{"id": id, "state": "left"}) } diff --git a/internal/cli/ns.go b/internal/cli/ns.go index dcf8fad..c4006e2 100644 --- a/internal/cli/ns.go +++ b/internal/cli/ns.go @@ -24,6 +24,7 @@ import ( "git.cloudinit.dev/coreci/orca/internal/ns" "git.cloudinit.dev/coreci/orca/internal/paths" + "git.cloudinit.dev/coreci/orca/internal/security" ) var nsCmd = &cobra.Command{ @@ -154,9 +155,13 @@ repeated to declare inheritance; _defaults is always appended last.`, // Explicit _defaults listing is allowed (de-duped silently). } body := renderNSMd(name, parents, nsCreateInheritsEnv, nsCreateInheritsSecret) - if err := os.WriteFile(paths.NSMd(name), []byte(body), 0o644); err != nil { + if err := writeNSMdAtomic(paths.NSMd(name), body); err != nil { return fmt.Errorf("write ns.md: %w", err) } + // REQ-156 / P07 T5: invalidate the namespaces cache so the + // next `orca ns list` does not surface a stale list missing + // the just-created namespace. + cacheInvalidate(cacheNamespaceClass) if jsonOutput { return printJSON(map[string]any{ "name": name, @@ -200,6 +205,10 @@ cannot be deleted.`, if err := os.RemoveAll(nsDir); err != nil { return fmt.Errorf("delete %s: %w", nsDir, err) } + // REQ-156 / P07 T5: invalidate the namespaces cache so the + // next `orca ns list` does not surface the just-deleted + // namespace. + cacheInvalidate(cacheNamespaceClass) if jsonOutput { return printJSON(map[string]string{"name": name, "deleted": nsDir}) } @@ -349,7 +358,7 @@ _defaults is always appended last (D-185).`, } body := renderNSMdFull(cfg, nsBody) - if err := os.WriteFile(nsMd, []byte(body), 0o644); err != nil { + if err := writeNSMdAtomic(nsMd, body); err != nil { return fmt.Errorf("write %s: %w", nsMd, err) } if jsonOutput { @@ -398,7 +407,7 @@ across the inheritance chain by the resolver.`, cfg.Constraints = append(cfg.Constraints, constraint) body := renderNSMdFull(cfg, nsBody) - if err := os.WriteFile(nsMd, []byte(body), 0o644); err != nil { + if err := writeNSMdAtomic(nsMd, body); err != nil { return fmt.Errorf("write %s: %w", nsMd, err) } if jsonOutput { @@ -472,6 +481,16 @@ func renderNSMd(name string, parents []string, inheritsEnv, inheritsSecrets bool return b.String() } +// writeNSMdAtomic writes the ns.md frontmatter for a namespace +// atomically (REQ-156, P07 T7). Uses security.WriteAtomic (temp + +// chmod + fsync + rename) so a crash mid-write does not leave a +// truncated ns.md that the inheritance resolver would fail to parse. +// The file mode is 0644 (ns.md is not secret - it contains +// frontmatter only). +func writeNSMdAtomic(path, body string) error { + return security.WriteAtomic(path, 0o644, []byte(body)) +} + // dirNonEmpty returns an error wrapping the offending entry if dir // contains any entries. func dirNonEmpty(dir string) error { diff --git a/internal/cli/recovery.go b/internal/cli/recovery.go index b37f9fe..14385d7 100644 --- a/internal/cli/recovery.go +++ b/internal/cli/recovery.go @@ -406,11 +406,16 @@ func findNamespaceDirs(targetDir string) []string { // read-only. It uses the same driver as the rest of the codebase // (modernc.org/sqlite via store.Open, but with a read-only pragma). func dbOpenable(path string) error { - dsn := "file:" + path + "?mode=ro&_pragma=journal_mode(WAL)" + // REQ-156 / P07 T1: busy_timeout(5000) so the read-only open + // used by post-restore verification does not fail with SQLITE_BUSY + // when another connection holds the writer. SetMaxOpenConns(1) + // serializes the (read-only) connections. + dsn := "file:" + path + "?mode=ro&_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)" db, err := sql.Open("sqlite", dsn) if err != nil { return err } + db.SetMaxOpenConns(1) defer db.Close() if err := db.Ping(); err != nil { return err diff --git a/internal/cli/rotate_lead.go b/internal/cli/rotate_lead.go index 9ad671b..1ff6fbb 100644 --- a/internal/cli/rotate_lead.go +++ b/internal/cli/rotate_lead.go @@ -20,6 +20,7 @@ import ( "git.cloudinit.dev/coreci/orca/internal/engine" "git.cloudinit.dev/coreci/orca/internal/model" "git.cloudinit.dev/coreci/orca/internal/paths" + "git.cloudinit.dev/coreci/orca/internal/security" "git.cloudinit.dev/coreci/orca/internal/store" ) @@ -246,10 +247,14 @@ func rotateSSHKeys(ctx context.Context, transport driftTransport, nodes []*model if err != nil { return nil, fmt.Errorf("generate new ssh key: %w", err) } - if err := os.WriteFile(keyPath, newPriv, 0o600); err != nil { + // REQ-156 / P07 T8/T9: write the new SSH keypair atomically so a + // crash mid-write does not leave a truncated key (which would + // break all peer SSH until manually regenerated). security.WriteAtomic + // does temp + chmod + fsync + rename. + if err := security.WriteAtomic(keyPath, 0o600, newPriv); err != nil { return nil, fmt.Errorf("write new ssh key: %w", err) } - if err := os.WriteFile(pubPath, newPub, 0o644); err != nil { + if err := security.WriteAtomic(pubPath, 0o644, newPub); err != nil { return nil, fmt.Errorf("write new ssh pub: %w", err) } @@ -318,7 +323,11 @@ func writeCurrentLead(ctx context.Context, name string) error { return err } leadPath := filepath.Join(dir, "lead") - return os.WriteFile(leadPath, []byte(name), 0o644) + // REQ-156 / P07 T8: write atomically (temp + fsync + rename) so + // a crash mid-write does not leave a truncated cluster/lead file + // (which would cause the next rotate-lead to mis-compare the + // current lead and potentially no-op or re-rotate). + return security.WriteAtomic(leadPath, 0o644, []byte(name)) } func trimSpace(s string) string { diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 7def67a..88d1ac0 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -29,6 +29,7 @@ import ( "git.cloudinit.dev/coreci/orca/internal/paths" "git.cloudinit.dev/coreci/orca/internal/secrets" + "git.cloudinit.dev/coreci/orca/internal/security" ) var secretsCmd = &cobra.Command{ @@ -93,6 +94,23 @@ func saveNSSecrets(namespace string, nsKey []byte, lines []string) error { return nil } +// lockNSSecrets acquires an exclusive advisory lock on the namespace's +// .env.secrets file (REQ-156, P07 T2). The lock file is +// paths.NSSecrets(ns) + ".lock". Returns a release function that MUST +// be deferred. Used by set/rotate/delete/rotate-master to prevent +// concurrent read-modify-write races: two operators running +// `orca secrets set` simultaneously against the same namespace would +// otherwise each load-then-save and the second write would clobber the +// first (losing a key). The flock is advisory; the parent dir is +// created first so Flock's O_CREATE does not fail on a missing dir. +func lockNSSecrets(namespace string) (func(), error) { + secPath := paths.NSSecrets(namespace) + if err := os.MkdirAll(filepath.Dir(secPath), 0o755); err != nil { + return nil, fmt.Errorf("create ns dir for lock: %w", err) + } + return security.Flock(secPath + ".lock") +} + // parseKV splits a "KEY=value" argument. The value may contain '='. func parseKV(arg string) (key, value string, err error) { idx := strings.IndexByte(arg, '=') @@ -136,6 +154,14 @@ is appended. The .env.secrets file is rewritten atomically.`, if err != nil { return err } + // REQ-156 / P07 T2: flock around load+save so concurrent + // `orca secrets set` on the same namespace don't clobber + // each other (the second write would lose the first's key). + release, err := lockNSSecrets(ns) + if err != nil { + return fmt.Errorf("acquire secrets lock: %w", err) + } + defer release() nsKey, lines, err := loadMasterAndNSSecrets(ns) if err != nil { return err @@ -230,6 +256,13 @@ old ciphertext copies. The .env.secrets file is rewritten atomically.`, RunE: func(cmd *cobra.Command, args []string) error { ns := args[0] key := args[1] + // REQ-156 / P07 T2: flock around load+save (re-encryption is a + // read-modify-write of the whole .env.secrets file). + release, err := lockNSSecrets(ns) + if err != nil { + return fmt.Errorf("acquire secrets lock: %w", err) + } + defer release() nsKey, lines, err := loadMasterAndNSSecrets(ns) if err != nil { return err @@ -263,6 +296,13 @@ var secretsDeleteCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ns := args[0] key := args[1] + // REQ-156 / P07 T2: flock around load+save (delete rewrites + // the whole file). + release, err := lockNSSecrets(ns) + if err != nil { + return fmt.Errorf("acquire secrets lock: %w", err) + } + defer release() nsKey, lines, err := loadMasterAndNSSecrets(ns) if err != nil { return err @@ -341,11 +381,20 @@ automatic rollback to the old key on any failure (C-30).`, // Re-encrypt each namespace. On any failure, rollback. rolled := make(map[string][]string) // ns -> old encrypted (for rollback) for _, ns := range namespaces { - _, lines, err := loadMasterAndNSSecrets(ns) + // REQ-156 / P07 T2: lock each namespace while we re-encrypt + // it so a concurrent `secrets set` cannot interleave a write + // under the OLD key after we have already rotated. + release, err := lockNSSecrets(ns) if err != nil { + rollbackRotation(rolled, oldKey) + return fmt.Errorf("acquire secrets lock for ns %s: %w", ns, err) + } + _, lines, loadErr := loadMasterAndNSSecrets(ns) + if loadErr != nil { + release() // Rollback already-processed namespaces. rollbackRotation(rolled, oldKey) - return fmt.Errorf("load secrets for ns %s: %w", ns, err) + return fmt.Errorf("load secrets for ns %s: %w", ns, loadErr) } // Save the old encrypted content for rollback. secPath := paths.NSSecrets(ns) @@ -355,18 +404,22 @@ automatic rollback to the old key on any failure (C-30).`, // Re-encrypt under the new key. newNSKey, err := secrets.DeriveNamespaceKey(newKey, ns) if err != nil { + release() rollbackRotation(rolled, oldKey) return fmt.Errorf("derive new ns key for %s: %w", ns, err) } enc, err := secrets.EncryptEnvFile(newNSKey, lines) if err != nil { + release() rollbackRotation(rolled, oldKey) return fmt.Errorf("re-encrypt ns %s: %w", ns, err) } if err := writeAtomicFile(secPath, []byte(enc), 0o600); err != nil { + release() rollbackRotation(rolled, oldKey) return fmt.Errorf("write ns %s: %w", ns, err) } + release() } // Save the new master key. diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index fe371f2..e7991c0 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -94,6 +94,33 @@ func init() { rootCmd.AddCommand(upgradeCmd) } +// acquireUpgradeLock atomically creates an exclusive lock file at +// paths.ClusterDir()/upgrade.lock (REQ-156, P07 T3). Returns a release +// function that MUST be deferred (it removes the lock file). If the +// lock file already exists, returns an error "upgrade already in +// progress" — preventing two concurrent `orca upgrade` invocations +// from racing on the same cluster state (cutover, install.sh, peer +// user creation). O_CREATE|O_EXCL is atomic under POSIX: only one of +// two racing callers succeeds; the other gets EEXIST. +func acquireUpgradeLock() (func(), error) { + lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock") + if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil { + return nil, fmt.Errorf("create cluster dir for upgrade lock: %w", err) + } + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + if os.IsExist(err) { + return nil, fmt.Errorf("upgrade already in progress (lock file %s exists; remove it if stale)", lockPath) + } + return nil, fmt.Errorf("acquire upgrade lock: %w", err) + } + // Write the current PID + timestamp for diagnostics (best-effort; + // a stale lock from a crashed process is the operator's signal). + _, _ = f.WriteString(fmt.Sprintf("pid=%d started=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339))) + _ = f.Close() + return func() { _ = os.Remove(lockPath) }, nil +} + // UpgradeResult is the JSON-serializable summary of an upgrade run. type UpgradeResult struct { TargetVersion string `json:"target_version"` @@ -134,8 +161,26 @@ func runUpgrade(cmd *cobra.Command, out interface{ Write([]byte) (int, error) }) return nil } + // REQ-156 / P07 T3: v0.8 layout detection is read-only and MUST + // run BEFORE the upgrade lock is acquired — the lock creates the + // cluster/ dir (for the lock file), and Detectv08 treats the + // presence of a cluster/ dir as "already v0.11" (no migration + // needed). Detecting first avoids a false negative that would + // skip the migration on a genuine v0.8 layout. home := paths.Root() - if migration.Detectv08(home) { + needV08Migration := migration.Detectv08(home) + + // Acquire an exclusive upgrade lock for the rest of the run so + // two concurrent `orca upgrade` invocations cannot race on the + // cutover / install.sh / peer user creation. The lock is released + // on return (including error paths). + upgradeRelease, err := acquireUpgradeLock() + if err != nil { + return err + } + defer upgradeRelease() + + if needV08Migration { if !jsonOutput { fmt.Fprintf(out, "• v0.8 layout detected; running data migration first\n") } diff --git a/internal/engine/executor.go b/internal/engine/executor.go index 04b0eae..60eb73e 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -98,24 +98,39 @@ type TaskSpec struct { } func (e *Executor) Run(ctx context.Context, job *model.Job, specs []TaskSpec) error { - e.mu.Lock() - defer e.mu.Unlock() + // REQ-156 / P07 T6: the mutex previously guarded the ENTIRE job + // (insert + status transitions + task execution + wait). That + // serialized unrelated jobs against each other and held the lock + // across long-running child processes, blocking concurrent + // Submit/Status/Run callers. The mutex is now scoped ONLY to the + // DB inserts/updates (the part that must be serialized against + // the single-writer SQLite connection pool — see store.Open + // SetMaxOpenConns(1)). The task goroutines spawned below do not + // hold e.mu; they share the per-job failure counter via a local + // sync.Mutex. - // Insert the job first so tasks can reference it via foreign key. + // Insert the job + flip to Running under the lock (serializes + // the DB writes; the underlying SQLite busy_timeout(5000) + + // SetMaxOpenConns(1) handles contention). + e.mu.Lock() if err := e.jobs.Insert(ctx, job); err != nil { + e.mu.Unlock() return err } if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusRunning, 0); err != nil { + e.mu.Unlock() return err } + e.mu.Unlock() + // Task execution runs WITHOUT e.mu — concurrent jobs (and + // concurrent Submit/Status callers) are no longer blocked by a + // long-running child process. var ( wg sync.WaitGroup failedCount int - exitCode int mu sync.Mutex ) - for _, ts := range specs { wg.Add(1) go func(ts TaskSpec) { @@ -133,14 +148,16 @@ func (e *Executor) Run(ctx context.Context, job *model.Job, specs []TaskSpec) er } wg.Wait() + // Final status transition under the lock (the DB write is the + // only thing that needs serialization). + e.mu.Lock() + defer e.mu.Unlock() if failedCount > 0 { - exitCode = 1 - if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusFailed, exitCode); err != nil { + if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusFailed, 1); err != nil { return err } return fmt.Errorf("%d/%d tasks failed", failedCount, len(specs)) } - if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusComplete, 0); err != nil { return err } diff --git a/internal/identity/oidc.go b/internal/identity/oidc.go index 7452713..1e807d4 100644 --- a/internal/identity/oidc.go +++ b/internal/identity/oidc.go @@ -29,6 +29,8 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" + + "git.cloudinit.dev/coreci/orca/internal/security" ) // OIDCConfig holds the OIDC client configuration. It is loaded from @@ -113,7 +115,13 @@ func SaveCredentials(c *Credentials) error { if err != nil { return fmt.Errorf("oidc: marshal: %w", err) } - return writeAtomic0600(path, data) + // REQ-156 / P07 T9: use the canonical security.WriteAtomic (temp + // + chmod + fsync + rename) instead of the local writeAtomic0600 + // (which did temp + chmod + rename with NO fsync - a crash before + // rename could leave a partially-written tmp file that rename + // would then promote, or the rename could land before the data + // reached durable storage). + return security.WriteAtomic(path, 0o600, data) } // ClearCredentials removes the stored credentials (logout). @@ -128,16 +136,6 @@ func ClearCredentials() error { return nil } -// writeAtomic0600 writes data to path atomically at mode 0600 -// (temp + chmod + rename). -func writeAtomic0600(path string, data []byte) error { - tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0o600); err != nil { - return fmt.Errorf("oidc: write tmp: %w", err) - } - return os.Rename(tmp, path) -} - // OIDCClient wraps the OIDC provider + oauth2 config for the auth flow. type OIDCClient struct { provider *oidc.Provider diff --git a/internal/store/store.go b/internal/store/store.go index 7a21685..22ab0ac 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -22,6 +22,11 @@ func Open(path string) (*sql.DB, error) { if err != nil { return nil, fmt.Errorf("open sqlite: %w", err) } + // REQ-156 / P07 T1: SQLite is a single-writer database. Cap the + // connection pool at 1 so concurrent goroutines serialize on the + // busy_timeout(5000) above instead of racing for the WAL writer + // lock and surfacing spurious SQLITE_BUSY errors to callers. + db.SetMaxOpenConns(1) if err := db.Ping(); err != nil { _ = db.Close() return nil, fmt.Errorf("ping sqlite: %w", err) diff --git a/internal/webauthn/connector.go b/internal/webauthn/connector.go index 4ac78e0..411d222 100644 --- a/internal/webauthn/connector.go +++ b/internal/webauthn/connector.go @@ -15,6 +15,7 @@ import ( "fmt" "net/http" "strings" + "sync" "time" "github.com/go-webauthn/webauthn/protocol" @@ -80,26 +81,54 @@ type RegistrationSession struct { CreatedAt time.Time } -// sessionStore holds in-flight sessions (registration + login). In -// production this would be a Redis/shared cache; for the bundled -// single-lead Dex, an in-memory map with TTL is sufficient. +// sessionStore holds in-flight sessions (registration). In production +// this would be a Redis/shared cache; for the bundled single-lead +// Dex, an in-memory map with TTL is sufficient. +// +// REQ-156 / P07 T10: the session maps are accessed from HTTP handler +// goroutines (one goroutine per request) and were previously plain +// maps with no synchronization. Concurrent BeginRegistration calls +// for the same username would race on map writes (detected by go +// test -race in T11). A sync.Mutex now guards all access. type sessionStore struct { + mu sync.Mutex sessions map[string]*RegistrationSession } +// regSessions is the global in-flight registration session store. var regSessions = &sessionStore{sessions: make(map[string]*RegistrationSession)} +// loginSessionStore holds in-flight login sessions (T10). Same +// mutex pattern as sessionStore. +type loginSessionStore struct { + mu sync.Mutex + sessions map[string]*LoginSession +} + +// loginSessions is the global in-flight login session store. +var loginSessions = &loginSessionStore{sessions: make(map[string]*LoginSession)} + // sessionTTL is the max time a registration/login session is valid. const sessionTTL = 5 * time.Minute -// cleanSessions removes expired sessions. +// cleanSessions removes expired registration + login sessions. +// Called under each store's lock by the Begin* handlers. func cleanSessions() { now := time.Now() - for id, s := range regSessions.sessions { - if now.Sub(s.CreatedAt) > sessionTTL { + regSessions.mu.Lock() + for id, sess := range regSessions.sessions { + if now.Sub(sess.CreatedAt) > sessionTTL { delete(regSessions.sessions, id) } } + regSessions.mu.Unlock() + loginSessions.mu.Lock() + for id, sess := range loginSessions.sessions { + if now.Sub(sess.CreatedAt) > sessionTTL { + delete(loginSessions.sessions, id) + } + } + loginSessions.mu.Unlock() } // requireAuth checks the request for an authenticated session. When @@ -165,11 +194,13 @@ func (c *Connector) BeginRegistration(w http.ResponseWriter, r *http.Request) { return } sessionID := base64.RawURLEncoding.EncodeToString(userID) + regSessions.mu.Lock() regSessions.sessions[sessionID] = &RegistrationSession{ UserID: username, Challenge: session, CreatedAt: time.Now(), } + regSessions.mu.Unlock() w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(options) } @@ -188,16 +219,20 @@ func (c *Connector) FinishRegistration(w http.ResponseWriter, r *http.Request) { return } sessionID := base64.RawURLEncoding.EncodeToString([]byte(username)) + regSessions.mu.Lock() session, ok := regSessions.sessions[sessionID] if !ok { + regSessions.mu.Unlock() http.Error(w, "no registration session; call /register first", http.StatusBadRequest) return } if time.Since(session.CreatedAt) > sessionTTL { delete(regSessions.sessions, sessionID) + regSessions.mu.Unlock() http.Error(w, "session expired", http.StatusBadRequest) return } + regSessions.mu.Unlock() parsed, err := protocol.ParseCredentialCreationResponseBody(r.Body) if err != nil { http.Error(w, fmt.Sprintf("parse attestation: %v", err), http.StatusBadRequest) @@ -221,7 +256,9 @@ func (c *Connector) FinishRegistration(w http.ResponseWriter, r *http.Request) { http.Error(w, fmt.Sprintf("store credential: %v", err), http.StatusInternalServerError) return } + regSessions.mu.Lock() delete(regSessions.sessions, sessionID) + regSessions.mu.Unlock() w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(map[string]string{"status": "registered", "user_id": username}) } @@ -233,8 +270,6 @@ type LoginSession struct { CreatedAt time.Time } -var loginSessions = map[string]*LoginSession{} - // BeginLogin starts the WebAuthn login ceremony. // GET /orca/webauthn/login?username= func (c *Connector) BeginLogin(w http.ResponseWriter, r *http.Request) { @@ -259,11 +294,13 @@ func (c *Connector) BeginLogin(w http.ResponseWriter, r *http.Request) { http.Error(w, fmt.Sprintf("begin login: %v", err), http.StatusInternalServerError) return } - loginSessions[username] = &LoginSession{ + loginSessions.mu.Lock() + loginSessions.sessions[username] = &LoginSession{ UserID: username, Challenge: session, CreatedAt: time.Now(), } + loginSessions.mu.Unlock() w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(options) } @@ -276,16 +313,20 @@ func (c *Connector) FinishLogin(w http.ResponseWriter, r *http.Request) { http.Error(w, "username required", http.StatusBadRequest) return } - session, ok := loginSessions[username] + loginSessions.mu.Lock() + session, ok := loginSessions.sessions[username] if !ok { + loginSessions.mu.Unlock() http.Error(w, "no login session; call /login first", http.StatusBadRequest) return } if time.Since(session.CreatedAt) > sessionTTL { - delete(loginSessions, username) + delete(loginSessions.sessions, username) + loginSessions.mu.Unlock() http.Error(w, "session expired", http.StatusBadRequest) return } + loginSessions.mu.Unlock() existing, _ := c.store.GetCredential(username) if existing == nil { http.Error(w, "user not registered", http.StatusNotFound) @@ -307,7 +348,9 @@ func (c *Connector) FinishLogin(w http.ResponseWriter, r *http.Request) { return } _ = c.store.UpdateSignCount(username, cred.Authenticator.SignCount) - delete(loginSessions, username) + loginSessions.mu.Lock() + delete(loginSessions.sessions, username) + loginSessions.mu.Unlock() // The OIDC sub is the username (the connector maps credential ID // to sub). Dex uses this to issue the ID token. w.Header().Set("Content-Type", "application/json") diff --git a/internal/webauthn/connector_concurrency_test.go b/internal/webauthn/connector_concurrency_test.go new file mode 100644 index 0000000..aece5f2 --- /dev/null +++ b/internal/webauthn/connector_concurrency_test.go @@ -0,0 +1,179 @@ +package webauthn + +// connector_concurrency_test.go covers REQ-156 / P07 T10: the +// WebAuthn session maps (regSessions, loginSessions) are accessed +// from HTTP handler goroutines (one goroutine per request) and were +// previously plain maps with no synchronization. Concurrent +// BeginRegistration calls for the same username would race on map +// writes (detected by `go test -race`). T10 added a sync.Mutex to +// each store; this test exercises the fix under the race detector. +// +// Run with: go test -race ./internal/webauthn/ + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" +) + +// TestBeginRegistrationConcurrentNoPanic fires many concurrent +// BeginRegistration requests (all authenticated, all for the SAME +// username so they hit the SAME session map entry) and asserts the +// handler does not panic and does not race on the shared +// regSessions.sessions map. Without the T10 mutex this test panics +// under -race with "concurrent map writes". +func TestBeginRegistrationConcurrentNoPanic(t *testing.T) { + dbPath := t.TempDir() + "/webauthn-conc.db" + store, err := NewStore(dbPath) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + defer store.Close() + c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster", + func(r *http.Request) (bool, string, error) { return true, "admin", nil }) + if err != nil { + t.Fatalf("NewConnector: %v", err) + } + mux := c.Routes() + + const n = 25 + var wg sync.WaitGroup + wg.Add(n) + panicCh := make(chan interface{}, n) + for i := 0; i < n; i++ { + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + select { + case panicCh <- r: + default: + } + } + }() + req := httptest.NewRequest("GET", "/orca/webauthn/register?username=admin", nil) + rec := httptest.NewRecorder() + // BeginRegistration writes to regSessions.sessions[sessionID] + // under the mutex; concurrent writers for the same key + // must not panic or race. + mux.ServeHTTP(rec, req) + }() + } + wg.Wait() + close(panicCh) + if p, ok := <-panicCh; ok { + t.Fatalf("BeginRegistration panicked under concurrency: %v", p) + } +} + +// TestBeginLoginConcurrentNoPanic is the login-session variant. It +// pre-registers a credential so BeginLogin finds the user, then fires +// concurrent BeginLogin calls for the same username. The login +// session map writes must be mutex-guarded (T10). +func TestBeginLoginConcurrentNoPanic(t *testing.T) { + dbPath := t.TempDir() + "/webauthn-conc-login.db" + store, err := NewStore(dbPath) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + defer store.Close() + // Pre-seed a credential so BeginLogin does not 404. + if err := store.PutCredential(&Credential{ + UserID: "loginuser", + CredentialID: []byte("cred-id-bytes"), + PublicKey: []byte("pub-key-bytes"), + }); err != nil { + t.Fatalf("PutCredential: %v", err) + } + c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster", + func(r *http.Request) (bool, string, error) { return true, "loginuser", nil }) + if err != nil { + t.Fatalf("NewConnector: %v", err) + } + mux := c.Routes() + + const n = 25 + var wg sync.WaitGroup + wg.Add(n) + panicCh := make(chan interface{}, n) + for i := 0; i < n; i++ { + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + select { + case panicCh <- r: + default: + } + } + }() + req := httptest.NewRequest("GET", "/orca/webauthn/login?username=loginuser", nil) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + }() + } + wg.Wait() + close(panicCh) + if p, ok := <-panicCh; ok { + t.Fatalf("BeginLogin panicked under concurrency: %v", p) + } +} + +// TestCleanSessionsConcurrentNoPanic exercises the cleanSessions +// helper which iterates + deletes from BOTH session maps. Without +// the T10 mutexes, concurrent cleanSessions + BeginRegistration +// would race. We drive cleanSessions from multiple goroutines while +// also doing BeginRegistration writes. +func TestCleanSessionsConcurrentNoPanic(t *testing.T) { + dbPath := t.TempDir() + "/webauthn-clean.db" + store, err := NewStore(dbPath) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + defer store.Close() + c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster", + func(r *http.Request) (bool, string, error) { return true, "admin", nil }) + if err != nil { + t.Fatalf("NewConnector: %v", err) + } + mux := c.Routes() + + const n = 15 + var wg sync.WaitGroup + wg.Add(n * 2) + panicCh := make(chan interface{}, n*2) + for i := 0; i < n; i++ { + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + select { + case panicCh <- r: + default: + } + } + }() + cleanSessions() + }() + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + select { + case panicCh <- r: + default: + } + } + }() + req := httptest.NewRequest("GET", "/orca/webauthn/register?username=admin", nil) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + }() + } + wg.Wait() + close(panicCh) + if p, ok := <-panicCh; ok { + t.Fatalf("cleanSessions/BeginRegistration panicked under concurrency: %v", p) + } +} diff --git a/internal/webauthn/store.go b/internal/webauthn/store.go index c9899e7..97f0544 100644 --- a/internal/webauthn/store.go +++ b/internal/webauthn/store.go @@ -46,11 +46,15 @@ func NewStore(dbPath string) (*Store, error) { if err := os.MkdirAll(filepath.Dir(dbPath), 0o700); err != nil { return nil, fmt.Errorf("webauthn: mkdir: %w", err) } - dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)", dbPath) + // REQ-156 / P07 T1: busy_timeout(5000) so concurrent webauthn + // DB opens wait up to 5s for the writer instead of failing with + // SQLITE_BUSY. SetMaxOpenConns(1) serializes the connections. + dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)", dbPath) db, err := sql.Open("sqlite", dsn) if err != nil { return nil, fmt.Errorf("webauthn: open db: %w", err) } + db.SetMaxOpenConns(1) if err := db.Ping(); err != nil { db.Close() return nil, fmt.Errorf("webauthn: ping: %w", err)