Skip to content

[EventGrid] Fix credential leak on cross-host redirect in legacy publisher (MSRC 126697)#48239

Open
Nishanth-MS wants to merge 14 commits into
mainfrom
Nishanth-MS/eventgrid-python-redirect-credential-leak
Open

[EventGrid] Fix credential leak on cross-host redirect in legacy publisher (MSRC 126697)#48239
Nishanth-MS wants to merge 14 commits into
mainfrom
Nishanth-MS/eventgrid-python-redirect-credential-leak

Conversation

@Nishanth-MS

Copy link
Copy Markdown
Member

Summary

Fixes a credential-leak vulnerability (MSRC 126697 / ICM 31000000661197) in the legacy EventGridPublisherClient.

When the server returns an HTTP 3xx redirect to a different host, the client re-attached the caller's credential header to the redirected request and sent it to the new host, disclosing the credential.

Root cause: the legacy client hand-rolls its pipeline (_policies()) and omitted azure-core's SensitiveHeaderCleanupPolicy. The stateless auth policies re-add the credential header on every redirect hop; RedirectPolicy flags the cross-domain hop (insecure_domain_change) but nothing consumed that flag to strip the header.

Fix

Add SensitiveHeaderCleanupPolicy after the authentication policy in both the sync (_legacy/_publisher_client.py) and async (_legacy/aio/_publisher_client_async.py) pipelines. The policy's default only covers Authorization, so the Event Grid SAS headers are added explicitly:

SensitiveHeaderCleanupPolicy(blocked_redirect_headers=[
    "Authorization", "x-ms-authorization-auxiliary",
    "aeg-sas-key", "aeg-sas-token",
])

This covers all three credential types:

  • AAD (TokenCredential) -> Authorization
  • Key (AzureKeyCredential) -> aeg-sas-key
  • SAS (AzureSasCredential) -> aeg-sas-token

Tests

tests/test_eg_redirect.py drives the pipeline through a mock transport that issues a cross-host 301 redirect and asserts the credential header is stripped from the redirected request, for all three credential types, sync and async.

Changelog / version

  • Bumped 4.22.0 -> 4.22.1 (backward-compatible security patch).
  • Added a Bugs Fixed CHANGELOG entry.

Notes

  • This is an MSRC security fix; resolving the incident additionally requires MSRC advisory/CVE coordination.
  • The same class of issue was found in the Go SDK (aeg-sas-key/aeg-sas-token leak by default) and as latent hardening gaps in JS/Java/.NET; those are tracked separately.

@Nishanth-MS
Nishanth-MS requested a review from shankarsama as a code owner July 24, 2026 10:18
Copilot AI review requested due to automatic review settings July 24, 2026 10:18
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
9 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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

Fixes cross-host redirect credential leakage in the legacy Event Grid publisher clients.

Changes:

  • Adds sensitive-header cleanup to sync and async pipelines.
  • Adds redirect regression tests for all credential types.
  • Bumps the patch version and updates the changelog.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_eg_redirect.py Tests credential stripping after redirects.
CHANGELOG.md Documents the security fix.
_version.py Bumps version to 4.22.1.
_legacy/_publisher_client.py Adds sync header cleanup.
_legacy/aio/_publisher_client_async.py Adds async header cleanup.
Comments suppressed due to low confidence (2)

