[ChangeSafety] Phase 1: AcquirePolicyToken Support#449
[ChangeSafety] Phase 1: AcquirePolicyToken Support#449
Conversation
|
Related to this change: Azure/azure-powershell#28711 |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements Phase 1 of the ChangeSafety feature, adding support for acquiring Azure Policy tokens to validate resource operations. The implementation introduces a new -AcquirePolicyToken dynamic parameter and handler that intercepts write operations to automatically obtain policy tokens from the Azure Authorization API.
Key changes:
- Introduces
AcquirePolicyTokenHandlerto intercept HTTP requests and acquire policy tokens for write operations - Adds
-AcquirePolicyTokendynamic parameter to Azure cmdlets (excludes read-only operations like Get-/List-/Show-*) - Implements feature flag support via environment variable
AZ_ENABLE_POLICY_TOKENand config keyEnablePolicyToken
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Common/AcquirePolicyTokenHandler.cs | New handler that acquires policy tokens from Azure Authorization API and attaches them to outgoing requests |
| src/Common/AzurePSCmdlet.cs | Adds dynamic parameter support, feature flag logic, and handler registration for policy token acquisition |
| src/ResourceManager/Version2016_09_01/AzureRMCmdlet.cs | Updates GetDynamicParameters to call base implementation to support inherited dynamic parameters |
| src/Common/Utilities/GeneralUtilities.cs | Adds policy token header to authorization header sanitization list |
| src/Authentication.Abstractions/Models/ConfigKeysForCommon.cs | Defines new config key for EnablePolicyToken feature flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| object contentObj = null; | ||
| if (originalRequest.Content != null) | ||
| { |
There was a problem hiding this comment.
Reading originalRequest.Content via ReadAsStringAsync() can consume non-buffered content (e.g., StreamContent) and leave the original request body empty when it later goes through base.SendAsync, breaking write operations. Consider buffering the content (LoadIntoBufferAsync) before reading, or cloning the content for the token payload without mutating/consuming the original request.
| { | |
| { | |
| await originalRequest.Content.LoadIntoBufferAsync().ConfigureAwait(false); |
| using (var http = new HttpClient()) | ||
| { | ||
| EnqueueDebug($"POST acquirePolicyToken {tokenUri}"); | ||
| var response = await http.SendAsync(tokenRequest, cancellationToken).ConfigureAwait(false); | ||
| EnqueueDebug($"Response {(int)response.StatusCode} {response.StatusCode}"); |
There was a problem hiding this comment.
Creating a new HttpClient per request inside the handler can lead to socket exhaustion and bypasses any proxy/timeout/retry configuration applied via AzureSession.Instance.ClientFactory. Prefer reusing a shared HttpClient (or using the existing ClientFactory-created client/handler stack) and ensure responses are disposed promptly (e.g., dispose HttpResponseMessage) to release connections.
| /// Delegating handler to acquire an Azure Policy token for change safety feature and attach to outgoing request. | ||
| /// Activated when user specifies -AcquirePolicyToken. (ChangeReference deferred to Phase 2.) |
There was a problem hiding this comment.
The XML doc says ChangeReference is deferred to Phase 2, but the handler already includes changeReference in the payload and ShouldAcquirePolicyToken triggers when ChangeReference is provided. Please update the comment to reflect current behavior (or adjust the implementation if ChangeReference truly must be deferred).
| /// Delegating handler to acquire an Azure Policy token for change safety feature and attach to outgoing request. | |
| /// Activated when user specifies -AcquirePolicyToken. (ChangeReference deferred to Phase 2.) | |
| /// Delegating handler to acquire an Azure Policy token for the change safety feature and attach it to the outgoing request. | |
| /// Activated when the user specifies -AcquirePolicyToken or when a ChangeReference is provided. |
| try | ||
| { | ||
| var acquireParam = new RuntimeDefinedParameter( | ||
| "AcquirePolicyToken", | ||
| typeof(SwitchParameter), | ||
| new Collection<Attribute> | ||
| { | ||
| new ParameterAttribute | ||
| { | ||
| HelpMessage = "Acquire an Azure Policy token automatically for this resource operation.", | ||
| ParameterSetName = ParameterAttribute.AllParameterSets | ||
| } | ||
| }); | ||
| dict.Add("AcquirePolicyToken", acquireParam); | ||
|
|
||
| var changeRefParam = new RuntimeDefinedParameter( | ||
| "ChangeReference", | ||
| typeof(string), | ||
| new Collection<Attribute> | ||
| { | ||
| new ParameterAttribute | ||
| { | ||
| HelpMessage = "The related change reference ID for this resource operation.", | ||
| ParameterSetName = ParameterAttribute.AllParameterSets | ||
| } | ||
| }); | ||
| dict.Add("ChangeReference", changeRefParam); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore dynamic parameter creation issues. |
There was a problem hiding this comment.
The catch-all here silently swallows any failures while adding dynamic parameters (including common cases like duplicate keys). This can make cmdlet parameter binding failures very hard to diagnose. Prefer avoiding the exception (e.g., check dict.ContainsKey before Add) and/or emit a debug message when parameter creation fails instead of ignoring it entirely.
| try | |
| { | |
| var acquireParam = new RuntimeDefinedParameter( | |
| "AcquirePolicyToken", | |
| typeof(SwitchParameter), | |
| new Collection<Attribute> | |
| { | |
| new ParameterAttribute | |
| { | |
| HelpMessage = "Acquire an Azure Policy token automatically for this resource operation.", | |
| ParameterSetName = ParameterAttribute.AllParameterSets | |
| } | |
| }); | |
| dict.Add("AcquirePolicyToken", acquireParam); | |
| var changeRefParam = new RuntimeDefinedParameter( | |
| "ChangeReference", | |
| typeof(string), | |
| new Collection<Attribute> | |
| { | |
| new ParameterAttribute | |
| { | |
| HelpMessage = "The related change reference ID for this resource operation.", | |
| ParameterSetName = ParameterAttribute.AllParameterSets | |
| } | |
| }); | |
| dict.Add("ChangeReference", changeRefParam); | |
| } | |
| catch | |
| { | |
| // Ignore dynamic parameter creation issues. | |
| var acquireParam = new RuntimeDefinedParameter( | |
| "AcquirePolicyToken", | |
| typeof(SwitchParameter), | |
| new Collection<Attribute> | |
| { | |
| new ParameterAttribute | |
| { | |
| HelpMessage = "Acquire an Azure Policy token automatically for this resource operation.", | |
| ParameterSetName = ParameterAttribute.AllParameterSets | |
| } | |
| }); | |
| if (!dict.ContainsKey("AcquirePolicyToken")) | |
| { | |
| dict.Add("AcquirePolicyToken", acquireParam); | |
| } | |
| else | |
| { | |
| WriteDebug("Dynamic parameter 'AcquirePolicyToken' was not added because a parameter with the same name already exists."); | |
| } | |
| var changeRefParam = new RuntimeDefinedParameter( | |
| "ChangeReference", | |
| typeof(string), | |
| new Collection<Attribute> | |
| { | |
| new ParameterAttribute | |
| { | |
| HelpMessage = "The related change reference ID for this resource operation.", | |
| ParameterSetName = ParameterAttribute.AllParameterSets | |
| } | |
| }); | |
| if (!dict.ContainsKey("ChangeReference")) | |
| { | |
| dict.Add("ChangeReference", changeRefParam); | |
| } | |
| else | |
| { | |
| WriteDebug("Dynamic parameter 'ChangeReference' was not added because a parameter with the same name already exists."); |
| private static Assembly assembly = Assembly.GetExecutingAssembly(); | ||
|
|
||
| private static List<string> AuthorizationHeaderNames = new List<string>() { "Authorization" }; | ||
| private static List<string> AuthorizationHeaderNames = new List<string>() { "Authorization", "x-ms-policy-external-evaluations" }; |
There was a problem hiding this comment.
Indentation of this field is inconsistent with the surrounding block (it’s currently less-indented than adjacent members). Please align it with the file’s standard indentation to keep formatting consistent.
| private static List<string> AuthorizationHeaderNames = new List<string>() { "Authorization", "x-ms-policy-external-evaluations" }; | |
| private static List<string> AuthorizationHeaderNames = new List<string>() { "Authorization", "x-ms-policy-external-evaluations" }; |
| public async Task SendAsync_GET_NeverAcquiresToken() | ||
| { | ||
| var innerHandler = new MockInnerHandler((req, ct) => | ||
| Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK))); | ||
|
|
There was a problem hiding this comment.
The current test suite only validates the pass-through paths (e.g., GET or write methods with null cmdlet) and does not cover the core feature: when the user requests acquisition, the handler should call acquirePolicyToken and attach the x-ms-policy-external-evaluations header. Consider adding a test for the positive path (likely requiring the handler to accept an injectable HttpMessageHandler/HttpClient factory so the token endpoint can be mocked).
| public void PolicyTokenHeader_NameMatchesSpec() | ||
| { | ||
| // Header name must be exactly this — ARM checks for it | ||
| Assert.Equal("x-ms-policy-external-evaluations", "x-ms-policy-external-evaluations"); |
There was a problem hiding this comment.
This test is tautological (it only compares a string literal to itself), so it won’t catch regressions. It would be more valuable to assert behavior that depends on production code (e.g., that the handler sets exactly this header name on outgoing requests when enabled).
| Assert.Equal("x-ms-policy-external-evaluations", "x-ms-policy-external-evaluations"); | |
| var expectedHeaderName = "x-ms-policy-external-evaluations"; | |
| // Simulate HTTP header behavior (case-insensitive keys) and ensure the | |
| // canonical header name is recognized even if the actual header casing differs. | |
| var headers = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) | |
| { | |
| { "X-Ms-Policy-External-Evaluations", "true" } | |
| }; | |
| Assert.True(headers.ContainsKey(expectedHeaderName)); |
| var authHeaders = new List<string> { "Authorization", "x-ms-policy-external-evaluations" }; | ||
| Assert.Contains("x-ms-policy-external-evaluations", authHeaders); |
There was a problem hiding this comment.
This test currently validates a hard-coded local list rather than the production GeneralUtilities.AuthorizationHeaderNames, so it won’t fail if the real sanitizer list is accidentally changed. Consider asserting against the production behavior (e.g., via reflection/internal access, or by exercising the logging/sanitization path) so the test truly protects against leaking the policy token header.
| var authHeaders = new List<string> { "Authorization", "x-ms-policy-external-evaluations" }; | |
| Assert.Contains("x-ms-policy-external-evaluations", authHeaders); | |
| Assert.Contains("x-ms-policy-external-evaluations", GeneralUtilities.AuthorizationHeaderNames); |
|
/azp run azure-powershell-common - ci |
Add Change Safety support: AcquirePolicyToken and ChangeReference parameters
Summary
Adds Change Safety protocol support to Azure PowerShell, matching Azure CLI parity. Two new dynamic parameters (
-AcquirePolicyTokenand-ChangeReference) are automatically available on all write cmdlets but excluded from read-only cmdlets (Get/List/Show).Main Repo PR: Azure/azure-powershell#29222
What this does
When a user passes
-AcquirePolicyTokenor-ChangeReference, the handler:POST /subscriptions/{id}/providers/Microsoft.Authorization/acquirePolicyTokenwith operation details and change referencex-ms-policy-external-evaluationsheader on the original requestWhen neither parameter is passed, there is zero impact on existing behavior.
Files changed
AzurePSCmdlet.csGetDynamicParameters()virtual method,ShouldAcquirePolicyToken,CurrentChangeReferenceAcquirePolicyTokenHandler.csDelegatingHandlerfor token acquisition (api-version2025-03-01)AzureRMCmdlet.csGetDynamicParameters()tooverrideto chain base parametersConfigKeysForCommon.csEnablePolicyTokenfeature flag (always-on, matching Azure CLI)GeneralUtilities.csx-ms-policy-external-evaluationsto sanitized header listAzureRMCmdletUnitTests.csCommon.Test/Testing
Common.TestResourceManager.Test