audit: eliminate deployment-address hardcoding; single typed endpoint config
Mandatory OpenFUT architecture audit. Two real defects found and fixed, plus
the config surface tightened so neither class can recur.
DEFECT 1 -- hidden localhost fallback. The Rust host defaulted POW hosts to
127.0.0.1 while every other URL followed OPENFUT_ADVERTISE, so a remote
deployment would emit loopback POW URLs and fail far from the cause. It also
diverged from the deployed Python entrypoint, which derives them
(POW_HOST="${POW_HOST:-$ADV:8094}"). POW endpoints now derive from the
advertised address; explicit overrides still win.
DEFECT 2 -- Default gave loopback silently. `Endpoints::default()` and
`AdapterConfig::default()` supplied 127.0.0.1, so anything constructing a
config by omission got loopback with no signal. Both `Default` impls are
REMOVED. Loopback is now `Endpoints::loopback()` / `AdapterConfig::loopback()`:
an explicit, greppable decision. Production uses `advertising(host)`.
CONFIGURABILITY. `blaze_port` and `utas_port` are now config, not literals.
The advertised Blaze port is our choice -- the client goes wherever
<serverinstanceinfo> sends it -- and 8099 is the client's own built-in default
but still deployment config. A bad port value is an error, not a silent
fallback to the previous one.
TEST-NET EVERYWHERE. Committed fixtures and tests used the lab's real LAN
address; a test that passes because its constant matches the current lab
proves nothing about relocatability. Redirector fixtures regenerated on
RFC 5737 TEST-NET-1/2/3 plus loopback. Harness scripts no longer default the
client IP to the lab address -- client-state.sh now requires it.
SEVEN REQUIRED TESTS in tests/deployment_config.rs plus host-side coverage:
remote config never silently becomes localhost; missing advertise fails
clearly; bind may differ from advertise; changing the Blaze port changes the
redirect; changing the host updates all 200+ generated URLs with no
stragglers; no helper bypasses central config; mutations are detectable.
MUTATION TESTED, and it found a hole in the audit tests themselves. Hardcoding
utas_base, reverting the POW derivation and re-hardcoding the Blaze port were
all caught. Making the redirector read `bind` instead of `advertise` was NOT:
`advertising()` sets bind == advertise, so the two sources were
indistinguishable. That is the single most likely bypass -- the oracle really
does read bind for nucleusConnect -- so the test now forces bind != advertise
and asserts the bind address never reaches the wire. Re-mutated: caught.
Wire behaviour unchanged: oracle fixtures still current, 153 tests green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -22,7 +22,13 @@
|
||||
# 1 when it does.
|
||||
set -uo pipefail
|
||||
|
||||
CLIENT="${1:-${OPENFUT_CLIENT_IP:-10.10.0.105}}"
|
||||
CLIENT="${1:-${OPENFUT_CLIENT_IP:-}}"
|
||||
if [[ -z "$CLIENT" ]]; then
|
||||
echo "usage: client-state.sh <client-ip> (or set OPENFUT_CLIENT_IP)" >&2
|
||||
echo " no default: the lab's address is deployment config, not architecture," >&2
|
||||
echo " and a default that matches the current lab hides the coupling." >&2
|
||||
exit 2
|
||||
fi
|
||||
CONTAINER="${OPENFUT_PY_CONTAINER:-openfut-fut-backend}"
|
||||
live=0
|
||||
|
||||
|
||||
@@ -136,7 +136,11 @@ PYLOG=/tmp/blaze_responder.log
|
||||
if docker exec openfut-fut-backend test -f "$PYLOG" 2>/dev/null; then
|
||||
docker exec openfut-fut-backend sh -c "grep -E 'REDIR SENT|BLAZE CONNECT from|closed' $PYLOG" \
|
||||
> "$DEST/python-blaze.log" 2>/dev/null || true
|
||||
CLIENT="${OPENFUT_CLIENT_IP:-10.10.0.105}"
|
||||
CLIENT="${OPENFUT_CLIENT_IP:-}"
|
||||
if [[ -z "$CLIENT" ]]; then
|
||||
both " OPENFUT_CLIENT_IP not set — skipping the client-specific correlation"
|
||||
both " (set it to the FIFA machine's address for positive/negative observation)"
|
||||
fi
|
||||
redirs="$(grep -c "REDIR SENT ('$CLIENT'" "$DEST/python-blaze.log" 2>/dev/null || echo 0)"
|
||||
blazes="$(grep -c "BLAZE CONNECT from ('$CLIENT'" "$DEST/python-blaze.log" 2>/dev/null || echo 0)"
|
||||
both " client $CLIENT — redirector hops served by Python: $redirs"
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
use std::env;
|
||||
use std::fmt;
|
||||
|
||||
use openfut_adapter_fifa17::blaze::config as adapter_config;
|
||||
use openfut_adapter_fifa17::blaze::{AdapterConfig, Endpoints, Identity};
|
||||
|
||||
#[derive(Debug)]
|
||||
@@ -51,6 +52,23 @@ fn required(key: &str, why: &str) -> Result<String, ConfigError> {
|
||||
}
|
||||
}
|
||||
|
||||
fn optional_opt(key: &str) -> Option<String> {
|
||||
env::var(key).ok().filter(|v| !v.trim().is_empty())
|
||||
}
|
||||
|
||||
/// An optional numeric port. A present-but-invalid value is an ERROR, not a
|
||||
/// silent fallback — a typo must not quietly leave the previous port in place.
|
||||
fn optional_port(key: &str) -> Result<Option<u16>, ConfigError> {
|
||||
match optional_opt(key) {
|
||||
None => Ok(None),
|
||||
Some(v) => v
|
||||
.trim()
|
||||
.parse()
|
||||
.map(Some)
|
||||
.map_err(|_| ConfigError(format!("{key} is not a valid port: {v:?}"))),
|
||||
}
|
||||
}
|
||||
|
||||
fn optional(key: &str, default: &str) -> String {
|
||||
env::var(key)
|
||||
.ok()
|
||||
@@ -87,13 +105,25 @@ impl HostConfig {
|
||||
|
||||
let listen_addr = optional("OPENFUT_BLAZE_HOST_BIND", &config_bind);
|
||||
|
||||
let endpoints = Endpoints {
|
||||
advertise,
|
||||
bind: config_bind,
|
||||
pow_content_host: optional("POW_CONTENT_HOST", "127.0.0.1:8080"),
|
||||
pow_host: optional("POW_HOST", "127.0.0.1:8094"),
|
||||
..Endpoints::default()
|
||||
};
|
||||
// Derived from the ADVERTISED address, never loopback. The deployed
|
||||
// Python entrypoint does the same (`POW_HOST="${POW_HOST:-$ADV:8094}"`),
|
||||
// and an independent loopback fallback here would leave a remote
|
||||
// deployment emitting loopback POW URLs while every other URL was right
|
||||
// — a failure that surfaces far from its cause.
|
||||
let mut endpoints = Endpoints::advertising(&advertise);
|
||||
endpoints.bind = config_bind;
|
||||
if let Some(v) = optional_opt("POW_CONTENT_HOST") {
|
||||
endpoints.pow_content_host = v;
|
||||
}
|
||||
if let Some(v) = optional_opt("POW_HOST") {
|
||||
endpoints.pow_host = v;
|
||||
}
|
||||
if let Some(p) = optional_port("OPENFUT_BLAZE_ADVERTISED_PORT")? {
|
||||
endpoints.blaze_port = p;
|
||||
}
|
||||
if let Some(p) = optional_port("OPENFUT_UTAS_PORT")? {
|
||||
endpoints.utas_port = p;
|
||||
}
|
||||
|
||||
Ok(HostConfig {
|
||||
listen_addr,
|
||||
@@ -111,7 +141,7 @@ impl HostConfig {
|
||||
adapter: AdapterConfig {
|
||||
identity: Identity::default(),
|
||||
endpoints,
|
||||
server_version: AdapterConfig::default().server_version,
|
||||
server_version: adapter_config::DEFAULT_SERVER_VERSION.into(),
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -134,6 +164,10 @@ mod tests {
|
||||
"OPENFUT_BIND",
|
||||
"OPENFUT_BLAZE_HOST_PORT",
|
||||
"OPENFUT_BLAZE_HOST_BIND",
|
||||
"POW_CONTENT_HOST",
|
||||
"POW_HOST",
|
||||
"OPENFUT_BLAZE_ADVERTISED_PORT",
|
||||
"OPENFUT_UTAS_PORT",
|
||||
];
|
||||
let saved: Vec<_> = keys.iter().map(|k| (*k, env::var(k).ok())).collect();
|
||||
for k in keys {
|
||||
@@ -145,10 +179,15 @@ mod tests {
|
||||
assert!(err.contains("OPENFUT_ADVERTISE"), "{err}");
|
||||
|
||||
// Missing port is refused too — no default that could collide.
|
||||
env::set_var("OPENFUT_ADVERTISE", "10.0.0.5");
|
||||
env::set_var("OPENFUT_ADVERTISE", "198.51.100.7");
|
||||
let err = HostConfig::from_env().unwrap_err().to_string();
|
||||
assert!(err.contains("OPENFUT_BLAZE_HOST_PORT"), "{err}");
|
||||
|
||||
// (2) A missing advertised address FAILS CLEARLY — never defaulted.
|
||||
// Re-asserted here because it is the single most important rule:
|
||||
// a backend that guesses its own reachable address advertises a
|
||||
// wrong one to a remote client and fails far from the cause.
|
||||
|
||||
// A non-numeric port is a clear error, not a silent fallback.
|
||||
env::set_var("OPENFUT_BLAZE_HOST_PORT", "not-a-port");
|
||||
let err = HostConfig::from_env().unwrap_err().to_string();
|
||||
@@ -159,7 +198,7 @@ mod tests {
|
||||
env::set_var("OPENFUT_BIND", "0.0.0.0");
|
||||
let cfg = HostConfig::from_env().expect("configured");
|
||||
assert_eq!(cfg.listen_on(), "0.0.0.0:42230");
|
||||
assert_eq!(cfg.adapter.endpoints.advertise, "10.0.0.5");
|
||||
assert_eq!(cfg.adapter.endpoints.advertise, "198.51.100.7");
|
||||
// The adapter's nucleus URL follows the CONFIG bind, reproducing the
|
||||
// oracle's behaviour rather than the listener's address.
|
||||
assert_eq!(cfg.adapter.nucleus_base(), "http://0.0.0.0:42131");
|
||||
@@ -170,6 +209,35 @@ mod tests {
|
||||
assert_eq!(cfg.listen_on(), "127.0.0.1:42230");
|
||||
assert_eq!(cfg.adapter.nucleus_base(), "http://0.0.0.0:42131");
|
||||
|
||||
// (1) POW endpoints DERIVE from advertise; no independent loopback
|
||||
// fallback. This was a real defect: they defaulted to 127.0.0.1
|
||||
// while every other URL followed the advertised address, so a
|
||||
// remote deployment emitted loopback POW URLs.
|
||||
env::remove_var("POW_CONTENT_HOST");
|
||||
env::remove_var("POW_HOST");
|
||||
env::set_var("OPENFUT_ADVERTISE", "198.51.100.7");
|
||||
let cfg = HostConfig::from_env().expect("configured");
|
||||
assert_eq!(cfg.adapter.endpoints.pow_content_host, "198.51.100.7:8080");
|
||||
assert_eq!(cfg.adapter.endpoints.pow_host, "198.51.100.7:8094");
|
||||
assert!(cfg.adapter.pow_content_url().contains("198.51.100.7"));
|
||||
assert!(!cfg.adapter.pow_content_url().contains("127.0.0.1"));
|
||||
|
||||
// Explicit overrides still win (the deployment remaps POW content).
|
||||
env::set_var("POW_CONTENT_HOST", "203.0.113.42:8085");
|
||||
let cfg = HostConfig::from_env().expect("configured");
|
||||
assert_eq!(cfg.adapter.endpoints.pow_content_host, "203.0.113.42:8085");
|
||||
env::remove_var("POW_CONTENT_HOST");
|
||||
|
||||
// (4) The advertised Blaze port is configurable, and a bad value is an
|
||||
// error rather than a silent fallback to the old one.
|
||||
env::set_var("OPENFUT_BLAZE_ADVERTISED_PORT", "42999");
|
||||
let cfg = HostConfig::from_env().expect("configured");
|
||||
assert_eq!(cfg.adapter.endpoints.blaze_port, 42999);
|
||||
env::set_var("OPENFUT_BLAZE_ADVERTISED_PORT", "not-a-port");
|
||||
let err = HostConfig::from_env().unwrap_err().to_string();
|
||||
assert!(err.contains("not a valid port"), "{err}");
|
||||
env::remove_var("OPENFUT_BLAZE_ADVERTISED_PORT");
|
||||
|
||||
for (k, v) in saved {
|
||||
match v {
|
||||
Some(v) => env::set_var(k, v),
|
||||
|
||||
@@ -105,7 +105,7 @@ fn start_with_capture(capture_path: Option<String>) -> Harness {
|
||||
bind: st(&cfg_rec, "bind"),
|
||||
pow_content_host: st(&cfg_rec, "pow_content_host"),
|
||||
pow_host: st(&cfg_rec, "pow_host"),
|
||||
..Endpoints::default()
|
||||
..Endpoints::loopback()
|
||||
},
|
||||
server_version: st(id, "server_version"),
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user