Skip to content

fix(mcp): Preserve existing auth in mTLS transport#6201

Open
h-tsuboi918 wants to merge 8 commits into
google:mainfrom
h-tsuboi918:fix/mcp-mtls-auth-header-case
Open

fix(mcp): Preserve existing auth in mTLS transport#6201
h-tsuboi918 wants to merge 8 commits into
google:mainfrom
h-tsuboi918:fix/mcp-mtls-auth-header-case

Conversation

@h-tsuboi918

@h-tsuboi918 h-tsuboi918 commented Jun 23, 2026

Copy link
Copy Markdown

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

2. Or, if no issue exists, describe the change:

N/A

Problem:

The MCP mTLS auth bridge checked only for an exact Authorization header key before adding ADC credentials. httpx.Request.headers can be converted into a plain dict with lowercase authorization, so the existing user / 3LO Authorization header could be missed.

When that happens, ADK may inject ADC / runtime Service Account credentials into the same MCP request, causing the MCP server to observe the runtime principal instead of the intended user principal.

Solution:

Make _RefreshableAsyncCredentials.before_request() check for an existing Authorization header case-insensitively before refreshing and injecting credentials.

This matches HTTP header semantics and preserves existing behavior for correctly cased Authorization headers while also handling lowercase authorization from the httpx request path.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Passed locally:

uv run pytest tests/unittests/tools/mcp_tool/test_mcp_session_manager.py
51 passed, 8 warnings

Additional checks:

git diff --check
git diff --cached --check
uv run pyink --check src/google/adk/tools/mcp_tool/mcp_session_manager.py tests/unittests/tools/mcp_tool/test_mcp_session_manager.py
uv run isort --check-only src/google/adk/tools/mcp_tool/mcp_session_manager.py tests/unittests/tools/mcp_tool/test_mcp_session_manager.py

Manual End-to-End (E2E) Tests:

Not run. This is covered by a focused unit test for the credential injection guard. The changed behavior is isolated to preserving an existing lowercase authorization header before ADC refresh/injection.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

The issue is specific to the MCP mTLS transport path where ADK bridges httpx requests into google-auth-aio AsyncAuthorizedSession. The existing Authorization header may originate from MCP auth credential handling or a header provider, including user / 3LO token propagation.

No new code comments were added because the code change is limited to the existing Authorization header guard. There are no dependent downstream changes.

HTTP header names are case-insensitive, but the mTLS auth bridge only checked for an exact Authorization key before adding ADC credentials. httpx can pass the existing header as lowercase authorization, causing the user token to be missed.

Fixes google#6200
@h-tsuboi918 h-tsuboi918 force-pushed the fix/mcp-mtls-auth-header-case branch from 564cbe6 to c637b5f Compare June 23, 2026 17:14
@h-tsuboi918 h-tsuboi918 marked this pull request as ready for review June 23, 2026 17:14
@adk-bot adk-bot added the mcp [Component] Issues about MCP support label Jun 23, 2026
@h-tsuboi918 h-tsuboi918 marked this pull request as draft June 24, 2026 02:58
@rohityan rohityan added the request clarification [Status] The maintainer need clarification or more information from the author label Jun 26, 2026
@rohityan

Copy link
Copy Markdown
Collaborator

Hi @h-tsuboi918 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing ci test before we can proceed with a review.

@h-tsuboi918

Copy link
Copy Markdown
Author

Hi @rohityan ,Thanks for checking this.

I looked into the failed CI job:

https://github.com/google/adk-python/actions/runs/28122333977/job/83277473901?pr=6201

The failing step is File Content Compliance / Check for hardcoded googleapis.com endpoints.

The relevant workflow logic is here:

  • .github/workflows/continuous-integration.yml, lines 244-258
  • It computes the changed Python files with:
CHANGED_FILES=$(git diff --diff-filter=ACMR --name-only origin/${GITHUB_BASE_REF}...HEAD | grep -E '\.py$' || true)
  • It then scans the whole changed file for any googleapis.com URL matching:
FILES_WITH_ENDPOINTS=$(grep -lE 'https?://[a-zA-Z0-9.-]+\.googleapis\.com' $CHANGED_FILES)
  • It fails if that file does not also contain .mtls.googleapis.com:
FILES_MISSING_MTLS=$(grep -L '.mtls.googleapis.com' $FILES_WITH_ENDPOINTS)

The job log reports this file:

src/google/adk/tools/mcp_tool/mcp_session_manager.py

This PR does touch that file, but the matched googleapis.com string is not a hardcoded API endpoint introduced by this PR. It is an existing OAuth scope string:

  • src/google/adk/tools/mcp_tool/mcp_session_manager.py, line 637
scopes = ['https://www.googleapis.com/auth/cloud-platform']

That value is an OAuth scope, not a service endpoint that can or should have a corresponding .mtls.googleapis.com variant. So my understanding is that this CI failure is a false positive caused by applying an endpoint-oriented grep to the entire changed file.

If we want to fix this, I think the right place is the CI check rather than adding a dummy .mtls.googleapis.com string in this PR. More specifically, the candidate fix location is around .github/workflows/continuous-integration.yml, lines 244-258, especially the logic that builds FILES_WITH_ENDPOINTS / FILES_MISSING_MTLS.

For example, before treating a changed file as a file containing a googleapis.com endpoint, the check could classify matches at the URL level and exclude OAuth scopes first. Conceptually, this could look like:

FILES_WITH_ENDPOINTS=$(
  grep -HEo 'https?://[a-zA-Z0-9.-]+\.googleapis\.com[^"'\''[:space:]]*' $CHANGED_FILES \
    | grep -vE 'https://www\.googleapis\.com/auth(/|$)' \
    | cut -d: -f1 \
    | sort -u || true
)

Then the existing .mtls.googleapis.com counterpart check would run only on files that contain non-scope googleapis.com URLs.

The intended behavior would be:

  • still fail on hardcoded service endpoints such as https://foo.googleapis.com/...
  • do not fail on OAuth scopes such as https://www.googleapis.com/auth/cloud-platform
  • avoid requiring .mtls.googleapis.com for strings that are not endpoints

The actual change in this PR is limited to preserving an existing Authorization header in _RefreshableAsyncCredentials.before_request and adding a regression test for lowercase authorization:

  • src/google/adk/tools/mcp_tool/mcp_session_manager.py, line 264
  • the added regression test in tests/unittests/tools/mcp_tool/test_mcp_session_manager.py

For that reason, I do not think adding a dummy .mtls.googleapis.com string to mcp_session_manager.py would be the right fix for this PR, because the detected value is not a service endpoint. Should I include the CI check fix in this PR as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mcp [Component] Issues about MCP support request clarification [Status] The maintainer need clarification or more information from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP mTLS transport may inject ADC Authorization when an existing auth header is lowercase

3 participants