feat(sdk): add Data Pipeline secret scanning check#11821
Open
YinkaMetrics wants to merge 3 commits into
Open
Conversation
github-actions
Bot
added
provider/aws
📝 WalkthroughWalkthroughThis PR adds a new AWS Data Pipeline service, a secret-scanning check for pipeline definitions, required IAM permissions, and tests covering pipeline loading and scan outcomes. ChangesData Pipeline secret detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DataPipeline as DataPipeline
participant RegionalClient as AWS Data Pipeline regional client
participant Pipeline as Pipeline
DataPipeline->>RegionalClient: list_pipelines
RegionalClient-->>DataPipeline: pipeline ids and names
DataPipeline->>Pipeline: create ARN and region entry
DataPipeline->>RegionalClient: describe_pipelines
RegionalClient-->>DataPipeline: tags
DataPipeline->>RegionalClient: get_pipeline_definition
RegionalClient-->>DataPipeline: pipelineObjects, parameterObjects, parameterValues
DataPipeline->>Pipeline: store tags and definition
sequenceDiagram
participant Check as datapipeline_pipeline_no_secrets_in_definition
participant Client as datapipeline_client
participant Scanner as detect_secrets_scan_batch
Check->>Client: read pipelines
Check->>Check: _build_definition_payload(definition)
Check->>Scanner: scan payloads with ignore/validate settings
Scanner-->>Check: scan results or SecretsScanError
Check->>Check: annotate_verified_secrets on FAIL
Check-->>Check: emit PASS/FAIL/MANUAL reports
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@prowler/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition.py`:
- Around line 19-42: The generator-based payload construction in the
Datapipeline secret check can leave later pipelines without line context if
detect_secrets_scan_batch raises early, causing the reporting loop to fall back
to PASS. Materialize the payload and line-context mapping up front in
datapipeline_pipeline_no_secrets_in_definition (around payloads() and the
scan_error handling) so every pipeline index is recorded before calling
detect_secrets_scan_batch, and keep the MANUAL path in the result loop based on
scan_error even when a pipeline was not reached by the scanner.
In `@prowler/providers/aws/services/datapipeline/datapipeline_service.py`:
- Around line 9-17: The Pipeline model currently defines tags but they are never
populated, so report.resource_tags remains empty. Update the AWS Data Pipeline
flow in datapipeline_service.py so the tags returned by describe_pipelines are
copied onto Pipeline.tags when building the model, using the existing Pipeline
class and list_pipelines/describe_pipelines data path; if tags cannot be
propagated, remove the unused tags field instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f07cb1e-8cb0-4a57-a6dc-7e435d221fb5
📒 Files selected for processing (11)
permissions/prowler-additions-policy.jsonpermissions/templates/cloudformation/prowler-scan-role.ymlprowler/providers/aws/services/datapipeline/__init__.pyprowler/providers/aws/services/datapipeline/datapipeline_client.pyprowler/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/__init__.pyprowler/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition.metadata.jsonprowler/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition.pyprowler/providers/aws/services/datapipeline/datapipeline_service.pytests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/__init__.pytests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.pytests/providers/aws/services/datapipeline/datapipeline_service_test.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prowler/providers/aws/services/datapipeline/datapipeline_service.py (1)
67-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSplit error handling so a
describe_pipelinesfailure doesn't block the actual secret scan.
describe_pipelines(tags-only, cosmetic) andget_pipeline_definition(the data the security check actually scans) share one try/except. Ifdescribe_pipelinesraises (e.g. throttling, missing permission — see next comment),get_pipeline_definitionnever runs andpipeline.definitionstays at its empty default. Downstream, thedatapipeline_pipeline_no_secrets_in_definitioncheck would then scan an empty definition and likely report PASS, silently missing real secrets instead of flagging a scan failure.🛠️ Proposed fix: isolate the tags fetch
try: regional_client = self.regional_clients[pipeline.region] - pipeline_descriptions = regional_client.describe_pipelines( - pipelineIds=[pipeline.id] - ).get("pipelineDescriptionList", []) - if pipeline_descriptions: - pipeline.tags = pipeline_descriptions[0].get("tags", []) + try: + pipeline_descriptions = regional_client.describe_pipelines( + pipelineIds=[pipeline.id] + ).get("pipelineDescriptionList", []) + if pipeline_descriptions: + pipeline.tags = pipeline_descriptions[0].get("tags", []) + except (ClientError, Exception) as error: + logger.error( + f"{pipeline.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}" + ) definition = regional_client.get_pipeline_definition(pipelineId=pipeline.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prowler/providers/aws/services/datapipeline/datapipeline_service.py` around lines 67 - 79, The shared try/except in DatapipelineService is letting a tags-only `describe_pipelines` failure prevent `get_pipeline_definition` from running, which can leave `pipeline.definition` empty and hide real findings. Refactor the logic in `DatapipelineService` so the `describe_pipelines` call is isolated from the definition fetch: keep tag loading best-effort, but always attempt `get_pipeline_definition` and assign `pipeline.definition` even if tag retrieval fails. Use the existing `regional_client`, `describe_pipelines`, and `get_pipeline_definition` calls as the key places to split the error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prowler/providers/aws/services/datapipeline/datapipeline_service.py`:
- Around line 69-71: The Data Pipeline scan path now calls describe_pipelines in
datapipeline_service, but the scan role permissions are missing
datapipeline:DescribePipelines, which causes AccessDeniedException when
resources are found. Update the scan-role permission sources in
permissions/prowler-additions-policy.json and
permissions/templates/cloudformation/prowler-scan-role.yml to include the new
action alongside GetPipelineDefinition and ListPipelines so pipeline tags can be
retrieved successfully.
In
`@tests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.py`:
- Around line 108-151: The current test coverage only verifies PASS/FAIL/MANUAL
outcomes in test_scan_error_marks_all_scannable_pipelines_manual, but it does
not exercise the verified-secret path. Add a focused test around _execute_check
/ detect_secrets_scan_batch that simulates a live-verified secret using the
annotate_verified_secrets or validate flag, and assert the resulting report
severity is escalated to Critical. Use the existing datapipeline_client and
report assertions pattern so the new test clearly covers the verified-secret
branch.
In `@tests/providers/aws/services/datapipeline/datapipeline_service_test.py`:
- Around line 33-53: Add a companion test around the datapipeline service path
covered by mock_make_api_call and the pipeline.tags assertion that simulates
DescribePipelines failing while GetPipelineDefinition still succeeds. Use the
existing Datapipeline service test helpers and the pipeline/pipeline_definition
fixtures to verify the service’s split try/except behavior still populates
pipeline.definition even when DescribePipelines raises, and keep the new tags
assertion in the happy-path test.
---
Outside diff comments:
In `@prowler/providers/aws/services/datapipeline/datapipeline_service.py`:
- Around line 67-79: The shared try/except in DatapipelineService is letting a
tags-only `describe_pipelines` failure prevent `get_pipeline_definition` from
running, which can leave `pipeline.definition` empty and hide real findings.
Refactor the logic in `DatapipelineService` so the `describe_pipelines` call is
isolated from the definition fetch: keep tag loading best-effort, but always
attempt `get_pipeline_definition` and assign `pipeline.definition` even if tag
retrieval fails. Use the existing `regional_client`, `describe_pipelines`, and
`get_pipeline_definition` calls as the key places to split the error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b837a1d3-008b-4d5e-a692-08baaf65f414
📒 Files selected for processing (4)
prowler/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition.pyprowler/providers/aws/services/datapipeline/datapipeline_service.pytests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.pytests/providers/aws/services/datapipeline/datapipeline_service_test.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.py (1)
108-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood addition — resolves prior gap on verified-secret escalation coverage.
This test correctly exercises the verified-secret path (
is_verified: True→Severity.critical, "confirmed to be live"), addressing the previously flagged coverage gap.One minor nit:
Severityis imported locally inside the test method (Line 109) instead of at the top of the file with other imports, which is inconsistent with the rest of the file's style.🧹 Optional cleanup
- def test_pipeline_with_verified_secret_escalates_severity(self): - from prowler.lib.check.models import Severity - - pipeline = _build_pipeline( + def test_pipeline_with_verified_secret_escalates_severity(self): + pipeline = _build_pipeline(And add at the top of the file:
from prowler.lib.check.models import Severity🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.py` around lines 108 - 149, The new verified-secret test is correct, but the `Severity` import is inconsistent because it is done locally inside `test_pipeline_with_verified_secret_escalates_severity` instead of with the file’s other imports. Move the `Severity` import to the top-level import section so the test method only uses it, matching the style used elsewhere in the test module. Use the `test_pipeline_with_verified_secret_escalates_severity` symbol to locate the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@tests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.py`:
- Around line 108-149: The new verified-secret test is correct, but the
`Severity` import is inconsistent because it is done locally inside
`test_pipeline_with_verified_secret_escalates_severity` instead of with the
file’s other imports. Move the `Severity` import to the top-level import section
so the test method only uses it, matching the style used elsewhere in the test
module. Use the `test_pipeline_with_verified_secret_escalates_severity` symbol
to locate the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 608ae993-3319-48da-9910-fb2de624e114
📒 Files selected for processing (5)
permissions/prowler-additions-policy.jsonpermissions/templates/cloudformation/prowler-scan-role.ymlprowler/providers/aws/services/datapipeline/datapipeline_service.pytests/providers/aws/services/datapipeline/datapipeline_pipeline_no_secrets_in_definition/datapipeline_pipeline_no_secrets_in_definition_test.pytests/providers/aws/services/datapipeline/datapipeline_service_test.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Context
Fixes #11818
Description
Adds AWS Data Pipeline support to the SDK and a new
datapipeline_pipeline_no_secrets_in_definitioncheck. The check scans Data Pipeline objects, parameter objects, and parameter values with the shared secret-scanning helper, reports the pipeline/object context without exposing secret values, and uses the existing verified-secret annotation path so live secrets are raised to critical.Also updates the Prowler scan role additions policy and CloudFormation template with the read-only Data Pipeline permissions needed for collection.
Steps to review
Local note:
uv run cfn-lint permissions/templates/cloudformation/prowler-scan-role.ymlcurrently reports pre-existing warning W1031 on the S3 integration bucket ARN substitution, outside the Data Pipeline change.Checklist
Community Checklist
SDK/CLI
datapipeline:GetPipelineDefinitionanddatapipeline:ListPipelinesto the scan role additions.UI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit