feat(waf): add check for regional web ACL logging enabled#11539
Open
Sid-0602 wants to merge 2 commits into
Open
feat(waf): add check for regional web ACL logging enabled#11539Sid-0602 wants to merge 2 commits into
Sid-0602 wants to merge 2 commits into
Conversation
github-actions
Bot
added
provider/aws
📝 WalkthroughWalkthroughA new Prowler check ChangesWAF Regional Web ACL Logging Check Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
![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/CHANGELOG.md`:
- Line 9: Update the placeholder PR number in the changelog entry for the
`waf_regional_webacl_logging_enabled` check: replace the `(`#XXXX`)` token with
the actual PR reference `(`#11539`)` in the CHANGELOG.md entry so the line reads
with the correct PR number.
In `@prowler/providers/aws/services/waf/waf_service.py`:
- Around line 283-301: Add a Google-style docstring to the
_get_logging_configuration method describing its purpose (retrieve WAF regional
Web ACL logging config), the parameters (acl: object with name, arn, region and
expected attributes), what it does (queries
regional_clients.get_logging_configuration and sets acl.logging_enabled based on
LoggingConfiguration.LogDestinationConfigs), return/side-effects (mutates
acl.logging_enabled) and error handling (logs exceptions). Place the docstring
immediately below the def _get_logging_configuration(self, acl): signature and
follow Google docstring sections: Args, Returns (or Side Effects), and
Raises/Errors if applicable.
🪄 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: f468c46d-ce8a-4aa0-a17d-f1909cfeeb69
📒 Files selected for processing (7)
prowler/CHANGELOG.mdprowler/providers/aws/services/waf/waf_regional_webacl_logging_enabled/__init__.pyprowler/providers/aws/services/waf/waf_regional_webacl_logging_enabled/waf_regional_webacl_logging_enabled.metadata.jsonprowler/providers/aws/services/waf/waf_regional_webacl_logging_enabled/waf_regional_webacl_logging_enabled.pyprowler/providers/aws/services/waf/waf_service.pytests/providers/aws/services/waf/waf_regional_webacl_logging_enabled/__init__.pytests/providers/aws/services/waf/waf_regional_webacl_logging_enabled/waf_regional_webacl_logging_enabled_test.py
…get_logging_configuration
There was a problem hiding this comment.
♻️ Duplicate comments (1)
prowler/providers/aws/services/waf/waf_service.py (1)
283-283: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd type hints to method signature.
The coding guidelines require type hints for all public functions. Although this is a private method, adding type hints improves maintainability and IDE support. Based on the docstring,
aclshould be typed asWebAcl, and the return type should be explicit.🔧 Proposed enhancement
- def _get_logging_configuration(self, acl): + def _get_logging_configuration(self, acl: "WebAcl") -> None: """Fetch and store the logging configuration for a Regional Web ACL.As per coding guidelines, type hints are required for all public functions in Python code (prowler/**/*.py guideline).
🤖 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/waf/waf_service.py` at line 283, Add explicit type hints to the _get_logging_configuration method signature: annotate the acl parameter as WebAcl and add an explicit return type (e.g., Optional[LoggingConfiguration] or the actual logging config type used in the module) so IDEs and linters can validate usage; update any necessary imports (from typing import Optional and the WebAcl/LoggingConfiguration types) and ensure the docstring still matches the annotated types.Source: Coding guidelines
🤖 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 `@prowler/providers/aws/services/waf/waf_service.py`:
- Line 283: Add explicit type hints to the _get_logging_configuration method
signature: annotate the acl parameter as WebAcl and add an explicit return type
(e.g., Optional[LoggingConfiguration] or the actual logging config type used in
the module) so IDEs and linters can validate usage; update any necessary imports
(from typing import Optional and the WebAcl/LoggingConfiguration types) and
ensure the docstring still matches the annotated types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7555487-cba3-4916-af6a-bbfde1c39d90
📒 Files selected for processing (2)
prowler/CHANGELOG.mdprowler/providers/aws/services/waf/waf_service.py
Author
|
Hey @danibarranqueroo @HugoPBrito !! |
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
This PR adds a new AWS check for WAF Classic Regional Web ACL logging. There is no existing issue; this gap was identified by cross-referencing Prowler's existing coverage,
waf_global_webacl_logging_enabledexists for CloudFront-scoped (global) Web ACLs but the regional equivalent protecting Application Load Balancers and API Gateway stages was missing.Description
Adds
waf_regional_webacl_logging_enabled, a new check that verifies each AWS WAF Classic Regional Web ACL has logging enabled by checking for at least one configured log destination.This PR includes a small service change alongside the new check:
waf_service.py: Added_get_logging_configurationmethod to theWAFRegionalclass and wired it into__init__. TheWebAclmodel already had alogging_enabled: boolfield; this change simply populates it for regional ACLs, mirroring the identical method already present in theWAFglobal class.wafregional_client, 3 test cases (no ACLs, logging disabled, logging enabled).No new IAM permissions needed. The check reads from
waf-regional:GetLoggingConfiguration, which is already included in Prowler's WAF Regional permission baseline.Steps to review
prowler/providers/aws/services/waf/waf_service.py, confirm the new_get_logging_configurationmethod inWAFRegionalmirrors the existing one inWAF, usingself.regional_clients[acl.region]instead ofself.clientwaf_regional_webacl_logging_enabled.py; it iterateswafregional_client.web_aclsand checksacl.logging_enabledpytest tests/providers/aws/services/waf/waf_regional_webacl_logging_enabled/ -vpython prowler-cli.py aws --list-checks | grep waf_regional_webacl_loggingwaf_global_webacl_logging_enabledto confirm the regional check is consistent in structure and messagingChecklist
Community Checklist
SDK/CLI
waf-regional:GetLoggingConfigurationis already part of Prowler's WAF Regional permission set.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