sdk/eventgrid/azure-eventgrid/azure/eventgrid/_legacy/_publisher_client.py:138

  • This remains vulnerable when the redirected host returns a retryable response. The package still permits azure-core>=1.30.0, but SensitiveHeaderCleanupPolicy did not preserve the cross-domain flag across retries until azure-core 1.38.3; on older supported versions the retry auth pass can re-add the credential after cleanup. Require azure-core 1.38.3+ (adjusting Python support metadata as needed), or implement equivalent retry-safe cleanup locally, and cover a 301 -> 500 -> 200 sequence.
            SensitiveHeaderCleanupPolicy(

sdk/eventgrid/azure-eventgrid/azure/eventgrid/_legacy/aio/_publisher_client_async.py:132

  • The async pipeline has the same retry gap: with the declared azure-core>=1.30.0, versions before 1.38.3 lose the cross-domain flag after the first redirected send, allowing the async auth policy to restore the credential on a retry to the new host. Require azure-core 1.38.3+ (and reconcile Python support), or implement retry-safe cleanup locally, with a 301 -> 500 -> 200 regression test.
            SensitiveHeaderCleanupPolicy(

Comment thread sdk/eventgrid/azure-eventgrid/azure/eventgrid/_legacy/_publisher_client.py Outdated
Copilot AI review requested due to automatic review settings July 24, 2026 10:32

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

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

Comment thread sdk/eventgrid/azure-eventgrid/tests/test_eg_redirect.py Outdated
Comment thread sdk/eventgrid/azure-eventgrid/setup.py
@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings July 24, 2026 14:18
@Nishanth-MS

Copy link
Copy Markdown
Member Author

Thanks for the review — addressed all comments:

1. Inaccurate policy comment (sync + async): Reworded to note that SensitiveHeaderCleanupPolicy already covers Authorization and x-ms-authorization-auxiliary by default, and only the Event Grid SAS headers (aeg-sas-key/aeg-sas-token) are added here.

2. Retry gap before azure-core 1.38.3: Raised the minimum dependency to azure-core>=1.38.3 (verified: 1.38.3 is where insecure_domain_change persistence across retries was fixed) and added a 301 -> 500 -> 200 regression test asserting the credential stays stripped across the retry, for all three credential types.

3. Async AAD test used a sync credential: Added an AsyncFakeTokenCredential (async def get_token) and a separate ASYNC_CREDENTIAL_CASES so the async AAD path is exercised correctly (no TypeError).

4. Python 3.8 vs azure-core 1.38.3: azure-core dropped 3.8 in 1.33.0, so bumped python_requires to >=3.9 and removed the 3.8 classifier.

Note on the CI failures (pylint in _model_base.py/_serialization.py and test_cncf_events/test_cncf_events_async): these are in auto-generated files and the CloudEvents conversion path, none of which are touched by this PR (which only changes the 6 files above). They appear pre-existing/environmental (the pylint items are in generated code; the cncf failures track the cloudevents<=2.0.0 pin) rather than introduced by this change.

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

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

Copilot AI review requested due to automatic review settings July 24, 2026 14:26

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

sdk/eventgrid/azure-eventgrid/setup.py:68

  • Raising python_requires from 3.8 to 3.9 is a compatibility break, so this conflicts with the PR description's claim that 4.22.1 is backward-compatible. It also means Python 3.8 installations cannot receive this security release and will remain on 4.22.0. Please explicitly document the runtime support removal and adjust the compatibility claim, or provide a security-fix path that remains installable on 3.8.
    python_requires=">=3.9",

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings July 24, 2026 15:56

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

sdk/eventgrid/azure-eventgrid/setup.py:68

  • This makes 4.22.1 incompatible with Python 3.8 even though the PR describes the patch as backward-compatible. The package README still says Python 3.8 is supported (README.md:20), and the existing changelog promises Python 3.8+ for future versions (CHANGELOG.md:70). Because azure-core 1.38.3 itself requires Python 3.9+, either explicitly document this runtime-support drop in the release notes/PR or provide a security-fix path that remains installable on the previously supported runtime.
    python_requires=">=3.9",

Comment thread .github/CODEOWNERS
# ServiceLabel: %Event Grid
# PRLabel: %Event Grid
/sdk/eventgrid/ @rajeshka @shankarsama
/sdk/eventgrid/ @shankarsama @Nishanth-MS
@github-actions

Copy link
Copy Markdown
Contributor
[Pilot] PR Pipeline Failure Analysis

A CI pipeline failed on this pull request. Here is an automated analysis of what went wrong and how to get the build green.

What failed

Two distinct failure categories were detected in the azure-eventgrid CI pipeline (build #6613897):

  1. Test failurestest_cncf_events (sync) and test_cncf_events_async failed consistently across all tested platforms and package formats (macOS 3.11, Ubuntu 3.10/3.13/3.14, Windows 3.12 — whl, sdist, and mindependency variants). The uniform failure across platforms and packaging modes suggests a code regression rather than an environment issue.

  2. Pylint validation failure (exit code 20) — Linting errors found in generated files:

    • azure/eventgrid/_model_base.py (lines 777, 780, 786): C0123 unidiomatic-typecheck — use isinstance() instead of type().
    • azure/eventgrid/_serialization.py (lines 192, 201, 210, 233, 236, 242): W0613 unused-argument in UTC/_FixedOffset timezone methods.

Recommended next steps

  • Investigate test_cncf_events / test_cncf_events_async: Check whether the PR changes (adding SensitiveHeaderCleanupPolicy to the pipeline or bumping the version to 4.22.1) inadvertently broke the CNCF cloud-events code path. Review the full test failure output in the build logs for the specific assertion error.
  • Fix pylint issues in generated files: The unidiomatic-typecheck and unused-argument warnings are in _model_base.py and _serialization.py. If these are auto-generated files, regenerate them from the source spec; otherwise apply the suggested pylint fixes (replace type(x) == Y with isinstance(x, Y) and suppress or fix the unused-argument warnings).
  • See the CI troubleshooting guide: https://aka.ms/ci-fix
  • Push new commits to address the failures; this comment updates automatically on the next failing run.
Raw pipeline analysis (azsdk ci analyze)
Failed Tests:
  sdk.eventgrid.azure-eventgrid.tests.test_cncf_events
  sdk.eventgrid.azure-eventgrid.tests.test_cncf_events_async
  (across all platforms: macos311, Ubuntu2404_313, ubuntu2404_310,
   ubuntu2404_310_coverage, windows2022_312, Ubuntu2404_314
   — whl, sdist, mindependency variants)

Pylint failures in azure-eventgrid:
  _model_base.py:777,780,786  C0123(unidiomatic-typecheck) Use isinstance() rather than type()
  _serialization.py:192,201,210,233,236,242  W0613(unused-argument)
  Score: 9.98/10 — exited with code 20 (FAIL)
  See: https://aka.ms/azsdk/python/pylint-guide

Pipeline: https://dev.azure.com/azure-sdk/public/_build/results?buildId=6613897

Copilot detected the failing pipeline and generated the analysis above. To have it attempt a fix automatically, reply with `@copilot please fix the failing pipeline on this PR`.

Generated by Pipeline Analysis - Next Steps · 35 AIC · ⌖ 6.16 AIC · ⊞ 6.7K ·

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.

2 participants