mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-01 12:02:05 +00:00
feat(dashboard_auth): support confidential clients (client_secret) in self-hosted OIDC (#55344)
The self-hosted OIDC dashboard provider was public-client + PKCE only, with two `# TODO(confidential-client)` seams. Authentik and Keycloak commonly default a new OIDC client to *confidential*, whose token endpoint rejects an unauthenticated exchange (`invalid_client`) — so a self-hoster who accepts their IDP's default could not complete dashboard login without manually flipping the client to public. Add optional confidential-client support: - New optional `client_secret` (env `HERMES_DASHBOARD_OIDC_CLIENT_SECRET`, or `dashboard.oauth.self_hosted.client_secret`; env-wins-config, empty treated as unset). It is a credential, so docs steer operators to the `.env` file; config.yaml is supported only for precedence symmetry. - `_token_endpoint_auth()` selects `client_secret_basic` (HTTP Basic header) vs `client_secret_post` (form body) from the IDP's advertised `token_endpoint_auth_methods_supported`, defaulting to basic (the OIDC default) when absent. Applied to complete_login, refresh_session, and revoke_session (RFC 7009 §2.1). - PKCE is sent in BOTH modes — the secret is client authentication layered on top, never a replacement (OAuth 2.1 / RFC 9700 keep PKCE mandatory). - Basic header url-encodes client_id/secret before base64 per RFC 6749 §2.3.1, so reserved chars (`:`, `@`, space) round-trip correctly. Non-breaking: with no secret configured the provider is a pure public PKCE client, byte-identical to prior behaviour (no Authorization header, no client_secret in the body). The secret is never logged — register() reports only a `confidential=<bool>` flag. Tests: 16 new cases covering basic/post selection, default-when-absent, public-unchanged contract, PKCE-preserved, reserved-char url-encoding, blank-secret-is-public, refresh + revoke auth, no-secret-in-logs, and env/config register wiring. Full dashboard-auth suite (nous provider, middleware, gate, cookies, WS, 401-reauth, status endpoint) — 396 tests — green, proving no existing auth path regressed.
This commit is contained in:
Ben Barclay
GitHub
parent
481caa66f2
commit
53a75f147f
3 changed files with 446 additions and 22 deletions
|
|
@ -127,14 +127,34 @@ def _mint_id_token(
|
|||
)
|
||||
|
||||
|
||||
def _make_provider(rsa_keypair, *, scopes: str | None = None):
|
||||
"""Construct a provider with discovery + JWKS stubbed (no network)."""
|
||||
def _make_provider(
|
||||
rsa_keypair,
|
||||
*,
|
||||
scopes: str | None = None,
|
||||
client_secret: str | None = None,
|
||||
auth_methods: Any = "__unset__",
|
||||
):
|
||||
"""Construct a provider with discovery + JWKS stubbed (no network).
|
||||
|
||||
``client_secret`` flips the provider into confidential mode. ``auth_methods``
|
||||
overrides ``token_endpoint_auth_methods_supported`` in the seeded discovery
|
||||
doc (pass a list, or ``None`` to omit the key entirely); left unset, the
|
||||
discovery doc carries no auth-methods key (the absent-key default).
|
||||
"""
|
||||
kwargs: Dict[str, Any] = {"issuer": _ISSUER, "client_id": _CLIENT_ID}
|
||||
if scopes is not None:
|
||||
kwargs["scopes"] = scopes
|
||||
if client_secret is not None:
|
||||
kwargs["client_secret"] = client_secret
|
||||
p = oidc_plugin.SelfHostedOIDCProvider(**kwargs)
|
||||
# Pre-seed discovery so nothing hits the network.
|
||||
p._discovery = dict(_DISCOVERY_DOC)
|
||||
disco = dict(_DISCOVERY_DOC)
|
||||
if auth_methods != "__unset__":
|
||||
if auth_methods is None:
|
||||
disco.pop("token_endpoint_auth_methods_supported", None)
|
||||
else:
|
||||
disco["token_endpoint_auth_methods_supported"] = auth_methods
|
||||
p._discovery = disco
|
||||
p._discovery_fetched_at = time.time()
|
||||
# Patch the JWKS client to return our fixture key.
|
||||
fake_key = MagicMock()
|
||||
|
|
@ -655,6 +675,185 @@ class TestCompleteLogin:
|
|||
assert kwargs["data"]["client_id"] == _CLIENT_ID
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Confidential client (client_secret) — token-endpoint client authentication
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
_GOOD_TOKEN_RESP_KEYS = {"token_type": "Bearer", "refresh_token": "rt_initial"}
|
||||
|
||||
|
||||
def _decode_basic(header_value: str) -> tuple[str, str]:
|
||||
"""Decode a ``Basic <b64>`` Authorization header back to (user, pass)."""
|
||||
assert header_value.startswith("Basic ")
|
||||
raw = base64.b64decode(header_value[len("Basic ") :]).decode("utf-8")
|
||||
user, _, pw = raw.partition(":")
|
||||
# client_id / secret are form-url-encoded before base64 (RFC 6749 §2.3.1).
|
||||
return urllib.parse.unquote(user), urllib.parse.unquote(pw)
|
||||
|
||||
|
||||
class TestConfidentialClient:
|
||||
"""A configured ``client_secret`` authenticates the client at the token
|
||||
endpoint (basic header or post body, auto-selected from discovery), while
|
||||
PKCE is still sent. A public client (no secret) is byte-identical to the
|
||||
pre-confidential-client behaviour — no secret anywhere, no auth header."""
|
||||
|
||||
def _complete(self, provider, rsa_keypair):
|
||||
id_token = _mint_id_token(rsa_keypair)
|
||||
mock_resp = _mock_post(200, {"id_token": id_token, **_GOOD_TOKEN_RESP_KEYS})
|
||||
with patch(
|
||||
"plugins.dashboard_auth.self_hosted.httpx.post", return_value=mock_resp
|
||||
) as mock_post:
|
||||
provider.complete_login(
|
||||
code="the-code",
|
||||
state="s",
|
||||
code_verifier="the-verifier",
|
||||
redirect_uri="https://hermes.example/auth/callback",
|
||||
)
|
||||
_, kwargs = mock_post.call_args
|
||||
return kwargs
|
||||
|
||||
# -- public client: nothing changes ------------------------------------
|
||||
|
||||
def test_public_client_sends_no_secret_or_auth_header(self, rsa_keypair):
|
||||
# No client_secret configured → no Authorization header, no
|
||||
# client_secret in the body. Pins the unchanged public-client contract.
|
||||
provider = _make_provider(rsa_keypair) # public
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
assert "Authorization" not in kwargs["headers"]
|
||||
assert "client_secret" not in kwargs["data"]
|
||||
# PKCE still present.
|
||||
assert kwargs["data"]["code_verifier"] == "the-verifier"
|
||||
# Header is exactly the pre-feature value.
|
||||
assert kwargs["headers"] == {"Accept": "application/json"}
|
||||
|
||||
# -- basic (default & explicit) ----------------------------------------
|
||||
|
||||
def test_confidential_defaults_to_basic_when_methods_absent(self, rsa_keypair):
|
||||
# Discovery advertises no auth methods → OIDC default is Basic.
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret="s3cr3t", auth_methods=None
|
||||
)
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
assert "client_secret" not in kwargs["data"] # not in body for basic
|
||||
user, pw = _decode_basic(kwargs["headers"]["Authorization"])
|
||||
assert (user, pw) == (_CLIENT_ID, "s3cr3t")
|
||||
# PKCE still sent alongside the secret.
|
||||
assert kwargs["data"]["code_verifier"] == "the-verifier"
|
||||
|
||||
def test_confidential_basic_when_explicitly_advertised(self, rsa_keypair):
|
||||
provider = _make_provider(
|
||||
rsa_keypair,
|
||||
client_secret="s3cr3t",
|
||||
auth_methods=["client_secret_basic", "client_secret_post"],
|
||||
)
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
# When both are advertised we prefer basic (secret stays out of body).
|
||||
assert "client_secret" not in kwargs["data"]
|
||||
user, pw = _decode_basic(kwargs["headers"]["Authorization"])
|
||||
assert (user, pw) == (_CLIENT_ID, "s3cr3t")
|
||||
|
||||
# -- post --------------------------------------------------------------
|
||||
|
||||
def test_confidential_post_when_only_post_advertised(self, rsa_keypair):
|
||||
provider = _make_provider(
|
||||
rsa_keypair,
|
||||
client_secret="s3cr3t",
|
||||
auth_methods=["client_secret_post"],
|
||||
)
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
assert kwargs["data"]["client_secret"] == "s3cr3t"
|
||||
assert "Authorization" not in kwargs["headers"]
|
||||
assert kwargs["data"]["code_verifier"] == "the-verifier"
|
||||
|
||||
# -- url-encoding of reserved chars ------------------------------------
|
||||
|
||||
def test_basic_url_encodes_reserved_chars_in_secret(self, rsa_keypair):
|
||||
# A secret with ':' / '@' / space must round-trip through the Basic
|
||||
# header exactly — these are exactly the chars that corrupt a naive
|
||||
# "id:secret" concatenation.
|
||||
tricky = "p@ss:wo rd/+="
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret=tricky, auth_methods=["client_secret_basic"]
|
||||
)
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
user, pw = _decode_basic(kwargs["headers"]["Authorization"])
|
||||
assert user == _CLIENT_ID
|
||||
assert pw == tricky
|
||||
|
||||
# -- blank secret is treated as public ---------------------------------
|
||||
|
||||
def test_whitespace_secret_is_public(self, rsa_keypair):
|
||||
# A provisioned-but-blank secret must NOT flip us into confidential
|
||||
# mode (which would send an empty secret and break the exchange).
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret=" ", auth_methods=["client_secret_basic"]
|
||||
)
|
||||
kwargs = self._complete(provider, rsa_keypair)
|
||||
assert "Authorization" not in kwargs["headers"]
|
||||
assert "client_secret" not in kwargs["data"]
|
||||
|
||||
# -- refresh grant also authenticates ----------------------------------
|
||||
|
||||
def test_refresh_grant_authenticates_confidential_client(self, rsa_keypair):
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret="s3cr3t", auth_methods=["client_secret_post"]
|
||||
)
|
||||
id_token = _mint_id_token(rsa_keypair)
|
||||
mock_resp = _mock_post(
|
||||
200, {"id_token": id_token, "token_type": "Bearer", "refresh_token": "rt2"}
|
||||
)
|
||||
with patch(
|
||||
"plugins.dashboard_auth.self_hosted.httpx.post", return_value=mock_resp
|
||||
) as mock_post:
|
||||
provider.refresh_session(refresh_token="rt_old")
|
||||
_, kwargs = mock_post.call_args
|
||||
assert kwargs["data"]["grant_type"] == "refresh_token"
|
||||
assert kwargs["data"]["client_secret"] == "s3cr3t"
|
||||
|
||||
def test_refresh_grant_basic_header_confidential_client(self, rsa_keypair):
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret="s3cr3t", auth_methods=["client_secret_basic"]
|
||||
)
|
||||
id_token = _mint_id_token(rsa_keypair)
|
||||
mock_resp = _mock_post(
|
||||
200, {"id_token": id_token, "token_type": "Bearer", "refresh_token": "rt2"}
|
||||
)
|
||||
with patch(
|
||||
"plugins.dashboard_auth.self_hosted.httpx.post", return_value=mock_resp
|
||||
) as mock_post:
|
||||
provider.refresh_session(refresh_token="rt_old")
|
||||
_, kwargs = mock_post.call_args
|
||||
user, pw = _decode_basic(kwargs["headers"]["Authorization"])
|
||||
assert (user, pw) == (_CLIENT_ID, "s3cr3t")
|
||||
|
||||
# -- revocation also authenticates -------------------------------------
|
||||
|
||||
def test_revoke_authenticates_confidential_client(self, rsa_keypair):
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret="s3cr3t", auth_methods=["client_secret_post"]
|
||||
)
|
||||
with patch(
|
||||
"plugins.dashboard_auth.self_hosted.httpx.post"
|
||||
) as mock_post:
|
||||
provider.revoke_session(refresh_token="rt_old")
|
||||
_, kwargs = mock_post.call_args
|
||||
assert kwargs["data"]["client_secret"] == "s3cr3t"
|
||||
|
||||
# -- the secret never appears in logs ----------------------------------
|
||||
|
||||
def test_secret_not_in_repr_or_log(self, rsa_keypair, caplog):
|
||||
import logging
|
||||
|
||||
with caplog.at_level(logging.INFO):
|
||||
provider = _make_provider(
|
||||
rsa_keypair, client_secret="sup3r-s3cr3t", auth_methods=None
|
||||
)
|
||||
# The provider object's repr must not leak the secret.
|
||||
assert "sup3r-s3cr3t" not in repr(provider)
|
||||
assert "sup3r-s3cr3t" not in caplog.text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# verify_session
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -853,6 +1052,7 @@ class TestPluginRegister:
|
|||
"HERMES_DASHBOARD_OIDC_ISSUER",
|
||||
"HERMES_DASHBOARD_OIDC_CLIENT_ID",
|
||||
"HERMES_DASHBOARD_OIDC_SCOPES",
|
||||
"HERMES_DASHBOARD_OIDC_CLIENT_SECRET",
|
||||
):
|
||||
monkeypatch.delenv(var, raising=False)
|
||||
|
||||
|
|
@ -978,3 +1178,87 @@ class TestPluginRegister:
|
|||
oidc_plugin.register(ctx)
|
||||
ctx.register_dashboard_auth_provider.assert_not_called()
|
||||
assert "construction failed" in oidc_plugin.LAST_SKIP_REASON
|
||||
|
||||
# -- client_secret wiring ----------------------------------------------
|
||||
|
||||
def test_registers_public_when_no_secret(self, patch_config, monkeypatch):
|
||||
patch_config(None)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_ISSUER", _ISSUER)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_ID", _CLIENT_ID)
|
||||
ctx = MagicMock()
|
||||
oidc_plugin.register(ctx)
|
||||
registered = ctx.register_dashboard_auth_provider.call_args.args[0]
|
||||
assert registered._client_secret == ""
|
||||
|
||||
def test_secret_from_env(self, patch_config, monkeypatch):
|
||||
patch_config(None)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_ISSUER", _ISSUER)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_ID", _CLIENT_ID)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_SECRET", "env-secret")
|
||||
ctx = MagicMock()
|
||||
oidc_plugin.register(ctx)
|
||||
registered = ctx.register_dashboard_auth_provider.call_args.args[0]
|
||||
assert registered._client_secret == "env-secret"
|
||||
|
||||
def test_secret_from_config_yaml(self, patch_config):
|
||||
patch_config(
|
||||
{
|
||||
"self_hosted": {
|
||||
"issuer": _ISSUER,
|
||||
"client_id": _CLIENT_ID,
|
||||
"client_secret": "cfg-secret",
|
||||
}
|
||||
}
|
||||
)
|
||||
ctx = MagicMock()
|
||||
oidc_plugin.register(ctx)
|
||||
registered = ctx.register_dashboard_auth_provider.call_args.args[0]
|
||||
assert registered._client_secret == "cfg-secret"
|
||||
|
||||
def test_env_secret_overrides_config(self, patch_config, monkeypatch):
|
||||
patch_config(
|
||||
{
|
||||
"self_hosted": {
|
||||
"issuer": _ISSUER,
|
||||
"client_id": _CLIENT_ID,
|
||||
"client_secret": "cfg-secret",
|
||||
}
|
||||
}
|
||||
)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_SECRET", "env-secret")
|
||||
ctx = MagicMock()
|
||||
oidc_plugin.register(ctx)
|
||||
registered = ctx.register_dashboard_auth_provider.call_args.args[0]
|
||||
assert registered._client_secret == "env-secret"
|
||||
|
||||
def test_empty_env_secret_does_not_shadow_config(self, patch_config, monkeypatch):
|
||||
patch_config(
|
||||
{
|
||||
"self_hosted": {
|
||||
"issuer": _ISSUER,
|
||||
"client_id": _CLIENT_ID,
|
||||
"client_secret": "cfg-secret",
|
||||
}
|
||||
}
|
||||
)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_SECRET", "")
|
||||
ctx = MagicMock()
|
||||
oidc_plugin.register(ctx)
|
||||
registered = ctx.register_dashboard_auth_provider.call_args.args[0]
|
||||
assert registered._client_secret == "cfg-secret"
|
||||
|
||||
def test_register_log_reports_confidential_not_secret(
|
||||
self, patch_config, monkeypatch, caplog
|
||||
):
|
||||
import logging
|
||||
|
||||
patch_config(None)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_ISSUER", _ISSUER)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_ID", _CLIENT_ID)
|
||||
monkeypatch.setenv("HERMES_DASHBOARD_OIDC_CLIENT_SECRET", "logme-secret")
|
||||
ctx = MagicMock()
|
||||
with caplog.at_level(logging.INFO):
|
||||
oidc_plugin.register(ctx)
|
||||
# The success log reports confidentiality as a boolean, never the value.
|
||||
assert "logme-secret" not in caplog.text
|
||||
assert "confidential=True" in caplog.text
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue