fix(agent-os): apply TLS floor to unregistered MCP servers in auth fallback

[yanran xie (xieyanran)]

Fixes #3814.

What changed

McpAuthPolicy.check() in agent_os/mcp_auth_enforcement.py now applies the same TLS-scheme gate to the "fall back to default policy" branch (unregistered server names) that already exists for the per-server allowlist branch (registered entries).

Added a default_require_tls: bool = True constructor option (and matching from_yaml key) so this floor can be explicitly disabled if a deployment relies on the old behavior for unregistered servers.

Why

As filed in #3814: when server_name has no entry in the allowlist, check() fell straight through to the default-allowlist path with no TLS check at all — require_tls semantics only existed for registered entries. A typo'd or newly-added-but-not-yet-registered MCP server name silently got weaker transport guarantees than a registered one, even when a plain-http:// URL was supplied.

The fix mirrors the registered-entry gate's own constraint: the check only runs when a URL is actually supplied (if self._default_require_tls and url:), so the S10.12 spec behavior — an omitted/empty URL stays allowed — is preserved for both paths.

How I tested

• Added 3 regression tests: unregistered server + http:// URL is now rejected, unregistered server + https:// URL is allowed, and the floor can be opted out via default_require_tls=False.
• Ran the full test_mcp_auth_enforcement.py suite: 20/20 passed (17 pre-existing + 3 new), confirming no existing behavior regressed — in particular the tests that call check() with no url argument at all still pass, since the TLS check is skipped when url is falsy.
• Checked other call sites of McpAuthPolicy(...) in the repo (test_spec_mcp_gateway_conformance.py) — all use keyword arguments, so the new constructor parameter doesn't shift anything positionally.
• Ran ruff check on both changed files; the only findings are pre-existing issues unrelated to this change (an from_yaml type-annotation style nit and an unsorted import block already present before this PR).

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

Welcome to the Agent Governance Toolkit! Thanks for your first pull request.
Please ensure tests pass, code follows style (ruff check), and you have signed the CLA.
See our Contributing Guide.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[yanran xie (xieyanran)]

@microsoft-github-policy-service agree

[Carlos Hernandez (carloshvp)]

Reviewed at 500e131. The change applies the existing HTTPS/WSS scheme gate to the unregistered-server fallback, keeps the established empty-URL behavior, and exposes an explicit constructor and YAML opt-out. I verified the 20-test file on Python 3.11 and 3.12, ran an independent scheme and YAML matrix, confirmed the source lint gate and diff check, and merge-simulated against current main at 359a233 without conflicts. The commit is signed off.

Non-blocking follow-up: please add default_require_tls to the normative MCP security spec policy-defaults table and YAML example so operators can discover the compatibility control.

[Imran Siddique (imran-siddique)]

Ran this branch's suite locally against its own tree (PYTHONPATH=src python -m pytest tests/test_mcp_auth_enforcement.py): 20 passed. The fix is correct and #3814 is a real hole.

Confirmed on main: the TLS gate lives entirely inside if entry:, so it only ever runs for a
server that is present in the allowlist. Any name that misses the dictionary falls through to the
default-policy branch, which checks the auth method and returns allowed=True with no transport
check at all. A typo in a server name, or a server that simply has not been registered yet, gets a
weaker guarantee than a registered one. That is the wrong way round for a fail-closed design, and
the argument in your docstring is the right framing of why.

Three specifics I checked and agree with.

Defaulting default_require_tls to True is the correct default even though it is a behaviour
change for anyone currently relying on the gap. The gap is the bug, so preserving it by default would
ship the bug as a feature.

Keeping the escape hatch, and wiring it through from_yaml, is the right amount of flexibility. An
operator with a genuine plaintext legacy server can set default_require_tls: false explicitly and
that choice is then visible in the policy file rather than implicit in a code path nobody noticed.

Matching the registered-entry gate on the empty-URL case (and url) rather than being stricter is
also right. Consistency between the two branches is what makes this reviewable, and going stricter
here than the registered path would have been a second, unrelated behaviour change hidden in the
same PR.

