Implement SECURITY-AUDIT.md remediation plan (v1.1.0 audit cleanup) by johnzfitch · Pull Request #18 · johnzfitch/dota

johnzfitch · 2026-05-30T20:57:34Z

Summary

Implements, end to end, the remediation plan recorded in SECURITY-AUDIT.md
from the prior audit session. Closes every open finding (H1, H3, H4 and the
full M/L tier); H2 was already resolved upstream and only its audit-doc
annotation is updated. Version bumped 1.0.0 → 1.1.0.

SECURITY-AUDIT.md now carries a per-finding resolution table at the top.

High

H1 — clipboard + dead UI deps. dota get NAME --copy and the shell
copy NAME route through a new src/cli/clipboard.rs using arboard
(default-features = false, dropping the image crate) with a
std::thread-based auto-clear (no async runtime). ratatui, crossterm,
tokio, and the unused codepage-437 are removed (zero call sites). The
README's TUI keyboard table is replaced with the real dota> shell commands,
and tui/app.rs is reduced to a documented stub.
H3 — migration backups retain old key material. change_passphrase and
rotate_keys scrub vault.backup.*.json into hollowed
vault.tombstone.*.json files: version / KDF params / public keys /
timestamps are kept for forensic correlation while the wrapped private keys,
the key commitment, and the secrets map are nulled. Originals are best-effort
zero-overwritten then unlinked. The copy-on-write caveat is documented in the
README. (create_backup already wrote 0o600-from-creation via
tempfile + persist.)
H4 — RC ml-kem + dead crypto-adjacent deps. ml-kem is exact-pinned
(=0.3.0-rc.0) to stop a floating pre-release from drifting on
cargo update; advancing to a stable line is deferred because it changes the
expanded 2400-byte decapsulation-key on-disk encoding (L3) and needs a
coordinated v8 migration. pqcrypto-kyber/pqcrypto-traits are now optional
behind a new legacy-migration feature (on by default); the legacy module,
step functions, and fixtures are #[cfg]-gated, and the no-feature migration
path returns an actionable error. deny.toml adds license/source/ban
enforcement on top of the existing RustSec audit step.
H2 — already resolved upstream; annotation refreshed.

Medium / Low

