Reject duplicate local_port among local forwards; document the two meanings
local_port means different things per direction, and conflating them is easy: a `direction: local` tunnel binds it here (-L local_port:remote_host:remote_port), while a reverse tunnel — the default — uses it as the *destination* on the workstation (-R remote_port:remote_host:local_port) and binds on the remote. Two local forwards on one port do not fail loudly: whichever binds first wins and the losers reconnect forever, so the map reads healthy while the port answers from a different machine than the one asked for. Reject that at config load. Reverse tunnels stay exempt by design — every state-hub-* reverse tunnel targets 8000 on purpose, so each remote box reaches this hub at its own 18000. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
701cb80d89
commit
2461ac3c70
2 changed files with 82 additions and 1 deletions
|
|
@ -5,7 +5,7 @@ import os
|
||||||
import warnings
|
import warnings
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Dict, Optional
|
from typing import Dict, List, Optional
|
||||||
|
|
||||||
import yaml
|
import yaml
|
||||||
|
|
||||||
|
|
@ -59,9 +59,45 @@ def _parse_tunnels(raw: dict) -> Dict[str, TunnelConfig]:
|
||||||
if not isinstance(data, dict):
|
if not isinstance(data, dict):
|
||||||
raise ConfigError(f"Tunnel '{name}' must be a mapping")
|
raise ConfigError(f"Tunnel '{name}' must be a mapping")
|
||||||
tunnels[name] = _parse_tunnel(name, data)
|
tunnels[name] = _parse_tunnel(name, data)
|
||||||
|
_reject_duplicate_local_ports(tunnels)
|
||||||
return tunnels
|
return tunnels
|
||||||
|
|
||||||
|
|
||||||
|
def _reject_duplicate_local_ports(tunnels: Dict[str, TunnelConfig]) -> None:
|
||||||
|
"""Two `direction: local` tunnels must never share a local_port.
|
||||||
|
|
||||||
|
Only local forwards bind a port on this machine
|
||||||
|
(``-L local_port:remote_host:remote_port``). If two claim the same one,
|
||||||
|
whichever binds first wins and the losers sit in an endless reconnect loop —
|
||||||
|
the map reads healthy while the port answers from a different machine than
|
||||||
|
the one you asked for. Silent, and worse than an outage. Fail at load
|
||||||
|
instead (BRIDGE-WP-0007).
|
||||||
|
|
||||||
|
Reverse tunnels are deliberately exempt. There, local_port is the
|
||||||
|
*destination* on this workstation
|
||||||
|
(``-R remote_port:remote_host:local_port``) and the listener is on the
|
||||||
|
remote, so sharing is the intended design: every ``state-hub-*`` reverse
|
||||||
|
tunnel targets 8000 so each remote box reaches this hub at its own 18000.
|
||||||
|
Rejecting that would be rejecting the point of the tool.
|
||||||
|
"""
|
||||||
|
by_port: Dict[int, List[str]] = {}
|
||||||
|
for name, tunnel in tunnels.items():
|
||||||
|
if tunnel.direction != "local":
|
||||||
|
continue
|
||||||
|
by_port.setdefault(tunnel.local_port, []).append(name)
|
||||||
|
collisions = {port: names for port, names in by_port.items() if len(names) > 1}
|
||||||
|
if not collisions:
|
||||||
|
return
|
||||||
|
detail = "; ".join(
|
||||||
|
f"port {port}: {', '.join(sorted(names))}" for port, names in sorted(collisions.items())
|
||||||
|
)
|
||||||
|
raise ConfigError(
|
||||||
|
f"Duplicate local_port among 'direction: local' tunnels ({detail}). "
|
||||||
|
f"Each local forward binds that port here, so they would race for it — "
|
||||||
|
f"give the non-canonical one a different port."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _parse_tunnel(name: str, data: dict) -> TunnelConfig:
|
def _parse_tunnel(name: str, data: dict) -> TunnelConfig:
|
||||||
required = ["host", "remote_port", "local_port", "ssh_user", "ssh_key", "actor"]
|
required = ["host", "remote_port", "local_port", "ssh_user", "ssh_key", "actor"]
|
||||||
for field in required:
|
for field in required:
|
||||||
|
|
|
||||||
|
|
@ -297,3 +297,48 @@ class TestCertCommandConfig:
|
||||||
monkeypatch.setenv("BRIDGE_CONFIG", str(config_file))
|
monkeypatch.setenv("BRIDGE_CONFIG", str(config_file))
|
||||||
cfg = load_config()
|
cfg = load_config()
|
||||||
assert cfg.tunnels["state-hub-coulombcore"].cert_command is None
|
assert cfg.tunnels["state-hub-coulombcore"].cert_command is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_duplicate_local_ports_are_rejected():
|
||||||
|
"""A local-forward port collision is silent at runtime; fail at load."""
|
||||||
|
from bridge.config import ConfigError, _parse_tunnels
|
||||||
|
|
||||||
|
base = {
|
||||||
|
"host": "192.0.2.1",
|
||||||
|
"remote_port": 18000,
|
||||||
|
"ssh_user": "tegwick",
|
||||||
|
"ssh_key": "~/.ssh/id_ops",
|
||||||
|
"actor": "agt-claude-railiance01",
|
||||||
|
"direction": "local",
|
||||||
|
}
|
||||||
|
with pytest.raises(ConfigError, match="Duplicate local_port"):
|
||||||
|
_parse_tunnels(
|
||||||
|
{
|
||||||
|
"k3s-a": {**base, "local_port": 16443},
|
||||||
|
"k3s-b": {**base, "local_port": 16443},
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_reverse_tunnels_may_share_a_local_port():
|
||||||
|
"""local_port is the destination on this workstation for a reverse tunnel.
|
||||||
|
|
||||||
|
Every state-hub-* reverse tunnel targets 8000 on purpose, so each remote box
|
||||||
|
reaches this hub at its own 18000. Rejecting that would break the tool.
|
||||||
|
"""
|
||||||
|
from bridge.config import _parse_tunnels
|
||||||
|
|
||||||
|
base = {
|
||||||
|
"host": "192.0.2.1",
|
||||||
|
"remote_port": 18000,
|
||||||
|
"ssh_user": "tegwick",
|
||||||
|
"ssh_key": "~/.ssh/id_ops",
|
||||||
|
"actor": "agt-claude-railiance01",
|
||||||
|
}
|
||||||
|
tunnels = _parse_tunnels(
|
||||||
|
{
|
||||||
|
"state-hub-a": {**base, "local_port": 8000},
|
||||||
|
"state-hub-b": {**base, "local_port": 8000},
|
||||||
|
}
|
||||||
|
)
|
||||||
|
assert {t.local_port for t in tunnels.values()} == {8000}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue