[keyvault-certificates] Fix begin_create_certificate validator to accept san_ip_addresses and san_uris#46380
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Key Vault Certificates client-side policy validation so SAN-only policies using IP addresses and/or URIs are accepted, and adds coverage + recordings to prevent regressions.
Changes:
- Update sync/async certificate creation validators to treat
san_ip_addressesandsan_urisas satisfying the “SAN or subject required” rule. - Add unit assertions and new live tests (sync + async) validating IP/URI-only SAN policies; refresh test recordings/assets tag.
- Adjust live-test credential environment-variable handling to avoid empty-string
AZURE_*values interfering withDefaultAzureCredential.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py | Expands validator to include san_ip_addresses and san_uris. |
| sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py | Same validator expansion for async create_certificate. |
| sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py | Adds sync live test + extends validator-behavior assertions. |
| sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py | Adds async live test + extends validator-behavior assertions. |
| sdk/keyvault/azure-keyvault-certificates/tests/_test_case.py | Avoids setting empty-string AZURE_* env vars during live tests. |
| sdk/keyvault/azure-keyvault-certificates/tests/_async_test_case.py | Same env-var adjustment for async tests. |
| sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md | Documents the fix in 4.11.1 section. |
| sdk/keyvault/azure-keyvault-certificates/assets.json | Updates assets tag for new recordings. |
| sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_models.py | Changes KeyProperties.tags typing to Optional[Dict[str, str]] (appears unrelated to this PR’s stated scope). |
| # san_ip_addresses alone should be accepted (no ValueError) | ||
| policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self, san_ip_addresses=["10.0.0.1"]) | ||
| try: | ||
| await client.create_certificate("...", policy=policy) | ||
| except ValueError: | ||
| pytest.fail("create_certificate should not raise ValueError for san_ip_addresses-only policy") | ||
| except Exception: | ||
| pass # Expected: network/auth error since we are using a fake client | ||
|
|
||
| # san_uris alone should be accepted (no ValueError) | ||
| policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self, san_uris=["https://example.com"]) | ||
| try: | ||
| await client.create_certificate("...", policy=policy) | ||
| except ValueError: | ||
| pytest.fail("create_certificate should not raise ValueError for san_uris-only policy") | ||
| except Exception: | ||
| pass # Expected: network/auth error since we are using a fake client |
There was a problem hiding this comment.
The new assertions use except Exception: pass to allow the fake client call to fail, but this will also swallow unexpected exceptions (e.g., AttributeError/TypeError from a regression in the validator or call signature) and let the test pass. Suggest catching only the expected request/auth exceptions (e.g., azure-core ServiceRequestError/ClientAuthenticationError/HttpResponseError) and letting other exceptions fail the test.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Done in commit e44579a. Replaced except Exception: pass with except (HttpResponseError, ServiceRequestError): pass in both test_certificates_client.py and test_certificates_client_async.py, and added HttpResponseError/ServiceRequestError to the azure.core.exceptions imports. Also removed the redundant await client.close() in the async live test (the preparer's context manager already closes the client).
| # san_ip_addresses alone should be accepted (no ValueError) | ||
| policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self, san_ip_addresses=["10.0.0.1"]) | ||
| try: | ||
| client.begin_create_certificate("...", policy=policy) | ||
| except ValueError: | ||
| pytest.fail("begin_create_certificate should not raise ValueError for san_ip_addresses-only policy") | ||
| except Exception: | ||
| pass # Expected: network/auth error since we are using a fake client | ||
|
|
||
| # san_uris alone should be accepted (no ValueError) | ||
| policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self, san_uris=["https://example.com"]) | ||
| try: | ||
| client.begin_create_certificate("...", policy=policy) | ||
| except ValueError: | ||
| pytest.fail("begin_create_certificate should not raise ValueError for san_uris-only policy") | ||
| except Exception: | ||
| pass # Expected: network/auth error since we are using a fake client |
There was a problem hiding this comment.
The new assertions use except Exception: pass to allow the fake client call to fail, but this will also swallow unexpected exceptions (e.g., AttributeError/TypeError from a regression in the validator or call signature) and let the test pass. Suggest catching only the expected request/auth exceptions (e.g., azure-core ServiceRequestError/ClientAuthenticationError/HttpResponseError) and letting other exceptions fail the test.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| @property | ||
| def tags(self) -> Dict[str, str]: | ||
| def tags(self) -> Optional[Dict[str, str]]: | ||
| """Application specific metadata in the form of key-value pairs. | ||
|
|
||
| :returns: A dictionary of tags attached to the key. | ||
| :rtype: dict[str, str] | ||
| :rtype: dict[str, str] or None |
There was a problem hiding this comment.
This PR is scoped to azure-keyvault-certificates, but it also changes the public type signature of KeyProperties.tags in azure-keyvault-keys (now Optional[Dict[str, str]]). If this is intentional, it likely needs to be called out separately (e.g., in the PR description and/or the keys package changelog) because it affects the API surface/type-checking for a different package. If unintentional, consider reverting it from this PR.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… san_uris - Extended the policy validator in both the sync and async CertificateClient to also accept policies that specify only san_ip_addresses or san_uris, which are valid SAN types but were previously treated as missing subject information - Added test_create_certificate_with_san_ip_and_uris (sync + async) with live recordings to verify IP-only and URI-only SAN policies succeed end-to-end - Extended test_policy_expected_errors_for_create_cert to assert that IP-only and URI-only policies no longer raise ValueError - Fixed _test_case.py and _async_test_case.py to avoid setting AZURE_CLIENT_ID/ AZURE_CLIENT_SECRET to empty strings, which newer azure-identity treats as an attempt to use ClientSecretCredential and raises ValueError
rohitsinghal4u
force-pushed
the
fix/keyvault-certificates-san-ip-uri-validator
branch
from
8e9343f to
db3c5f8
Compare
…s://github.com/Azure/azure-sdk-for-python into fix/keyvault-certificates-san-ip-uri-validator
Summary
Fixes a bug where
CertificateClient.begin_create_certificate(and its async counterpartcreate_certificate) incorrectly raisedValueErrorwhen aCertificatePolicywas created with onlysan_ip_addressesorsan_uris(with nosubject,san_dns_names,san_emails, orsan_user_principal_names).IP addresses and URIs are valid Subject Alternative Name (SAN) types and should be accepted by the client's policy validator.
Root Cause
The validator in both
_client.pyandaio/_client.pychecked:san_ip_addressesandsan_uriswere missing from this condition.Changes
azure/keyvault/certificates/_client.py— addedsan_ip_addressesandsan_uristo the validator condition inbegin_create_certificateazure/keyvault/certificates/aio/_client.py— same fix for the async client'screate_certificateCHANGELOG.md— added 4.11.1 (Unreleased) section documenting the fixtests/test_certificates_client.py— extendedtest_policy_expected_errors_for_create_certto assert IP/URI-only policies no longer raise; addedtest_create_certificate_with_san_ip_and_urislive test (recorded)tests/test_certificates_client_async.py— same test additions for the async clienttests/_test_case.pyandtests/_async_test_case.py— fixed a secondary issue where empty-string values forAZURE_CLIENT_ID/AZURE_CLIENT_SECRETwere set into the environment, causing newer versions ofazure-identity'sEnvironmentCredential(which checksis not Nonerather than truthiness) to attempt and failClientSecretCredentialconstruction during developer auth. Now empty-string KEYVAULT_* vars cause the corresponding AZURE_* vars to be removed from the env, allowingDefaultAzureCredentialto fall through toAzureCliCredential.assets.json— updated tag topython/keyvault/azure-keyvault-certificates_a5c5c34814(new recordings pushed)Testing
https://kv-sdk-test-5903.vault.azure.net/test_policy_expected_errors_for_create_cert(sync + async) updated and verified