sneak
63cc06b93c
- Changed DecryptWithIdentity to return *memguard.LockedBuffer instead of []byte - Updated all callers throughout the codebase to handle LockedBuffer - This ensures decrypted data is protected in memory immediately after decryption - Fixed all usages in vault, secret, version, and unlocker implementations - Removed duplicate buffer creation and unnecessary memory clearing
126 lines
6.9 KiB
Markdown
126 lines
6.9 KiB
Markdown
# 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 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
|
|
- [ ] **7. DecryptWithPassphrase returns unprotected data**: `internal/secret/crypto.go:94` - `DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuffer) ([]byte, error)` - returns decrypted data unprotected
|
|
- [ ] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - returns decrypted data unprotected
|
|
- [ ] **9. getSecretValue returns unprotected data**: `internal/cli/crypto.go:269` - `getSecretValue()` returns bare []byte
|
|
|
|
### Intermediate string variables for passphrases
|
|
- [ ] **10. Passphrase extracted to string**: `internal/secret/crypto.go:79,100` - `passphraseStr := passphrase.String()` - passphrase extracted to plain string before use with age.NewScryptRecipient
|
|
- [ ] **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
|