feat(azdext): Add KeyVault resolver for extension secret resolution#7043
Merged
jongio merged 22 commits intoAzure:mainfrom Mar 31, 2026
Merged
feat(azdext): Add KeyVault resolver for extension secret resolution#7043jongio merged 22 commits intoAzure:mainfrom
jongio merged 22 commits intoAzure:mainfrom
Conversation
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
2 times, most recently
from
2729231 to
b682e24
Compare
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds shared Key Vault secret-reference resolution to the extension SDK / CLI flow, so extensions can receive resolved secret values (rather than raw akvs://... or @Microsoft.KeyVault(...) references) without duplicating parsing + fetch logic.
Changes:
- Add
azdext.KeyVaultResolverwith structured error classification and batch resolution (ResolveMap). - Extend core
pkg/keyvaultwith parsing helpers for@Microsoft.KeyVault(SecretUri=...)and an env-var resolver (ResolveSecretEnvironment). - Integrate env-var resolution into extension invocation (
cmd/extensions.go) and update spelling dictionary entries.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
cli/azd/pkg/keyvault/keyvault.go |
Adds @Microsoft.KeyVault(SecretUri=...) parsing + unified reference resolution + env-var resolution helper. |
cli/azd/pkg/azdext/keyvault_resolver.go |
Introduces the extension-facing KeyVaultResolver API, parsing, and structured error types. |
cli/azd/pkg/azdext/keyvault_resolver_test.go |
Unit tests covering resolver behavior, parsing, and error classification. |
cli/azd/cmd/extensions.go |
Resolves KV references in azd-managed env vars before launching extension processes. |
cli/azd/.vscode/cspell.yaml |
Adds KV-related terms to cspell dictionary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@copilot please re-review the latest changes. |
wbreza
reviewed
Mar 17, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
What Looks Good
- Azure KV DNS suffixes are complete — all 5 valid suffixes (public, China, US Gov, Germany deprecated, Managed HSM) match Azure documentation
- Vault name regex is correct — enforces Azure's 3-24 char naming rules accurately
- SSRF protection — host validation against known Azure DNS suffixes prevents arbitrary URL resolution
- Structured error types — KeyVaultResolveError with ResolveReason classification is well-designed
- Per-vault client caching — sync.Map-based getOrCreateClient with LoadOrStore is correct and race-free
- Error collection — ResolveMap and ResolveSecretEnvironment collect all errors via errors.Join
- Reference leak prevention — on failure, env var value is set to empty rather than leaking the raw akvs:// reference
Findings Summary
| Priority | Count |
|---|---|
| Critical | 1 |
| High | 2 |
| Medium | 7 |
| Low | 2 |
| Total | 12 |
Overall Assessment: Comment. Good feature with solid security posture. The inline comments below highlight the sort.Strings bug and areas where additional unit tests would strengthen confidence before merge. Would love to see some more unit tests to validate the PR findings.
wbreza
approved these changes
Mar 20, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #7043 (Re-review)
Prior Review Resolution
All 10 findings from the Mar 17 review have been verified as addressed:
| # | Priority | Finding | Status |
|---|---|---|---|
| 1 | Critical | sort.Strings(result) breaks env var ordering |
✅ Fixed — removed entirely, ordering test added |
| 2 | High | No tests for ResolveSecretEnvironment / SecretFromKeyVaultReference |
✅ Fixed — 7 tests in keyvault_test.go |
| 3 | High | IsSecretReference test missing @Microsoft.KeyVault cases |
✅ Fixed — both test files cover it |
| 4 | Medium | Non-standard ports pass validation | ✅ Fixed — rejects non-443 ports |
| 5 | Medium | Empty vault name accepted for bare suffix | ✅ Fixed — idx > 0 guard |
| 6 | Medium | Case-sensitive prefix matching | ✅ Fixed — strings.EqualFold |
| 7 | Medium | Adding method to exported interface | ✅ Acknowledged — internal-only |
| 8 | Medium | context.Background() → t.Context() |
✅ Fixed — all 15 occurrences |
| 9 | Medium | slices.Sorted(maps.Keys()) |
✅ Fixed in ResolveMap |
| 10 | Low | Test consolidation & recording stub | ✅ Fixed |
New Findings
| Priority | Count |
|---|---|
| Medium | 1 |
| Total | 1 |
✅ What Looks Good
- Excellent test coverage — 1083 lines of tests covering parsing, resolution, error collection, ordering, sovereign clouds, SSRF protection, and edge cases
- Strong security posture — vault host suffix validation, port 443 enforcement, HTTPS-only scheme checks
- Clean separation — core
pkg/keyvaulthandles parsing/resolution;pkg/azdextprovides the extension-facing API with per-vault client caching - Modern Go patterns —
slices.Sorted(maps.Keys(...)),strings.Cut,t.Context(),t.Parallel()throughout
Overall Assessment: Approve — all prior Critical/High findings resolved. One minor modernization note below.
Review performed with GitHub Copilot CLI
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
2 times, most recently
from
694c2e4 to
4d67a17
Compare
jongio
requested review from
achauhan-scc,
bwateratmsft,
danieljurek,
kingernupur,
rabollin,
saanikaguptamicrosoft,
therealjohn,
trangevi and
trrwilson
as code owners
Co-authored-by: Copilot <[email protected]>
- Add 'managedhsm' and 'microsoftazure' to cspell dictionary - Suppress gosec G101 false positive on test data string Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
- Call ResolveSecretEnvironment unconditionally so akvs:// refs work without AZURE_SUBSCRIPTION_ID being set - Add per-vault client cache (sync.Map) to avoid N client creations when resolving N secrets from the same vault - Preserve original reference in ResolveMap output on failure so callers see all input keys - Tighten IsKeyVaultAppReference to only match SecretUri= variant - Use u.Hostname() for port-safe host validation in SSRF checks - Return ([]string, error) from ResolveSecretEnvironment so callers can handle failures explicitly - Update docs to clarify only SecretUri= format is supported Co-authored-by: Copilot <[email protected]>
ResolveSecretEnvironment appends system env vars first then azd vars, so last-wins ordering matters for duplicate keys. Sorting alphabetically destroys this override semantics. Addresses review thread 1 (keyvault.go:651).
Azure App Service handles @Microsoft.KeyVault(...) references case-insensitively. Switch from strings.HasPrefix to strings.EqualFold for the prefix and SecretUri= parameter checks so that variations like @microsoft.keyvault(secreturi=...) are not silently skipped. Addresses review thread 6 (keyvault.go:540).
A URI like https://myvault.vault.azure.net:9999/secrets/foo would pass host validation but send the bearer token to an unexpected port. Reject any port other than 443 (or no port) for SSRF protection. Addresses review thread 4 (keyvault.go:617).
If the SecretUri hostname has no subdomain prefix (e.g., 'vault.azure.net' instead of 'myvault.vault.azure.net'), vault name extraction would yield an incorrect or empty value. Add a guard to ensure the extracted vault name is non-empty and distinct from the full host. Addresses review thread 5 (keyvault.go:625).
Replace the manual key collection + sort.Strings pattern with the idiomatic Go 1.26 equivalent. This is cleaner and consistent with the rest of the codebase. Addresses review thread 9 (keyvault_resolver.go:299).
Per Go 1.24+ conventions and repo patterns, use t.Context() to get a test-scoped context that is automatically cancelled when the test ends. Addresses review thread 8 (keyvault_resolver_test.go:548).
The test only covered akvs:// variants. Add cases for the @Microsoft.KeyVault(SecretUri=...) format (true), case-insensitive prefix matching (true), and the unsupported VaultName/SecretName form (false). Addresses review thread 3 (keyvault_resolver_test.go:534).
…llection - Replace 4 structurally identical HTTP error tests with a single table-driven TestResolve_HTTPErrorClassification - Make stubSecretGetter record name/version args and verify dispatch in TestResolve_Success and TestResolve_AppRefWithVersion - Add 3 failing refs (+ 1 plain value) to TestResolveMap_ErrorCollectsAllFailures to verify error collection and partial results Addresses review thread 10 (keyvault_resolver_test.go:766).
…ference Cover the key untested paths: - Mixed akvs:// and @Microsoft.KeyVault refs (both resolved) - Malformed env vars (no = sign) passed through unchanged - nil kvService passthrough (returns input as-is) - Error collection when multiple refs fail - Ordering preserved (no alphabetical sort) - Non-standard port rejection - Port 443 allowed - Empty vault name rejection - Case-insensitive prefix matching - Comprehensive IsSecretReference coverage Addresses review thread 2 (keyvault.go:424).
Replace errors.As + var declaration with errors.AsType generic pattern for cleaner, more idiomatic error type assertion. Addresses review thread 11 (keyvault_resolver.go:231). Co-authored-by: Copilot <[email protected]>
- Fix gofmt formatting in keyvault_test.go (tabs vs spaces in imports) - Add SecretFromKeyVaultReference to mockKeyVaultService in cmdsubst tests to satisfy updated KeyVaultService interface Co-authored-by: Copilot <[email protected]>
… KeyVault resolver - Add ResolveEnvironment(ctx, env) method that scans env map, resolves only secret references, and passes through plain values unchanged - Add VaultName=;SecretName= format support to IsSecretReference and ParseSecretReference (third format alongside SecretUri and akvs://) - Add quote/whitespace stripping to IsSecretReference so quoted values like "akvs://..." and 'akvs://...' are detected - Add vault name validation for VaultName format (matches akvs:// rules) - Add 24 new tests covering all new functionality and edge cases Co-authored-by: Copilot <[email protected]>
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
from
113a2eb to
3aceac6
Compare
…ns (t.Context, errors.AsType, wg.Go) Co-authored-by: Copilot <[email protected]>
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
wbreza
approved these changes
Mar 31, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #7043 (1 new commit since Mar 30 approval)
Prior Findings: All 16 ✅ Verified
Every finding from prior reviews remains properly addressed:
| # | Priority | Finding | Status |
|---|---|---|---|
| 1 | Critical | sort.Strings(result) breaks ordering |
✅ Removed — index-based assignment |
| 2 | High | No tests for ResolveSecretEnvironment |
✅ 7 dedicated tests added |
| 3 | High | IsSecretReference missing @Microsoft.KeyVault |
✅ Both test files cover it |
| 4 | Medium | Non-standard ports pass validation | ✅ Port 443 enforcement |
| 5 | Medium | Empty vault name accepted | ✅ idx > 0 guard |
| 6 | Medium | Case-sensitive prefix matching | ✅ strings.EqualFold |
| 7 | Medium | Breaking exported interface | ✅ Acknowledged — internal-only |
| 8 | Medium | context.Background() in tests |
✅ t.Context() throughout |
| 9 | Medium | Manual key sorting | ✅ slices.Sorted(maps.Keys()) |
| 10 | Low | Test consolidation | ✅ Table-driven + recording stub |
| 11 | Medium | Data race in concurrent test | ✅ Fixed — sync.Mutex added to stubSecretGetter |
| 12 | Medium | context.Background() in cmdsubst tests |
✅ N/A — pre-existing, not part of this PR |
| 13 | Low | errors.As → errors.AsType |
✅ Fixed — modern pattern throughout |
| 14 | Low | wg.Add/Done → wg.Go |
✅ Fixed — modern pattern adopted |
New Findings: None
No new issues found in the latest commit.
Overall: ✅ APPROVE — all findings resolved, comprehensive test coverage, strong security posture.
Review performed with GitHub Copilot CLI
github-actions bot
added a commit
to weikanglim/azure-dev
that referenced
this pull request
Mar 31, 2026
…nd document KeyVaultResolver
Sync docs from upstream Azure/azure-dev main and add missing documentation
for new Extension SDK helpers merged in the last 24 hours:
- Add extension-sdk-reference.md: comprehensive API reference for all
azdext SDK helpers including new sections for:
- Process Management helpers (IsProcessRunning, GetProcessInfo, etc.)
- Environment Loading helpers (LoadAzdEnvironment, ParseEnvironmentVariables)
- Project Resolution helpers (GetProjectDir, FindFileUpward)
- Security Validation helpers (ValidateServiceName, ValidateScriptName, etc.)
- SSRF Guard (NewSSRFGuard, DefaultSSRFGuard with fluent builder)
- Testing Helpers (CaptureOutput)
- Key Vault Secret Resolution (KeyVaultResolver, IsSecretReference,
ParseSecretReference, KeyVaultResolveError) -- from PR Azure#7043
- Add extension-e2e-walkthrough.md: end-to-end guide for building an
extension from scratch
- Add extension-migration-guide.md: migration guide from pre-SDK patterns
to new azdext helpers
- Update extension-framework.md: sync latest content including
allowed_locations for PromptLocation and quota-aware capacity resolution
from PR Azure#7397
Related PRs: Azure#7025, Azure#7043, Azure#7397
Co-authored-by: Copilot <[email protected]>
github-actions bot
added a commit
to weikanglim/azure-dev
that referenced
this pull request
Apr 1, 2026
- Document azd auth token raw token default output (Azure#7384) - Add Key Vault Secret Resolution section for extension SDK (Azure#7043) - Update PromptLocation to include allowed_locations filter (Azure#7397) Co-authored-by: Copilot <[email protected]>
This was referenced Apr 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Key Vault secret resolution capabilities for both the azd host and the extension SDK (
pkg/azdext), enabling extensions to resolve secret references in environment variables without custom Key Vault code.Changes
Host-Level Resolution (
pkg/keyvault+cmd/extensions.go)@Microsoft.KeyVault(SecretUri=...)detection and parsing helpers topkg/keyvault:IsKeyVaultAppReference— detects@Microsoft.KeyVault(SecretUri=...)formatParseKeyVaultAppReference— parses SecretUri into vault/secret/version componentsSecretFromKeyVaultReference— resolves a single reference viaKeyVaultServiceResolveSecretEnvironment— resolves all secret references in aKEY=VALUEenv var listextensionAction.Run()so azd-managed environment variables are resolved before being passed to extensionsExtension SDK (
pkg/azdext)KeyVaultResolver— standalone resolver for extensions with three supported formats:akvs://<subscription-id>/<vault-name>/<secret-name>@Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<secret>[/<version>])@Microsoft.KeyVault(VaultName=<vault>;SecretName=<secret>[;SecretVersion=<version>])IsSecretReference(s)— detects all 3 formats with quote/whitespace strippingParseSecretReference(ref)— parses any format intoSecretReferenceNewKeyVaultResolver(credential, opts)— creates resolver with injectable credential and configurable vault suffixResolve(ctx, ref)— resolves a single referenceResolveMap(ctx, refs)— resolves a map where all values are referencesResolveEnvironment(ctx, env)— resolves a mixed env var map (detects refs, resolves them, passes through plain values)KeyVaultResolveError/ResolveReason, per-vault client caching with normalized keys, vault name validation, duplicate parameter rejectionTest & CI Fixes
SecretFromKeyVaultReferencemethod tocmdsubst_additional_test.gokeyvault_test.goTesting
pkg/azdext(including 56 KeyVault resolver tests)pkg/keyvaultgo vetandgofmtcleanRelated