Skip to content

Conversation

@mccoyp
Copy link
Member

@mccoyp mccoyp commented Nov 21, 2025

Description

@Cherrett found an issue that currently only affects Managed HSM LROs, where the status monitor URL is lowercase even if the original request URL was not. The challenge cache is currently case sensitive, so the challenge flow is erroneously followed upon the first status monitor endpoint request.

This PR updates our challenge cache logic to always lower-case endpoints during operations and comparisons and tests this logic with a regression test.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

Copilot AI review requested due to automatic review settings November 21, 2025 03:54
@mccoyp mccoyp requested a review from a team as a code owner November 21, 2025 03:54
@mccoyp mccoyp added the Client This issue points to a problem in the data-plane of the library. label Nov 21, 2025
Copilot finished reviewing on behalf of mccoyp November 21, 2025 03:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a bug where Key Vault's challenge cache fails to retrieve cached challenges when URLs differ only in case (e.g., when Managed HSM LRO status monitor URLs are returned in lowercase). The fix makes the challenge cache case-insensitive by lower-casing all netloc values during cache operations.

Key Changes:

  • Modified challenge cache to lower-case netloc values in all cache operations (get, set, remove, validate)
  • Applied the fix consistently across all five Key Vault packages
  • Added regression test to verify case-insensitive behavior

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_shared/http_challenge_cache.py Added .lower() calls to make cache operations case-insensitive
sdk/keyvault/azure-keyvault-secrets/azure/keyvault/secrets/_shared/http_challenge_cache.py Added .lower() calls to make cache operations case-insensitive
sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_shared/http_challenge_cache.py Added .lower() calls to make cache operations case-insensitive
sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_internal/http_challenge_cache.py Added .lower() calls to make cache operations case-insensitive
sdk/keyvault/azure-keyvault-securitydomain/azure/keyvault/securitydomain/_internal/http_challenge_cache.py Added .lower() calls to make cache operations case-insensitive
sdk/keyvault/azure-keyvault-keys/tests/test_challenge_auth.py Added test_challenge_cache_casing to verify case-insensitive cache behavior

Comment on lines 62 to +65
parsed = parse.urlparse(url)

with _lock:
del _cache[parsed.netloc]
del _cache[parsed.netloc.lower()]
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The remove_challenge_for_url function directly uses parsed.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge can be set and retrieved, but cannot be removed correctly.

The fix should use _get_cache_key(url).lower() instead of parsed.netloc.lower():

key = _get_cache_key(url)
with _lock:
    del _cache[key.lower()]

Copilot uses AI. Check for mistakes.
Comment on lines 85 to +86
with _lock:
_cache[src_url.netloc] = challenge
_cache[src_url.netloc.lower()] = challenge
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The set_challenge_for_url function directly uses src_url.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge would be stored with the port included but retrieved with the port stripped, causing cache misses.

The fix should use _get_cache_key(url).lower() instead of src_url.netloc.lower():

key = _get_cache_key(url)
with _lock:
    _cache[key.lower()] = challenge

Note: The validation on line 82 should remain as-is since it's comparing the source URL's authority with the challenge's source authority, which is separate from the cache key logic.

Copilot uses AI. Check for mistakes.

with _lock:
_cache[src_url.netloc] = challenge
_cache[src_url.netloc.lower()] = challenge
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The set_challenge_for_url function directly uses src_url.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge would be stored with the port included but retrieved with the port stripped, causing cache misses.

The fix should use _get_cache_key(url).lower() instead of src_url.netloc.lower():

key = _get_cache_key(url)
with _lock:
    _cache[key.lower()] = challenge

Note: The validation on line 82 should remain as-is since it's comparing the source URL's authority with the challenge's source authority, which is separate from the cache key logic.

Suggested change
_cache[src_url.netloc.lower()] = challenge
key = _get_cache_key(url)
_cache[key.lower()] = challenge

Copilot uses AI. Check for mistakes.
Comment on lines 62 to +65
parsed = parse.urlparse(url)

with _lock:
del _cache[parsed.netloc]
del _cache[parsed.netloc.lower()]
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The remove_challenge_for_url function directly uses parsed.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge can be set and retrieved, but cannot be removed correctly.

The fix should use _get_cache_key(url).lower() instead of parsed.netloc.lower():

key = _get_cache_key(url)
with _lock:
    del _cache[key.lower()]

Copilot uses AI. Check for mistakes.
Comment on lines 62 to +65
parsed = parse.urlparse(url)

with _lock:
del _cache[parsed.netloc]
del _cache[parsed.netloc.lower()]
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The remove_challenge_for_url function directly uses parsed.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge can be set and retrieved, but cannot be removed correctly.

The fix should use _get_cache_key(url).lower() instead of parsed.netloc.lower():

key = _get_cache_key(url)
with _lock:
    del _cache[key.lower()]

Copilot uses AI. Check for mistakes.

with _lock:
_cache[src_url.netloc] = challenge
_cache[src_url.netloc.lower()] = challenge
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The set_challenge_for_url function directly uses src_url.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge would be stored with the port included but retrieved with the port stripped, causing cache misses.

The fix should use _get_cache_key(url).lower() instead of src_url.netloc.lower():

key = _get_cache_key(url)
with _lock:
    _cache[key.lower()] = challenge

Note: The validation on line 82 should remain as-is since it's comparing the source URL's authority with the challenge's source authority, which is separate from the cache key logic.

Suggested change
_cache[src_url.netloc.lower()] = challenge
_cache[_get_cache_key(url).lower()] = challenge

Copilot uses AI. Check for mistakes.
Comment on lines 62 to +65
parsed = parse.urlparse(url)

with _lock:
del _cache[parsed.netloc]
del _cache[parsed.netloc.lower()]
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The remove_challenge_for_url function directly uses parsed.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge can be set and retrieved, but cannot be removed correctly.

The fix should use _get_cache_key(url).lower() instead of parsed.netloc.lower():

key = _get_cache_key(url)
with _lock:
    del _cache[key.lower()]

Copilot uses AI. Check for mistakes.

with _lock:
_cache[src_url.netloc] = challenge
_cache[src_url.netloc.lower()] = challenge
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The set_challenge_for_url function directly uses src_url.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge would be stored with the port included but retrieved with the port stripped, causing cache misses.

The fix should use _get_cache_key(url).lower() instead of src_url.netloc.lower():

key = _get_cache_key(url)
with _lock:
    _cache[key.lower()] = challenge

Note: The validation on line 82 should remain as-is since it's comparing the source URL's authority with the challenge's source authority, which is separate from the cache key logic.

Suggested change
_cache[src_url.netloc.lower()] = challenge
key = _get_cache_key(url)
_cache[key.lower()] = challenge

Copilot uses AI. Check for mistakes.
Comment on lines 62 to +65
parsed = parse.urlparse(url)

with _lock:
del _cache[parsed.netloc]
del _cache[parsed.netloc.lower()]
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The remove_challenge_for_url function directly uses parsed.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge can be set and retrieved, but cannot be removed correctly.

The fix should use _get_cache_key(url).lower() instead of parsed.netloc.lower():

key = _get_cache_key(url)
with _lock:
    del _cache[key.lower()]

Copilot uses AI. Check for mistakes.

with _lock:
_cache[src_url.netloc] = challenge
_cache[src_url.netloc.lower()] = challenge
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The set_challenge_for_url function directly uses src_url.netloc.lower() as the cache key, but get_challenge_for_url uses _get_cache_key(url).lower() which has special logic to strip the default HTTPS port (:443). This inconsistency means that if a URL contains :443, the challenge would be stored with the port included but retrieved with the port stripped, causing cache misses.

The fix should use _get_cache_key(url).lower() instead of src_url.netloc.lower():

key = _get_cache_key(url)
with _lock:
    _cache[key.lower()] = challenge

Note: The validation on line 82 should remain as-is since it's comparing the source URL's authority with the challenge's source authority, which is separate from the cache key logic.

Suggested change
_cache[src_url.netloc.lower()] = challenge
_cache[_get_cache_key(url).lower()] = challenge

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. KeyVault

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

2 participants