From 76b12704f4eae801da2471f703472fa91df82847 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 6 Jul 2026 20:31:40 +0200 Subject: [PATCH 1/2] Update TODO.md --- TODO.md | 214 ++++++++++++++++++++------------------------------------ 1 file changed, 75 insertions(+), 139 deletions(-) diff --git a/TODO.md b/TODO.md index 4ec4990..151d144 100644 --- a/TODO.md +++ b/TODO.md @@ -1,147 +1,83 @@ -# TODO for 1.0 Release +# Status -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). +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). -## CRITICAL BLOCKERS FOR 1.0 RELEASE +# Next Step -### 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 +Bring the repo into policy compliance in one commit: -### 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 +- 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. -### 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 +# Completed Steps -### 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 +- 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). -### 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 +# Future Steps -## CRITICAL MEMORY SECURITY ISSUES - -### 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 - -### 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 ` - 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). -- 2.49.1 From bcc820a76422ad79813186f64835d91bf13ed093 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 6 Jul 2026 21:06:46 +0200 Subject: [PATCH 2/2] Add standard Workflow section to TODO.md --- TODO.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/TODO.md b/TODO.md index 151d144..0a23b23 100644 --- a/TODO.md +++ b/TODO.md @@ -1,3 +1,13 @@ +# Workflow + +* 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 + # Status pre-1.0. No git tags. TODO.md carries open 1.0 security blockers. Work in -- 2.49.1