TODO (#25)

Reviewed-on: #25

TODO.md

@@ -1,147 +1,93 @@
-# TODO for 1.0 Release
+# Workflow
 
-This document outlines the bugs, issues, and improvements that need to be
-addressed before the 1.0 release of the secret manager. Items are
-prioritized from most critical (top) to least critical (bottom).
+* branch (from `main`)
+* do the work in Next Step
+* move Next Step to the top of Completed Steps
+* move the top item of Future Steps into Next Step
+* commit (`TODO.md` changes in the same commit as the work)
+* merge to `main` if the branch is not protected, otherwise open a PR
+* push
 
-## CRITICAL BLOCKERS FOR 1.0 RELEASE
+# Status
 
-### Command Injection Vulnerabilities
-- [ ] **1. PGP command injection risk**: `internal/secret/pgpunlocker.go:323-327` - GPG key IDs passed directly to exec.Command without proper escaping
-- [ ] **2. Keychain command injection risk**: `internal/secret/keychainunlocker.go:472-476` - data.String() passed to security command without escaping
+pre-1.0. No git tags. TODO.md carries open 1.0 security blockers. Work in
+flight on branch secure-enclave-unlocker (clean tree as of 2026-07-06).
 
-### Memory Security Critical Issues
-- [ ] **3. Plain text passphrase in memory**: `internal/secret/keychainunlocker.go:342,393-396` - KeychainData struct stores AgePrivKeyPassphrase as unprotected string
-- [ ] **4. Sensitive string conversions**: `internal/secret/keychainunlocker.go:356`, `internal/secret/pgpunlocker.go:256`, `internal/secret/version.go:155` - Age identity .String() creates unprotected copies
+# Next Step
 
-### Race Conditions (Data Corruption Risk)
-- [ ] **5. No file locking mechanism**: `internal/vault/secrets.go:142-176` - Multiple concurrent operations can corrupt vault state
-- [ ] **6. Non-atomic file operations**: Various locations - Interrupted writes leave vault inconsistent
+Bring the repo into policy compliance in one commit:
 
-### Input Validation Vulnerabilities
-- [ ] **7. Path traversal risk**: `internal/vault/secrets.go:75-99` - Secret names allow dots which could enable traversal attacks with encoding
-- [ ] **8. Missing size limits**: `internal/vault/secrets.go:102` - No maximum secret size allows DoS via memory exhaustion
+- Add fmt-check and hooks targets to the Makefile (test/lint/fmt/check/
+  docker already exist).
+- Add REPO_POLICIES.md and .editorconfig.
+- Add .gitea/workflows/check.yml running make check.
+- Verify Dockerfile base images are pinned by sha256.
 
-### Timing Attack Vulnerabilities
-- [ ] **9. Non-constant-time passphrase comparison**: `internal/cli/init.go:209-216` - bytes.Equal() vulnerable to timing attacks
-- [ ] **10. Non-constant-time key validation**: `internal/vault/vault.go:95-100` - Public key comparison leaks timing information
+# Completed Steps
 
-## CRITICAL MEMORY SECURITY ISSUES
+- 2026-03-11: Secure Enclave unlocker for hardware-backed secret
+  protection, plus review fixes (stub panics, derivation index, tests,
+  README) on branch secure-enclave-unlocker.
+- 2026-02-28: Repo cleanup, removed stale .cursorrules and coverage.out.
+- Audit fix wave (issues #1, #2, #3, #13, #14): skip unlockers with
+  missing metadata, allow uppercase secret names, fix hardcoded
+  derivation index, validate names in GetSecretVersion against path
+  traversal, return errors instead of panicking, add Warn() on silent
+  anomalies.
+- Memory security hardening: LockedBuffer used through encrypt/decrypt
+  paths (Save/EncryptWithPassphrase/GetValue/gpg helpers), deprecated
+  bare-[]byte APIs removed.
+- Per-secret keypair architecture, vault package refactor, versioning
+  with --version, comprehensive test suite with in-memory filesystem.
+- Debug logging system (slog, GODEBUG flag, TTY-aware output).
+- Renamed SEP unlocker to Keychain, reorganized import commands.
+- 2025-05-28: Initial implementation (vault, age encryption, mnemonic,
+  CLI).
 
-### Functions accepting bare []byte for sensitive data
-- [x] **1. Secret.Save accepts unprotected data**: `internal/secret/secret.go:67` - `Save(value []byte, force bool)` - ✓ REMOVED - deprecated function deleted
-- [x] **2. EncryptWithPassphrase accepts unprotected data**: `internal/secret/crypto.go:73` - `EncryptWithPassphrase(data []byte, passphrase *memguard.LockedBuffer)` - ✓ FIXED - now accepts LockedBuffer for data
-- [x] **3. storeInKeychain accepts unprotected data**: `internal/secret/keychainunlocker.go:469` - `storeInKeychain(itemName string, data []byte)` - ✓ FIXED - now accepts LockedBuffer for data
-- [x] **4. gpgEncryptDefault accepts unprotected data**: `internal/secret/pgpunlocker.go:351` - `gpgEncryptDefault(data []byte, keyID string)` - ✓ FIXED - now accepts LockedBuffer for data
+# Future Steps
 
-### Functions returning unprotected secrets
-- [x] **5. GetValue returns unprotected secret**: `internal/secret/secret.go:93` - `GetValue(unlocker Unlocker) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer internally
-- [x] **6. DecryptWithIdentity returns unprotected data**: `internal/secret/crypto.go:57` - `DecryptWithIdentity(data []byte, identity age.Identity) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
-- [x] **7. DecryptWithPassphrase returns unprotected data**: `internal/secret/crypto.go:94` - `DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuffer) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
-- [x] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
-- [x] **9. getSecretValue returns unprotected data**: `internal/cli/crypto.go:269` - `getSecretValue()` returns bare []byte - ✓ ALREADY FIXED - returns LockedBuffer
-
-### Intermediate string variables for passphrases
-- [x] **10. Passphrase extracted to string**: `internal/secret/crypto.go:79,100` - `passphraseStr := passphrase.String()` - ✓ UNAVOIDABLE - age library requires string parameter
-- [ ] **11. Age secret key in plain string**: `internal/cli/crypto.go:86,91,113` - Age secret key stored in plain string variable before conversion back to secure buffer
-
-### Unprotected buffer.Bytes() usage
-- [ ] **12. GPG encrypt exposes private key**: `internal/secret/pgpunlocker.go:256` - `GPGEncryptFunc(agePrivateKeyBuffer.Bytes(), gpgKeyID)` - private key exposed to external function
-- [ ] **13. Keychain encrypt exposes private key**: `internal/secret/keychainunlocker.go:371` - `EncryptWithPassphrase(agePrivKeyBuffer.Bytes(), passphraseBuffer)` - private key passed as bare bytes
-
-## Code Cleanups
-
-* we shouldn't be passing around a statedir, it should be read from the
-  environment or default.
-
-## HIGH PRIORITY SECURITY ISSUES
-
-- [ ] **4. Application crashes on corrupted metadata**: Code panics instead
-  of returning errors when metadata is corrupt, causing denial of service.
-  Found in pgpunlocker.go:116 and keychainunlocker.go:141.
-
-- [ ] **5. Insufficient input validation**: Secret names allow potentially
-  dangerous patterns including dots that could enable path traversal attacks
-  (vault/secrets.go:70-93).
-
-- [ ] **6. Race conditions in file operations**: Multiple concurrent
-  operations could corrupt the vault state due to lack of file locking
-  mechanisms.
-
-- [ ] **7. Insecure temporary file handling**: Temporary files containing
-  sensitive data may not be properly cleaned up or secured.
-
-## HIGH PRIORITY FUNCTIONALITY ISSUES
-
-- [ ] **8. Inappropriate Cobra usage printing**: Commands currently print
-  usage information for all errors, including internal program failures.
-  Usage should only be printed when the user provides incorrect arguments or
-  invalid commands.
-
-- [ ] **9. Missing current unlock key initialization**: When creating
-  vaults, no default unlock key is selected, which can cause operations to
-  fail.
-
-- [ ] **10. Add confirmation prompts for destructive operations**:
-  Operations like `keys rm` and vault deletion should require confirmation.
-
-- [ ] **11. No secret deletion command**: Missing `secret rm <secret-name>`
-  functionality.
-
-- [ ] **12. Missing vault deletion command**: No way to delete vaults that
-  are no longer needed.
-
-## MEDIUM PRIORITY ISSUES
-
-- [ ] **13. Inconsistent error messages**: Error messages need
-  standardization and should be user-friendly. Many errors currently expose
-  internal implementation details.
-
-- [ ] **14. No graceful handling of corrupted state**: If key files are
-  corrupted or missing, the tool should provide clear error messages and
-  recovery suggestions.
-
-- [ ] **15. No validation of GPG key existence**: Should verify the
-  specified GPG key exists before creating PGP unlock keys.
-
-- [ ] **16. Better separation of concerns**: Some functions in CLI do too
-  much and should be split.
-
-- [ ] **17. Environment variable security**: Sensitive data read from
-  environment variables (SB_UNLOCK_PASSPHRASE, SB_SECRET_MNEMONIC) without
-  proper clearing. Document security implications.
-
-- [ ] **18. No secure memory allocation**: No use of mlock/munlock to
-  prevent sensitive data from being swapped to disk.
-
-## LOWER PRIORITY ENHANCEMENTS
-
-- [ ] **19. Add `--help` examples**: Command help should include practical examples for each operation.
-
-- [ ] **20. Add shell completion**: Bash/Zsh completion for commands and secret names.
-
-- [ ] **21. Colored output**: Use colors to improve readability of lists and error messages.
-
-- [ ] **22. Add `--quiet` flag**: Option to suppress non-essential output.
-
-- [ ] **23. Smart secret name suggestions**: When a secret name is not found, suggest similar names.
-
-- [ ] **24. Audit logging**: Log all secret access and modifications for security auditing.
-
-- [ ] **25. Integration tests for hardware features**: Automated testing of Keychain and GPG functionality.
-
-- [ ] **26. Consistent naming conventions**: Some variables and functions use inconsistent naming patterns.
-
-- [ ] **27. Export/import functionality**: Add ability to export/import entire vaults, not just individual secrets.
-
-- [ ] **28. Batch operations**: Add commands to process multiple secrets at once.
-
-- [ ] **29. Search functionality**: Add ability to search secret names and potentially contents.
-
-- [ ] **30. Secret metadata**: Add support for descriptions, tags, or other metadata with secrets.
-
-## COMPLETED ITEMS ✓
-
-- [x] **Missing secret history/versioning**: ✓ Implemented - versioning system exists with --version flag support
-- [x] **XDG compliance on Linux**: ✓ Implemented - uses os.UserConfigDir() which respects XDG_CONFIG_HOME
-- [x] **Consistent interface implementation**: ✓ Implemented - Unlocker interface is well-defined and consistently implemented
+- Compliance (after Next Step lands): keep main green under the new
+  .gitea workflow; run make check before every merge.
+- Merge secure-enclave-unlocker to main once review is done.
+- 1.0 critical security blockers (from repo TODO.md):
+    - Command injection: GPG key IDs passed unescaped to exec.Command
+      (pgpunlocker.go:323-327); data.String() passed unescaped to the
+      security command (keychainunlocker.go:472-476).
+    - Memory security: KeychainData stores AgePrivKeyPassphrase as a
+      plain string (keychainunlocker.go:342,393-396); age identity
+      .String() creates unprotected copies (keychainunlocker.go:356,
+      pgpunlocker.go:256, version.go:155); age secret key held in a
+      plain string in cli/crypto.go:86,91,113; private keys exposed via
+      buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
+    - Race conditions: no file locking in vault/secrets.go:142-176;
+      non-atomic writes can leave the vault inconsistent.
+    - Input validation: dots in secret names risk path traversal
+      (vault/secrets.go:75-99); no maximum secret size (DoS).
+    - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
+      209-216); non-constant-time public key compare (vault.go:95-100).
+- High priority:
+    - Return errors instead of panicking on corrupted metadata
+      (pgpunlocker.go:116, keychainunlocker.go:141).
+    - Secure temporary file handling and cleanup.
+    - Print cobra usage only for argument errors, not internal
+      failures.
+    - Initialize a default unlock key at vault creation.
+    - Confirmation prompts for destructive operations (keys rm, vault
+      deletion).
+    - Add secret rm and vault deletion commands.
+- Medium priority:
+    - Standardize error messages; stop leaking internals.
+    - Graceful handling of corrupted or missing key files with recovery
+      suggestions.
+    - Validate GPG key existence before creating PGP unlock keys.
+    - Split oversized CLI functions.
+    - Document env var security (SB_UNLOCK_PASSPHRASE,
+      SB_SECRET_MNEMONIC); clear after use.
+    - mlock/munlock for sensitive allocations.
+- Cleanups: read statedir from environment or default instead of
+  passing it around.
+- Enhancements: help examples, shell completion, colored output,
+  --quiet flag, name suggestions on miss, audit logging, hardware
+  integration tests (Keychain, GPG), naming consistency, vault
+  export/import, batch operations, search, secret metadata
+  (descriptions, tags).