Skip to content

[Key Vault] az keyvault: Add host validation for the resolved vault URI - #33975

Open
Yash (notyashhh) wants to merge 2 commits into
devfrom
yash/msrc-123589-keyvault-uri-validation
Open

[Key Vault] az keyvault: Add host validation for the resolved vault URI#33975
Yash (notyashhh) wants to merge 2 commits into
devfrom
yash/msrc-123589-keyvault-uri-validation

Conversation

@notyashhh

@notyashhh Yash (notyashhh) commented Aug 25, 2026

Copy link
Copy Markdown
Member

🤖 PR Validation — ️✔️ All clear

Breaking Changes Tests
️✔️ None ️✔️ 130/130

Related command

az keyvault key/secret/certificate/security-domain/role/setting/backup/restore — all data-plane commands.

Description

Every az keyvault data-plane command resolves its target vault URL from --id, --vault-name or --hsm-name and passes it straight to the Key Vault SDK client without checking where it points. The clients are also constructed with verify_challenge_resource=False, so the SDK does not verify the authentication challenge against the contacted host either.

The Key Vault library guidance (https://aka.ms/azsdk/blog/vault-uri) requires applications that accept user-provided vault URIs to validate them, and calls this out specifically when verify_challenge_resource is disabled. This PR adds that missing validation.

validate_vault_uri() is added to _validators.py and called from:

  • _prepare_data_plane_azure_keyvault_client() — the single choke point for all eight data-plane clients, so --id, --vault-name and --hsm-name are all covered;
  • both completers in _completers.py, which build clients during tab completion.

A URI is accepted only if it is an absolute https URI, carries no userinfo, has well-formed DNS labels, and its host ends with the active cloud's keyvaultDns or mhsmDns suffix. Suffixes are compared with a leading . so that maliciousvault.azure.net does not match .vault.azure.net. The validated origin is returned and used as the client's vault_url. Validation runs before credentials are acquired.

Private or disconnected deployments whose suffixes are not published in the cloud metadata can register them:

az config set keyvault.allowed_dns_suffixes=.vault.contoso.local

Note for reviewers — behaviour change: a command whose resolved host falls outside the current cloud's Key Vault / Managed HSM suffixes now fails with InvalidArgumentValueError instead of attempting the request. Sovereign clouds are unaffected; each validates against its own suffixes.

Testing Guide

Accepted as before:

az keyvault secret show --vault-name mykv -n mysecret
az keyvault secret show --id https://mykv.vault.azure.net/secrets/mysecret
az keyvault key list --hsm-name myhsm

Now rejected before any request is made:

az keyvault secret show --id https://example.com/secrets/mysecret
az keyvault secret show --id https://maliciousvault.azure.net/secrets/mysecret

15 unit tests added in test_validators.py covering vault and Managed HSM hosts (including multi-level regional MHSM names), origin normalisation, sovereign clouds, cross-cloud rejection, suffix look-alikes, malformed DNS labels, non-HTTPS schemes, embedded credentials and the configured suffix allow-list.

pylint and flake8 are clean on the module, and the existing keyvault test suite shows no change in results.


This checklist is used to make sure that common guidelines for a pull request are followed.

Copilot AI lite review requested due to automatic review settings August 25, 2026 00:53
@notyashhh
Yash (notyashhh) requested a review from a team as a code owner August 25, 2026 00:53
@yonzhan

Copy link
Copy Markdown
Collaborator

Thank you for your contribution! We will review the pull request and get back to you soon.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds host validation for resolved Key Vault and Managed HSM URIs before authentication and completion requests.

Changes:

  • Validates HTTPS origins, DNS labels, userinfo, and cloud suffixes.
  • Integrates validation into client factories and completers.
  • Adds validator tests for cloud, malformed, and configured-suffix cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Summary
src/azure-cli/azure/cli/command_modules/keyvault/tests/latest/test_validators.py Adds coverage for accepted and rejected vault URI scenarios.
src/azure-cli/azure/cli/command_modules/keyvault/_validators.py Implements URI validation. Critical (3 votes): empty userinfo can bypass the current check. Moderate (3 votes): credentials may be exposed in error messages. Moderate (2 votes): configured private suffixes are not applied during name-to-URL conversion.
src/azure-cli/azure/cli/command_modules/keyvault/_completers.py Applies validation during tab completion.
src/azure-cli/azure/cli/command_modules/keyvault/_client_factory.py Validates vault URLs before acquiring credentials and creating clients.
Suppressed comments (1)

src/azure-cli/azure/cli/command_modules/keyvault/_validators.py:596

  • rstrip('.') removes every terminal dot before DNS-label validation. Consequently https://myvault.vault.azure.net.. is reduced to the valid hostname myvault.vault.azure.net, passes the suffix check, and is returned with a malformed authority. Remove at most one optional DNS root terminator so extra dots leave an empty label and are rejected.
    hostname = hostname.rstrip('.').lower()

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +591 to +592
if parsed.username or parsed.password:
raise _invalid('it must not contain credentials')
Comment on lines +573 to +575
def _invalid(reason):
return InvalidArgumentValueError(
"'{}' is not a valid Key Vault or Managed HSM URI: {}.".format(uri, reason))
Comment on lines +551 to +554
# Escape hatch for private/disconnected footprints whose suffixes aren't in the cloud metadata.
configured = cli_ctx.config.get('keyvault', 'allowed_dns_suffixes', None)
if configured:
suffixes.extend(configured.split(','))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants