feat(azdext): Add KeyVaultResolver for extension Key Vault secret resolution#7043
Open
jongio wants to merge 9 commits intoAzure:mainfrom
Open
feat(azdext): Add KeyVaultResolver for extension Key Vault secret resolution#7043jongio wants to merge 9 commits intoAzure:mainfrom
jongio wants to merge 9 commits intoAzure:mainfrom
Conversation
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
from
6efe774 to
808a1fe
Compare
…olution
Add KeyVaultResolver to the Extension SDK for resolving Azure Key Vault
secret references embedded in environment variables. Supports three
reference formats:
- akvs://<subscription>/<vault>/<secret>
- @Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<name>[/<version>])
- @Microsoft.KeyVault(VaultName=...;SecretName=...)
Features:
- Thread-safe per-vault client caching
- Batch resolution via ResolveMap
- Structured error types with ResolveReason classification
- Configurable vault suffix for sovereign clouds
- Helper functions: IsSecretReference, ParseKeyVaultAppReference,
ResolveSecretEnvironment for bulk env var resolution
Also adds supporting functions to pkg/keyvault:
- IsKeyVaultAppReference, IsSecretReference
- ParseKeyVaultAppReference for @Microsoft.KeyVault format
- SecretFromKeyVaultReference for unified resolution
- ResolveSecretEnvironment for bulk env var processing
Evidence: Extensions like azd-exec, azd-app duplicate 100+ lines of KV
resolution logic each. This centralizes the pattern in the SDK.
Related: Azure#6945, Azure#6853
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Reverted detect_confirm_apphost.go (had color.MagentaString() without import, causing build failure) and show.go (KV comments belong on improvements branch) to match upstream/main. Co-authored-by: Copilot <[email protected]>
The 'secret' map key in test data triggers gosec G101 (hardcoded credentials) but these are fake akvs:// URIs in test fixtures, not real credentials. Co-authored-by: Copilot <[email protected]>
- SSRF: Validate KeyVault SecretUri hostname against Azure vault suffixes - Scoping: Only resolve azd env vars through KV resolver, not os.Environ() - Errors: Use ServiceError reason for non-ResponseError failures - Security: Explicit DisableChallengeResourceVerification: false - Testing: Add 8 tests for @Microsoft.KeyVault reference format - Reliability: Return empty string on resolution failure, not raw reference - Determinism: Sort keys in ResolveMap, collect all errors with errors.Join Co-authored-by: Copilot <[email protected]>
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]>
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
from
2729231 to
b682e24
Compare
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
|
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.
Comment on lines
+228
to
+230
| if subId := env.Getenv("AZURE_SUBSCRIPTION_ID"); subId != "" { | ||
| azdEnvVars = kv.ResolveSecretEnvironment(ctx, a.kvService, azdEnvVars, subId) | ||
| } |
There was a problem hiding this comment.
Key Vault resolution is currently gated on AZURE_SUBSCRIPTION_ID being set. That prevents resolving akvs:// references (which include their own subscription ID) in environments where AZURE_SUBSCRIPTION_ID is missing. Consider calling ResolveSecretEnvironment unconditionally (passing an empty defaultSubscriptionId), and have the resolver only require a default subscription when it encounters an @Microsoft.KeyVault(...) reference.
Suggested change
| if subId := env.Getenv("AZURE_SUBSCRIPTION_ID"); subId != "" { | |
| azdEnvVars = kv.ResolveSecretEnvironment(ctx, a.kvService, azdEnvVars, subId) | |
| } | |
| subId := env.Getenv("AZURE_SUBSCRIPTION_ID") | |
| azdEnvVars = kv.ResolveSecretEnvironment(ctx, a.kvService, azdEnvVars, subId) |
Comment on lines
+136
to
+146
| client, err := r.clientFactory(vaultURL, r.credential) | ||
| if err != nil { | ||
| return "", &KeyVaultResolveError{ | ||
| Reference: ref, | ||
| Reason: ResolveReasonClientCreation, | ||
| Err: fmt.Errorf("failed to create Key Vault client for %s: %w", vaultURL, err), | ||
| } | ||
| } | ||
|
|
||
| resp, err := client.GetSecret(ctx, parsed.SecretName, secretVersion, nil) | ||
| if err != nil { |
There was a problem hiding this comment.
KeyVaultResolver currently creates a new Key Vault client on every Resolve() call; ResolveMap() will therefore create N clients for N secrets, even when many secrets share the same vault. This is a noticeable perf regression vs the stated “per-vault caching” design and can add latency/connection overhead. Consider adding a thread-safe per-vault client cache (e.g., map[vaultURL]secretGetter guarded by a mutex or sync.Map).
Comment on lines
+212
to
+227
| for _, key := range keys { | ||
| value := refs[key] | ||
|
|
||
| if !IsSecretReference(value) { | ||
| result[key] = value | ||
| continue | ||
| } | ||
|
|
||
| resolved, err := r.Resolve(ctx, value) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("key %q: %w", key, err)) | ||
| continue | ||
| } | ||
|
|
||
| result[key] = resolved | ||
| } |
There was a problem hiding this comment.
ResolveMap omits entries whose values were secret references but failed to resolve (the key is not present in the returned map). This can unexpectedly drop configuration keys for callers that expect the output map to preserve the original shape. Consider explicitly setting result[key] on failure (e.g., to the original reference or to an empty string) so the caller can still reason about all input keys.
Comment on lines
+24
to
+31
| // Two reference formats are supported: | ||
| // | ||
| // akvs://<subscription-id>/<vault-name>/<secret-name> | ||
| // @Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<secret>[/<version>]) | ||
| // | ||
| // The akvs:// scheme is the preferred compact form. The @Microsoft.KeyVault | ||
| // format is the standard Azure App Configuration / App Service reference | ||
| // syntax, supported for backwards compatibility with existing configurations. |
There was a problem hiding this comment.
PR description lists support for the @Microsoft.KeyVault(VaultName=...;SecretName=...;SecretVersion=...) reference form, but the resolver documentation and parsing logic here only support the SecretUri= variant. Either add support for the VaultName/SecretName format or update the PR description/docs so consumers don’t assume it works.
| envVars []string, | ||
| defaultSubscriptionId string, | ||
| ) []string { | ||
| if kvService == nil || defaultSubscriptionId == "" { |
There was a problem hiding this comment.
ResolveSecretEnvironment currently returns early when defaultSubscriptionId is empty, which prevents resolving valid akvs:// references (they already embed the subscription ID). Consider only requiring defaultSubscriptionId when encountering an @Microsoft.KeyVault(...) reference (or move the check inside the app-ref branch) so akvs:// continues to work without AZURE_SUBSCRIPTION_ID being set.
Suggested change
| if kvService == nil || defaultSubscriptionId == "" { | |
| if kvService == nil { |
| // IsKeyVaultAppReference reports whether s uses the @Microsoft.KeyVault(SecretUri=...) format | ||
| // used by Azure App Service and App Configuration for Key Vault references. | ||
| func IsKeyVaultAppReference(s string) bool { | ||
| return strings.HasPrefix(s, keyVaultAppRefPrefix) && strings.HasSuffix(s, ")") |
There was a problem hiding this comment.
IsKeyVaultAppReference only checks the wrapper "@Microsoft.KeyVault(...)", so it will return true for other Azure-supported forms like "@Microsoft.KeyVault(VaultName=...;SecretName=...)". Those will then fail ParseKeyVaultAppReference (which expects SecretUri=) and, in ResolveSecretEnvironment, get blanked out. Either implement parsing for the VaultName/SecretName form or tighten detection so only supported variants return true.
Suggested change
| return strings.HasPrefix(s, keyVaultAppRefPrefix) && strings.HasSuffix(s, ")") | |
| // Must be wrapped as @Microsoft.KeyVault(...). | |
| if !strings.HasPrefix(s, keyVaultAppRefPrefix) || !strings.HasSuffix(s, ")") { | |
| return false | |
| } | |
| // Extract the inner expression between the prefix and the trailing ')'. | |
| inner := strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(s, keyVaultAppRefPrefix), ")")) | |
| // Only support the SecretUri=... variant; other Azure forms such as | |
| // VaultName=...;SecretName=... should return false here so they are | |
| // not misclassified as supported references. | |
| const secretUriPrefix = "SecretUri=" | |
| if !strings.HasPrefix(inner, secretUriPrefix) { | |
| return false | |
| } | |
| uriValue := strings.TrimSpace(inner[len(secretUriPrefix):]) | |
| return uriValue != "" |
Comment on lines
+555
to
+562
| if u.Host == "" { | ||
| return KeyVaultAppReference{}, fmt.Errorf( | ||
| "invalid @Microsoft.KeyVault reference %q: SecretUri must include a host", ref) | ||
| } | ||
|
|
||
| if !isValidVaultHost(u.Host) { | ||
| return KeyVaultAppReference{}, fmt.Errorf( | ||
| "invalid @Microsoft.KeyVault reference %q: host %q is not a known Azure Key Vault endpoint", ref, u.Host) |
There was a problem hiding this comment.
ParseKeyVaultAppReference validates the vault host using u.Host, which includes an optional port (e.g. "myvault.vault.azure.net:443"). That will fail isValidVaultHost suffix checks even though it’s still an Azure Key Vault endpoint. Using u.Hostname() for validation (and for building VaultURL) avoids false negatives while keeping the SSRF protection.
Suggested change
| if u.Host == "" { | |
| return KeyVaultAppReference{}, fmt.Errorf( | |
| "invalid @Microsoft.KeyVault reference %q: SecretUri must include a host", ref) | |
| } | |
| if !isValidVaultHost(u.Host) { | |
| return KeyVaultAppReference{}, fmt.Errorf( | |
| "invalid @Microsoft.KeyVault reference %q: host %q is not a known Azure Key Vault endpoint", ref, u.Host) | |
| host := u.Hostname() | |
| if host == "" { | |
| return KeyVaultAppReference{}, fmt.Errorf( | |
| "invalid @Microsoft.KeyVault reference %q: SecretUri must include a host", ref) | |
| } | |
| if !isValidVaultHost(host) { | |
| return KeyVaultAppReference{}, fmt.Errorf( | |
| "invalid @Microsoft.KeyVault reference %q: host %q is not a known Azure Key Vault endpoint", ref, host) |
Comment on lines
+600
to
+633
| // Resolution failures for individual variables are logged. On failure the | ||
| // value is set to an empty string rather than leaking the raw reference to | ||
| // downstream consumers who would not know how to handle it. | ||
| func ResolveSecretEnvironment( | ||
| ctx context.Context, | ||
| kvService KeyVaultService, | ||
| envVars []string, | ||
| defaultSubscriptionId string, | ||
| ) []string { | ||
| if kvService == nil || defaultSubscriptionId == "" { | ||
| return envVars | ||
| } | ||
|
|
||
| result := make([]string, len(envVars)) | ||
| for i, envVar := range envVars { | ||
| before, after, ok := strings.Cut(envVar, "=") | ||
| if !ok { | ||
| result[i] = envVar | ||
| continue | ||
| } | ||
|
|
||
| key := before | ||
| value := after | ||
|
|
||
| if !IsSecretReference(value) { | ||
| result[i] = envVar | ||
| continue | ||
| } | ||
|
|
||
| resolved, err := kvService.SecretFromKeyVaultReference(ctx, value, defaultSubscriptionId) | ||
| if err != nil { | ||
| log.Printf("warning: failed to resolve Key Vault reference for %s: %v", key, err) | ||
| result[i] = key + "=" // Empty value — don't leak the raw reference | ||
| continue |
There was a problem hiding this comment.
ResolveSecretEnvironment logs per-variable failures and then sets the env var to an empty value, but azd’s global log output is discarded unless debug logging is enabled. That makes secret resolution failures effectively silent and can cause extensions to run with missing configuration. Consider returning an error (or a (env, err) tuple) so the caller can decide whether to fail the command, warn via UX output, or proceed.
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.
Extension SDK: Key Vault Secret Resolution
Summary
Add
KeyVaultResolverto the Extension SDK for resolving Azure Key Vault secret references embedded in environment variables. This centralizes ~100+ lines of KV resolution logic that each extension currently duplicates independently.Extracted from PR #7025 to allow independent review of this feature.
Problem
Extensions running scripts or managing environments need to resolve Azure Key Vault references embedded in environment variables before passing them to child processes. Without framework support, each extension must independently implement regex parsing for multiple reference formats, thread-safe per-vault client caching, and credential management.
Supported Reference Formats
akvs://<vault>/<secret>[/<version>]@Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<secret>[/<version>])@Microsoft.KeyVault(VaultName=<vault>;SecretName=<secret>[;SecretVersion=<version>])Evidence
Extensions like azd-exec and azd-app each duplicate 100+ lines of KV resolution logic:
StopOnKeyVaultErrorconfig flag and factory pattern wrapping azd-core's resolverChanges
New Files
pkg/azdext/keyvault_resolver.goKeyVaultResolver— thread-safe per-vault client caching, batch resolution viaResolveMap, structured error types withResolveReasonclassification, configurable vault suffix for sovereign cloudspkg/azdext/keyvault_resolver_test.goModified Files
cmd/extensions.gokv.ResolveSecretEnvironment()internal/cmd/show/show.gopkg/keyvault/keyvault.goIsKeyVaultAppReference,IsSecretReference,ParseKeyVaultAppReference,SecretFromKeyVaultReference,ResolveSecretEnvironment,KeyVaultAppReferencetypeinternal/repository/detect_confirm_apphost.gocolorvariable was undefined on main)Key Design Decisions
azsecrets.Client, cached withsync.RWMutexfor concurrent accessResolveErrorincludesResolveReason(ParseFailed,VaultUnreachable,SecretNotFound,PermissionDenied,Unknown) for programmatic error handling.vault.azure.net)ResolveSecretEnvironmentresolves all KV refs in a map and returns a clean env mapTesting
ResolveMap)stubCredentialhelper included)Related