[djbclark]

Summary

• Add a single Rust sudo-secretspec binary that mediates SecretSpec access through a root-owned broker path
• Fail-closed SQLite audit (value-free), metadata-only doctor/drift checks, explicit install/adopt/rollback
• Homebrew packaging and downstream release tooling with no privileged install side effects

Test plan

• cargo test -p sudo-secretspec-cli --locked (58 passed)
• pytest -q tests/sudo_packaging (9 passed)
• brew style packaging/homebrew/sudo-secretspec.rb
• Live dry-run install with Touch ID against adopted /var/db/stayturgid-secrets (operator)
• Independent security review of final Rust boundary

Notes

• Branch is based on exact upstream v0.19.1 via sudo-main
• Downstream package version: 0.19.1-djbclark.1
• No upstream PR intended

[djbclark]

Privilege-boundary security review (PROMPT-SECREV.md)

Reviewed at sudo-main @ ecd03c2, which is ahead of this PR's merge commit (adds uninstall.rs, the per-operation auth gate, and the doctor shadow check). Scope: broker.rs, audit.rs, drift.rs, install.rs, rollback.rs, uninstall.rs, config.rs, main.rs, Cargo.toml, packaging/homebrew/sudo-secretspec.rb, packaging/release.py.

cargo test -p sudo-secretspec-cli → 101 passed, 0 failed (lib 50, audit 19, cli 3, config 3, drift 10, install_rollback 16). The checklist's "58 tests" is stale.

Legend: 🔴 SECURITY (exploitable / stated invariant violated) · 🟡 ADVISORY (hardening) · 🟢 OK (invariant holds)

---

🔴 SECURITY-1 — the root broker loads the caller's user-global SecretSpec config

broker.rs::execute purges four SECRETSPEC_* variables and then calls secretspec::Secrets::load_from(&manifest). That constructor calls GlobalConfig::load() (secretspec/src/secrets.rs:833), which reads the HOME-derived user-global config (~/.config/secretspec/config.toml, config.rs:2609). Two levers survive the purge inside a root process:

1. [audit] retargets a root-privileged writer. AuditLogger::from_config is built from that file (secrets.rs:836). enabled defaults to true, path may be any absolute path, and JsonlSink::new (secretspec/src/audit.rs:217) does DirBuilder::recursive(true).mode(0o700).create(parent) then OpenOptions::create(true).append(true).mode(0o600).open(path) — as root. Worse, max_size_bytes is caller-supplied too, and the sink calls set_len(0) on the existing file when the next line would cross the cap: max_size_bytes = 1 turns any absolute path into root-privileged truncate-then-append. The record itself contains the plaintext reason, project, profile, secret key names and provider URI — i.e. the value-free-audit property holds for the SQLite ledger but is bypassed for the reason on this path.
2. [defaults] profile picks the profile. resolve_profile_name (secrets.rs:1855) falls back to the user-global default. The broker purges SECRETSPEC_PROFILE but never pins a profile, so this decides which profile of the protected manifest resolves.

Precondition: HOME (or the XDG var etcetera consults) must reach the broker process. The installed policy sets env_reset for the libexec path, but macOS's stock /etc/sudoers ships env_keep entries and is unreadable here, so I could not verify it — please confirm with:

sudo /usr/bin/printenv HOME XDG_CONFIG_HOME

If that prints the operator's home rather than /var/root, this is live today. Regardless of the answer, "fixed paths in broker" is stated as the control-plane mitigation and this is a config file outside the boundary.

Fix: in execute(), alongside the existing purge, set HOME to the vault-owner-independent fixed value (or remove_var HOME/XDG_CONFIG_HOME/XDG_STATE_HOME), and pin the profile explicitly with set_profile rather than letting resolution fall through.

---

🔴 SECURITY-2 — the SECRETSPEC_* purge covers 4 of ~20 variables, including none of the exec-path ones

Threat-model row: "execute() purges ambient SECRETSPEC_* env". It purges SECRETSPEC_PROVIDER/FILE/PROFILE/SCOPE. The library reads at least:

SECRETSPEC_OPCLI_PATH  SECRETSPEC_BWS_CLI_PATH  SECRETSPEC_PASSBOLT_CLI_PATH
SECRETSPEC_PROTONPASS_CLI_PATH   ← each names an executable the library spawns
SECRETSPEC_KDBX_PASSWORD  SECRETSPEC_PASSBOLT_PRIVATE_KEY_FILE  SECRETSPEC_PASSBOLT_PASSPHRASE
SECRETSPEC_PASSBOLT_PRIVATE_KEY  SECRETSPEC_PASSBOLT_SERVER  SECRETSPEC_REASON
SECRETSPEC_AGENT  SECRETSPEC_PROVIDER_CONCURRENCY

