Repository navigation
Conversation
`McpAuthPolicy.check` enforced `require_tls` only against the URL the caller
passed. That parameter defaults to `""` and the gate was skipped entirely when
it was falsy, so `entry.url` -- written by both constructors and by
`from_yaml`, documented on `McpServerEntry` and in the gateway spec -- was
never read by any code path.
A server registered with a plaintext URL and `require_tls=True` was therefore
allowed by any caller that did not repeat the URL:
policy = McpAuthPolicy(servers=[McpServerEntry(
name="finance", url="http://mcp.internal/finance",
allowed_auth_methods=["mtls"], require_tls=True)])
policy.check("finance", "mtls").allowed # True
policy.check("finance", "mtls", url="http://mcp.internal/finance") # False
Both calls describe the same connection to the same server under the same
policy. Which one fires depended on whether the caller redundantly restated
configuration the policy object already held -- and the `from_yaml` path, where
the URL lives in config and the call site has no reason to pass it again, always
took the permissive branch.
The URL now falls back to `entry.url`. A caller-supplied URL keeps priority,
since it describes the connection actually being dialed and may legitimately
differ from the registered pattern. With no URL from either source the gate
stays skipped as before; denying every server registered without a URL is a
separate policy decision.
Scheme inspection moves to `_tls_violation`, which reports a scheme-less URL --
the shape a bare host or a glob pattern takes -- as "TLS cannot be verified"
rather than as `scheme '' is not in the allowlist`. That case was already denied
by set membership; it is now denied on purpose, with a reason that says why,
and covered by a test. This path becomes reachable from configured URLs for the
first time with this change.
Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your detailed contribution! Your test coverage is excellent, ensuring the fix is well-verified. Before merging, please check:
Refer to CONTRIBUTING.md for guidance. |
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 0 warnings. The PR resolves a critical security gap in the
No action items or warnings. Clean change. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
Notes
|
🤖 AI Agent: test-generator — `agent-os/src/agent_os/mcp_auth_enforcement.py`
|
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
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. |
There was a problem hiding this comment.
Pull request overview
TL;DR: 0 blockers, 2 warnings. Warnings are fine as follow-ups.
This PR fixes a correctness gap in agent_os MCP auth enforcement: require_tls is now enforced against the effective server URL (caller url if provided, otherwise the server’s configured entry.url), and the TLS scheme check is centralized in a helper. It also adds regression tests covering the previously fail-open configuration path (notably from_yaml).
Changes:
- Enforce
require_tlsagainsteffective_url = url or entry.url, so configured plaintext URLs are denied even when callers omiturl. - Refactor scheme validation into
_tls_violation()and introduce aTLS_SCHEMESmodule constant. - Add a dedicated test suite covering configured URL enforcement, schemeless URLs, override behavior, and YAML parsing.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Warn | New tests use is True / is False boolean identity assertions; rest of file uses truthiness |
tests/test_mcp_auth_enforcement.py |
| 2 | Warn | Inline comment inaccurately describes prior TLS logic as prefix-based / bypassed by non-http schemes | src/agent_os/mcp_auth_enforcement.py |
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
agent-governance-python/agent-os/src/agent_os/mcp_auth_enforcement.py |
Applies TLS gate to configured URL via effective_url and factors TLS scheme logic into _tls_violation() with TLS_SCHEMES. |
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py |
Adds regression tests ensuring require_tls applies to YAML/configured URLs and covers schemeless/non-TLS schemes. |
Comments suppressed due to low confidence (6)
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:148
- Avoid
... is Truein assertions; preferassert exprfor consistency with the rest of the test module and to avoid boolean-identity comparisons.
@pytest.mark.parametrize("url", ["https://mcp.internal/finance", "wss://mcp.internal/ws"])
def test_tls_configured_url_is_allowed(self, url: str) -> None:
assert self._policy(url).check("finance", auth_method="mtls").allowed is True
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:161
- These assertions use
is True/is False. In this file, other tests use truthiness (assert .../assert not ...), which is clearer and avoids boolean-identity comparisons.
assert policy.check("finance", auth_method="mtls").allowed is True
assert (
policy.check("finance", auth_method="mtls", url="http://mcp.internal/finance").allowed
is False
)
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:168
- Avoid
... is Truein assertions; preferassert exprfor consistency with the rest of the test module.
def test_no_url_anywhere_leaves_the_gate_skipped(self) -> None:
# Unchanged behaviour: with no URL from either source there is nothing
# to evaluate. Denying this would be a separate policy decision about
# every server registered without a URL.
assert self._policy("").check("finance", auth_method="mtls").allowed is True
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:172
- Avoid
... is True/... is Falsein assertions; preferassert expr/assert not exprto match the style used elsewhere in this test file.
def test_require_tls_false_allows_plaintext(self) -> None:
policy = self._policy("http://mcp.internal/finance", require_tls=False)
assert policy.check("finance", auth_method="mtls").allowed is True
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:178
- Avoid
is Falsewhen asserting on booleans; the rest of this test module usesassert not result.allowedfor denials.
result = self._policy("http://mcp.internal/finance").check("finance", auth_method="oauth2")
assert result.allowed is False
assert "not allowed for server" in result.reason
agent-governance-python/agent-os/tests/test_mcp_auth_enforcement.py:189
- Avoid
... is Falsein assertions; preferassert not ...for consistency with the rest of the tests.
assert policy.check("finance-tools", auth_method="mtls").allowed is False
| def test_plaintext_configured_url_is_denied_without_a_caller_url(self, url: str) -> None: | ||
| result = self._policy(url).check("finance", auth_method="mtls") | ||
| assert result.allowed is False | ||
| assert "requires TLS" in result.reason |
There was a problem hiding this comment.
Fixed in 3e349183 — switched to truthiness throughout.
You're right that it's inconsistent: the other 9 assertions in this file already use
assert not result.allowed, and mine were the odd ones out.
One note on the suggested alternatives, since they aren't equivalent here. == False is
ruff E712 and this package enables it (select = ["E"] in agent-os/pyproject.toml), so
it would fail lint:
E712 Avoid equality comparisons to `False`; use `not result.allowed` for false checks
not ... is clean, so that's what I used — which is also what you led with and what the
file already does. 31 passed.
| @@ -152,20 +184,28 @@ def check(self, server_name: str, auth_method: str, url: str = "") -> AuthCheckR | |||
| # all bypassed the require_tls gate because none of | |||
| # them start with "http://" — only `https://` and | |||
There was a problem hiding this comment.
Correct, and thanks — that paragraph is wrong and I should have caught it when I
added mine underneath it.
To be precise about what happened: the prefix wording is pre-existing, not new in this PR
(it's at the merge base, 24d5725). But it was accurate about nothing even then — the base
code is if entry.require_tls and url: followed by urlparse(url).scheme, exactly as you
say. So a non-HTTP scheme never bypassed anything; the only bypass was the falsy url,
which is what this PR fixes.
Since my paragraph sits directly below it, leaving it would read as though this PR were
also changing prefix matching to scheme parsing. Narrowed to what the code does in
3e349183:
# TLS check: the scheme is parsed and matched against the
# allowlist below, so only `https://` and `wss://` count as
# TLS-secured transports.The falsy-url explanation below it stays, since that part is what changes here.
|
The Spell Check failure here is not about anything this branch wrote — it is the CI bug in #3530, and I can now point at this PR as a live instance of it.
With the base resolved to its merge base with So this branch's added lines are clean; the check is reading 40 commits of I would rather not rebase to go green, since that hides the instance. Happy to rebase if a maintainer prefers a green tick here over the evidence. |
…x note Two review points from microsoft#3512, both correct. The new tests used `assert ... is False`, while the other 9 assertions in this file use `assert not ...`. `== False` is not an option here (ruff E712 is enabled via `select = ["E"]`), so the fix is truthiness, which also matches the file's existing convention. The "rather than testing for the literal 'http://' prefix" paragraph was inherited from the pre-existing comment and describes a prefix check that was never in the code -- the gate was skipped because `url` was falsy, and a URL that *was* supplied already went through scheme parsing. Narrowed it to what the code actually does; the part explaining the falsy-`url` skip, which is what this PR changes, stays.
| if not scheme: | ||
| return f"URL {url!r} has no scheme, so TLS cannot be verified" |
There was a problem hiding this comment.
Fixed in 97d400d8, and this one is mine rather than pre-existing — worth being explicit
about that, since the previous two were inherited.
The base version only ever emitted the scheme (URL scheme {scheme!r} is not in the TLS allowlist). I added the no-scheme branch in this PR and echoed the URL verbatim in it, so
http://user:s3cret@mcp.internal or ?access_token=s3cret would have reached
agt.audit.reason. Confirmed the propagation path you cite:
# otel_audit_backend.py
ATTR_REASON = "agt.audit.reason"
...
if entry.reason:
attributes[ATTR_REASON] = entry.reasonNote the schemed branch was never affected — it only ever interpolated scheme. It was
specifically the schemeless case, which is the one where the "URL" is most likely to be a
raw config string.
Kept the message actionable without the URL by naming the allowlist instead:
if not scheme:
return f"the URL has no scheme, so TLS cannot be verified (need {allowlist})"server_name is already on AuthCheckResult and is in the outer reason, so context isn't
lost. Added a parametrized regression test over userinfo, query-param, and schemeless forms
asserting both the secret and the raw URL are absent from reason — it fails on the previous
commit and passes now. 34 passed.
Third review point from microsoft#3512, and this one is mine: the no-scheme branch echoed the URL verbatim. `AuthCheckResult.reason` is promoted into audit attributes -- `otel_audit_backend` maps it to `agt.audit.reason` -- and a server URL can carry a token in its userinfo or query string, so `http://user:s3cret@host` or `?access_token=s3cret` would land in logs. The base version only ever emitted the scheme, so this was a regression I introduced, not pre-existing. The scheme is all the message needs, and it is not secret. Added the allowlist to the no-scheme text so the reason stays actionable without the URL, and a parametrized test asserting the secret and the raw URL are both absent from `reason`.
|
The gate fix is right, since require_tls was skipped whenever the caller passed no url, which is every from_yaml call site. Servers that are not in the allowlist fall through to the default policy path, which applies no TLS check at all, so an unregistered http:// server still passes on auth method alone. min_tls_version is parsed from YAML and stored on McpServerEntry but never read anywhere in the repo, so require_tls is all or nothing. |
|
Thanks for confirming. I agree the configured server URL needs to go through the same TLS gate, and the current branch keeps that enforcement while avoiding exposing the URL in the denial reason. |
|
Closing per maintainer decision; this repository is not accepting submissions from this account. |
Fixes #3511
What was wrong
McpAuthPolicy.checkenforcedrequire_tlsonly against the URL the caller passed. That parameter defaults to""and the gate was skipped entirely when it was falsy, soentry.url— written by both constructors and byfrom_yaml, documented onMcpServerEntryand indocs/specs/MCP-SECURITY-GATEWAY-1.0.md§10.3 — was never read by any code path.Both calls describe the same connection to the same server under the same policy, and they disagree. Which one fires depended on whether the caller redundantly restated configuration the policy object already held.
The
from_yamlpath always took the permissive branch, since that is exactly the deployment shape where the URL lives in config and the call site has no reason to pass it again:from_yamlparsesurlinto the entry and nothing reads it back. A configuration that saysrequire_tls: trueand points athttp://is the misconfiguration this check exists to catch, and the check stayed silent.What changed
effective_url = url or entry.url. The caller-supplied URL keeps priority, since it describes the connection actually being dialed and may legitimately differ from the registered pattern — a plaintext dial against anhttps-registered server is still blocked.With no URL from either source, the gate stays skipped, exactly as before. Denying every server registered without a URL would be a separate policy decision about existing configurations, not part of fixing this one, so it is deliberately out of scope.
Scheme inspection moves into
_tls_violation, andTLS_SCHEMESbecomes a module constant. A non-empty URL with no scheme — the shape a bare host or the documented glob pattern takes (mcp.internal,*.internal/finance) — was already denied by"" not in {"https","wss"}, but only as a by-product of set membership, reported asscheme '' is not in the TLS allowlist. It is now denied deliberately, withURL 'mcp.internal' has no scheme, so TLS cannot be verified, and covered by a test. This matters more than it did before: the change makes that path reachable from configured URLs for the first time.Compatibility
Strictly more denials, and only in the case the guard was written to reject: a server whose configured URL is not TLS while
require_tlsis True. Every combination that was allowed for a legitimate reason still is —require_tls=False, anhttps/wssconfigured URL, no URL anywhere, and unknown servers on the default path (which has no TLS gate at all, unchanged here).min_tls_versionis stored, documented, and specified (§10.6 step 5) but read by no code path. That is unimplemented functionality rather than a guard that misreports, so it is noted in #3511 and left for its own issue rather than folded in here.Verification
tests/test_mcp_auth_enforcement.py: 31 passed. With the source file stashed: 7 failed / 24 passed — every new assertion fails onmain.tests/suite (-p no:randomly --continue-on-collection-errors): 4478 passed / 257 failed against base 4464 / 257 — exactly +14, the new test count, no new failures.tests/test_spec_mcp_gateway_conformance.pycannot be collected onmain(pre-existingModuleNotFoundError: agent_sre, one of three such modules), so its S10.1–S10.11 assertions were replayed by hand against the patched module — all still hold, including S10.7 (http://caller URL denied) and S10.11 (add_serverwith no URL still allowed).ruff check: 4 findings, identical to base (UP037in the source,I001+ twoF401in the test file — all pre-existing).ruff format --diff: the remaining reformat suggestions are all on lines this PR does not touch, matching base exactly.cspellon all 133 added lines: clean.Tests added
TestTlsGateUsesTheConfiguredUrl— 8 methods / 14 cases:http,ws,ftp,gopher, and two scheme-less formshttpsandwssconfigured URLs allowed; scheme case ignoredrequire_tls=Falsestill allows plaintextfrom_yamlpath, which is where this fails in practiceChecklist
ruff check/ruff formatstate unchanged relative to base