ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

fix(relay): accept a provision response that issues no secret (lockstep with gateway-gateway#223) (#104767)

Browse Source 
* fix(relay): accept a provision response that issues no secret

Lockstep with gateway-gateway #223 (finding F-004). The connector used to return
the stored per-gateway secret from POST /relay/provision on EVERY replay, to
anyone reaching the endpoint with the right gatewayId. It now returns credential
material only to a caller that proves recorded ownership or possession, and
answers secretIssued:false otherwise with secret/deliveryKey ABSENT. This
gateway treats that as a hard failure, so #223 cannot merge until this ships.

1. A secret-less response is a VALUE, not an error.

_post_provision() raised on any response without a secret. Two legitimate
responses hit that raise once #223 lands:
  - every platform after the first in a multi-platform boot. All platforms share
    ONE gatewayId, so the first POST mints the secret and the rest are replays of
    an unchanged binding. self_provision_relay() catches RuntimeError per
    platform and continues, so a Telegram+Discord gateway would front Telegram
    and leave Discord unfronted with a warning.
  - a re-provision by a caller that legitimately no longer holds the secret.
    Refusing to hand it back IS F-004; /relay/rotate is the way back.

Only a malformed body raises now. No secret AND no secretIssued key is still the
old unexpected-response case (a pre-F-004 connector), so that keeps failing
loudly -- which is what makes this deployable in either order.

2. Only a response that CARRIES credentials may write them.

A separate bug that change 1 exposes rather than causes. The guard tested the
same env var it wrote: once a withheld response reaches it, it writes an empty
secret -- which then reads as "still unset" on the next pass, masking itself,
while GATEWAY_RELAY_ID and _DELIVERY_KEY have ALREADY been stamped from a
response carrying no credentials. Guard on the secret being present.

Tests: 5 invariants in tests/gateway/relay/test_provision_secret_optional.py.
Proven red on base -- reverting gateway/relay/__init__.py to origin/main turns 2
of the 5 red, one per fix, then green with the fix restored. My first draft of
the multi-platform test passed on base because it had the FIRST platform issue
the secret, which satisfies the old guard so it never re-enters and the bug
never fires; it proved nothing until the ordering was fixed and a direct
write-rule test added.

tests/gateway/relay: 303 passed, 0 failed. The 6 failures in the wider
tests/gateway run (systemd_notify, wecom_callback, shutdown_forensics,
scale_to_zero) reproduce identically on clean origin/main -- pre-existing.

* fix(relay): fail closed on the F-004 discriminator; a fully-withheld boot is not a provision

Two review findings on the parent commit, each with a red-on-parent test.

1. `_post_provision()` accepted ANY present `secretIssued` value when `secret`
   was absent (`true`, `"false"`, `0`, …) and settled them as a successful
   provision. The only credential-less shape the connector emits is literal
   JSON `false`, so match the discriminator by value: `is not False` raises.

2. `self_provision_relay()` returned True and logged the INFO "self-provisioned"
   line when every platform answered `secretIssued:false` — no credential in
   hand, WS upgrade about to be refused, nothing in the boot log to say why.
   Now: WARNING naming `/relay/rotate` / pin `GATEWAY_RELAY_SECRET`, return False.

tests/gateway/relay/: 308 passed, 0 failed.
This commit is contained in:
Ben Barclay
2026-09-10 12:10:39 +10:00
committed by GitHub
parent ecefb1e2a2
commit f97a4102dd
2 changed files with 355 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
gateway/relay/__init__.py
+55 -5
View File
@@ -332,7 +332,32 @@ def _post_provision(
except urllib.error.URLError as exc:
raise RuntimeError(f"could not reach connector: {exc.reason}") from exc
if not isinstance(payload, dict) or not payload.get("secret"):
if not isinstance(payload, dict):
raise RuntimeError("connector returned an unexpected response")
# A response WITHOUT a secret is valid, not an error (connector F-004).
#
# `/relay/provision` used to hand the stored per-gateway secret back on every
# replay, to anyone who reached the endpoint with the right gatewayId. The
# connector now returns credential material only to a caller that proves
# ownership or possession, and answers `secretIssued: false` otherwise — with
# `secret`/`deliveryKey` absent rather than empty.
#
# Two legitimate shapes reach here, and neither is a failure:
# * the SECOND and later platforms of a multi-platform boot. All platforms
# share one gatewayId, so the first POST mints the secret and the rest are
# replays of an unchanged binding. Raising here aborted every platform
# after the first.
# * a re-provision by a caller that legitimately no longer holds the secret;
# `POST /relay/rotate` is the supported recovery path.
#
# Treat "no secret" as a value, and let the caller decide — it knows whether
# it already holds credentials. Only a malformed body is an error.
if not payload.get("secret") and payload.get("secretIssued") is not False:
# Fail closed on the discriminator itself. The ONLY credential-less shape
# the connector emits is literal JSON `false`. No key at all is a
# pre-F-004 connector (the old unexpected-response case); `true` with no
# secret, or any non-boolean value, is a malformed body. Accepting those
# would mark the platform provisioned with no credential in hand.
raise RuntimeError("connector returned an unexpected response (no secret)")
return payload
@@ -490,11 +515,20 @@ def self_provision_relay() -> bool:
)
continue
provisioned.append(platform)
# Set creds in-process on the FIRST success (the per-gateway secret
# authenticates the outbound WS upgrade). Never logged.
if not os.environ.get("GATEWAY_RELAY_SECRET"):
# Set creds in-process on the first response that ACTUALLY CARRIES them.
# Never logged.
#
# `secret` may legitimately be absent (connector F-004 — see _post_provision):
# only a caller that proves ownership or possession is handed credential
# material, and every platform after the first in a multi-platform boot is
# a replay of an unchanged binding. Guard on the secret being PRESENT
# rather than on the response having arrived, or the first withheld
# response writes empty strings over the real values — and because the
# `if` tests the same env var it writes, an empty write looks "unset" on
# the next pass while deliveryKey has already been clobbered.
if result.get("secret") and not os.environ.get("GATEWAY_RELAY_SECRET"):
os.environ["GATEWAY_RELAY_ID"] = str(result.get("gatewayId") or gateway_id)
os.environ["GATEWAY_RELAY_SECRET"] = str(result.get("secret") or "")
os.environ["GATEWAY_RELAY_SECRET"] = str(result["secret"])
os.environ["GATEWAY_RELAY_DELIVERY_KEY"] = str(result.get("deliveryKey") or "")
if not provisioned:
@@ -504,6 +538,22 @@ def self_provision_relay() -> bool:
)
return False
if not os.environ.get("GATEWAY_RELAY_SECRET"):
# Every platform answered, none issued a credential: the connector holds a
# binding for this gatewayId that this caller could prove neither ownership
# of nor possession of (F-004). The routes are bound but the WS upgrade
# will be refused, so this is NOT a provision — do not report one. The
# operator's recovery path is POST /relay/rotate (or pinning
# GATEWAY_RELAY_SECRET).
logger.warning(
"relay self-provision withheld credentials for ALL platforms (%s): the connector "
"holds a binding for gateway_id=%s this caller could not prove ownership of; "
"gateway will boot without relay auth. Recover with POST /relay/rotate or pin "
"GATEWAY_RELAY_SECRET",
",".join(provisioned), gateway_id,
)
return False
logger.info(
"relay self-provisioned (gateway_id=%s tenant=%s platforms=%s routes=%d inbound=%s instance=%s wake=%s)",
os.environ.get("GATEWAY_RELAY_ID", gateway_id),
tests/gateway/relay/test_provision_secret_optional.py
+300
View File
@@ -0,0 +1,300 @@
"""Lockstep contract with the connector's F-004 change (gateway-gateway #223).
`POST /relay/provision` used to return the stored per-gateway secret on every
replay, to anyone who reached the endpoint with the right gatewayId. The
connector now returns credential material ONLY to a caller that proves ownership
or possession, and answers ``secretIssued: false`` otherwise, with
``secret``/``deliveryKey`` absent rather than empty.
These are behaviour contracts on how this gateway must treat that response, not
snapshots of it:
1. a secret-less response is a VALUE, not an error — the multi-platform boot
loop must keep going, because every platform after the first shares one
gatewayId and is therefore a replay of an unchanged binding;
2. credentials are only ever written from a response that actually carries
them, so a withheld response cannot blank creds already in hand;
3. a genuinely malformed body still fails loudly.
"""
from __future__ import annotations
import os
import pytest
import gateway.relay as relay
@pytest.fixture(autouse=True)
def _clean_env(monkeypatch):
for k in (
"GATEWAY_RELAY_URL",
"GATEWAY_RELAY_ID",
"GATEWAY_RELAY_SECRET",
"GATEWAY_RELAY_DELIVERY_KEY",
"GATEWAY_RELAY_ENDPOINT",
"GATEWAY_RELAY_ROUTE_KEYS",
"GATEWAY_RELAY_PLATFORM",
"GATEWAY_RELAY_BOT_ID",
"GATEWAY_RELAY_INSTANCE_ID",
"GATEWAY_RELAY_WAKE_URL",
):
monkeypatch.delenv(k, raising=False)
monkeypatch.setattr("gateway.run._load_gateway_config", lambda: {}, raising=False)
class _Resp:
"""Minimal stand-in for the urlopen context manager `_post_provision` uses."""
def __init__(self, body: bytes) -> None:
self._body = body
def read(self) -> bytes:
return self._body
def __enter__(self):
return self
def __exit__(self, *_exc) -> bool:
return False
def _post_returning(monkeypatch, body: str) -> None:
def _fake_json_post(url, token, payload, timeout): # noqa: ANN001
return _Resp(body.encode())
monkeypatch.setattr(relay, "_json_post", _fake_json_post, raising=True)
def test_secretless_provision_response_is_not_an_error(monkeypatch):
"""A withheld secret must be returned to the caller, not raised.
This is the leg that gates the whole lockstep: the connector answers
`secretIssued:false` on a replay by a caller that proved neither ownership
nor possession, and on every platform after the first in a multi-platform
boot. Raising here aborts a legitimate provision.
"""
_post_returning(
monkeypatch,
'{"secretIssued": false, "tenant": "org-x", "gatewayId": "gw-1", "routeKeys": ["k"]}',
)
payload = relay._post_provision(
provision_url="https://connector.example/relay/provision",
access_token="tok",
gateway_id="gw-1",
platform="telegram",
bot_id="BOT",
gateway_endpoint="",
route_keys=["k"],
)
assert payload["secretIssued"] is False
assert "secret" not in payload
assert payload["gatewayId"] == "gw-1"
@pytest.mark.parametrize(
"body",
[
pytest.param('{"tenant": "org-x", "gatewayId": "gw-1"}', id="no-discriminator-pre-f004"),
pytest.param('{"secretIssued": true, "gatewayId": "gw-1"}', id="issued-true-but-absent"),
pytest.param('{"secretIssued": "false", "gatewayId": "gw-1"}', id="string-false"),
pytest.param('{"secretIssued": 0, "gatewayId": "gw-1"}', id="falsy-non-bool"),
pytest.param('{"secretIssued": null, "gatewayId": "gw-1"}', id="explicit-null"),
],
)
def test_malformed_response_still_raises(monkeypatch, body):
"""Only literal `secretIssued: false` may stand in for a secret.
The discriminator is the credential-issuance trust boundary, so it is
matched by VALUE, not presence: no key is a pre-F-004 body, `true` with no
secret is a contradiction, and a non-boolean is not the connector's shape.
Every one of them must fail closed — accepting any would mark the platform
provisioned with no credential in hand.
"""
_post_returning(monkeypatch, body)
with pytest.raises(RuntimeError, match="no secret"):
relay._post_provision(
provision_url="https://connector.example/relay/provision",
access_token="tok",
gateway_id="gw-1",
platform="telegram",
bot_id="BOT",
gateway_endpoint="",
route_keys=["k"],
)
def test_multiplatform_boot_survives_a_secretless_second_platform(monkeypatch):
"""The credentials from platform 1 must survive platform 2 withholding them.
All platforms share one gatewayId, so exactly one POST mints the secret and
the rest are replays. The invariant: creds are written only from a response
that carries them, and every platform is still provisioned.
"""
calls: list[str] = []
def _fake_post(**kwargs):
calls.append(kwargs["platform"])
if len(calls) == 1:
return {
"secretIssued": True,
"secret": "a" * 64,
"deliveryKey": "b" * 64,
"tenant": "org-x",
"gatewayId": kwargs["gateway_id"],
"routeKeys": kwargs["route_keys"],
}
# Replay of an unchanged binding: no credential material.
return {
"secretIssued": False,
"tenant": "org-x",
"gatewayId": kwargs["gateway_id"],
"routeKeys": kwargs["route_keys"],
}
monkeypatch.setattr(relay, "_post_provision", _fake_post, raising=True)
monkeypatch.setattr(relay, "_resolve_relay_identity_token", lambda: "tok", raising=True)
monkeypatch.setenv("GATEWAY_RELAY_URL", "https://connector.example")
monkeypatch.setattr(
relay,
"relay_platform_identities",
lambda: [("telegram", "BOT_T"), ("discord", "BOT_D")],
raising=False,
)
ok = relay.self_provision_relay()
assert ok is True
assert calls == ["telegram", "discord"], "the secret-less platform must not abort the loop"
# THE POINT: platform 2's withheld response must not blank platform 1's creds.
assert os.environ["GATEWAY_RELAY_SECRET"] == "a" * 64
assert os.environ["GATEWAY_RELAY_DELIVERY_KEY"] == "b" * 64
def test_a_secretless_first_platform_does_not_write_empty_creds(monkeypatch):
"""A withheld FIRST response must leave the env untouched for the next one.
The old guard tested the same variable it wrote, so an empty write read as
"still unset" on the next pass while deliveryKey had already been clobbered.
"""
calls: list[str] = []
def _fake_post(**kwargs):
calls.append(kwargs["platform"])
if len(calls) == 1:
# Withheld, and it names a DIFFERENT gatewayId so we can see which
# response actually stamped the env.
return {
"secretIssued": False,
"tenant": "org-x",
"gatewayId": "gw-from-withheld",
"routeKeys": kwargs["route_keys"],
}
return {
"secretIssued": True,
"secret": "c" * 64,
"deliveryKey": "d" * 64,
"tenant": "org-x",
"gatewayId": "gw-self",
"routeKeys": kwargs["route_keys"],
}
monkeypatch.setattr(relay, "_post_provision", _fake_post, raising=True)
monkeypatch.setattr(relay, "_resolve_relay_identity_token", lambda: "tok", raising=True)
monkeypatch.setenv("GATEWAY_RELAY_URL", "https://connector.example")
monkeypatch.setattr(
relay,
"relay_platform_identities",
lambda: [("telegram", "BOT_T"), ("discord", "BOT_D")],
raising=False,
)
ok = relay.self_provision_relay()
assert ok is True
# The later real credentials must land, not be shadowed by an empty write.
assert os.environ["GATEWAY_RELAY_SECRET"] == "c" * 64
assert os.environ["GATEWAY_RELAY_DELIVERY_KEY"] == "d" * 64
# And the withheld FIRST response must not have written anything at all.
# The old guard wrote `str(result.get("secret") or "")` unconditionally, so
# GATEWAY_RELAY_ID was stamped from a response carrying no credentials — the
# secret stayed falsy (masking it), but the id and delivery key were already
# set from the wrong response. Assert the id came from the credential-bearing
# response, which is the only one entitled to name the gateway.
assert os.environ["GATEWAY_RELAY_ID"] == "gw-self"
def test_a_withheld_response_never_overwrites_credentials_in_hand(monkeypatch):
"""Creds already held must survive a later withheld response.
Directly pins the write rule: only a response carrying a secret may touch
the credential env vars. Under the old guard the env var it tested was the
same one it wrote, so an empty write looked "unset" and the next pass
re-entered — clobbering deliveryKey with the withheld response's absent one.
"""
calls: list[str] = []
def _fake_post(**kwargs):
calls.append(kwargs["platform"])
# EVERY platform withholds; nothing may be written.
return {
"secretIssued": False,
"tenant": "org-x",
"gatewayId": "gw-connector-chosen",
"routeKeys": kwargs["route_keys"],
}
monkeypatch.setattr(relay, "_post_provision", _fake_post, raising=True)
monkeypatch.setattr(relay, "_resolve_relay_identity_token", lambda: "tok", raising=True)
monkeypatch.setenv("GATEWAY_RELAY_URL", "https://connector.example")
monkeypatch.setattr(
relay,
"relay_platform_identities",
lambda: [("telegram", "BOT_T"), ("discord", "BOT_D")],
raising=False,
)
ok = relay.self_provision_relay()
assert calls == ["telegram", "discord"]
# Nothing was written, because nothing was issued.
assert not os.environ.get("GATEWAY_RELAY_SECRET")
assert not os.environ.get("GATEWAY_RELAY_DELIVERY_KEY")
assert not os.environ.get("GATEWAY_RELAY_ID")
# And the caller must be told: routes bound but no credential in hand is not
# a provision — the WS upgrade will be refused, and the operator needs to hear
# it here, not as a 4401 later.
assert ok is False
def test_all_platforms_withheld_warns_with_the_recovery_path(monkeypatch, caplog):
"""A boot that ends with no credential must log a WARNING naming the way out.
Before this, a fully-withheld boot logged the same INFO 'self-provisioned'
line as a real one and returned True — a silent success in place of the
loud failure it replaced.
"""
monkeypatch.setattr(
relay,
"_post_provision",
lambda **kw: {"secretIssued": False, "tenant": "org-x", "gatewayId": kw["gateway_id"], "routeKeys": []},
raising=True,
)
monkeypatch.setattr(relay, "_resolve_relay_identity_token", lambda: "tok", raising=True)
monkeypatch.setenv("GATEWAY_RELAY_URL", "https://connector.example")
monkeypatch.setattr(relay, "relay_platform_identities", lambda: [("telegram", "BOT_T")], raising=False)
import logging
with caplog.at_level(logging.INFO, logger=relay.logger.name):
ok = relay.self_provision_relay()
assert ok is False
warnings = [r for r in caplog.records if r.levelno >= logging.WARNING]
assert warnings, "a fully-withheld boot must warn"
assert "/relay/rotate" in warnings[-1].getMessage()
assert not any("self-provisioned (" in r.getMessage() for r in caplog.records)