Repository navigation
fix(agent-os): enforce TLS for configured MCP URLs - #3793
MohammadHaroonAbuomar merged 2 commits into
Conversation
Signed-off-by: Artem Parshin <me@apararti.ru>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
@microsoft-github-policy-service agree |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Ran this branch's suite locally against its own tree (PYTHONPATH=src python -m pytest tests/test_mcp_auth_enforcement.py): 35 passed. The fix is correct.
Confirmed the defect on main: if entry.require_tls and url: means a registered entry configured
with url: http://mcp.internal/api and require_tls: true is allowed whenever the caller omits the
URL. Since the parameter was documented as "for logging/audit", omitting it is the natural thing for
a caller to do, so this was the default path rather than an edge case.
The parametrized test is the best part of this PR. Sweeping the full nine-way caller-URL by
configured-URL matrix crossed with require_tls states the property rather than sampling it, and it
pins the precedence in both directions: an https caller against an http entry is allowed, an
http caller against an https entry is denied. That second case is the one a hand-written test set
tends to miss, and getting it backwards would let a config value override where the connection is
actually going.
Overlap. #3797 fixes the same defect with the same mechanism and is functionally identical to
this branch, right down to effective_url = url or entry.url. It was opened against the same issue
(#3785). It carries twelve named tests reaching schemes this branch does not exercise (ws, wss,
ftp, bare hostname with no scheme), an end-to-end YAML case, and a new docs/mcp-auth-tls.md. The
two edit the same lines, so they will conflict.
My read for the maintainers is that #3797 is the one to land on coverage, and that this branch's
parametrized matrix is worth carrying across afterwards, because it is a tidier statement of the
precedence property than #3797's named pair. That is a genuine contribution and it should not be
lost just because the code change is the same on both.
Also worth knowing: #3849 closes the sibling hole (#3814, unregistered server names skipping the
gate entirely) in the fallback branch of the same method, and does not conflict with either of these.
The three of them together are what actually closes the TLS floor.
Nothing blocking on the change itself.
|
The only CI run on this head is from mid-August and its required checks have expired, so the merge gate cannot see them. Please rebase onto current main and force-push to trigger a fresh run. The content is already reviewed and approved; nothing else is needed. |
Signed-off-by: Artem Parshin <me@apararti.ru> Co-authored-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com> Signed-off-by: yuvrajsingh2428 <offcyuvi2428@gmail.com>
3 participants
Related Issue
Fixes #3785
Problem & Solution
McpAuthPolicy.check()enforcedrequire_tlsonly against the URL supplied by the caller. When the caller omitted that argument, a non-TLS URL already stored in the matchingMcpServerEntrywas never evaluated.This PR selects the effective URL as the caller URL or the configured entry URL. The caller URL retains priority because it describes the connection being checked. If both sources are empty, the existing behavior remains unchanged and the TLS gate is skipped, preserving S10.12 compatibility.
Impact on Your Work
Configured MCP server URLs now receive the same TLS scheme enforcement as caller-supplied URLs.
Timeline
None.
Alternatives Considered
Type of Change
Package(s) Affected
Core & runtime:
Governance & security:
Platform & tooling:
CLI plugins:
Shared / other:
Testing
Unit Testing
Added an 18-case parameterized matrix covering:
require_tls=TrueandFalseManual Testing
git diff --checkpasses. Ruff reports the same four existing file-level findings asmain; none are introduced by the changed lines. The direct conformance module was not collectable in this local checkout because the compiledagent_control_specification._nativemodule is unavailable; the regression matrix directly pins the S10.12 empty-both behavior.Checklist
Attribution & Prior Art
Prior art / related projects (if any):
The configured-URL fallback was previously discussed within this repository in #3512 and was raised again in #3785. No external project code was copied or adapted.
AI Assistance
Codex was used to inspect the issue, draft the regression matrix and minimal patch, and prepare this PR text. All changes were reviewed and explicitly approved by the contributor before submission.
IP, Patents, and Licensing