Harden config persistence so API keys and env secrets cannot leak

[aflanagan]

Stop displaying Cronitor API keys, ping-auth keys, dashboard credentials, and unrelated environment secrets through configure output, verbose dumps, signup errors, and new config files. Supported CLI flags and default paths are unchanged. Existing config files keep their permissions, group, and ACLs.

What changed

• cronitor configure prints API Key: Set / Ping API Key: Set (or Not Set) instead of complete keys. Dashboard passwords stay ********. MCP instance passwords are not printed.
• --api-key, --ping-api-key, and --dash-password remain for compatibility; help and README warn that they appear in shell history and process lists. Prefer env vars.
• The os.Environ() dump under --verbose is gone. Verbose output lists an allowlist of Cronitor variable names with Set / Not Set, never values.
• Signup persist failures (CLI and dashboard HTTP) no longer interpolate returned keys. Dashboard signup JSON no longer returns api_key / ping_api_key. Signup merges the two keys into the existing file's JSON, removing any case variant of them first, so settings outside the ConfigFile struct survive and no stale lowercase copy remains.
• All credential-bearing writes go through persistConfigFile: configure, signup, and the three dashboard save paths. Unrelated 0644 writes (debug logs, crontabs, --output files) are unchanged.

File permissions

Every save stages in a sibling temp file and renames it over the destination. Readers see the old file or the new one, never a partial one, and a failed write leaves the old file intact.

• Existing files: the temp is given the existing file's access before the secret is written, and the result is verified. Mode bits, re-checked after the ACL step. Owner and group; when the writer cannot give the file away (only root can), the group is kept on its own, and the save is refused if a group that others depend on cannot be kept. Extended attributes. The access ACL: copied when the original has one, removed when it does not so a directory default ACL cannot add a grant, and read back to confirm an exact match or the save fails. Linux reads and writes system.posix_acl_access; macOS uses getattrlist/setattrlist with ATTR_CMN_EXTENDED_SECURITY, the interface acl_get_file/acl_set_file are built on; Windows copies the DACL. A 0644 system file stays 0644; a root:shared 0660 file saved by a group member keeps its group and mode. Upgrading never changes who can read the config.
• New files get a bare owner-only temp: 0600 on Unix with any inherited ACL removed, a protected owner-only DACL on Windows applied before the first byte is written. configure prints a note at creation saying other users cannot read the file, with the env-var, per-user-file, and chmod options.
• cronitor configure --restrict takes the new-file path for an existing file: mode bits and ACL grants are both dropped.
• After a save that leaves an existing file readable by other users, configure prints a note on stderr naming --restrict. The note never changes the file.
• If the config file exists but cannot be read, the CLI prints one warning to stderr with the path and a reason (permission denied, file could not be opened, file is not valid JSON), never the parser's text. A missing file stays silent.
• Symlinks at the config path are refused.
• FreeBSD: mode, owner, and group are preserved; ACLs are not reachable through the xattr interface and are not preserved. Documented limitation.

Verified scenarios

Linux, real binary as root and as nobody:

Scenario	Result
Root saves over an existing root-owned 0644 file	Stays 0644, content merged, nobody still reads it
Group member (nobody) saves a root:nogroup 0660 file carrying an xattr, in a 2770 dir	nobody:nogroup 0660, xattr intact, content updated
0640 file with no ACL, in a directory whose default ACL grants nobody read	Stays 0640 with no ACL; nobody cannot read before or after
New file in that same directory	0600, no inherited ACL; nobody cannot read
--restrict on a file carrying a grant	0600, grant gone
Existing access ACL on the file	Identical bytes after save
Root creates a new file	0600, creation notice on stderr, no key in output
Data write fails halfway	Original file byte-identical, no temp left behind
Missing config file	Silent, exit 0

macOS, go test ./cmd -run TestDarwinACL -v run by the reviewer: a plain file reads as "no ACL" and saves; an explicit everyone allow read grant survives a normal save and is dropped by --restrict; a file_inherit directory grant is not added to a saved or new file.

Tests

Go tests in cmd/config_persist_test.go, cmd/config_persist_unix_test.go, cmd/config_persist_linux_test.go (real POSIX default ACL on the temp dir), cmd/config_persist_attrlist_test.go (attrreference layout, all platforms), cmd/config_persist_darwin_test.go, cmd/config_persist_windows_test.go, and cmd/configure_secrets_test.go; bats cases in tests/test-configure.bats. All fixtures use *-not-real strings. GOOS=linux|darwin|freebsd|windows go vet ./cmd/ all pass. Rebased on master with #52 merged.

