-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_web_application_firewall_policy
- fix the value of rule_group_name
does not exist for Application Gateway Firewall
#24194
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sinbai - I think this can be fixed in a more straight forward way depending on what the API accepts. Please take a look at the comments left in-line.
func convertRuleGroupNameToBeAcceptedByAPI(name string) string { | ||
var ruleGroupName string | ||
switch name { | ||
case "APPLICATION-ATTACK-LFI": | ||
ruleGroupName = "LFI" | ||
case "APPLICATION-ATTACK-RFI": | ||
ruleGroupName = "RFI" | ||
case "APPLICATION-ATTACK-RCE": | ||
ruleGroupName = "RCE" | ||
case "APPLICATION-ATTACK-PHP": | ||
ruleGroupName = "PHP" | ||
case "APPLICATION-ATTACK-NodeJS": | ||
ruleGroupName = "NODEJS" | ||
case "APPLICATION-ATTACK-XSS": | ||
ruleGroupName = "XSS" | ||
case "APPLICATION-ATTACK-SQLI": | ||
ruleGroupName = "SQLI" | ||
case "APPLICATION-ATTACK-SESSION-FIXATION": | ||
ruleGroupName = "FIX" | ||
case "APPLICATION-ATTACK-SESSION-JAVA": | ||
ruleGroupName = "JAVA" | ||
default: | ||
ruleGroupName = name | ||
} | ||
return ruleGroupName | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the API doesn't accept any of these values then we can probably just remove these from the validation function instead of adding a conversion function here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
func convertRuleGroupNameFromAPI(name string) string { | ||
var ruleGroupName string | ||
switch name { | ||
case "LFI": | ||
ruleGroupName = "APPLICATION-ATTACK-LFI" | ||
case "RFI": | ||
ruleGroupName = "APPLICATION-ATTACK-RFI" | ||
case "RCE": | ||
ruleGroupName = "APPLICATION-ATTACK-RCE" | ||
case "PHP": | ||
ruleGroupName = "APPLICATION-ATTACK-PHP" | ||
case "NODEJS": | ||
ruleGroupName = "APPLICATION-ATTACK-NodeJS" | ||
case "XSS": | ||
ruleGroupName = "APPLICATION-ATTACK-XSS" | ||
case "SQLI": | ||
ruleGroupName = "APPLICATION-ATTACK-SQLI" | ||
case "FIX": | ||
ruleGroupName = "APPLICATION-ATTACK-SESSION-FIXATION" | ||
case "JAVA": | ||
ruleGroupName = "APPLICATION-ATTACK-SESSION-JAVA" | ||
default: | ||
ruleGroupName = name | ||
} | ||
return ruleGroupName | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can also remove this unnecessary conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it’s better to just add new values (needed for Microsoft_DefaultRuleSet 2.1 ) and don't touch the old ones (needed for OWASP 3.2) ?
@alexdokka if the old ones are still valid for a different configuration then I agree, we should just add the new ones. |
@alexdokka In fact, the old ones are not supported by the other versions either. |
Hi @stephybun thanks for your feedback. The code has been updated. Could you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the validation values and for checking the behaviour of the API @sinbai
There are actually two values that are missing from the validation and I think it'd be helpful to add a comment that will enable us to keep these values up to date.
Once those two additions are in this is good to go!
"APPLICATION-ATTACK-SQLI", | ||
"APPLICATION-ATTACK-SESSION-FIXATION", | ||
"APPLICATION-ATTACK-SESSION-JAVA", | ||
"LFI", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this further there is a command which returns a list of supported rule set names and there are two that are still missing. Can you please add crs_49_inbound_blocking
and KnownBadBots
to the validation.
Can you also please add the following comment in this validation function - It will be helpful to ensure this list remains up to date:
// The following command will return a list of available Rule Group Names with information on whether the rules are GA, Deprecated, etc.:
// az rest --method get --url “https://management.azure.com/subscriptions/{subscription_id_here}/providers/Microsoft.Network/locations/{location}/applicationGatewayWafDynamicManifests/default?api-version=2023-05-01” --query “properties.availableRuleSets[].ruleGroups[].ruleGroupName” | sort | uniq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion about adding above comment, the code has been updated. Could you please take a look at it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🧯
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.84.0" to "3.85.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.85.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.85.0
FEATURES:

* New Data Source: `azurerm_locations` ([#23324](hashicorp/terraform-provider-azurerm#23324 New Resource: `azurerm_iotcentral_organization` ([#23132](https://github.com/hashicorp/terraform-provider-azurerm/issues/23132))

ENHANCEMENTS:

* provider: support for authenticating using Azure Kubernetes Service Workload Identity ([#23965](hashicorp/terraform-provider-azurerm#23965 dependencies: updating to `v0.65.0` of `github.com/hashicorp/go-azure-helpers` ([#24222](hashicorp/terraform-provider-azurerm#24222 dependencies: updating to `v0.20231214.1220802` of `github.com/hashicorp/go-azure-sdk` ([#24246](hashicorp/terraform-provider-azurerm#24246 dependencies: updating to version `v0.20231214.1160726` of `github.com/hashicorp/go-azure-sdk` ([#24241](hashicorp/terraform-provider-azurerm#24241 dependencies: update `security/automation` to use `hashicorp/go-azure-sdk` ([#24156](hashicorp/terraform-provider-azurerm#24156 `dataprotection`: updating to API Version `2023-05-01` ([#24143](hashicorp/terraform-provider-azurerm#24143 `kusto`: removing the remnants of the old Resource ID Parsers now this uses `hashicorp/go-azure-sdk` ([#24238](hashicorp/terraform-provider-azurerm#24238 Data Source: `azurerm_cognitive_account` - export the `identity` block ([#24214](hashicorp/terraform-provider-azurerm#24214 Data Source: `azurerm_monitor_workspace` - add support for the `default_data_collection_endpoint_id` and `default_data_collection_rule_id` properties ([#24153](hashicorp/terraform-provider-azurerm#24153 Data Source: `azurerm_shared_image_gallery` - add support for the `image_names` property ([#24176](hashicorp/terraform-provider-azurerm#24176 `azurerm_dns_txt_record` - allow up to `4096` characters for the property `record.value` ([#24169](hashicorp/terraform-provider-azurerm#24169 `azurerm_container_app` - support for the `workload_profile_name` property ([#24219](hashicorp/terraform-provider-azurerm#24219 `azurerm_container_app` - suppot for the `init_container` block ([#23955](hashicorp/terraform-provider-azurerm#23955 `azurerm_hpc_cache_blob_nfs_target` - support for the `verification_timer_in_seconds` and `write_back_timer_in_seconds` properties ([#24207](hashicorp/terraform-provider-azurerm#24207 `azurerm_hpc_cache_nfs_target` - support for the `verification_timer_in_seconds` and `write_back_timer_in_seconds` properties ([#24208](hashicorp/terraform-provider-azurerm#24208 `azurerm_linux_web_app` - make `client_secret_setting_name` optional and conflict with `client_secret_certificate_thumbprint` ([#21834](hashicorp/terraform-provider-azurerm#21834 `azurerm_linux_web_app_slot` - make `client_secret_setting_name` optional and conflict with `client_secret_certificate_thumbprint` ([#21834](hashicorp/terraform-provider-azurerm#21834 `azurerm_linux_web_app` - fix a bug in `app_settings` where settings could be lost ([#24221](hashicorp/terraform-provider-azurerm#24221 `azurerm_linux_web_app_slot` - fix a bug in `app_settings` where settings could be lost ([#24221](hashicorp/terraform-provider-azurerm#24221 `azurerm_log_analytics_workspace` - add support for the `immediate_data_purge_on_30_days_enabled` property ([#24015](hashicorp/terraform-provider-azurerm#24015 `azurerm_mssql_server` - support for other identity types for the key vault key ([#24236](hashicorp/terraform-provider-azurerm#24236 `azurerm_machine_learning_datastore_blobstorage` - resource now skips validation when being created ([#24078](hashicorp/terraform-provider-azurerm#24078 `azurerm_machine_learning_datastore_datalake_gen2` - resource now skips validation when being created ([#24078](hashicorp/terraform-provider-azurerm#24078 `azurerm_machine_learning_datastore_fileshare` - resource now skips validation when being created ([#24078](hashicorp/terraform-provider-azurerm#24078 `azurerm_monitor_workspace` - support for the `default_data_collection_endpoint_id` and `default_data_collection_rule_id` properties ([#24153](hashicorp/terraform-provider-azurerm#24153 `azurerm_redis_cache` - support for the `storage_account_subscription_id` property ([#24101](hashicorp/terraform-provider-azurerm#24101 `azurerm_storage_blob` - support for the `source_content` type `Page` ([#24177](hashicorp/terraform-provider-azurerm#24177 `azurerm_web_application_firewall_policy` - support new values to the `rule_group_name` property ([#24194](hashicorp/terraform-provider-azurerm#24194 `azurerm_windows_web_app` - make the `client_secret_setting_name` property optional and conflicts with the `client_secret_certificate_thumbprint` property ([#21834](hashicorp/terraform-provider-azurerm#21834 `azurerm_windows_web_app_slot` - make the `client_secret_setting_name` property optional and conflicts with the `client_secret_certificate_thumbprint` property ([#21834](hashicorp/terraform-provider-azurerm#21834 `azurerm_windows_web_app` - fix a bug in `app_settings` where settings could be lost ([#24221](hashicorp/terraform-provider-azurerm#24221 `azurerm_windows_web_app_slot` - fix a bug in `app_settings` where settings could be lost ([#24221](hashicorp/terraform-provider-azurerm#24221 `azurerm_cognitive_account` - add `ContentSafety` to the `kind` property validation ([#24205](https://github.com/hashicorp/terraform-provider-azurerm/issues/24205))

BUG FIXES:

* provider: fix an authentication issue with Azure Storage when running in Azure China cloud ([#24246](hashicorp/terraform-provider-azurerm#24246 Data Source: `azurerm_role_definition` - fix bug where `role_definition_id` and `scope` were being incorrectly set ([#24211](hashicorp/terraform-provider-azurerm#24211 `azurerm_batch_account` - fix bug where `UserAssigned, SystemAssigned` could be passed to the resource even though it isn't supported ([#24204](hashicorp/terraform-provider-azurerm#24204 `azurerm_batch_pool` - fix bug where `settings_json` and `protected_settings` were not being unmarshaled ([#24075](hashicorp/terraform-provider-azurerm#24075 `azurerm_bot_service_azure_bot` - fix bug where `public_network_access_enabled` was being set as the value for `LuisKey` ([#24164](hashicorp/terraform-provider-azurerm#24164 `azurerm_cognitive_account_customer_managed_key` - `identity_client_id` is no longer passed to the api when it is empty ([#24231](hashicorp/terraform-provider-azurerm#24231 `azurerm_linux_web_app_slot` - error when `service_plan_id` is identical to the parent `service_plan_id` ([#23403](hashicorp/terraform-provider-azurerm#23403 `azurerm_management_group_template_deployment` - fixing a bug where `template_spec_version_id` couldn't be updated ([#24072](hashicorp/terraform-provider-azurerm#24072 `azurerm_pim_active_role_assignment` - fix an importing issue by filtering available role assignments based on the provided `scope` ([#24077](hashicorp/terraform-provider-azurerm#24077 `azurerm_pim_eligible_role_assignment` - fix an importing issue by filtering available role assignments based on the provided `scope` ([#24077](hashicorp/terraform-provider-azurerm#24077 `azurerm_resource_group_template_deployment` - fixing a bug where `template_spec_version_id` couldn't be updated ([#24072](hashicorp/terraform-provider-azurerm#24072 `azurerm_security_center_setting` - fix the casing for the `setting_name` `Sentinel` ([#24210](hashicorp/terraform-provider-azurerm#24210 `azurerm_storage_account` - Fix crash when checking for `routingInputs.PublishInternetEndpoints` and `routingInputs.PublishMicrosoftEndpoints` ([#24228](hashicorp/terraform-provider-azurerm#24228 `azurerm_storage_share_file` - prevent panic when the file specified by `source` is empty ([#24179](hashicorp/terraform-provider-azurerm#24179 `azurerm_subscription_template_deployment` - fixing a bug where `template_spec_version_id` couldn't be updated ([#24072](hashicorp/terraform-provider-azurerm#24072 `azurerm_tenant_template_deployment` - fixing a bug where `template_spec_version_id` couldn't be updated ([#24072](hashicorp/terraform-provider-azurerm#24072 `azurerm_virtual_machine` - prevent a panic by nil checking the first element of `additional_capabilities` ([#24159](hashicorp/terraform-provider-azurerm#24159 `azurerm_windows_web_app_slot` - error when `service_plan_id` is identical to the parent `service_plan_id` ([#23403](https://github.com/hashicorp/terraform-provider-azurerm/issues/23403))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/942/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
After testing, the following values of
rule_group_name
of DRS do not exist for Application Gateway Firewall."APPLICATION-ATTACK-LFI",
"APPLICATION-ATTACK-RFI",
"APPLICATION-ATTACK-RCE",
"APPLICATION-ATTACK-PHP",
"APPLICATION-ATTACK-NodeJS",
"APPLICATION-ATTACK-XSS",
"APPLICATION-ATTACK-SQLI",
"APPLICATION-ATTACK-SESSION-FIXATION",
"APPLICATION-ATTACK-SESSION-JAVA"
The facts should look like this:
"LFI",
"RFI",
"RCE",
"PHP",
"NODEJS",
"XSS",
"SQLI",
"FIX",
"JAVA"
Submit this PR to update
rule_group_name
in order to fix #24160.Test result:
PASS: TestAccWebApplicationFirewallPolicy_ManagedRuleSetDRS (312.58s)