Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(aws): Add a test_connection method #4563

Merged
merged 18 commits into from
Aug 9, 2024

Conversation

jfagoagas
Copy link
Member

@jfagoagas jfagoagas commented Jul 29, 2024

Context

We need to be able to test the connection to each provider using one of the available authentication methods, starting from the environment variables.

Description

  • Add a test_connection method for the AWS provider.
  • Create a test_connection abstract method in the Provider class but not enforced for now not to break anything.

Notes for reviewers

  • Review the removal of sys.exit() in favor of catching the exceptions during the __init__.
  • You can test it with the following:
from prowler.providers.aws.aws_provider import AwsProvider


# - Using environment variables
t = AwsProvider.test_connection()
print(t)

# - Using profile
t = AwsProvider.test_connection(profile="dev")
print(t)


# - With MFA
t = AwsProvider.test_connection(profile="dev", mfa_enabled=True)
print(t)


# - With Role without external ID
t = AwsProvider.test_connection(
    role_arn="arn:aws:iam::111122223333:role/ProwlerRole"
)
print(t)

# - With Role with external ID from ENV
t = AwsProvider.test_connection(
    role_arn="arn:aws:iam::111122223333:role/ProwlerRole",
    external_id="8eabb29e-e591-4828-919f-a9b2839a137d",
)
print(t)


t = AwsProvider.test_connection(
    role_arn="arn:aws:iam::111122223333:role/ProwlerRole",
    external_id="8eabb29e-e591-4828-919f-a9b2839a137d",
    session_duration=900,
    profile="test",
)
print(t)

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jfagoagas jfagoagas requested review from a team as code owners July 29, 2024 13:15
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label Jul 29, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (7e630eb) to head (3fcddaf).
Report is 590 commits behind head on master.

Files with missing lines Patch % Lines
prowler/providers/aws/aws_provider.py 86.07% 11 Missing ⚠️
prowler/providers/common/provider.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4563       /-   ##
==========================================
  Coverage   88.89%   89.11%    0.21%     
==========================================
  Files         907      913        6     
  Lines       27612    27866      254     
==========================================
  Hits        24547    24834      287     
  Misses       3065     3032      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jfagoagas jfagoagas added the no-merge Please, DO NOT MERGE this PR. label Aug 1, 2024
@jfagoagas jfagoagas marked this pull request as draft August 1, 2024 14:12
@jfagoagas jfagoagas removed the no-merge Please, DO NOT MERGE this PR. label Aug 1, 2024
@MrCloudSec MrCloudSec marked this pull request as ready for review August 2, 2024 14:46
@jfagoagas jfagoagas marked this pull request as draft August 5, 2024 09:19
@jfagoagas jfagoagas marked this pull request as ready for review August 5, 2024 11:33
@jfagoagas jfagoagas requested a review from a team August 6, 2024 12:41
Copy link
Member

@vicferpoy vicferpoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I left some comments and I also wondered how is the API going to use this?

As far as I know, we only store the provider_id (relevant to what I see in the test_connection expected arguments). Would that be the external_id field? Would that be enough to test the connection?

prowler/providers/aws/aws_provider.py Outdated Show resolved Hide resolved
prowler/providers/aws/aws_provider.py Outdated Show resolved Hide resolved
@jfagoagas
Copy link
Member Author

This looks good! I left some comments and I also wondered how is the API going to use this?

As far as I know, we only store the provider_id (relevant to what I see in the test_connection expected arguments).

It mostly depends on the AWS Credentials method used, you can see more about it here https://boto3.amazonaws.com/v1/documentation/api/latest/guide/credentials.html. Currently Prowler supports out of the box methods 3, 4, 5 and 6. For example using the environment variables or the profile/config will make the SDK to automatically look for them and then try to authenticate with AWS.

Would that be the external_id field? Would that be enough to test the connection?

The external_id will be used while assuming AWS IAM roles, you can get more information about it in the following links:

vicferpoy
vicferpoy previously approved these changes Aug 8, 2024
Copy link
Member

@vicferpoy vicferpoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the explanation too!

@jfagoagas
Copy link
Member Author

jfagoagas commented Aug 8, 2024

We are still discussing about the format of the test_connection, probably this is going to be the chosen one:

@staticmethod
    def test_connection(
        session: Session = None,
        profile: str = None,
        aws_region: str = AWS_STS_GLOBAL_ENDPOINT_REGION,
        role_arn: str = None,
        role_session_name: str = ROLE_SESSION_NAME,
        session_duration: int = 3600,
        external_id: str = None,
        mfa_enabled: bool = False,
        raise_on_exception: bool = True,
    ) -> TestConnection:
        """
        Validates AWS credentials using the provided session and AWS region.

        If no session is provided, the method will create a new session using the Boto3 default session.

        Args:
            session (Session): The AWS session object.
            profile (str): The AWS profile to use for the session.
            aws_region (str): The AWS region to validate the credentials in.
            role_arn (str): The ARN of the IAM role to assume.
            role_session_name (str): The name of the role session.
            session_duration (int): The duration of the assumed role session in seconds.
            external_id (str): The external ID to use when assuming the role.
            mfa_enabled (bool): Whether MFA (Multi-Factor Authentication) is enabled.
            raise_on_exception (bool): Whether to raise an exception if an error occurs.

        Returns:

            TestConnection: A named tuple containing the result of the validation.
                - connected (bool): Indicates whether the validation was successful.
                - result (AWSCallerIdentity): An object representing the caller's identity if the validation was successful.
                - error (Exception): An exception object if an error occurs during the validation.

        Raises:
            Exception: If an error occurs during the validation process.

        Examples:
            >>> AwsProvider.test_connection(
                role_arn="arn:aws:iam::111122223333:role/ProwlerRole",
                external_id="67f7a641-ecb0-4f6d-921d-3587febd379c"
            )
            AWSCallerIdentity(user_id='AROAAAAAAAAAAAAAAAAAA:ProwlerAssessmentSession', account='111122223333', arn=ARN(arn='arn:aws:sts::111122223333:assumed-role/ProwlerRole/ProwlerAssessmentSession', partition='aws', service='sts', region=None, account_id='111122223333', resource='ProwlerRole/ProwlerAssessmentSession', resource_type='assumed-role'), region='us-east-1')

            >>> AwsProvider.test_connection(profile="test")
            AWSCallerIdentity(user_id='AROAAAAAAAAAAAAAAAAAA:test-user', account='111122223333', arn=ARN(arn='arn:aws:sts::111122223333:user/test-user', partition='aws', service='sts', region=None, account_id='111122223333', resource='test-user', resource_type='user'), region='us-east-1')
        """
        try:
            # Create the default session if no session is given
            session = (
                session if session else AwsProvider.setup_session(mfa_enabled, profile)
            )

            # Test Connection using the IAM Role
            if role_arn:
                session_duration = validate_session_duration(session_duration)
                role_session_name = validate_role_session_name(role_session_name)
                role_arn = parse_iam_credentials_arn(role_arn)
                assumed_role_information = AWSAssumeRoleInfo(
                    role_arn=role_arn,
                    session_duration=session_duration,
                    external_id=external_id,
                    mfa_enabled=mfa_enabled,
                    role_session_name=role_session_name,
                )
                assumed_role_credentials = AwsProvider.assume_role(
                    session,
                    assumed_role_information,
                )
                session = Session(
                    aws_access_key_id=assumed_role_credentials.aws_access_key_id,
                    aws_secret_access_key=assumed_role_credentials.aws_secret_access_key,
                    aws_session_token=assumed_role_credentials.aws_session_token,
                    region_name=aws_region,
                    profile_name=profile,
                )

            sts_client = AwsProvider.create_sts_session(session, aws_region)
            caller_identity = sts_client.get_caller_identity()
            # Include the region where the caller_identity has validated the credentials
            return TestConnection(
                connected=True,
                result=AWSCallerIdentity(
                    user_id=caller_identity.get("UserId"),
                    account=caller_identity.get("Account"),
                    arn=ARN(caller_identity.get("Arn")),
                    region=aws_region,
                ),
            )

        except (ClientError, ProfileNotFound) as credentials_error:
            logger.error(
                f"{credentials_error.__class__.__name__}[{credentials_error.__traceback__.tb_lineno}]: {credentials_error}"
            )
            if raise_on_exception:
                raise credentials_error
            else:
                return TestConnection(error=credentials_error)

        except ArgumentTypeError as validation_error:
            logger.error(
                f"{validation_error.__class__.__name__}[{validation_error.__traceback__.tb_lineno}]: {validation_error}"
            )
            if raise_on_exception:
                raise validation_error
            else:
                return TestConnection(error=validation_error)

        except Exception as error:
            logger.critical(
                f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
            )
            raise error

and the TestConnection object:

@dataclass
class TestConnection:
    _connected: bool = False
    _error: Exception = None
    _result: Any = None

    @property
    def connected(self) -> bool:
        return self._connected

    @property
    def error(self) -> Exception:
        return self._error

    @property
    def result(self) -> Any:
        return self._result

This PR needs to be updated accordingly.

Also the init should call it with:

caller_identity = self.test_connection(
            session=self.session.current_session,
            aws_region=sts_region,
            raise_exception=True,
        )

@jfagoagas jfagoagas added the no-merge Please, DO NOT MERGE this PR. label Aug 8, 2024
@jfagoagas
Copy link
Member Author

Do not merge until the above change is addressed.

@pedrooot pedrooot requested a review from vicferpoy August 8, 2024 12:08
Copy link
Member

@vicferpoy vicferpoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation looks better, great job. I left some small requested changes.

prowler/providers/common/test_connection_dataclass.py Outdated Show resolved Hide resolved
prowler/providers/common/test_connection_dataclass.py Outdated Show resolved Hide resolved
prowler/providers/aws/aws_provider.py Outdated Show resolved Hide resolved
prowler/providers/aws/aws_provider.py Outdated Show resolved Hide resolved
@jfagoagas jfagoagas removed the no-merge Please, DO NOT MERGE this PR. label Aug 9, 2024
Copy link
Member

@pedrooot pedrooot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments but 😍

prowler/providers/aws/aws_provider.py Outdated Show resolved Hide resolved
prowler/providers/aws/aws_provider.py Show resolved Hide resolved
prowler/providers/aws/aws_provider.py Show resolved Hide resolved
@jfagoagas jfagoagas requested a review from pedrooot August 9, 2024 09:21
Copy link
Member

@pedrooot pedrooot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇 🐐

Copy link
Member

@vicferpoy vicferpoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@jfagoagas jfagoagas merged commit 761eeba into master Aug 9, 2024
10 of 11 checks passed
@jfagoagas jfagoagas deleted the PRWLR-4318-aws-test-connection branch August 9, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/aws Issues/PRs related with the AWS provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants