Update TODO.md
This commit is contained in:
214
TODO.md
214
TODO.md
@@ -1,147 +1,83 @@
|
|||||||
# TODO for 1.0 Release
|
# Status
|
||||||
|
|
||||||
This document outlines the bugs, issues, and improvements that need to be
|
pre-1.0. No git tags. TODO.md carries open 1.0 security blockers. Work in
|
||||||
addressed before the 1.0 release of the secret manager. Items are
|
flight on branch secure-enclave-unlocker (clean tree as of 2026-07-06).
|
||||||
prioritized from most critical (top) to least critical (bottom).
|
|
||||||
|
|
||||||
## CRITICAL BLOCKERS FOR 1.0 RELEASE
|
# Next Step
|
||||||
|
|
||||||
### Command Injection Vulnerabilities
|
Bring the repo into policy compliance in one commit:
|
||||||
- [ ] **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
|
|
||||||
|
|
||||||
### Memory Security Critical Issues
|
- Add fmt-check and hooks targets to the Makefile (test/lint/fmt/check/
|
||||||
- [ ] **3. Plain text passphrase in memory**: `internal/secret/keychainunlocker.go:342,393-396` - KeychainData struct stores AgePrivKeyPassphrase as unprotected string
|
docker already exist).
|
||||||
- [ ] **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 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)
|
# Completed Steps
|
||||||
- [ ] **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
|
|
||||||
|
|
||||||
### Input Validation Vulnerabilities
|
- 2026-03-11: Secure Enclave unlocker for hardware-backed secret
|
||||||
- [ ] **7. Path traversal risk**: `internal/vault/secrets.go:75-99` - Secret names allow dots which could enable traversal attacks with encoding
|
protection, plus review fixes (stub panics, derivation index, tests,
|
||||||
- [ ] **8. Missing size limits**: `internal/vault/secrets.go:102` - No maximum secret size allows DoS via memory exhaustion
|
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
|
# Future Steps
|
||||||
- [ ] **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
|
|
||||||
|
|
||||||
## CRITICAL MEMORY SECURITY ISSUES
|
- Compliance (after Next Step lands): keep main green under the new
|
||||||
|
.gitea workflow; run make check before every merge.
|
||||||
### Functions accepting bare []byte for sensitive data
|
- Merge secure-enclave-unlocker to main once review is done.
|
||||||
- [x] **1. Secret.Save accepts unprotected data**: `internal/secret/secret.go:67` - `Save(value []byte, force bool)` - ✓ REMOVED - deprecated function deleted
|
- 1.0 critical security blockers (from repo TODO.md):
|
||||||
- [x] **2. EncryptWithPassphrase accepts unprotected data**: `internal/secret/crypto.go:73` - `EncryptWithPassphrase(data []byte, passphrase *memguard.LockedBuffer)` - ✓ FIXED - now accepts LockedBuffer for data
|
- Command injection: GPG key IDs passed unescaped to exec.Command
|
||||||
- [x] **3. storeInKeychain accepts unprotected data**: `internal/secret/keychainunlocker.go:469` - `storeInKeychain(itemName string, data []byte)` - ✓ FIXED - now accepts LockedBuffer for data
|
(pgpunlocker.go:323-327); data.String() passed unescaped to the
|
||||||
- [x] **4. gpgEncryptDefault accepts unprotected data**: `internal/secret/pgpunlocker.go:351` - `gpgEncryptDefault(data []byte, keyID string)` - ✓ FIXED - now accepts LockedBuffer for data
|
security command (keychainunlocker.go:472-476).
|
||||||
|
- Memory security: KeychainData stores AgePrivKeyPassphrase as a
|
||||||
### Functions returning unprotected secrets
|
plain string (keychainunlocker.go:342,393-396); age identity
|
||||||
- [x] **5. GetValue returns unprotected secret**: `internal/secret/secret.go:93` - `GetValue(unlocker Unlocker) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer internally
|
.String() creates unprotected copies (keychainunlocker.go:356,
|
||||||
- [x] **6. DecryptWithIdentity returns unprotected data**: `internal/secret/crypto.go:57` - `DecryptWithIdentity(data []byte, identity age.Identity) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
pgpunlocker.go:256, version.go:155); age secret key held in a
|
||||||
- [x] **7. DecryptWithPassphrase returns unprotected data**: `internal/secret/crypto.go:94` - `DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuffer) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
plain string in cli/crypto.go:86,91,113; private keys exposed via
|
||||||
- [x] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
|
||||||
- [x] **9. getSecretValue returns unprotected data**: `internal/cli/crypto.go:269` - `getSecretValue()` returns bare []byte - ✓ ALREADY FIXED - returns LockedBuffer
|
- Race conditions: no file locking in vault/secrets.go:142-176;
|
||||||
|
non-atomic writes can leave the vault inconsistent.
|
||||||
### Intermediate string variables for passphrases
|
- Input validation: dots in secret names risk path traversal
|
||||||
- [x] **10. Passphrase extracted to string**: `internal/secret/crypto.go:79,100` - `passphraseStr := passphrase.String()` - ✓ UNAVOIDABLE - age library requires string parameter
|
(vault/secrets.go:75-99); no maximum secret size (DoS).
|
||||||
- [ ] **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
|
- Timing attacks: bytes.Equal passphrase compare (cli/init.go:
|
||||||
|
209-216); non-constant-time public key compare (vault.go:95-100).
|
||||||
### Unprotected buffer.Bytes() usage
|
- High priority:
|
||||||
- [ ] **12. GPG encrypt exposes private key**: `internal/secret/pgpunlocker.go:256` - `GPGEncryptFunc(agePrivateKeyBuffer.Bytes(), gpgKeyID)` - private key exposed to external function
|
- Return errors instead of panicking on corrupted metadata
|
||||||
- [ ] **13. Keychain encrypt exposes private key**: `internal/secret/keychainunlocker.go:371` - `EncryptWithPassphrase(agePrivKeyBuffer.Bytes(), passphraseBuffer)` - private key passed as bare bytes
|
(pgpunlocker.go:116, keychainunlocker.go:141).
|
||||||
|
- Secure temporary file handling and cleanup.
|
||||||
## Code Cleanups
|
- Print cobra usage only for argument errors, not internal
|
||||||
|
failures.
|
||||||
* we shouldn't be passing around a statedir, it should be read from the
|
- Initialize a default unlock key at vault creation.
|
||||||
environment or default.
|
- Confirmation prompts for destructive operations (keys rm, vault
|
||||||
|
deletion).
|
||||||
## HIGH PRIORITY SECURITY ISSUES
|
- Add secret rm and vault deletion commands.
|
||||||
|
- Medium priority:
|
||||||
- [ ] **4. Application crashes on corrupted metadata**: Code panics instead
|
- Standardize error messages; stop leaking internals.
|
||||||
of returning errors when metadata is corrupt, causing denial of service.
|
- Graceful handling of corrupted or missing key files with recovery
|
||||||
Found in pgpunlocker.go:116 and keychainunlocker.go:141.
|
suggestions.
|
||||||
|
- Validate GPG key existence before creating PGP unlock keys.
|
||||||
- [ ] **5. Insufficient input validation**: Secret names allow potentially
|
- Split oversized CLI functions.
|
||||||
dangerous patterns including dots that could enable path traversal attacks
|
- Document env var security (SB_UNLOCK_PASSPHRASE,
|
||||||
(vault/secrets.go:70-93).
|
SB_SECRET_MNEMONIC); clear after use.
|
||||||
|
- mlock/munlock for sensitive allocations.
|
||||||
- [ ] **6. Race conditions in file operations**: Multiple concurrent
|
- Cleanups: read statedir from environment or default instead of
|
||||||
operations could corrupt the vault state due to lack of file locking
|
passing it around.
|
||||||
mechanisms.
|
- Enhancements: help examples, shell completion, colored output,
|
||||||
|
--quiet flag, name suggestions on miss, audit logging, hardware
|
||||||
- [ ] **7. Insecure temporary file handling**: Temporary files containing
|
integration tests (Keychain, GPG), naming consistency, vault
|
||||||
sensitive data may not be properly cleaned up or secured.
|
export/import, batch operations, search, secret metadata
|
||||||
|
(descriptions, tags).
|
||||||
## 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
|
|
||||||
|
|||||||
Reference in New Issue
Block a user