feat(auth): drop test-client defaults, add ALLOWED_MGMT_CLIENT allowlist
Both auth surfaces now fail-closed by default: - ALLOWED_MCP_CLIENTS: removed the silent `claude-desktop` and `test-mcp-client` fallbacks. Empty/unset env var leaves the registry empty so /oauth/authorize rejects every client_id. - ALLOWED_MGMT_CLIENT (new): comma-separated list of OIDC client_ids whose tokens are accepted by /api/management/*. Enforced in verify_token_for_management_api on both the cache-hit and cache-miss paths against the token's client_id claim. Unset/empty rejects all. Compose: set ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient on mcp-multi-user-basic so the existing Astrolabe integration test (test_astrolabe_chunk_context.py) still passes. env.sample documents both vars and notes they may be consolidated later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
2bd1c18b43
commit
bd7702ad12
@@ -168,6 +168,11 @@ services:
|
||||
# - NEXTCLOUD_OIDC_CLIENT_ID=your_client_id
|
||||
# - NEXTCLOUD_OIDC_CLIENT_SECRET=your_client_secret
|
||||
|
||||
# Management API allowlist (ADR-018): only Astrolabe-issued tokens may
|
||||
# call /api/management/*. Default test client_id used by the
|
||||
# configure_astrolabe_for_mcp_server fixture.
|
||||
- ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient
|
||||
|
||||
# NO admin credentials - credentials come from client Authorization header
|
||||
volumes:
|
||||
- multi-user-basic-data:/app/data
|
||||
|
||||
+25
@@ -119,6 +119,31 @@ NEXTCLOUD_PASSWORD=
|
||||
# Optional features (semantic search, document processing):
|
||||
# See "Optional Features" section below
|
||||
|
||||
# ============================================
|
||||
# OAUTH CLIENT ALLOWLISTS (OAuth modes)
|
||||
# ============================================
|
||||
# Both env vars are FAIL-CLOSED: if unset/empty, the corresponding auth route
|
||||
# rejects every request. There are no built-in defaults.
|
||||
#
|
||||
# Note: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate to
|
||||
# keep the MCP-route and management-API auth surfaces independent. They may be
|
||||
# consolidated into a single env var later.
|
||||
|
||||
# ===== ALLOWED_MCP_CLIENTS =====
|
||||
# Clients allowed to use the OAuth AS proxy (/oauth/authorize, /oauth/token).
|
||||
# Comma-separated. Each entry is either:
|
||||
# - "client_id" → loopback redirect URIs (http://localhost:*, http://127.0.0.1:*)
|
||||
# - "client_id|https://app/cb" → bind to a specific HTTPS callback
|
||||
# HTTP redirect URIs are rejected unless the host is loopback (localhost/127.0.0.1/::1).
|
||||
#ALLOWED_MCP_CLIENTS=claude-desktop, zed-editor, cloud-app|https://cloud.example.com/cb
|
||||
|
||||
# ===== ALLOWED_MGMT_CLIENT =====
|
||||
# OIDC client_ids whose tokens are accepted by the management API
|
||||
# (/api/management/*). Comma-separated. The token's `client_id` claim must
|
||||
# match one of these entries. Typical value: the Astrolabe NC PHP app's
|
||||
# OAuth client_id (ADR-018).
|
||||
#ALLOWED_MGMT_CLIENT=astrolabe
|
||||
|
||||
# ============================================
|
||||
# OPTIONAL FEATURES (All Deployment Modes)
|
||||
# ============================================
|
||||
|
||||
@@ -66,7 +66,15 @@ class ClientRegistry:
|
||||
- http://localhost:* and http://127.0.0.1:* are allowed (native clients)
|
||||
- https:// redirect URIs are allowed (cloud clients)
|
||||
- http:// non-localhost redirect URIs are rejected with a warning
|
||||
|
||||
If the env var is unset or empty, the registry remains empty and
|
||||
``validate_client`` rejects every client_id (fail-closed). There are no
|
||||
built-in defaults; operators must opt-in clients explicitly.
|
||||
"""
|
||||
# NOTE: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate
|
||||
# env vars to keep the MCP-route and management-API auth surfaces
|
||||
# independent. These may be consolidated into a single env var later
|
||||
# once the deployment story stabilises.
|
||||
allowed_clients = os.getenv("ALLOWED_MCP_CLIENTS", "").strip()
|
||||
|
||||
if allowed_clients:
|
||||
@@ -123,9 +131,11 @@ class ClientRegistry:
|
||||
)
|
||||
logger.info(f"Registered static client: {entry}")
|
||||
|
||||
# Add well-known clients if not explicitly configured
|
||||
if not self._clients:
|
||||
self._add_well_known_clients()
|
||||
logger.warning(
|
||||
"Client registry is empty: ALLOWED_MCP_CLIENTS is unset or empty. "
|
||||
"All MCP-flow OAuth requests will be rejected until configured."
|
||||
)
|
||||
|
||||
def _get_client_name(self, client_id: str) -> str:
|
||||
"""Get human-readable name for client_id."""
|
||||
@@ -135,35 +145,9 @@ class ClientRegistry:
|
||||
"continue-dev": "Continue IDE Extension",
|
||||
"zed-editor": "Zed Editor",
|
||||
"vscode-mcp": "VS Code MCP Extension",
|
||||
"test-mcp-client": "Test MCP Client",
|
||||
}
|
||||
return known_names.get(client_id, client_id.replace("-", " ").title())
|
||||
|
||||
def _add_well_known_clients(self):
|
||||
"""Add well-known MCP clients for testing and development."""
|
||||
well_known = [
|
||||
MCPClientInfo(
|
||||
client_id="claude-desktop",
|
||||
name="Claude Desktop",
|
||||
redirect_uris=["http://localhost:*", "http://127.0.0.1:*"],
|
||||
allowed_scopes=["*"],
|
||||
is_public=True,
|
||||
metadata={"vendor": "Anthropic"},
|
||||
),
|
||||
MCPClientInfo(
|
||||
client_id="test-mcp-client",
|
||||
name="Test MCP Client",
|
||||
redirect_uris=["http://localhost:*", "http://127.0.0.1:*"],
|
||||
allowed_scopes=["*"],
|
||||
is_public=True,
|
||||
metadata={"purpose": "testing"},
|
||||
),
|
||||
]
|
||||
|
||||
for client in well_known:
|
||||
self._clients[client.client_id] = client
|
||||
logger.info(f"Registered well-known client: {client.client_id}")
|
||||
|
||||
def validate_client(
|
||||
self,
|
||||
client_id: str,
|
||||
|
||||
@@ -17,6 +17,7 @@ Key Design Principles:
|
||||
|
||||
import hashlib
|
||||
import logging
|
||||
import os
|
||||
import time
|
||||
from typing import Any
|
||||
|
||||
@@ -97,6 +98,25 @@ class UnifiedTokenVerifier(TokenVerifier):
|
||||
self._token_cache: dict[str, tuple[dict[str, Any], float]] = {}
|
||||
self.cache_ttl = 3600 # 1 hour default
|
||||
|
||||
# NOTE: ALLOWED_MCP_CLIENTS and ALLOWED_MGMT_CLIENT are currently separate
|
||||
# env vars to keep the MCP-route and management-API auth surfaces
|
||||
# independent. These may be consolidated into a single env var later
|
||||
# once the deployment story stabilises.
|
||||
self._allowed_mgmt_clients: frozenset[str] = frozenset(
|
||||
entry.strip()
|
||||
for entry in os.getenv("ALLOWED_MGMT_CLIENT", "").split(",")
|
||||
if entry.strip()
|
||||
)
|
||||
if not self._allowed_mgmt_clients:
|
||||
logger.warning(
|
||||
"ALLOWED_MGMT_CLIENT is unset or empty: management API will reject "
|
||||
"all requests until configured."
|
||||
)
|
||||
else:
|
||||
logger.info(
|
||||
f"Management API allowlist: {sorted(self._allowed_mgmt_clients)}"
|
||||
)
|
||||
|
||||
logger.info(
|
||||
f"UnifiedTokenVerifier initialized in {self.mode} mode. "
|
||||
f"MCP audience: {settings.oidc_client_id} or {settings.nextcloud_mcp_server_url}, "
|
||||
@@ -169,10 +189,11 @@ class UnifiedTokenVerifier(TokenVerifier):
|
||||
token: Bearer token to verify
|
||||
|
||||
Returns:
|
||||
AccessToken if valid (regardless of audience), None otherwise
|
||||
AccessToken if valid AND issued by an allowlisted client, None otherwise
|
||||
"""
|
||||
# Check cache first (using separate cache key to avoid mixing with MCP tokens)
|
||||
cache_key = f"mgmt:{hashlib.sha256(token.encode()).hexdigest()}"
|
||||
access_token: AccessToken | None = None
|
||||
if cache_key in self._token_cache:
|
||||
userinfo, expiry = self._token_cache[cache_key]
|
||||
if time.time() < expiry:
|
||||
@@ -181,7 +202,7 @@ class UnifiedTokenVerifier(TokenVerifier):
|
||||
username = userinfo.get("sub") or userinfo.get("preferred_username")
|
||||
scope_string = userinfo.get("scope", "")
|
||||
scopes = scope_string.split() if scope_string else []
|
||||
return AccessToken(
|
||||
access_token = AccessToken(
|
||||
token=token,
|
||||
client_id=userinfo.get("client_id", ""),
|
||||
scopes=scopes,
|
||||
@@ -191,10 +212,23 @@ class UnifiedTokenVerifier(TokenVerifier):
|
||||
else:
|
||||
del self._token_cache[cache_key]
|
||||
|
||||
oauth_token_cache_hits_total.labels(hit="false").inc()
|
||||
if access_token is None:
|
||||
oauth_token_cache_hits_total.labels(hit="false").inc()
|
||||
access_token = await self._verify_without_audience_check(token, cache_key)
|
||||
|
||||
# Verify token without audience check
|
||||
return await self._verify_without_audience_check(token, cache_key)
|
||||
if access_token is None:
|
||||
return None
|
||||
|
||||
# Enforce ALLOWED_MGMT_CLIENT allowlist (fail-closed when unset)
|
||||
token_client_id = access_token.client_id
|
||||
if not token_client_id or token_client_id not in self._allowed_mgmt_clients:
|
||||
logger.warning(
|
||||
"Management API token rejected: client_id %r not in ALLOWED_MGMT_CLIENT",
|
||||
token_client_id,
|
||||
)
|
||||
return None
|
||||
|
||||
return access_token
|
||||
|
||||
async def _verify_mcp_audience(self, token: str) -> AccessToken | None:
|
||||
"""
|
||||
|
||||
@@ -88,20 +88,23 @@ def test_http_non_localhost_rejected(monkeypatch, caplog):
|
||||
assert "evil.com" in caplog.text
|
||||
|
||||
|
||||
def test_empty_string_uses_well_known(monkeypatch):
|
||||
registry = _get_registry(monkeypatch, "")
|
||||
clients = registry.list_clients()
|
||||
client_ids = {c.client_id for c in clients}
|
||||
assert "claude-desktop" in client_ids
|
||||
assert "test-mcp-client" in client_ids
|
||||
def test_empty_string_yields_empty_registry(monkeypatch, caplog):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
registry = _get_registry(monkeypatch, "")
|
||||
assert registry.list_clients() == []
|
||||
valid, err = registry.validate_client("claude-desktop")
|
||||
assert valid is False
|
||||
assert "Unknown client_id" in err
|
||||
assert "ALLOWED_MCP_CLIENTS is unset or empty" in caplog.text
|
||||
|
||||
|
||||
def test_unset_env_uses_well_known(monkeypatch):
|
||||
registry = _get_registry(monkeypatch, None)
|
||||
clients = registry.list_clients()
|
||||
client_ids = {c.client_id for c in clients}
|
||||
assert "claude-desktop" in client_ids
|
||||
assert "test-mcp-client" in client_ids
|
||||
def test_unset_env_yields_empty_registry(monkeypatch, caplog):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
registry = _get_registry(monkeypatch, None)
|
||||
assert registry.list_clients() == []
|
||||
valid, err = registry.validate_client("test-mcp-client")
|
||||
assert valid is False
|
||||
assert "Unknown client_id" in err
|
||||
|
||||
|
||||
def test_malformed_entries_skipped_with_warning(monkeypatch, caplog):
|
||||
@@ -153,14 +156,6 @@ def test_validate_redirect_uri_localhost_wildcard(monkeypatch):
|
||||
assert err is None
|
||||
|
||||
|
||||
def test_well_known_clients_wildcard_scopes(monkeypatch):
|
||||
registry = _get_registry(monkeypatch, None)
|
||||
for client in registry.list_clients():
|
||||
assert client.allowed_scopes == ["*"], (
|
||||
f"Well-known client {client.client_id} should have wildcard scopes"
|
||||
)
|
||||
|
||||
|
||||
def test_client_name_resolution(monkeypatch):
|
||||
registry = _get_registry(monkeypatch, "claude-desktop, custom-tool")
|
||||
assert registry.get_client("claude-desktop").name == "Claude Desktop"
|
||||
|
||||
@@ -512,3 +512,114 @@ class TestVerifyTokenFlow:
|
||||
result = await verifier.verify_token("opaque-token")
|
||||
assert result is not None
|
||||
assert result.resource == "testuser"
|
||||
|
||||
|
||||
class TestManagementApiAllowlist:
|
||||
"""Test ALLOWED_MGMT_CLIENT enforcement in verify_token_for_management_api."""
|
||||
|
||||
@staticmethod
|
||||
def _underlying_token(client_id: str = "astrolabe"):
|
||||
from mcp.server.auth.provider import AccessToken
|
||||
|
||||
return AccessToken(
|
||||
token="t",
|
||||
client_id=client_id,
|
||||
scopes=["openid"],
|
||||
expires_at=int(time.time() + 3600),
|
||||
resource="testuser",
|
||||
)
|
||||
|
||||
async def test_unset_allowlist_rejects_all(self, monkeypatch, base_settings):
|
||||
monkeypatch.delenv("ALLOWED_MGMT_CLIENT", raising=False)
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
assert verifier._allowed_mgmt_clients == frozenset()
|
||||
|
||||
with patch.object(
|
||||
verifier,
|
||||
"_verify_without_audience_check",
|
||||
return_value=self._underlying_token("astrolabe"),
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is None
|
||||
|
||||
async def test_empty_allowlist_rejects_all(self, monkeypatch, base_settings):
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", " , ,")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
assert verifier._allowed_mgmt_clients == frozenset()
|
||||
|
||||
with patch.object(
|
||||
verifier,
|
||||
"_verify_without_audience_check",
|
||||
return_value=self._underlying_token("astrolabe"),
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is None
|
||||
|
||||
async def test_allowlisted_client_accepted(self, monkeypatch, base_settings):
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe, admin-tool")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
assert verifier._allowed_mgmt_clients == {"astrolabe", "admin-tool"}
|
||||
|
||||
underlying = self._underlying_token("astrolabe")
|
||||
with patch.object(
|
||||
verifier, "_verify_without_audience_check", return_value=underlying
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is underlying
|
||||
|
||||
async def test_non_allowlisted_client_rejected(self, monkeypatch, base_settings):
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
|
||||
with patch.object(
|
||||
verifier,
|
||||
"_verify_without_audience_check",
|
||||
return_value=self._underlying_token("some-other-client"),
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is None
|
||||
|
||||
async def test_token_missing_client_id_rejected(self, monkeypatch, base_settings):
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
|
||||
with patch.object(
|
||||
verifier,
|
||||
"_verify_without_audience_check",
|
||||
return_value=self._underlying_token(""),
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is None
|
||||
|
||||
async def test_underlying_verification_failure_propagates(
|
||||
self, monkeypatch, base_settings
|
||||
):
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
|
||||
with patch.object(
|
||||
verifier, "_verify_without_audience_check", return_value=None
|
||||
):
|
||||
result = await verifier.verify_token_for_management_api("any-token")
|
||||
assert result is None
|
||||
|
||||
async def test_cache_hit_also_enforces_allowlist(self, monkeypatch, base_settings):
|
||||
"""A previously-cached token must still be re-checked against the allowlist."""
|
||||
import hashlib
|
||||
|
||||
monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "astrolabe")
|
||||
verifier = UnifiedTokenVerifier(base_settings)
|
||||
|
||||
token = "cached-token"
|
||||
cache_key = f"mgmt:{hashlib.sha256(token.encode()).hexdigest()}"
|
||||
verifier._token_cache[cache_key] = (
|
||||
{
|
||||
"sub": "testuser",
|
||||
"scope": "openid",
|
||||
"client_id": "not-allowlisted",
|
||||
},
|
||||
time.time() + 3600,
|
||||
)
|
||||
|
||||
result = await verifier.verify_token_for_management_api(token)
|
||||
assert result is None
|
||||
|
||||
Reference in New Issue
Block a user