From ce0e2370514cf26dd327301321408cdd5ee9aa94 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Mon, 24 Aug 2026 15:51:25 -0700 Subject: [PATCH] Refuse a fallback-admin at preflight, not after the migration finishes A dress rehearsal on a clone of production migrated cleanly and then failed all three post-cutover steps with 401: the health check, the quota rebuild, and the content comparison. The account was `admin` - a v0.15 [authentication.fallback-admin], not a directory account. v0.16 keeps its configuration in the store, so the block defining it does not survive, and the credential stops working the instant the migration completes. Every check passed beforehand, because on v0.15 that account authenticates perfectly well. The README has said to use a named account for a while; the tool now says it too, while nothing has been touched and changing it costs one flag. The migration itself is unaffected - it succeeds either way. What is lost is the ability to verify it afterwards, which is the part worth having. --- internal/preflight/checks.go | 34 ++++++++++++++++++++++++ internal/preflight/checks_test.go | 43 ++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index e5813ef..3bfd780 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -331,6 +331,12 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi }); err != nil { return report, err } + + if _, err := runCheck("admin-account-kind", func() (CheckResult, string) { + return c.adminAccountKind(rs), "" + }); err != nil { + return report, err + } } else { report.Results = append(report.Results, CheckResult{ Name: "admin-reachable", @@ -411,3 +417,31 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi return report, nil } + +// adminAccountKind reports whether the account preflight authenticated as +// still exists after the migration. +func (c *Checker) adminAccountKind(rs *checkpoint.RunState) CheckResult { + if rs.PreflightSnapshot == nil { + return CheckResult{Status: StatusWarn, Detail: "no snapshot was captured, so the admin account could not be looked up in the directory"} + } + // v0.16 keeps its configuration in the store, so the + // [authentication.fallback-admin] block a v0.15 config can + // define simply ceases to exist. An operator who authenticates + // as that admin gets through every check here - it works fine + // today - and then finds it rejected the moment the migration + // completes, taking the quota rebuild and the post-migration + // content comparison with it. Cheaper to say so now. + for account := range rs.PreflightSnapshot.UsedQuota { + if strings.EqualFold(account, c.opts.AdminUser) { + return CheckResult{Status: StatusOK, Detail: fmt.Sprintf("%s is an account in the directory, so it survives the migration", c.opts.AdminUser)} + } + if local, _, ok := strings.Cut(account, "@"); ok && strings.EqualFold(local, c.opts.AdminUser) { + return CheckResult{Status: StatusOK, Detail: fmt.Sprintf("%s matches directory account %s, which survives the migration", c.opts.AdminUser, account)} + } + } + return CheckResult{Status: StatusFail, Detail: fmt.Sprintf( + "%s authenticates now but is not an account in this directory - it is a config fallback-admin, and v0.16 keeps its "+ + "config in the store, so the block defining it does not survive. The migration itself would succeed, then the quota "+ + "rebuild and the post-migration content check would both be refused with 401. Re-run as an account that exists in "+ + "the directory (see the README's \"You need a named admin account\")", c.opts.AdminUser)} +} diff --git a/internal/preflight/checks_test.go b/internal/preflight/checks_test.go index ef68876..e623b0c 100644 --- a/internal/preflight/checks_test.go +++ b/internal/preflight/checks_test.go @@ -294,7 +294,9 @@ func TestCheckerRunCapturesAccountSnapshotWhenAdminURLSet(t *testing.T) { } checker := New(Options{ BinaryPath: binaryPath, ConfigPath: configPath, DataDir: dataDir, TargetVersion: "latest", - AdminURL: adminSrv.URL, AdminUser: "admin", AdminPassword: "hunter2", + // A directory account, not a config fallback-admin: preflight now + // refuses the latter, since it does not survive into v0.16. + AdminURL: adminSrv.URL, AdminUser: "alice@example.com", AdminPassword: "hunter2", }) report, err := checker.Run(context.Background(), store, rs) @@ -409,3 +411,42 @@ func TestTargetReleaseRejectsAMismatchedLocalBinary(t *testing.T) { } t.Fatal("no target-release check in the report") } + +// v0.16 keeps its configuration in the store, so a v0.15 +// [authentication.fallback-admin] simply ceases to exist. Authenticating as +// one passes every check today and is refused the moment the migration +// finishes - taking the quota rebuild and the post-migration content +// comparison with it. Observed on a production clone: the run migrated +// cleanly and then failed all three post-cutover steps with 401. +func TestAdminAccountKind(t *testing.T) { + for _, tc := range []struct { + name string + admin string + want Status + }{ + {"a directory account survives", "alice@example.com", StatusOK}, + {"its local part is accepted too", "alice", StatusOK}, + {"a config fallback-admin does not", "admin", StatusFail}, + } { + t.Run(tc.name, func(t *testing.T) { + rs := &checkpoint.RunState{PreflightSnapshot: &checkpoint.PreflightSnapshot{ + UsedQuota: map[string]int64{"alice@example.com": 1, "bob@example.com": 2}, + }} + c := New(Options{AdminUser: tc.admin}) + res := c.adminAccountKind(rs) + if res.Status != tc.want { + t.Fatalf("status = %q, want %q (%s)", res.Status, tc.want, res.Detail) + } + if tc.want == StatusFail && !strings.Contains(res.Detail, "fallback-admin") { + t.Fatalf("the operator needs to be told why, got %q", res.Detail) + } + }) + } +} + +func TestAdminAccountKindWithoutASnapshot(t *testing.T) { + res := New(Options{AdminUser: "admin"}).adminAccountKind(&checkpoint.RunState{}) + if res.Status != StatusWarn { + t.Fatalf("status = %q, want a warning when there was nothing to look in", res.Status) + } +}