Context for sequencing rather than a change request: #3785 is the sibling issue, and #3793 and #3797
both fix it in the registered-entry branch higher up in the same method. This PR touches the fallback
branch and __init__, so it does not conflict textually with either of them, and all three are
needed before the TLS floor is actually a floor.

One thing outside the scope of all three: min_tls_version is accepted on McpServerEntry, parsed
by from_yaml, and documented in docs/specs/MCP-SECURITY-GATEWAY-1.0.md and
agent-mesh/docs/zero-trust.md, but no code in the repo ever reads it. A policy setting
min_tls_version: "1.3" gets nothing enforced. That deserves its own issue rather than a fourth PR
in this cluster.

Nothing blocking.

[Imran Siddique (imran-siddique)]

MohammadHaroonAbuomar liamcrumm this one plus a pick between two others closes the MCP TLS floor,
and none of it is closed today.

• This PR (fix(agent-os): apply TLS floor to unregistered MCP servers in auth fallback #3849) fixes MCP auth: unregistered server names bypass the TLS gate entirely #3814: the TLS gate lives entirely inside if entry:, so any server name
  not in the allowlist skips the transport check completely and gets a weaker guarantee than a
  registered one. 20 tests pass locally. It does not conflict with anything.
• fix(agent-os): enforce TLS for configured MCP URLs #3793 and fix: mcp auth TLS gate falls back to entry.url when caller omits url #3797 both fix the sibling issue MCP auth: require_tls gate never consults the configured entry.url #3785 with the identical mechanism and will conflict
  with each other. My read is fix: mcp auth TLS gate falls back to entry.url when caller omits url #3797 on coverage, carrying fix(agent-os): enforce TLS for configured MCP URLs #3793's parametrized precedence matrix
  across; detail on both threads.

All three are needed before the floor is actually a floor. Separately, min_tls_version is accepted,
parsed and documented but never read by any code in the repo, which deserves its own issue.

[yanran xie (xieyanran)]

Carlos Hernandez (@carloshvp) liamcrumm Thanks for the review — addressed in d257948: added default_require_tls to the §10.5 Policy Defaults table, updated §10.6 Check Logic step 6 to describe the TLS floor on the default-policy path, added it to the §10.7 YAML example, and added two new tests going through from_yaml() (test_yaml_default_require_tls_defaults_true, test_yaml_default_require_tls_false_disables_the_floor) to cover the YAML path liamcrumm flagged, not just the constructor path.

[Carlos Hernandez (carloshvp)]

Re-reviewed exact head d2579480e90e44df27e4b7fd749aedb91cfdc630, including the follow-up to the previously approved implementation.

The requested documentation and configuration-path coverage are now present. The documented default, YAML example, and enforcement flow all reflect fail-closed TLS enforcement for unregistered servers, while preserving the explicit default_require_tls: false compatibility escape hatch.

Independent validation:

• Focused MCP auth enforcement suite: 22 passed.
• Adversarial scheme and YAML matrix: HTTPS/WSS accepted; HTTP, WS, FTP, and schemeless URLs rejected by default; the explicit opt-out behaves as documented.
• Docs link check: 2,561 links checked, no new broken links.
• Docs frontmatter check: 279 files checked, no findings.
• No new Ruff findings relative to the previously reviewed head.
• Clean diff check, conflict-free merge simulation against current main, and DCO signoffs present.

The full local development install was not reproducible because the package metadata currently resolves incompatible agent-governance-toolkit-core requirements. Hosted checks are green, and the focused validation above exercises the changed behavior directly.

Approving.

[MohammadHaroonAbuomar]

Re-triggering CI against current main (the earlier run predates the typing-extensions pin fix in #3928). Reopening now.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[MohammadHaroonAbuomar]

Approved after group integration review (tests, gates and adversarial pass on the combined change).

[MohammadHaroonAbuomar]

Approved after group integration review (tests, gates and adversarial pass); Liam's documentation ask is addressed in d257948.