Added AWS Bedrock Configuration for models Handling.#434
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces AWS Bedrock as a new backend option for PentestGPT, adding the BedrockBackend class, updating configuration schemas, and extending the setup scripts to support AWS credentials. Review feedback identifies a bug where session resumption is incorrectly marked as supported despite being unimplemented, notes several unused json imports, and suggests refactoring credential validation to allow for standard AWS authentication methods like IAM roles.
| def supports_resume(self) -> bool: | ||
| """Bedrock supports session resume via conversation history.""" | ||
| return True | ||
|
|
||
| async def resume(self, session_id: str) -> bool: | ||
| """Resume a previous session (loads conversation history).""" | ||
| # TODO: Implement session persistence for Bedrock | ||
| # For now, just reconnect | ||
| self._session_id = session_id | ||
| await self.connect() | ||
| return True |
There was a problem hiding this comment.
The BedrockBackend claims to support session resumption by having supports_resume return True, but the resume method is an incomplete stub. It doesn't load the conversation history, causing any resumed session to start from scratch. This is a significant bug that can lead to loss of context in a resumed session.
The TODO comment indicates this is a known issue. Until session persistence is fully implemented for Bedrock, this functionality should be disabled to prevent incorrect behavior.
| def supports_resume(self) -> bool: | |
| """Bedrock supports session resume via conversation history.""" | |
| return True | |
| async def resume(self, session_id: str) -> bool: | |
| """Resume a previous session (loads conversation history).""" | |
| # TODO: Implement session persistence for Bedrock | |
| # For now, just reconnect | |
| self._session_id = session_id | |
| await self.connect() | |
| return True | |
| @property | |
| def supports_resume(self) -> bool: | |
| """Bedrock does not yet support session resume.""" | |
| return False | |
| async def resume(self, session_id: str) -> bool: | |
| """Resume a previous session (loads conversation history).""" | |
| # TODO: Implement session persistence for Bedrock | |
| raise NotImplementedError("Session resume for Bedrock is not yet implemented.") |
| async def connect(self) -> None: | ||
| """Connect to AWS Bedrock.""" | ||
| import boto3 | ||
| import json |
There was a problem hiding this comment.
The json module is imported but not used within the connect method. It should be removed to keep the code clean.
|
|
||
| async def receive_messages(self) -> AsyncIterator[AgentMessage]: | ||
| """Get messages from Bedrock via Converse API.""" | ||
| import json |
There was a problem hiding this comment.
The json module is imported but not used within the receive_messages method. It should be removed to keep the code clean.
| if not config.aws_access_key_id or not config.aws_secret_access_key: | ||
| raise ValueError( | ||
| "AWS credentials not configured. " | ||
| "Please set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY" | ||
| ) |
There was a problem hiding this comment.
This check forces users to provide AWS credentials explicitly via AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY. This prevents leveraging other standard boto3 authentication mechanisms like IAM roles (e.g., on EC2) or shared credential files (~/.aws/credentials), which can be more secure and convenient.
To improve flexibility, I recommend removing this explicit check and allowing boto3 to discover credentials automatically. This would involve:
- Removing this
ifblock. - Updating
BedrockBackend.__init__to acceptaccess_keyandsecret_keyas optional (str | None). - Passing these optional keys to
boto3.client(), which correctly handlesNonevalues by falling back to its default credential provider chain.
2 participants
Author : @imHax0x90