Add credential manager for external secret providers #63
Conversation
Implement unified credential management system that supports retrieving secrets from Bitwarden, HashiCorp Vault, and AWS Secrets Manager. Key features: - Factory pattern - Command-line interface - Python API - Full test coverage The system allows secure credential management without hardcoding sensitive information in configuration files. Signed-off-by: [Your Name] <[your-email]> This is a combination of 2 commits. Added credential_manager tool and tests. Updated README.md to include credential_manager instructions,. Signed-off-by: Alberto Ferrer Sánchez <[email protected]>
| Exception: If couldn't inizialize the client | ||
| """ | ||
| try: | ||
| _logger.info("Creating client and logging in.") |
There was a problem hiding this comment.
I would reduce the amount of logging.info from the library, so that when it’s used externally, it only prints a few messages. For example, when it tries to access the credentials and whether that attempt was successful or not could be info. The other logs should be debug.
|
|
||
| _logger.info("Checking Bitwarden login status") | ||
| status_result = subprocess.run( | ||
| ["/snap/bin/bw", "status"], capture_output=True, text=True, check=False |
There was a problem hiding this comment.
/snap/bin/bw is very specific about the installation method of bw (in this case using snap). But bw might also be installed via other methods (e.g. apt, brew, manual download), in which case it could just be on the PATH as bw.
| except FileNotFoundError: | ||
| _logger.error("File not found") | ||
|
|
||
| def _login(self, bw_email: str, bw_password: str) -> str: |
There was a problem hiding this comment.
The function is doing too much in one place: checking the session, logging in, unlocking, and syncing with a lot of nested ifs that make it hard to follow. Consider breaking it into smaller methods. Something like:
def _login():
if self.session_key and self.validate_session()
if self._should_sync():
self._sync_vault()
return self.session_key
status = self._get_status()
if status.returncode != 0:
raise Exception("No valid Bitwarden status found")
if status.email != bw_email:
return self._perform_login()
if status == "unlocked":
self.session_key = status.session_key
elif status == "locked":
return self._unlock_vault(bw_password)
if not self.session_key:
raise Exception("Couldn't obtain session key during login")
if self._should_sync():
self._sync_vault()
return self.session_key
This would flatten the nested conditionals and make it easier to follow.
| if self.client.sys.is_initialized(): | ||
| _logger.info("Client is initialized") | ||
|
|
||
| if self.client.is_authenticated(): | ||
| _logger.info("Client is authenticated") |
There was a problem hiding this comment.
You’re logging is_initialized and is_authenticated, but you might consider raising an error if authentication fails. Otherwise, later calls to get_secret might fail unexpectedly.
| logging.getLogger("urllib3").setLevel(logging.WARNING) | ||
| logging.getLogger("botocore").setLevel(logging.WARNING) | ||
| logging.basicConfig( | ||
| level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s" | ||
| ) |
There was a problem hiding this comment.
Usually, library modules should not call basicConfig or define the level of loggers, that’s the application’s responsibility.
In the main method, you can configure the logging for the application, like we have in Perceval
This also happens in the other modules
| # Handle specific authentication errors | ||
| if "invalid_grant" in error_msg.lower(): | ||
| _logger.error("Invalid credentials provided for Bitwarden") | ||
| return "" |
There was a problem hiding this comment.
I would avoid returning an empty string. Instead, raise an exception or return None in case of an error, since an empty string could be mistaken for an empty password and could fail in later usages of the module.
| * hvac >= 2.3.0 | ||
| * boto3 >= 1.35.63 |
There was a problem hiding this comment.
Update the pyproject file using Poetry to include these requirements.
|
I'm on it, will update when I fix those. |
Implement unified credential management system that supports retrieving secrets from Bitwarden, HashiCorp Vault, and AWS Secrets Manager.
Key features:
The system allows secure credential management without hardcoding sensitive information in configuration files.
Updated README.md to include instructions.