From 94d370ea9cdea46d5c910786a4978d247e9a3a83 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 00:42:06 +0000 Subject: [PATCH 01/13] Preflight check: detect Azure Policy blocking local authentication Add a local preflight check that detects Azure Policy assignments which deny resources with local authentication enabled (disableLocalAuth != true). The check: - Lists policy assignments on the target subscription via armpolicy SDK - Fetches policy definitions and inspects policyRule for disableLocalAuth field conditions with deny effect - Handles parameterized effects, policy sets (initiatives), and nested allOf/anyOf conditions - Cross-references affected resource types against the Bicep snapshot - Reports a warning (not error) when template resources would be blocked Also handles storage accounts which use allowSharedKeyAccess (inverted logic) instead of disableLocalAuth. Fixes #7177 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/.vscode/cspell.yaml | 8 + cli/azd/cmd/container.go | 1 + cli/azd/go.mod | 1 + cli/azd/go.sum | 2 + cli/azd/pkg/azapi/policy_service.go | 583 ++++++++++++++++++ cli/azd/pkg/azapi/policy_service_test.go | 400 ++++++++++++ .../provisioning/bicep/bicep_provider.go | 73 +++ 7 files changed, 1068 insertions(+) create mode 100644 cli/azd/pkg/azapi/policy_service.go create mode 100644 cli/azd/pkg/azapi/policy_service_test.go diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index bb200afb9c5..58ac9cdca11 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -357,6 +357,14 @@ overrides: - filename: "{pkg/azapi/permissions.go,pkg/infra/provisioning/bicep/bicep_provider.go}" words: - ABAC + - filename: pkg/azapi/policy_service.go + words: + - armpolicy + - disablelocalauth + - allowsharedkeyaccess + - localauthenabled + - policydefinitions + - policysetdefinitions - filename: extensions/azure.ai.models/internal/cmd/custom_create.go words: - Qwen diff --git a/cli/azd/cmd/container.go b/cli/azd/cmd/container.go index 95cd937c48e..53651c9d313 100644 --- a/cli/azd/cmd/container.go +++ b/cli/azd/cmd/container.go @@ -699,6 +699,7 @@ func registerCommonDependencies(container *ioc.NestedContainer) { // Tools container.MustRegisterSingleton(azapi.NewResourceService) container.MustRegisterSingleton(azapi.NewPermissionsService) + container.MustRegisterSingleton(azapi.NewPolicyService) container.MustRegisterSingleton(docker.NewCli) container.MustRegisterSingleton(dotnet.NewCli) container.MustRegisterSingleton(git.NewCli) diff --git a/cli/azd/go.mod b/cli/azd/go.mod index 6414505dd78..41d4b0e4047 100644 --- a/cli/azd/go.mod +++ b/cli/azd/go.mod @@ -91,6 +91,7 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.2.0 // indirect github.com/alecthomas/chroma/v2 v2.20.0 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect diff --git a/cli/azd/go.sum b/cli/azd/go.sum index 71c301de039..df5d966d10a 100644 --- a/cli/azd/go.sum +++ b/cli/azd/go.sum @@ -49,6 +49,8 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourceg github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0/go.mod h1:wVEOJfGTj0oPAUGA1JuRAvz/lxXQsWW16axmHPP47Bk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 h1:bcgO/crpp7wqI0Froi/I4C2fme7Vk/WLusbV399Do8I= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1/go.mod h1:kvfPmsE8gpOwwC1qrO1FeyBDDNfnwBN5UU3MPNiWW7I= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 h1:FCprRw2Uzske3FiFVGm6MqJY829zrAJLiN4coFueWis= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0/go.mod h1:koK4/Mf6lxFkYavGzZnzTUOEmY8ic9tN44UmWZsGfrk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0/go.mod h1:5kakwfW5CjC9KK+Q4wjXAg+ShuIm2mBMua0ZFj2C8PE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 h1:wxQx2Bt4xzPIKvW59WQf1tJNx/ZZKPfN+EhPX3Z6CYY= diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go new file mode 100644 index 00000000000..e6ead60263d --- /dev/null +++ b/cli/azd/pkg/azapi/policy_service.go @@ -0,0 +1,583 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azapi + +import ( + "context" + "encoding/json" + "fmt" + "log" + "maps" + "strings" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" + "github.com/azure/azure-dev/cli/azd/pkg/account" +) + +// LocalAuthDenyPolicy describes a deny policy that requires disableLocalAuth to be true +// for a specific Azure resource type. +type LocalAuthDenyPolicy struct { + // PolicyName is the display name of the policy assignment. + PolicyName string + // ResourceType is the Azure resource type targeted (e.g. "Microsoft.CognitiveServices/accounts"). + ResourceType string + // FieldPath is the full field path checked (e.g. "Microsoft.CognitiveServices/accounts/disableLocalAuth"). + FieldPath string +} + +// PolicyService queries Azure Policy assignments and definitions to detect +// policies that would block deployment of resources with local authentication enabled. +type PolicyService struct { + credentialProvider account.SubscriptionCredentialProvider + armClientOptions *arm.ClientOptions +} + +// NewPolicyService creates a new PolicyService. +func NewPolicyService( + credentialProvider account.SubscriptionCredentialProvider, + armClientOptions *arm.ClientOptions, +) *PolicyService { + return &PolicyService{ + credentialProvider: credentialProvider, + armClientOptions: armClientOptions, + } +} + +// FindLocalAuthDenyPolicies lists policy assignments on the subscription and inspects +// their definitions for deny-effect rules that require disableLocalAuth to be true. +// It returns a list of matching policies with their target resource types. +func (s *PolicyService) FindLocalAuthDenyPolicies( + ctx context.Context, + subscriptionId string, +) ([]LocalAuthDenyPolicy, error) { + credential, err := s.credentialProvider.CredentialForSubscription(ctx, subscriptionId) + if err != nil { + return nil, fmt.Errorf("getting credential for subscription %s: %w", subscriptionId, err) + } + + assignmentsClient, err := armpolicy.NewAssignmentsClient(subscriptionId, credential, s.armClientOptions) + if err != nil { + return nil, fmt.Errorf("creating policy assignments client: %w", err) + } + + definitionsClient, err := armpolicy.NewDefinitionsClient(subscriptionId, credential, s.armClientOptions) + if err != nil { + return nil, fmt.Errorf("creating policy definitions client: %w", err) + } + + setDefinitionsClient, err := armpolicy.NewSetDefinitionsClient(subscriptionId, credential, s.armClientOptions) + if err != nil { + return nil, fmt.Errorf("creating policy set definitions client: %w", err) + } + + // List all policy assignments for the subscription. + var assignments []*armpolicy.Assignment + pager := assignmentsClient.NewListPager(nil) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return nil, fmt.Errorf("listing policy assignments: %w", err) + } + assignments = append(assignments, page.Value...) + } + + var results []LocalAuthDenyPolicy + + for _, assignment := range assignments { + if assignment.Properties == nil || assignment.Properties.PolicyDefinitionID == nil { + continue + } + + defID := *assignment.Properties.PolicyDefinitionID + assignmentName := "" + if assignment.Properties.DisplayName != nil { + assignmentName = *assignment.Properties.DisplayName + } + + // Resolve the assignment's parameter values so we can evaluate parameterized effects. + assignmentParams := extractAssignmentParams(assignment) + + if isBuiltInPolicyDefinition(defID) || isCustomPolicyDefinition(defID) { + // Single policy definition — check it directly. + policies := s.checkPolicyDefinition( + ctx, definitionsClient, defID, assignmentName, assignmentParams, + ) + results = append(results, policies...) + } else if isPolicySetDefinition(defID) { + // Policy set (initiative) — enumerate its member definitions. + policies := s.checkPolicySetDefinition( + ctx, setDefinitionsClient, definitionsClient, defID, assignmentName, assignmentParams, + ) + results = append(results, policies...) + } + } + + return results, nil +} + +// checkPolicyDefinition fetches a single policy definition and inspects it for +// disableLocalAuth deny rules. Returns any matching policies found. +func (s *PolicyService) checkPolicyDefinition( + ctx context.Context, + client *armpolicy.DefinitionsClient, + definitionID string, + assignmentName string, + assignmentParams map[string]any, +) []LocalAuthDenyPolicy { + definition, err := getPolicyDefinitionByID(ctx, client, definitionID) + if err != nil { + log.Printf("policy preflight: could not fetch policy definition %s: %v", definitionID, err) + return nil + } + + return extractLocalAuthDenyPolicies(definition, assignmentName, assignmentParams) +} + +// checkPolicySetDefinition fetches a policy set definition (initiative) and checks +// each of its member policy definitions for disableLocalAuth deny rules. +func (s *PolicyService) checkPolicySetDefinition( + ctx context.Context, + setClient *armpolicy.SetDefinitionsClient, + defClient *armpolicy.DefinitionsClient, + setDefinitionID string, + assignmentName string, + assignmentParams map[string]any, +) []LocalAuthDenyPolicy { + setDef, err := getPolicySetDefinitionByID(ctx, setClient, setDefinitionID) + if err != nil { + log.Printf("policy preflight: could not fetch policy set definition %s: %v", setDefinitionID, err) + return nil + } + + if setDef.Properties == nil || setDef.Properties.PolicyDefinitions == nil { + return nil + } + + var results []LocalAuthDenyPolicy + for _, member := range setDef.Properties.PolicyDefinitions { + if member.PolicyDefinitionID == nil { + continue + } + + // Merge set-level parameters with member-level parameter values. + memberParams := mergeParams(assignmentParams, member.Parameters) + + policies := s.checkPolicyDefinition( + ctx, defClient, *member.PolicyDefinitionID, assignmentName, memberParams, + ) + results = append(results, policies...) + } + + return results +} + +// getPolicyDefinitionByID fetches a policy definition by its full resource ID. +// It handles both built-in (/providers/Microsoft.Authorization/...) and subscription-scoped definitions. +func getPolicyDefinitionByID( + ctx context.Context, + client *armpolicy.DefinitionsClient, + definitionID string, +) (*armpolicy.Definition, error) { + name := lastSegment(definitionID) + if name == "" { + return nil, fmt.Errorf("invalid policy definition ID: %s", definitionID) + } + + if isBuiltInPolicyDefinition(definitionID) { + resp, err := client.GetBuiltIn(ctx, name, nil) + if err != nil { + return nil, err + } + return &resp.Definition, nil + } + + resp, err := client.Get(ctx, name, nil) + if err != nil { + return nil, err + } + return &resp.Definition, nil +} + +// getPolicySetDefinitionByID fetches a policy set definition by its full resource ID. +func getPolicySetDefinitionByID( + ctx context.Context, + client *armpolicy.SetDefinitionsClient, + setDefinitionID string, +) (*armpolicy.SetDefinition, error) { + name := lastSegment(setDefinitionID) + if name == "" { + return nil, fmt.Errorf("invalid policy set definition ID: %s", setDefinitionID) + } + + if isBuiltInPolicySetDefinition(setDefinitionID) { + resp, err := client.GetBuiltIn(ctx, name, nil) + if err != nil { + return nil, err + } + return &resp.SetDefinition, nil + } + + resp, err := client.Get(ctx, name, nil) + if err != nil { + return nil, err + } + return &resp.SetDefinition, nil +} + +// extractLocalAuthDenyPolicies inspects a policy definition for deny-effect rules +// that target disableLocalAuth fields. It returns any matching policies. +func extractLocalAuthDenyPolicies( + def *armpolicy.Definition, + assignmentName string, + assignmentParams map[string]any, +) []LocalAuthDenyPolicy { + if def.Properties == nil || def.Properties.PolicyRule == nil { + return nil + } + + ruleMap, ok := def.Properties.PolicyRule.(map[string]any) + if !ok { + return nil + } + + // Check if the effect is "deny" (either literal or via parameter reference). + if !isDenyEffect(ruleMap, def.Properties.Parameters, assignmentParams) { + return nil + } + + // Parse the "if" condition to find disableLocalAuth field references. + ifBlock, ok := ruleMap["if"] + if !ok { + return nil + } + + return findLocalAuthConditions(ifBlock, assignmentName) +} + +// isDenyEffect checks whether the policy's effect resolves to "deny". +// Effects can be a literal string or a parameter reference like "[parameters('effect')]". +func isDenyEffect( + ruleMap map[string]any, + definitionParams map[string]*armpolicy.ParameterDefinitionsValue, + assignmentParams map[string]any, +) bool { + thenBlock, ok := ruleMap["then"] + if !ok { + return false + } + + thenMap, ok := thenBlock.(map[string]any) + if !ok { + return false + } + + effectVal, ok := thenMap["effect"] + if !ok { + return false + } + + effectStr, ok := effectVal.(string) + if !ok { + return false + } + + // Check for literal deny. + if strings.EqualFold(effectStr, "deny") { + return true + } + + // Check for parameter reference: "[parameters('effect')]" or "[parameters('effectName')]". + paramName := extractParameterReference(effectStr) + if paramName == "" { + return false + } + + // First check assignment-level parameters (these override definition defaults). + if v, ok := assignmentParams[paramName]; ok { + if s, ok := v.(string); ok && strings.EqualFold(s, "deny") { + return true + } + return false + } + + // Fall back to the definition's default value. + if definitionParams != nil { + if paramDef, ok := definitionParams[paramName]; ok && paramDef.DefaultValue != nil { + if s, ok := paramDef.DefaultValue.(string); ok && strings.EqualFold(s, "deny") { + return true + } + } + } + + return false +} + +// findLocalAuthConditions traverses the policy rule's "if" block to find conditions +// that reference disableLocalAuth fields and extracts the target resource type. +func findLocalAuthConditions(ifBlock any, assignmentName string) []LocalAuthDenyPolicy { + condMap, ok := ifBlock.(map[string]any) + if !ok { + return nil + } + + // Check for allOf / anyOf compound conditions. + if allOf, ok := condMap["allOf"]; ok { + return findInCompoundCondition(allOf, assignmentName) + } + if anyOf, ok := condMap["anyOf"]; ok { + return findInCompoundCondition(anyOf, assignmentName) + } + + // Single condition — unlikely to be the full pattern but check anyway. + return checkSingleCondition(condMap, assignmentName, "") +} + +// findInCompoundCondition processes an allOf/anyOf array looking for conditions that +// reference both a resource type and a disableLocalAuth field. +func findInCompoundCondition(compound any, assignmentName string) []LocalAuthDenyPolicy { + conditions, ok := compound.([]any) + if !ok { + return nil + } + + // First pass: find the resource type from "field: type, equals: ..." conditions. + resourceType := "" + for _, cond := range conditions { + condMap, ok := cond.(map[string]any) + if !ok { + continue + } + + // Handle nested allOf/anyOf. + if allOf, ok := condMap["allOf"]; ok { + if results := findInCompoundCondition(allOf, assignmentName); len(results) > 0 { + return results + } + } + if anyOf, ok := condMap["anyOf"]; ok { + if results := findInCompoundCondition(anyOf, assignmentName); len(results) > 0 { + return results + } + } + + fieldVal, _ := condMap["field"].(string) + if strings.EqualFold(fieldVal, "type") { + if eq, ok := condMap["equals"].(string); ok { + resourceType = eq + } + if in, ok := condMap["in"].([]any); ok && len(in) > 0 { + // Multiple resource types — take the first one for now. + if s, ok := in[0].(string); ok { + resourceType = s + } + } + } + } + + // Second pass: find disableLocalAuth field references. + var results []LocalAuthDenyPolicy + for _, cond := range conditions { + condMap, ok := cond.(map[string]any) + if !ok { + continue + } + results = append(results, checkSingleCondition(condMap, assignmentName, resourceType)...) + + // Also recurse into nested conditions. + if allOf, ok := condMap["allOf"]; ok { + results = append(results, findInCompoundCondition(allOf, assignmentName)...) + } + if anyOf, ok := condMap["anyOf"]; ok { + results = append(results, findInCompoundCondition(anyOf, assignmentName)...) + } + } + + return results +} + +// checkSingleCondition checks if a single condition references a disableLocalAuth field. +func checkSingleCondition( + condMap map[string]any, + assignmentName string, + resourceType string, +) []LocalAuthDenyPolicy { + fieldVal, ok := condMap["field"].(string) + if !ok { + return nil + } + + if !isLocalAuthField(fieldVal) { + return nil + } + + // If we don't have the resource type from a sibling condition, try to derive it + // from the field path (e.g. "Microsoft.Storage/storageAccounts/allowSharedKeyAccess"). + if resourceType == "" { + resourceType = resourceTypeFromFieldPath(fieldVal) + } + + if resourceType == "" { + return nil + } + + return []LocalAuthDenyPolicy{{ + PolicyName: assignmentName, + ResourceType: resourceType, + FieldPath: fieldVal, + }} +} + +// isLocalAuthField returns true if the field path references a local authentication property. +func isLocalAuthField(field string) bool { + lower := strings.ToLower(field) + return strings.HasSuffix(lower, "/disablelocalauth") || + strings.HasSuffix(lower, "/allowsharedkeyaccess") || + strings.HasSuffix(lower, "/localauthenabled") || + strings.EqualFold(field, "disableLocalAuth") || + strings.EqualFold(field, "allowSharedKeyAccess") +} + +// resourceTypeFromFieldPath extracts the resource type from a fully qualified field path. +// For example, "Microsoft.CognitiveServices/accounts/disableLocalAuth" returns +// "Microsoft.CognitiveServices/accounts". +func resourceTypeFromFieldPath(field string) string { + idx := strings.LastIndex(field, "/") + if idx <= 0 { + return "" + } + candidate := field[:idx] + // Basic validation: resource types contain at least one "/". + if !strings.Contains(candidate, "/") { + return "" + } + return candidate +} + +// extractParameterReference extracts the parameter name from an ARM template parameter +// reference expression like "[parameters('effect')]". Returns empty if not a parameter reference. +func extractParameterReference(expr string) string { + lower := strings.ToLower(strings.TrimSpace(expr)) + if !strings.HasPrefix(lower, "[parameters('") || !strings.HasSuffix(lower, "')]") { + return "" + } + // Extract between [parameters(' and ')] + inner := expr[len("[parameters('"):] + before, _, ok := strings.Cut(inner, "')]") + if !ok { + return "" + } + return before +} + +// extractAssignmentParams extracts parameter values from a policy assignment into a simple map. +func extractAssignmentParams(assignment *armpolicy.Assignment) map[string]any { + if assignment.Properties == nil || assignment.Properties.Parameters == nil { + return nil + } + + params := make(map[string]any, len(assignment.Properties.Parameters)) + for name, val := range assignment.Properties.Parameters { + if val != nil && val.Value != nil { + params[name] = val.Value + } + } + return params +} + +// mergeParams merges assignment-level parameters with member-level parameter values +// from a policy set definition reference. Member parameters may contain parameter +// references like "[parameters('effect')]" that resolve against the assignment parameters. +func mergeParams(assignmentParams map[string]any, memberParams map[string]*armpolicy.ParameterValuesValue) map[string]any { + if len(memberParams) == 0 { + return assignmentParams + } + + merged := make(map[string]any, len(assignmentParams)+len(memberParams)) + maps.Copy(merged, assignmentParams) + + for name, val := range memberParams { + if val == nil || val.Value == nil { + continue + } + + // Check if the member parameter value is itself a reference to an assignment parameter. + if s, ok := val.Value.(string); ok { + if refName := extractParameterReference(s); refName != "" { + if resolved, ok := assignmentParams[refName]; ok { + merged[name] = resolved + continue + } + } + } + + merged[name] = val.Value + } + + return merged +} + +// isBuiltInPolicyDefinition returns true if the definition ID is a built-in policy. +func isBuiltInPolicyDefinition(id string) bool { + return strings.HasPrefix(strings.ToLower(id), "/providers/microsoft.authorization/policydefinitions/") +} + +// isCustomPolicyDefinition returns true if the definition ID is a subscription-scoped custom policy. +func isCustomPolicyDefinition(id string) bool { + lower := strings.ToLower(id) + return strings.Contains(lower, "/providers/microsoft.authorization/policydefinitions/") && + !isBuiltInPolicyDefinition(id) +} + +// isPolicySetDefinition returns true if the definition ID references a policy set (initiative). +func isPolicySetDefinition(id string) bool { + return strings.Contains(strings.ToLower(id), "/providers/microsoft.authorization/policysetdefinitions/") +} + +// isBuiltInPolicySetDefinition returns true if the set definition ID is a built-in policy set. +func isBuiltInPolicySetDefinition(id string) bool { + return strings.HasPrefix(strings.ToLower(id), "/providers/microsoft.authorization/policysetdefinitions/") +} + +// lastSegment returns the last path segment of a resource ID. +func lastSegment(resourceID string) string { + parts := strings.Split(resourceID, "/") + if len(parts) == 0 { + return "" + } + return parts[len(parts)-1] +} + +// ResourceHasLocalAuthDisabled checks whether a resource's properties JSON has +// the disableLocalAuth property set to true (or allowSharedKeyAccess set to false +// for storage accounts). +func ResourceHasLocalAuthDisabled(resourceType string, properties json.RawMessage) bool { + if len(properties) == 0 { + return false + } + + var props map[string]any + if err := json.Unmarshal(properties, &props); err != nil { + return false + } + + // Storage accounts use allowSharedKeyAccess (inverted logic). + if strings.EqualFold(resourceType, "Microsoft.Storage/storageAccounts") { + if v, ok := props["allowSharedKeyAccess"]; ok { + if b, ok := v.(bool); ok { + return !b // allowSharedKeyAccess=false means local auth is disabled + } + } + return false + } + + // Most resource types use disableLocalAuth. + if v, ok := props["disableLocalAuth"]; ok { + if b, ok := v.(bool); ok { + return b + } + } + + return false +} diff --git a/cli/azd/pkg/azapi/policy_service_test.go b/cli/azd/pkg/azapi/policy_service_test.go new file mode 100644 index 00000000000..6cd8257008c --- /dev/null +++ b/cli/azd/pkg/azapi/policy_service_test.go @@ -0,0 +1,400 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azapi + +import ( + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" + "github.com/stretchr/testify/require" +) + +func TestExtractLocalAuthDenyPolicies_DenyLiteral(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.CognitiveServices/accounts", + }, + map[string]any{ + "field": "Microsoft.CognitiveServices/accounts/disableLocalAuth", + "notEquals": true, + }, + }, + }, + "then": map[string]any{ + "effect": "deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + require.Len(t, results, 1) + require.Equal(t, "Microsoft.CognitiveServices/accounts", results[0].ResourceType) + require.Equal(t, "test-policy", results[0].PolicyName) + require.Contains(t, results[0].FieldPath, "disableLocalAuth") +} + +func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Deny(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + "effect": { + DefaultValue: "Audit", + }, + }, + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.EventHub/namespaces", + }, + map[string]any{ + "field": "Microsoft.EventHub/namespaces/disableLocalAuth", + "notEquals": true, + }, + }, + }, + "then": map[string]any{ + "effect": "[parameters('effect')]", + }, + }, + }, + } + + // With assignment params overriding to Deny. + assignmentParams := map[string]any{"effect": "Deny"} + results := extractLocalAuthDenyPolicies(def, "test-policy", assignmentParams) + require.Len(t, results, 1) + require.Equal(t, "Microsoft.EventHub/namespaces", results[0].ResourceType) +} + +func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Audit(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + "effect": { + DefaultValue: "Audit", + }, + }, + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.EventHub/namespaces", + }, + map[string]any{ + "field": "Microsoft.EventHub/namespaces/disableLocalAuth", + "notEquals": true, + }, + }, + }, + "then": map[string]any{ + "effect": "[parameters('effect')]", + }, + }, + }, + } + + // No assignment params — falls back to default "Audit", which is not deny. + results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + require.Empty(t, results) +} + +func TestExtractLocalAuthDenyPolicies_NoLocalAuthField(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.Storage/storageAccounts", + }, + map[string]any{ + "field": "Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", + "equals": false, + }, + }, + }, + "then": map[string]any{ + "effect": "deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + require.Empty(t, results) +} + +func TestExtractLocalAuthDenyPolicies_StorageAllowSharedKeyAccess(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.Storage/storageAccounts", + }, + map[string]any{ + "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", + "notEquals": false, + }, + }, + }, + "then": map[string]any{ + "effect": "deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + require.Len(t, results, 1) + require.Equal(t, "Microsoft.Storage/storageAccounts", results[0].ResourceType) +} + +func TestExtractLocalAuthDenyPolicies_NestedAnyOf(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.ServiceBus/namespaces", + }, + map[string]any{ + "anyOf": []any{ + map[string]any{ + "field": "Microsoft.ServiceBus/namespaces/disableLocalAuth", + "equals": false, + }, + map[string]any{ + "field": "Microsoft.ServiceBus/namespaces/disableLocalAuth", + "exists": false, + }, + }, + }, + }, + }, + "then": map[string]any{ + "effect": "Deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "test-nested", nil) + require.NotEmpty(t, results) + require.Equal(t, "Microsoft.ServiceBus/namespaces", results[0].ResourceType) +} + +func TestIsLocalAuthField(t *testing.T) { + t.Parallel() + + tests := []struct { + field string + want bool + }{ + {"Microsoft.CognitiveServices/accounts/disableLocalAuth", true}, + {"Microsoft.EventHub/namespaces/disableLocalAuth", true}, + {"Microsoft.Storage/storageAccounts/allowSharedKeyAccess", true}, + {"Microsoft.ServiceBus/namespaces/disableLocalAuth", true}, + {"disableLocalAuth", true}, + {"Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", false}, + {"type", false}, + {"location", false}, + } + + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, isLocalAuthField(tt.field)) + }) + } +} + +func TestResourceTypeFromFieldPath(t *testing.T) { + t.Parallel() + + tests := []struct { + field string + want string + }{ + {"Microsoft.CognitiveServices/accounts/disableLocalAuth", "Microsoft.CognitiveServices/accounts"}, + {"Microsoft.EventHub/namespaces/disableLocalAuth", "Microsoft.EventHub/namespaces"}, + {"disableLocalAuth", ""}, + {"", ""}, + } + + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, resourceTypeFromFieldPath(tt.field)) + }) + } +} + +func TestExtractParameterReference(t *testing.T) { + t.Parallel() + + tests := []struct { + expr string + want string + }{ + {"[parameters('effect')]", "effect"}, + {"[parameters('myEffect')]", "myEffect"}, + {"deny", ""}, + {"[concat('a', 'b')]", ""}, + {"", ""}, + } + + for _, tt := range tests { + t.Run(tt.expr, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, extractParameterReference(tt.expr)) + }) + } +} + +func TestResourceHasLocalAuthDisabled(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + resourceType string + properties string + want bool + }{ + { + "CognitiveServices disabled", + "Microsoft.CognitiveServices/accounts", + `{"disableLocalAuth": true}`, + true, + }, + { + "CognitiveServices enabled", + "Microsoft.CognitiveServices/accounts", + `{"disableLocalAuth": false}`, + false, + }, + { + "CognitiveServices missing property", + "Microsoft.CognitiveServices/accounts", + `{}`, + false, + }, + { + "Storage allowSharedKeyAccess false", + "Microsoft.Storage/storageAccounts", + `{"allowSharedKeyAccess": false}`, + true, + }, + { + "Storage allowSharedKeyAccess true", + "Microsoft.Storage/storageAccounts", + `{"allowSharedKeyAccess": true}`, + false, + }, + { + "Empty properties", + "Microsoft.EventHub/namespaces", + ``, + false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, ResourceHasLocalAuthDisabled( + tt.resourceType, []byte(tt.properties), + )) + }) + } +} + +func TestIsDenyEffect(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ruleMap map[string]any + definitionParams map[string]*armpolicy.ParameterDefinitionsValue + assignmentParams map[string]any + want bool + }{ + { + "literal deny", + map[string]any{"then": map[string]any{"effect": "deny"}}, + nil, nil, true, + }, + { + "literal Deny (capitalized)", + map[string]any{"then": map[string]any{"effect": "Deny"}}, + nil, nil, true, + }, + { + "literal audit", + map[string]any{"then": map[string]any{"effect": "audit"}}, + nil, nil, false, + }, + { + "parameterized deny via assignment", + map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + nil, + map[string]any{"effect": "Deny"}, + true, + }, + { + "parameterized deny via default", + map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + map[string]*armpolicy.ParameterDefinitionsValue{ + "effect": {DefaultValue: "Deny"}, + }, + nil, true, + }, + { + "parameterized audit via default", + map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + map[string]*armpolicy.ParameterDefinitionsValue{ + "effect": {DefaultValue: "Audit"}, + }, + nil, false, + }, + { + "no then block", + map[string]any{}, + nil, nil, false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, isDenyEffect(tt.ruleMap, tt.definitionParams, tt.assignmentParams)) + }) + } +} diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 12079d36cbe..0ab3549972a 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2179,6 +2179,11 @@ func (p *BicepProvider) validatePreflight( Fn: p.checkRoleAssignmentPermissions, }) + // Register the local auth policy check. This detects Azure Policy assignments + // that deny resources with local authentication enabled (disableLocalAuth != true) + // and warns if the template contains affected resources. + localPreflight.AddCheck(p.checkLocalAuthPolicy) + results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) if err != nil { p.setPreflightOutcome(span, preflightOutcomeError, nil) @@ -2399,6 +2404,74 @@ func (p *BicepProvider) resolveResourceTenantPrincipalId( return principalId, nil } +// checkLocalAuthPolicy is a PreflightCheckFn that detects Azure Policy assignments on the +// subscription that deny resources with local authentication enabled. If any template resources +// have disableLocalAuth unset or false while a deny policy exists for that resource type, +// a warning is returned so the user knows the deployment will be blocked. +func (p *BicepProvider) checkLocalAuthPolicy( + ctx context.Context, valCtx *validationContext, +) (*PreflightCheckResult, error) { + if len(valCtx.SnapshotResources) == 0 { + return nil, nil + } + + var policyService *azapi.PolicyService + if err := p.serviceLocator.Resolve(&policyService); err != nil { + log.Printf("could not resolve PolicyService, skipping local auth policy check: %v", err) + return nil, nil + } + + subscriptionId := p.env.GetSubscriptionId() + + denyPolicies, err := policyService.FindLocalAuthDenyPolicies(ctx, subscriptionId) + if err != nil { + log.Printf("error checking local auth policies, skipping check: %v", err) + return nil, nil + } + + if len(denyPolicies) == 0 { + return nil, nil + } + + // Build a set of resource types that have deny policies. + policyByType := make(map[string]azapi.LocalAuthDenyPolicy) + for _, policy := range denyPolicies { + policyByType[strings.ToLower(policy.ResourceType)] = policy + } + + // Check each snapshot resource against the deny policies. + var violations []string + for _, resource := range valCtx.SnapshotResources { + policy, found := policyByType[strings.ToLower(resource.Type)] + if !found { + continue + } + + if !azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) { + violations = append(violations, fmt.Sprintf( + " - %s (type: %s) — policy: %q", + resource.Name, resource.Type, policy.PolicyName, + )) + } + } + + if len(violations) == 0 { + return nil, nil + } + + return &PreflightCheckResult{ + Severity: PreflightCheckWarning, + Message: fmt.Sprintf( + "subscription %s has Azure Policy assignments that deny resources with local "+ + "authentication enabled. The following resources may be blocked:\n%s\n"+ + "Set 'disableLocalAuth: true' on these resources or request a policy exemption. "+ + "See https://aka.ms/safesecretsstandard for details.", + subscriptionId, + strings.Join(violations, "\n"), + ), + }, nil +} + // Deploys the specified Bicep module and parameters with the selected provisioning scope (subscription vs resource group) func (p *BicepProvider) deployModule( ctx context.Context, From 4de6c45d2aa25826e0d08fd9a6cdff75f9d778ad Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 04:36:55 +0000 Subject: [PATCH 02/13] Address PR review feedback - Fix extractParameterReference whitespace bug: use trimmed string for both prefix check and inner extraction - Remove localAuthEnabled from isLocalAuthField to avoid false positives (ResourceHasLocalAuthDisabled doesn't handle it) - Handle multiple resource types in policy 'in' array (was only taking first entry) - Thread resolved resourceType through recursive findInCompoundCondition calls so nested conditions inherit parent's resource type - Use []LocalAuthDenyPolicy per resource type to avoid overwriting when multiple deny policies target the same type - Add in-memory cache for policy definition/set definition fetches to avoid duplicate API calls across assignments - Improve warning message to mention both disableLocalAuth and allowSharedKeyAccess for storage accounts - Add tests for whitespace parameter extraction, multi-type 'in' arrays, and nested condition resource type inheritance - Run go mod tidy to move armpolicy to direct dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/go.mod | 2 +- cli/azd/pkg/azapi/policy_service.go | 131 +++++++++++------- cli/azd/pkg/azapi/policy_service_test.go | 77 ++++++++++ .../provisioning/bicep/bicep_provider.go | 26 +++- 4 files changed, 179 insertions(+), 57 deletions(-) diff --git a/cli/azd/go.mod b/cli/azd/go.mod index 41d4b0e4047..be20bfe4249 100644 --- a/cli/azd/go.mod +++ b/cli/azd/go.mod @@ -22,6 +22,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/operationalinsights/armoperationalinsights/v2 v2.0.2 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql/v2 v2.0.0-beta.7 @@ -91,7 +92,6 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.2.0 // indirect github.com/alecthomas/chroma/v2 v2.20.0 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index e6ead60263d..7c41aeee4af 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -85,6 +85,11 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( var results []LocalAuthDenyPolicy + // Cache fetched definitions/set definitions to avoid duplicate API calls when + // multiple assignments reference the same definition. + defCache := make(map[string]*armpolicy.Definition) + setDefCache := make(map[string]*armpolicy.SetDefinition) + for _, assignment := range assignments { if assignment.Properties == nil || assignment.Properties.PolicyDefinitionID == nil { continue @@ -102,13 +107,14 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( if isBuiltInPolicyDefinition(defID) || isCustomPolicyDefinition(defID) { // Single policy definition — check it directly. policies := s.checkPolicyDefinition( - ctx, definitionsClient, defID, assignmentName, assignmentParams, + ctx, definitionsClient, defID, assignmentName, assignmentParams, defCache, ) results = append(results, policies...) } else if isPolicySetDefinition(defID) { // Policy set (initiative) — enumerate its member definitions. policies := s.checkPolicySetDefinition( ctx, setDefinitionsClient, definitionsClient, defID, assignmentName, assignmentParams, + defCache, setDefCache, ) results = append(results, policies...) } @@ -117,7 +123,7 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( return results, nil } -// checkPolicyDefinition fetches a single policy definition and inspects it for +// checkPolicyDefinition fetches a single policy definition (using cache) and inspects it for // disableLocalAuth deny rules. Returns any matching policies found. func (s *PolicyService) checkPolicyDefinition( ctx context.Context, @@ -125,10 +131,20 @@ func (s *PolicyService) checkPolicyDefinition( definitionID string, assignmentName string, assignmentParams map[string]any, + cache map[string]*armpolicy.Definition, ) []LocalAuthDenyPolicy { - definition, err := getPolicyDefinitionByID(ctx, client, definitionID) - if err != nil { - log.Printf("policy preflight: could not fetch policy definition %s: %v", definitionID, err) + definition, ok := cache[definitionID] + if !ok { + var err error + definition, err = getPolicyDefinitionByID(ctx, client, definitionID) + if err != nil { + log.Printf("policy preflight: could not fetch policy definition %s: %v", definitionID, err) + cache[definitionID] = nil + return nil + } + cache[definitionID] = definition + } + if definition == nil { return nil } @@ -144,10 +160,22 @@ func (s *PolicyService) checkPolicySetDefinition( setDefinitionID string, assignmentName string, assignmentParams map[string]any, + defCache map[string]*armpolicy.Definition, + setDefCache map[string]*armpolicy.SetDefinition, ) []LocalAuthDenyPolicy { - setDef, err := getPolicySetDefinitionByID(ctx, setClient, setDefinitionID) - if err != nil { - log.Printf("policy preflight: could not fetch policy set definition %s: %v", setDefinitionID, err) + setDef, ok := setDefCache[setDefinitionID] + if !ok { + var err error + setDef, err = getPolicySetDefinitionByID(ctx, setClient, setDefinitionID) + if err != nil { + log.Printf( + "policy preflight: could not fetch policy set definition %s: %v", setDefinitionID, err) + setDefCache[setDefinitionID] = nil + return nil + } + setDefCache[setDefinitionID] = setDef + } + if setDef == nil { return nil } @@ -165,7 +193,7 @@ func (s *PolicyService) checkPolicySetDefinition( memberParams := mergeParams(assignmentParams, member.Parameters) policies := s.checkPolicyDefinition( - ctx, defClient, *member.PolicyDefinitionID, assignmentName, memberParams, + ctx, defClient, *member.PolicyDefinitionID, assignmentName, memberParams, defCache, ) results = append(results, policies...) } @@ -324,58 +352,55 @@ func findLocalAuthConditions(ifBlock any, assignmentName string) []LocalAuthDeny // Check for allOf / anyOf compound conditions. if allOf, ok := condMap["allOf"]; ok { - return findInCompoundCondition(allOf, assignmentName) + return findInCompoundCondition(allOf, assignmentName, nil) } if anyOf, ok := condMap["anyOf"]; ok { - return findInCompoundCondition(anyOf, assignmentName) + return findInCompoundCondition(anyOf, assignmentName, nil) } // Single condition — unlikely to be the full pattern but check anyway. - return checkSingleCondition(condMap, assignmentName, "") + return checkSingleCondition(condMap, assignmentName, nil) } // findInCompoundCondition processes an allOf/anyOf array looking for conditions that // reference both a resource type and a disableLocalAuth field. -func findInCompoundCondition(compound any, assignmentName string) []LocalAuthDenyPolicy { +// parentResourceTypes carries any resource types resolved by an ancestor compound condition. +func findInCompoundCondition( + compound any, assignmentName string, parentResourceTypes []string, +) []LocalAuthDenyPolicy { conditions, ok := compound.([]any) if !ok { return nil } - // First pass: find the resource type from "field: type, equals: ..." conditions. - resourceType := "" + // First pass: find resource types from "field: type, equals/in: ..." conditions. + var resourceTypes []string for _, cond := range conditions { condMap, ok := cond.(map[string]any) if !ok { continue } - // Handle nested allOf/anyOf. - if allOf, ok := condMap["allOf"]; ok { - if results := findInCompoundCondition(allOf, assignmentName); len(results) > 0 { - return results - } - } - if anyOf, ok := condMap["anyOf"]; ok { - if results := findInCompoundCondition(anyOf, assignmentName); len(results) > 0 { - return results - } - } - fieldVal, _ := condMap["field"].(string) if strings.EqualFold(fieldVal, "type") { if eq, ok := condMap["equals"].(string); ok { - resourceType = eq + resourceTypes = append(resourceTypes, eq) } - if in, ok := condMap["in"].([]any); ok && len(in) > 0 { - // Multiple resource types — take the first one for now. - if s, ok := in[0].(string); ok { - resourceType = s + if in, ok := condMap["in"].([]any); ok { + for _, item := range in { + if s, ok := item.(string); ok { + resourceTypes = append(resourceTypes, s) + } } } } } + // Merge with parent resource types if this level didn't find any. + if len(resourceTypes) == 0 { + resourceTypes = parentResourceTypes + } + // Second pass: find disableLocalAuth field references. var results []LocalAuthDenyPolicy for _, cond := range conditions { @@ -383,14 +408,14 @@ func findInCompoundCondition(compound any, assignmentName string) []LocalAuthDen if !ok { continue } - results = append(results, checkSingleCondition(condMap, assignmentName, resourceType)...) + results = append(results, checkSingleCondition(condMap, assignmentName, resourceTypes)...) - // Also recurse into nested conditions. + // Recurse into nested conditions, passing resolved resource types. if allOf, ok := condMap["allOf"]; ok { - results = append(results, findInCompoundCondition(allOf, assignmentName)...) + results = append(results, findInCompoundCondition(allOf, assignmentName, resourceTypes)...) } if anyOf, ok := condMap["anyOf"]; ok { - results = append(results, findInCompoundCondition(anyOf, assignmentName)...) + results = append(results, findInCompoundCondition(anyOf, assignmentName, resourceTypes)...) } } @@ -398,10 +423,11 @@ func findInCompoundCondition(compound any, assignmentName string) []LocalAuthDen } // checkSingleCondition checks if a single condition references a disableLocalAuth field. +// resourceTypes are the candidate resource types resolved from sibling conditions. func checkSingleCondition( condMap map[string]any, assignmentName string, - resourceType string, + resourceTypes []string, ) []LocalAuthDenyPolicy { fieldVal, ok := condMap["field"].(string) if !ok { @@ -412,21 +438,28 @@ func checkSingleCondition( return nil } - // If we don't have the resource type from a sibling condition, try to derive it + // If we don't have resource types from a sibling condition, try to derive one // from the field path (e.g. "Microsoft.Storage/storageAccounts/allowSharedKeyAccess"). - if resourceType == "" { - resourceType = resourceTypeFromFieldPath(fieldVal) + if len(resourceTypes) == 0 { + if rt := resourceTypeFromFieldPath(fieldVal); rt != "" { + resourceTypes = []string{rt} + } } - if resourceType == "" { + if len(resourceTypes) == 0 { return nil } - return []LocalAuthDenyPolicy{{ - PolicyName: assignmentName, - ResourceType: resourceType, - FieldPath: fieldVal, - }} + // Emit one result per resource type. + results := make([]LocalAuthDenyPolicy, 0, len(resourceTypes)) + for _, rt := range resourceTypes { + results = append(results, LocalAuthDenyPolicy{ + PolicyName: assignmentName, + ResourceType: rt, + FieldPath: fieldVal, + }) + } + return results } // isLocalAuthField returns true if the field path references a local authentication property. @@ -434,7 +467,6 @@ func isLocalAuthField(field string) bool { lower := strings.ToLower(field) return strings.HasSuffix(lower, "/disablelocalauth") || strings.HasSuffix(lower, "/allowsharedkeyaccess") || - strings.HasSuffix(lower, "/localauthenabled") || strings.EqualFold(field, "disableLocalAuth") || strings.EqualFold(field, "allowSharedKeyAccess") } @@ -458,12 +490,13 @@ func resourceTypeFromFieldPath(field string) string { // extractParameterReference extracts the parameter name from an ARM template parameter // reference expression like "[parameters('effect')]". Returns empty if not a parameter reference. func extractParameterReference(expr string) string { - lower := strings.ToLower(strings.TrimSpace(expr)) + trimmed := strings.TrimSpace(expr) + lower := strings.ToLower(trimmed) if !strings.HasPrefix(lower, "[parameters('") || !strings.HasSuffix(lower, "')]") { return "" } // Extract between [parameters(' and ')] - inner := expr[len("[parameters('"):] + inner := trimmed[len("[parameters('"):] before, _, ok := strings.Cut(inner, "')]") if !ok { return "" diff --git a/cli/azd/pkg/azapi/policy_service_test.go b/cli/azd/pkg/azapi/policy_service_test.go index 6cd8257008c..211f74127d6 100644 --- a/cli/azd/pkg/azapi/policy_service_test.go +++ b/cli/azd/pkg/azapi/policy_service_test.go @@ -211,6 +211,81 @@ func TestExtractLocalAuthDenyPolicies_NestedAnyOf(t *testing.T) { require.Equal(t, "Microsoft.ServiceBus/namespaces", results[0].ResourceType) } +func TestExtractLocalAuthDenyPolicies_MultipleResourceTypesInArray(t *testing.T) { + t.Parallel() + + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "in": []any{ + "Microsoft.EventHub/namespaces", + "Microsoft.ServiceBus/namespaces", + }, + }, + map[string]any{ + "field": "disableLocalAuth", + "notEquals": true, + }, + }, + }, + "then": map[string]any{ + "effect": "Deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "multi-type-policy", nil) + require.Len(t, results, 2) + + types := make(map[string]bool) + for _, r := range results { + types[r.ResourceType] = true + } + require.True(t, types["Microsoft.EventHub/namespaces"]) + require.True(t, types["Microsoft.ServiceBus/namespaces"]) +} + +func TestExtractLocalAuthDenyPolicies_NestedConditionInheritsResourceType(t *testing.T) { + t.Parallel() + + // The resource type is declared at the outer allOf level, and the disableLocalAuth + // condition is in a nested anyOf. The nested level should inherit the resource type. + def := &armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "if": map[string]any{ + "allOf": []any{ + map[string]any{ + "field": "type", + "equals": "Microsoft.Search/searchServices", + }, + map[string]any{ + "anyOf": []any{ + map[string]any{ + "field": "disableLocalAuth", + "equals": false, + }, + }, + }, + }, + }, + "then": map[string]any{ + "effect": "Deny", + }, + }, + }, + } + + results := extractLocalAuthDenyPolicies(def, "nested-inherit", nil) + require.Len(t, results, 1) + require.Equal(t, "Microsoft.Search/searchServices", results[0].ResourceType) +} + func TestIsLocalAuthField(t *testing.T) { t.Parallel() @@ -269,6 +344,8 @@ func TestExtractParameterReference(t *testing.T) { {"deny", ""}, {"[concat('a', 'b')]", ""}, {"", ""}, + {" [parameters('effect')] ", "effect"}, + {"\t[parameters('effect')]\n", "effect"}, } for _, tt := range tests { diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 0ab3549972a..c6d9ba0c397 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2433,24 +2433,35 @@ func (p *BicepProvider) checkLocalAuthPolicy( return nil, nil } - // Build a set of resource types that have deny policies. - policyByType := make(map[string]azapi.LocalAuthDenyPolicy) + // Build a map of resource types to their deny policies (may have multiple per type). + policiesByType := make(map[string][]azapi.LocalAuthDenyPolicy) for _, policy := range denyPolicies { - policyByType[strings.ToLower(policy.ResourceType)] = policy + key := strings.ToLower(policy.ResourceType) + policiesByType[key] = append(policiesByType[key], policy) } // Check each snapshot resource against the deny policies. var violations []string for _, resource := range valCtx.SnapshotResources { - policy, found := policyByType[strings.ToLower(resource.Type)] + policies, found := policiesByType[strings.ToLower(resource.Type)] if !found { continue } if !azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) { + policyNames := make([]string, 0, len(policies)) + for _, p := range policies { + if p.PolicyName != "" { + policyNames = append(policyNames, p.PolicyName) + } + } + policyDesc := strings.Join(policyNames, ", ") + if policyDesc == "" { + policyDesc = "(unnamed)" + } violations = append(violations, fmt.Sprintf( - " - %s (type: %s) — policy: %q", - resource.Name, resource.Type, policy.PolicyName, + " - %s (type: %s) — policies: %s", + resource.Name, resource.Type, policyDesc, )) } } @@ -2464,7 +2475,8 @@ func (p *BicepProvider) checkLocalAuthPolicy( Message: fmt.Sprintf( "subscription %s has Azure Policy assignments that deny resources with local "+ "authentication enabled. The following resources may be blocked:\n%s\n"+ - "Set 'disableLocalAuth: true' on these resources or request a policy exemption. "+ + "Disable local authentication on these resources (e.g. set 'disableLocalAuth: true' "+ + "or 'allowSharedKeyAccess: false' for storage accounts) or request a policy exemption. "+ "See https://aka.ms/safesecretsstandard for details.", subscriptionId, strings.Join(violations, "\n"), From fabf4c088021462866fe6f7551119cd48f1aa8c9 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 05:20:52 +0000 Subject: [PATCH 03/13] Add diagnostic logging to local auth policy preflight check Adds log.Printf statements at key decision points so the flow can be traced with --debug to diagnose why the check might not fire: - Number of policy assignments found - Policy rule parse failures - Number of deny policies found - Per-resource type matching and localAuthDisabled status - Effect type mismatches Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/policy_service.go | 19 ++++++++++++++++++- .../provisioning/bicep/bicep_provider.go | 11 +++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index 7c41aeee4af..58110569a7c 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -82,6 +82,7 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( } assignments = append(assignments, page.Value...) } + log.Printf("policy preflight: found %d policy assignments for subscription %s", len(assignments), subscriptionId) var results []LocalAuthDenyPolicy @@ -267,6 +268,8 @@ func extractLocalAuthDenyPolicies( ruleMap, ok := def.Properties.PolicyRule.(map[string]any) if !ok { + log.Printf("policy preflight: policy rule is not a map for definition %s (type %T)", + stringOrEmpty(def.Name), def.Properties.PolicyRule) return nil } @@ -281,7 +284,12 @@ func extractLocalAuthDenyPolicies( return nil } - return findLocalAuthConditions(ifBlock, assignmentName) + results := findLocalAuthConditions(ifBlock, assignmentName) + if len(results) > 0 { + log.Printf("policy preflight: found %d local auth deny condition(s) in policy %q", + len(results), assignmentName) + } + return results } // isDenyEffect checks whether the policy's effect resolves to "deny". @@ -308,6 +316,7 @@ func isDenyEffect( effectStr, ok := effectVal.(string) if !ok { + log.Printf("policy preflight: effect value is not a string (type %T): %v", effectVal, effectVal) return false } @@ -614,3 +623,11 @@ func ResourceHasLocalAuthDisabled(resourceType string, properties json.RawMessag return false } + +// stringOrEmpty safely dereferences a string pointer, returning "" if nil. +func stringOrEmpty(s *string) string { + if s == nil { + return "" + } + return *s +} diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index c6d9ba0c397..129cf40b499 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2412,6 +2412,7 @@ func (p *BicepProvider) checkLocalAuthPolicy( ctx context.Context, valCtx *validationContext, ) (*PreflightCheckResult, error) { if len(valCtx.SnapshotResources) == 0 { + log.Printf("policy preflight: no snapshot resources, skipping local auth policy check") return nil, nil } @@ -2429,6 +2430,8 @@ func (p *BicepProvider) checkLocalAuthPolicy( return nil, nil } + log.Printf("policy preflight: found %d deny policies targeting local auth", len(denyPolicies)) + if len(denyPolicies) == 0 { return nil, nil } @@ -2440,6 +2443,14 @@ func (p *BicepProvider) checkLocalAuthPolicy( policiesByType[key] = append(policiesByType[key], policy) } + // Log snapshot resource types for diagnostics. + for _, resource := range valCtx.SnapshotResources { + _, matched := policiesByType[strings.ToLower(resource.Type)] + hasLocalAuthDisabled := azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) + log.Printf("policy preflight: resource %q type=%s policyMatch=%v localAuthDisabled=%v", + resource.Name, resource.Type, matched, hasLocalAuthDisabled) + } + // Check each snapshot resource against the deny policies. var violations []string for _, resource := range valCtx.SnapshotResources { From 2d998e818e6a5f70d6acf2b7c647f3fcb6af72d0 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 05:55:08 +0000 Subject: [PATCH 04/13] Fix: fetch management-group-scoped policy definitions The subscription-level API lists inherited policy assignments from management groups, but the definition IDs reference management group scope (e.g. /providers/Microsoft.Management/managementGroups/{mgId}/...). Previously, getPolicyDefinitionByID and getPolicySetDefinitionByID only handled built-in and subscription-scoped definitions, silently failing to fetch management-group-scoped ones. This caused the check to miss policies like 'Storage Accounts - Safe Secrets Standard' which are typically assigned at the management group level. Fix: detect management group scope from the definition ID and use GetAtManagementGroup SDK method. Also add diagnostic logging throughout the check pipeline to aid debugging. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/policy_service.go | 37 ++++++++++++++++++- cli/azd/pkg/azapi/policy_service_test.go | 46 ++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index 58110569a7c..66a5d6a8b14 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -203,7 +203,7 @@ func (s *PolicyService) checkPolicySetDefinition( } // getPolicyDefinitionByID fetches a policy definition by its full resource ID. -// It handles both built-in (/providers/Microsoft.Authorization/...) and subscription-scoped definitions. +// It handles built-in, subscription-scoped, and management-group-scoped definitions. func getPolicyDefinitionByID( ctx context.Context, client *armpolicy.DefinitionsClient, @@ -222,6 +222,14 @@ func getPolicyDefinitionByID( return &resp.Definition, nil } + if mgID := extractManagementGroupID(definitionID); mgID != "" { + resp, err := client.GetAtManagementGroup(ctx, mgID, name, nil) + if err != nil { + return nil, err + } + return &resp.Definition, nil + } + resp, err := client.Get(ctx, name, nil) if err != nil { return nil, err @@ -230,6 +238,7 @@ func getPolicyDefinitionByID( } // getPolicySetDefinitionByID fetches a policy set definition by its full resource ID. +// It handles built-in, subscription-scoped, and management-group-scoped set definitions. func getPolicySetDefinitionByID( ctx context.Context, client *armpolicy.SetDefinitionsClient, @@ -248,6 +257,14 @@ func getPolicySetDefinitionByID( return &resp.SetDefinition, nil } + if mgID := extractManagementGroupID(setDefinitionID); mgID != "" { + resp, err := client.GetAtManagementGroup(ctx, mgID, name, nil) + if err != nil { + return nil, err + } + return &resp.SetDefinition, nil + } + resp, err := client.Get(ctx, name, nil) if err != nil { return nil, err @@ -591,6 +608,24 @@ func lastSegment(resourceID string) string { return parts[len(parts)-1] } +// extractManagementGroupID extracts the management group ID from a resource ID like +// "/providers/Microsoft.Management/managementGroups/{mgId}/providers/Microsoft.Authorization/..." +// Returns empty string if the ID is not management-group-scoped. +func extractManagementGroupID(resourceID string) string { + lower := strings.ToLower(resourceID) + const prefix = "/providers/microsoft.management/managementgroups/" + idx := strings.Index(lower, prefix) + if idx < 0 { + return "" + } + rest := resourceID[idx+len(prefix):] + end := strings.Index(rest, "/") + if end < 0 { + return rest + } + return rest[:end] +} + // ResourceHasLocalAuthDisabled checks whether a resource's properties JSON has // the disableLocalAuth property set to true (or allowSharedKeyAccess set to false // for storage accounts). diff --git a/cli/azd/pkg/azapi/policy_service_test.go b/cli/azd/pkg/azapi/policy_service_test.go index 211f74127d6..df7308f5aa5 100644 --- a/cli/azd/pkg/azapi/policy_service_test.go +++ b/cli/azd/pkg/azapi/policy_service_test.go @@ -286,6 +286,52 @@ func TestExtractLocalAuthDenyPolicies_NestedConditionInheritsResourceType(t *tes require.Equal(t, "Microsoft.Search/searchServices", results[0].ResourceType) } +func TestExtractManagementGroupID(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id string + want string + }{ + { + "management group scoped policy definition", + "/providers/Microsoft.Management/managementGroups/myMgGroup" + + "/providers/Microsoft.Authorization/policyDefinitions/abc123", + "myMgGroup", + }, + { + "management group scoped policy set definition", + "/providers/Microsoft.Management/managementGroups/" + + "72f988bf-86f1-41af-91ab-2d7cd011db47" + + "/providers/Microsoft.Authorization/policySetDefinitions/8e7a35ba", + "72f988bf-86f1-41af-91ab-2d7cd011db47", + }, + { + "built-in policy definition", + "/providers/Microsoft.Authorization/policyDefinitions/6300012e-e9a4-4649-b41f-a85f5c43be91", + "", + }, + { + "subscription scoped custom definition", + "/subscriptions/faa080af/providers/Microsoft.Authorization/policyDefinitions/custom123", + "", + }, + { + "empty string", + "", + "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, extractManagementGroupID(tt.id)) + }) + } +} + func TestIsLocalAuthField(t *testing.T) { t.Parallel() From 2fa89836551b935939bcb1b3ee329188d849fdf9 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 06:02:15 +0000 Subject: [PATCH 05/13] Improve preflight warning message for local auth policy - Remove subscription ID (redundant, already shown in context) - Remove unresolved ARM expression resource names (not helpful) - Aggregate by resource type instead of per-resource - Deduplicate policy names - Shorter, cleaner message format Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../provisioning/bicep/bicep_provider.go | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 129cf40b499..90d388f9027 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2451,8 +2451,9 @@ func (p *BicepProvider) checkLocalAuthPolicy( resource.Name, resource.Type, matched, hasLocalAuthDisabled) } - // Check each snapshot resource against the deny policies. - var violations []string + // Collect affected resource types (deduplicated) and unique policy names. + affectedTypes := make(map[string]bool) + uniquePolicyNames := make(map[string]bool) for _, resource := range valCtx.SnapshotResources { policies, found := policiesByType[strings.ToLower(resource.Type)] if !found { @@ -2460,37 +2461,39 @@ func (p *BicepProvider) checkLocalAuthPolicy( } if !azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) { - policyNames := make([]string, 0, len(policies)) + affectedTypes[resource.Type] = true for _, p := range policies { if p.PolicyName != "" { - policyNames = append(policyNames, p.PolicyName) + uniquePolicyNames[p.PolicyName] = true } } - policyDesc := strings.Join(policyNames, ", ") - if policyDesc == "" { - policyDesc = "(unnamed)" - } - violations = append(violations, fmt.Sprintf( - " - %s (type: %s) — policies: %s", - resource.Name, resource.Type, policyDesc, - )) } } - if len(violations) == 0 { + if len(affectedTypes) == 0 { return nil, nil } + typeList := slices.Sorted(maps.Keys(affectedTypes)) + var lines []string + for _, t := range typeList { + lines = append(lines, fmt.Sprintf(" - %s", t)) + } + + policyList := slices.Sorted(maps.Keys(uniquePolicyNames)) + policyDesc := strings.Join(policyList, ", ") + if policyDesc == "" { + policyDesc = "(unnamed policy)" + } + return &PreflightCheckResult{ Severity: PreflightCheckWarning, Message: fmt.Sprintf( - "subscription %s has Azure Policy assignments that deny resources with local "+ - "authentication enabled. The following resources may be blocked:\n%s\n"+ - "Disable local authentication on these resources (e.g. set 'disableLocalAuth: true' "+ - "or 'allowSharedKeyAccess: false' for storage accounts) or request a policy exemption. "+ - "See https://aka.ms/safesecretsstandard for details.", - subscriptionId, - strings.Join(violations, "\n"), + "Azure Policy denies resources with local authentication enabled (%s). "+ + "Affected resource types in this deployment:\n%s\n"+ + "Disable local authentication on these resources or request a policy exemption.", + policyDesc, + strings.Join(lines, "\n"), ), }, nil } From b4dbec48d0e9eb5024ea2efe4b06e2c284105496 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 06:07:07 +0000 Subject: [PATCH 06/13] Simplify warning: drop policy names, keep resource types only Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../provisioning/bicep/bicep_provider.go | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 90d388f9027..5a017fec756 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2451,22 +2451,16 @@ func (p *BicepProvider) checkLocalAuthPolicy( resource.Name, resource.Type, matched, hasLocalAuthDisabled) } - // Collect affected resource types (deduplicated) and unique policy names. + // Collect affected resource types (deduplicated). affectedTypes := make(map[string]bool) - uniquePolicyNames := make(map[string]bool) for _, resource := range valCtx.SnapshotResources { - policies, found := policiesByType[strings.ToLower(resource.Type)] + _, found := policiesByType[strings.ToLower(resource.Type)] if !found { continue } if !azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) { affectedTypes[resource.Type] = true - for _, p := range policies { - if p.PolicyName != "" { - uniquePolicyNames[p.PolicyName] = true - } - } } } @@ -2480,19 +2474,12 @@ func (p *BicepProvider) checkLocalAuthPolicy( lines = append(lines, fmt.Sprintf(" - %s", t)) } - policyList := slices.Sorted(maps.Keys(uniquePolicyNames)) - policyDesc := strings.Join(policyList, ", ") - if policyDesc == "" { - policyDesc = "(unnamed policy)" - } - return &PreflightCheckResult{ Severity: PreflightCheckWarning, Message: fmt.Sprintf( - "Azure Policy denies resources with local authentication enabled (%s). "+ - "Affected resource types in this deployment:\n%s\n"+ + "an Azure Policy on this subscription denies resources with local authentication enabled. "+ + "The following resource types in this deployment may be blocked:\n%s\n"+ "Disable local authentication on these resources or request a policy exemption.", - policyDesc, strings.Join(lines, "\n"), ), }, nil From a5a93e3a94b370efd85105f944f969c11f3b99e9 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 18 Mar 2026 06:17:46 +0000 Subject: [PATCH 07/13] Fix CI: add managementgroups to cspell, apply go fix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/.vscode/cspell.yaml | 2 +- cli/azd/pkg/azapi/policy_service.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index 58ac9cdca11..5c3e9d239ec 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -362,9 +362,9 @@ overrides: - armpolicy - disablelocalauth - allowsharedkeyaccess - - localauthenabled - policydefinitions - policysetdefinitions + - managementgroups - filename: extensions/azure.ai.models/internal/cmd/custom_create.go words: - Qwen diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index 66a5d6a8b14..4322b407c82 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -619,11 +619,11 @@ func extractManagementGroupID(resourceID string) string { return "" } rest := resourceID[idx+len(prefix):] - end := strings.Index(rest, "/") - if end < 0 { + before, _, ok := strings.Cut(rest, "/") + if !ok { return rest } - return rest[:end] + return before } // ResourceHasLocalAuthDisabled checks whether a resource's properties JSON has From 89c6f245e9c2667da3b1e168496e445d7d189fc0 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Fri, 20 Mar 2026 18:48:32 +0000 Subject: [PATCH 08/13] Refactor policy preflight to use server-side checkPolicyRestrictions API Replace the custom client-side policy rule parsing engine with the server-side checkPolicyRestrictions API (Microsoft.PolicyInsights). The check is now scoped to storage accounts and calls the API in parallel for each account. Key changes: - PolicyService now uses armpolicyinsights.PolicyRestrictionsClient instead of manually fetching/parsing policy assignments & definitions - checkLocalAuthPolicy renamed to checkStorageAccountPolicy, filters snapshot resources to Microsoft.Storage/storageAccounts only - Parallel API calls per storage account via sync.WaitGroup.Go - Warning messages now include policy reasons from the server response - Replaced armpolicy SDK dependency with armpolicyinsights - Rewrote tests with mock HTTP transport for the new API Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .vscode/launch.json | 3 +- .../docs/design/extension-flag-handling.md | 291 ++++++++++++ cli/azd/go.mod | 6 +- cli/azd/go.sum | 12 +- cli/azd/pkg/azapi/policy_service.go | 413 ++++++++++++------ cli/azd/pkg/azapi/policy_service_test.go | 107 +++-- .../provisioning/bicep/bicep_provider.go | 131 +++--- .../provisioning/bicep/local_preflight.go | 105 +++-- 8 files changed, 808 insertions(+), 260 deletions(-) create mode 100644 cli/azd/docs/design/extension-flag-handling.md diff --git a/.vscode/launch.json b/.vscode/launch.json index eb889afec27..949c4b379ed 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -25,8 +25,9 @@ "request": "launch", "mode": "debug", "program": "${workspaceFolder}/cli/azd", - "args": "${input:cliArgs}", + "args": "up", "console": "integratedTerminal", + "cwd": "/home/vivazqu/workspace/build/debug/preflight-agentic" } ], "inputs": [ diff --git a/cli/azd/docs/design/extension-flag-handling.md b/cli/azd/docs/design/extension-flag-handling.md new file mode 100644 index 00000000000..ae645d0cb11 --- /dev/null +++ b/cli/azd/docs/design/extension-flag-handling.md @@ -0,0 +1,291 @@ +# Extension Flag Handling Design + +> **Status**: Design summary of current implementation (not a prescriptive spec). +> Extracted from the codebase as of March 2026 to document the existing behavior and +> call out undefined or ambiguous scenarios. + +## Overview + +When a user runs an extension command (e.g. `azd myext --my-flag value -e dev`), flags +must be split between two consumers: + +| Consumer | Needs | Examples | +|----------|-------|---------| +| **azd host** | Global flags that control host behavior | `--debug`, `--cwd`, `-e`, `--no-prompt` | +| **Extension process** | Extension-specific flags + positional args | `--my-flag value`, `--model gpt-4` | + +Today there is **no explicit separator** (like `--`) between the two. Instead, azd uses +a combination of early pre-parsing and Cobra's `DisableFlagParsing` to handle this. + +## Current Implementation + +### Phase 1: Early Pre-Parsing (`ParseGlobalFlags`) + +**File**: `cmd/auto_install.go` — `ParseGlobalFlags()` + +Before Cobra builds the command tree, `ParseGlobalFlags()` extracts known azd global +flags from the raw `os.Args` using a pflag FlagSet with `ParseErrorsAllowlist{UnknownFlags: true}`. +Unknown flags (extension flags) are silently ignored. + +**Flags parsed in this phase:** + +| Flag | Short | Stored in `GlobalCommandOptions` | +|------|-------|----------------------------------| +| `--cwd` | `-C` | `Cwd` | +| `--debug` | — | `EnableDebugLogging` | +| `--no-prompt` | — | `NoPrompt` | +| `--trace-log-file` | — | *(read by telemetry system)* | +| `--trace-log-url` | — | *(read by telemetry system)* | + +**Not parsed here**: `-e` / `--environment` is currently absent from `CreateGlobalFlagSet()`. +It is registered as a per-command flag on commands that need it (via `envFlag` binding), +not as a global flag. This is the root cause of [#7034](https://github.com/Azure/azure-dev/issues/7034). + +The result is stored in a `GlobalCommandOptions` singleton registered in the IoC container. + +**Agent detection**: If `--no-prompt` was not explicitly set, `ParseGlobalFlags` also +checks `agentdetect.IsRunningInAgent()` and auto-enables `NoPrompt` for AI coding agents. + +### Phase 2: Cobra Command Registration + +**File**: `cmd/extensions.go` — `bindExtension()` + +Extension commands are registered with: + +```go +cmd := &cobra.Command{ + Use: lastPart, + DisableFlagParsing: true, +} +``` + +`DisableFlagParsing: true` means Cobra will **not** parse any flags for these commands. +All tokens after the command name are passed to the action handler as raw `args`. + +**Consequence**: Cobra's persistent flags (including `-e`/`--environment` if registered) +are not parsed for extension commands. Calls to `cmd.Flags().GetString("environment")` +silently return `""`. + +### Phase 3: Extension Action Execution + +**File**: `cmd/extensions.go` — `extensionAction.Run()` + +The action handler builds `InvokeOptions` from two sources: + +1. **Cobra persistent flags** (`cmd.Flags().Get*`) — for `debug`, `cwd`, `environment` +2. **`GlobalCommandOptions`** — for `NoPrompt` (because it includes agent detection) + +```go +debugEnabled, _ := a.cmd.Flags().GetBool("debug") +cwd, _ := a.cmd.Flags().GetString("cwd") +envName, _ := a.cmd.Flags().GetString("environment") + +options := &extensions.InvokeOptions{ + Args: a.args, // ALL raw args (extension + global flags) + Debug: debugEnabled, + NoPrompt: a.globalOptions.NoPrompt, + Cwd: cwd, + Environment: envName, +} +``` + +**Problem**: Because `DisableFlagParsing: true`, `cmd.Flags().Get*` calls for `debug`, +`cwd`, and `environment` return zero values. Only `NoPrompt` works correctly because it +reads from `globalOptions` (populated in Phase 1). + +### Phase 4: Environment Variable Propagation + +**File**: `pkg/extensions/runner.go` — `Runner.Invoke()` + +Global flag values from `InvokeOptions` are converted to environment variables before +spawning the extension process: + +| `InvokeOptions` field | Environment variable | Condition | +|-----------------------|---------------------|-----------| +| `Debug` | `AZD_DEBUG=true` | if true | +| `NoPrompt` | `AZD_NO_PROMPT=true` | if true | +| `Cwd` | `AZD_CWD=` | if non-empty | +| `Environment` | `AZD_ENVIRONMENT=` | if non-empty | + +The extension process also receives: +- `AZD_SERVER=:` — gRPC server address +- `AZD_ACCESS_TOKEN=` — Authentication token +- `TRACEPARENT=` — W3C trace context (if tracing is active) +- Environment variables from the resolved azd environment (`env.Environ()`) + +### Phase 5: Middleware Extensions (Listener Path) + +**File**: `cmd/middleware/extensions.go` — `ExtensionsMiddleware.Run()` + +Lifecycle extensions (those with `lifecycle-events`, `service-target-provider`, or +`framework-service-provider` capabilities) use a different execution path: + +- Started with fixed args `["listen"]` +- Read global flags from `m.options.Flags` (Cobra persistent flags from the *parent* command) +- Also have access to `m.globalOptions` + +This path works for standard azd commands (like `azd deploy`) where Cobra *does* parse +persistent flags. The middleware invocation constructs `InvokeOptions` similarly. + +## What Extensions Receive + +When a user runs: `azd myext --my-flag value -e dev --debug` + +The extension process gets: + +| Channel | Content | +|---------|---------| +| **Command-line args** | `["--my-flag", "value", "-e", "dev", "--debug"]` — ALL tokens after the command name, including azd global flags | +| **Environment variables** | `AZD_DEBUG`, `AZD_NO_PROMPT`, `AZD_CWD`, `AZD_ENVIRONMENT` — set only if their `InvokeOptions` field is non-empty | +| **gRPC services** | Extension can call `EnvironmentService.GetCurrentEnvironment()` to get the resolved environment | + +**Key observation**: Global azd flags appear in **both** the raw args and environment +variables. Extensions must decide which source to trust. + +## Extension-Side Flag Parsing + +Extensions are expected to: + +1. **Read global azd flags from environment variables** (`AZD_DEBUG`, `AZD_NO_PROMPT`, + `AZD_CWD`, `AZD_ENVIRONMENT`), not from their command-line args. +2. **Parse their own flags** from the command-line args they receive. The azd SDK provides + helpers (`azdext` package) for this. +3. **Ignore azd global flags in their args** — these are artifacts of `DisableFlagParsing` + and should not be interpreted by the extension. + +However, this contract is **not formally documented or enforced**. Extensions using the +Go SDK with Cobra will likely re-parse all args including the azd global flags. + +## Undefined Scenarios and Edge Cases + +### 1. Flag Name Collisions + +**Scenario**: Extension defines a flag with the same name as an azd global flag +(e.g., `--debug` meaning "debug output format" in the extension). + +**Current behavior**: `ParseGlobalFlags` will consume `--debug` and set +`GlobalCommandOptions.EnableDebugLogging = true`. The extension will also see `--debug` +in its raw args and may interpret it differently. + +**Impact**: Both azd and the extension act on the same flag with different semantics. +There is no way to say "this `--debug` is for the extension, not azd". + +### 2. Global Flags Not Stripped From Extension Args + +**Scenario**: User runs `azd myext -e dev --debug --my-flag value`. + +**Current behavior**: Extension receives args `["-e", "dev", "--debug", "--my-flag", "value"]`. +The azd global flags are **not** removed from the args before passing to the extension. + +**Impact**: If the extension uses a strict flag parser, unknown flags like `--debug` may +cause parse errors. Extensions must use permissive parsing or explicitly handle azd flags. + +### 3. No `--` Separator Convention + +**Scenario**: User wants to pass `--debug` to the extension but not to azd. + +**Current behavior**: No mechanism exists. `ParseGlobalFlags` will always consume +recognized flags regardless of position. + +**Potential design**: A `--` separator could be introduced: +`azd --debug myext -- --debug` (first `--debug` for azd, second for extension). +This is not implemented. + +### 4. `-e` / `--environment` Not Pre-Parsed + +**Scenario**: User runs `azd myext -e dev`. + +**Current behavior** (main branch): `-e` is NOT in `CreateGlobalFlagSet()`, so +`ParseGlobalFlags` ignores it. `cmd.Flags().GetString("environment")` returns `""` +due to `DisableFlagParsing`. Result: `AZD_ENVIRONMENT` is never set, and `lazyEnv` +resolves to the default environment. + +**Impact**: This is the bug described in [#7034](https://github.com/Azure/azure-dev/issues/7034). +The default environment's variables leak into the extension process. + +### 5. Environment Variable Injection From Default Environment + +**Scenario**: Extension commands inject `lazyEnv.Environ()` (the resolved azd +environment's key-value pairs) into the extension process's environment. + +**Question**: Should extensions receive environment variables from the azd environment +at all, or should they exclusively use the gRPC `EnvironmentService`? + +**Current behavior**: `extensionAction.Run()` calls `a.lazyEnv.GetValue()` and appends +all key-value pairs as process environment variables. This means the extension inherits +every value from the resolved azd environment. + +**Trade-off**: Injecting env vars is convenient (extensions "just work" with standard +`os.Getenv`), but it creates an implicit coupling and makes it hard to distinguish +azd-injected variables from system environment variables. + +### 6. `--cwd` Semantics + +**Scenario**: User runs `azd myext -C ./subdir --my-flag value`. + +**Current behavior**: `ParseGlobalFlags` captures `Cwd = "./subdir"`. The root command +changes the process working directory before the extension runs. `AZD_CWD=./subdir` is +set in the extension's environment. + +**Question**: Should the extension receive the *original* cwd or the *resolved* cwd? +Currently it receives the raw flag value (potentially relative), but the process has +already `cd`'d into that directory. + +### 7. Middleware vs Command Extension Flag Parity + +**Scenario**: A lifecycle extension (middleware path) needs the `-e` environment name. + +**Current behavior**: Middleware reads `m.options.Flags.GetString("environment")` from +Cobra persistent flags of the parent command (e.g., `azd deploy`). This works because +the parent command *does* parse flags. + +**Gap**: The two execution paths (command extensions vs middleware extensions) have +different flag resolution strategies. Command extensions cannot rely on Cobra flags; +middleware extensions can. + +### 8. Help Text for Extension Commands + +**Scenario**: User runs `azd myext --help`. + +**Current behavior**: Because `DisableFlagParsing: true`, Cobra does not intercept +`--help`. The `--help` flag is passed as an arg to the extension. The extension must +handle it. Cobra's auto-generated help (which would show global flags) is not displayed. + +**Impact**: Users don't see a consistent help experience between azd commands and +extension commands. Global flags like `-e`, `--debug` are not listed in extension help. + +## Recommendations + +These are observations about gaps in the current design, not prescriptive changes: + +1. **Document the flag contract**: Extensions should know definitively whether to read + global flags from env vars or args. Today this is implicit. + +2. **Consider stripping global flags from extension args**: If azd globals are communicated + via env vars, they arguably should not appear in the extension's command-line args. + This would eliminate collision issues. However, this would be a breaking change for + extensions that currently parse `-e` from their args. + +3. **Consider a `--` separator**: `azd [azd-flags] myext -- [ext-flags]` would make the + boundary explicit. This is a convention used by many CLI tools (npm, cargo, kubectl). + +4. **Unify flag resolution**: The divergence between command extensions (can't use Cobra + flags) and middleware extensions (can use Cobra flags) is a source of bugs. A single + resolution path through `GlobalCommandOptions` would be more robust. + +5. **Document `AZD_ENVIRONMENT`, `AZD_DEBUG`, `AZD_NO_PROMPT`, `AZD_CWD`**: These + environment variables are set for extension processes but not listed in + `docs/environment-variables.md`. + +## File Reference + +| File | Role | +|------|------| +| `cmd/auto_install.go` | `CreateGlobalFlagSet()`, `ParseGlobalFlags()` | +| `cmd/extensions.go` | `bindExtension()` (DisableFlagParsing), `extensionAction.Run()` | +| `cmd/container.go` | `EnvFlag` DI resolver (environment name resolution chain) | +| `cmd/middleware/extensions.go` | Lifecycle extension invocation (middleware path) | +| `cmd/root.go` | Command tree construction, global flag registration | +| `pkg/extensions/runner.go` | `InvokeOptions`, env var propagation, process spawning | +| `internal/global_command_options.go` | `GlobalCommandOptions` struct | +| `internal/env_flag.go` | `EnvFlag` type, `EnvironmentNameFlagName` constant | diff --git a/cli/azd/go.mod b/cli/azd/go.mod index be20bfe4249..ae24df71c73 100644 --- a/cli/azd/go.mod +++ b/cli/azd/go.mod @@ -5,7 +5,7 @@ go 1.26.0 require ( dario.cat/mergo v1.0.2 github.com/AlecAivazis/survey/v2 v2.3.7 - github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 + github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/apimanagement/armapimanagement v1.1.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appconfiguration/armappconfiguration v1.1.1 @@ -22,7 +22,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/operationalinsights/armoperationalinsights/v2 v2.0.2 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql/v2 v2.0.0-beta.7 @@ -47,7 +47,7 @@ require ( github.com/fsnotify/fsnotify v1.9.0 github.com/github/copilot-sdk/go v0.1.32 github.com/gofrs/flock v0.12.1 - github.com/golang-jwt/jwt/v5 v5.3.0 + github.com/golang-jwt/jwt/v5 v5.3.1 github.com/golobby/container/v3 v3.3.2 github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 diff --git a/cli/azd/go.sum b/cli/azd/go.sum index df5d966d10a..cbec8c3261e 100644 --- a/cli/azd/go.sum +++ b/cli/azd/go.sum @@ -3,8 +3,8 @@ dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ= github.com/AlecAivazis/survey/v2 v2.3.7/go.mod h1:xUTIdE4KCOIjsBAE1JYsUPoCqYdZ1reCfTwbto0Fduo= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 h1:JXg2dwJUmPB9JmtVmdEB16APJ7jurfbY5jnfXpJoRMc= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0/go.mod h1:YD5h/ldMsG0XiIw7PdyNhLxaM317eFh5yNLccNfGdyw= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 h1:fou+2+WFTib47nS+nz/ozhEBnvU96bKHy6LjRsY4E28= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0/go.mod h1:t76Ruy8AHvUAC8GfMWJMa0ElSbuIcO03NLpynfbgsPA= github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 h1:Hk5QBxZQC1jb2Fwj6mpzme37xbCDdNTxU7O9eb5+LB4= github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1/go.mod h1:IYus9qsFobWIc2YVwe/WPjcnyCkPKtnHAqUYeebc8z0= github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.2 h1:yz1bePFlP5Vws5+8ez6T3HWXPmwOK7Yvq8QxDBD3SKY= @@ -49,8 +49,8 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourceg github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0/go.mod h1:wVEOJfGTj0oPAUGA1JuRAvz/lxXQsWW16axmHPP47Bk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 h1:bcgO/crpp7wqI0Froi/I4C2fme7Vk/WLusbV399Do8I= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1/go.mod h1:kvfPmsE8gpOwwC1qrO1FeyBDDNfnwBN5UU3MPNiWW7I= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0 h1:FCprRw2Uzske3FiFVGm6MqJY829zrAJLiN4coFueWis= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v0.10.0/go.mod h1:koK4/Mf6lxFkYavGzZnzTUOEmY8ic9tN44UmWZsGfrk= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0 h1:FKYUd0TZfjjWtdk7LR+6rI64yNY1gMe9rNV40Y2Fh4o= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0/go.mod h1:w987ZDGBg74HZu5sAqaBEhDTlvlJlxPggIhBiDviYhE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0/go.mod h1:5kakwfW5CjC9KK+Q4wjXAg+ShuIm2mBMua0ZFj2C8PE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 h1:wxQx2Bt4xzPIKvW59WQf1tJNx/ZZKPfN+EhPX3Z6CYY= @@ -163,8 +163,8 @@ github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E= github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0= github.com/gofrs/uuid v3.3.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= -github.com/golang-jwt/jwt/v5 v5.3.0 h1:pv4AsKCKKZuqlgs5sUmn4x8UlGa0kEVt/puTpKx9vvo= -github.com/golang-jwt/jwt/v5 v5.3.0/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= +github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= +github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index 4322b407c82..199719d4e95 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -16,19 +16,21 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/account" ) -// LocalAuthDenyPolicy describes a deny policy that requires disableLocalAuth to be true -// for a specific Azure resource type. -type LocalAuthDenyPolicy struct { +// DenyPolicy describes a deny-effect policy that targets a specific field on +// an Azure resource type (e.g. disableLocalAuth, allowSharedKeyAccess). +type DenyPolicy struct { // PolicyName is the display name of the policy assignment. PolicyName string - // ResourceType is the Azure resource type targeted (e.g. "Microsoft.CognitiveServices/accounts"). + // ResourceType is the Azure resource type targeted + // (e.g. "Microsoft.CognitiveServices/accounts"). ResourceType string - // FieldPath is the full field path checked (e.g. "Microsoft.CognitiveServices/accounts/disableLocalAuth"). + // FieldPath is the full field path checked + // (e.g. "Microsoft.CognitiveServices/accounts/disableLocalAuth"). FieldPath string } // PolicyService queries Azure Policy assignments and definitions to detect -// policies that would block deployment of resources with local authentication enabled. +// deny-effect policies that would block resource deployment. type PolicyService struct { credentialProvider account.SubscriptionCredentialProvider armClientOptions *arm.ClientOptions @@ -45,31 +47,49 @@ func NewPolicyService( } } -// FindLocalAuthDenyPolicies lists policy assignments on the subscription and inspects -// their definitions for deny-effect rules that require disableLocalAuth to be true. +// FindDenyPolicies lists policy assignments on the subscription (including +// inherited assignments from management groups) and inspects their definitions +// for deny-effect rules that target resource property fields. // It returns a list of matching policies with their target resource types. -func (s *PolicyService) FindLocalAuthDenyPolicies( +func (s *PolicyService) FindDenyPolicies( ctx context.Context, subscriptionId string, -) ([]LocalAuthDenyPolicy, error) { - credential, err := s.credentialProvider.CredentialForSubscription(ctx, subscriptionId) +) ([]DenyPolicy, error) { + credential, err := s.credentialProvider.CredentialForSubscription( + ctx, subscriptionId, + ) if err != nil { - return nil, fmt.Errorf("getting credential for subscription %s: %w", subscriptionId, err) + return nil, fmt.Errorf( + "getting credential for subscription %s: %w", + subscriptionId, err, + ) } - assignmentsClient, err := armpolicy.NewAssignmentsClient(subscriptionId, credential, s.armClientOptions) + assignmentsClient, err := armpolicy.NewAssignmentsClient( + subscriptionId, credential, s.armClientOptions, + ) if err != nil { - return nil, fmt.Errorf("creating policy assignments client: %w", err) + return nil, fmt.Errorf( + "creating policy assignments client: %w", err, + ) } - definitionsClient, err := armpolicy.NewDefinitionsClient(subscriptionId, credential, s.armClientOptions) + definitionsClient, err := armpolicy.NewDefinitionsClient( + subscriptionId, credential, s.armClientOptions, + ) if err != nil { - return nil, fmt.Errorf("creating policy definitions client: %w", err) + return nil, fmt.Errorf( + "creating policy definitions client: %w", err, + ) } - setDefinitionsClient, err := armpolicy.NewSetDefinitionsClient(subscriptionId, credential, s.armClientOptions) + setDefinitionsClient, err := armpolicy.NewSetDefinitionsClient( + subscriptionId, credential, s.armClientOptions, + ) if err != nil { - return nil, fmt.Errorf("creating policy set definitions client: %w", err) + return nil, fmt.Errorf( + "creating policy set definitions client: %w", err, + ) } // List all policy assignments for the subscription. @@ -82,17 +102,21 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( } assignments = append(assignments, page.Value...) } - log.Printf("policy preflight: found %d policy assignments for subscription %s", len(assignments), subscriptionId) + log.Printf( + "policy preflight: found %d policy assignments for subscription %s", + len(assignments), subscriptionId, + ) - var results []LocalAuthDenyPolicy + var results []DenyPolicy - // Cache fetched definitions/set definitions to avoid duplicate API calls when - // multiple assignments reference the same definition. + // Cache fetched definitions/set definitions to avoid duplicate API + // calls when multiple assignments reference the same definition. defCache := make(map[string]*armpolicy.Definition) setDefCache := make(map[string]*armpolicy.SetDefinition) for _, assignment := range assignments { - if assignment.Properties == nil || assignment.Properties.PolicyDefinitionID == nil { + if assignment.Properties == nil || + assignment.Properties.PolicyDefinitionID == nil { continue } @@ -102,19 +126,21 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( assignmentName = *assignment.Properties.DisplayName } - // Resolve the assignment's parameter values so we can evaluate parameterized effects. + // Resolve the assignment's parameter values so we can evaluate + // parameterized effects. assignmentParams := extractAssignmentParams(assignment) - if isBuiltInPolicyDefinition(defID) || isCustomPolicyDefinition(defID) { - // Single policy definition — check it directly. + if isBuiltInPolicyDefinition(defID) || + isCustomPolicyDefinition(defID) { policies := s.checkPolicyDefinition( - ctx, definitionsClient, defID, assignmentName, assignmentParams, defCache, + ctx, definitionsClient, defID, assignmentName, + assignmentParams, defCache, ) results = append(results, policies...) } else if isPolicySetDefinition(defID) { - // Policy set (initiative) — enumerate its member definitions. policies := s.checkPolicySetDefinition( - ctx, setDefinitionsClient, definitionsClient, defID, assignmentName, assignmentParams, + ctx, setDefinitionsClient, definitionsClient, + defID, assignmentName, assignmentParams, defCache, setDefCache, ) results = append(results, policies...) @@ -124,8 +150,8 @@ func (s *PolicyService) FindLocalAuthDenyPolicies( return results, nil } -// checkPolicyDefinition fetches a single policy definition (using cache) and inspects it for -// disableLocalAuth deny rules. Returns any matching policies found. +// checkPolicyDefinition fetches a single policy definition (using cache) and +// inspects it for deny rules targeting resource property fields. func (s *PolicyService) checkPolicyDefinition( ctx context.Context, client *armpolicy.DefinitionsClient, @@ -133,13 +159,18 @@ func (s *PolicyService) checkPolicyDefinition( assignmentName string, assignmentParams map[string]any, cache map[string]*armpolicy.Definition, -) []LocalAuthDenyPolicy { +) []DenyPolicy { definition, ok := cache[definitionID] if !ok { var err error - definition, err = getPolicyDefinitionByID(ctx, client, definitionID) + definition, err = getPolicyDefinitionByID( + ctx, client, definitionID, + ) if err != nil { - log.Printf("policy preflight: could not fetch policy definition %s: %v", definitionID, err) + log.Printf( + "policy preflight: could not fetch policy definition %s: %v", + definitionID, err, + ) cache[definitionID] = nil return nil } @@ -149,11 +180,13 @@ func (s *PolicyService) checkPolicyDefinition( return nil } - return extractLocalAuthDenyPolicies(definition, assignmentName, assignmentParams) + return extractDenyPolicies( + definition, assignmentName, assignmentParams, + ) } -// checkPolicySetDefinition fetches a policy set definition (initiative) and checks -// each of its member policy definitions for disableLocalAuth deny rules. +// checkPolicySetDefinition fetches a policy set definition (initiative) and +// checks each of its member policy definitions for deny rules. func (s *PolicyService) checkPolicySetDefinition( ctx context.Context, setClient *armpolicy.SetDefinitionsClient, @@ -163,14 +196,18 @@ func (s *PolicyService) checkPolicySetDefinition( assignmentParams map[string]any, defCache map[string]*armpolicy.Definition, setDefCache map[string]*armpolicy.SetDefinition, -) []LocalAuthDenyPolicy { +) []DenyPolicy { setDef, ok := setDefCache[setDefinitionID] if !ok { var err error - setDef, err = getPolicySetDefinitionByID(ctx, setClient, setDefinitionID) + setDef, err = getPolicySetDefinitionByID( + ctx, setClient, setDefinitionID, + ) if err != nil { log.Printf( - "policy preflight: could not fetch policy set definition %s: %v", setDefinitionID, err) + "policy preflight: could not fetch policy set definition %s: %v", + setDefinitionID, err, + ) setDefCache[setDefinitionID] = nil return nil } @@ -180,21 +217,32 @@ func (s *PolicyService) checkPolicySetDefinition( return nil } - if setDef.Properties == nil || setDef.Properties.PolicyDefinitions == nil { + if setDef.Properties == nil || + setDef.Properties.PolicyDefinitions == nil { return nil } - var results []LocalAuthDenyPolicy + // Build effective parameters by applying the set definition's defaults + // for any parameters not explicitly set by the assignment. Without this, + // "Opt In" policies whose assignment relies on the set's default (e.g. + // "Audit") would incorrectly fall back to the member definition's + // default (often "Deny"). + effectiveParams := applySetDefaults( + assignmentParams, setDef.Properties.Parameters, + ) + + var results []DenyPolicy for _, member := range setDef.Properties.PolicyDefinitions { if member.PolicyDefinitionID == nil { continue } // Merge set-level parameters with member-level parameter values. - memberParams := mergeParams(assignmentParams, member.Parameters) + memberParams := mergeParams(effectiveParams, member.Parameters) policies := s.checkPolicyDefinition( - ctx, defClient, *member.PolicyDefinitionID, assignmentName, memberParams, defCache, + ctx, defClient, *member.PolicyDefinitionID, + assignmentName, memberParams, defCache, ) results = append(results, policies...) } @@ -203,7 +251,8 @@ func (s *PolicyService) checkPolicySetDefinition( } // getPolicyDefinitionByID fetches a policy definition by its full resource ID. -// It handles built-in, subscription-scoped, and management-group-scoped definitions. +// It handles built-in, subscription-scoped, and management-group-scoped +// definitions. func getPolicyDefinitionByID( ctx context.Context, client *armpolicy.DefinitionsClient, @@ -211,7 +260,9 @@ func getPolicyDefinitionByID( ) (*armpolicy.Definition, error) { name := lastSegment(definitionID) if name == "" { - return nil, fmt.Errorf("invalid policy definition ID: %s", definitionID) + return nil, fmt.Errorf( + "invalid policy definition ID: %s", definitionID, + ) } if isBuiltInPolicyDefinition(definitionID) { @@ -237,8 +288,9 @@ func getPolicyDefinitionByID( return &resp.Definition, nil } -// getPolicySetDefinitionByID fetches a policy set definition by its full resource ID. -// It handles built-in, subscription-scoped, and management-group-scoped set definitions. +// getPolicySetDefinitionByID fetches a policy set definition by its full +// resource ID. It handles built-in, subscription-scoped, and +// management-group-scoped set definitions. func getPolicySetDefinitionByID( ctx context.Context, client *armpolicy.SetDefinitionsClient, @@ -246,7 +298,9 @@ func getPolicySetDefinitionByID( ) (*armpolicy.SetDefinition, error) { name := lastSegment(setDefinitionID) if name == "" { - return nil, fmt.Errorf("invalid policy set definition ID: %s", setDefinitionID) + return nil, fmt.Errorf( + "invalid policy set definition ID: %s", setDefinitionID, + ) } if isBuiltInPolicySetDefinition(setDefinitionID) { @@ -272,45 +326,59 @@ func getPolicySetDefinitionByID( return &resp.SetDefinition, nil } -// extractLocalAuthDenyPolicies inspects a policy definition for deny-effect rules -// that target disableLocalAuth fields. It returns any matching policies. -func extractLocalAuthDenyPolicies( +// extractDenyPolicies inspects a policy definition for deny-effect rules +// that target resource property fields. It returns any matching policies. +func extractDenyPolicies( def *armpolicy.Definition, assignmentName string, assignmentParams map[string]any, -) []LocalAuthDenyPolicy { +) []DenyPolicy { if def.Properties == nil || def.Properties.PolicyRule == nil { return nil } ruleMap, ok := def.Properties.PolicyRule.(map[string]any) if !ok { - log.Printf("policy preflight: policy rule is not a map for definition %s (type %T)", - stringOrEmpty(def.Name), def.Properties.PolicyRule) + log.Printf( + "policy preflight: policy rule is not a map "+ + "for definition %s (type %T)", + stringOrEmpty(def.Name), def.Properties.PolicyRule, + ) return nil } - // Check if the effect is "deny" (either literal or via parameter reference). - if !isDenyEffect(ruleMap, def.Properties.Parameters, assignmentParams) { + // Check if the effect is "deny" (literal or via parameter reference). + if !isDenyEffect( + ruleMap, def.Properties.Parameters, assignmentParams, + ) { return nil } - // Parse the "if" condition to find disableLocalAuth field references. + log.Printf( + "policy preflight: definition %q resolved as deny effect "+ + "(assignment=%q)", + stringOrEmpty(def.Name), assignmentName, + ) + + // Parse the "if" condition to find field references. ifBlock, ok := ruleMap["if"] if !ok { return nil } - results := findLocalAuthConditions(ifBlock, assignmentName) + results := findDenyConditions(ifBlock, assignmentName) if len(results) > 0 { - log.Printf("policy preflight: found %d local auth deny condition(s) in policy %q", - len(results), assignmentName) + log.Printf( + "policy preflight: found %d deny condition(s) in policy %q", + len(results), assignmentName, + ) } return results } // isDenyEffect checks whether the policy's effect resolves to "deny". -// Effects can be a literal string or a parameter reference like "[parameters('effect')]". +// Effects can be a literal string or a parameter reference like +// "[parameters('effect')]". func isDenyEffect( ruleMap map[string]any, definitionParams map[string]*armpolicy.ParameterDefinitionsValue, @@ -333,24 +401,33 @@ func isDenyEffect( effectStr, ok := effectVal.(string) if !ok { - log.Printf("policy preflight: effect value is not a string (type %T): %v", effectVal, effectVal) + log.Printf( + "policy preflight: effect value is not a string (type %T): %v", + effectVal, effectVal, + ) return false } // Check for literal deny. if strings.EqualFold(effectStr, "deny") { + log.Printf("policy preflight: effect is literal deny") return true } - // Check for parameter reference: "[parameters('effect')]" or "[parameters('effectName')]". + // Check for parameter reference. paramName := extractParameterReference(effectStr) if paramName == "" { return false } - // First check assignment-level parameters (these override definition defaults). + // First check assignment-level parameters (override definition defaults). if v, ok := assignmentParams[paramName]; ok { if s, ok := v.(string); ok && strings.EqualFold(s, "deny") { + log.Printf( + "policy preflight: effect resolved to deny "+ + "via assignment param %q=%q", + paramName, s, + ) return true } return false @@ -358,8 +435,15 @@ func isDenyEffect( // Fall back to the definition's default value. if definitionParams != nil { - if paramDef, ok := definitionParams[paramName]; ok && paramDef.DefaultValue != nil { - if s, ok := paramDef.DefaultValue.(string); ok && strings.EqualFold(s, "deny") { + if paramDef, ok := definitionParams[paramName]; ok && + paramDef.DefaultValue != nil { + if s, ok := paramDef.DefaultValue.(string); ok && + strings.EqualFold(s, "deny") { + log.Printf( + "policy preflight: effect resolved to deny "+ + "via definition default param %q=%q", + paramName, s, + ) return true } } @@ -368,9 +452,12 @@ func isDenyEffect( return false } -// findLocalAuthConditions traverses the policy rule's "if" block to find conditions -// that reference disableLocalAuth fields and extracts the target resource type. -func findLocalAuthConditions(ifBlock any, assignmentName string) []LocalAuthDenyPolicy { +// findDenyConditions traverses the policy rule's "if" block to find +// conditions that reference resource property fields and extracts the target +// resource type. +func findDenyConditions( + ifBlock any, assignmentName string, +) []DenyPolicy { condMap, ok := ifBlock.(map[string]any) if !ok { return nil @@ -384,22 +471,23 @@ func findLocalAuthConditions(ifBlock any, assignmentName string) []LocalAuthDeny return findInCompoundCondition(anyOf, assignmentName, nil) } - // Single condition — unlikely to be the full pattern but check anyway. + // Single condition. return checkSingleCondition(condMap, assignmentName, nil) } -// findInCompoundCondition processes an allOf/anyOf array looking for conditions that -// reference both a resource type and a disableLocalAuth field. -// parentResourceTypes carries any resource types resolved by an ancestor compound condition. +// findInCompoundCondition processes an allOf/anyOf array looking for +// conditions that reference both a resource type and a property field. +// parentResourceTypes carries any resource types resolved by an ancestor +// compound condition. func findInCompoundCondition( compound any, assignmentName string, parentResourceTypes []string, -) []LocalAuthDenyPolicy { +) []DenyPolicy { conditions, ok := compound.([]any) if !ok { return nil } - // First pass: find resource types from "field: type, equals/in: ..." conditions. + // First pass: find resource types from "field: type" conditions. var resourceTypes []string for _, cond := range conditions { condMap, ok := cond.(map[string]any) @@ -427,45 +515,61 @@ func findInCompoundCondition( resourceTypes = parentResourceTypes } - // Second pass: find disableLocalAuth field references. - var results []LocalAuthDenyPolicy + // Second pass: find property field references. + var results []DenyPolicy for _, cond := range conditions { condMap, ok := cond.(map[string]any) if !ok { continue } - results = append(results, checkSingleCondition(condMap, assignmentName, resourceTypes)...) + results = append( + results, + checkSingleCondition( + condMap, assignmentName, resourceTypes, + )..., + ) // Recurse into nested conditions, passing resolved resource types. if allOf, ok := condMap["allOf"]; ok { - results = append(results, findInCompoundCondition(allOf, assignmentName, resourceTypes)...) + results = append( + results, + findInCompoundCondition( + allOf, assignmentName, resourceTypes, + )..., + ) } if anyOf, ok := condMap["anyOf"]; ok { - results = append(results, findInCompoundCondition(anyOf, assignmentName, resourceTypes)...) + results = append( + results, + findInCompoundCondition( + anyOf, assignmentName, resourceTypes, + )..., + ) } } return results } -// checkSingleCondition checks if a single condition references a disableLocalAuth field. -// resourceTypes are the candidate resource types resolved from sibling conditions. +// checkSingleCondition checks if a single condition references a resource +// property field. resourceTypes are the candidate resource types resolved +// from sibling conditions. func checkSingleCondition( condMap map[string]any, assignmentName string, resourceTypes []string, -) []LocalAuthDenyPolicy { +) []DenyPolicy { fieldVal, ok := condMap["field"].(string) if !ok { return nil } - if !isLocalAuthField(fieldVal) { + if !IsLocalAuthField(fieldVal) { return nil } - // If we don't have resource types from a sibling condition, try to derive one - // from the field path (e.g. "Microsoft.Storage/storageAccounts/allowSharedKeyAccess"). + // If we don't have resource types from a sibling condition, try to + // derive one from the field path. if len(resourceTypes) == 0 { if rt := resourceTypeFromFieldPath(fieldVal); rt != "" { resourceTypes = []string{rt} @@ -477,9 +581,9 @@ func checkSingleCondition( } // Emit one result per resource type. - results := make([]LocalAuthDenyPolicy, 0, len(resourceTypes)) + results := make([]DenyPolicy, 0, len(resourceTypes)) for _, rt := range resourceTypes { - results = append(results, LocalAuthDenyPolicy{ + results = append(results, DenyPolicy{ PolicyName: assignmentName, ResourceType: rt, FieldPath: fieldVal, @@ -488,8 +592,9 @@ func checkSingleCondition( return results } -// isLocalAuthField returns true if the field path references a local authentication property. -func isLocalAuthField(field string) bool { +// IsLocalAuthField returns true if the field path references a local +// authentication property. +func IsLocalAuthField(field string) bool { lower := strings.ToLower(field) return strings.HasSuffix(lower, "/disablelocalauth") || strings.HasSuffix(lower, "/allowsharedkeyaccess") || @@ -497,8 +602,9 @@ func isLocalAuthField(field string) bool { strings.EqualFold(field, "allowSharedKeyAccess") } -// resourceTypeFromFieldPath extracts the resource type from a fully qualified field path. -// For example, "Microsoft.CognitiveServices/accounts/disableLocalAuth" returns +// resourceTypeFromFieldPath extracts the resource type from a fully qualified +// field path. For example, +// "Microsoft.CognitiveServices/accounts/disableLocalAuth" returns // "Microsoft.CognitiveServices/accounts". func resourceTypeFromFieldPath(field string) string { idx := strings.LastIndex(field, "/") @@ -513,15 +619,16 @@ func resourceTypeFromFieldPath(field string) string { return candidate } -// extractParameterReference extracts the parameter name from an ARM template parameter -// reference expression like "[parameters('effect')]". Returns empty if not a parameter reference. +// extractParameterReference extracts the parameter name from an ARM template +// parameter reference expression like "[parameters('effect')]". Returns empty +// if not a parameter reference. func extractParameterReference(expr string) string { trimmed := strings.TrimSpace(expr) lower := strings.ToLower(trimmed) - if !strings.HasPrefix(lower, "[parameters('") || !strings.HasSuffix(lower, "')]") { + if !strings.HasPrefix(lower, "[parameters('") || + !strings.HasSuffix(lower, "')]") { return "" } - // Extract between [parameters(' and ')] inner := trimmed[len("[parameters('"):] before, _, ok := strings.Cut(inner, "')]") if !ok { @@ -530,13 +637,19 @@ func extractParameterReference(expr string) string { return before } -// extractAssignmentParams extracts parameter values from a policy assignment into a simple map. -func extractAssignmentParams(assignment *armpolicy.Assignment) map[string]any { - if assignment.Properties == nil || assignment.Properties.Parameters == nil { +// extractAssignmentParams extracts parameter values from a policy assignment +// into a simple map. +func extractAssignmentParams( + assignment *armpolicy.Assignment, +) map[string]any { + if assignment.Properties == nil || + assignment.Properties.Parameters == nil { return nil } - params := make(map[string]any, len(assignment.Properties.Parameters)) + params := make( + map[string]any, len(assignment.Properties.Parameters), + ) for name, val := range assignment.Properties.Parameters { if val != nil && val.Value != nil { params[name] = val.Value @@ -545,15 +658,21 @@ func extractAssignmentParams(assignment *armpolicy.Assignment) map[string]any { return params } -// mergeParams merges assignment-level parameters with member-level parameter values -// from a policy set definition reference. Member parameters may contain parameter -// references like "[parameters('effect')]" that resolve against the assignment parameters. -func mergeParams(assignmentParams map[string]any, memberParams map[string]*armpolicy.ParameterValuesValue) map[string]any { +// mergeParams merges assignment-level parameters with member-level parameter +// values from a policy set definition reference. Member parameters may contain +// parameter references like "[parameters('effect')]" that resolve against the +// assignment parameters. +func mergeParams( + assignmentParams map[string]any, + memberParams map[string]*armpolicy.ParameterValuesValue, +) map[string]any { if len(memberParams) == 0 { return assignmentParams } - merged := make(map[string]any, len(assignmentParams)+len(memberParams)) + merged := make( + map[string]any, len(assignmentParams)+len(memberParams), + ) maps.Copy(merged, assignmentParams) for name, val := range memberParams { @@ -561,7 +680,8 @@ func mergeParams(assignmentParams map[string]any, memberParams map[string]*armpo continue } - // Check if the member parameter value is itself a reference to an assignment parameter. + // Check if the member parameter value is itself a reference to + // an assignment parameter. if s, ok := val.Value.(string); ok { if refName := extractParameterReference(s); refName != "" { if resolved, ok := assignmentParams[refName]; ok { @@ -577,26 +697,69 @@ func mergeParams(assignmentParams map[string]any, memberParams map[string]*armpo return merged } -// isBuiltInPolicyDefinition returns true if the definition ID is a built-in policy. +// applySetDefaults fills in default values from the policy set definition's +// parameter declarations for any parameters not explicitly set by the +// assignment. This ensures that "Opt In" initiatives whose assignments rely +// on the set's default value (e.g. "Audit") are correctly resolved instead +// of falling through to the member definition's default (often "Deny"). +func applySetDefaults( + assignmentParams map[string]any, + setParams map[string]*armpolicy.ParameterDefinitionsValue, +) map[string]any { + if len(setParams) == 0 { + return assignmentParams + } + + effective := make(map[string]any, len(assignmentParams)+len(setParams)) + maps.Copy(effective, assignmentParams) + + for name, paramDef := range setParams { + if _, alreadySet := effective[name]; alreadySet { + continue + } + if paramDef != nil && paramDef.DefaultValue != nil { + effective[name] = paramDef.DefaultValue + } + } + + return effective +} + +// isBuiltInPolicyDefinition returns true if the definition ID is a built-in +// policy. func isBuiltInPolicyDefinition(id string) bool { - return strings.HasPrefix(strings.ToLower(id), "/providers/microsoft.authorization/policydefinitions/") + return strings.HasPrefix( + strings.ToLower(id), + "/providers/microsoft.authorization/policydefinitions/", + ) } -// isCustomPolicyDefinition returns true if the definition ID is a subscription-scoped custom policy. +// isCustomPolicyDefinition returns true if the definition ID is a +// subscription-scoped custom policy. func isCustomPolicyDefinition(id string) bool { lower := strings.ToLower(id) - return strings.Contains(lower, "/providers/microsoft.authorization/policydefinitions/") && - !isBuiltInPolicyDefinition(id) + return strings.Contains( + lower, + "/providers/microsoft.authorization/policydefinitions/", + ) && !isBuiltInPolicyDefinition(id) } -// isPolicySetDefinition returns true if the definition ID references a policy set (initiative). +// isPolicySetDefinition returns true if the definition ID references a policy +// set (initiative). func isPolicySetDefinition(id string) bool { - return strings.Contains(strings.ToLower(id), "/providers/microsoft.authorization/policysetdefinitions/") + return strings.Contains( + strings.ToLower(id), + "/providers/microsoft.authorization/policysetdefinitions/", + ) } -// isBuiltInPolicySetDefinition returns true if the set definition ID is a built-in policy set. +// isBuiltInPolicySetDefinition returns true if the set definition ID is a +// built-in policy set. func isBuiltInPolicySetDefinition(id string) bool { - return strings.HasPrefix(strings.ToLower(id), "/providers/microsoft.authorization/policysetdefinitions/") + return strings.HasPrefix( + strings.ToLower(id), + "/providers/microsoft.authorization/policysetdefinitions/", + ) } // lastSegment returns the last path segment of a resource ID. @@ -608,8 +771,8 @@ func lastSegment(resourceID string) string { return parts[len(parts)-1] } -// extractManagementGroupID extracts the management group ID from a resource ID like -// "/providers/Microsoft.Management/managementGroups/{mgId}/providers/Microsoft.Authorization/..." +// extractManagementGroupID extracts the management group ID from a resource ID +// like "/providers/Microsoft.Management/managementGroups/{mgId}/providers/...". // Returns empty string if the ID is not management-group-scoped. func extractManagementGroupID(resourceID string) string { lower := strings.ToLower(resourceID) @@ -627,9 +790,11 @@ func extractManagementGroupID(resourceID string) string { } // ResourceHasLocalAuthDisabled checks whether a resource's properties JSON has -// the disableLocalAuth property set to true (or allowSharedKeyAccess set to false -// for storage accounts). -func ResourceHasLocalAuthDisabled(resourceType string, properties json.RawMessage) bool { +// the disableLocalAuth property set to true (or allowSharedKeyAccess set to +// false for storage accounts). +func ResourceHasLocalAuthDisabled( + resourceType string, properties json.RawMessage, +) bool { if len(properties) == 0 { return false } @@ -640,10 +805,12 @@ func ResourceHasLocalAuthDisabled(resourceType string, properties json.RawMessag } // Storage accounts use allowSharedKeyAccess (inverted logic). - if strings.EqualFold(resourceType, "Microsoft.Storage/storageAccounts") { + if strings.EqualFold( + resourceType, "Microsoft.Storage/storageAccounts", + ) { if v, ok := props["allowSharedKeyAccess"]; ok { if b, ok := v.(bool); ok { - return !b // allowSharedKeyAccess=false means local auth is disabled + return !b } } return false diff --git a/cli/azd/pkg/azapi/policy_service_test.go b/cli/azd/pkg/azapi/policy_service_test.go index df7308f5aa5..24af385038e 100644 --- a/cli/azd/pkg/azapi/policy_service_test.go +++ b/cli/azd/pkg/azapi/policy_service_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestExtractLocalAuthDenyPolicies_DenyLiteral(t *testing.T) { +func TestExtractDenyPolicies_DenyLiteral(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -35,14 +35,14 @@ func TestExtractLocalAuthDenyPolicies_DenyLiteral(t *testing.T) { }, } - results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + results := extractDenyPolicies(def, "test-policy", nil) require.Len(t, results, 1) require.Equal(t, "Microsoft.CognitiveServices/accounts", results[0].ResourceType) require.Equal(t, "test-policy", results[0].PolicyName) require.Contains(t, results[0].FieldPath, "disableLocalAuth") } -func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Deny(t *testing.T) { +func TestExtractDenyPolicies_ParameterizedEffect_Deny(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -74,12 +74,12 @@ func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Deny(t *testing.T) { // With assignment params overriding to Deny. assignmentParams := map[string]any{"effect": "Deny"} - results := extractLocalAuthDenyPolicies(def, "test-policy", assignmentParams) + results := extractDenyPolicies(def, "test-policy", assignmentParams) require.Len(t, results, 1) require.Equal(t, "Microsoft.EventHub/namespaces", results[0].ResourceType) } -func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Audit(t *testing.T) { +func TestExtractDenyPolicies_ParameterizedEffect_Audit(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -110,11 +110,11 @@ func TestExtractLocalAuthDenyPolicies_ParameterizedEffect_Audit(t *testing.T) { } // No assignment params — falls back to default "Audit", which is not deny. - results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + results := extractDenyPolicies(def, "test-policy", nil) require.Empty(t, results) } -func TestExtractLocalAuthDenyPolicies_NoLocalAuthField(t *testing.T) { +func TestExtractDenyPolicies_NoLocalAuthField(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -139,11 +139,11 @@ func TestExtractLocalAuthDenyPolicies_NoLocalAuthField(t *testing.T) { }, } - results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + results := extractDenyPolicies(def, "test-policy", nil) require.Empty(t, results) } -func TestExtractLocalAuthDenyPolicies_StorageAllowSharedKeyAccess(t *testing.T) { +func TestExtractDenyPolicies_StorageAllowSharedKeyAccess(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -168,12 +168,12 @@ func TestExtractLocalAuthDenyPolicies_StorageAllowSharedKeyAccess(t *testing.T) }, } - results := extractLocalAuthDenyPolicies(def, "test-policy", nil) + results := extractDenyPolicies(def, "test-policy", nil) require.Len(t, results, 1) require.Equal(t, "Microsoft.Storage/storageAccounts", results[0].ResourceType) } -func TestExtractLocalAuthDenyPolicies_NestedAnyOf(t *testing.T) { +func TestExtractDenyPolicies_NestedAnyOf(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -206,12 +206,12 @@ func TestExtractLocalAuthDenyPolicies_NestedAnyOf(t *testing.T) { }, } - results := extractLocalAuthDenyPolicies(def, "test-nested", nil) + results := extractDenyPolicies(def, "test-nested", nil) require.NotEmpty(t, results) require.Equal(t, "Microsoft.ServiceBus/namespaces", results[0].ResourceType) } -func TestExtractLocalAuthDenyPolicies_MultipleResourceTypesInArray(t *testing.T) { +func TestExtractDenyPolicies_MultipleResourceTypesInArray(t *testing.T) { t.Parallel() def := &armpolicy.Definition{ @@ -239,7 +239,7 @@ func TestExtractLocalAuthDenyPolicies_MultipleResourceTypesInArray(t *testing.T) }, } - results := extractLocalAuthDenyPolicies(def, "multi-type-policy", nil) + results := extractDenyPolicies(def, "multi-type-policy", nil) require.Len(t, results, 2) types := make(map[string]bool) @@ -250,11 +250,12 @@ func TestExtractLocalAuthDenyPolicies_MultipleResourceTypesInArray(t *testing.T) require.True(t, types["Microsoft.ServiceBus/namespaces"]) } -func TestExtractLocalAuthDenyPolicies_NestedConditionInheritsResourceType(t *testing.T) { +func TestExtractDenyPolicies_NestedConditionInheritsResourceType(t *testing.T) { t.Parallel() - // The resource type is declared at the outer allOf level, and the disableLocalAuth - // condition is in a nested anyOf. The nested level should inherit the resource type. + // The resource type is declared at the outer allOf level, and the + // disableLocalAuth condition is in a nested anyOf. The nested level + // should inherit the resource type. def := &armpolicy.Definition{ Properties: &armpolicy.DefinitionProperties{ PolicyRule: map[string]any{ @@ -281,7 +282,7 @@ func TestExtractLocalAuthDenyPolicies_NestedConditionInheritsResourceType(t *tes }, } - results := extractLocalAuthDenyPolicies(def, "nested-inherit", nil) + results := extractDenyPolicies(def, "nested-inherit", nil) require.Len(t, results, 1) require.Equal(t, "Microsoft.Search/searchServices", results[0].ResourceType) } @@ -309,12 +310,14 @@ func TestExtractManagementGroupID(t *testing.T) { }, { "built-in policy definition", - "/providers/Microsoft.Authorization/policyDefinitions/6300012e-e9a4-4649-b41f-a85f5c43be91", + "/providers/Microsoft.Authorization/policyDefinitions/" + + "6300012e-e9a4-4649-b41f-a85f5c43be91", "", }, { "subscription scoped custom definition", - "/subscriptions/faa080af/providers/Microsoft.Authorization/policyDefinitions/custom123", + "/subscriptions/faa080af/providers/" + + "Microsoft.Authorization/policyDefinitions/custom123", "", }, { @@ -339,12 +342,27 @@ func TestIsLocalAuthField(t *testing.T) { field string want bool }{ - {"Microsoft.CognitiveServices/accounts/disableLocalAuth", true}, - {"Microsoft.EventHub/namespaces/disableLocalAuth", true}, - {"Microsoft.Storage/storageAccounts/allowSharedKeyAccess", true}, - {"Microsoft.ServiceBus/namespaces/disableLocalAuth", true}, + { + "Microsoft.CognitiveServices/accounts/disableLocalAuth", + true, + }, + { + "Microsoft.EventHub/namespaces/disableLocalAuth", + true, + }, + { + "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", + true, + }, + { + "Microsoft.ServiceBus/namespaces/disableLocalAuth", + true, + }, {"disableLocalAuth", true}, - {"Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", false}, + { + "Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", + false, + }, {"type", false}, {"location", false}, } @@ -352,7 +370,7 @@ func TestIsLocalAuthField(t *testing.T) { for _, tt := range tests { t.Run(tt.field, func(t *testing.T) { t.Parallel() - require.Equal(t, tt.want, isLocalAuthField(tt.field)) + require.Equal(t, tt.want, IsLocalAuthField(tt.field)) }) } } @@ -364,8 +382,14 @@ func TestResourceTypeFromFieldPath(t *testing.T) { field string want string }{ - {"Microsoft.CognitiveServices/accounts/disableLocalAuth", "Microsoft.CognitiveServices/accounts"}, - {"Microsoft.EventHub/namespaces/disableLocalAuth", "Microsoft.EventHub/namespaces"}, + { + "Microsoft.CognitiveServices/accounts/disableLocalAuth", + "Microsoft.CognitiveServices/accounts", + }, + { + "Microsoft.EventHub/namespaces/disableLocalAuth", + "Microsoft.EventHub/namespaces", + }, {"disableLocalAuth", ""}, {"", ""}, } @@ -486,14 +510,22 @@ func TestIsDenyEffect(t *testing.T) { }, { "parameterized deny via assignment", - map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + map[string]any{ + "then": map[string]any{ + "effect": "[parameters('effect')]", + }, + }, nil, map[string]any{"effect": "Deny"}, true, }, { "parameterized deny via default", - map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + map[string]any{ + "then": map[string]any{ + "effect": "[parameters('effect')]", + }, + }, map[string]*armpolicy.ParameterDefinitionsValue{ "effect": {DefaultValue: "Deny"}, }, @@ -501,7 +533,11 @@ func TestIsDenyEffect(t *testing.T) { }, { "parameterized audit via default", - map[string]any{"then": map[string]any{"effect": "[parameters('effect')]"}}, + map[string]any{ + "then": map[string]any{ + "effect": "[parameters('effect')]", + }, + }, map[string]*armpolicy.ParameterDefinitionsValue{ "effect": {DefaultValue: "Audit"}, }, @@ -517,7 +553,14 @@ func TestIsDenyEffect(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - require.Equal(t, tt.want, isDenyEffect(tt.ruleMap, tt.definitionParams, tt.assignmentParams)) + require.Equal( + t, tt.want, + isDenyEffect( + tt.ruleMap, + tt.definitionParams, + tt.assignmentParams, + ), + ) }) } } diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 5a017fec756..6599d17a771 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2179,10 +2179,25 @@ func (p *BicepProvider) validatePreflight( Fn: p.checkRoleAssignmentPermissions, }) - // Register the local auth policy check. This detects Azure Policy assignments - // that deny resources with local authentication enabled (disableLocalAuth != true) - // and warns if the template contains affected resources. - localPreflight.AddCheck(p.checkLocalAuthPolicy) + // Register the storage account policy check. This uses deny policies + // fetched from the subscription's policy assignments (including + // management-group-inherited policies) to detect restrictions that + // would block deployment of storage accounts. + localPreflight.AddCheck(p.checkStorageAccountPolicy) + + // Set up the deny policy fetch function so policies are loaded in + // parallel with the Bicep snapshot. + localPreflight.SetDenyPoliciesFn( + func(ctx context.Context) ([]azapi.DenyPolicy, error) { + var policyService *azapi.PolicyService + if err := p.serviceLocator.Resolve(&policyService); err != nil { + return nil, err + } + return policyService.FindDenyPolicies( + ctx, p.env.GetSubscriptionId(), + ) + }, + ) results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) if err != nil { @@ -2244,6 +2259,7 @@ func (p *BicepProvider) validatePreflight( } if report.HasWarnings() { + p.console.Message(ctx, "") continueDeployment, promptErr := p.console.Confirm(ctx, input.ConsoleOptions{ Message: "Preflight validation found warnings that may cause the " + "deployment to fail. Do you want to continue?", @@ -2404,84 +2420,73 @@ func (p *BicepProvider) resolveResourceTenantPrincipalId( return principalId, nil } -// checkLocalAuthPolicy is a PreflightCheckFn that detects Azure Policy assignments on the -// subscription that deny resources with local authentication enabled. If any template resources -// have disableLocalAuth unset or false while a deny policy exists for that resource type, -// a warning is returned so the user knows the deployment will be blocked. -func (p *BicepProvider) checkLocalAuthPolicy( +// checkStorageAccountPolicy is a PreflightCheckFn that uses deny policies from +// the subscription's policy assignments to detect policies requiring local +// authentication to be disabled on storage accounts. It filters the +// pre-fetched deny policies to those targeting storage accounts with local +// auth fields and warns when any storage account in the template doesn't +// have local authentication disabled. +func (p *BicepProvider) checkStorageAccountPolicy( ctx context.Context, valCtx *validationContext, ) (*PreflightCheckResult, error) { - if len(valCtx.SnapshotResources) == 0 { - log.Printf("policy preflight: no snapshot resources, skipping local auth policy check") - return nil, nil + // Collect storage accounts from the snapshot. + var storageAccounts []armTemplateResource + for _, resource := range valCtx.SnapshotResources { + if strings.EqualFold( + resource.Type, "Microsoft.Storage/storageAccounts", + ) { + storageAccounts = append(storageAccounts, resource) + } } - - var policyService *azapi.PolicyService - if err := p.serviceLocator.Resolve(&policyService); err != nil { - log.Printf("could not resolve PolicyService, skipping local auth policy check: %v", err) + if len(storageAccounts) == 0 { return nil, nil } - subscriptionId := p.env.GetSubscriptionId() - - denyPolicies, err := policyService.FindLocalAuthDenyPolicies(ctx, subscriptionId) - if err != nil { - log.Printf("error checking local auth policies, skipping check: %v", err) - return nil, nil + // Filter deny policies to those targeting storage accounts with + // local auth fields. + hasStoragePolicy := false + for _, policy := range valCtx.DenyPolicies { + if strings.EqualFold( + policy.ResourceType, + "Microsoft.Storage/storageAccounts", + ) && azapi.IsLocalAuthField(policy.FieldPath) { + hasStoragePolicy = true + break + } } - - log.Printf("policy preflight: found %d deny policies targeting local auth", len(denyPolicies)) - - if len(denyPolicies) == 0 { + if !hasStoragePolicy { return nil, nil } - // Build a map of resource types to their deny policies (may have multiple per type). - policiesByType := make(map[string][]azapi.LocalAuthDenyPolicy) - for _, policy := range denyPolicies { - key := strings.ToLower(policy.ResourceType) - policiesByType[key] = append(policiesByType[key], policy) - } - - // Log snapshot resource types for diagnostics. - for _, resource := range valCtx.SnapshotResources { - _, matched := policiesByType[strings.ToLower(resource.Type)] - hasLocalAuthDisabled := azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) - log.Printf("policy preflight: resource %q type=%s policyMatch=%v localAuthDisabled=%v", - resource.Name, resource.Type, matched, hasLocalAuthDisabled) - } - - // Collect affected resource types (deduplicated). - affectedTypes := make(map[string]bool) - for _, resource := range valCtx.SnapshotResources { - _, found := policiesByType[strings.ToLower(resource.Type)] - if !found { - continue - } - - if !azapi.ResourceHasLocalAuthDisabled(resource.Type, resource.Properties) { - affectedTypes[resource.Type] = true + // Count how many storage accounts don't have local auth disabled. + affectedCount := 0 + for _, account := range storageAccounts { + if !azapi.ResourceHasLocalAuthDisabled( + account.Type, account.Properties, + ) { + affectedCount++ } } - - if len(affectedTypes) == 0 { + if affectedCount == 0 { return nil, nil } - typeList := slices.Sorted(maps.Keys(affectedTypes)) - var lines []string - for _, t := range typeList { - lines = append(lines, fmt.Sprintf(" - %s", t)) + msg := "an Azure Policy on this subscription requires storage accounts " + + "to disable local authentication (shared key access). " + if affectedCount == 1 { + msg += "1 storage account in this deployment does not have " + + "'allowSharedKeyAccess' set to false." + } else { + msg += fmt.Sprintf( + "%d storage accounts in this deployment do not have "+ + "'allowSharedKeyAccess' set to false.", + affectedCount, + ) } return &PreflightCheckResult{ Severity: PreflightCheckWarning, - Message: fmt.Sprintf( - "an Azure Policy on this subscription denies resources with local authentication enabled. "+ - "The following resource types in this deployment may be blocked:\n%s\n"+ - "Disable local authentication on these resources or request a policy exemption.", - strings.Join(lines, "\n"), - ), + Message: msg, }, nil } diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go index 46d7b373f0c..d10a6e8a06c 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go @@ -14,7 +14,9 @@ import ( "path/filepath" "slices" "strings" + "sync" + "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/azure" "github.com/azure/azure-dev/cli/azd/pkg/infra" "github.com/azure/azure-dev/cli/azd/pkg/input" @@ -265,10 +267,6 @@ const ( type PreflightCheckResult struct { // Severity indicates whether this result is a warning or a blocking error. Severity PreflightCheckSeverity - // DiagnosticID is a unique, stable identifier for this specific finding type - // (e.g. "role_assignment_missing"). Used in telemetry to correlate actioned - // warnings with deployment outcomes and to track error frequency over time. - DiagnosticID string // Message is a human-readable description of the finding. Message string } @@ -280,6 +278,9 @@ type validationContext struct { Console input.Console // Props contains derived properties from analyzing the ARM template resources. Props resourcesProperties + // Target is the deployment scope (subscription or resource group). + // It may be nil when the deployment scope is unknown. + Target infra.Deployment // ResourcesSnapshot is the raw JSON output from `bicep snapshot`, containing the fully // resolved deployment graph. It may be nil if the Bicep CLI was not available. ResourcesSnapshot json.RawMessage @@ -287,6 +288,10 @@ type validationContext struct { // Each entry represents a resource that would be deployed, with resolved values. // It may be nil if the Bicep CLI was not available. SnapshotResources []armTemplateResource + // DenyPolicies contains deny-effect policies fetched from the subscription's + // policy assignments, including inherited policies from management groups. + // Populated in parallel with the snapshot. + DenyPolicies []azapi.DenyPolicy } // snapshotResult represents the top-level structure of the Bicep snapshot JSON output. @@ -304,17 +309,6 @@ type PreflightCheckFn func( valCtx *validationContext, ) (*PreflightCheckResult, error) -// PreflightCheck pairs a unique rule identifier with its check function. -// The RuleID is a stable, unique string used in telemetry to identify which rule -// produced a result (e.g. for crash tracking). Each rule may emit results with -// different DiagnosticIDs to distinguish specific finding types. -type PreflightCheck struct { - // RuleID is a unique, stable identifier for the rule (e.g. "role_assignment_permissions"). - RuleID string - // Fn is the check function that performs the validation. - Fn PreflightCheckFn -} - // localArmPreflight provides local (client-side) validation of an ARM template before deployment. // It parses the template and parameters to build a comprehensive view of all resources that would // be deployed, enabling early detection of issues without making Azure API calls. @@ -330,7 +324,10 @@ type localArmPreflight struct { // target is the deployment scope (subscription or resource group) used to derive snapshot options. // It may be nil, in which case snapshot options are left empty. target infra.Deployment - checks []PreflightCheck + checks []PreflightCheckFn + // denyPoliciesFn fetches deny policies for the subscription. + // Called in parallel with the snapshot. May be nil. + denyPoliciesFn func(ctx context.Context) ([]azapi.DenyPolicy, error) } // newLocalArmPreflight creates a new instance of localArmPreflight. @@ -341,10 +338,18 @@ func newLocalArmPreflight(modulePath string, bicepCli *bicep.Cli, target infra.D return &localArmPreflight{modulePath: modulePath, bicepCli: bicepCli, target: target} } -// AddCheck registers a preflight check to be executed during validate. -// Checks are invoked in the order they are added. -func (l *localArmPreflight) AddCheck(check PreflightCheck) { - l.checks = append(l.checks, check) +// AddCheck registers a preflight check function to be executed during validate. +// Check functions are invoked in the order they are added. +func (l *localArmPreflight) AddCheck(fn PreflightCheckFn) { + l.checks = append(l.checks, fn) +} + +// SetDenyPoliciesFn sets a function that fetches deny policies in parallel +// with the Bicep snapshot. If not set, no policies are fetched. +func (l *localArmPreflight) SetDenyPoliciesFn( + fn func(ctx context.Context) ([]azapi.DenyPolicy, error), +) { + l.denyPoliciesFn = fn } // validate performs local preflight validation on the given ARM template and parameters. @@ -405,19 +410,51 @@ func (l *localArmPreflight) validate( } } - // Run the Bicep snapshot command to produce a deployment snapshot from the bicepparam file. - // The snapshot contains the fully resolved deployment graph with expressions evaluated, - // conditions applied, and copy loops expanded. - // If the snapshot fails (e.g., older Bicep binary without snapshot support), skip local - // preflight rather than blocking the deployment. - data, err := l.bicepCli.Snapshot(ctx, bicepParamFile, snapshotOpts) - if err != nil { - log.Printf("local preflight: skipping checks, bicep snapshot unavailable: %v", err) + // Run the Bicep snapshot and policy fetch in parallel. + // The snapshot produces the fully resolved deployment graph; the policy fetch + // retrieves deny-effect policies from the subscription's assignments (including + // management-group-inherited policies). + console.ShowSpinner(ctx, "Validating deployment [setup]", input.Step) + + var wg sync.WaitGroup + var snapshotData []byte + var snapshotErr error + var denyPolicies []azapi.DenyPolicy + + wg.Go(func() { + snapshotData, snapshotErr = l.bicepCli.Snapshot( + ctx, bicepParamFile, snapshotOpts, + ) + }) + + if l.denyPoliciesFn != nil { + wg.Go(func() { + policies, err := l.denyPoliciesFn(ctx) + if err != nil { + log.Printf( + "policy preflight: failed to fetch deny policies: %v", + err, + ) + return // non-fatal + } + denyPolicies = policies + }) + } + + wg.Wait() + + // If the snapshot fails (e.g., older Bicep binary without snapshot + // support), skip local preflight rather than blocking the deployment. + if snapshotErr != nil { + log.Printf( + "local preflight: skipping checks, bicep snapshot unavailable: %v", + snapshotErr, + ) return nil, nil } var snapshot snapshotResult - if err := json.Unmarshal(data, &snapshot); err != nil { + if err := json.Unmarshal(snapshotData, &snapshot); err != nil { return nil, fmt.Errorf("parsing bicep snapshot: %w", err) } @@ -426,15 +463,19 @@ func (l *localArmPreflight) validate( valCtx := &validationContext{ Console: console, Props: props, - ResourcesSnapshot: json.RawMessage(data), + Target: l.target, + ResourcesSnapshot: json.RawMessage(snapshotData), SnapshotResources: snapshot.PredictedResources, + DenyPolicies: denyPolicies, } + console.ShowSpinner(ctx, "Validating deployment [checks]", input.Step) + var results []PreflightCheckResult for _, check := range l.checks { - result, err := check.Fn(ctx, valCtx) + result, err := check(ctx, valCtx) if err != nil { - return results, fmt.Errorf("preflight check %q failed: %w", check.RuleID, err) + return results, fmt.Errorf("preflight check failed: %w", err) } if result != nil { results = append(results, *result) From 08b1fae397b3ca4dc9996ffe3553b7a831ec410d Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Mon, 30 Mar 2026 21:21:21 +0000 Subject: [PATCH 09/13] Address PR review feedback: revert launch.json, remove unrelated doc, improve policy loop - Revert .vscode/launch.json: restore parameterized ${input:cliArgs} and remove developer-specific cwd path - Remove cli/azd/docs/design/extension-flag-handling.md: unrelated to this PR - Add context cancellation check at top of policy assignment iteration loop - Remove now-unused isCustomPolicyDefinition function Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .vscode/launch.json | 3 +- .../docs/design/extension-flag-handling.md | 291 ------------------ cli/azd/pkg/azapi/policy_service.go | 17 +- 3 files changed, 6 insertions(+), 305 deletions(-) delete mode 100644 cli/azd/docs/design/extension-flag-handling.md diff --git a/.vscode/launch.json b/.vscode/launch.json index 949c4b379ed..eb889afec27 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -25,9 +25,8 @@ "request": "launch", "mode": "debug", "program": "${workspaceFolder}/cli/azd", - "args": "up", + "args": "${input:cliArgs}", "console": "integratedTerminal", - "cwd": "/home/vivazqu/workspace/build/debug/preflight-agentic" } ], "inputs": [ diff --git a/cli/azd/docs/design/extension-flag-handling.md b/cli/azd/docs/design/extension-flag-handling.md deleted file mode 100644 index ae645d0cb11..00000000000 --- a/cli/azd/docs/design/extension-flag-handling.md +++ /dev/null @@ -1,291 +0,0 @@ -# Extension Flag Handling Design - -> **Status**: Design summary of current implementation (not a prescriptive spec). -> Extracted from the codebase as of March 2026 to document the existing behavior and -> call out undefined or ambiguous scenarios. - -## Overview - -When a user runs an extension command (e.g. `azd myext --my-flag value -e dev`), flags -must be split between two consumers: - -| Consumer | Needs | Examples | -|----------|-------|---------| -| **azd host** | Global flags that control host behavior | `--debug`, `--cwd`, `-e`, `--no-prompt` | -| **Extension process** | Extension-specific flags + positional args | `--my-flag value`, `--model gpt-4` | - -Today there is **no explicit separator** (like `--`) between the two. Instead, azd uses -a combination of early pre-parsing and Cobra's `DisableFlagParsing` to handle this. - -## Current Implementation - -### Phase 1: Early Pre-Parsing (`ParseGlobalFlags`) - -**File**: `cmd/auto_install.go` — `ParseGlobalFlags()` - -Before Cobra builds the command tree, `ParseGlobalFlags()` extracts known azd global -flags from the raw `os.Args` using a pflag FlagSet with `ParseErrorsAllowlist{UnknownFlags: true}`. -Unknown flags (extension flags) are silently ignored. - -**Flags parsed in this phase:** - -| Flag | Short | Stored in `GlobalCommandOptions` | -|------|-------|----------------------------------| -| `--cwd` | `-C` | `Cwd` | -| `--debug` | — | `EnableDebugLogging` | -| `--no-prompt` | — | `NoPrompt` | -| `--trace-log-file` | — | *(read by telemetry system)* | -| `--trace-log-url` | — | *(read by telemetry system)* | - -**Not parsed here**: `-e` / `--environment` is currently absent from `CreateGlobalFlagSet()`. -It is registered as a per-command flag on commands that need it (via `envFlag` binding), -not as a global flag. This is the root cause of [#7034](https://github.com/Azure/azure-dev/issues/7034). - -The result is stored in a `GlobalCommandOptions` singleton registered in the IoC container. - -**Agent detection**: If `--no-prompt` was not explicitly set, `ParseGlobalFlags` also -checks `agentdetect.IsRunningInAgent()` and auto-enables `NoPrompt` for AI coding agents. - -### Phase 2: Cobra Command Registration - -**File**: `cmd/extensions.go` — `bindExtension()` - -Extension commands are registered with: - -```go -cmd := &cobra.Command{ - Use: lastPart, - DisableFlagParsing: true, -} -``` - -`DisableFlagParsing: true` means Cobra will **not** parse any flags for these commands. -All tokens after the command name are passed to the action handler as raw `args`. - -**Consequence**: Cobra's persistent flags (including `-e`/`--environment` if registered) -are not parsed for extension commands. Calls to `cmd.Flags().GetString("environment")` -silently return `""`. - -### Phase 3: Extension Action Execution - -**File**: `cmd/extensions.go` — `extensionAction.Run()` - -The action handler builds `InvokeOptions` from two sources: - -1. **Cobra persistent flags** (`cmd.Flags().Get*`) — for `debug`, `cwd`, `environment` -2. **`GlobalCommandOptions`** — for `NoPrompt` (because it includes agent detection) - -```go -debugEnabled, _ := a.cmd.Flags().GetBool("debug") -cwd, _ := a.cmd.Flags().GetString("cwd") -envName, _ := a.cmd.Flags().GetString("environment") - -options := &extensions.InvokeOptions{ - Args: a.args, // ALL raw args (extension + global flags) - Debug: debugEnabled, - NoPrompt: a.globalOptions.NoPrompt, - Cwd: cwd, - Environment: envName, -} -``` - -**Problem**: Because `DisableFlagParsing: true`, `cmd.Flags().Get*` calls for `debug`, -`cwd`, and `environment` return zero values. Only `NoPrompt` works correctly because it -reads from `globalOptions` (populated in Phase 1). - -### Phase 4: Environment Variable Propagation - -**File**: `pkg/extensions/runner.go` — `Runner.Invoke()` - -Global flag values from `InvokeOptions` are converted to environment variables before -spawning the extension process: - -| `InvokeOptions` field | Environment variable | Condition | -|-----------------------|---------------------|-----------| -| `Debug` | `AZD_DEBUG=true` | if true | -| `NoPrompt` | `AZD_NO_PROMPT=true` | if true | -| `Cwd` | `AZD_CWD=` | if non-empty | -| `Environment` | `AZD_ENVIRONMENT=` | if non-empty | - -The extension process also receives: -- `AZD_SERVER=:` — gRPC server address -- `AZD_ACCESS_TOKEN=` — Authentication token -- `TRACEPARENT=` — W3C trace context (if tracing is active) -- Environment variables from the resolved azd environment (`env.Environ()`) - -### Phase 5: Middleware Extensions (Listener Path) - -**File**: `cmd/middleware/extensions.go` — `ExtensionsMiddleware.Run()` - -Lifecycle extensions (those with `lifecycle-events`, `service-target-provider`, or -`framework-service-provider` capabilities) use a different execution path: - -- Started with fixed args `["listen"]` -- Read global flags from `m.options.Flags` (Cobra persistent flags from the *parent* command) -- Also have access to `m.globalOptions` - -This path works for standard azd commands (like `azd deploy`) where Cobra *does* parse -persistent flags. The middleware invocation constructs `InvokeOptions` similarly. - -## What Extensions Receive - -When a user runs: `azd myext --my-flag value -e dev --debug` - -The extension process gets: - -| Channel | Content | -|---------|---------| -| **Command-line args** | `["--my-flag", "value", "-e", "dev", "--debug"]` — ALL tokens after the command name, including azd global flags | -| **Environment variables** | `AZD_DEBUG`, `AZD_NO_PROMPT`, `AZD_CWD`, `AZD_ENVIRONMENT` — set only if their `InvokeOptions` field is non-empty | -| **gRPC services** | Extension can call `EnvironmentService.GetCurrentEnvironment()` to get the resolved environment | - -**Key observation**: Global azd flags appear in **both** the raw args and environment -variables. Extensions must decide which source to trust. - -## Extension-Side Flag Parsing - -Extensions are expected to: - -1. **Read global azd flags from environment variables** (`AZD_DEBUG`, `AZD_NO_PROMPT`, - `AZD_CWD`, `AZD_ENVIRONMENT`), not from their command-line args. -2. **Parse their own flags** from the command-line args they receive. The azd SDK provides - helpers (`azdext` package) for this. -3. **Ignore azd global flags in their args** — these are artifacts of `DisableFlagParsing` - and should not be interpreted by the extension. - -However, this contract is **not formally documented or enforced**. Extensions using the -Go SDK with Cobra will likely re-parse all args including the azd global flags. - -## Undefined Scenarios and Edge Cases - -### 1. Flag Name Collisions - -**Scenario**: Extension defines a flag with the same name as an azd global flag -(e.g., `--debug` meaning "debug output format" in the extension). - -**Current behavior**: `ParseGlobalFlags` will consume `--debug` and set -`GlobalCommandOptions.EnableDebugLogging = true`. The extension will also see `--debug` -in its raw args and may interpret it differently. - -**Impact**: Both azd and the extension act on the same flag with different semantics. -There is no way to say "this `--debug` is for the extension, not azd". - -### 2. Global Flags Not Stripped From Extension Args - -**Scenario**: User runs `azd myext -e dev --debug --my-flag value`. - -**Current behavior**: Extension receives args `["-e", "dev", "--debug", "--my-flag", "value"]`. -The azd global flags are **not** removed from the args before passing to the extension. - -**Impact**: If the extension uses a strict flag parser, unknown flags like `--debug` may -cause parse errors. Extensions must use permissive parsing or explicitly handle azd flags. - -### 3. No `--` Separator Convention - -**Scenario**: User wants to pass `--debug` to the extension but not to azd. - -**Current behavior**: No mechanism exists. `ParseGlobalFlags` will always consume -recognized flags regardless of position. - -**Potential design**: A `--` separator could be introduced: -`azd --debug myext -- --debug` (first `--debug` for azd, second for extension). -This is not implemented. - -### 4. `-e` / `--environment` Not Pre-Parsed - -**Scenario**: User runs `azd myext -e dev`. - -**Current behavior** (main branch): `-e` is NOT in `CreateGlobalFlagSet()`, so -`ParseGlobalFlags` ignores it. `cmd.Flags().GetString("environment")` returns `""` -due to `DisableFlagParsing`. Result: `AZD_ENVIRONMENT` is never set, and `lazyEnv` -resolves to the default environment. - -**Impact**: This is the bug described in [#7034](https://github.com/Azure/azure-dev/issues/7034). -The default environment's variables leak into the extension process. - -### 5. Environment Variable Injection From Default Environment - -**Scenario**: Extension commands inject `lazyEnv.Environ()` (the resolved azd -environment's key-value pairs) into the extension process's environment. - -**Question**: Should extensions receive environment variables from the azd environment -at all, or should they exclusively use the gRPC `EnvironmentService`? - -**Current behavior**: `extensionAction.Run()` calls `a.lazyEnv.GetValue()` and appends -all key-value pairs as process environment variables. This means the extension inherits -every value from the resolved azd environment. - -**Trade-off**: Injecting env vars is convenient (extensions "just work" with standard -`os.Getenv`), but it creates an implicit coupling and makes it hard to distinguish -azd-injected variables from system environment variables. - -### 6. `--cwd` Semantics - -**Scenario**: User runs `azd myext -C ./subdir --my-flag value`. - -**Current behavior**: `ParseGlobalFlags` captures `Cwd = "./subdir"`. The root command -changes the process working directory before the extension runs. `AZD_CWD=./subdir` is -set in the extension's environment. - -**Question**: Should the extension receive the *original* cwd or the *resolved* cwd? -Currently it receives the raw flag value (potentially relative), but the process has -already `cd`'d into that directory. - -### 7. Middleware vs Command Extension Flag Parity - -**Scenario**: A lifecycle extension (middleware path) needs the `-e` environment name. - -**Current behavior**: Middleware reads `m.options.Flags.GetString("environment")` from -Cobra persistent flags of the parent command (e.g., `azd deploy`). This works because -the parent command *does* parse flags. - -**Gap**: The two execution paths (command extensions vs middleware extensions) have -different flag resolution strategies. Command extensions cannot rely on Cobra flags; -middleware extensions can. - -### 8. Help Text for Extension Commands - -**Scenario**: User runs `azd myext --help`. - -**Current behavior**: Because `DisableFlagParsing: true`, Cobra does not intercept -`--help`. The `--help` flag is passed as an arg to the extension. The extension must -handle it. Cobra's auto-generated help (which would show global flags) is not displayed. - -**Impact**: Users don't see a consistent help experience between azd commands and -extension commands. Global flags like `-e`, `--debug` are not listed in extension help. - -## Recommendations - -These are observations about gaps in the current design, not prescriptive changes: - -1. **Document the flag contract**: Extensions should know definitively whether to read - global flags from env vars or args. Today this is implicit. - -2. **Consider stripping global flags from extension args**: If azd globals are communicated - via env vars, they arguably should not appear in the extension's command-line args. - This would eliminate collision issues. However, this would be a breaking change for - extensions that currently parse `-e` from their args. - -3. **Consider a `--` separator**: `azd [azd-flags] myext -- [ext-flags]` would make the - boundary explicit. This is a convention used by many CLI tools (npm, cargo, kubectl). - -4. **Unify flag resolution**: The divergence between command extensions (can't use Cobra - flags) and middleware extensions (can use Cobra flags) is a source of bugs. A single - resolution path through `GlobalCommandOptions` would be more robust. - -5. **Document `AZD_ENVIRONMENT`, `AZD_DEBUG`, `AZD_NO_PROMPT`, `AZD_CWD`**: These - environment variables are set for extension processes but not listed in - `docs/environment-variables.md`. - -## File Reference - -| File | Role | -|------|------| -| `cmd/auto_install.go` | `CreateGlobalFlagSet()`, `ParseGlobalFlags()` | -| `cmd/extensions.go` | `bindExtension()` (DisableFlagParsing), `extensionAction.Run()` | -| `cmd/container.go` | `EnvFlag` DI resolver (environment name resolution chain) | -| `cmd/middleware/extensions.go` | Lifecycle extension invocation (middleware path) | -| `cmd/root.go` | Command tree construction, global flag registration | -| `pkg/extensions/runner.go` | `InvokeOptions`, env var propagation, process spawning | -| `internal/global_command_options.go` | `GlobalCommandOptions` struct | -| `internal/env_flag.go` | `EnvFlag` type, `EnvironmentNameFlagName` constant | diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go index 199719d4e95..45116cbd01f 100644 --- a/cli/azd/pkg/azapi/policy_service.go +++ b/cli/azd/pkg/azapi/policy_service.go @@ -115,6 +115,10 @@ func (s *PolicyService) FindDenyPolicies( setDefCache := make(map[string]*armpolicy.SetDefinition) for _, assignment := range assignments { + if err := ctx.Err(); err != nil { + return nil, err + } + if assignment.Properties == nil || assignment.Properties.PolicyDefinitionID == nil { continue @@ -130,8 +134,7 @@ func (s *PolicyService) FindDenyPolicies( // parameterized effects. assignmentParams := extractAssignmentParams(assignment) - if isBuiltInPolicyDefinition(defID) || - isCustomPolicyDefinition(defID) { + if !isPolicySetDefinition(defID) { policies := s.checkPolicyDefinition( ctx, definitionsClient, defID, assignmentName, assignmentParams, defCache, @@ -734,16 +737,6 @@ func isBuiltInPolicyDefinition(id string) bool { ) } -// isCustomPolicyDefinition returns true if the definition ID is a -// subscription-scoped custom policy. -func isCustomPolicyDefinition(id string) bool { - lower := strings.ToLower(id) - return strings.Contains( - lower, - "/providers/microsoft.authorization/policydefinitions/", - ) && !isBuiltInPolicyDefinition(id) -} - // isPolicySetDefinition returns true if the definition ID references a policy // set (initiative). func isPolicySetDefinition(id string) bool { From 119c9039e3ee68fa743361570694db19c90278b7 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Mon, 30 Mar 2026 22:32:04 +0000 Subject: [PATCH 10/13] Remove storage account policy check, add investigation docs Remove the storage account local auth policy check due to false positives that cannot be resolved with current Azure APIs: - Client-side armpolicy parsing sees management-group policies but cannot evaluate ARM expressions in policy conditions (subscription tags, region opt-in gates), causing false warnings on subscriptions where the policy does not actually apply. - Server-side checkPolicyRestrictions API correctly evaluates all conditions but does not see management-group-inherited policies, missing real denials. Move local preflight docs to cli/azd/docs/local-preflight/ and add a detailed investigation writeup documenting both approaches, the specific failure modes, and why neither approach is viable today. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/.vscode/cspell.yaml | 8 - cli/azd/cmd/container.go | 1 - .../README.md} | 10 + .../storage-account-policy-check.md | 219 +++++ cli/azd/go.mod | 5 +- cli/azd/go.sum | 10 +- cli/azd/pkg/azapi/policy_service.go | 828 ------------------ cli/azd/pkg/azapi/policy_service_test.go | 566 ------------ .../provisioning/bicep/bicep_provider.go | 91 -- .../provisioning/bicep/local_preflight.go | 76 +- 10 files changed, 245 insertions(+), 1569 deletions(-) rename cli/azd/docs/{design/local-preflight-validation.md => local-preflight/README.md} (94%) create mode 100644 cli/azd/docs/local-preflight/storage-account-policy-check.md delete mode 100644 cli/azd/pkg/azapi/policy_service.go delete mode 100644 cli/azd/pkg/azapi/policy_service_test.go diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index 5c3e9d239ec..bb200afb9c5 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -357,14 +357,6 @@ overrides: - filename: "{pkg/azapi/permissions.go,pkg/infra/provisioning/bicep/bicep_provider.go}" words: - ABAC - - filename: pkg/azapi/policy_service.go - words: - - armpolicy - - disablelocalauth - - allowsharedkeyaccess - - policydefinitions - - policysetdefinitions - - managementgroups - filename: extensions/azure.ai.models/internal/cmd/custom_create.go words: - Qwen diff --git a/cli/azd/cmd/container.go b/cli/azd/cmd/container.go index 53651c9d313..95cd937c48e 100644 --- a/cli/azd/cmd/container.go +++ b/cli/azd/cmd/container.go @@ -699,7 +699,6 @@ func registerCommonDependencies(container *ioc.NestedContainer) { // Tools container.MustRegisterSingleton(azapi.NewResourceService) container.MustRegisterSingleton(azapi.NewPermissionsService) - container.MustRegisterSingleton(azapi.NewPolicyService) container.MustRegisterSingleton(docker.NewCli) container.MustRegisterSingleton(dotnet.NewCli) container.MustRegisterSingleton(git.NewCli) diff --git a/cli/azd/docs/design/local-preflight-validation.md b/cli/azd/docs/local-preflight/README.md similarity index 94% rename from cli/azd/docs/design/local-preflight-validation.md rename to cli/azd/docs/local-preflight/README.md index fc30e257bec..7c8af4453c3 100644 --- a/cli/azd/docs/design/local-preflight-validation.md +++ b/cli/azd/docs/local-preflight/README.md @@ -122,6 +122,16 @@ localPreflight.AddCheck(func(ctx context.Context, valCtx *validationContext) (*P |---|---|---| | Role assignment permissions | Detects `Microsoft.Authorization/roleAssignments` in the snapshot and verifies the current principal has `roleAssignments/write` permission on the subscription. | Warning | +### Investigated Checks (Not Implemented) + +The following checks were investigated but not shipped due to technical +limitations. Each link leads to a detailed writeup of the investigation, +including the approaches tried and why they were not viable. + +| Check | Goal | Reason Not Implemented | +|---|---|---| +| [Storage account policy check](storage-account-policy-check.md) | Warn when Azure Policy denies storage accounts with local authentication enabled (`allowSharedKeyAccess: true`). | Client-side policy parsing produces false positives (cannot evaluate ARM expressions in policy conditions); server-side `checkPolicyRestrictions` API does not evaluate management-group-inherited policies. | + ## UX Presentation Results are displayed using the `PreflightReport` UX component (`pkg/output/ux/preflight_report.go`), which implements the standard `UxItem` interface. The report groups and orders findings: all warnings appear first, followed by all errors. Each entry is prefixed with the standard azd status icons. diff --git a/cli/azd/docs/local-preflight/storage-account-policy-check.md b/cli/azd/docs/local-preflight/storage-account-policy-check.md new file mode 100644 index 00000000000..9bfddfe2b39 --- /dev/null +++ b/cli/azd/docs/local-preflight/storage-account-policy-check.md @@ -0,0 +1,219 @@ +# Storage Account Policy Check — Investigation Summary + +> **Status**: Not implemented — false positive rate too high for the warning to be actionable. +> Investigation completed March 2026. + +## Goal + +Detect Azure Policy assignments that deny storage account deployments when local +authentication (shared key access) is enabled. Warn users **before** deployment so +they can set `allowSharedKeyAccess: false` in their Bicep templates instead of +waiting for a late `RequestDisallowedByPolicy` failure. + +## Background + +Enterprise subscriptions commonly enforce security policies through Azure Policy +at the management group level. A typical deny policy targeting storage accounts +looks like: + +```json +{ + "if": { + "allOf": [ + { "field": "type", "equals": "Microsoft.Storage/storageAccounts" }, + { + "anyOf": [ + { "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", "exists": "false" }, + { "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", "equals": "true" } + ] + } + ] + }, + "then": { "effect": "deny" } +} +``` + +When a template deploys a storage account without `allowSharedKeyAccess: false`, +ARM returns: + +``` +RequestDisallowedByPolicy: Resource 'st-contoso-dev-01' was disallowed by policy. +``` + +This error appears only after the deployment has been submitted and partially +processed (often several minutes into the operation). + +## Approaches Investigated + +### Approach 1: Client-Side Policy Rule Parsing (ARM Policy SDK) + +**How it works:** + +1. List all policy assignments for the subscription using + `AssignmentsClient.NewListPager()` from the ARM policy SDK. This returns assignments from + all scopes — subscription-level, resource-group-level, and inherited from + parent management groups. +2. Fetch each assignment's policy definition (with caching) and inspect the rule + for `field` conditions targeting `allowSharedKeyAccess` or `disableLocalAuth`. +3. Resolve parameterized effects (e.g. `[parameters('effect')]`) using the + assignment's parameter values. +4. If a deny-effect policy targets storage accounts with local auth fields, and + the Bicep snapshot contains storage accounts without `allowSharedKeyAccess: + false`, emit a warning. + +**Result: Detects real denials but produces false positives.** + +The list pager correctly returns management-group-inherited assignments. This is +critical because enterprise deny policies almost always originate from management +groups, not from the subscription itself. The SDK handles the inheritance +automatically. + +However, enterprise policies often include **gating conditions** that the +client-side parser cannot evaluate: + +- **Opt-in tags**: `[contains(subscription().tags, parameters('optInTagName'))]` + — the policy only applies if the subscription has a specific tag. +- **Region gates**: `[split(subscription().tags[parameters('optInTagName')], ',')]` + — the policy applies only in regions the subscription has opted into. +- **Skip tags**: `[concat('tags[', parameters('skipTagName'), ']')]` — resources + with a specific tag are exempt. +- **Resource group filters**: `[resourceGroup().managedBy]` — managed resource + groups are exempt. + +These are ARM template expressions (`[...]`) that require runtime context +(subscription tags, resource group metadata) to evaluate. The client-side parser +sees the deny-effect rule and the `allowSharedKeyAccess` field, but cannot +determine whether the gating conditions would exclude the deployment. + +**Example — false positive scenario:** + +A management group assigns the policy "Storage — Disable Local Auth (Opt-In)" +with `effect: deny`. The policy rule includes: + +```json +{ + "allOf": [ + { "field": "type", "equals": "Microsoft.Storage/storageAccounts" }, + { "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", "equals": "true" }, + { "value": "[contains(subscription().tags, parameters('optInTagName'))]", "equals": "false" }, + { "anyOf": [ + { "field": "location", "in": "[split(subscription().tags[parameters('optInTagName')], ',')]" }, + { "value": "all_regions", "in": "[split(subscription().tags[parameters('optInTagName')], ',')]" } + ]} + ] +} +``` + +The assignment sets `optInTagName` = `"Az.Sec.DisableLocalAuth.Storage::OptIn"`. + +A subscription `contoso-dev-sub` does **not** have the +`Az.Sec.DisableLocalAuth.Storage::OptIn` tag. The opt-in `anyOf` block evaluates +to `false`, so the policy never fires — deployments with `allowSharedKeyAccess: +true` succeed. + +The client-side parser cannot evaluate `subscription().tags[...]` or `split()` +expressions. It sees the deny + `allowSharedKeyAccess` pattern and warns the +user, even though the policy would not actually block the deployment. + +On a different subscription `contoso-prod-sub` with +`Az.Sec.DisableLocalAuth.Storage::OptIn = all_regions`, the same policy **does** +fire, and the warning would be correct. + +Both subscriptions produce identical deny policy detection results from the +client-side parser — it cannot distinguish between them. + +**Considered mitigations:** + +- *Evaluate `contains(subscription().tags, ...)` by fetching subscription tags*: + Handles the opt-in pattern but not the full range of ARM expressions (region + gating with `split()`, `resourceGroup().managedBy`, `count`/`where` blocks). + Each new expression pattern would require custom parsing logic, creating an + ever-growing mini ARM expression evaluator. +- *Skip policies with conditions that cannot be evaluated locally*: Would suppress the false positive + but also suppress warnings for real denials. The `"Storage Accounts - Safe + Secrets Standard"` policy (which **does** block deployments) uses the same + ARM expression patterns for skip-tag exemptions. + +### Approach 2: Server-Side Policy Evaluation (`checkPolicyRestrictions` API) + +**How it works:** + +The `Microsoft.PolicyInsights` resource provider offers a +`checkPolicyRestrictions` API that evaluates hypothetical resources against all +assigned policies server-side. You submit a resource's content (type, location, +properties) and Azure returns which policies would deny it and why. + +``` +POST /subscriptions/{id}/providers/Microsoft.PolicyInsights/checkPolicyRestrictions + ?api-version=2022-03-01 + +{ + "resourceDetails": { + "resourceContent": { + "type": "Microsoft.Storage/storageAccounts", + "location": "eastus2", + "properties": { "allowSharedKeyAccess": true } + }, + "apiVersion": "2023-05-01" + } +} +``` + +The API: +- Evaluates ALL conditions (ARM expressions, tag lookups, region gates) +- Handles exemptions and parameter overrides +- Returns policy evaluation results and field-level restrictions +- Requires only `Microsoft.PolicyInsights/*/read` (included in Reader role) + +**Result: Accurate evaluation but does not see management-group-inherited policies.** + +Testing against subscriptions with management-group-assigned deny policies +confirmed that the API returns **empty results** (`policyEvaluations: []`, +`fieldRestrictions: []`) even when: + +- The deny policy is confirmed active (deployments fail with + `RequestDisallowedByPolicy`) +- The subscription tags satisfy all opt-in conditions +- The resource content explicitly sets the denied property +- Both subscription-scope and resource-group-scope endpoints are tried +- A `PendingFields` parameter is included in the request + +The API only evaluates policies assigned directly at the subscription or resource +group scope. Since enterprise storage deny policies originate from management +groups, `checkPolicyRestrictions` does not evaluate them. + +### Approach 3: Policy States API (`policyStates`) + +The `policyStates` API (`Microsoft.PolicyInsights/policyStates/latest/queryResults`) +evaluates compliance of **existing** resources. It sees management-group-inherited +policies and correctly reports non-compliance. + +However, it cannot evaluate **hypothetical** resources from a Bicep snapshot. It +only works with resources that have already been deployed. Since the goal is to +warn *before* deployment, this API is not applicable. + +## Conclusion + +| Approach | Sees MG policies | Evaluates all conditions | Suitable | +|---|---|---|---| +| Client-side ARM policy SDK parsing | ✅ Yes | ❌ No (ARM expressions) | ❌ False positives | +| Server-side `checkPolicyRestrictions` | ❌ No | ✅ Yes | ❌ Misses real denials | +| `policyStates` API | ✅ Yes | ✅ Yes | ❌ Existing resources only | + +No currently available approach provides both accurate policy detection +(including management-group-inherited policies) and correct evaluation of +complex policy conditions for hypothetical resources. The check was not shipped +to avoid showing incorrect warnings that would erode user trust in the preflight +system. + +## Future Considerations + +- If `checkPolicyRestrictions` is updated to evaluate management-group-inherited + policies, it would be the ideal solution — accurate server-side evaluation with + minimal client complexity. +- A hybrid approach (client-side detection + subscription tag evaluation for + common patterns) could reduce false positives for the most common gating + conditions, at the cost of maintaining a partial ARM expression evaluator. +- The ARM deployment validation API (`/validate`) does evaluate policies but + requires submitting the full deployment, making it equivalent to the + server-side preflight that already runs after local preflight. diff --git a/cli/azd/go.mod b/cli/azd/go.mod index ae24df71c73..6414505dd78 100644 --- a/cli/azd/go.mod +++ b/cli/azd/go.mod @@ -5,7 +5,7 @@ go 1.26.0 require ( dario.cat/mergo v1.0.2 github.com/AlecAivazis/survey/v2 v2.3.7 - github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 + github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/apimanagement/armapimanagement v1.1.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appconfiguration/armappconfiguration v1.1.1 @@ -22,7 +22,6 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/operationalinsights/armoperationalinsights/v2 v2.0.2 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql/v2 v2.0.0-beta.7 @@ -47,7 +46,7 @@ require ( github.com/fsnotify/fsnotify v1.9.0 github.com/github/copilot-sdk/go v0.1.32 github.com/gofrs/flock v0.12.1 - github.com/golang-jwt/jwt/v5 v5.3.1 + github.com/golang-jwt/jwt/v5 v5.3.0 github.com/golobby/container/v3 v3.3.2 github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 diff --git a/cli/azd/go.sum b/cli/azd/go.sum index cbec8c3261e..71c301de039 100644 --- a/cli/azd/go.sum +++ b/cli/azd/go.sum @@ -3,8 +3,8 @@ dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ= github.com/AlecAivazis/survey/v2 v2.3.7/go.mod h1:xUTIdE4KCOIjsBAE1JYsUPoCqYdZ1reCfTwbto0Fduo= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 h1:fou+2+WFTib47nS+nz/ozhEBnvU96bKHy6LjRsY4E28= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0/go.mod h1:t76Ruy8AHvUAC8GfMWJMa0ElSbuIcO03NLpynfbgsPA= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 h1:JXg2dwJUmPB9JmtVmdEB16APJ7jurfbY5jnfXpJoRMc= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0/go.mod h1:YD5h/ldMsG0XiIw7PdyNhLxaM317eFh5yNLccNfGdyw= github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 h1:Hk5QBxZQC1jb2Fwj6mpzme37xbCDdNTxU7O9eb5+LB4= github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1/go.mod h1:IYus9qsFobWIc2YVwe/WPjcnyCkPKtnHAqUYeebc8z0= github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.2 h1:yz1bePFlP5Vws5+8ez6T3HWXPmwOK7Yvq8QxDBD3SKY= @@ -49,8 +49,6 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourceg github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0/go.mod h1:wVEOJfGTj0oPAUGA1JuRAvz/lxXQsWW16axmHPP47Bk= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1 h1:bcgO/crpp7wqI0Froi/I4C2fme7Vk/WLusbV399Do8I= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks v1.0.1/go.mod h1:kvfPmsE8gpOwwC1qrO1FeyBDDNfnwBN5UU3MPNiWW7I= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0 h1:FKYUd0TZfjjWtdk7LR+6rI64yNY1gMe9rNV40Y2Fh4o= -github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy v1.0.0/go.mod h1:w987ZDGBg74HZu5sAqaBEhDTlvlJlxPggIhBiDviYhE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 h1:Dd+RhdJn0OTtVGaeDLZpcumkIVCtA/3/Fo42+eoYvVM= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0/go.mod h1:5kakwfW5CjC9KK+Q4wjXAg+ShuIm2mBMua0ZFj2C8PE= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 h1:wxQx2Bt4xzPIKvW59WQf1tJNx/ZZKPfN+EhPX3Z6CYY= @@ -163,8 +161,8 @@ github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E= github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0= github.com/gofrs/uuid v3.3.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= -github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= -github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= +github.com/golang-jwt/jwt/v5 v5.3.0 h1:pv4AsKCKKZuqlgs5sUmn4x8UlGa0kEVt/puTpKx9vvo= +github.com/golang-jwt/jwt/v5 v5.3.0/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= diff --git a/cli/azd/pkg/azapi/policy_service.go b/cli/azd/pkg/azapi/policy_service.go deleted file mode 100644 index 45116cbd01f..00000000000 --- a/cli/azd/pkg/azapi/policy_service.go +++ /dev/null @@ -1,828 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -package azapi - -import ( - "context" - "encoding/json" - "fmt" - "log" - "maps" - "strings" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" - "github.com/azure/azure-dev/cli/azd/pkg/account" -) - -// DenyPolicy describes a deny-effect policy that targets a specific field on -// an Azure resource type (e.g. disableLocalAuth, allowSharedKeyAccess). -type DenyPolicy struct { - // PolicyName is the display name of the policy assignment. - PolicyName string - // ResourceType is the Azure resource type targeted - // (e.g. "Microsoft.CognitiveServices/accounts"). - ResourceType string - // FieldPath is the full field path checked - // (e.g. "Microsoft.CognitiveServices/accounts/disableLocalAuth"). - FieldPath string -} - -// PolicyService queries Azure Policy assignments and definitions to detect -// deny-effect policies that would block resource deployment. -type PolicyService struct { - credentialProvider account.SubscriptionCredentialProvider - armClientOptions *arm.ClientOptions -} - -// NewPolicyService creates a new PolicyService. -func NewPolicyService( - credentialProvider account.SubscriptionCredentialProvider, - armClientOptions *arm.ClientOptions, -) *PolicyService { - return &PolicyService{ - credentialProvider: credentialProvider, - armClientOptions: armClientOptions, - } -} - -// FindDenyPolicies lists policy assignments on the subscription (including -// inherited assignments from management groups) and inspects their definitions -// for deny-effect rules that target resource property fields. -// It returns a list of matching policies with their target resource types. -func (s *PolicyService) FindDenyPolicies( - ctx context.Context, - subscriptionId string, -) ([]DenyPolicy, error) { - credential, err := s.credentialProvider.CredentialForSubscription( - ctx, subscriptionId, - ) - if err != nil { - return nil, fmt.Errorf( - "getting credential for subscription %s: %w", - subscriptionId, err, - ) - } - - assignmentsClient, err := armpolicy.NewAssignmentsClient( - subscriptionId, credential, s.armClientOptions, - ) - if err != nil { - return nil, fmt.Errorf( - "creating policy assignments client: %w", err, - ) - } - - definitionsClient, err := armpolicy.NewDefinitionsClient( - subscriptionId, credential, s.armClientOptions, - ) - if err != nil { - return nil, fmt.Errorf( - "creating policy definitions client: %w", err, - ) - } - - setDefinitionsClient, err := armpolicy.NewSetDefinitionsClient( - subscriptionId, credential, s.armClientOptions, - ) - if err != nil { - return nil, fmt.Errorf( - "creating policy set definitions client: %w", err, - ) - } - - // List all policy assignments for the subscription. - var assignments []*armpolicy.Assignment - pager := assignmentsClient.NewListPager(nil) - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return nil, fmt.Errorf("listing policy assignments: %w", err) - } - assignments = append(assignments, page.Value...) - } - log.Printf( - "policy preflight: found %d policy assignments for subscription %s", - len(assignments), subscriptionId, - ) - - var results []DenyPolicy - - // Cache fetched definitions/set definitions to avoid duplicate API - // calls when multiple assignments reference the same definition. - defCache := make(map[string]*armpolicy.Definition) - setDefCache := make(map[string]*armpolicy.SetDefinition) - - for _, assignment := range assignments { - if err := ctx.Err(); err != nil { - return nil, err - } - - if assignment.Properties == nil || - assignment.Properties.PolicyDefinitionID == nil { - continue - } - - defID := *assignment.Properties.PolicyDefinitionID - assignmentName := "" - if assignment.Properties.DisplayName != nil { - assignmentName = *assignment.Properties.DisplayName - } - - // Resolve the assignment's parameter values so we can evaluate - // parameterized effects. - assignmentParams := extractAssignmentParams(assignment) - - if !isPolicySetDefinition(defID) { - policies := s.checkPolicyDefinition( - ctx, definitionsClient, defID, assignmentName, - assignmentParams, defCache, - ) - results = append(results, policies...) - } else if isPolicySetDefinition(defID) { - policies := s.checkPolicySetDefinition( - ctx, setDefinitionsClient, definitionsClient, - defID, assignmentName, assignmentParams, - defCache, setDefCache, - ) - results = append(results, policies...) - } - } - - return results, nil -} - -// checkPolicyDefinition fetches a single policy definition (using cache) and -// inspects it for deny rules targeting resource property fields. -func (s *PolicyService) checkPolicyDefinition( - ctx context.Context, - client *armpolicy.DefinitionsClient, - definitionID string, - assignmentName string, - assignmentParams map[string]any, - cache map[string]*armpolicy.Definition, -) []DenyPolicy { - definition, ok := cache[definitionID] - if !ok { - var err error - definition, err = getPolicyDefinitionByID( - ctx, client, definitionID, - ) - if err != nil { - log.Printf( - "policy preflight: could not fetch policy definition %s: %v", - definitionID, err, - ) - cache[definitionID] = nil - return nil - } - cache[definitionID] = definition - } - if definition == nil { - return nil - } - - return extractDenyPolicies( - definition, assignmentName, assignmentParams, - ) -} - -// checkPolicySetDefinition fetches a policy set definition (initiative) and -// checks each of its member policy definitions for deny rules. -func (s *PolicyService) checkPolicySetDefinition( - ctx context.Context, - setClient *armpolicy.SetDefinitionsClient, - defClient *armpolicy.DefinitionsClient, - setDefinitionID string, - assignmentName string, - assignmentParams map[string]any, - defCache map[string]*armpolicy.Definition, - setDefCache map[string]*armpolicy.SetDefinition, -) []DenyPolicy { - setDef, ok := setDefCache[setDefinitionID] - if !ok { - var err error - setDef, err = getPolicySetDefinitionByID( - ctx, setClient, setDefinitionID, - ) - if err != nil { - log.Printf( - "policy preflight: could not fetch policy set definition %s: %v", - setDefinitionID, err, - ) - setDefCache[setDefinitionID] = nil - return nil - } - setDefCache[setDefinitionID] = setDef - } - if setDef == nil { - return nil - } - - if setDef.Properties == nil || - setDef.Properties.PolicyDefinitions == nil { - return nil - } - - // Build effective parameters by applying the set definition's defaults - // for any parameters not explicitly set by the assignment. Without this, - // "Opt In" policies whose assignment relies on the set's default (e.g. - // "Audit") would incorrectly fall back to the member definition's - // default (often "Deny"). - effectiveParams := applySetDefaults( - assignmentParams, setDef.Properties.Parameters, - ) - - var results []DenyPolicy - for _, member := range setDef.Properties.PolicyDefinitions { - if member.PolicyDefinitionID == nil { - continue - } - - // Merge set-level parameters with member-level parameter values. - memberParams := mergeParams(effectiveParams, member.Parameters) - - policies := s.checkPolicyDefinition( - ctx, defClient, *member.PolicyDefinitionID, - assignmentName, memberParams, defCache, - ) - results = append(results, policies...) - } - - return results -} - -// getPolicyDefinitionByID fetches a policy definition by its full resource ID. -// It handles built-in, subscription-scoped, and management-group-scoped -// definitions. -func getPolicyDefinitionByID( - ctx context.Context, - client *armpolicy.DefinitionsClient, - definitionID string, -) (*armpolicy.Definition, error) { - name := lastSegment(definitionID) - if name == "" { - return nil, fmt.Errorf( - "invalid policy definition ID: %s", definitionID, - ) - } - - if isBuiltInPolicyDefinition(definitionID) { - resp, err := client.GetBuiltIn(ctx, name, nil) - if err != nil { - return nil, err - } - return &resp.Definition, nil - } - - if mgID := extractManagementGroupID(definitionID); mgID != "" { - resp, err := client.GetAtManagementGroup(ctx, mgID, name, nil) - if err != nil { - return nil, err - } - return &resp.Definition, nil - } - - resp, err := client.Get(ctx, name, nil) - if err != nil { - return nil, err - } - return &resp.Definition, nil -} - -// getPolicySetDefinitionByID fetches a policy set definition by its full -// resource ID. It handles built-in, subscription-scoped, and -// management-group-scoped set definitions. -func getPolicySetDefinitionByID( - ctx context.Context, - client *armpolicy.SetDefinitionsClient, - setDefinitionID string, -) (*armpolicy.SetDefinition, error) { - name := lastSegment(setDefinitionID) - if name == "" { - return nil, fmt.Errorf( - "invalid policy set definition ID: %s", setDefinitionID, - ) - } - - if isBuiltInPolicySetDefinition(setDefinitionID) { - resp, err := client.GetBuiltIn(ctx, name, nil) - if err != nil { - return nil, err - } - return &resp.SetDefinition, nil - } - - if mgID := extractManagementGroupID(setDefinitionID); mgID != "" { - resp, err := client.GetAtManagementGroup(ctx, mgID, name, nil) - if err != nil { - return nil, err - } - return &resp.SetDefinition, nil - } - - resp, err := client.Get(ctx, name, nil) - if err != nil { - return nil, err - } - return &resp.SetDefinition, nil -} - -// extractDenyPolicies inspects a policy definition for deny-effect rules -// that target resource property fields. It returns any matching policies. -func extractDenyPolicies( - def *armpolicy.Definition, - assignmentName string, - assignmentParams map[string]any, -) []DenyPolicy { - if def.Properties == nil || def.Properties.PolicyRule == nil { - return nil - } - - ruleMap, ok := def.Properties.PolicyRule.(map[string]any) - if !ok { - log.Printf( - "policy preflight: policy rule is not a map "+ - "for definition %s (type %T)", - stringOrEmpty(def.Name), def.Properties.PolicyRule, - ) - return nil - } - - // Check if the effect is "deny" (literal or via parameter reference). - if !isDenyEffect( - ruleMap, def.Properties.Parameters, assignmentParams, - ) { - return nil - } - - log.Printf( - "policy preflight: definition %q resolved as deny effect "+ - "(assignment=%q)", - stringOrEmpty(def.Name), assignmentName, - ) - - // Parse the "if" condition to find field references. - ifBlock, ok := ruleMap["if"] - if !ok { - return nil - } - - results := findDenyConditions(ifBlock, assignmentName) - if len(results) > 0 { - log.Printf( - "policy preflight: found %d deny condition(s) in policy %q", - len(results), assignmentName, - ) - } - return results -} - -// isDenyEffect checks whether the policy's effect resolves to "deny". -// Effects can be a literal string or a parameter reference like -// "[parameters('effect')]". -func isDenyEffect( - ruleMap map[string]any, - definitionParams map[string]*armpolicy.ParameterDefinitionsValue, - assignmentParams map[string]any, -) bool { - thenBlock, ok := ruleMap["then"] - if !ok { - return false - } - - thenMap, ok := thenBlock.(map[string]any) - if !ok { - return false - } - - effectVal, ok := thenMap["effect"] - if !ok { - return false - } - - effectStr, ok := effectVal.(string) - if !ok { - log.Printf( - "policy preflight: effect value is not a string (type %T): %v", - effectVal, effectVal, - ) - return false - } - - // Check for literal deny. - if strings.EqualFold(effectStr, "deny") { - log.Printf("policy preflight: effect is literal deny") - return true - } - - // Check for parameter reference. - paramName := extractParameterReference(effectStr) - if paramName == "" { - return false - } - - // First check assignment-level parameters (override definition defaults). - if v, ok := assignmentParams[paramName]; ok { - if s, ok := v.(string); ok && strings.EqualFold(s, "deny") { - log.Printf( - "policy preflight: effect resolved to deny "+ - "via assignment param %q=%q", - paramName, s, - ) - return true - } - return false - } - - // Fall back to the definition's default value. - if definitionParams != nil { - if paramDef, ok := definitionParams[paramName]; ok && - paramDef.DefaultValue != nil { - if s, ok := paramDef.DefaultValue.(string); ok && - strings.EqualFold(s, "deny") { - log.Printf( - "policy preflight: effect resolved to deny "+ - "via definition default param %q=%q", - paramName, s, - ) - return true - } - } - } - - return false -} - -// findDenyConditions traverses the policy rule's "if" block to find -// conditions that reference resource property fields and extracts the target -// resource type. -func findDenyConditions( - ifBlock any, assignmentName string, -) []DenyPolicy { - condMap, ok := ifBlock.(map[string]any) - if !ok { - return nil - } - - // Check for allOf / anyOf compound conditions. - if allOf, ok := condMap["allOf"]; ok { - return findInCompoundCondition(allOf, assignmentName, nil) - } - if anyOf, ok := condMap["anyOf"]; ok { - return findInCompoundCondition(anyOf, assignmentName, nil) - } - - // Single condition. - return checkSingleCondition(condMap, assignmentName, nil) -} - -// findInCompoundCondition processes an allOf/anyOf array looking for -// conditions that reference both a resource type and a property field. -// parentResourceTypes carries any resource types resolved by an ancestor -// compound condition. -func findInCompoundCondition( - compound any, assignmentName string, parentResourceTypes []string, -) []DenyPolicy { - conditions, ok := compound.([]any) - if !ok { - return nil - } - - // First pass: find resource types from "field: type" conditions. - var resourceTypes []string - for _, cond := range conditions { - condMap, ok := cond.(map[string]any) - if !ok { - continue - } - - fieldVal, _ := condMap["field"].(string) - if strings.EqualFold(fieldVal, "type") { - if eq, ok := condMap["equals"].(string); ok { - resourceTypes = append(resourceTypes, eq) - } - if in, ok := condMap["in"].([]any); ok { - for _, item := range in { - if s, ok := item.(string); ok { - resourceTypes = append(resourceTypes, s) - } - } - } - } - } - - // Merge with parent resource types if this level didn't find any. - if len(resourceTypes) == 0 { - resourceTypes = parentResourceTypes - } - - // Second pass: find property field references. - var results []DenyPolicy - for _, cond := range conditions { - condMap, ok := cond.(map[string]any) - if !ok { - continue - } - results = append( - results, - checkSingleCondition( - condMap, assignmentName, resourceTypes, - )..., - ) - - // Recurse into nested conditions, passing resolved resource types. - if allOf, ok := condMap["allOf"]; ok { - results = append( - results, - findInCompoundCondition( - allOf, assignmentName, resourceTypes, - )..., - ) - } - if anyOf, ok := condMap["anyOf"]; ok { - results = append( - results, - findInCompoundCondition( - anyOf, assignmentName, resourceTypes, - )..., - ) - } - } - - return results -} - -// checkSingleCondition checks if a single condition references a resource -// property field. resourceTypes are the candidate resource types resolved -// from sibling conditions. -func checkSingleCondition( - condMap map[string]any, - assignmentName string, - resourceTypes []string, -) []DenyPolicy { - fieldVal, ok := condMap["field"].(string) - if !ok { - return nil - } - - if !IsLocalAuthField(fieldVal) { - return nil - } - - // If we don't have resource types from a sibling condition, try to - // derive one from the field path. - if len(resourceTypes) == 0 { - if rt := resourceTypeFromFieldPath(fieldVal); rt != "" { - resourceTypes = []string{rt} - } - } - - if len(resourceTypes) == 0 { - return nil - } - - // Emit one result per resource type. - results := make([]DenyPolicy, 0, len(resourceTypes)) - for _, rt := range resourceTypes { - results = append(results, DenyPolicy{ - PolicyName: assignmentName, - ResourceType: rt, - FieldPath: fieldVal, - }) - } - return results -} - -// IsLocalAuthField returns true if the field path references a local -// authentication property. -func IsLocalAuthField(field string) bool { - lower := strings.ToLower(field) - return strings.HasSuffix(lower, "/disablelocalauth") || - strings.HasSuffix(lower, "/allowsharedkeyaccess") || - strings.EqualFold(field, "disableLocalAuth") || - strings.EqualFold(field, "allowSharedKeyAccess") -} - -// resourceTypeFromFieldPath extracts the resource type from a fully qualified -// field path. For example, -// "Microsoft.CognitiveServices/accounts/disableLocalAuth" returns -// "Microsoft.CognitiveServices/accounts". -func resourceTypeFromFieldPath(field string) string { - idx := strings.LastIndex(field, "/") - if idx <= 0 { - return "" - } - candidate := field[:idx] - // Basic validation: resource types contain at least one "/". - if !strings.Contains(candidate, "/") { - return "" - } - return candidate -} - -// extractParameterReference extracts the parameter name from an ARM template -// parameter reference expression like "[parameters('effect')]". Returns empty -// if not a parameter reference. -func extractParameterReference(expr string) string { - trimmed := strings.TrimSpace(expr) - lower := strings.ToLower(trimmed) - if !strings.HasPrefix(lower, "[parameters('") || - !strings.HasSuffix(lower, "')]") { - return "" - } - inner := trimmed[len("[parameters('"):] - before, _, ok := strings.Cut(inner, "')]") - if !ok { - return "" - } - return before -} - -// extractAssignmentParams extracts parameter values from a policy assignment -// into a simple map. -func extractAssignmentParams( - assignment *armpolicy.Assignment, -) map[string]any { - if assignment.Properties == nil || - assignment.Properties.Parameters == nil { - return nil - } - - params := make( - map[string]any, len(assignment.Properties.Parameters), - ) - for name, val := range assignment.Properties.Parameters { - if val != nil && val.Value != nil { - params[name] = val.Value - } - } - return params -} - -// mergeParams merges assignment-level parameters with member-level parameter -// values from a policy set definition reference. Member parameters may contain -// parameter references like "[parameters('effect')]" that resolve against the -// assignment parameters. -func mergeParams( - assignmentParams map[string]any, - memberParams map[string]*armpolicy.ParameterValuesValue, -) map[string]any { - if len(memberParams) == 0 { - return assignmentParams - } - - merged := make( - map[string]any, len(assignmentParams)+len(memberParams), - ) - maps.Copy(merged, assignmentParams) - - for name, val := range memberParams { - if val == nil || val.Value == nil { - continue - } - - // Check if the member parameter value is itself a reference to - // an assignment parameter. - if s, ok := val.Value.(string); ok { - if refName := extractParameterReference(s); refName != "" { - if resolved, ok := assignmentParams[refName]; ok { - merged[name] = resolved - continue - } - } - } - - merged[name] = val.Value - } - - return merged -} - -// applySetDefaults fills in default values from the policy set definition's -// parameter declarations for any parameters not explicitly set by the -// assignment. This ensures that "Opt In" initiatives whose assignments rely -// on the set's default value (e.g. "Audit") are correctly resolved instead -// of falling through to the member definition's default (often "Deny"). -func applySetDefaults( - assignmentParams map[string]any, - setParams map[string]*armpolicy.ParameterDefinitionsValue, -) map[string]any { - if len(setParams) == 0 { - return assignmentParams - } - - effective := make(map[string]any, len(assignmentParams)+len(setParams)) - maps.Copy(effective, assignmentParams) - - for name, paramDef := range setParams { - if _, alreadySet := effective[name]; alreadySet { - continue - } - if paramDef != nil && paramDef.DefaultValue != nil { - effective[name] = paramDef.DefaultValue - } - } - - return effective -} - -// isBuiltInPolicyDefinition returns true if the definition ID is a built-in -// policy. -func isBuiltInPolicyDefinition(id string) bool { - return strings.HasPrefix( - strings.ToLower(id), - "/providers/microsoft.authorization/policydefinitions/", - ) -} - -// isPolicySetDefinition returns true if the definition ID references a policy -// set (initiative). -func isPolicySetDefinition(id string) bool { - return strings.Contains( - strings.ToLower(id), - "/providers/microsoft.authorization/policysetdefinitions/", - ) -} - -// isBuiltInPolicySetDefinition returns true if the set definition ID is a -// built-in policy set. -func isBuiltInPolicySetDefinition(id string) bool { - return strings.HasPrefix( - strings.ToLower(id), - "/providers/microsoft.authorization/policysetdefinitions/", - ) -} - -// lastSegment returns the last path segment of a resource ID. -func lastSegment(resourceID string) string { - parts := strings.Split(resourceID, "/") - if len(parts) == 0 { - return "" - } - return parts[len(parts)-1] -} - -// extractManagementGroupID extracts the management group ID from a resource ID -// like "/providers/Microsoft.Management/managementGroups/{mgId}/providers/...". -// Returns empty string if the ID is not management-group-scoped. -func extractManagementGroupID(resourceID string) string { - lower := strings.ToLower(resourceID) - const prefix = "/providers/microsoft.management/managementgroups/" - idx := strings.Index(lower, prefix) - if idx < 0 { - return "" - } - rest := resourceID[idx+len(prefix):] - before, _, ok := strings.Cut(rest, "/") - if !ok { - return rest - } - return before -} - -// ResourceHasLocalAuthDisabled checks whether a resource's properties JSON has -// the disableLocalAuth property set to true (or allowSharedKeyAccess set to -// false for storage accounts). -func ResourceHasLocalAuthDisabled( - resourceType string, properties json.RawMessage, -) bool { - if len(properties) == 0 { - return false - } - - var props map[string]any - if err := json.Unmarshal(properties, &props); err != nil { - return false - } - - // Storage accounts use allowSharedKeyAccess (inverted logic). - if strings.EqualFold( - resourceType, "Microsoft.Storage/storageAccounts", - ) { - if v, ok := props["allowSharedKeyAccess"]; ok { - if b, ok := v.(bool); ok { - return !b - } - } - return false - } - - // Most resource types use disableLocalAuth. - if v, ok := props["disableLocalAuth"]; ok { - if b, ok := v.(bool); ok { - return b - } - } - - return false -} - -// stringOrEmpty safely dereferences a string pointer, returning "" if nil. -func stringOrEmpty(s *string) string { - if s == nil { - return "" - } - return *s -} diff --git a/cli/azd/pkg/azapi/policy_service_test.go b/cli/azd/pkg/azapi/policy_service_test.go deleted file mode 100644 index 24af385038e..00000000000 --- a/cli/azd/pkg/azapi/policy_service_test.go +++ /dev/null @@ -1,566 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -package azapi - -import ( - "testing" - - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" - "github.com/stretchr/testify/require" -) - -func TestExtractDenyPolicies_DenyLiteral(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.CognitiveServices/accounts", - }, - map[string]any{ - "field": "Microsoft.CognitiveServices/accounts/disableLocalAuth", - "notEquals": true, - }, - }, - }, - "then": map[string]any{ - "effect": "deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "test-policy", nil) - require.Len(t, results, 1) - require.Equal(t, "Microsoft.CognitiveServices/accounts", results[0].ResourceType) - require.Equal(t, "test-policy", results[0].PolicyName) - require.Contains(t, results[0].FieldPath, "disableLocalAuth") -} - -func TestExtractDenyPolicies_ParameterizedEffect_Deny(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ - "effect": { - DefaultValue: "Audit", - }, - }, - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.EventHub/namespaces", - }, - map[string]any{ - "field": "Microsoft.EventHub/namespaces/disableLocalAuth", - "notEquals": true, - }, - }, - }, - "then": map[string]any{ - "effect": "[parameters('effect')]", - }, - }, - }, - } - - // With assignment params overriding to Deny. - assignmentParams := map[string]any{"effect": "Deny"} - results := extractDenyPolicies(def, "test-policy", assignmentParams) - require.Len(t, results, 1) - require.Equal(t, "Microsoft.EventHub/namespaces", results[0].ResourceType) -} - -func TestExtractDenyPolicies_ParameterizedEffect_Audit(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ - "effect": { - DefaultValue: "Audit", - }, - }, - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.EventHub/namespaces", - }, - map[string]any{ - "field": "Microsoft.EventHub/namespaces/disableLocalAuth", - "notEquals": true, - }, - }, - }, - "then": map[string]any{ - "effect": "[parameters('effect')]", - }, - }, - }, - } - - // No assignment params — falls back to default "Audit", which is not deny. - results := extractDenyPolicies(def, "test-policy", nil) - require.Empty(t, results) -} - -func TestExtractDenyPolicies_NoLocalAuthField(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.Storage/storageAccounts", - }, - map[string]any{ - "field": "Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", - "equals": false, - }, - }, - }, - "then": map[string]any{ - "effect": "deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "test-policy", nil) - require.Empty(t, results) -} - -func TestExtractDenyPolicies_StorageAllowSharedKeyAccess(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.Storage/storageAccounts", - }, - map[string]any{ - "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", - "notEquals": false, - }, - }, - }, - "then": map[string]any{ - "effect": "deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "test-policy", nil) - require.Len(t, results, 1) - require.Equal(t, "Microsoft.Storage/storageAccounts", results[0].ResourceType) -} - -func TestExtractDenyPolicies_NestedAnyOf(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.ServiceBus/namespaces", - }, - map[string]any{ - "anyOf": []any{ - map[string]any{ - "field": "Microsoft.ServiceBus/namespaces/disableLocalAuth", - "equals": false, - }, - map[string]any{ - "field": "Microsoft.ServiceBus/namespaces/disableLocalAuth", - "exists": false, - }, - }, - }, - }, - }, - "then": map[string]any{ - "effect": "Deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "test-nested", nil) - require.NotEmpty(t, results) - require.Equal(t, "Microsoft.ServiceBus/namespaces", results[0].ResourceType) -} - -func TestExtractDenyPolicies_MultipleResourceTypesInArray(t *testing.T) { - t.Parallel() - - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "in": []any{ - "Microsoft.EventHub/namespaces", - "Microsoft.ServiceBus/namespaces", - }, - }, - map[string]any{ - "field": "disableLocalAuth", - "notEquals": true, - }, - }, - }, - "then": map[string]any{ - "effect": "Deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "multi-type-policy", nil) - require.Len(t, results, 2) - - types := make(map[string]bool) - for _, r := range results { - types[r.ResourceType] = true - } - require.True(t, types["Microsoft.EventHub/namespaces"]) - require.True(t, types["Microsoft.ServiceBus/namespaces"]) -} - -func TestExtractDenyPolicies_NestedConditionInheritsResourceType(t *testing.T) { - t.Parallel() - - // The resource type is declared at the outer allOf level, and the - // disableLocalAuth condition is in a nested anyOf. The nested level - // should inherit the resource type. - def := &armpolicy.Definition{ - Properties: &armpolicy.DefinitionProperties{ - PolicyRule: map[string]any{ - "if": map[string]any{ - "allOf": []any{ - map[string]any{ - "field": "type", - "equals": "Microsoft.Search/searchServices", - }, - map[string]any{ - "anyOf": []any{ - map[string]any{ - "field": "disableLocalAuth", - "equals": false, - }, - }, - }, - }, - }, - "then": map[string]any{ - "effect": "Deny", - }, - }, - }, - } - - results := extractDenyPolicies(def, "nested-inherit", nil) - require.Len(t, results, 1) - require.Equal(t, "Microsoft.Search/searchServices", results[0].ResourceType) -} - -func TestExtractManagementGroupID(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - id string - want string - }{ - { - "management group scoped policy definition", - "/providers/Microsoft.Management/managementGroups/myMgGroup" + - "/providers/Microsoft.Authorization/policyDefinitions/abc123", - "myMgGroup", - }, - { - "management group scoped policy set definition", - "/providers/Microsoft.Management/managementGroups/" + - "72f988bf-86f1-41af-91ab-2d7cd011db47" + - "/providers/Microsoft.Authorization/policySetDefinitions/8e7a35ba", - "72f988bf-86f1-41af-91ab-2d7cd011db47", - }, - { - "built-in policy definition", - "/providers/Microsoft.Authorization/policyDefinitions/" + - "6300012e-e9a4-4649-b41f-a85f5c43be91", - "", - }, - { - "subscription scoped custom definition", - "/subscriptions/faa080af/providers/" + - "Microsoft.Authorization/policyDefinitions/custom123", - "", - }, - { - "empty string", - "", - "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - require.Equal(t, tt.want, extractManagementGroupID(tt.id)) - }) - } -} - -func TestIsLocalAuthField(t *testing.T) { - t.Parallel() - - tests := []struct { - field string - want bool - }{ - { - "Microsoft.CognitiveServices/accounts/disableLocalAuth", - true, - }, - { - "Microsoft.EventHub/namespaces/disableLocalAuth", - true, - }, - { - "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", - true, - }, - { - "Microsoft.ServiceBus/namespaces/disableLocalAuth", - true, - }, - {"disableLocalAuth", true}, - { - "Microsoft.Storage/storageAccounts/supportsHttpsTrafficOnly", - false, - }, - {"type", false}, - {"location", false}, - } - - for _, tt := range tests { - t.Run(tt.field, func(t *testing.T) { - t.Parallel() - require.Equal(t, tt.want, IsLocalAuthField(tt.field)) - }) - } -} - -func TestResourceTypeFromFieldPath(t *testing.T) { - t.Parallel() - - tests := []struct { - field string - want string - }{ - { - "Microsoft.CognitiveServices/accounts/disableLocalAuth", - "Microsoft.CognitiveServices/accounts", - }, - { - "Microsoft.EventHub/namespaces/disableLocalAuth", - "Microsoft.EventHub/namespaces", - }, - {"disableLocalAuth", ""}, - {"", ""}, - } - - for _, tt := range tests { - t.Run(tt.field, func(t *testing.T) { - t.Parallel() - require.Equal(t, tt.want, resourceTypeFromFieldPath(tt.field)) - }) - } -} - -func TestExtractParameterReference(t *testing.T) { - t.Parallel() - - tests := []struct { - expr string - want string - }{ - {"[parameters('effect')]", "effect"}, - {"[parameters('myEffect')]", "myEffect"}, - {"deny", ""}, - {"[concat('a', 'b')]", ""}, - {"", ""}, - {" [parameters('effect')] ", "effect"}, - {"\t[parameters('effect')]\n", "effect"}, - } - - for _, tt := range tests { - t.Run(tt.expr, func(t *testing.T) { - t.Parallel() - require.Equal(t, tt.want, extractParameterReference(tt.expr)) - }) - } -} - -func TestResourceHasLocalAuthDisabled(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - resourceType string - properties string - want bool - }{ - { - "CognitiveServices disabled", - "Microsoft.CognitiveServices/accounts", - `{"disableLocalAuth": true}`, - true, - }, - { - "CognitiveServices enabled", - "Microsoft.CognitiveServices/accounts", - `{"disableLocalAuth": false}`, - false, - }, - { - "CognitiveServices missing property", - "Microsoft.CognitiveServices/accounts", - `{}`, - false, - }, - { - "Storage allowSharedKeyAccess false", - "Microsoft.Storage/storageAccounts", - `{"allowSharedKeyAccess": false}`, - true, - }, - { - "Storage allowSharedKeyAccess true", - "Microsoft.Storage/storageAccounts", - `{"allowSharedKeyAccess": true}`, - false, - }, - { - "Empty properties", - "Microsoft.EventHub/namespaces", - ``, - false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - require.Equal(t, tt.want, ResourceHasLocalAuthDisabled( - tt.resourceType, []byte(tt.properties), - )) - }) - } -} - -func TestIsDenyEffect(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - ruleMap map[string]any - definitionParams map[string]*armpolicy.ParameterDefinitionsValue - assignmentParams map[string]any - want bool - }{ - { - "literal deny", - map[string]any{"then": map[string]any{"effect": "deny"}}, - nil, nil, true, - }, - { - "literal Deny (capitalized)", - map[string]any{"then": map[string]any{"effect": "Deny"}}, - nil, nil, true, - }, - { - "literal audit", - map[string]any{"then": map[string]any{"effect": "audit"}}, - nil, nil, false, - }, - { - "parameterized deny via assignment", - map[string]any{ - "then": map[string]any{ - "effect": "[parameters('effect')]", - }, - }, - nil, - map[string]any{"effect": "Deny"}, - true, - }, - { - "parameterized deny via default", - map[string]any{ - "then": map[string]any{ - "effect": "[parameters('effect')]", - }, - }, - map[string]*armpolicy.ParameterDefinitionsValue{ - "effect": {DefaultValue: "Deny"}, - }, - nil, true, - }, - { - "parameterized audit via default", - map[string]any{ - "then": map[string]any{ - "effect": "[parameters('effect')]", - }, - }, - map[string]*armpolicy.ParameterDefinitionsValue{ - "effect": {DefaultValue: "Audit"}, - }, - nil, false, - }, - { - "no then block", - map[string]any{}, - nil, nil, false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - require.Equal( - t, tt.want, - isDenyEffect( - tt.ruleMap, - tt.definitionParams, - tt.assignmentParams, - ), - ) - }) - } -} diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 6599d17a771..12079d36cbe 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -2179,26 +2179,6 @@ func (p *BicepProvider) validatePreflight( Fn: p.checkRoleAssignmentPermissions, }) - // Register the storage account policy check. This uses deny policies - // fetched from the subscription's policy assignments (including - // management-group-inherited policies) to detect restrictions that - // would block deployment of storage accounts. - localPreflight.AddCheck(p.checkStorageAccountPolicy) - - // Set up the deny policy fetch function so policies are loaded in - // parallel with the Bicep snapshot. - localPreflight.SetDenyPoliciesFn( - func(ctx context.Context) ([]azapi.DenyPolicy, error) { - var policyService *azapi.PolicyService - if err := p.serviceLocator.Resolve(&policyService); err != nil { - return nil, err - } - return policyService.FindDenyPolicies( - ctx, p.env.GetSubscriptionId(), - ) - }, - ) - results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) if err != nil { p.setPreflightOutcome(span, preflightOutcomeError, nil) @@ -2259,7 +2239,6 @@ func (p *BicepProvider) validatePreflight( } if report.HasWarnings() { - p.console.Message(ctx, "") continueDeployment, promptErr := p.console.Confirm(ctx, input.ConsoleOptions{ Message: "Preflight validation found warnings that may cause the " + "deployment to fail. Do you want to continue?", @@ -2420,76 +2399,6 @@ func (p *BicepProvider) resolveResourceTenantPrincipalId( return principalId, nil } -// checkStorageAccountPolicy is a PreflightCheckFn that uses deny policies from -// the subscription's policy assignments to detect policies requiring local -// authentication to be disabled on storage accounts. It filters the -// pre-fetched deny policies to those targeting storage accounts with local -// auth fields and warns when any storage account in the template doesn't -// have local authentication disabled. -func (p *BicepProvider) checkStorageAccountPolicy( - ctx context.Context, valCtx *validationContext, -) (*PreflightCheckResult, error) { - // Collect storage accounts from the snapshot. - var storageAccounts []armTemplateResource - for _, resource := range valCtx.SnapshotResources { - if strings.EqualFold( - resource.Type, "Microsoft.Storage/storageAccounts", - ) { - storageAccounts = append(storageAccounts, resource) - } - } - if len(storageAccounts) == 0 { - return nil, nil - } - - // Filter deny policies to those targeting storage accounts with - // local auth fields. - hasStoragePolicy := false - for _, policy := range valCtx.DenyPolicies { - if strings.EqualFold( - policy.ResourceType, - "Microsoft.Storage/storageAccounts", - ) && azapi.IsLocalAuthField(policy.FieldPath) { - hasStoragePolicy = true - break - } - } - if !hasStoragePolicy { - return nil, nil - } - - // Count how many storage accounts don't have local auth disabled. - affectedCount := 0 - for _, account := range storageAccounts { - if !azapi.ResourceHasLocalAuthDisabled( - account.Type, account.Properties, - ) { - affectedCount++ - } - } - if affectedCount == 0 { - return nil, nil - } - - msg := "an Azure Policy on this subscription requires storage accounts " + - "to disable local authentication (shared key access). " - if affectedCount == 1 { - msg += "1 storage account in this deployment does not have " + - "'allowSharedKeyAccess' set to false." - } else { - msg += fmt.Sprintf( - "%d storage accounts in this deployment do not have "+ - "'allowSharedKeyAccess' set to false.", - affectedCount, - ) - } - - return &PreflightCheckResult{ - Severity: PreflightCheckWarning, - Message: msg, - }, nil -} - // Deploys the specified Bicep module and parameters with the selected provisioning scope (subscription vs resource group) func (p *BicepProvider) deployModule( ctx context.Context, diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go index d10a6e8a06c..e2b8ba642ca 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go @@ -14,9 +14,7 @@ import ( "path/filepath" "slices" "strings" - "sync" - "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/azure" "github.com/azure/azure-dev/cli/azd/pkg/infra" "github.com/azure/azure-dev/cli/azd/pkg/input" @@ -278,9 +276,6 @@ type validationContext struct { Console input.Console // Props contains derived properties from analyzing the ARM template resources. Props resourcesProperties - // Target is the deployment scope (subscription or resource group). - // It may be nil when the deployment scope is unknown. - Target infra.Deployment // ResourcesSnapshot is the raw JSON output from `bicep snapshot`, containing the fully // resolved deployment graph. It may be nil if the Bicep CLI was not available. ResourcesSnapshot json.RawMessage @@ -288,10 +283,6 @@ type validationContext struct { // Each entry represents a resource that would be deployed, with resolved values. // It may be nil if the Bicep CLI was not available. SnapshotResources []armTemplateResource - // DenyPolicies contains deny-effect policies fetched from the subscription's - // policy assignments, including inherited policies from management groups. - // Populated in parallel with the snapshot. - DenyPolicies []azapi.DenyPolicy } // snapshotResult represents the top-level structure of the Bicep snapshot JSON output. @@ -325,9 +316,6 @@ type localArmPreflight struct { // It may be nil, in which case snapshot options are left empty. target infra.Deployment checks []PreflightCheckFn - // denyPoliciesFn fetches deny policies for the subscription. - // Called in parallel with the snapshot. May be nil. - denyPoliciesFn func(ctx context.Context) ([]azapi.DenyPolicy, error) } // newLocalArmPreflight creates a new instance of localArmPreflight. @@ -344,14 +332,6 @@ func (l *localArmPreflight) AddCheck(fn PreflightCheckFn) { l.checks = append(l.checks, fn) } -// SetDenyPoliciesFn sets a function that fetches deny policies in parallel -// with the Bicep snapshot. If not set, no policies are fetched. -func (l *localArmPreflight) SetDenyPoliciesFn( - fn func(ctx context.Context) ([]azapi.DenyPolicy, error), -) { - l.denyPoliciesFn = fn -} - // validate performs local preflight validation on the given ARM template and parameters. // It parses the template, resolves parameters, analyzes the resources, and then runs all // registered check functions. It returns the collected results from all checks and an error @@ -410,51 +390,19 @@ func (l *localArmPreflight) validate( } } - // Run the Bicep snapshot and policy fetch in parallel. - // The snapshot produces the fully resolved deployment graph; the policy fetch - // retrieves deny-effect policies from the subscription's assignments (including - // management-group-inherited policies). - console.ShowSpinner(ctx, "Validating deployment [setup]", input.Step) - - var wg sync.WaitGroup - var snapshotData []byte - var snapshotErr error - var denyPolicies []azapi.DenyPolicy - - wg.Go(func() { - snapshotData, snapshotErr = l.bicepCli.Snapshot( - ctx, bicepParamFile, snapshotOpts, - ) - }) - - if l.denyPoliciesFn != nil { - wg.Go(func() { - policies, err := l.denyPoliciesFn(ctx) - if err != nil { - log.Printf( - "policy preflight: failed to fetch deny policies: %v", - err, - ) - return // non-fatal - } - denyPolicies = policies - }) - } - - wg.Wait() - - // If the snapshot fails (e.g., older Bicep binary without snapshot - // support), skip local preflight rather than blocking the deployment. - if snapshotErr != nil { - log.Printf( - "local preflight: skipping checks, bicep snapshot unavailable: %v", - snapshotErr, - ) + // Run the Bicep snapshot command to produce a deployment snapshot from the bicepparam file. + // The snapshot contains the fully resolved deployment graph with expressions evaluated, + // conditions applied, and copy loops expanded. + // If the snapshot fails (e.g., older Bicep binary without snapshot support), skip local + // preflight rather than blocking the deployment. + data, err := l.bicepCli.Snapshot(ctx, bicepParamFile, snapshotOpts) + if err != nil { + log.Printf("local preflight: skipping checks, bicep snapshot unavailable: %v", err) return nil, nil } var snapshot snapshotResult - if err := json.Unmarshal(snapshotData, &snapshot); err != nil { + if err := json.Unmarshal(data, &snapshot); err != nil { return nil, fmt.Errorf("parsing bicep snapshot: %w", err) } @@ -463,14 +411,10 @@ func (l *localArmPreflight) validate( valCtx := &validationContext{ Console: console, Props: props, - Target: l.target, - ResourcesSnapshot: json.RawMessage(snapshotData), + ResourcesSnapshot: json.RawMessage(data), SnapshotResources: snapshot.PredictedResources, - DenyPolicies: denyPolicies, } - console.ShowSpinner(ctx, "Validating deployment [checks]", input.Step) - var results []PreflightCheckResult for _, check := range l.checks { result, err := check(ctx, valCtx) From de81ee9bcb248eb2313e0fd019f03cd65afd7ab5 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Thu, 2 Apr 2026 17:02:35 +0000 Subject: [PATCH 11/13] Address review feedback on investigation doc - Fix self-contradictory example policy: change contains() condition from equals "false" to equals "true" so opt-in logic is consistent - Add API version annotation (2022-03-01) to checkPolicyRestrictions example - Add Limitation column to conclusion table for clarity (policyStates row) - Fix /validate claim: ARM /validate and /whatIf don't evaluate deny effects - Add 2024-10-01 MG-scope endpoint to Future Considerations as most promising next step Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../storage-account-policy-check.md | 44 ++++++++++++------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/cli/azd/docs/local-preflight/storage-account-policy-check.md b/cli/azd/docs/local-preflight/storage-account-policy-check.md index 9bfddfe2b39..e6d131fb75b 100644 --- a/cli/azd/docs/local-preflight/storage-account-policy-check.md +++ b/cli/azd/docs/local-preflight/storage-account-policy-check.md @@ -95,7 +95,7 @@ with `effect: deny`. The policy rule includes: "allOf": [ { "field": "type", "equals": "Microsoft.Storage/storageAccounts" }, { "field": "Microsoft.Storage/storageAccounts/allowSharedKeyAccess", "equals": "true" }, - { "value": "[contains(subscription().tags, parameters('optInTagName'))]", "equals": "false" }, + { "value": "[contains(subscription().tags, parameters('optInTagName'))]", "equals": "true" }, { "anyOf": [ { "field": "location", "in": "[split(subscription().tags[parameters('optInTagName')], ',')]" }, { "value": "all_regions", "in": "[split(subscription().tags[parameters('optInTagName')], ',')]" } @@ -107,9 +107,9 @@ with `effect: deny`. The policy rule includes: The assignment sets `optInTagName` = `"Az.Sec.DisableLocalAuth.Storage::OptIn"`. A subscription `contoso-dev-sub` does **not** have the -`Az.Sec.DisableLocalAuth.Storage::OptIn` tag. The opt-in `anyOf` block evaluates -to `false`, so the policy never fires — deployments with `allowSharedKeyAccess: -true` succeed. +`Az.Sec.DisableLocalAuth.Storage::OptIn` tag. The `contains()` condition evaluates +to `false`, which does not match `"equals": "true"`, so the `allOf` short-circuits +and the policy never fires — deployments with `allowSharedKeyAccess: true` succeed. The client-side parser cannot evaluate `subscription().tags[...]` or `split()` expressions. It sees the deny + `allowSharedKeyAccess` pattern and warns the @@ -145,7 +145,7 @@ properties) and Azure returns which policies would deny it and why. ``` POST /subscriptions/{id}/providers/Microsoft.PolicyInsights/checkPolicyRestrictions - ?api-version=2022-03-01 + ?api-version=2022-03-01 # version tested in this investigation { "resourceDetails": { @@ -194,11 +194,11 @@ warn *before* deployment, this API is not applicable. ## Conclusion -| Approach | Sees MG policies | Evaluates all conditions | Suitable | -|---|---|---|---| -| Client-side ARM policy SDK parsing | ✅ Yes | ❌ No (ARM expressions) | ❌ False positives | -| Server-side `checkPolicyRestrictions` | ❌ No | ✅ Yes | ❌ Misses real denials | -| `policyStates` API | ✅ Yes | ✅ Yes | ❌ Existing resources only | +| Approach | Sees MG policies | Evaluates all conditions | Limitation | Suitable | +|---|---|---|---|---| +| Client-side ARM policy SDK parsing | ✅ Yes | ❌ No (ARM expressions) | Cannot evaluate runtime expressions → false positives | ❌ | +| Server-side `checkPolicyRestrictions` | ❌ No | ✅ Yes | Subscription scope misses MG-inherited policies | ❌ | +| `policyStates` API | ✅ Yes | ✅ Yes | Only evaluates already-deployed resources | ❌ | No currently available approach provides both accurate policy detection (including management-group-inherited policies) and correct evaluation of @@ -208,12 +208,24 @@ system. ## Future Considerations -- If `checkPolicyRestrictions` is updated to evaluate management-group-inherited - policies, it would be the ideal solution — accurate server-side evaluation with - minimal client complexity. +- **Management group scope endpoint (`2024-10-01`)**: The `checkPolicyRestrictions` + API added a [management group scope endpoint](https://learn.microsoft.com/en-us/rest/api/policyinsights/policy-restrictions/check-at-management-group-scope?view=rest-policyinsights-2024-10-01) + in `api-version=2024-10-01`: + ``` + POST /providers/Microsoft.Management/managementGroups/{mgId}/providers/Microsoft.PolicyInsights/checkPolicyRestrictions + ?api-version=2024-10-01 + ``` + This could address the core limitation — the subscription-scope endpoint + doesn't see MG-inherited policies, but the MG-scope endpoint evaluates against + the full policy hierarchy. This is the most promising next step. A hybrid + approach (client-side detection to find relevant MG IDs + MG-scope server-side + evaluation) could combine accurate detection with full condition evaluation. + **Note**: This investigation tested only `api-version=2022-03-01` at the + subscription scope. The MG-scope endpoint was not tested. - A hybrid approach (client-side detection + subscription tag evaluation for common patterns) could reduce false positives for the most common gating conditions, at the cost of maintaining a partial ARM expression evaluator. -- The ARM deployment validation API (`/validate`) does evaluate policies but - requires submitting the full deployment, making it equivalent to the - server-side preflight that already runs after local preflight. +- The ARM deployment validation API (`/validate`) and what-if API (`/whatIf`) do + **not** evaluate Azure Policy deny effects. Only actual deployment submission + triggers deny policy evaluation, which is equivalent to the server-side + preflight that already runs after local preflight. From 0618e1b78fc42f390e2c0b0a491d5d8ad7d6fcd6 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Thu, 2 Apr 2026 17:35:51 +0000 Subject: [PATCH 12/13] Update investigation doc with 2024-10-01 API test results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tested checkPolicyRestrictions with api-version=2024-10-01 against a subscription with a tenant-root-MG deny policy (Storage Accounts - Safe Secrets Standard). Findings: - Subscription-scope 2024-10-01: still does not see MG-inherited policies - MG-scope endpoint: only supports 'type' pending field, rejects resourceDetails — cannot evaluate property-level restrictions - includeAuditEffect parameter: no change in results Updated Approach 2 section with detailed 2024-10-01 follow-up, added per-endpoint comparison table, and revised conclusion table and Future Considerations to reflect that the newer API does not resolve the limitation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../storage-account-policy-check.md | 89 +++++++++++++------ 1 file changed, 64 insertions(+), 25 deletions(-) diff --git a/cli/azd/docs/local-preflight/storage-account-policy-check.md b/cli/azd/docs/local-preflight/storage-account-policy-check.md index e6d131fb75b..5cbe9eaaf73 100644 --- a/cli/azd/docs/local-preflight/storage-account-policy-check.md +++ b/cli/azd/docs/local-preflight/storage-account-policy-check.md @@ -145,7 +145,7 @@ properties) and Azure returns which policies would deny it and why. ``` POST /subscriptions/{id}/providers/Microsoft.PolicyInsights/checkPolicyRestrictions - ?api-version=2022-03-01 # version tested in this investigation + ?api-version=2022-03-01 { "resourceDetails": { @@ -167,20 +167,66 @@ The API: **Result: Accurate evaluation but does not see management-group-inherited policies.** -Testing against subscriptions with management-group-assigned deny policies -confirmed that the API returns **empty results** (`policyEvaluations: []`, -`fieldRestrictions: []`) even when: +Testing against a subscription with a management-group-assigned deny policy +("Storage Accounts - Safe Secrets Standard", assigned at the tenant root +management group) confirmed that the API returns **empty results** +(`policyEvaluations: []`, `fieldRestrictions: []`) even when: -- The deny policy is confirmed active (deployments fail with - `RequestDisallowedByPolicy`) -- The subscription tags satisfy all opt-in conditions +- The deny policy is confirmed active (`policyStates` reports it with + `effect: deny` for existing storage accounts) - The resource content explicitly sets the denied property - Both subscription-scope and resource-group-scope endpoints are tried - A `PendingFields` parameter is included in the request -The API only evaluates policies assigned directly at the subscription or resource -group scope. Since enterprise storage deny policies originate from management -groups, `checkPolicyRestrictions` does not evaluate them. +The API correctly detects subscription-level policies — for example, a +subscription-scoped `modify` policy for `allowBlobPublicAccess` returned the +expected `fieldRestrictions` with `result: Required`. Only management-group- +inherited policies are invisible. + +#### `api-version=2024-10-01` follow-up (April 2026) + +The `2024-10-01` API version added a +[management group scope endpoint](https://learn.microsoft.com/en-us/rest/api/policyinsights/policy-restrictions/check-at-management-group-scope?view=rest-policyinsights-2024-10-01) +and an `includeAuditEffect` parameter on the subscription scope. We tested both +to see if they resolve the MG-inherited policy blind spot. + +**Subscription-scope with `2024-10-01`:** Same empty results as `2022-03-01`. +The newer API version does not change the subscription-scope behavior — MG- +inherited policies remain invisible. Adding `includeAuditEffect: true` also +returned empty. + +**Management group scope endpoint:** This endpoint only supports `pendingFields` +with a single `type` field — it rejects `resourceDetails` entirely: + +``` +POST /providers/Microsoft.Management/managementGroups/{mgId} + /providers/Microsoft.PolicyInsights/checkPolicyRestrictions + ?api-version=2024-10-01 + +# Only this request body is accepted: +{ "pendingFields": [{ "field": "type" }] } + +# This is rejected with InvalidCheckRestrictionsRequest: +{ "resourceDetails": { ... } } +``` + +The error message confirms the limitation: +> *"The 'resourceDetails' property is not supported in requests at Management +> Group level. The request content can only have a single 'type' pending field."* + +Testing at multiple levels of the MG hierarchy (direct parent, intermediate MGs, +tenant root) all returned empty `fieldRestrictions` even for the `type` field. +The MG-scope endpoint is designed to answer "which resource types are +restricted" at a management group level, not "would this specific resource +configuration be denied" — making it unsuitable for property-level checks like +`allowSharedKeyAccess`. + +| Endpoint | API Version | Sees MG deny policies | Supports resource properties | +|---|---|---|---| +| Subscription scope | `2022-03-01` | ❌ | ✅ | +| Subscription scope | `2024-10-01` | ❌ | ✅ | +| Resource group scope | `2024-10-01` | ❌ | ✅ | +| MG scope | `2024-10-01` | Untested (empty for `type`) | ❌ (rejected) | ### Approach 3: Policy States API (`policyStates`) @@ -197,7 +243,8 @@ warn *before* deployment, this API is not applicable. | Approach | Sees MG policies | Evaluates all conditions | Limitation | Suitable | |---|---|---|---|---| | Client-side ARM policy SDK parsing | ✅ Yes | ❌ No (ARM expressions) | Cannot evaluate runtime expressions → false positives | ❌ | -| Server-side `checkPolicyRestrictions` | ❌ No | ✅ Yes | Subscription scope misses MG-inherited policies | ❌ | +| Server-side `checkPolicyRestrictions` (sub scope) | ❌ No | ✅ Yes | Misses MG-inherited policies (confirmed with both `2022-03-01` and `2024-10-01`) | ❌ | +| Server-side `checkPolicyRestrictions` (MG scope, `2024-10-01`) | Untested | ❌ No | Only supports `type` field; rejects `resourceDetails` | ❌ | | `policyStates` API | ✅ Yes | ✅ Yes | Only evaluates already-deployed resources | ❌ | No currently available approach provides both accurate policy detection @@ -208,20 +255,12 @@ system. ## Future Considerations -- **Management group scope endpoint (`2024-10-01`)**: The `checkPolicyRestrictions` - API added a [management group scope endpoint](https://learn.microsoft.com/en-us/rest/api/policyinsights/policy-restrictions/check-at-management-group-scope?view=rest-policyinsights-2024-10-01) - in `api-version=2024-10-01`: - ``` - POST /providers/Microsoft.Management/managementGroups/{mgId}/providers/Microsoft.PolicyInsights/checkPolicyRestrictions - ?api-version=2024-10-01 - ``` - This could address the core limitation — the subscription-scope endpoint - doesn't see MG-inherited policies, but the MG-scope endpoint evaluates against - the full policy hierarchy. This is the most promising next step. A hybrid - approach (client-side detection to find relevant MG IDs + MG-scope server-side - evaluation) could combine accurate detection with full condition evaluation. - **Note**: This investigation tested only `api-version=2022-03-01` at the - subscription scope. The MG-scope endpoint was not tested. +- **`checkPolicyRestrictions` MG-scope with `resourceDetails` support**: If + Microsoft updates the MG-scope endpoint to accept `resourceDetails` (not just + `type` pending fields), it would enable property-level evaluation against + MG-inherited policies. This is the only server-side path that could solve the + problem without client-side expression evaluation. As of `2024-10-01`, the + MG-scope endpoint rejects `resourceDetails` requests. - A hybrid approach (client-side detection + subscription tag evaluation for common patterns) could reduce false positives for the most common gating conditions, at the cost of maintaining a partial ARM expression evaluator. From 0e94c460d060343851fad40d6e218b7344f4d958 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Fri, 3 Apr 2026 23:51:14 +0000 Subject: [PATCH 13/13] Revert unintended code changes to local_preflight.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR should only contain documentation changes (investigation doc and README update). Restore local_preflight.go to match main — the DiagnosticID, PreflightCheck struct, and AddCheck signature belong to a separate code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../provisioning/bicep/local_preflight.go | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go index e2b8ba642ca..46d7b373f0c 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go @@ -265,6 +265,10 @@ const ( type PreflightCheckResult struct { // Severity indicates whether this result is a warning or a blocking error. Severity PreflightCheckSeverity + // DiagnosticID is a unique, stable identifier for this specific finding type + // (e.g. "role_assignment_missing"). Used in telemetry to correlate actioned + // warnings with deployment outcomes and to track error frequency over time. + DiagnosticID string // Message is a human-readable description of the finding. Message string } @@ -300,6 +304,17 @@ type PreflightCheckFn func( valCtx *validationContext, ) (*PreflightCheckResult, error) +// PreflightCheck pairs a unique rule identifier with its check function. +// The RuleID is a stable, unique string used in telemetry to identify which rule +// produced a result (e.g. for crash tracking). Each rule may emit results with +// different DiagnosticIDs to distinguish specific finding types. +type PreflightCheck struct { + // RuleID is a unique, stable identifier for the rule (e.g. "role_assignment_permissions"). + RuleID string + // Fn is the check function that performs the validation. + Fn PreflightCheckFn +} + // localArmPreflight provides local (client-side) validation of an ARM template before deployment. // It parses the template and parameters to build a comprehensive view of all resources that would // be deployed, enabling early detection of issues without making Azure API calls. @@ -315,7 +330,7 @@ type localArmPreflight struct { // target is the deployment scope (subscription or resource group) used to derive snapshot options. // It may be nil, in which case snapshot options are left empty. target infra.Deployment - checks []PreflightCheckFn + checks []PreflightCheck } // newLocalArmPreflight creates a new instance of localArmPreflight. @@ -326,10 +341,10 @@ func newLocalArmPreflight(modulePath string, bicepCli *bicep.Cli, target infra.D return &localArmPreflight{modulePath: modulePath, bicepCli: bicepCli, target: target} } -// AddCheck registers a preflight check function to be executed during validate. -// Check functions are invoked in the order they are added. -func (l *localArmPreflight) AddCheck(fn PreflightCheckFn) { - l.checks = append(l.checks, fn) +// AddCheck registers a preflight check to be executed during validate. +// Checks are invoked in the order they are added. +func (l *localArmPreflight) AddCheck(check PreflightCheck) { + l.checks = append(l.checks, check) } // validate performs local preflight validation on the given ARM template and parameters. @@ -417,9 +432,9 @@ func (l *localArmPreflight) validate( var results []PreflightCheckResult for _, check := range l.checks { - result, err := check(ctx, valCtx) + result, err := check.Fn(ctx, valCtx) if err != nil { - return results, fmt.Errorf("preflight check failed: %w", err) + return results, fmt.Errorf("preflight check %q failed: %w", check.RuleID, err) } if result != nil { results = append(results, *result)