The *_CLI_PATH four are the sharp ones: a caller-chosen absolute path that a root process would exec. Not reachable today — set_provider collapses every per-secret route to the pinned dotenv:// store (plan.rs:65, plan.rs:226), so no external provider CLI is ever built — but the guard is one manifest or routing change away from root code execution, and SECRETSPEC_REASON already feeds env_reason() as the fallback reason if with_reason ever normalizes to None.

Fix: the client already does this correctly in main.rs:640-644 — mirror it:

for (key, _) in std::env::vars_os() {
    if key.to_string_lossy().starts_with("SECRETSPEC_") { unsafe { std::env::remove_var(&key) } }
}

---

🟡 ADVISORY-1 — a crashed mutation hard-fails doctor, defeating the deliberate PENDING_ROLLBACK advisory

Mutation::begin creates the backups with std::fs::copy while running as root. fs::copy on macOS preserves mode but not ownership — I verified this empirically (source 0600 user:everyone → destination 0600 user:wheel). So .secretspec.toml.rollback.<uuid> and .secretspec.env.rollback.<uuid> land root-owned inside a vault owned by the service user.

drift::inspect then runs both checks on that entry: it pushes PENDING_ROLLBACK (advisory, by design — ADVISORY_CODES, and drift.rs:82-88 explains that a permanent advisory must not wedge automated callers), and it falls through to check_protected_file(&path, service_user, service_group, 0600), which emits a non-advisory METADATA_MISMATCH. Report.ok is false, doctor exits 1, and every agent instructed to treat a drift failure as a hard stop is wedged — precisely the outcome the advisory carve-out exists to prevent.

Fix: chown the backups to the vault owner in Mutation::begin (as audit::open_connection step 5 already does for the ledger), or skip check_protected_file for .rollback. entries.

Mutation::restore() is unaffected: the destination already exists, so the copy truncates in place and the inode's ownership is preserved.

---

🟡 ADVISORY-2 — Mutation::begin failure leaves an attempt with no terminal event

Mutation::begin(&cfg, transaction)? (broker.rs:340) is the one early return that happens after the attempt event is appended. A rollback-path collision or a copy failure therefore returns exit 2 with a dangling attempt and no terminal row, breaking the attempt/terminal pairing the ledger otherwise maintains. Append a failure/unknown terminal before returning.

🟡 ADVISORY-3 — the verification paths assert no ownership

run_audit_verify skips require_boundary() entirely and calls audit::verify(&cfg.vault, None); drift::inspect does the same (drift.rs:625). With expected_uid = None, check_protected_dir and check_ledger_metadata skip their owner comparisons — on the one command whose whole job is to prove the ledger is intact. Pass Some(service_uid) (broker: after resolving the service user; doctor: from layout.service_user).

🟡 ADVISORY-4 — the broker's file-mode check is looser than doctor's

require_boundary accepts any manifest/.env mode with mode & 0o077 == 0, so 0700 or 0400 pass; drift requires exactly 0600. The enforcing side should not be the permissive one — compare against 0o600 exactly.

🟡 ADVISORY-5 — the ledger's residual-risk note overstates truncation detection

