From 826875777358c6ce500e8c9461380222206c06f6 Mon Sep 17 00:00:00 2001 From: Claus Lohmar Date: Wed, 14 Oct 2026 11:30:09 +0000 Subject: [PATCH] fix(security): sign the setup ceremony with startup-generated keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW-001 (M0) M1s: the /setup wizard's pre-session double-submit ceremony (A15) was signed with csrf_key/cookie_secret left at the shipped CHANGE_ME placeholder until `finish` persisted generated values — a publicly-known signing key for the whole bootstrap. Generate strong ephemeral values at startup in setup mode, mirroring the TOTP key (R-6/D-1): `finish` skips a non-placeholder, so the wizard persists the very values the process already uses. Also add cmd/myday's first test — REVIEW-001 M2c: the D-1 invariant (the startup TOTP key is the persisted key) was covered only by a manual smoke. It generates the key, enrolls a secret, persists via the wizard, reopens the DB with the persisted key and asserts the secret still unseals. --- cmd/myday/main.go | 86 +++++++++++++++++---- cmd/myday/main_test.go | 166 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 238 insertions(+), 14 deletions(-) create mode 100644 cmd/myday/main_test.go diff --git a/cmd/myday/main.go b/cmd/myday/main.go index 995d93f..73ca679 100644 --- a/cmd/myday/main.go +++ b/cmd/myday/main.go @@ -104,20 +104,9 @@ func run() error { // R-6 already rules that a placeholder is replaced with a strong generated // value, so generate it here; it is adopted (and thereby persisted by the // wizard's `finish`, which skips a non-placeholder) only in setup mode. - sealerKey := cfg.Secrets.TOTPEncryptionKey - generatedKey := false - switch { - case config.SecretIsPlaceholder(sealerKey): - k, err := config.GenerateSecret(24) // 24 bytes -> 32 base64url chars == auth.KeySize - if err != nil { - return err - } - sealerKey, generatedKey = k, true - case len(sealerKey) != auth.KeySize: - // An operator-supplied key of the wrong size: fail closed and say so, - // rather than run a ceremony whose result we cannot store (A9). - return fmt.Errorf("myday: secrets.totp_encryption_key must be exactly %d bytes, got %d", - auth.KeySize, len(sealerKey)) + sealerKey, generatedKey, err := prepareTOTPKey(cfg) + if err != nil { + return err } sealer, err := auth.NewAEADSealer([]byte(sealerKey)) if err != nil { @@ -163,6 +152,21 @@ func run() error { "the wizard enrolls TOTP before it persists, so the key must exist now") } + // M1s: the CSRF and cookie signing keys drive the whole pre-session + // ceremony (A15), so they must be strong from the FIRST request too — not + // only after `finish` persists them (which would leave the wizard signing + // with the shipped, publicly-known placeholder until a restart). Generated + // here in setup mode only; `finish` then persists these very values. + if inSetup { + generated, err := prepareSigningKeys(cfg) + if err != nil { + return err + } + for _, name := range generated { + slog.Info("myday: generated a strong signing key for setup (R-6/M1s)", "name", name) + } + } + csrf, err := httpx.NewCSRF([]byte(cfg.Secrets.CSRFKey)) if err != nil { return err @@ -199,6 +203,7 @@ func run() error { CSRF: preSession, Audit: aud, SuppressNudge: setupMode, + BaseURL: cfg.Server.BaseURL, Secure: cfg.IsSecure(), Logger: slog.Default(), }) @@ -221,6 +226,7 @@ func run() error { Audit: aud, TOTPOnlyEnroll: true, SuccessPath: func(string) string { return "/setup" }, + BaseURL: cfg.Server.BaseURL, Secure: cfg.IsSecure(), Logger: slog.Default(), }) @@ -392,6 +398,58 @@ func buildRegistry(authFlow *flow.Actions, setupFlow *setup.Actions, recoveryFlo return reg } +// prepareTOTPKey returns the TOTP sealing key the process will use, generating +// a strong one when the configured value is a placeholder (R-6/D-1). generated +// reports that the value was minted here — run() adopts it only in setup mode, +// where the process must still be able to seal a ceremony result. +// +// An operator-supplied key of the wrong size fails closed: rather than run a +// ceremony whose result we cannot store (A9), say so and stop. +func prepareTOTPKey(cfg *config.Config) (key string, generated bool, err error) { + key = cfg.Secrets.TOTPEncryptionKey + if config.SecretIsPlaceholder(key) { + // 24 bytes -> 32 base64url chars == auth.KeySize. + key, err = config.GenerateSecret(24) + if err != nil { + return "", false, err + } + return key, true, nil + } + if len(key) != auth.KeySize { + return "", false, fmt.Errorf("myday: secrets.totp_encryption_key must be exactly %d bytes, got %d", + auth.KeySize, len(key)) + } + return key, false, nil +} + +// prepareSigningKeys replaces placeholder CSRF / cookie signing keys with strong +// generated values (R-6/M1s) and returns the names of the secrets it generated. +// +// run() calls it only in setup mode: the wizard's pre-session double-submit +// ceremony (A15) is signed with these keys from the first request, so the +// shipped, publicly-known placeholder must never be used. In productive mode +// config.Validate already refuses a placeholder (fail closed, SPEC-003 §1). +func prepareSigningKeys(cfg *config.Config) (generated []string, err error) { + for _, s := range []struct { + name string + dst *string + }{ + {"secrets.csrf_key", &cfg.Secrets.CSRFKey}, + {"secrets.cookie_secret", &cfg.Secrets.CookieSecret}, + } { + if !config.SecretIsPlaceholder(*s.dst) { + continue + } + v, err := config.GenerateSecret(32) + if err != nil { + return generated, err + } + *s.dst = v + generated = append(generated, s.name) + } + return generated, nil +} + // configTarget picks where the wizard persists settings: `config.yaml` beside // the loaded file. It NEVER overwrites the committed `config.example.yaml` // (D-010) — so a dev run on the example writes a real config.yaml next to it. diff --git a/cmd/myday/main_test.go b/cmd/myday/main_test.go new file mode 100644 index 0000000..57bc7ce --- /dev/null +++ b/cmd/myday/main_test.go @@ -0,0 +1,166 @@ +package main + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "testing" + + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/audit" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/auth" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/config" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/httpx" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/setup" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/store" + "git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/users" +) + +// stubEnroller satisfies setup.Enroller — this test never runs a ceremony. +type stubEnroller struct{} + +func (stubEnroller) TOTPBegin(http.ResponseWriter, *http.Request, string) error { return nil } +func (stubEnroller) TOTPVerify(http.ResponseWriter, *http.Request, string) error { return nil } + +// setupYAML is a fresh-instance config: setup mode, every secret a placeholder. +const setupYAML = `setup: true +server: + listen: "127.0.0.1:8080" + base_url: "https://myday.example.com" +app: + locale: "en-GB" +database: + path: "DBPATH" +secrets: + cookie_secret: "CHANGE_ME_32_BYTES_RANDOM" + csrf_key: "CHANGE_ME_32_BYTES_RANDOM" + totp_encryption_key: "CHANGE_ME_32_BYTES_RANDOM" +` + +// TestTOTPKeySurvivesSetupAndRestart pins the D-1 invariant (M2c): the TOTP key +// the process GENERATES AT STARTUP is the key the wizard persists, so a restart +// adopts the same key and the secrets enrolled during setup still unseal. A +// regression here silently destroys every future TOTP secret. +func TestTOTPKeySurvivesSetupAndRestart(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + cfgPath := filepath.Join(dir, config.FileName) + dbPath := filepath.Join(dir, "myday.db") + body := strings.Replace(setupYAML, "DBPATH", dbPath, 1) + if err := os.WriteFile(cfgPath, []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("config.Load: %v", err) + } + + // Startup (D-1 + M1s): every placeholder is replaced with a strong value + // the PROCESS will use — the wizard's ceremony must not run on a shipped + // placeholder key. + key, generated, err := prepareTOTPKey(cfg) + if err != nil { + t.Fatalf("prepareTOTPKey: %v", err) + } + if !generated || config.SecretIsPlaceholder(key) || len(key) != auth.KeySize { + t.Fatalf("the TOTP key was not generated (generated=%v len=%d)", generated, len(key)) + } + cfg.Secrets.TOTPEncryptionKey = key // run() adopts it in setup mode + if _, err := prepareSigningKeys(cfg); err != nil { + t.Fatalf("prepareSigningKeys: %v", err) + } + + // First run: the wizard enrolls the admin and seals its TOTP secret with + // the startup key. + db, err := store.Open(cfg.Database.Path) + if err != nil { + t.Fatalf("store.Open: %v", err) + } + if err := db.Migrate(); err != nil { + t.Fatalf("Migrate: %v", err) + } + aud := audit.New(db.DB) + us := users.New(db.DB, aud) + if _, err := us.Create(ctx, 0, "admin@example.com", "Claus", users.RoleAdmin); err != nil { + t.Fatalf("create admin: %v", err) + } + sealer, err := auth.NewAEADSealer([]byte(key)) + if err != nil { + t.Fatalf("NewAEADSealer: %v", err) + } + ids := auth.New(db.DB, aud, auth.WithSealer(sealer)).Identities() + idID, _, err := ids.Mint(ctx, 1) + if err != nil { + t.Fatalf("Mint: %v", err) + } + if err := ids.Activate(ctx, idID); err != nil { + t.Fatalf("Activate: %v", err) + } + const secret = "JBSWY3DPEHPK3PXP" + if err := ids.PutTOTPSecret(ctx, idID, []byte(secret)); err != nil { + t.Fatalf("PutTOTPSecret: %v", err) + } + + // The wizard persists the config (`finish`). + pre, err := httpx.NewPreSessionCSRF([]byte(cfg.Secrets.CookieSecret), false) + if err != nil { + t.Fatalf("NewPreSessionCSRF: %v", err) + } + wiz, err := setup.New(setup.Deps{ + Config: cfg, ConfigPath: cfgPath, Users: us, Identities: ids, + Enroll: stubEnroller{}, CSRF: pre, Audit: aud, Printout: io.Discard, + }) + if err != nil { + t.Fatalf("setup.New: %v", err) + } + form := url.Values{"app.locale": {"en-GB"}, "app.timezone": {"Europe/London"}} + req := httptest.NewRequest("POST", "/setup?do=setup&action=finish", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + if err := wiz.Finish(rec, req); err != nil { + t.Fatalf("Finish: %v", err) + } + if rec.Code != http.StatusSeeOther { + t.Fatalf("Finish = %d, want 303", rec.Code) + } + if err := db.Close(); err != nil { + t.Fatalf("close db: %v", err) + } + + // Restart: the persisted config must carry the SAME TOTP key, and the + // sealed secret must still open with it (D-1 — the whole point). + restored, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("the persisted config does not load after setup: %v", err) + } + if restored.Setup { + t.Error("the persisted config still says setup: true") + } + if restored.Secrets.TOTPEncryptionKey != key { + t.Fatalf("the persisted TOTP key differs from the runtime key (D-1):\n got %q\nwant %q", + restored.Secrets.TOTPEncryptionKey, key) + } + + db2, err := store.Open(restored.Database.Path) + if err != nil { + t.Fatalf("reopen db: %v", err) + } + defer func() { _ = db2.Close() }() + sealer2, err := auth.NewAEADSealer([]byte(restored.Secrets.TOTPEncryptionKey)) + if err != nil { + t.Fatalf("restart sealer: %v", err) + } + ids2 := auth.New(db2.DB, nil, auth.WithSealer(sealer2)).Identities() + got, err := ids2.TOTPSecret(ctx, idID) + if err != nil { + t.Fatalf("the TOTP secret no longer unseals after a restart (D-1/M2c): %v", err) + } + if string(got) != secret { + t.Errorf("TOTP secret after restart = %q, want %q", got, secret) + } +}