Add Bitwarden provider with comprehensive security enhancements #24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements unified provider for both Bitwarden Password Manager and Secrets Manager: - **Bitwarden Provider**: Full implementation with SecretString support, vault-wide access to all item types (Login, Card, Identity, SSH Key, Secure Note), smart field extraction, URI-based configuration (bitwarden:// and bws://) - **Integration Tests**: Comprehensive test suite covering all item types, error handling, authentication states, and real CLI integration - **Documentation**: Complete provider docs following OnePassword structure with usage examples, configuration options, and troubleshooting guide - **Project Plan**: Detailed implementation documentation with testing results - **Dependencies**: Added base64 crate for CLI item creation, secrecy integration Supports both interactive development (bw login/unlock) and CI/CD automation (BWS_ACCESS_TOKEN). Provides vault-wide secret access without folder restrictions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add CLI command timeouts (30s default) to prevent hangs - Implement comprehensive error message sanitization - Fix project-wide security vulnerability (no existing sanitization) - Add timeout configuration via env vars and URL params - Comprehensive test coverage for new functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add helper functions to centralize conversion logic - Replace ~50+ repetitive patterns with clean AsRef<str> API - Eliminate unnecessary string cloning and intermediate conversions - Improve code readability and maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Performance data shows CLI/network latency dominates (99.9% of execution time). JSON processing is only 133μs vs 2-4 seconds for CLI operations. Deprioritized caching as it would provide minimal benefit for current usage patterns. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Refactored 120-line method into focused single-responsibility methods: - redact_secret_patterns() - handles JSON/key-value pattern redaction - redact_bearer_tokens() - handles Bearer token sanitization - redact_base64_tokens() - handles base64-like string redaction - redact_file_paths() - handles file path sanitization - truncate_long_message() - handles message length limits Improves code readability and testability while maintaining identical functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add .pre-commit-config.yaml with rustfmt hooks to match devenv CI - Update .gitignore to allow pre-commit config while ignoring hooks - Ensures local formatting matches devenv-based CI exactly - Resolves formatting inconsistencies between local and CI environments Code formatting fixes to match CI standards: - Fix missing newlines at end of files in test modules - Apply consistent rustfmt formatting to test files - Clean up whitespace and indentation inconsistencies 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Implement test_concurrent_access_to_mock_provider with 10 threads × 50 operations - Add test_concurrent_read_heavy_workload achieving ~482k reads/sec - Create test_mixed_concurrent_workload with reader/writer scenarios - Add test_provider_thread_safety verifying 8-thread safety - Implement test_performance_baseline_measurements showing 1.9M ops/sec throughput - All tests demonstrate thread safety and high-performance concurrent access Fix unused code warnings: - Add #[allow(dead_code)] to unused structs and methods in bitwarden provider - Mark unused parameters with underscore prefix to satisfy clippy 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add SECRETSPEC_TEST_PROVIDERS environment variable checks to integration tests - Follow existing pattern used by other provider integration tests - Tests now skip gracefully in CI when bw/bws CLI tools are not available - Tests still run when SECRETSPEC_TEST_PROVIDERS includes bitwarden - Ensures CI passes while maintaining comprehensive local testing capability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…en provider ## High Priority Fixes ### Resource leak fix in timeout implementation - Implement proper thread and process cleanup using Arc<Mutex<Option<Child>>> pattern - Add process handle management for cross-platform timeout handling - Ensure thread cleanup on successful completion and timeout scenarios - Add best-effort process killing and zombie cleanup on timeout ### Remove debug output from production code - Remove 3 unconditional debug statements from get() method - Preserve PERF logging functionality for performance monitoring - Clean up development debugging artifacts ## Medium Priority Enhancements ### Comprehensive unit test coverage - Add 7 new unit tests for individual BitwardenProvider sanitization methods - Make sanitization methods pub(crate) for testability - Test secret pattern redaction, bearer tokens, base64/JWT detection, file paths, truncation - Add helper function tests for to_secret_string and option_to_secret_string - Add comprehensive error sanitization integration test ### Enhanced sanitization for edge cases - **JWT Detection**: Improve base64 token detection to handle JWT format (base64.base64.base64) - **Windows File Paths**: Add support for C:\path and \\server\share path formats - **Extended Field Names**: Add 10+ new secret field patterns (authorization, jwt, client_secret, etc.) - **Improved Token Parsing**: Fix space handling in tokens like "Bearer token123" - **Error Message Patterns**: Add support for "token: value" format in error messages ### Comprehensive error path sanitization - Apply sanitization to all error paths including UTF-8 parsing, process execution, I/O errors - Ensure all e.to_string() calls go through sanitization pipeline - Fix sanitization order to process file paths before secret patterns (prevents false positives) - Add 16 new error message patterns for comprehensive coverage ## Technical Details - **Thread Safety**: Use Arc<Mutex<>> for shared process handles across threads - **Cross-Platform**: Enhanced timeout works on Unix and Windows systems - **Performance**: Maintain PERF logging while removing debug output - **Security**: All error messages now sanitized before user exposure - **Test Coverage**: 18 passing tests including new comprehensive sanitization test ## Test Results All 18 Bitwarden provider tests pass, demonstrating enhanced functionality works correctly without breaking existing behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Member
|
I'm not comfortable maintaining 2500 lines of Rust just for one provider, can we use the SDK and use async interface within the trait? Also, let's remove performance testing, I'm not worried about that at all. I've taken a similar approach in #23, and in the future we can change the trait to be async. |
Author
|
Thanks for the comments, Domen. Happy to do the rework to use the SDK, now that I know async is allowed. As for size, though, I'm not sure it will be that much smaller. We'll see. There are other things I can do to make the provider a bit smaller, though:
As for the perf stuff, I'll remove it. It was added because the provider seemed pretty slow, with 1-2 seconds per secret fetched. It turned out, though, that it was just Bitwarden's servers being slow. |
Author
|
Hey, just wanted to let you know I haven't forgotten about this, I've just been busy with my other project. |
|
Hey @ashebanow, are you still planning on working on your implementation in the near future? Would be great to have Bitwarden support! |
Author
|
Honestly not sure. Domen wasn't enthusiastic about the implementation route I chose and wanted essentially a rewrite. What's there was functionally complete and well tested, but I haven't gotten around to a second try yet. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new Bitwarden provider supporting both Bitwarden Password Manager and Bitwarden Secrets Manager.
Closes #15
Features
bitwarden://URIsbws://URIsSecurity Enhancements
Implementation Details
All tests pass and the implementation follows the existing provider patterns and architecture.