sneak/secret
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
a09fa89f30
secret/TODO.md
sneak 816f53f819 Replace shell-based keychain implementation with keybase/go-keychain library 
- Replaced exec.Command calls to /usr/bin/security with native keybase/go-keychain API
- Added comprehensive test suite for keychain operations
- Fixed binary data storage in tests using hex encoding
- Updated macse tests to skip with explanation about ADE requirements
- All tests passing with CGO_ENABLED=1
2025-07-21 15:58:41 +02:00

148 lines
8.5 KiB
Markdown
Raw Blame History

# TODO for 1.0 Release
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).
## CRITICAL BLOCKERS FOR 1.0 RELEASE
### 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
### 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
### 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
### 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
### 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
## 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 <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 View Git Blame Copy Permalink