M1 (every command unlocks via read_passphrase, so DOTA_PASSPHRASE is honored
uniformly), M2 (okm wrapped in Zeroizing in both hybrid combiners, wiped on
the error path too), M3 (belt-and-suspenders comment), M4/M5/M7 (README
threat-model docs: metadata exposure, stdout exposure, Linux-only hardening),
M6 (32-byte salt from OsRng for new vaults; 16-byte validation floor kept for
legacy), M8 (a non-default --vault parent dir that cannot be restricted to
0o700 now fails loudly; default ~/.dota/ keeps the warning), M9/M10
(migration/backup banners gated on interactive stderr + // SECURITY: notes),
L1 (X25519 zero-check renamed nonzero_acc), L3 (MlKemPrivateKey comment),
L5 (timestamp leak documented), L6 (ratatui removed), L7
(algorithm/parallelism KDF rejection tests present).

A src/lib.rs is added so integration tests call dota::vault::ops directly;
src/main.rs imports from the library crate. No public-API change for binary
users.

Test plan

Verified locally (Rust 1.94):

cargo fmt --check
cargo clippy --all-targets --all-features -- -D warnings
cargo clippy --all-targets --no-default-features -- -D warnings
cargo test — 123 passed, 0 failed
cargo test --no-default-features — 87 passed, 0 failed
cargo build --release and cargo build --release --no-default-features
End-to-end CLI sweep on the release binary: init / set (stdin) /
get / list / export-env round-trip; vault file 0o600, parent dir
0o700; get --copy degrades cleanly on a headless host.

New integration tests

File	Covers
`tests/backup_tombstone_lifecycle.rs`	H3: backup → tombstone on change-passphrase and rotate-keys; tombstones are non-unlockable
`tests/salt_entropy.rs`	M6: 32-byte salt on init, change-passphrase, and rotate-keys

Note: cargo deny is not installed in the dev container, so deny.toml was
validated for structure only; CI / downstream users run cargo deny check.

Findings closed

H1, H3, H4, M1, M2, M4, M5, M6, M7, M8, M9, M10,
L1, L3, L5, L6, L7 (plus H2/M3/L2/L4 annotated).

https://claude.ai/code/session_014uS12GBDEicXLF5CZvLsPQ

Generated by Claude Code

Closes the open findings recorded in SECURITY-AUDIT.md from the prior audit session. Version bumped 1.0.0 -> 1.1.0. High: - H1: real clipboard path (`dota get NAME --copy`, shell `copy NAME`) via a new src/cli/clipboard.rs using arboard with default-features=false and a std::thread auto-clear; removed dead ratatui/crossterm/tokio (and the unused codepage-437) deps; README TUI keyboard table replaced with the actual `dota>` shell commands; tui/app.rs reduced to a documented stub. - H3: change_passphrase / rotate_keys now scrub vault.backup.*.json into hollowed vault.tombstone.*.json files (version/KDF/public-key/timestamp metadata kept for forensics; wrapped private keys, key commitment, and the secrets map nulled), then best-effort zero-overwrite + unlink the originals. COW-filesystem caveat documented in the README. - H4: ml-kem exact-pinned ("=0.3.0-rc.0") to stop floating-RC drift on `cargo update`; pqcrypto-kyber/-traits made optional behind a new `legacy-migration` feature (on by default); legacy step functions, the legacy_kyber module, and matching fixtures are cfg-gated, with an actionable bail message on the no-feature path; deny.toml added. - H2: already resolved upstream; SECURITY-AUDIT.md annotation updated. Medium/Low: M1 (every command unlocks via read_passphrase, env var honored uniformly), M2 (okm wrapped in Zeroizing in both hybrid combiners), M3 (belt-and-suspenders comment), M4/M5/M7 (README threat-model docs), M6 (32-byte salt drawn from OsRng for new vaults; 16-byte validation floor kept for legacy), M8 (non-default --vault parent dir that cannot be restricted now fails loudly; default ~/.dota keeps the warning), M9/M10 (migration/backup banners gated on interactive stderr; SECURITY notes added), L1 (X25519 zero-check renamed nonzero_acc), L3 (MlKemPrivateKey comment), L5 (timestamp leak documented), L6 (ratatui removed), L7 (algorithm/parallelism KDF rejection tests present). Adds src/lib.rs so integration tests exercise dota::vault::ops directly; main.rs now imports from the library crate (no public API change for the binary). New tests: tests/backup_tombstone_lifecycle.rs, tests/salt_entropy.rs. Verified locally (Rust 1.94): cargo fmt --check; clippy --all-targets {--all-features,--no-default-features} -- -D warnings; cargo test (123 pass) and --no-default-features (87 pass); release builds under both feature configs; and an end-to-end CLI sweep on the release binary (init/set via stdin/get/list/ export-env round-trip, vault file 0o600, parent dir 0o700). https://claude.ai/code/session_014uS12GBDEicXLF5CZvLsPQ

test_v5_vault_rejects_stripped_key_commitment exercises the v5->v6 migration path, which only exists under the legacy-migration feature. Without it, unlock bails with the feature-gate message before reaching the v5 commitment check, so the test must be cfg-gated to keep cargo test --no-default-features green. https://claude.ai/code/session_014uS12GBDEicXLF5CZvLsPQ

Copilot

Pull request overview

Implements the v1.1.0 remediation plan described in SECURITY-AUDIT.md, focusing on reducing secret exposure (clipboard path / stdout leakage), hardening migration/backup handling, and trimming supply-chain surface (feature-gated legacy migration + cargo-deny policy).

Changes:

Adds clipboard copy-with-auto-clear for dota get --copy and the interactive copy command; removes unused/dead UI deps.
Scrubs legacy migration backups into “tombstone” files after re-key operations, and gates migration/backup banners on interactive stderr.
Increases new vault KDF salt strength to 32 bytes; introduces a library crate surface for integration tests; adds deny.toml and feature-gates legacy migration.

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 24 comments.

Show a summary per file

File	Description
tests/salt_entropy.rs	New integration test asserting 32-byte salts on init and re-key operations.
tests/backup_tombstone_lifecycle.rs	New integration test validating backup → tombstone scrubbing and non-unlockability.
src/vault/ops.rs	Gates migration banners on interactive stderr; scrubs backups after re-key; tightens directory chmod policy for non-default `--vault`.
src/vault/migration.rs	Feature-gates legacy migration steps; adds backup scrubbing into tombstones; gates banners on interactive stderr.
src/tui/mod.rs	Updates interactive shell to use shared passphrase reader; adds `copy` shell command using clipboard module.
src/tui/app.rs	Documents `ratatui` removal; leaves a stub for future UI work.
src/main.rs	Converts binary into a thin wrapper over the new `dota` library crate; updates `get` to pass `--copy`.
src/lib.rs	Adds library crate root exporting modules for reuse and integration tests.
src/crypto/x25519.rs	Clarifies constant-time nonzero accumulator naming for the all-zero shared-secret check.
src/crypto/mod.rs	Gates legacy Kyber module behind `legacy-migration` feature.
src/crypto/mlkem.rs	Updates documentation around the expanded private-key encoding contract.
src/crypto/kdf.rs	Switches to 32-byte raw CSPRNG salt generation; updates tests accordingly.
src/crypto/hybrid.rs	Wraps HKDF OKM buffers in `Zeroizing`; feature-gates legacy hybrid paths and tests.
src/cli/mod.rs	Adds `clipboard` module; introduces `--copy` flag on `get`.
src/cli/export.rs	Routes through shared `read_passphrase` (env-var support) instead of direct prompting.
src/cli/commands.rs	Centralizes passphrase reading; updates `get` to support clipboard path; adjusts init passphrase sourcing.
src/cli/clipboard.rs	New clipboard integration with blocking/background auto-clear semantics.
SECURITY-AUDIT.md	Adds per-finding resolution table and updated annotations for v1.1.0.
README.md	Updates usage/docs around clipboard copy, interactive shell commands, threat-model notes, and build flags.
llms.txt	Updates repo map pointers for the new library/clipboard/shell layout.
deny.toml	Adds cargo-deny policy for licenses/sources/bans alongside RustSec audit.
Cargo.toml	Bumps version to 1.1.0; exact-pins `ml-kem`; adds `legacy-migration` feature gating; removes TUI deps; configures `arboard` without default features.
Cargo.lock	Reflects dependency removals/additions and version normalization from the above changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    let timestamp = Utc::now().format("%Y%m%d_%H%M%S_%6f");
+    let tombstone_path = parent.join(format!("{}.tombstone.{}.{}", stem, timestamp, ext));
+    super::ops::reject_symlink_path(&tombstone_path, "write tombstone")?;
+    write_private_file(parent, &tombstone_path, serialized.as_bytes())?;


+    println!(
+        "Secret copied to clipboard; it will be cleared in {CLIPBOARD_CLEAR_SECONDS}s. \
+         Press Ctrl-C to exit sooner (the clipboard is then cleared by your session)."
+    );


+/// Every command that unlocks the vault routes through this one function so the
+/// env-var path is honored uniformly (SECURITY-AUDIT.md M1). Callers that need
+/// a *new* passphrase (init, change-passphrase) still prompt-and-confirm
+/// interactively — the env var only supplies the unlock passphrase.


 # Launch TUI (default command)
 dota


+    let timestamp = Utc::now().format("%Y%m%d_%H%M%S_%6f");
+    let tombstone_path = parent.join(format!("{}.tombstone.{}.{}", stem, timestamp, ext));
+    super::ops::reject_symlink_path(&tombstone_path, "write tombstone")?;
+    write_private_file(parent, &tombstone_path, serialized.as_bytes())?;


 # Launch TUI (default command)
 dota


+    let timestamp = Utc::now().format("%Y%m%d_%H%M%S_%6f");
+    let tombstone_path = parent.join(format!("{}.tombstone.{}.{}", stem, timestamp, ext));
+    super::ops::reject_symlink_path(&tombstone_path, "write tombstone")?;
+    write_private_file(parent, &tombstone_path, serialized.as_bytes())?;


+    println!(
+        "Secret copied to clipboard; it will be cleared in {CLIPBOARD_CLEAR_SECONDS}s. \
+         Press Ctrl-C to exit sooner (the clipboard is then cleared by your session)."
+    );


+/// Every command that unlocks the vault routes through this one function so the
+/// env-var path is honored uniformly (SECURITY-AUDIT.md M1). Callers that need
+/// a *new* passphrase (init, change-passphrase) still prompt-and-confirm
+/// interactively — the env var only supplies the unlock passphrase.


 # Launch TUI (default command)
 dota


CI (non-root) failure: the M8 change makes secure_vault_directory hard-fail when a non-default --vault parent dir cannot be chmod'd to 0o700. The ops unit tests wrote vaults straight into /tmp via NamedTempFile; as root (local) the chmod of /tmp succeeds, but CI runs non-root so it fails and trips the new guard. Convert all 23 ops test sites to a private tempdir() (owned, 0o700) with a vault.json inside — the realistic layout — so the directory is restrictable. Review comments addressed: - src/cli/clipboard.rs: copy_blocking status banner moved from stdout to stderr, and only when stderr is interactive, so `dota get NAME --copy` keeps stdout empty for safe piping/redirection. - src/vault/migration.rs: tombstone filenames now reuse the backup's unique component instead of Utc::now(), so scrubbing many backups in the same microsecond cannot collide and leave a backup unsanitized (H3 integrity). - src/cli/commands.rs: read_passphrase docstring corrected — handle_init also uses DOTA_PASSPHRASE as the new vault passphrase in non-interactive mode. - README.md: quickstart default-command line no longer says "Launch TUI"; the shipped interface is the text-mode dota> shell. Verified: cargo fmt --check; clippy --all-targets {--all-features, --no-default-features} -- -D warnings; cargo test (121) and --no-default-features (87); release build; e2e sweep on a non-default --vault subdir (rotate-keys scrubs 2 same-second backups into 2 distinct tombstones, vault 0o600 / dir 0o700, get round-trips). https://claude.ai/code/session_014uS12GBDEicXLF5CZvLsPQ

johnzfitch · 2026-05-30T21:07:06Z

Addressed the CI failure and all four actionable review comments in 00285f8.

CI Test failure (root cause + fix). The M8 hardening makes secure_vault_directory hard-fail when a non-default --vault parent directory cannot be chmod'd to 0o700. The vault::ops unit tests created vaults directly in /tmp via NamedTempFile; that directory can't be restricted by a non-root user, so on CI the new guard tripped (locally it passed only because the dev container runs as root and can chmod /tmp). Fixed by moving all 23 affected test sites to a private tempdir() with a vault.json inside — the realistic layout, and a directory that is actually restrictable.

Review comments:

src/cli/clipboard.rs — copy_blocking's status banner now goes to stderr, and only when stderr is a terminal, so dota get NAME --copy keeps stdout empty for safe piping/redirection.
src/vault/migration.rs — tombstone filenames now reuse the backup's unique component (<stem>.backup.<unique>.<ext> → <stem>.tombstone.<unique>.<ext>) instead of Utc::now(), so scrubbing several backups within the same microsecond can no longer collide and leave one unsanitized. Verified end-to-end: two same-second backups produce two distinct tombstones with zero backups left.
src/cli/commands.rs — read_passphrase docstring corrected to note that handle_init also reads DOTA_PASSPHRASE as the new vault passphrase for non-interactive provisioning.
README.md — quickstart's default-command line no longer says "Launch TUI"; it now describes the shipped text-mode dota> shell.

Local gate on the new commit: cargo fmt --check; clippy --all-targets {--all-features, --no-default-features} -- -D warnings; cargo test (121) and --no-default-features (87); release build — all green.

Generated by Claude Code

johnzfitch · 2026-05-30T21:07:20Z

Addressed the CI failure and all four actionable review comments in 00285f8.

CI Test failure (root cause + fix). The M8 hardening makes secure_vault_directory hard-fail when a non-default --vault parent directory cannot be chmod'd to 0o700. The vault::ops unit tests created vaults directly in /tmp via NamedTempFile; that directory can't be restricted by a non-root user, so on CI the new guard tripped (locally it passed only because the dev container runs as root and can chmod /tmp). Fixed by moving all 23 affected test sites to a private tempdir() with a vault.json inside — the realistic layout, and a directory that is actually restrictable.

Review comments:

src/cli/clipboard.rs — copy_blocking's status banner now goes to stderr, and only when stderr is a terminal, so dota get NAME --copy keeps stdout empty for safe piping/redirection.
src/vault/migration.rs — tombstone filenames now reuse the backup's unique component (<stem>.backup.<unique>.<ext> → <stem>.tombstone.<unique>.<ext>) instead of Utc::now(), so scrubbing several backups within the same microsecond can no longer collide and leave one unsanitized. Verified end-to-end: two same-second backups produce two distinct tombstones with zero backups left.
src/cli/commands.rs — read_passphrase docstring corrected to note that handle_init also reads DOTA_PASSPHRASE as the new vault passphrase for non-interactive provisioning.
README.md — quickstart's default-command line no longer says "Launch TUI"; it now describes the shipped text-mode dota> shell.

Local gate on the new commit: cargo fmt --check; clippy --all-targets {--all-features, --no-default-features} -- -D warnings; cargo test (121) and --no-default-features (87); release build — all green.

Generated by Claude Code

johnzfitch · 2026-05-31T02:49:56Z

@claude fix the issues with merging

johnzfitch · 2026-05-31T03:11:42Z

The merge conflict here isn't fixable in the usual sense: PR #17 already merged the same v1.1.0 audit cleanup into main (73a1f78) while this PR was in review. #18 implements the identical findings (H1/H3/H4 + M/L tier) independently, so it now conflicts on all 18 files, and main already contains the clipboard path, lib.rs, salt-entropy work, and a richer integration-test suite.

Resolving #18 to merge would either no-op (where it overlaps #17) or regress main — in particular it would revert the ml-kem pin from the stable =0.3.2 back to =0.3.0-rc.0 and delete six of #17's test files. Per the maintainer's call, I've salvaged the only net-new item instead: the cargo-deny policy (deny.toml) plus a CI job to enforce it, which main was missing. That's up as #19.

Closing this PR as superseded by #17, with its unique value carried forward in #19.

Generated by Claude Code

johnzfitch · 2026-05-31T03:11:49Z

Correction: the salvage PR is #20, not #19. The cargo-deny policy + CI enforcement carried forward from here lives there.

Generated by Claude Code

Copilot AI review requested due to automatic review settings May 30, 2026 20:57

Copilot started reviewing on behalf of johnzfitch May 30, 2026 20:57 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

johnzfitch mentioned this pull request May 31, 2026

Add cargo-deny policy and CI enforcement (salvaged from #18) #20

Open

johnzfitch closed this May 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SECURITY-AUDIT.md remediation plan (v1.1.0 audit cleanup)#18

Implement SECURITY-AUDIT.md remediation plan (v1.1.0 audit cleanup)#18
johnzfitch wants to merge 3 commits into
mainfrom
claude/plan-implementation-end-to-end-13GyA

johnzfitch commented May 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

johnzfitch commented May 30, 2026

Uh oh!

johnzfitch commented May 30, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

johnzfitch commented May 30, 2026

Summary

High

Medium / Low

Test plan

New integration tests

Findings closed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

johnzfitch commented May 30, 2026

Uh oh!

johnzfitch commented May 30, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

johnzfitch commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants