new plugin for Azure Network Virtual Hub#6172
Conversation
|
|
||
| return if (defined($self->{option_results}->{command_options}) && $self->{option_results}->{command_options} ne ''); | ||
|
|
||
| my $cmd_options = "monitor metrics list-definitions --resource '$options{resource}' --only-show-errors --output json"; |
There was a problem hiding this comment.
azure_list_resource_metrics_set_cmd places $options{resource} directly into a shell command string (--resource '$options{resource}'), enabling command/argument injection if resource is untrusted. Use argument arrays or sanitize/encode the value.
Details
✨ AI Reasoning
A new function returns an Azure CLI command string with the --resource argument directly interpolated from $options{resource} into the command string. If $options{resource} can be controlled by an external user, this direct interpolation into a shell-executed command permits injection of additional CLI options or shell metacharacters. The risk arises where the assembled command is passed to the system execution routine without safe argument separation or sanitization.
🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| $self->{output}->option_exit(short_msg => | ||
| 'Need to specify either --resource <name> with --resource-group option or --resource <id>.'); | ||
| } else { | ||
| foreach my $tmp_resource (@{$self->{az_resource}}) { |
There was a problem hiding this comment.
check_options iterates over $self->{az_resource} before assigning it from option_results, so validation can fail on uninitialized state before reaching the intended setup.
Details
✨ AI Reasoning
The validation path checks whether a resource was provided, then immediately iterates over an internal collection that is only populated later in the same routine. This makes the loop depend on state that is not initialized yet, so the guard logic can fail before reaching the assignment step.
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
|
|
||
| return if (defined($self->{option_results}->{command_options}) && $self->{option_results}->{command_options} ne ''); | ||
|
|
||
| my $cmd_options = "network vhub list --resource-group '$options{resource_group}' --only-show-errors --output json "; |
There was a problem hiding this comment.
azure_list_virtualhubs_set_cmd concatenates $options{resource_group} and $self->{subscription} into a shell command string; avoid direct interpolation into commands (escape or use safe argument passing).
Details
✨ AI Reasoning
The newly added function constructs a CLI command with the resource group and subscription values interpolated directly into the command string. If $options{resource_group} or $self->{subscription} are user-controlled, they could include shell metacharacters leading to command injection when executed. There is no escaping or use of safe argument passing in the constructed $cmd_options.
🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| defined($metric_results{$resource_name}->{$metric_label_name}->{lc($aggregation)}) ? | ||
| $metric_results{$resource_name}->{$metric_label_name}->{lc($aggregation)} : |
There was a problem hiding this comment.
manage_selection checks metric_results with $metric_name but reads the value with $metric_label_name, so it can miss existing data and report incorrect metric values.
| defined($metric_results{$resource_name}->{$metric_label_name}->{lc($aggregation)}) ? | |
| $metric_results{$resource_name}->{$metric_label_name}->{lc($aggregation)} : | |
| defined($metric_results{$resource_name}->{$metric_name}->{lc($aggregation)}) ? | |
| $metric_results{$resource_name}->{$metric_name}->{lc($aggregation)} : |
Details
✨ AI Reasoning
The selection logic normalizes metric names to one identifier for lookup, but later reads values from the results map using a different identifier format. This inconsistency means the retrieved metric value path does not match the stored path, leading to incorrect empty/default values.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Community contributors
Description
Modes Available:
hub-status
Virtual Hubs
hub-traffic
Metric documentation
I have implemented a process where the available metrics for the resource are first queried via the /providers/microsoft.insights/metricDefinitions endpoint.
for API
https://learn.microsoft.com/en-us/rest/api/monitor/metric-definitions/list?view=rest-monitor-2023-10-01&tabs=HTTP
for CLI
https://learn.microsoft.com/en-us/cli/azure/monitor/metrics?view=azure-cli-latest#az-monitor-metrics-list-definitions
This also allows you to determine the primaryAggregationType and use it if it has not been overridden with --aggregation.
Not all metrics are always available for all regions or features. That is why this is a good dynamic solution.
Type of change
How this pull request can be tested ?
hub-status.debug.txt
hub-traffic.debug.txt
Checklist
Summary by Aikido
🚀 New Features
⚡ Enhancements
More info