diff --git a/backend/app.py b/backend/app.py index 7398d5e..644edf4 100644 --- a/backend/app.py +++ b/backend/app.py @@ -9,7 +9,7 @@ from fastapi import FastAPI, HTTPException, Depends, APIRouter from fastapi.security import OAuth2PasswordBearer, OAuth2PasswordRequestForm from jose import jwt from utils.sqlite import SQLite -from utils import API_VERSION, FIREGEX_PORT, FIREGEX_HOST, FIREGEX_SOCKET, JWT_ALGORITHM, get_interfaces, socketio_emit, DEBUG, SysctlManager, NORELOAD, safe_join +from utils import SAFE_DB_NAME, SAFE_PY_NAME, API_VERSION, FIREGEX_PORT, FIREGEX_HOST, FIREGEX_SOCKET, JWT_ALGORITHM, get_interfaces, socketio_emit, DEBUG, SysctlManager, NORELOAD, safe_join from utils.loader import frontend_deploy, load_routers from utils.models import AuthModeForm, ChangePasswordModel, IpInterface, PasswordChangeForm, PasswordForm, ResetRequest, StatusModel, StatusMessageModel from contextlib import asynccontextmanager @@ -18,7 +18,6 @@ import socketio from socketio.exceptions import ConnectionRefusedError import hashlib import hmac -import re from ipaddress import ip_network, ip_address # DB init db = SQLite('db/firegex.db') @@ -403,11 +402,8 @@ async def export_db(): dbs['service_filters'][f] = base64.b64encode(script_file.read()).decode('utf-8') return dbs -# Backup entries are limited to plain basenames of the two shapes export_db() -# produces: ".db" databases and ".py" filter code files. The charset -# forbids path separators and "..", so a crafted key can't escape the target dir. -_SAFE_DB_NAME = re.compile(r'^[A-Za-z0-9_-]+\.db$') -_SAFE_PY_NAME = re.compile(r'^[A-Za-z0-9_-]+\.py$') +# The charset a backup entry's name has to satisfy lives in `utils`, beside `safe_join`: +# the two are one defence in two halves — what a name may contain, and where it may land. @api.post('/import', response_model=StatusMessageModel) async def import_db(data: dict): @@ -420,7 +416,7 @@ async def import_db(data: dict): for key, value in data.items(): if not isinstance(key, str): raise HTTPException(status_code=400, detail="Invalid backup: keys must be strings") - if _SAFE_DB_NAME.match(key): + if SAFE_DB_NAME.fullmatch(key): if not isinstance(value, dict): raise HTTPException(status_code=400, detail=f"Invalid backup: '{key}' must be an object") # safe_join is defense-in-depth on top of the regex: it rejects any @@ -430,7 +426,7 @@ async def import_db(data: dict): if not isinstance(value, dict): raise HTTPException(status_code=400, detail="Invalid backup: 'service_filters' must be an object") for fname, script_content in value.items(): - if not isinstance(fname, str) or not _SAFE_PY_NAME.match(fname): + if not isinstance(fname, str) or not SAFE_PY_NAME.fullmatch(fname): raise HTTPException(status_code=400, detail=f"Invalid backup: illegal filter filename '{fname}'") if not isinstance(script_content, str): raise HTTPException(status_code=400, detail=f"Invalid backup: filter '{fname}' must be a base64 string") diff --git a/backend/modules/services/nftables.py b/backend/modules/services/nftables.py index daa46c6..4702975 100644 --- a/backend/modules/services/nftables.py +++ b/backend/modules/services/nftables.py @@ -138,6 +138,27 @@ def one_address(ip: str) -> str: return str(ip).split("/")[0] +def hijack_endpoint(proxy_ip: str | None, service_ip: str) -> str: + """Where the operator's own proxy listens for one address, as a single host. + + The one spelling of the default, because it is asked in three places that must agree: + the rule that sends traffic to that proxy, the rule that undoes the rewrite on the way + back, and the uniqueness check that stops two addresses sharing one endpoint. Loopback + is where such a proxy normally is, in the family of the address it fronts. + + It is resolved when an address is **written**, not only when its rules are built. The + column used to be left NULL and defaulted here — and SQLite's UNIQUE treats every NULL + as distinct, so the partial index meant to forbid two addresses behind one endpoint + happily accepted any number of them as long as nobody typed the address out. They then + all resolved to the same loopback port, which is exactly the collision the index exists + to prevent: the return rule recognises the proxy by address and port, so it could not + tell them apart on the way out. + """ + return one_address(proxy_ip or "") or ( + "::1" if ip_family(one_address(service_ip)) == "ip6" else "127.0.0.1" + ) + + class NoRelayAddress(Exception): """An address a UDP relay cannot be bound for.""" @@ -220,6 +241,22 @@ class InstalledRule: self.packets = int(packets) self.bytes = int(bytes_) + def _is(self, ip: str, port: int) -> bool: + """Whether this rule matches one particular address and port. + + Both halves together, because an installed rule is identified by the pair: the + two legs of a hand-off match different addresses on **different ports**, and + comparing the port against only one of them is what made the outbound one + invisible. + """ + if self.port != int(port): + return False + if is_ip_parse(self.ip_int) and is_ip_parse(ip): + return ip_parse(self.ip_int) == ip_parse(ip) + if not is_ip_parse(self.ip_int) and not is_ip_parse(ip): + return self.ip_int == ip + return False + def matches(self, srv: Service, addr: Address) -> bool: """Whether this installed rule is one of `addr`'s.""" # The address already carries the transport the kernel matches on, derived @@ -230,22 +267,17 @@ class InstalledRule: if target is None: return False target_ip, target_port = target - if self.port != int(target_port): - return False - if is_ip_parse(self.ip_int) and is_ip_parse(target_ip): - if ip_parse(self.ip_int) == ip_parse(target_ip): - return True - elif not is_ip_parse(self.ip_int) and not is_ip_parse(target_ip): - if self.ip_int == target_ip: - return True + if self._is(target_ip, target_port): + return True # The return leg of an external hand-off matches the operator's proxy, not the - # service. Recognising only the inbound rule would leave the outbound one behind - # every time a service was stopped, and it would keep rewriting. + # service — **and their port, not the service's**. The port was compared against + # the service's before either identity was considered, so this branch was only + # ever reached when the two happened to be the same number: every other hand-off + # left its outbound rule behind on stop, still rewriting the source of anything + # from that proxy endpoint, and installed a second copy on the next start. if srv.transport == TRANSPORT.EXTERNAL and addr.proxy_port and is_ip_parse(target_ip): - proxy_ip = one_address(addr.proxy_ip or "") or ( - "::1" if ip_family(target_ip) == "ip6" else "127.0.0.1" - ) - return is_ip_parse(self.ip_int) and ip_parse(self.ip_int) == ip_parse(proxy_ip) + proxy_ip = hijack_endpoint(addr.proxy_ip, target_ip) + return self._is(proxy_ip, addr.proxy_port) return False def matches_any(self, srv: Service, addresses: list[Address]) -> bool: @@ -639,7 +671,7 @@ class FiregexTables(NFTableManager): raise Exception("the external transport needs the port your proxy listens on") # One host, not a network: a hand-off points at the single address the # operator's proxy is listening on, so any prefix a normaliser added is dropped. - proxy_ip = one_address(addr.proxy_ip or "") or ("::1" if family == "ip6" else "127.0.0.1") + proxy_ip = hijack_endpoint(addr.proxy_ip, ip) inbound = self._match(ip, port, family, l4, "daddr", "dport") + [ self.COUNTER, {"mangle": {"key": {"payload": {"protocol": l4, "field": "dport"}}, diff --git a/backend/routers/services.py b/backend/routers/services.py index 0af750f..3c61aa6 100644 --- a/backend/routers/services.py +++ b/backend/routers/services.py @@ -35,7 +35,7 @@ from modules.services import stats from modules.services.logs import log_for from modules.services.models import (KIND, L4, MODE, PROTO, STATUS, Service, TRANSPORT, UPSTREAM, upstream_refusal) -from modules.services.nftables import FiregexTables +from modules.services.nftables import FiregexTables, hijack_endpoint from modules.services import transports from modules.services.transports import PROXY_ENGINE, PYWORKER, UnsupportedChain from utils import ( @@ -788,6 +788,7 @@ def _insert_address(service_id: str, proto: str, transport: str, form: AddressFo edge = _address_edge(proto, form) target_port = _address_target(transport, form) upstream = _address_upstream(proto, form) + proxy_ip = _hijack_ip(transport, parsed_ip, form) address_id = gen_id() db.query( "INSERT INTO service_addresses (address_id, service_id, ip_int, port, proto, " @@ -801,12 +802,31 @@ def _insert_address(service_id: str, proto: str, transport: str, form: AddressFo edge, target_port, upstream, - form.proxy_ip or None, + proxy_ip, form.proxy_port, ) return address_id +def _hijack_ip(transport: str, service_ip: str, form: AddressForm) -> str | None: + """Where this address's traffic is handed off, stored rather than defaulted later. + + Only the hand-off layer has one; everywhere else it stays NULL, because there is no + endpoint and a value would be a setting that does nothing. + + Resolved **here**, at the point the row is written, so that the partial unique index + on `(proxy_ip, proxy_port)` can do its job. It could not before: the column was left + NULL whenever the operator did not type an address, SQLite counts every NULL as + distinct in a UNIQUE index, and the rules then defaulted all of them to the same + loopback address — so any number of addresses could sit behind one endpoint, which is + the single thing that index was added to forbid. The return rule tells them apart by + address and port, so it could not. + """ + if str(transport) != TRANSPORT.EXTERNAL or not form.proxy_port: + return form.proxy_ip or None + return hijack_endpoint(form.proxy_ip, service_ip) + + def _address_edge(proto: str, form: AddressForm) -> str: """What clients speak at one address of a service. @@ -904,8 +924,21 @@ def _address_upstream(proto: str, form: AddressForm) -> str: def _address_taken(e: sqlite3.IntegrityError) -> str: - """Say which uniqueness rule was broken, because they mean different things.""" - if "unique_hijack_target" in str(e): + """Say which uniqueness rule was broken, because they mean different things. + + **Told apart by the columns, not by the index name.** SQLite reports a violated + unique index as `UNIQUE constraint failed: ., ...` — it names the + index only for one on an expression — so keying on `unique_hijack_target` matched + nothing, and the sentence below has never once been shown. What an operator saw + instead was "one of these addresses is already protected", which points at the + service address while the thing actually colliding is their proxy's port: they go + looking for a service that does not exist. + + The index name is still accepted, because it costs nothing and is what a future + SQLite, or an index written as an expression, might report instead. + """ + said = str(e) + if "proxy_port" in said or "unique_hijack_target" in said: return ( "another address already hands its traffic to that proxy endpoint. Give " "this one its own port: the return rule recognises your proxy by address " @@ -1308,11 +1341,12 @@ async def edit_address(service_id: str, address_id: str, form: AddressForm): except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + proxy_ip = _hijack_ip(row["transport"], parsed_ip, form) manager = firewall.get(service_id) await manager.address_removed(address_id) try: write(parsed_ip, form.port, L4.l4_of(edge), edge, target_port, upstream, - form.proxy_ip or None, form.proxy_port) + proxy_ip, form.proxy_port) except sqlite3.IntegrityError as e: await restore() raise HTTPException(status_code=400, detail=_address_taken(e)) @@ -1820,6 +1854,39 @@ async def get_regexes(service_id: str, filter_id: str): return db.query("SELECT * FROM regexes WHERE filter_id = ?;", filter_id) +def _decoded_pattern(encoded: str) -> str: + """The pattern a request carries, or a 400 saying why it is not one. + + Two things the obvious `b64decode(...)` does not do, and both of them matter because + what gets through here is compiled into a live service's chain. + + **Strict decoding.** Python's decoder ignores characters outside the alphabet unless + it is asked not to, so `"!!!"` is not an error — it is `b""`. A malformed field + therefore arrived as an empty pattern rather than as a refusal. + + **And an empty pattern is refused.** hyperscan compiles it happily (the flags allow a + pattern that matches an empty buffer, which real patterns like `a*` need) and then it + matches every buffer it is shown — so a service whose chain holds one refuses every + connection it carries. There is no legitimate way to ask for that, an empty field in + the form is the only way to arrive at it, and the symptom is a CTF service that has + silently stopped answering. It is worth one sentence at the door. + """ + try: + # Whitespace out first, then strict: a base64 field that arrived wrapped across + # lines is still a perfectly good one, and `validate=True` would refuse it for a + # newline. What is being caught here is a field that is not base64 at all. + raw = base64.b64decode("".join(str(encoded).split()), validate=True) + except (binascii.Error, ValueError): + raise HTTPException(status_code=400, detail="The pattern must be base64-encoded") + if not raw: + raise HTTPException( + status_code=400, + detail="A pattern cannot be empty: an empty one matches every byte of every " + "connection, so the service would refuse all of its traffic.", + ) + return raw.decode(errors="replace") + + @app.post("/{service_id}/filters/{filter_id}/regexes", response_model=StatusMessageModel) async def add_regex(service_id: str, filter_id: str, form: RegexAddForm): row = _filter_or_404(service_id, filter_id) @@ -1827,10 +1894,7 @@ async def add_regex(service_id: str, filter_id: str, form: RegexAddForm): raise HTTPException(status_code=400, detail="This filter does not hold patterns") if form.mode not in MODE.ALL: raise HTTPException(status_code=400, detail=f"Unknown mode {form.mode!r}") - try: - pattern = base64.b64decode(form.regex).decode(errors="replace") - except (binascii.Error, ValueError): - raise HTTPException(status_code=400, detail="The pattern must be base64-encoded") + pattern = _decoded_pattern(form.regex) # Checked by the engine that will run it, not by a stand-in. A pattern accepted # here and refused at start time would leave the operator with a service that # will not come up and no idea which rule is at fault. @@ -1888,10 +1952,7 @@ async def edit_regex(service_id: str, filter_id: str, regex_id: str, form: Regex # Whatever the row will hold once this is applied, checked as a whole rather than # field by field: what the engine has to accept is the resulting pattern. - try: - pattern = base64.b64decode(fields.get("regex", was["regex"])).decode(errors="replace") - except (binascii.Error, ValueError): - raise HTTPException(status_code=400, detail="The pattern must be base64-encoded") + pattern = _decoded_pattern(fields.get("regex", was["regex"])) ok, why = check_pattern(pattern, fields.get("case_sensitive", was["case_sensitive"])) if not ok: raise HTTPException(status_code=400, detail=f"Invalid pattern: {why}") diff --git a/backend/utils/__init__.py b/backend/utils/__init__.py index 6b2ea07..edac6ca 100644 --- a/backend/utils/__init__.py +++ b/backend/utils/__init__.py @@ -105,6 +105,24 @@ API_VERSION = _get_version() PortType = Annotated[int, Field(gt=0, lt=65536)] +#: What a backup entry may be called: a plain basename of one of the two shapes +#: `export_db()` produces — `.db` for a database and `.py` for a filter's code. +#: The charset forbids path separators and `..`, so a crafted key cannot escape the +#: directory it is joined onto. +#: +#: Here rather than in `app.py`, beside `safe_join`, because the two are one defence in +#: two halves: this decides what a name may contain and that one decides where it may +#: land. Keeping them apart meant the charset could only be exercised by importing the +#: whole application — a server, a JWT library and everything else `app.py` pulls in — +#: to check two regular expressions. +#: +#: Matched with `fullmatch`, never `match`: in Python `$` also matches *before a trailing +#: newline*, so `"services.db\n"` satisfied a pattern written to mean "this and nothing +#: else". +SAFE_DB_NAME = re.compile(r'[A-Za-z0-9_-]+\.db') +SAFE_PY_NAME = re.compile(r'[A-Za-z0-9_-]+\.py') + + def safe_join(base_dir: Union[str, Path], *paths: str) -> Path: """ Safely join a base directory with one or more path components. @@ -196,9 +214,20 @@ def is_interface_name(name: str) -> bool: return bool(re.match(r'^[a-zA-Z0-9_.:-]+$', name)) and 0 < len(name) <= 15 def parse_ip_or_int(ip: str) -> str: - if is_ip_parse(ip): - return ip_parse(ip) + """Normalise what an operator typed into what the rules and the key will use. + + **Stripped once, before either question is asked.** `is_interface_name` strips and + `is_ip_parse` does not, so surrounding whitespace used to decide *which of the two an + address was*: `" 10.0.0.1 "` failed the IP test, passed the interface-name test — + every character in it is in that charset — and was stored verbatim. Nothing then + broke loudly, which is the problem: the rules still matched (both sides re-parse the + value), but `"10.0.0.1"` and `"10.0.0.1/32"` are different strings, so the + `(ip_int, port, proto)` uniqueness key no longer saw them as the same address and two + services could each believe they were protecting it. + """ ip_str = str(ip).strip() + if is_ip_parse(ip_str): + return ip_parse(ip_str) if is_interface_name(ip_str): return ip_str raise ValueError(f"'{ip}' is neither a valid IP address nor a valid interface name") diff --git a/fgex-lib/firegex/cli.py b/fgex-lib/firegex/cli.py index da337cc..824807b 100644 --- a/fgex-lib/firegex/cli.py +++ b/fgex-lib/firegex/cli.py @@ -153,10 +153,18 @@ def regex_test( is_input = not from_service blocked_by = ruleset.apply(sample, is_input) + # Said before the verdict, because it changes what the verdict means: "passed + # through" from a run that could not try every rule is not the same answer. + for rule_id, why in ruleset.unscannable.items(): + print(f"[bold yellow]not tried here[/] [bold]{escape(rule_id)}[/]: {escape(why)}. " + f"It is valid and will run on a real service — only matching it against a " + f"sample needs a mode hyperscan does not offer for it.") + if blocked_by is not None: print(f"[bold red]blocked[/] by [bold]{escape(blocked_by)}[/]") close_cli(0) - print("[bold green]passed through[/]") + print("[bold green]passed through[/]" + + (" by the rules that could be tried" if ruleset.unscannable else "")) @regex_app.command("proxy", help="Run a local proxy applying a ruleset, like `fgex pyfilters`") @@ -179,7 +187,15 @@ def regex_proxy( close_cli() print("[dim]Blocking matches per chunk here, but across the whole stream on a real " "service: a pattern split over two reads is caught there and not here.[/]") - run_regex_simulation(Ruleset(rules), address, port, from_address, from_port, ipv6) + ruleset = Ruleset(rules) + # Built before the first connection rather than on it, so the warning is on screen + # while the operator is still reading the banner instead of appearing mid-traffic. + for is_input in (True, False): + ruleset._db(is_input) + for rule_id, why in ruleset.unscannable.items(): + print(f"[bold yellow]not applied here[/] [bold]{escape(rule_id)}[/]: {escape(why)}. " + f"It is valid and will run on a real service.") + run_regex_simulation(ruleset, address, port, from_address, from_port, ipv6) def version_callback(verison: bool): diff --git a/fgex-lib/firegex/pyfilters/internals/__init__.py b/fgex-lib/firegex/pyfilters/internals/__init__.py index 9d25e90..96e6525 100644 --- a/fgex-lib/firegex/pyfilters/internals/__init__.py +++ b/fgex-lib/firegex/pyfilters/internals/__init__.py @@ -277,9 +277,16 @@ def compile(glob: dict) -> None: continue value = glob[name] if kind is int: - # A size is taken from anything that reads as one, and a nonsense size is - # ignored rather than refused: the file has already loaded. - value = int(value) + # A size is taken from anything that reads as one, and anything else is + # ignored — which is what the other two settings already did, and what this + # one only claimed to do: `int(value)` raises for a typo, so a file setting + # `FGEX_STREAM_MAX_SIZE = "1MB"` did not get the default, it failed to load + # altogether. The operator lost every filter in that file over an optional + # knob, and the message named neither the setting nor the file. + try: + value = int(value) + except (TypeError, ValueError): + continue if value <= 0: continue elif not isinstance(value, kind): diff --git a/fgex-lib/firegex/regex/__init__.py b/fgex-lib/firegex/regex/__init__.py index 8115e26..968eb29 100644 --- a/fgex-lib/firegex/regex/__init__.py +++ b/fgex-lib/firegex/regex/__init__.py @@ -234,16 +234,44 @@ class Database: def validate(rule: Rule) -> str | None: - """Would firegex accept this rule? Returns hyperscan's own message if not. + """Would firegex accept this rule? Returns the reason if not. Checked against the mode it will actually run in — stream matching, which is how a pattern follows a connection across chunk boundaries — because hyperscan does not accept quite the same patterns in every mode, and judging by the wrong one would refuse a rule that works. + + The empty pattern is the one refusal that is ours rather than hyperscan's. It + compiles, because the flags have to allow a pattern that *can* match an empty buffer + for real ones like `a*` to work — and then it matches every buffer it is shown, so a + service carrying it refuses all of its traffic. firegex refuses it when a rule is + saved; refusing it here too is what keeps this tool's answer the same as the + product's, which is the whole reason this module binds libhs instead of using `re`. """ - mode = HS_MODE_STREAM + if not rule.pattern: + return ("a pattern cannot be empty: an empty one matches every byte of every " + "connection, so the service would refuse all of its traffic") try: - Database([rule], mode) + Database([rule], HS_MODE_STREAM) + except ValueError as e: + return str(e) + return None + + +def scannable(rule: Rule) -> str | None: + """Can this rule be tried against a sample here, or only run on a real service? + + A separate question from [`validate`], and the difference is hyperscan's: matching a + stream and reporting *where* a match started are different modes, and block mode — + the only one that reports offsets — accepts slightly less. A pattern like + `a{1,1000}b` follows a stream perfectly well and cannot be block-scanned. + + So such a rule is valid, will run, and simply cannot be exercised locally. Saying so + is the honest answer; the datapath's own tester reports exactly the same thing, under + the name `unscannable`. + """ + try: + Database([rule], HS_MODE_BLOCK) except ValueError as e: return str(e) return None @@ -261,11 +289,32 @@ class Ruleset: def __init__(self, rules: list[Rule]): self.rules = list(rules) self._cache: dict[bool, Database | None] = {} + #: The rules this simulator had to leave out, and why — valid patterns that + #: cannot be block-scanned. Read it and say so: silently matching with fewer + #: rules than the operator wrote is the failure this whole module avoids. + self.unscannable: dict[str, str] = {} def _db(self, is_input: bool) -> Database | None: - if is_input not in self._cache: - selected = [r for r in self.rules if r.direction.covers(is_input)] + if is_input in self._cache: + return self._cache[is_input] + selected = [r for r in self.rules if r.direction.covers(is_input)] + try: self._cache[is_input] = Database(selected, HS_MODE_BLOCK) if selected else None + except ValueError: + # One rule that cannot be block-scanned refused the whole set, and the caller + # got a `ValueError` out of what looked like a clean ruleset — `fgex regex + # check` had already said it was valid, because validity is judged in stream + # mode and that is the mode it will run in. Compiled one at a time, the ones + # that can be tried here still are, and the rest are named rather than + # silently dropped or allowed to take the run down. + usable = [] + for rule in selected: + why = scannable(rule) + if why: + self.unscannable[rule.id] = why + else: + usable.append(rule) + self._cache[is_input] = Database(usable, HS_MODE_BLOCK) if usable else None return self._cache[is_input] def apply(self, data: bytes, is_input: bool) -> str | None: diff --git a/tests/integration/test_external.py b/tests/integration/test_external.py index 159b8f5..89be16e 100644 --- a/tests/integration/test_external.py +++ b/tests/integration/test_external.py @@ -74,3 +74,35 @@ def test_two_addresses_cannot_share_one_proxy_endpoint(api, protected, external_ service_id, external_layer.ip, free_port(external_layer.ipv6), proxy_ip=external_layer.ip, proxy_port=server.external.port) assert why is not None, "two addresses were allowed to share one proxy endpoint" + # The *reason*, not just a refusal. Asserting only that something was refused is what + # let this pass for as long as the sentence naming the endpoint was unreachable: the + # operator was told one of their addresses was taken, which points at the service + # address while the thing colliding is their proxy's port. + assert "proxy endpoint" in why, why + + +def test_an_endpoint_nobody_typed_is_still_an_endpoint(api, protected, external_layer): + """Two addresses cannot share a proxy endpoint, including when neither named one. + + `proxy_ip` may be left out — such a proxy is normally on loopback, and that is what + the rules used when nothing said otherwise. But the uniqueness rule is a partial + index on `(proxy_ip, proxy_port)` and SQLite counts every NULL as **distinct**, so + leaving it out used to mean any number of addresses could sit behind one endpoint: + stored apart, resolved to the same loopback port, and indistinguishable to the return + rule that has to put the original port back. + """ + from helpers.net import free_port + + service_id, _, _ = protected(external_layer, name="extnull") + shared = free_port(external_layer.ipv6) + + assert api.services_add_address( + service_id, external_layer.ip, free_port(external_layer.ipv6), + proxy_port=shared), "an address with an unstated proxy address was refused" + + why = api.services_add_address_error( + service_id, external_layer.ip, free_port(external_layer.ipv6), + proxy_port=shared) + assert why is not None, \ + "two addresses were allowed to share one endpoint by not naming it" + assert "proxy endpoint" in why, why diff --git a/tests/integration/test_nftables.py b/tests/integration/test_nftables.py index 440adba..a5e02b8 100644 --- a/tests/integration/test_nftables.py +++ b/tests/integration/test_nftables.py @@ -75,3 +75,46 @@ def test_everything_firegex_installs_is_named_for_it(api, protected, inspecting_ if line.strip().startswith("chain ")] assert chains, "the table has no chains at all" assert all(chain.split()[1].startswith("fgex_") for chain in chains), str(chains) + + +def test_stopping_a_hand_off_takes_back_its_return_rule_too(api, protected, + external_layer): + """The hand-off installs two rules on two different ports, and both have to go. + + The inbound one matches the service's address and port; the return one matches the + operator's proxy and **its** port, to put the original port back on the way out. + Only the first was ever recognised again — the port was compared against the + service's before the proxy's identity was considered — so every hand-off left its + return rule installed when it stopped, rewriting the source of anything leaving that + endpoint for a service that was no longer protected, and gained a second copy on the + next start. + """ + service_id, server, port = protected(external_layer, name="handoff-clean") + proxy_port = server.external.port + + start_and_settle(api, service_id) + during = [line for line in _ruleset().splitlines() if str(proxy_port) in line] + assert during, f"starting the hand-off installed no return rule for port {proxy_port}" + + assert api.services_stop(service_id) + time.sleep(0.8) + for label, number in (("the service", port), ("the proxy endpoint", proxy_port)): + left = [line for line in _ruleset().splitlines() if str(number) in line] + assert left == [], f"rules for {label} ({number}) leaked after the stop: {left}" + + +def test_a_hand_off_restarted_does_not_accumulate_rules(api, protected, external_layer): + """The other half of the same failure: a rule that is never taken back is installed + again on the next start, so the ruleset grows by one on every restart.""" + service_id, server, _ = protected(external_layer, name="handoff-twice") + proxy_port = server.external.port + + start_and_settle(api, service_id) + first = len([line for line in _ruleset().splitlines() if str(proxy_port) in line]) + assert first, "no return rule was installed at all" + + assert api.services_stop(service_id) + time.sleep(0.8) + start_and_settle(api, service_id) + second = len([line for line in _ruleset().splitlines() if str(proxy_port) in line]) + assert second == first, f"the ruleset grew from {first} to {second} rules on a restart" diff --git a/tests/unit/test_backup_guards.py b/tests/unit/test_backup_guards.py new file mode 100644 index 0000000..9ffd6a6 --- /dev/null +++ b/tests/unit/test_backup_guards.py @@ -0,0 +1,112 @@ +"""What a backup may carry, and what it may be named. + +`/api/import` takes a JSON object an operator uploaded and writes files out of it. Two +things stand between that and the filesystem — a charset for the key, and `safe_join` — +and one thing stands between a restore and this instance's own credentials, which is what +`dump()` refuses to export. + +All three have integration coverage, which means they are checked against a live instance +by someone who has one. They are pure functions; checking them here costs a millisecond +and covers the cases nobody would stage against a running firewall. +""" + +import re +import sqlite3 +from pathlib import Path + +import pytest +from fastapi import HTTPException + +from utils import SAFE_DB_NAME, SAFE_PY_NAME, safe_join +from utils.sqlite import SQLite + + +# --- what a backup entry may be called --------------------------------------- + + +@pytest.mark.parametrize("name", ["services.db", "firegex.db", "a.db", "A-b_9.db"]) +def test_the_shapes_export_db_produces_are_accepted(name): + assert SAFE_DB_NAME.fullmatch(name) + + +@pytest.mark.parametrize("name", [ + "../services.db", "db/services.db", "/etc/services.db", "..", ".db", + "services.DB", "services.db.py", "services db.db", "services.db\x00", +]) +def test_anything_that_is_not_a_plain_basename_is_refused(name): + assert not SAFE_DB_NAME.fullmatch(name) + + +def test_a_trailing_newline_does_not_get_past_the_guard(): + """`$` matches before a trailing newline, so `match` accepted `"services.db\\n"`. + + It was never a way out of the directory — `safe_join` contains the result whatever + the name — so this cost nothing. It is fixed because a pattern anchored to mean + "this and nothing else" should mean that: the next thing written against one of + these guards should not have to know about the exception. + """ + assert re.match(r'^[A-Za-z0-9_-]+\.db$', "services.db\n"), "the old spelling accepted it" + assert not SAFE_DB_NAME.fullmatch("services.db\n") + assert not SAFE_PY_NAME.fullmatch("abc123.py\n") + + +def test_the_two_shapes_do_not_accept_each_other(): + assert not SAFE_PY_NAME.fullmatch("services.db") + assert not SAFE_DB_NAME.fullmatch("abc123.py") + + +# --- and where it may be written --------------------------------------------- + + +def test_a_name_that_climbs_out_is_refused(tmp_path): + """Defence in depth on top of the charset: the regex is what is *meant* to catch + these, and this is what catches them if the regex is ever loosened.""" + for attempt in ("../escape.db", "a/../../escape.db", ".."): + with pytest.raises(HTTPException) as raised: + safe_join(tmp_path, attempt) + assert raised.value.status_code == 403 + + +def test_an_absolute_path_is_taken_as_a_relative_one(tmp_path): + """Not refused but *contained*, which is the same outcome by a different route.""" + landed = safe_join(tmp_path, "/etc/passwd") + assert landed.is_relative_to(Path(tmp_path).resolve()) + + +def test_an_ordinary_name_lands_where_it_should(tmp_path): + assert safe_join(tmp_path, "services.db") == Path(tmp_path).resolve() / "services.db" + + +# --- what a backup must never carry ------------------------------------------ + + +def test_a_backup_carries_neither_credentials_nor_the_auth_mode(tmp_path): + """The third one is the newest and the easiest to lose. + + `password` and `secret` are this instance's credentials. `auth_disabled` is not a + credential — it is *whether credentials are asked for*, which belongs to the + deployment and not to the configuration being restored. A backup able to carry it is + a backup able to turn a firewall's authentication off. + """ + db = SQLite(str(tmp_path / "firegex.db"), {}) + db.connect() + db.create_schema({}) + for key, value in (("password", "$pbkdf2$secret"), ("secret", "jwt-signing-key"), + ("auth_disabled", "1"), ("something_else", "kept")): + db.query("INSERT INTO keys_values (key, value) VALUES (?, ?);", key, value) + + carried = {row["key"] for row in db.dump()["keys_values"]} + assert carried == {"something_else"}, carried + db.disconnect() + + +def test_everything_else_in_a_table_is_carried(tmp_path): + """The exclusion is three named keys, not a table nobody exports.""" + db = SQLite(str(tmp_path / "other.db"), {}) + db.connect() + db.create_schema({"things": {"id": "VARCHAR(10) PRIMARY KEY", "v": "TEXT"}}) + db.query("INSERT INTO things (id, v) VALUES ('a', 'kept');") + + dumped = db.dump() + assert dumped["things"] == [{"id": "a", "v": "kept"}] + db.disconnect() diff --git a/tests/unit/test_chain_payloads.py b/tests/unit/test_chain_payloads.py new file mode 100644 index 0000000..ed8f98c --- /dev/null +++ b/tests/unit/test_chain_payloads.py @@ -0,0 +1,235 @@ +"""What the backend actually hands each datapath, and how a block comes back attributed. + +Two wire formats, two engines, one operator-visible promise: whichever layer a service is +on, a block names the rule that made it. Getting the encoding wrong is not a crash — it is +a pattern that never matches, or a block credited to nothing, and both of those look from +the outside like a filter that is simply switched off. + +None of this had a unit test: it is exercised only end to end, so a mistake here shows up +on a machine with a live instance, as traffic, rather than here. +""" + +import pytest + +from modules.services.models import KIND, PROTO, Filter, Regex, Service, TRANSPORT +from modules.services.transports import ChainLink, _QueueStage, announcement + + +def regex(regex_id: str, pattern: bytes, mode: str = "B", case_sensitive: bool = True, + active: bool = True) -> Regex: + return Regex(regex_id=regex_id, filter_id="f1", regex=pattern, mode=mode, + case_sensitive=case_sensitive, active=active) + + +def a_filter(kind: str = KIND.REGEX, active: bool = True, proto: str = PROTO.TCP) -> Filter: + return Filter(filter_id="f1", service_id="s", position=0, kind=kind, name="p", + active=active, proto=proto) + + +@pytest.fixture +def stage(): + """A stage that has never been started: only its payload building is being asked.""" + srv = Service(service_id="s", name="n", status="stop", proto="tcp", + transport=TRANSPORT.NFQUEUE) + return _QueueStage(srv, ChainLink(a_filter()), owner=None) + + +# --- what cppregex is told ---------------------------------------------------- +# ``, space separated. A pattern is bytes, and hex is the only +# encoding both ends agree on without quoting rules. + + +def test_a_pattern_is_sent_as_case_direction_and_hex(stage): + assert stage._regex_payload([regex("r1", b"FLAG", mode="C")]) == b"1C464c4147\n" + + +def test_case_insensitivity_is_the_leading_digit(stage): + assert stage._regex_payload( + [regex("r1", b"x", mode="C", case_sensitive=False)]) == b"0C78\n" + + +def test_a_pattern_matched_both_ways_is_sent_once_per_direction(stage): + """And **both codes attribute back to the one rule**. + + The engine reports whichever code matched; if the map held only one of them, a block + in the other direction would arrive with nothing to credit it to — which the operator + sees as a counter that never moves on a rule that is plainly working. + """ + payload = stage._regex_payload([regex("r1", b"FLAG", mode="B")]) + assert payload == b"1C464c4147 1S464c4147\n" + assert stage._codes == {"1C464c4147": "r1", "1S464c4147": "r1"} + + +def test_a_pattern_switched_off_is_not_sent(stage): + payload = stage._regex_payload([regex("r1", b"on", mode="C"), + regex("r2", b"off", mode="C", active=False)]) + assert b"6f6666" not in payload, "the disabled pattern was sent anyway" + assert "r2" not in stage._codes.values() + + +def test_an_empty_filter_is_still_a_valid_payload(stage): + """A service whose patterns are all off keeps its datapath: the operator turned the + rules off, not the service, and turning one back on must not need a restart.""" + assert stage._regex_payload([]) == b"\n" + + +def test_a_pattern_can_be_bytes_that_are_not_text(stage): + """Matching a raw byte sequence is a normal thing to want, which is why patterns are + stored base64 and sent as hex.""" + assert stage._regex_payload( + [regex("r1", bytes([0x00, 0xFF]), mode="C")]) == b"1C00ff\n" + + +# --- which of a file's functions run ------------------------------------------ +# Three states, distinct all the way down to the worker's argv: absent means every +# function the file defines, a list means exactly those, and an empty list means the +# operator switched them all off. Collapsing two of them either runs filters that were +# turned off or stops running filters nobody touched. + + +def test_a_filter_whose_functions_were_never_derived_runs_all_of_them(): + """No rows at all is a filter predating the per-function switches, or one whose code + did not parse when it was saved. Defaulting to "none" there would silently stop every + filter in the file.""" + assert ChainLink(a_filter(), functions=[]).enabled_functions is None + assert ChainLink(a_filter()).enabled_functions is None + + +def test_only_the_functions_left_switched_on_are_named(): + link = ChainLink(a_filter(), functions=[{"name": "a", "active": 1}, + {"name": "b", "active": 0}, + {"name": "c", "active": 1}]) + assert link.enabled_functions == ["a", "c"] + + +def test_every_function_switched_off_is_an_empty_list_not_an_absence(): + """The distinction the whole three-state design exists for.""" + link = ChainLink(a_filter(), functions=[{"name": "a", "active": 0}, + {"name": "b", "active": 0}]) + assert link.enabled_functions == [] + assert link.enabled_functions is not None + + +# --- what the proxy engine is told about an address --------------------------- + + +def _service(proto: str = "tcp") -> Service: + return Service(service_id="s", name="n", status="stop", proto=proto, + transport=TRANSPORT.PROXY) + + +def _address(**kw): + from modules.services.models import Address + return Address(**{"address_id": "a", "service_id": "s", "ip_int": "10.0.0.1/32", + "port": 443, "proto": "tcp", "edge": "tcp", **kw}) + + +def test_an_ordinary_address_is_not_announced_at_all(): + """The engine's answer for an address it has never heard of is the transparent case, + which is exactly what it would be told. An entry restating the default is one more + thing that can be wrong.""" + assert announcement(_service(), _address()) is None + + +def test_an_address_that_moved_its_service_is_announced(): + said = announcement(_service(), _address(target_port=80)) + assert (said.word, said.onward, said.target_port) == ("any", "same", 80) + + +def test_a_target_that_is_the_port_itself_is_not_a_move(): + assert announcement(_service(), _address(target_port=443)) is None + + +def test_only_an_http_service_declares_an_encrypted_address(): + """`tls` is the one edge with a name of its own, because it is the only one that is a + promise: a client opening that port in the clear is refused.""" + assert announcement(_service("http"), _address(edge="tls")).word == "tls" + assert announcement(_service("http"), _address(edge="tcp")) is None + assert announcement(_service("tls"), _address(edge="tls")) is None, \ + "a TLS service speaks TLS at every address; there is nothing to declare" + + +def test_what_the_service_behind_speaks_is_announced_on_its_own(): + """The one that was forgotten when this was two copies: it has no other way in.""" + said = announcement(_service("tls"), _address(edge="tls", upstream="tcp")) + assert (said.word, said.onward, said.target_port) == ("any", "plain", None) + + +def test_a_relayed_address_is_never_announced_this_way(): + """A UDP or QUIC address has a relay of its own, bound to the service it fronts, so + both questions were already answered when it was opened.""" + assert announcement(_service("http"), + _address(edge="quic", proto="udp", target_port=80)) is None + + +# --- what a service says about itself ----------------------------------------- +# These drive two decisions with visible consequences: whether a UDP relay is opened at +# all, and whether adding an address costs every existing connection a restart. Both are +# wrong in a way nobody sees — a needless rebuild drops connections that were fine, and a +# missed one leaves a listener that cannot accept what the rules now send it. + + +def test_a_service_with_no_addresses_yet_answers_from_its_protocol(): + """A check made at creation, before there is an address to look at, still means + something.""" + assert _service("tcp").carries("tcp") and not _service("tcp").carries("udp") + assert _service("quic").carries("udp") and not _service("quic").carries("tcp") + assert _service("http").carries("tcp") and _service("http").carries("udp"), \ + "http is the one protocol reached on both" + + +def test_which_addresses_need_a_relay_of_their_own(): + """Datagrams and QUIC; `SO_ORIGINAL_DST` is TCP-only, so there is nothing to recover + per datagram and a socket is bound per address instead.""" + srv = _service("http") + srv.addresses = [_address(port=443, proto="tcp", edge="tls"), + _address(port=443, proto="udp", edge="quic")] + assert [a.edge for a in srv.udp_addresses] == ["quic"] + + +def test_an_ipv6_quic_address_does_not_make_the_tcp_listener_ipv6(): + """The narrower question, and the one worth asking before rebuilding a service. + + The only listener that can have its family wrong is the TCP one: everything relayed + per address binds a socket in the family of the address when it arrives. On an `http` + service the two sit side by side, so an IPv6 HTTP/3 address must not cost every TCP + connection on it a restart it did not need. + """ + from modules.services.models import Address + + srv = _service("http") + srv.addresses = [ + Address(address_id="v4", service_id="s", ip_int="10.0.0.1/32", port=443, + proto="tcp", edge="tls"), + Address(address_id="v6", service_id="s", ip_int="fd00::1/128", port=443, + proto="udp", edge="quic"), + ] + assert srv.has_ipv6 is True, "it does answer on an IPv6 address" + assert srv.has_ipv6_tcp is False, "but not one the TCP listener has to accept" + + +def test_an_ipv6_tcp_address_does(): + from modules.services.models import Address + + srv = _service("tcp") + srv.addresses = [Address(address_id="v6", service_id="s", ip_int="fd00::1/128", + port=80, proto="tcp", edge="tcp")] + assert srv.has_ipv6_tcp is True + + +def test_an_interface_is_not_an_address_of_either_family(): + """It stands for whatever it carries, resolved when the rules are installed — so the + service cannot answer the family question from the name alone.""" + from modules.services.models import Address + + srv = _service("tcp") + srv.addresses = [Address(address_id="i", service_id="s", ip_int="eth0", port=80, + proto="tcp", edge="tcp")] + assert srv.addresses[0].is_interface is True + assert srv.has_ipv6 is False + + +def test_which_protocols_are_decrypted_by_the_engine(): + """Named for what it does rather than for TLS, because two protocols do it now.""" + assert [p for p in ("tcp", "udp", "tls", "quic", "http") if _service(p).decrypts] \ + == ["tls", "quic", "http"] diff --git a/tests/unit/test_nftables_addresses.py b/tests/unit/test_nftables_addresses.py index d7c097a..8a92ec3 100644 --- a/tests/unit/test_nftables_addresses.py +++ b/tests/unit/test_nftables_addresses.py @@ -90,8 +90,26 @@ def test_address_model_interface(): assert addr_iface.is_ipv6 is False + +def detached_table() -> FiregexTables: + """A `FiregexTables` that is **not** the one the rest of the process shares. + + `FiregexTables` is a `Singleton`, so `FiregexTables()` — and `FiregexTables.__new__` + with it — hands back the very instance `modules/services/firewall.py` holds. These + tests replace `cmd` and `list_rules` on what they are given, and assigning to the + shared object left those stand-ins in place for the rest of the run: every later test + that installed a rule wrote it into a list belonging to a test that had finished, and + saw nothing happen. Nothing did until `test_rule_round_trip.py` started installing + rules, which is a long time for a fixture to be quietly disabling the module. + + `object.__new__` skips the singleton entirely, which is what these want: an instance + whose methods can be called and whose attributes belong to one test. + """ + return object.__new__(FiregexTables) + + def test_nftables_match_generation(): - table = FiregexTables.__new__(FiregexTables) + table = detached_table() # IP match (inbound daddr) m_ip_in = table._match("127.0.0.1/32", 80, "ip", "tcp", "daddr", "dport") @@ -187,7 +205,7 @@ def test_installed_rule_parsing(): } # Simulate get() parsing logic - table = FiregexTables.__new__(FiregexTables) + table = detached_table() table.list_rules = lambda tables, chains: [raw_rule] rules = table.get() assert len(rules) == 1 @@ -225,7 +243,7 @@ def test_an_interfaces_output_rule_is_read_back_as_the_interfaces(): {"redirect": {"port": 38472}}, ], } - table = FiregexTables.__new__(FiregexTables) + table = detached_table() table.list_rules = lambda tables, chains: [raw_rule] rule = table.get()[0] assert rule.ip_int == "wg0", "the rule was read back under the address it matches" @@ -252,7 +270,7 @@ def test_proxy_rule_generation_interface_vs_ip(monkeypatch): nft, "get_interface_ips", lambda name: ["192.168.1.5", "fe80::1%eth0", "fd00::5"] if name == "eth0" else [], ) - table = FiregexTables.__new__(FiregexTables) + table = detached_table() commands = [] table.cmd = lambda *cmds: commands.extend(cmds) @@ -330,7 +348,7 @@ def test_proxy_rule_generation_interface_vs_ip(monkeypatch): def test_queue_rule_generation_interface(): - table = FiregexTables.__new__(FiregexTables) + table = detached_table() commands = [] table.cmd = lambda *cmds: commands.extend(cmds) @@ -410,3 +428,110 @@ def test_udp_relay_host_prefers_a_real_ipv4_address(): if not loopbacks: pytest.skip("no loopback interface to resolve") assert udp_relay_host(loopbacks[0]) == "127.0.0.1" + + +# --- taking the rules back again --------------------------------------------- +# Rules are found by what they match rather than by a handle anybody remembered, so a +# restart that lost its bookkeeping still cleans up after itself. What that costs is +# that `matches` has to recognise every shape the module installs — and the hand-off's +# two legs are not the same shape: the inbound rule matches the service, the outbound +# one matches the operator's proxy, on the proxy's own port. + + +def _external_service(port=80, proxy_port=8080, ip="10.0.0.1/32", proxy_ip="127.0.0.1"): + srv = Service(service_id="s", name="handoff", status="active", proto="tcp", + transport=TRANSPORT.EXTERNAL) + addr = Address(address_id="a", service_id="s", ip_int=ip, port=port, proto="tcp", + edge="tcp", proxy_ip=proxy_ip, proxy_port=proxy_port) + srv.addresses = [addr] + return srv, addr + + +def test_the_inbound_handoff_rule_is_recognised(): + srv, addr = _external_service() + rule = InstalledRule(chain=FiregexTables.hijack_in_chain, handle=1, proto="tcp", + port=80, ip_int="10.0.0.1/32") + assert rule.matches(srv, addr) is True + + +def test_the_outbound_handoff_rule_is_recognised_too(): + """The one that was invisible, and what it cost. + + The return rule matches the proxy's address **and the proxy's port**, and the port + was compared against the *service's* before either identity was considered — so this + only ever matched when the two happened to be the same number. Every other hand-off + left its outbound rule installed when the service stopped: still rewriting the source + of anything leaving that proxy endpoint, for a service that was no longer protected, + and joined by a second copy on the next start. + """ + srv, addr = _external_service(port=80, proxy_port=8080) + rule = InstalledRule(chain=FiregexTables.hijack_out_chain, handle=2, proto="tcp", + port=8080, ip_int="127.0.0.1/32") + assert rule.matches(srv, addr) is True + + +def test_the_outbound_rule_of_one_address_is_not_another_addresss(): + """Two hand-offs cannot share an endpoint, so one must never claim the other's rule.""" + srv, addr = _external_service(proxy_port=8080) + other = Address(address_id="b", service_id="s", ip_int="10.0.0.2/32", port=80, + proto="tcp", edge="tcp", proxy_ip="127.0.0.1", proxy_port=9090) + rule = InstalledRule(chain=FiregexTables.hijack_out_chain, handle=2, proto="tcp", + port=8080, ip_int="127.0.0.1/32") + assert rule.matches(srv, other) is False + + +def test_only_the_handoff_layer_recognises_a_proxy_endpoint(): + """A proxy-layer service has no endpoint, so the branch must not be reachable for it.""" + srv, addr = _external_service() + srv.transport = TRANSPORT.PROXY + rule = InstalledRule(chain=FiregexTables.hijack_out_chain, handle=2, proto="tcp", + port=8080, ip_int="127.0.0.1/32") + assert rule.matches(srv, addr) is False + + +def test_a_rule_on_the_wrong_transport_is_not_ours(): + srv, addr = _external_service() + rule = InstalledRule(chain=FiregexTables.hijack_in_chain, handle=1, proto="udp", + port=80, ip_int="10.0.0.1/32") + assert rule.matches(srv, addr) is False + + +# --- where a hand-off actually points ---------------------------------------- + + +def test_the_handoff_endpoint_defaults_to_loopback_in_the_right_family(): + assert nft.hijack_endpoint(None, "10.0.0.1/32") == "127.0.0.1" + assert nft.hijack_endpoint(None, "fd00::1/128") == "::1" + + +def test_the_handoff_endpoint_is_one_host_not_a_network(): + """A `mangle` writes a single address into the packet and refuses anything else.""" + assert nft.hijack_endpoint("192.168.1.5/32", "10.0.0.1/32") == "192.168.1.5" + + +# --- what an operator typed, normalised once --------------------------------- + + +def test_whitespace_cannot_turn_an_address_into_an_interface(): + """The two tests disagreed about stripping, and that decided which of the two it was. + + `is_interface_name` strips and `is_ip_parse` does not, so ` 10.0.0.1 ` failed the + address test, passed the interface test — every character in it is in that charset — + and was stored verbatim. Nothing broke loudly: the rules still matched, because both + sides re-parse the value. What broke quietly is the uniqueness key, which is a string + comparison: `10.0.0.1` and `10.0.0.1/32` are two different addresses to it, so two + services could each believe they were protecting that one. + """ + assert parse_ip_or_int(" 10.0.0.1 ") == parse_ip_or_int("10.0.0.1") == "10.0.0.1/32" + assert parse_ip_or_int(" fd00::1 ") == parse_ip_or_int("fd00::1") + + +def test_an_interface_name_survives_the_same_normalising(): + assert parse_ip_or_int(" eth0 ") == "eth0" + + +def test_something_that_is_neither_is_refused_by_name(): + with pytest.raises(ValueError, match="neither a valid IP address nor"): + parse_ip_or_int("") + with pytest.raises(ValueError, match="neither a valid IP address nor"): + parse_ip_or_int("not a name, and not an address") diff --git a/tests/unit/test_pyfilter_inference.py b/tests/unit/test_pyfilter_inference.py new file mode 100644 index 0000000..87b610d --- /dev/null +++ b/tests/unit/test_pyfilter_inference.py @@ -0,0 +1,212 @@ +"""Which protocol a filter file speaks, and the knobs it may set. + +Both are read **off the file**, and that is the mechanism the whole model rests on: the +library decides when to call a filter from what its parameters are annotated with, and +`infer_proto` reads the same table back the other way to say what the file is. Nothing +declares a protocol, so nothing can disagree with the code — which is what a `proto` +passed in from outside used to do, refusing an HTTP filter on a TCP service for asking +for an `HttpRequest`, the only thing an HTTP filter does. + +None of it had a unit test. It is exercised end to end by the integration suite, which +means a change here surfaces as a filter that silently never runs, on a machine with a +live instance, rather than here. +""" + +import pytest + +from firegex.pyfilters import collect_pyfilters, pyfilter +from firegex.pyfilters.internals import (compile as fgex_compile, get_code_proto, + get_filter_names, infer_proto, simplest_proto) +from firegex.pyfilters.internals.data import DataStreamCtx +from firegex.pyfilters.internals.models import ExceptionAction, FullStreamAction +from firegex.pyfilters.models import (HttpRequest, RawPacket, TCPClientStream, + TCPInputStream, TCPServerStream, TCPOutputStream, + type_annotations_associations) + +HEAD = ("from firegex.pyfilters import pyfilter, ACCEPT\n" + "from firegex.pyfilters.models import *\n") + + +def one(annotation: str, name: str = "a") -> str: + return f"@pyfilter\ndef {name}(x: {annotation}): return ACCEPT\n" + + +# --- what a file turns out to speak ------------------------------------------ + + +def test_asking_only_for_raw_payloads_makes_a_tcp_filter(): + """"Simplest" is fewest models: a file that only wants bytes is not an HTTP filter + that happens never to parse.""" + assert get_code_proto(HEAD + one("RawPacket")) == "tcp" + assert get_code_proto(HEAD + one("TCPInputStream")) == "tcp" + + +def test_asking_for_a_parsed_message_makes_an_http_filter(): + assert get_code_proto(HEAD + one("HttpRequest")) == "http" + assert get_code_proto(HEAD + one("GrpcMessage")) == "http", \ + "gRPC is a body format inside HTTP, not a protocol beside it" + + +def test_one_file_can_hold_both_kinds(): + """`http` provides everything `tcp` does, so a filter that parses HTTP and one that + reads the raw stream live together — and the file is an HTTP one.""" + code = HEAD + one("RawPacket", "raw") + one("HttpRequest", "parsed") + assert get_filter_names(code) == ["raw", "parsed"] + assert get_code_proto(code) == "http" + + +def test_a_file_with_no_filters_speaks_the_simplest_protocol(): + """It asks for nothing, so nothing is ruled out.""" + assert get_code_proto(HEAD) == simplest_proto() == "tcp" + + +def test_the_aliases_are_the_same_models(): + """`docs/pyfilter.md` documents both spellings, so both have to be annotatable. + + They are the same class object rather than a second pair, which is why they are in + the protocol table without being listed in it — and why annotating with one of them + is not "an annotation no protocol provides". + """ + assert TCPClientStream is TCPInputStream + assert TCPServerStream is TCPOutputStream + assert get_code_proto(HEAD + one("TCPClientStream")) == "tcp" + + +# --- what a file cannot say -------------------------------------------------- + + +def test_a_parameter_with_no_annotation_is_refused_naming_it(): + """The annotation is what decides when the filter is called, so a bare parameter + leaves the library with nothing to go on.""" + with pytest.raises(Exception, match="has no type annotation"): + get_filter_names(HEAD + "@pyfilter\ndef a(x): return ACCEPT\n") + + +def test_an_annotation_no_model_matches_is_refused_with_the_list(): + """Told what is available, rather than left to guess at a name.""" + with pytest.raises(Exception) as raised: + get_filter_names(HEAD + one("int")) + assert "no protocol provides it" in str(raised.value) + assert "HttpRequest" in str(raised.value), "the message has to name what there is" + + +def test_two_different_application_protocols_in_one_file_are_refused(monkeypatch): + """A connection is only ever one of them. + + Unreachable with the table as it stands — `http` provides everything `tcp` does, so + no pair of real models can conflict — which is exactly why it is pinned here rather + than left to be discovered broken the day a protocol is added that is not a superset. + The message has to name **both** functions: told only that a file is inconsistent, + the operator has to find the pair themselves. + """ + monkeypatch.setitem(type_annotations_associations, "tcp", + {RawPacket: RawPacket._fetch_packet, + TCPInputStream: TCPInputStream._fetch_packet}) + monkeypatch.setitem(type_annotations_associations, "http", + {RawPacket: RawPacket._fetch_packet, + HttpRequest: HttpRequest._fetch_packet}) + + with pytest.raises(Exception) as raised: + get_filter_names(HEAD + one("TCPInputStream", "streamy") + one("HttpRequest", "parsy")) + said = str(raised.value) + assert "streamy" in said and "parsy" in said, said + assert "one application protocol" in said, said + + +def test_the_shared_model_still_decides_nothing(monkeypatch): + """With the same split table, a file wanting only `RawPacket` is still ambiguous and + still resolves to the simplest answer rather than refusing.""" + monkeypatch.setitem(type_annotations_associations, "tcp", + {RawPacket: RawPacket._fetch_packet, + TCPInputStream: TCPInputStream._fetch_packet}) + monkeypatch.setitem(type_annotations_associations, "http", + {RawPacket: RawPacket._fetch_packet, + HttpRequest: HttpRequest._fetch_packet}) + assert get_code_proto(HEAD + one("RawPacket")) == "http", \ + "fewest models wins, and with this table that is the two-model one" + + +# --- the order the file defines them in -------------------------------------- + + +def test_definition_order_is_the_order_they_run_in(): + """The only order a reader of the file can predict. + + What this replaced was a *set* of names, so the order was whatever hashing produced + — which decided which function got credited with a block. + """ + code = HEAD + "".join(one("RawPacket", name) for name in ("third", "first", "second")) + assert get_filter_names(code) == ["third", "first", "second"] + + +def test_a_function_registered_twice_is_listed_once(): + """`collect_pyfilters` reads module globals, and a decorated function can be bound to + more than one name.""" + namespace = {} + exec(HEAD + one("RawPacket", "a") + "b = a\n", namespace, namespace) + assert collect_pyfilters(namespace) == ["a"] + + +def test_the_decorator_hands_the_function_back_unwrapped(): + """A wrapper would add a frame to every traceback the operator has to read.""" + def target(x: RawPacket): + return None + + assert pyfilter(target) is target + + +# --- the knobs a file may set ------------------------------------------------ + + +def settings_of(extra: str) -> DataStreamCtx: + code = HEAD + extra + one("RawPacket") + glob: dict = {} + exec(code, glob, glob) + glob["__firegex_pyfilter_enabled"] = ["a"] + fgex_compile(glob) + return DataStreamCtx(glob, init_pkt=False) + + +def test_the_documented_default_stream_cap_is_the_one_in_force(): + """It read `1*8e20` under a comment saying 1MB, so `FGEX_FULL_STREAM_ACTION` never + fired unless a size was set by hand — and the float it produced would have been + refused by the setter beside it.""" + assert settings_of("").stream_max_size == 1024 * 1024 + + +def test_a_size_is_taken_from_anything_that_reads_as_one(): + assert settings_of("FGEX_STREAM_MAX_SIZE = 4096\n").stream_max_size == 4096 + assert settings_of("FGEX_STREAM_MAX_SIZE = '8192'\n").stream_max_size == 8192 + + +@pytest.mark.parametrize("value", ["'1MB'", "None", "'big'", "[]"]) +def test_a_nonsense_size_is_ignored_rather_than_refused(value): + """It used to take the whole file down with it. + + `int(value)` raises for a typo, so a file carrying `FGEX_STREAM_MAX_SIZE = "1MB"` did + not fall back to the default — it failed to load, the operator lost every filter in + it, and the message named neither the setting nor the file. The other two settings + ignored a wrong value all along; this one only said it did. + """ + assert settings_of(f"FGEX_STREAM_MAX_SIZE = {value}\n").stream_max_size == 1024 * 1024 + + +def test_a_size_that_is_not_a_size_is_ignored_too(): + assert settings_of("FGEX_STREAM_MAX_SIZE = -5\n").stream_max_size == 1024 * 1024 + assert settings_of("FGEX_STREAM_MAX_SIZE = 0\n").stream_max_size == 1024 * 1024 + + +def test_the_other_two_knobs_want_their_own_enum(): + assert settings_of("").full_stream_action is FullStreamAction.FLUSH + assert settings_of("").invalid_encoding_action is ExceptionAction.REJECT + + chosen = settings_of("from firegex.pyfilters import FullStreamAction\n" + "FGEX_FULL_STREAM_ACTION = FullStreamAction.REJECT\n") + assert chosen.full_stream_action is FullStreamAction.REJECT + + +def test_a_knob_set_to_the_wrong_kind_of_thing_is_ignored(): + """`FullStreamAction.FLUSH` is not the string `"flush"`, and a file that says so + keeps the default rather than failing to load.""" + assert settings_of("FGEX_FULL_STREAM_ACTION = 'flush'\n" + ).full_stream_action is FullStreamAction.FLUSH diff --git a/tests/unit/test_regex_library.py b/tests/unit/test_regex_library.py index 1ba46a2..0a0ec96 100644 --- a/tests/unit/test_regex_library.py +++ b/tests/unit/test_regex_library.py @@ -92,3 +92,79 @@ def test_a_ruleset_asking_to_rewrite_is_refused_rather_than_quietly_blocking(): def test_an_explicit_block_action_is_still_accepted(): """Old ruleset files carry it, and it says exactly what happens.""" assert regex.load_rules([{"id": "r", "pattern": "x", "action": "block"}])[0].id == "r" + + +# --- the two answers a pattern can get, and why they are two ------------------ + + +def test_an_empty_pattern_is_refused(): + """It compiles, and then it blocks everything. + + The flags have to allow a pattern that *can* match an empty buffer, because real + ones like `a*` need it — so hyperscan accepts the empty pattern itself and then + matches every buffer it is shown. A service whose chain holds one refuses all of its + traffic, which on a CTF service reads as "firegex broke it". There is no legitimate + way to ask for that and exactly one way to arrive at it by accident, which is an + empty field in the form. + """ + why = regex.validate(regex.Rule(id="e", pattern="", direction=regex.Direction.BOTH, + case_sensitive=True)) + assert why is not None + assert "empty" in why + + +def test_a_real_pattern_is_still_accepted(): + assert regex.validate(regex.Rule(id="r", pattern="FLAG\\{[a-z]+\\}", + direction=regex.Direction.BOTH, + case_sensitive=True)) is None + + +#: Valid, will run on a real service, and cannot be block-scanned. The asymmetry is +#: hyperscan's: reporting *where* a match started is a different mode from following a +#: stream, and it accepts slightly less. +STREAM_ONLY = "a{1,1000}b" + + +def test_a_pattern_can_be_valid_and_still_not_testable_here(): + rule = regex.Rule(id="big", pattern=STREAM_ONLY, direction=regex.Direction.BOTH, + case_sensitive=True) + assert regex.validate(rule) is None, "it follows a stream perfectly well" + assert regex.scannable(rule) is not None, "and cannot be matched against a sample" + + +def test_one_untestable_pattern_does_not_take_the_whole_ruleset_down(): + """`check` said the ruleset was valid and `test` raised a ValueError out of ctypes. + + Both answers were right and they were about different modes, which is precisely the + kind of disagreement this module exists to prevent — so the ruleset now matches with + the rules it can and says which ones it had to leave out. + """ + rules = [ + regex.Rule(id="big", pattern=STREAM_ONLY, direction=regex.Direction.BOTH, + case_sensitive=True), + regex.Rule(id="easy", pattern="FLAG", direction=regex.Direction.BOTH, + case_sensitive=True), + ] + ruleset = regex.Ruleset(rules) + assert ruleset.apply(b"xx FLAG xx", True) == "easy" + assert ruleset.apply(b"nothing here", True) is None + assert list(ruleset.unscannable) == ["big"] + + +def test_a_ruleset_of_nothing_but_untestable_patterns_still_answers(): + ruleset = regex.Ruleset([ + regex.Rule(id="big", pattern=STREAM_ONLY, direction=regex.Direction.BOTH, + case_sensitive=True), + ]) + assert ruleset.apply(b"aaab", True) is None + assert list(ruleset.unscannable) == ["big"] + + +def test_an_ordinary_ruleset_leaves_nothing_out(): + """The reporting must stay empty for the normal case, or it means nothing.""" + ruleset = regex.Ruleset([ + regex.Rule(id="r", pattern="FLAG", direction=regex.Direction.BOTH, + case_sensitive=True), + ]) + assert ruleset.apply(b"FLAG", True) == "r" + assert ruleset.unscannable == {} diff --git a/tests/unit/test_rule_round_trip.py b/tests/unit/test_rule_round_trip.py new file mode 100644 index 0000000..5a056d6 --- /dev/null +++ b/tests/unit/test_rule_round_trip.py @@ -0,0 +1,217 @@ +"""Every rule this module writes has to be findable again by the code that removes it. + +Rules are found by **what they match** rather than by a handle anybody remembered, so a +restart that lost its bookkeeping still cleans up after itself. The price of that choice +is an invariant nothing else enforces: `add()` and `delete()` have to agree about what a +rule looks like, and they are written far apart and reason differently — one builds an +expression, the other reads one back. + +When they disagree the rule is simply never removed. It survives the service that +installed it, pointing traffic at a queue nobody reads or a port nobody is bound to, and +the next start adds another beside it. Nothing raises; the ruleset just grows. + +So this round-trips: install a service's rules against a stand-in nftables, hand what was +written back to the reader, and ask `delete()` for every one of them. It is the test that +would have caught the hand-off's return rule being invisible — the outbound leg matches +the operator's proxy on the proxy's port, and `matches()` compared the port against the +service's before considering which of the two identities it was looking at. +""" + +import pytest + +from modules.services.models import TRANSPORT, Address, L4, Service +from modules.services.nftables import FiregexTables + + +class FakeNft: + """The smallest thing `FiregexTables` can talk to. + + Rules are kept in the order they were installed, each with a handle, and `insert` puts + one at the front exactly as nftables does — which is what the queue chains rely on. + """ + + def __init__(self): + self.rules: list[dict] = [] + self._handle = 0 + + def json_cmd(self, payload): + for command in payload["nftables"]: + for verb in ("add", "insert"): + rule = command.get(verb, {}).get("rule") + if rule is None: + continue + self._handle += 1 + stored = {**rule, "handle": self._handle} + self.rules.insert(0 if verb == "insert" else len(self.rules), stored) + gone = command.get("delete", {}).get("rule") + if gone is not None: + self.rules = [r for r in self.rules if r["handle"] != gone["handle"]] + return 0, {"nftables": [{"rule": rule} for rule in self.rules]}, "" + + +@pytest.fixture +def tables(monkeypatch): + """A `FiregexTables` whose ruleset is the fake above. + + It is a `Singleton`, so the instance is shared with anything else that built one — + the stand-in is installed on the instance rather than replacing the class. + """ + handle = FiregexTables() + fake = FakeNft() + monkeypatch.setattr(handle, "nft", fake) + # Restored explicitly, because they are what a test elsewhere in this directory used + # to replace on this same shared object and leave replaced. Asking for the class's + # own implementations back makes these tests independent of what ran before them. + monkeypatch.setattr(handle, "cmd", FiregexTables.cmd.__get__(handle)) + monkeypatch.setattr(handle, "list_rules", FiregexTables.list_rules.__get__(handle)) + yield handle, fake + + +def service(transport: str, proto: str = "tcp", **address_fields) -> Service: + srv = Service(service_id="s", name="round-trip", status="active", proto=proto, + transport=transport) + srv.addresses = [Address(address_id="a", service_id="s", ip_int="10.0.0.1/32", + port=8080, proto=L4.l4_of(proto), edge=proto, + **address_fields)] + return srv + + +def installed_in(fake: FakeNft, table_chains: set[str] | None = None) -> list[dict]: + return [r for r in fake.rules + if table_chains is None or r["chain"] in table_chains] + + +# --- one layer at a time ------------------------------------------------------ + + +def test_the_handoffs_rules_are_all_taken_back(tables): + """Three rules on two different ports, and every one of them has to go. + + Inbound and output-hook rules match the service; the return rule matches the + operator's proxy on **its** port. Only the first two were ever recognised again. + """ + handle, fake = tables + srv = service(TRANSPORT.EXTERNAL, proxy_ip="127.0.0.1", proxy_port=9000) + + handle.add(srv) + assert fake.rules, "the hand-off installed no rules at all" + ports = {r["chain"] for r in fake.rules} + assert handle.hijack_out_chain in ports, "no return rule was written" + + handle.delete(srv) + assert fake.rules == [], \ + f"rules survived the service that installed them: {fake.rules}" + + +def test_the_handoff_does_not_accumulate_across_restarts(tables): + """The other half of the same failure: what is never removed is installed again.""" + handle, fake = tables + srv = service(TRANSPORT.EXTERNAL, proxy_ip="127.0.0.1", proxy_port=9000) + + handle.add(srv) + first = len(fake.rules) + handle.delete(srv) + handle.add(srv) + assert len(fake.rules) == first, \ + f"the ruleset grew from {first} to {len(fake.rules)} over one restart" + + +def test_the_nfqueue_layers_rules_are_all_taken_back(tables): + handle, fake = tables + srv = service(TRANSPORT.NFQUEUE) + + handle.add(srv, queue_nums=[1000, 1001]) + assert len(fake.rules) == 4, "two positions, one pair of rules each" + + handle.delete(srv) + assert fake.rules == [], str(fake.rules) + + +def test_the_proxy_layers_rules_are_all_taken_back(tables): + handle, fake = tables + srv = service(TRANSPORT.PROXY) + + handle.add(srv, proxy_port=40000) + assert fake.rules, "the proxy layer installed no rules" + + handle.delete(srv) + assert fake.rules == [], str(fake.rules) + + +def test_a_published_address_is_still_matched_by_where_it_is_dialled(tables): + """The rules match where the world knocks, never where the service turned out to be — + getting that backwards redirected the service's own port twice and left the published + one unprotected.""" + handle, fake = tables + srv = service(TRANSPORT.PROXY) + srv.addresses[0].target_port = 80 + + handle.add(srv, proxy_port=40000) + assert any("8080" in str(r["expr"]) for r in fake.rules), \ + "no rule matched the port the address is dialled on" + + handle.delete(srv) + assert fake.rules == [], str(fake.rules) + + +# --- one address of several --------------------------------------------------- + + +def test_removing_one_address_leaves_the_others_steered(tables): + """Passing a subset is how an address is taken off a running service.""" + handle, fake = tables + srv = service(TRANSPORT.PROXY) + second = Address(address_id="b", service_id="s", ip_int="10.0.0.2/32", port=9090, + proto="tcp", edge="tcp") + srv.addresses.append(second) + + handle.add(srv, proxy_port=40000) + both = len(fake.rules) + + handle.delete(srv, [second]) + assert 0 < len(fake.rules) < both, \ + "removing one address took the other's rules with it, or none at all" + assert all("10.0.0.2" not in str(r["expr"]) for r in fake.rules) + + handle.delete(srv) + assert fake.rules == [] + + +def test_an_address_already_steered_is_not_steered_twice(tables): + """`add` is called again whenever an address is added to a running service, so it has + to be idempotent for the ones already there.""" + handle, fake = tables + srv = service(TRANSPORT.PROXY) + + handle.add(srv, proxy_port=40000) + once = len(fake.rules) + handle.add(srv, proxy_port=40000) + assert len(fake.rules) == once, "the same address was steered twice" + + +# --- what an interface leaves behind ------------------------------------------ + + +def test_an_interface_rule_is_found_again_by_its_comment(tables): + """The output-hook rule an interface installs matches an **address**, so nothing in + it says whose it is. The comment does, and without reading it back every one of them + would outlive its service as a redirect to a port nobody is listening on.""" + from modules.services import nftables as nft + + handle, fake = tables + srv = service(TRANSPORT.PROXY) + srv.addresses = [Address(address_id="a", service_id="s", ip_int="fgexlo0", port=8080, + proto="tcp", edge="tcp")] + + # The interface carries one address, so the output hook writes one rule for it. + nft_addresses = nft.interface_addresses + try: + nft.interface_addresses = lambda ip: ["10.9.9.9"] if ip == "fgexlo0" else nft_addresses(ip) + handle.add(srv, proxy_port=40000) + comments = [r.get("comment") for r in fake.rules if r.get("comment")] + assert any(c == f"{nft.IFACE_COMMENT}fgexlo0" for c in comments), \ + f"the interface rule carries no comment naming it: {fake.rules}" + handle.delete(srv) + assert fake.rules == [], f"the interface's rules leaked: {fake.rules}" + finally: + nft.interface_addresses = nft_addresses diff --git a/tests/unit/test_service_validation.py b/tests/unit/test_service_validation.py new file mode 100644 index 0000000..56abc87 --- /dev/null +++ b/tests/unit/test_service_validation.py @@ -0,0 +1,261 @@ +"""What the API refuses before anything is started, and the reason it gives. + +These are the pure functions in `routers/services.py` — no instance, no kernel, no +database. They are worth pinning on their own because of *when* they run: each one is +the last chance to turn a combination that cannot work into a sentence, and the +alternative to every one of them is a row that is accepted happily and then either +refuses to start for ever or, worse, starts and quietly does nothing. + +The rule they share is that a setting which would be **read by nothing** is refused +rather than stored. An operator told their service is published, who then finds it +merely intercepted, has been lied to by a form. +""" + +import base64 + +import pytest +from fastapi import HTTPException + +from routers.services import (AddressForm, _address_edge, _address_target, + _address_upstream, _check_tls_material, _decoded_pattern, + _hijack_ip, describe_error, needs_a_stream) +from modules.services.models import L4, TRANSPORT, UPSTREAM + + +def form(**kw) -> AddressForm: + return AddressForm(**{"ip_int": "10.0.0.1", "port": 80, **kw}) + + +def refusal(fn, *args) -> str: + with pytest.raises(HTTPException) as raised: + fn(*args) + assert raised.value.status_code == 400 + return str(raised.value.detail) + + +# --- what is spoken at an address -------------------------------------------- + + +def test_only_an_http_service_is_asked_what_an_address_speaks(): + """Every other protocol has one answer for the whole service.""" + assert _address_edge(L4.TCP, form()) == L4.TCP + assert _address_edge(L4.TLS, form()) == L4.TLS + assert _address_edge(L4.QUIC, form()) == L4.QUIC + + +def test_an_edge_that_disagrees_with_its_service_is_refused(): + """A row that says one thing while its service says another would be intercepted on + one transport and served on the other.""" + assert "cannot be" in refusal(_address_edge, L4.TCP, form(edge="tls")) + + +def test_an_http_address_says_how_it_is_reached(): + assert _address_edge(L4.HTTP, form(edge="tls")) == L4.TLS + assert _address_edge(L4.HTTP, form(edge="quic")) == L4.QUIC + assert _address_edge(L4.HTTP, form()) == L4.TCP, "cleartext unless it says otherwise" + + +def test_an_http_address_cannot_be_reached_in_a_way_http_is_not(): + assert "not 'udp'" in refusal(_address_edge, L4.HTTP, form(edge="udp")) + + +# --- where the service actually is ------------------------------------------- + + +def test_publishing_needs_a_layer_that_dials(): + """`target_port` is a question for the layer, not for the protocol. + + The proxy terminates the connection and opens the one to the service, so it is free + to open it elsewhere. NFQUEUE hands the kernel a verdict on packets already on their + way; the hand-off gives the traffic to somebody else's proxy. On both, a stored port + would be read by nothing at all. + """ + assert _address_target(TRANSPORT.PROXY, form(target_port=8080)) == 8080 + assert "NFQUEUE" in refusal(_address_target, TRANSPORT.NFQUEUE, form(target_port=8080)) + assert "your own proxy" in refusal(_address_target, TRANSPORT.EXTERNAL, + form(target_port=8080)) + + +def test_not_publishing_is_allowed_on_every_layer(): + """Absent is the transparent case: the address *is* the service.""" + for transport in TRANSPORT.ALL: + assert _address_target(transport, form()) is None + + +def test_a_published_port_has_to_be_a_port(): + assert "Invalid port" in refusal(_address_target, TRANSPORT.PROXY, + form(target_port=70000)) + + +# --- what the service behind an address speaks ------------------------------- + + +def test_only_a_decrypted_service_has_an_upstream_leg(): + """Firegex terminates nothing elsewhere, so there is nothing to put back or leave off.""" + assert _address_upstream(L4.TLS, form(upstream=UPSTREAM.TCP)) == UPSTREAM.TCP + assert _address_upstream(L4.HTTP, form(upstream=UPSTREAM.TLS)) == UPSTREAM.TLS + assert "carried as it arrives" in refusal(_address_upstream, L4.TCP, + form(upstream=UPSTREAM.TCP)) + + +def test_the_default_upstream_is_allowed_everywhere(): + """`same` is what every address did before there was a choice.""" + for proto in L4.ALL: + assert _address_upstream(proto, form()) == UPSTREAM.SAME + assert _address_upstream(proto, form(upstream=UPSTREAM.SAME)) == UPSTREAM.SAME + + +def test_an_unknown_upstream_is_named(): + assert "Unknown upstream" in refusal(_address_upstream, L4.TLS, form(upstream="carrier-pigeon")) + + +# --- where the operator's own proxy is --------------------------------------- + + +def test_a_handoff_endpoint_is_resolved_when_the_row_is_written(): + """Left NULL, the partial unique index cannot forbid what it exists to forbid. + + SQLite counts every NULL as distinct in a UNIQUE index, so any number of addresses + could be stored with no endpoint — and the rules then defaulted all of them to the + same loopback port, which is exactly the collision that index was added for. The + return rule tells two hand-offs apart by address and port; it could not. + """ + assert _hijack_ip(TRANSPORT.EXTERNAL, "10.0.0.1/32", form(proxy_port=8080)) == "127.0.0.1" + assert _hijack_ip(TRANSPORT.EXTERNAL, "fd00::1/128", form(proxy_port=8080)) == "::1" + + +def test_a_stated_endpoint_is_kept(): + assert _hijack_ip(TRANSPORT.EXTERNAL, "10.0.0.1/32", + form(proxy_ip="192.168.1.5", proxy_port=8080)) == "192.168.1.5" + + +def test_nothing_else_has_an_endpoint(): + """A value on a layer that hands nothing off would be a setting that does nothing.""" + assert _hijack_ip(TRANSPORT.PROXY, "10.0.0.1/32", form()) is None + assert _hijack_ip(TRANSPORT.NFQUEUE, "10.0.0.1/32", form()) is None + + +# --- the TLS material --------------------------------------------------------- + + +def test_pem_material_passes_through_untouched(): + """Only the envelope is judged. A second opinion on what the engine accepts is the + one that would be wrong.""" + _check_tls_material("-----BEGIN CERTIFICATE-----\nx\n-----END CERTIFICATE-----", + "-----BEGIN PRIVATE KEY-----\ny\n-----END PRIVATE KEY-----") + _check_tls_material(None, None) + _check_tls_material("", "") + + +def test_the_two_fields_the_other_way_round_are_named_as_such(): + """The mistake is made by hand and used to cost a restart to discover.""" + swapped = refusal(_check_tls_material, + "-----BEGIN PRIVATE KEY-----\ny\n-----END PRIVATE KEY-----", + "-----BEGIN CERTIFICATE-----\nx\n-----END CERTIFICATE-----") + assert "other way round" in swapped + + +def test_something_that_is_not_pem_is_refused(): + assert "PEM certificate" in refusal(_check_tls_material, "\x00\x01binary", None) + + +def test_a_passphrase_protected_key_says_how_to_decrypt_it(): + why = refusal(_check_tls_material, None, + "-----BEGIN ENCRYPTED PRIVATE KEY-----\nz\n-----END ENCRYPTED PRIVATE KEY-----") + assert "openssl pkey" in why + + +# --- the pattern a request carries ------------------------------------------- + + +def test_a_pattern_is_decoded_strictly(): + """Python's decoder ignores what is not in the alphabet unless told not to, so a + malformed field arrived as an empty pattern instead of as a refusal.""" + assert _decoded_pattern(base64.b64encode(b"FLAG").decode()) == "FLAG" + assert "base64" in refusal(_decoded_pattern, "!!!not base64!!!") + + +def test_an_empty_pattern_is_refused_at_the_door(): + """It compiles, and then it matches every byte of every connection. + + Which makes the service refuse all of its traffic — and the only way to arrive at it + is an empty field, so nothing is being taken away from anybody. + """ + for encoded in ("", base64.b64encode(b"").decode()): + assert "every byte" in refusal(_decoded_pattern, encoded) + + +def test_a_pattern_is_bytes_and_may_be_any_of_them(): + assert _decoded_pattern(base64.b64encode(b"\xc3\xa8FLAG").decode()).endswith("FLAG") + + +# --- models a datagram cannot carry ------------------------------------------ + + +def test_only_rawpacket_survives_a_datagram(): + """Every other model reaches for something only a stream has, and answers + `NotReadyToRun` on a datagram — so the filter would sit in the chain and never be + called, which is the failure the whole module is arranged to prevent.""" + assert needs_a_stream(["RawPacket"]) == [] + assert needs_a_stream(["HttpRequest", "RawPacket"]) == ["HttpRequest"] + assert needs_a_stream(["TCPInputStream", "HttpResponse"]) == ["HttpResponse", + "TCPInputStream"] + + +def test_an_error_is_described_with_its_line_when_it_has_one(): + assert describe_error({"type": "SyntaxError", "line": 7, "message": "bad"}) == \ + "SyntaxError (line 7): bad" + assert describe_error({"type": "ImportError", "line": 0, "message": "no"}) == \ + "ImportError: no" + + +def test_base64_wrapped_across_lines_is_still_base64(): + """Strict decoding must catch a field that is not base64, not one that is wrapped.""" + encoded = base64.b64encode(b"FLAG{" + b"x" * 80 + b"}").decode() + wrapped = "\n".join(encoded[i:i + 40] for i in range(0, len(encoded), 40)) + assert _decoded_pattern(wrapped) == _decoded_pattern(encoded) + + +# --- which uniqueness rule was broken ---------------------------------------- + + +def test_a_collision_on_the_proxy_endpoint_says_so(): + """Told apart by the columns, because that is what SQLite actually reports. + + It names the columns of the violated index — `UNIQUE constraint failed: + service_addresses.proxy_ip, service_addresses.proxy_port` — and names the index only + for one built on an expression. Keying on the index name therefore matched nothing, + and this sentence had never been shown: an operator whose two hand-offs collided was + told "one of these addresses is already protected", which points at the service + address while the thing colliding is their proxy's port, and sent them looking for a + service that does not exist. + """ + import sqlite3 + + from routers.services import _address_taken + + said = _address_taken(sqlite3.IntegrityError( + "UNIQUE constraint failed: service_addresses.proxy_ip, " + "service_addresses.proxy_port")) + assert "proxy endpoint" in said + + +def test_a_collision_on_the_address_itself_says_that_instead(): + import sqlite3 + + from routers.services import _address_taken + + said = _address_taken(sqlite3.IntegrityError( + "UNIQUE constraint failed: service_addresses.ip_int, service_addresses.port, " + "service_addresses.proto")) + assert "already protected by a service" in said + assert "proxy endpoint" not in said + + +def test_the_index_name_is_still_understood_if_it_is_ever_reported(): + import sqlite3 + + from routers.services import _address_taken + + assert "proxy endpoint" in _address_taken(sqlite3.IntegrityError( + "UNIQUE constraint failed: index 'unique_hijack_target'"))