microsoft / microsoft/agent-governance-toolkit

McpAuthPolicy require_tls never checks the server's configured URL, so a plaintext MCP server passes

Open
#3,511 2 comments 0 reactions 0 assignees View on GitHub
needs-review:HIGH
Dominant language
Python
Stars
6.3k
Forks
1.1k
Avg merge
5d 11h
Merged PRs (30d)
142

Description

### Summary

`McpAuthPolicy.check` enforces `require_tls` only against the URL the **caller** passes. That parameter defaults to `""`, and the gate is skipped entirely when it is falsy — so the URL configured on the `McpServerEntry` is never inspected. A server registered with a plaintext URL and `require_tls=True` is allowed by any caller that does not repeat the URL, which includes every caller on the `from_yaml` path, where the URL lives in config and the call site has no reason to pass it again.

### Reproduction

```python
from agent_os.mcp_auth_enforcement import McpAuthPolicy, McpServerEntry

policy = McpAuthPolicy(servers=[
McpServerEntry(
name="finance-tools",
url="http://mcp.internal/finance", # plaintext, as configured
allowed_auth_methods=["mtls"],
require_tls=True, # ...and TLS is required
),
])

policy.check("finance-tools", auth_method="mtls").allowed
# -> True
# reason: "Auth method 'mtls' is in server 'finance-tools' allowlist"

# The exact same policy, with the caller repeating the URL it already configured:
policy.check("finance-tools", auth_method="mtls", url="http://mcp.internal/finance").allowed
# -> False
# reason: "Server 'finance-tools' requires TLS but URL scheme 'http' is not in
# the TLS allowlist (https, wss)"
```

The two calls describe the same connection to the same server under the same policy and disagree. Whether the guard fires depends on whether the caller redundantly restates configuration the policy object already holds.

Same defect through the documented YAML entry point, where restating the URL at the call site is not a natural thing to do:

```python
policy = McpAuthPolicy.from_yaml("""
mcp_auth_policy:
servers:
- name: finance-tools
url: http://mcp.internal/finance
allowed_auth_methods: [mtls]
require_tls: true
""")

policy.check("finance-tools", auth_method="mtls").allowed
# -> True
```

`from_yaml` parses `url` into the entry (`mcp_auth_enforcement.py:230`) and no code path ever reads it back.

### Root cause

`mcp_auth_enforcement.py:155`:

```python
entry = self._servers.get(server_name)
if entry:
if auth_method in entry.allowed_auth_methods:
if entry.require_tls and url: # <-- `url` is the caller's, default ""
...
```

`entry.url` is written by both constructors and by `from_yaml`, is documented in `McpServerEntry` ("Server URL pattern") and in the spec (`docs/specs/MCP-SECURITY-GATEWAY-1.0.md` §10.3), and is read by nothing. `grep -rn "entry.url\|\.url" src/` confirms there is no consumer.

The `and url` guard is what makes this fail **open** rather than closed: the absence of a caller-supplied URL is treated as "no TLS requirement to check" rather than "fall back to what was configured".

### Impact

A configuration that says `require_tls: true` and points at `http://` is exactly the misconfiguration this check exists to catch, and it passes. The guard's own YAML path is the one most likely to hit it, since a deployment that centralises server URLs in config is precisely the deployment whose call sites pass only `(server_name, auth_method)`.

This is a policy check reporting `allowed=True` for the configuration it was written to reject. It needs no attacker — a single wrong scheme in a config file is enough, and the check that should surface it stays silent.

### Proposed fix

Fall back to `entry.url` when the caller does not supply one:

```python
effective_url = url or entry.url
if entry.require_tls and effective_url:
...
```

The caller-supplied URL keeps priority, since it describes the connection actually being dialed and may legitimately differ from the registered pattern.

Two related points, handled deliberately:

- **A non-empty URL with no scheme** (`mcp.internal`, `*.internal/finance` — the "glob pattern" shape the field documents) is already denied, since `urlparse` yields `""` and `"" not in {"https", "wss"}`. That is the right outcome — nothing in a scheme-less URL says the transport is encrypted — but it currently falls out of a set-membership check by accident and produces the confusing reason `URL scheme '' is not in the TLS allowlist`. Worth pinning with a test and a reason that says what actually happened, especially now that this path is reachable from configured URLs.
- **No URL from either source** should stay skipped. Denying every server registered without a URL is a separate policy decision, not part of fixing this one.

Also noting for the record, not fixing here: `min_tls_version` is stored, documented, and specified (§10.6 step 5, "verify the minimum TLS version requirement is met") but read by no code path. That is unimplemented functionality rather than a guard that misreports, so it belongs in its own issue.

PR to follow.

Contributor guide

Open the contributing guide

Research direction

Read mcp_auth_enforcement.py around McpAuthPolicy.check at line 155 and the from_yaml handling around line 230, then reproduce the YAML example from the issue. Confirm that configured server URLs participate in the TLS decision, caller-supplied URLs retain their stated priority, and the relevant regression cases pass; the specification is in docs/specs/MCP-SECURITY-GATEWAY-1.0.md §10.3.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
authentication, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.