Support managed identity auth in AzureFoundry provider#9707
Conversation
The AzureFoundry chat client provider now authenticates with DefaultAzureCredential (Entra ID / managed identity) when no AZURE_OPENAI_API_KEY is set, keeping the provider secure-by-default for Azure-hosted scenarios. The API key remains an explicit fallback, and IsAvailable no longer requires an API key. Fixes #8412 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR makes the Microsoft.Testing.Extensions.AzureFoundry provider support Microsoft Entra ID / managed-identity authentication instead of hard-requiring AZURE_OPENAI_API_KEY, closing the secure-by-default gap described in issue #8412. The provider now reports itself available whenever the endpoint and deployment name are configured, and it authenticates via DefaultAzureCredential when no API key is supplied, keeping the explicit API-key path as an override.
Changes:
IsAvailableno longer requiresAZURE_OPENAI_API_KEY(only endpoint + deployment name).CreateChatClientAsyncusesApiKeyCredentialwhen a key is present, otherwise falls back toDefaultAzureCredential.- Adds the
Azure.Identitydependency (version pinned via CPM) and documents both auth modes inPACKAGE.md.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.AzureFoundry/OpenAIChatClientProvider.cs | Drops the mandatory API-key check and selects DefaultAzureCredential vs ApiKeyCredential based on whether the key is set. |
| src/Platform/Microsoft.Testing.Extensions.AzureFoundry/PACKAGE.md | Adds a configuration table and prose documenting the managed-identity and API-key authentication modes. |
| src/Platform/Microsoft.Testing.Extensions.AzureFoundry/Microsoft.Testing.Extensions.AzureFoundry.csproj | References the new Azure.Identity package. |
| Directory.Packages.props | Pins Azure.Identity version 1.14.2 under central package management. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| <PackageVersion Include="Microsoft.Extensions.Logging" Version="9.0.4" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Logging.Console" Version="9.0.4" /> | ||
| <PackageVersion Include="Azure.AI.OpenAI" Version="2.1.0" /> | ||
| <PackageVersion Include="Azure.Identity" Version="1.14.2" /> |
There was a problem hiding this comment.
Good catch — bumped Azure.Identity from 1.14.2 to 1.21.0 (the current supported release; 1.14.x is deprecated because it depends on a deprecated MSAL). Verified a clean restore/build across all TFMs (netstandard2.0, net8.0, net9.0, net462, net472) with no NU1605 downgrade or NuGetAudit warnings, and it resolves Azure.Core upward compatibly with Azure.AI.OpenAI 2.1.0.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness | 🟡 1 MODERATE |
| 20 | Build Infrastructure & Dependencies | 🟡 1 MODERATE |
| 17 | Documentation Accuracy | ⚪ 1 NIT |
✅ 19/22 dimensions clean (dimensions 10–12, 14, 18–19, 22 skipped as N/A).
- Test Completeness — no unit tests cover the new
DefaultAzureCredentialbranch (or any path ofAzureOpenAIChatClientProvider) - Build Infrastructure —
Azure.Identityadds a heavyweight transitive dependency graph; acceptable whilealpha/non-shipping, but worth revisiting before GA - Documentation — consider noting local-dev latency from
DefaultAzureCredentialcredential chain traversal
Overall: The implementation is clean, correct, and well-documented. The ternary conditional for credential selection is straightforward. The IsAvailable change correctly reflects the new optionality of the API key. No correctness, threading, security, or API compatibility concerns.
| // authentication via DefaultAzureCredential. This keeps the provider secure-by-default for | ||
| // Azure-hosted scenarios where distributing API keys is undesirable. | ||
| AzureOpenAIClient client = RoslynString.IsNullOrEmpty(apiKey) | ||
| ? new AzureOpenAIClient(new Uri(endpoint), new DefaultAzureCredential()) |
There was a problem hiding this comment.
[MODERATE] Test Completeness
The DefaultAzureCredential path is new behavior with no unit test coverage. There are no existing tests for AzureOpenAIChatClientProvider at all — neither the API-key path nor the new managed-identity path is exercised.
Consider adding unit tests (even if they only verify the correct branch is taken and the right client type is constructed) to prevent regressions. The class is internal so [InternalsVisibleTo] or a test-specific seam would be needed.
Recommendation: Add at least a basic test verifying that when AZURE_OPENAI_API_KEY is unset, the code doesn't throw InvalidOperationException (the old behavior) and that when it is set, ApiKeyCredential is used.
There was a problem hiding this comment.
Addressed — added a dedicated unit test project reference (Microsoft.Testing.Extensions.UnitTests with InternalsVisibleTo) covering AzureOpenAIChatClientProvider. It now exercises IsAvailable (API-key-optional path), ModelName, HasToolsCapability, the missing-endpoint/deployment InvalidOperationException cases, and the API-key-vs-DefaultAzureCredential branch selection via an internal GetAuthenticationMode seam (14 tests, all passing).
| | --- | --- | --- | | ||
| | `AZURE_OPENAI_ENDPOINT` | Yes | The Azure OpenAI resource endpoint, for example `https://my-resource.openai.azure.com`. | | ||
| | `AZURE_OPENAI_DEPLOYMENT_NAME` | Yes | The name of the model deployment to use. | | ||
| | `AZURE_OPENAI_API_KEY` | No | An API key for the resource. When omitted, authentication uses `DefaultAzureCredential` (Managed Identity, Workload Identity, Azure CLI, Visual Studio, etc.). | |
There was a problem hiding this comment.
[NIT] Documentation Accuracy
Consider adding a brief note that DefaultAzureCredential traverses a credential chain (Managed Identity → Workload Identity → Azure CLI → Visual Studio → etc.) which can add noticeable latency in local development scenarios where early credentials in the chain time out before reaching one that works (e.g., Azure CLI). This is a common source of confusion for developers trying the package locally for the first time.
Something like:
Note: In local development,
DefaultAzureCredentialmay take several seconds on first use as it probes multiple credential sources. ProvidingAZURE_OPENAI_API_KEYavoids this delay.
There was a problem hiding this comment.
Done — added a note to PACKAGE.md that DefaultAzureCredential probes a credential chain and can add several seconds of latency on first use in local development, and that setting AZURE_OPENAI_API_KEY avoids the delay.
| <PackageVersion Include="Microsoft.Extensions.Logging" Version="9.0.4" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Logging.Console" Version="9.0.4" /> | ||
| <PackageVersion Include="Azure.AI.OpenAI" Version="2.1.0" /> | ||
| <PackageVersion Include="Azure.Identity" Version="1.14.2" /> |
There was a problem hiding this comment.
[MODERATE] Build Infrastructure & Dependencies
Azure.Identity 1.14.2 is a heavyweight dependency — it transitively pulls in MSAL (Microsoft.Identity.Client), Azure.Core, System.Memory, Microsoft.IdentityModel.*, and others. This becomes a transitive dependency for all consumers of the Microsoft.Testing.Extensions.AzureFoundry NuGet package.
Since the package is currently alpha / IsShipping=false, this is acceptable for now. However, when this package moves to shipping status, consider whether Azure.Identity should remain a direct dependency or whether the credential creation should be abstracted (e.g., accept a TokenCredential parameter) so consumers who don't need managed identity aren't forced to pull the full MSAL graph.
There was a problem hiding this comment.
Agreed. The package is currently alpha / IsShipping=false, so the transitive MSAL/Azure.Core graph is acceptable for now. I've filed a follow-up to revisit before GA — either keep Azure.Identity as a direct dependency or abstract credential creation behind an injectable TokenCredential so consumers who only use API keys don't pull the full MSAL graph. Tracking issue: #9712.
- Reuse a single DefaultAzureCredential instance (Lazy) so the token cache and credential-chain discovery are shared across calls instead of being rebuilt on every CreateChatClientAsync invocation. - Add unit tests for the auth-selection logic (IsAvailable, ModelName, API-key vs Entra path, missing-variable failures) in Microsoft.Testing.Extensions.UnitTests, with env-var snapshot/restore and DoNotParallelize; grant InternalsVisibleTo to the test assembly. - Document AZURE_CLIENT_ID (user-assigned managed identity) and the deferred authentication-failure behavior in PACKAGE.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Bump Azure.Identity 1.14.2 -> 1.21.0. The 1.14.x line is deprecated (depends on a deprecated MSAL); 1.21.0 is the current supported release. Verified clean restore/build across all TFMs (no NU1605, no audit warnings) and compatible with Azure.AI.OpenAI 2.1.0. - Extract an internal AuthenticationMode + GetAuthenticationMode seam so the API-key-vs-DefaultAzureCredential selection is deterministically unit-tested (upgrades the two 'only IsNotNull' tests and adds three dedicated selection tests). - Document DefaultAzureCredential local-dev credential-chain latency in PACKAGE.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #970714 new tests graded across 1 file. 12 earn A for clean AAA structure, proper env-var isolation via
This advisory comment was generated automatically. Grades are heuristic
|
Fixes #8412
Summary
Microsoft.Testing.Extensions.AzureFoundrypreviously hard-requiredAZURE_OPENAI_API_KEYand always constructedAzureOpenAIClientwithApiKeyCredential, with no managed-identity / Entra path. This made the only built-in AI provider not secure-by-default for Azure-hosted scenarios.Changes
IsAvailablenow only requiresAZURE_OPENAI_ENDPOINTandAZURE_OPENAI_DEPLOYMENT_NAME; the API key is no longer mandatory.CreateChatClientAsyncprefers an explicitAZURE_OPENAI_API_KEYwhen present, otherwise falls back toDefaultAzureCredential(managed identity, workload identity, Azure CLI, Visual Studio, …).Azure.Identitypackage reference (version pinned inDirectory.Packages.props).PACKAGE.mdwith a configuration table documenting both authentication modes.Notes
.resx/.xlfchanges were needed — the existingEnvironmentVariableNotSetstring is still used for the endpoint/deployment checks.