[confidentialledger] Enforce stricter redirect destination policy#47368
Open
ryazhang-microsoft wants to merge 4 commits into
Open
[confidentialledger] Enforce stricter redirect destination policy#47368ryazhang-microsoft wants to merge 4 commits into
ryazhang-microsoft wants to merge 4 commits into
Conversation
Restrict redirects followed by the data-plane redirect policy to the original ledger host or one of its subdomains (e.g. an individual node). Redirects to sibling ledgers, parent domains, unrelated hosts, and look-alike suffix domains are now rejected, logged at WARNING level, and never followed or cached. - Add _is_allowed_redirect_target() host/subdomain check (case-insensitive, port-agnostic, fail-safe when a host is missing). - Enforce it in both RedirectCachingPolicy.send and AsyncRedirectCachingPolicy.send by capturing the pristine request URL before any cache rewrite. - Add unit tests covering allowed (same host, subdomains) and disallowed (sibling, parent, unrelated, suffix look-alike) targets, plus caplog assertions for the block warning. - Update CHANGELOG. Fixes ICM 31000000622491.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens azure-confidentialledger’s custom redirect-caching policies to prevent sensitive headers from being forwarded to unintended redirect destinations by only following redirects whose target host is the original ledger host or one of its subdomains, and adds unit coverage for the new rules.
Changes:
- Added
_is_allowed_redirect_target(original_url, target_url)and integrated it into sync/async redirect-following loops to refuse disallowed redirect targets and emit aWARNING. - Updated/expanded unit tests to cover allowed vs disallowed redirect targets and to assert warning logs for blocked redirects.
- Added a CHANGELOG entry describing the stricter redirect destination policy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/confidentialledger/azure-confidentialledger/azure/confidentialledger/_redirect_caching_policy.py | Adds redirect-target allowlisting and warning logging to prevent following/caching redirects to disallowed hosts. |
| sdk/confidentialledger/azure-confidentialledger/tests/test_redirect_caching_policy.py | Adds unit tests for the allowlist helper and ensures disallowed redirects are not followed/cached (sync + async), including caplog assertions. |
| sdk/confidentialledger/azure-confidentialledger/CHANGELOG.md | Documents the stricter redirect destination policy in the next release notes. |
…t targets Address review feedback: the redirect destination check validated only the host, so an https->http downgrade or a same-host-different-port target would pass (urlparse(...).hostname drops scheme/port). Because the redirected request reuses the same request object downstream of auth, the bearer token could be sent over cleartext or to an unexpected port. Match the JS/.NET fix: a redirect is now followed only when the target uses HTTPS, has the same effective port (scheme default applied when absent), and the host is the original host or a subdomain. - Add _effective_port() helper (explicit port, else 443/80 by scheme). - Reject non-HTTPS targets and effective-port mismatches in _is_allowed_redirect_target. - Add unit tests for http downgrade and different-port targets (host and subdomain) at both the helper and policy.send levels; extend the sync/async parametrized block lists. - Update CHANGELOG and module docstring.
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
Hardens the data-plane redirect handling in
azure-confidentialledgerso that the client only follows HTTP redirects whose target host is the original ledger host or one of its subdomains. Redirects to sibling ledgers, parent domains, unrelated hosts, or look-alike suffix domains are now rejected, logged atWARNING, and never followed or cached.Fixes ICM 31000000622491.
Problem
The custom
RedirectCachingPolicy/AsyncRedirectCachingPolicy(used in place of the defaultRedirectPolicy) followed anyLocationtarget returned by the service. Because Confidential Ledger intentionally disables sensitive-header cleanup on redirects (disable_redirect_cleanup=True) to preserve auth headers across service-managed redirects within the ledger, a misconfigured or malicious load balancer could redirect a request — along with its sensitive headers — to an unintended destination.Policy
Given an original request to
https://test-ledger.confidential-ledger.azure.com, redirects are now evaluated as follows:https://test-ledger.confidential-ledger.azure.com/...(same host)https://node3.test-ledger.confidential-ledger.azure.com/...(subdomain / node)https://primary-1.test-ledger.confidential-ledger.azure.com/...(subdomain / node)https://other-ledger.confidential-ledger.azure.com/...(sibling ledger)https://confidential-ledger.azure.com/...(parent domain)https://evil.example.com/...(unrelated host)https://test-ledger.confidential-ledger.azure.com.evil.com/...(suffix look-alike)Changes
_is_allowed_redirect_target(original_url, target_url)— host/subdomain check that is case-insensitive, ignores port, and fails safe (rejects) if either host can't be parsed. Logic:target == original or target.endswith("." + original).send()now capture the pristine request URL (before any cached-primary rewrite) and validate every redirect hop against it. Disallowed targets are logged (Refusing to follow redirect to disallowed target: ...) and the redirect loop breaks without following or caching.2.0.0b3 (Unreleased).Testing
Unit tests (
tests/test_redirect_caching_policy.py) — 39 passing:_is_allowed_redirect_target: same host, subdomains, port/case variations (allowed); sibling, parent, unrelated, suffix look-alike, missing-host (blocked).307returned as-is, cache stays empty); legitimate subdomain redirects are still followed and cached.caplogassertions that theRefusing to follow redirect to disallowed targetwarning is emitted (sync + async).Live validation against a real ledger (AAD auth):
accledger-2.<ledger>.confidential-ledger.azure.comsubdomain is correctly allowed, followed, cached, and reused on subsequent writes — confirming the legitimate path the service depends on is preserved.Scope / notes
azure-confidentialledgerredirect policy. The certificate client andazure-mgmt-confidentialledgeruse the generated defaultRedirectPolicyand are out of scope.