🤖 Generated with Claude Code

https://claude.ai/code/session_0134bj2HYgae8W9cxdLicpeo

[cursor]

APPROVE

Reviewed at 63c3c5fb94 against the locked credential-hardening spec. All eleven items are met. GitHub will not let me submit a formal approving review on a PR authored by this account, so this comment is the review verdict; three inline comments follow on the notes below.

What I checked

Spec item	Result
1. configure never prints complete keys; no --show-secrets	Pass — secretPresenceLabel prints Set / Not Set; no such flag anywhere in the tree
2. Dash password redacted, MCP passwords not printed	Pass — ********; MCP loop prints name and URL only
3. Verbose prints allowlisted names only, no values	Pass — printCronitorEnvSources iterates a fixed CRONITOR_* list and discards the value from LookupEnv; the old os.Environ() dump is gone
4. Signup persist errors omit keys, give safe recovery	Pass — both the CLI (cmd/signup.go) and the dashboard handler go through formatSignupPersistError
5. Dashboard signup JSON omits keys	Pass — returns {status, message}
6. One shared persist helper for all five credential writes	Pass — configure, signup, dash credential prompt, dash settings POST, dash signup. The unrelated 0644 writes (debug log, crontabs, --output dumps) are untouched
7. Unix 0600 / atomic / no symlink / reject 0644+ / keep 0640	Pass — and notably it does not chmod an existing /etc/cronitor/cronitor.json
8. Windows owner-restricted ACL, limitations documented	Pass, with two small gaps in the notes below
9. Flags kept, README has no literal keys, migration documented	Pass
10. Tests cover the required cases with fake keys only	Pass — every new fixture is a *-not-real string; no live API calls added
11. No release or live changes; browser auth is follow-up only	Pass — no version bump, no workflow changes

A few things I confirmed beyond reading the diff, since they were the plausible ways this could break something:

• Removing the keys from the signup JSON does not break the dashboard UI. SignupForm.js passes the parsed body straight through to LearnMoreModal.js and then JobCard.js, and none of them ever read api_key or ping_api_key — the object is only used as an opaque "signup worked" signal. It branches on response.ok, which still holds.
• The atomic-write rollback is real. Content only ever goes into the sibling temp file; the destination is replaced by rename after write, sync, close and permission fix-up, and the deferred cleanup removes the temp on any failure. There is no path that truncates or destroys the previous valid config.
• On Unix the secret never touches a file looser than requested. os.CreateTemp already creates at 0600, and the mode is applied before the first Write. The only relaxation to 0640 happens on a temp file that is about to replace an existing 0640 file.
• Builds clean for linux/amd64 and windows/amd64. gofmt is clean on all touched files. I ran only the new hardening tests (go test ./cmd -run 'TestPersistConfigFile|TestConfigure|TestSaveSignup|TestFormatSignup|TestSignupSuccess|TestPrintCronitorEnvSources') rather than the full suite, since CI covers the rest — they pass.

Three non-blocking notes

1. On Windows the temp file holds the key before the ACL is applied. persistApplyMode is a no-op on Windows, so the owner-only DACL is set after the secret has already been written and closed. A temp file created in %ProgramData%\Cronitor inherits that folder's ACL, which normally grants BUILTIN\Users read. Also, sddlContainsWorldAllow only recognises Everyone (WD) and BUILTIN\Users (BU), so a file granting Authenticated Users (AU) read would pass the existing-file check.
2. The migration text an existing Windows install will see is Unix-only. A config file written by the current release inherits Users: Read, so the first configure after upgrading will reject it and then tell the operator to run chgrp and chmod 0640. Windows CI won't catch this because it always starts from no file at all.
3. GET /api/settings still returns the complete API key, ping key, dashboard password and MCP passwords to the browser, and the POST handler echoes the same fields back. This is pre-existing, the Settings UI depends on it, and it is outside this spec — but with the CLI-side leaks closed it is now the widest remaining path a key takes out of the process, so it belongs next to the cronitor auth login follow-up.

None of these block the merge.

[cursor]

Non-blocking note 1 of 3.

Because this is a no-op on Windows, the owner-only DACL is only applied after the key has been written to the temp file. In atomicWriteConfigFile the order is: CreateTemp → persistApplyMode (nothing happens here on Windows) → Write(data) → Sync → Close → then persistLockdownNewFile or persistPreserveSecurity.

A file freshly created in %ProgramData%\Cronitor inherits that folder's ACL, and %ProgramData% normally grants BUILTIN\Users read down the tree. So for the duration of the write there is a temp file containing the API key that any local user can open. It's a narrow race — you'd need something watching the directory for change notifications — but it is avoidable: calling applyOwnerOnlyACL(tmpName) from persistApplyMode (or right after os.CreateTemp) would close it, and it would match what the Unix side already gets for free from CreateTemp's 0600.

Separately, at line 140 sddlContainsWorldAllow matches only WD (Everyone) and BU (BUILTIN\Users). An existing file whose ACL grants Authenticated Users (AU) read would pass the check and be treated as restricted enough, even though on a multi-user box that is effectively the same exposure as BU. Worth adding AU to that list.

I could not exercise either of these from Linux, so the inheritance detail is worth a sanity check on a real Windows box.

[cursor]

Non-blocking note 2 of 3.

Rejecting the file here is exactly what the spec asks for, so this is about the message rather than the behaviour. The Windows path reuses configMigrationGuidance, which is written for Unix: it names allowed modes 0600 and 0640, and its three options are "chgrp + chmod 0640", "a user-owned 0600 file", and env vars. On Windows the first two aren't actionable at all.

This matters more than it looks, because I think every existing Windows install hits it. The current release writes the config with ioutil.WriteFile(path, b, 0644), which on Windows just inherits the parent ACL — and %ProgramData% grants BUILTIN\Users read. So the first cronitor configure (or dashboard settings save) after upgrading should see BU in the DACL, reject the file, and print chgrp/chmod instructions.

Windows CI won't surface this: setup_suite.bash starts from a runner with no C:\ProgramData\Cronitor\cronitor.json, so the first write always takes the new-file path and gets a clean protected DACL.

A Windows-specific branch in the guidance text would fix it — something along the lines of "set CRONITOR_API_KEY in the service environment, or delete the existing file and re-run cronitor configure to have it recreated with an owner-only ACL", optionally mentioning icacls for operators who want to inspect it. Same fix would make the two configOverlyPermissiveError(path, 0644) calls less confusing, since 0644 is a synthetic mode here rather than something the operator can see or change.

[cursor]

Non-blocking note 3 of 3 — follow-up, not something to change here.

Routing this write through persistConfigFile is correct and is one of the five sites the spec asks for. Flagging the surrounding handler only because it is now the widest remaining path a key takes out of the process.

SettingsResponse embeds ConfigFile, so the GET branch serialises CRONITOR_API_KEY, CRONITOR_PING_API_KEY, CRONITOR_DASH_PASS and every mcp_instances[].password to the browser in full, and further down it overwrites response.DashPassword with the live CRONITOR_DASH_PASS value. A few lines below this one, the POST branch echoes the same b it just persisted back in the response body.

All of that is pre-existing, the Settings UI reads those fields into its form inputs, and it is outside this spec — so leaving it alone is the right call for this PR. But once the CLI-side leaks are closed, an API key travelling over HTTP on port 9000 is the biggest one left, and the dashboard's auth is optional. This seems like the natural companion to the cronitor auth login work mentioned in the PR description: return presence flags plus a masked tail for display, and have the POST only accept a new value when the field actually changed.

One smaller thing in the same area: the http.Error(w, err.Error(), ...) on the next line now sends the full persist error to the client, which includes the absolute config path and the whole migration block. No secrets in it, but it is more internal detail than the previous fixed string, and this endpoint can be unauthenticated when no dashboard credentials are set.

[cursor]

Design update: we no longer reject existing 0644/0640 credential files. exec/ping still read them. The next configure, signup, or dash save rewrites the file atomically as 0600 (Windows: owner-only ACL) and prints a warning that other users lose read access unless CRONITOR_API_KEY / CRONITOR_PING_API_KEY are set in the crontab/service environment, or the operator sets up a shared 0640/ACL file themselves. Unexpected symlinks are still refused; secrets are still not printed.

[cursor]

APPROVE

Re-reviewed at 3c16d76b1a. My earlier approval at 63c3c5fb94 is void; this is a fresh verdict against the revised spec. The new write behavior does what was asked, and I found no regression of the parts of the original spec that survived. Three inline comments follow with non-blocking notes.

The revised behavior