audit.rs:26-30 says the chain "detects modification and truncation of the tail, because every event commits with the singleton head row", and names only whole-ledger deletion as undetectable. But head lives in the same database: anyone who can write the ledger can delete the last N events and rewrite the singleton head to the new tip, and verify_rows re-verifies cleanly (count is the last row's sequence, and expected_head is derived from the rows that remain). Tail truncation is only detectable against an externally pinned tip — the same caveat the note already draws for full deletion. Worth correcting, since "hash-chained events + atomic head" is the stated mitigation for audit tampering.

🟡 ADVISORY-6 — --reason crosses the boundary as plaintext argv

The checklist asserts "--reason SHA-256 hashed before crossing broker boundary". It is not: main.rs:570 passes the raw string to sudo -n … --reason <text>, and reason_sha256() runs broker-side. On macOS KERN_PROCARGS2 is same-uid-or-root, so this is not cross-user exposure — but every other agent running as the operator, plus shell history, sees it. Either hash client-side (the broker only needs a 64-hex digest and a non-empty check) or drop the claim.

🟡 ADVISORY-7 — the installer does not check the directories it writes into

is_protected_dir's doc says "The installer requires it of every directory it writes into", but validate_protected_ancestors() lists only /usr, /usr/local, /private, /private/var, /private/var/db, /private/etc, /private/etc/sudoers.d — not /usr/local/{bin,libexec,etc,share}, which is where the NOPASSWD broker binary, the client, and the config actually land. drift does cover them (check_ancestor_chain walks up to /usr), so this is install-time only, and reaching a group/world-writable /usr/local/libexec needs root or a legacy chown -R of the Intel-Homebrew prefix. Cheap to close: run the same is_protected_dir predicate over the four target dirs after create_dir_all.

🟡 ADVISORY-8 — the release gate never runs the companion's tests

release.py::run_tests runs pytest tests/sudo_packaging and cargo build -p secretspec. cargo test -p sudo-secretspec-cli — every boundary, ledger, rollback and uninstall test, 101 of them — does not gate a release. Add it.

🟡 ADVISORY-9 — secrets linger unzeroized in the client

run_target captures the whole export with .output() into a Vec<u8>, parses it to a serde_json::Value, and copies each value into the child env. Three plaintext copies on the heap, none zeroized, before exec. The library uses SecretString elsewhere; the client is the weak link. Minor — the values are headed into the child's environment anyway.

🟡 ADVISORY-10 — invoked_as_privileged_broker() fails open

current_exe() or canonicalize() failing returns false, which lets install/rollback/uninstall through the in-binary allowlist. The sudoers policy still blocks them (it grants only __broker * and doctor), but main.rs:157-163 presents this as the second layer that catches a policy mistake, so it should fail closed.

---

🟢 OK — invariants that hold

• Boundary. require_root() is geteuid() == 0. require_boundary() checks the vault (non-symlink, directory, 0700, owned by the configured service user, canonicalize() equal to the pinned vault_realpath) and both runtime files (non-symlink, regular, correct owner, no group/other bits) before any operation.
• Allowlist. Exactly source-{get,set,add,delete,check,export}, source-template-check, audit-verify; everything else exits 2.
• Fail-closed audit. Empty --reason is refused before any operation; the attempt event is appended before the mutation begins and its failure aborts; the terminal event is mandatory and its failure yields 126 (distinct from the operation's own code).
• Mutation safety. restore() returns bool, a failed restore maps to the unknown phase and exit 125, commit() removes both backups.
• Secret names cannot reach the library unvalidated. --name is embedded in the attempt event, and append_event rejects anything outside ^[A-Z][A-Z0-9_]*$ before opening the database — so an invalid name fails the audit and the operation never runs.
• The dotenv pin is absolute. An explicit builder override collapses every per-secret providers chain to that single store with no fallback (plan.rs:64-65, plan.rs:225-226), so a manifest-declared chain cannot escape dotenv:///var/db/…/.env. The URI is built with three slashes, as the absolute form requires.
• No value in argv. set/add call secrets.set(name, None), so the value is prompted by the root process; get writes to inherited stdout; no shell anywhere, and run reaches the target through CommandExt::exec.
• Ledger. STRICT tables; BEGIN IMMEDIATE with event + head upsert committed together (the fault-injection test proves they roll back together); umask(0o077) around create; ledger chowned to the vault owner when root; post-open (dev, ino) re-check rejects a swap during open; PRAGMA integrity_check in verify. The canonical JSON actually covers sequence and timestamp_ns — stronger than the doc comment above canonical_event_json, which says they are excluded; worth fixing the comment, not the code.
• rollback.rs. canonicalize before the prefix test, snapshot must be root-owned 0700, every destination must appear in installed_artifacts(), every prior file must match the snapshot manifest hash, the restored mode comes from the artifact table (not the snapshot), sudoers is re-staged through stage_sudoers + visudo -c, and the vault is never touched.
• install.rs. --adopt-existing verifies mode and runtime-file shape without ever chowning or chmodding the vault; dry-run still requires root; sudoers is staged under a dotted (sudo-ignored) name, validated in isolation, then re-validated combined with a restore-on-failure path; snapshots are captured before the first overwrite and pruned under the same guards rollback applies.
• uninstall.rs. Policy is planned first so the grant closes before the paths it names disappear; a policy the install manifest cannot account for is kept; --purge-vault re-resolves and requires a direct child of /private/var/db; --remove-service-user refuses anything without a leading underscore or outside uid/gid 400–499; symlink_metadata throughout keeps a fifo or symlink at an owned path from being opened or followed.
• config.rs. deny_unknown_fields, absolute paths with no .., vault constrained to one direct child of /var/db whose name must match the /private/var/db spelling, service identity ^_?[a-z_][a-z0-9_]{0,30}$ with the underscore required for the user.
• Cargo.toml. rusqlite = { version = "0.31" } with no bundled feature (system SQLite), secretspec as a path dep with default-features = false.
• Homebrew formula. Files only — no sudo, no dscl, no sudoers edit, no /var/db mutation; the companion goes to libexec (unlinked), the caveat directs the operator to run install explicitly, and brew test asserts bin/sudo-secretspec does not exist.
• release.py. Tag/version regex pinned to 0.19.1-djbclark.N, release URL forced to HTTPS + github.com, tarball SHA-256 rewritten into the formula with a pinned count of version sites, tap synced and brew tested, and readback verifies repo slug, fork parent cachix/secretspec, tag ref, published non-draft release, and the installed binary's version.

---

Priority order if you want one: SECURITY-2 (one loop, mirrors existing code), SECURITY-1 (needs the sudo printenv HOME answer first), then ADVISORY-1 (it silently converts a designed advisory into a hard stop for every automated caller).