fix(azure): keyvault_logging_enabled false-fails on explicit AuditEvent category#11657
Open
m-khan-97 wants to merge 2 commits into
Open
Conversation
1 task
github-actions
Bot
added
the
provider/azure
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Azure Changeskeyvault_logging_enabled AuditEvent fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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)
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
|
✅ 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: 1
🤖 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
`@tests/providers/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled_test.py`:
- Around line 247-321: Add a new negative test case method to validate the FAIL
scenario for the new AuditEvent path. Create a test similar to
test_diagnostic_setting_with_explicit_auditevent_category but configure the
diagnostic setting so that category="AuditEvent" is enabled=False while
category_group="allLogs" is also not enabled or enabled=False. This test should
verify that the check returns a FAIL status and confirm that the new OR
condition in the keyvault_logging_enabled check logic properly handles the case
where neither audit logging mechanism is properly enabled.
🪄 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: 41668a11-c4f9-4d0d-9da8-81a69559e9cc
📒 Files selected for processing (3)
prowler/CHANGELOG.mdprowler/providers/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled.pytests/providers/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled_test.py
Comment on lines
+247
to
+321
| def test_diagnostic_setting_with_explicit_auditevent_category(self): | ||
| keyvault_client = mock.MagicMock | ||
| keyvault_client.subscriptions = {AZURE_SUBSCRIPTION_ID: AZURE_SUBSCRIPTION_NAME} | ||
|
|
||
| with ( | ||
| mock.patch( | ||
| "prowler.providers.common.provider.Provider.get_global_provider", | ||
| return_value=set_mocked_azure_provider(), | ||
| ), | ||
| mock.patch( | ||
| "prowler.providers.azure.services.monitor.monitor_service.Monitor", | ||
| new=mock.MagicMock(), | ||
| ), | ||
| mock.patch( | ||
| "prowler.providers.azure.services.keyvault.keyvault_logging_enabled.keyvault_logging_enabled.keyvault_client", | ||
| new=keyvault_client, | ||
| ), | ||
| ): | ||
| from prowler.providers.azure.services.keyvault.keyvault_logging_enabled.keyvault_logging_enabled import ( | ||
| keyvault_logging_enabled, | ||
| ) | ||
| from prowler.providers.azure.services.keyvault.keyvault_service import ( | ||
| KeyVaultInfo, | ||
| ) | ||
| from prowler.providers.azure.services.monitor.monitor_service import ( | ||
| DiagnosticSetting, | ||
| ) | ||
|
|
||
| keyvault_client.key_vaults = { | ||
| AZURE_SUBSCRIPTION_ID: [ | ||
| KeyVaultInfo( | ||
| id="id", | ||
| name="name_keyvault", | ||
| location="westeurope", | ||
| resource_group="resource_group", | ||
| properties=VaultProperties( | ||
| tenant_id="tenantid", | ||
| sku="sku", | ||
| enable_rbac_authorization=False, | ||
| ), | ||
| keys=[], | ||
| secrets=[], | ||
| monitor_diagnostic_settings=[ | ||
| DiagnosticSetting( | ||
| id="id/ds1", | ||
| logs=[ | ||
| mock.MagicMock( | ||
| category_group=None, | ||
| category="AuditEvent", | ||
| enabled=True, | ||
| ), | ||
| mock.MagicMock( | ||
| category_group="allLogs", | ||
| category=None, | ||
| enabled=True, | ||
| ), | ||
| ], | ||
| storage_account_name="sa1", | ||
| storage_account_id="sa_id1", | ||
| name="ds_compliant", | ||
| ), | ||
| ], | ||
| ), | ||
| ] | ||
| } | ||
| check = keyvault_logging_enabled() | ||
| result = check.execute() | ||
| assert len(result) == 1 | ||
| assert result[0].status == "PASS" | ||
| assert ( | ||
| result[0].status_extended | ||
| == f"Key Vault name_keyvault in subscription {AZURE_SUBSCRIPTION_DISPLAY} has a diagnostic setting with audit logging." | ||
| ) | ||
| assert result[0].resource_name == "name_keyvault" | ||
| assert result[0].resource_id == "id" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding a negative test case for the new AuditEvent path.
The new test validates the PASS scenario when category="AuditEvent" is enabled alongside allLogs. For comprehensive coverage of the new OR condition, consider adding a FAIL test case where category="AuditEvent" is enabled but category_group="allLogs" is disabled. This would explicitly verify that the new condition doesn't inadvertently pass when it shouldn't.
Note: The existing test_diagnostic_setting_without_audit_logging already covers this pattern with category_group="audit", so this is an optional enhancement rather than a critical gap.
🤖 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/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled_test.py`
around lines 247 - 321, Add a new negative test case method to validate the FAIL
scenario for the new AuditEvent path. Create a test similar to
test_diagnostic_setting_with_explicit_auditevent_category but configure the
diagnostic setting so that category="AuditEvent" is enabled=False while
category_group="allLogs" is also not enabled or enabled=False. This test should
verify that the check returns a FAIL status and confirm that the new OR
condition in the keyvault_logging_enabled check logic properly handles the case
where neither audit logging mechanism is properly enabled.
…nt category Azure diagnostic settings can represent Key Vault audit logging either via the "audit" category group, or via an explicit log category named "AuditEvent" with a null category group. The check only recognized the former, causing false FAIL results for vaults that have audit logging enabled through the explicit category.
m-khan-97
force-pushed
the
fix/azure-keyvault-logging-auditevent-category
branch
from
7260440 to
4442778
Compare
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
1 participant
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 #11656.
keyvault_logging_enabledonly recognized Key Vault audit logging when the diagnostic setting log used theauditcategory group. Azure can also represent the same audit logging via an explicit log category namedAuditEventwith anullcategory group, which the check did not account for, producing a falseFAIL.Description
In
prowler/providers/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled.py, thehas_auditcondition now also acceptslog.category == "AuditEvent"(in addition to the existinglog.category_group == "audit"), both gated onlog.enabled. TheallLogscategory-group condition is unchanged.Added
test_diagnostic_setting_with_explicit_auditevent_categorytotests/providers/azure/services/keyvault/keyvault_logging_enabled/keyvault_logging_enabled_test.py, mirroring the existing test style, covering a diagnostic setting withcategory="AuditEvent"/category_group=Nonepluscategory_group="allLogs"and assertingPASS.Added a CHANGELOG.md entry under
### 🐞 Fixed.Steps to review
uv run pytest -q tests/providers/azure/services/keyvault/keyvault_logging_enabled/— all 7 tests pass, including the new regression test.uv run pytest -q tests/providers/azure/services/keyvault/— full keyvault service suite (52 tests) still passes.Checklist
SDK/CLI
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
Bug Fixes
AuditEventlog category, preventing false-negative results.Tests
AuditEventcategory.