Requirement	Result
Do not reject existing 0644+ on write	Pass — checkExistingConfigPerms and configOverlyPermissiveError are gone entirely. Nothing in the write path returns an error based on file mode or ACL any more
Reads still work on the old file	Pass — the read side is untouched; the file is only modified when something actually saves
Next write rewrites atomically as 0600 with a warning	Pass — persistLockdownNewFile now runs on every write instead of only for new files, and configAccessNarrowedWarning prints to stderr after a successful narrowing save. It names all three recovery routes: env vars in the crontab or service, a shared file the operator sets up themselves, or a per-user 0600 file
0640 also becomes 0600	Pass — covered by TestPersistConfigFile_Existing0640RewrittenTo0600WithWarning and the matching BATS case
Warning only when access actually narrows	Pass — existingAccessWillNarrow checks Perm()&0077, so an already-0600 file saves silently. TestPersistConfigFile_Existing0600DoesNotWarn pins that
No secret printing, no --show-secrets, symlinks still refused	Pass — the warning text is a constant with no interpolated values, and the tests assert the key never appears in it. The symlink and non-regular-file refusals are unchanged
Windows: tighten on write, never fail on an open old ACL, actionable text	Pass — existingAccessWillNarrow returns "warn" rather than an error when it cannot read the ACL, and the guidance now talks about scheduled tasks, Windows services and file ACLs instead of chgrp. AU (Authenticated Users) was also added to the world-readable check

Dropping copyFileDACL is a real improvement on Windows, not just a simplification: the update path now applies a fresh owner-only DACL instead of carrying the old file's ACL forward, and the post-rename re-apply covers the case where the destination's security descriptor could otherwise survive the replace.

Regressions of the original spec

None. All ten of the original items still hold: keys and passwords are still redacted in configure output, the verbose env listing is still an allowlist of names with no values, signup persist errors still omit the returned keys, the dashboard signup JSON still returns only {status, message}, all five credential writes still go through persistConfigFile, and the unrelated 0644 writes are still left alone.

Two details I specifically re-checked because the rewrite could plausibly have broken them:

• The atomic rollback still holds. Content only ever goes into the sibling temp file, and the destination is replaced by rename after write, sync, close, chown and chmod all succeed. TestPersistConfigFile_AtomicFailureLeavesOriginal still passes, and it still finds CRONITOR_CONFIG in the error text because the rewritten configMigrationGuidance kept that string.
• The signup-error test still tests something. It used to force a failure with a 0644 file, which no longer fails by design. Pointing the config path at a directory instead is a better trigger, and it now runs on Windows too.

Builds clean for linux/amd64 and windows/amd64, the Windows test binary compiles, gofmt is clean on every touched file, and no dangling references to the six removed helpers remain. I ran only the hardening tests rather than the full suite; all 16 pass, including the three new mode-rewrite cases.

Also in range but outside the spec: tests/test-exec.bats widens the duration bound from 0.5–2s to 0.3–30s. I have no objection given Windows CI startup timing, though at 30s the assertion is close to "a number was recorded at all".

Three non-blocking notes

1. Nobody sees the warning when the save comes from the dashboard. handleSettings tightens the file and returns 200, while the warning goes to the cronitor dash process's stderr — a journal or container log, not the browser. An operator who clicks Save in Settings silently breaks their non-root cron jobs. The CLI prompt path at cmd/dash.go:725 is fine because that one prints to the user's terminal.
2. A failed chown aborts the whole write. Preserving the previous owner is a nice-to-have, but treating it as fatal turns a save that used to work into a hard failure for a non-owner on a group-writable config. I confirmed the EPERM locally.
3. The post-rename lockdown reports failure after the data is already committed. If that last persistLockdownNewFile fails, the new content is on disk but the caller is told the save failed — and cronitor signup would tell the user their credentials could not be saved when in fact they were.

None of these block the merge.

[cursor]

Non-blocking note 1 of 3.

This is the one place where the new tighten-and-warn behavior loses its warning. persistConfigFile prints configAccessNarrowedWarning to the cronitor dash process's stderr, but this is an HTTP handler — the operator who just clicked Save in the Settings UI is looking at a browser, and the dash server's stderr is a terminal they walked away from, a systemd journal, or a container log. The handler then returns 200 with the config body and no indication that anything changed about who can read the file.

So the practical outcome on a machine with the usual world-readable /etc/cronitor/cronitor.json: someone edits a setting in the dashboard, the file silently becomes 0600, and every non-root cron job that was reading it starts failing with no visible connection to what they just did. That is exactly the situation the warning exists to prevent.

The CLI paths are all fine — configure and the first-run credential prompt at line 725 both print to the user's own terminal, and the BATS cases confirm the warning shows up in configure output.

Worth surfacing it in the response so the UI can show it. The smallest version is to have the persist helper hand back whether it narrowed access (a bool, or a typed warning alongside the error) instead of printing directly, then include it in this handler's JSON as something like "warning": "..." for the Settings page to render as a banner. That would also let handleSignup do the same thing, and it would stop the warning from being written into the middle of the cronitor signup Bubble Tea UI, which is the other caller where stderr is not a great destination.

