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(codepipeline): add new check codepipeline_project_repo_private #5915

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

yyyy7246
Copy link

Context

This PR introduces a critical security enhancement for AWS CodePipeline by implementing automated verification of repository visibility. This check helps organizations protect sensitive deployment configurations and mitigate potential supply chain attack vectors by ensuring the use of private repositories.

Description

The implementation includes:

  • A new security check codepipeline_project_repo_private that:
    • Validates repository visibility in CodePipeline projects
    • Supports both GitHub and GitLab repositories through CodeStar Connections

Example Output

codepipeline

Core components:

  • codepipeline_client.py: Client interface implementation
  • codepipeline_service.py: Service layer with pipeline state management
  • codepipeline_project_repo_private.py: Main check implementation
  • metadata.json: Check specifications and security context

Technical implementation highlights:

  • Integration with AWS CodeStar Connections API
  • Automated repository visibility detection
  • Comprehensive error handling and logging

Required Permissions

The check requires the following AWS permissions:

  • CodePipeline: ListPipelines, GetPipeline
  • CodeStar Connections: GetConnection

Checklist

License

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

@yyyy7246 yyyy7246 requested review from a team as code owners November 26, 2024 19:09
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label Nov 26, 2024
@MrCloudSec
Copy link
Member

Thanks for this new check @yyyy7246! Could you please add the tests to cover the new service and the check? You can copy the tests from another one, I can help you if you need help, here you can find documentation about the unit testing in Prowler.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 94.18605% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.92%. Comparing base (a4c92ea) to head (4879aec).
Report is 718 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5915      +/-   ##
==========================================
+ Coverage   89.90%   89.92%   +0.02%     
==========================================
  Files        1147     1150       +3     
  Lines       35608    35694      +86     
==========================================
+ Hits        32013    32099      +86     
  Misses       3595     3595              
Components Coverage Δ
prowler 89.92% <94.18%> (+0.02%) ⬆️
api ∅ <ø> (∅)

@yyyy7246
Copy link
Author

Thanks for this new check @yyyy7246! Could you please add the tests to cover the new service and the check? You can copy the tests from another one, I can help you if you need help, here you can find documentation about the unit testing in Prowler.

Thank you for the feedback and guidance!

I have added the unit tests to cover both the new service and the check. These tests include scenarios for both private and public repositories, as well as checks for multiple repository providers (e.g., GitHub and GitLab). The tests can be found in the following files:

  • tests/providers/aws/services/codepipeline/codepipeline_service_test.py
  • tests/providers/aws/services/codepipeline/codepipeline_project_repo_private/codepipeline_project_repo_private_test.py

Please let me know if there are any additional scenarios or improvements you'd like me to include. I’d be happy to address them!

class codepipeline_project_repo_private(Check):
def execute(self):
findings = []
client = boto3.client("codestar-connections")
Copy link
Member

Choose a reason for hiding this comment

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

You cannot use boto3 inside the checks, you will need to create another service class inside codepipeline_service.py. See Logs in cloudwatch_service.py as an example.

Comment on lines 44 to 47
configuration=source_info["configuration"],
)

pipeline.tags = pipeline_info.get("tags", [])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
configuration=source_info["configuration"],
)
pipeline.tags = pipeline_info.get("tags", [])
configuration=source_info["configuration"],
pipeline.tags=pipeline_info.get("tags", []),
)

Copy link
Member

@MrCloudSec MrCloudSec left a comment

Choose a reason for hiding this comment

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

please @yyyy7246, review my comments. Thanks!

@yyyy7246 yyyy7246 force-pushed the feat/codepipeline-project-repo-private branch from fbe0999 to 6dfb8bc Compare December 13, 2024 03:13
@yyyy7246
Copy link
Author

please @yyyy7246, review my comments. Thanks!

Hi MrCloudSec,

Thank you so much for taking the time to review my pull request and provide such detailed feedback. I truly appreciate your guidance.

I have addressed the changes you requested, including:

  1. Refactoring the check logic to avoid direct boto3 calls:
    Instead of using boto3 within codepipeline_project_repo_private.py, I created a dedicated service class in codepipeline_service.py to handle the logic, following the recommended architecture pattern.

  2. Improving source configuration handling and tags assignment:
    In codepipeline_service.py, I’ve made the code more structured and applied your suggested improvements for assigning pipeline.tags.

  3. Updating the metadata file:
    I revised the codepipeline_project_repo_private.metadata.json file to accurately reflect the new check, including an updated description, risks, and remediation steps.

I also want to clarify the force-push action: I initially misunderstood its impact. Fortunately, it didn’t cause any issues, and I’ll be more cautious about such actions in the future.

Thank you again for your thorough review and helpful suggestions. If there’s anything else you’d like me to adjust or improve, please let me know.

Warm regards,
yyyy7246

type=source_info["actionTypeId"]["provider"],
location=source_info["configuration"].get("FullRepositoryId", ""),
configuration=source_info["configuration"],
tags=pipeline_info.get("tags", []),
Copy link
Member

Choose a reason for hiding this comment

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

The key tags does not exist, you have to use the API call list_tags_for_resource

Copy link
Member

@MrCloudSec MrCloudSec left a comment

Choose a reason for hiding this comment

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

Please, review my comments @yyyy7246

@yyyy7246
Copy link
Author

Please, review my comments @yyyy7246

Thank you for the feedback, @MrCloudSec. I've updated the code to use the list_tags_for_resource API as suggested. This change should now properly handle the retrieval of tags.

Please let me know if there's anything else that needs adjustment. Thanks for your guidance!


# Get tags using list_tags_for_resource API
try:
tags_response = regional_client.list_tags_for_resource(
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@yyyy7246 and a test for that function in the service test please.

Copy link
Author

Choose a reason for hiding this comment

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

@yyyy7246 and a test for that function in the service test please.

Hello @MrCloudSec,

I've refactored the code to move the tag retrieval logic into a separate _list_tags_for_resource function, following the pattern you pointed out in the SNS service. In addition, I've added a dedicated test for this function within the service tests.

Thank you for the feedback—it helped me improve the code structure and testing approach. Please let me know if you have any further suggestions.

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.

2 participants