[cursor]

Non-blocking note 2 of 3.

Preserving the previous owner is the right instinct, but making it fatal is stricter than it needs to be and it can turn a save that used to work into a hard failure.

persistPreserveOwner chowns the temp file to the existing file's uid and gid, and POSIX only lets you give a file away if you are root. So if the config file is owned by someone else and is group-writable, a non-owner who could previously update it now cannot. I confirmed both halves on this box:

-rw-rw-r-- 1 root ubuntu cronitor.json   # 0664, owned by root, group ubuntu

$ printf '...' > cronitor.json           # what the pre-PR ioutil.WriteFile did
plain write SUCCEEDED

$ chown 0:1000 mytemp                    # what persistPreserveOwner does
chown: changing ownership of 'mytemp': Operation not permitted

That EPERM propagates up through wrapPersistWriteError and aborts, so configure exits 126 with a message about the file not being writable — which is confusing, because the file plainly is writable, and the whole point of the new behavior is that this save should have tightened the file to 0600 instead of refusing.

The blast radius is limited: it needs a 0664 or 0660 credential file owned by another account, which is unusual, and the old file is left intact so there is no data loss. The common upgrade path (root saving a root-owned 0644 file, or a user saving their own --config file) chowns to values it already has and succeeds.

Since the result of skipping the chown is a file owned by the writer at mode 0600 — no worse from a secrets standpoint, arguably better — I would just let it be best-effort:

if exists {
    // Best effort: keeping the previous owner is nice for shared installs,
    // but a non-root writer cannot give the file away and the 0600 result
    // is safe either way.
    _ = persistPreserveOwner(tmpName, existing)
}

If you would rather keep it strict, skipping the call when the uid and gid already match would at least avoid the pointless syscall in the common case.

[cursor]

Non-blocking note 3 of 3.

Belt-and-braces here is reasonable, especially for Windows where you do not want the destination's old security descriptor to survive the replace. The problem is only what happens when it fails: renamed is already true and the new content is on disk, but returning an error tells every caller the save did not happen.

That produces a genuinely wrong message rather than just an ugly one. cronitor signup would print "your account was created but credentials could not be saved. Sign in at cronitor.io or retry authorization" — while the keys are sitting correctly in the config file. A user following that advice would go re-authorize an account that was already set up. configure would exit 126 after having successfully written the file, and the dashboard settings save would return a 500 for a save that landed.

It is unlikely to fire — on Unix this is a chmod on a file we either own or are root for, immediately after a successful rename — but the failure mode is bad enough to be worth not returning an error from here. Two options, either is fine:

• Treat a post-commit failure as a warning rather than an error: log it or print it to stderr alongside the access-narrowed warning, and still return nil, since the write did succeed and the temp file was already locked down before the rename.
• Or return a distinguishable error so callers can say "saved, but the permissions could not be tightened" instead of "not saved" — which matters most for formatSignupPersistError, whose whole job is telling the user what to do next.

For what it is worth, the pre-rename persistLockdownNewFile(tmpName) at line 146 already guarantees the file is owner-only at the moment it becomes visible, on both platforms, so failing the operation here buys nothing that the earlier call did not already provide.

Separately, and still non-blocking from the previous round: persistApplyMode is a no-op on Windows, so the temp file holds the API key with the parent directory's inherited ACL from the Write on line 131 until the lockdown on line 146. In %ProgramData%\Cronitor that inherited ACL normally includes BUILTIN\Users read. Applying the owner-only ACL right after os.CreateTemp would close that window and match what CreateTemp's 0600 already gives you on Unix.

[aflanagan]

Five findings from the correctness and security review of f284709. Four were reproduced locally; the Windows temporary-file finding is based on code inspection.

[aflanagan]

Three findings from the follow-up correctness and security review of e8a06bf. All three were reproduced locally.

[aflanagan]

Three findings from the follow-up correctness and security review of 459b9cb. The Linux and macOS ACL issues and the FreeBSD build failure were reproduced locally.

[aflanagan]

One confirmed blocker from the follow-up review of 652dd25, reproduced with existing tests and a real macOS binary.

[aflanagan]

Verified on an Apple Silicon Mac: the macOS persistence blocker is fixed in 85dddbb. Plain-file saves, explicit ACL preservation/restriction, and independently checked inherited ACL handling pass. Full Go and race suites pass, and a real configure invocation exits 0. One test coverage correction remains below; no remaining functional blocker was found in this macOS change.