diff --git a/AGENTS.md b/AGENTS.md index ae32137..f7583ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -442,7 +442,7 @@ Frontend note: a 401 used to call `window.location.reload()`. A token left over Independently of that, `IPFilterMiddleware` restricts access by CIDR (`--allowed-ips`, optionally reading the client IP from `--proxy-ip-header`). It **fails closed**: a missing, unparseable, or non-matching IP is denied. Note the header variant trusts a client-supplied value, so it only holds if the proxy overwrites it. ### SQLite wrapper (`backend/utils/sqlite.py`) -Each DB file's schema is versioned by hashing the Python schema dict; on mismatch the **entire file is deleted and recreated** (not migrated) — any schema dict change wipes that DB file on next startup, including `db/firegex.db`'s `keys_values` table (password/JWT secret). `dump()`/`load()` back the export/import backup feature; `dump()` deliberately excludes `password`, `secret`, **`auth_disabled` and `auth_disabled_host`** from exported `keys_values` rows, so `/api/import` explicitly re-preserves all four around the import. The last two are the easiest to lose: they are not credentials, they are *whether credentials are asked for*, which belongs to the deployment and not to the configuration being restored — a backup able to carry them is a backup able to turn a firewall's authentication off. The import restarts the application state with `startup_main(seed_auth=False)`: re-deciding authentication from the environment is what a boot does, and doing it during an import opened a window — the length of every service's start — in which a container booted without authentication answered everybody as an administrator before the preserved value was written back. `tests/integration/test_backup.py` turns authentication off *around* the import on purpose — asserting it is still on afterwards passes either way on a container whose environment agrees, and the bug only shows where the running state and the boot-time environment differ. +Each DB file's schema is versioned by hashing the Python schema dict; on mismatch the **entire file is deleted and recreated** (not migrated) — any schema dict change wipes that DB file on next startup, including `db/firegex.db`'s `keys_values` table (password/JWT secret). `dump()`/`load()` back the export/import backup feature; `dump()` deliberately excludes `password`, `secret`, **`auth_disabled` and `auth_disabled_host`** from exported `keys_values` rows, so `/api/import` explicitly re-preserves all four around the import. The last two are the easiest to lose: they are not credentials, they are *whether credentials are asked for*, which belongs to the deployment and not to the configuration being restored — a backup able to carry them is a backup able to turn a firewall's authentication off. The import restarts the application state with `startup_main(seed_auth=False)`: re-deciding authentication from the environment is what a boot does, and doing it during an import opened a window — the length of every service's start — in which a container booted without authentication answered everybody as an administrator before the preserved value was written back. `tests/integration/test_backup.py` turns authentication off *around* the import on purpose — asserting it is still on afterwards passes either way on a container whose environment agrees, and the bug only shows where the running state and the boot-time environment differ. **An import is all or nothing.** Every database is first loaded with `load(dry_run=True)`, which does the writes and rolls them back, and only when all of them would load is any loaded for real; the preserved keys are written back in a `finally`. They used to be loaded one after another, so a refusal from one left the ones before it replaced and the rest of the import never ran — with `firegex.db` first, as an export lists it, the instance came back with no password and no secret, asking the next visitor to choose one. What refused was the firewall's own `table` column, which `load` wrote unquoted: no backup holding a single firewall rule could be imported. `load` quotes every identifier now (they come from `PRAGMA table_info`, never from the backup). **And the filter files follow the database**: `_prune_orphan_code` runs at every services start — which is what an import ends with — and removes each `.py` no Python filter refers to. An import used to write the backup's files and leave every other one, so the code of each filter created since the backup stayed on disk for a filter that no longer existed. `tests/integration/test_backup.py` restores a deleted service — chain, patterns, code, switched-off functions, certificate — and checks it is running and filtering, restores the firewall's rules and policy, and imports a backup that cannot load to check nothing changed. ### Real-time frontend updates One Socket.IO event, `"update"`, whose payload is a list of strings treated as a react-query `queryKey` prefix (e.g. `["services"]`, `["firewall"]`). `App.tsx` does `queryClient.invalidateQueries({ queryKey: data })` — prefix matching invalidates any query whose key starts with that array. Convention: each module's mutating endpoints emit the same tag string that module's frontend queries use as the first `queryKey` element. `staleTime: Infinity` is set globally, so this event is the **only** automatic refresh mechanism — a missing/mismatched tag means silent staleness for other clients/tabs. diff --git a/backend/app.py b/backend/app.py index 25510f6..36defdc 100644 --- a/backend/app.py +++ b/backend/app.py @@ -481,21 +481,41 @@ async def import_db(data: dict): # them), so preserve this instance's own values instead of losing them on import. kept = {key: db.get(key) for key in ("password", "secret", "auth_disabled", AUTH_HOST_KEY)} - for db_path, db_data in db_imports: - temp_db = SQLite(str(db_path)) - # Load into the existing schema; SQLite.load only writes to tables/columns - # that actually exist, so unknown fields in the backup are ignored. - temp_db.load(db_data) + # Every database is loaded once for nothing first, and only if all of them would load + # is any of them loaded for real. They used to go one after another: a refusal from + # one left the ones before it replaced and the rest of the import never ran — so a + # backup listing `firegex.db` first wiped its `keys_values` and never put this + # instance's password and secret back, leaving it asking the next visitor to choose a + # password. Load into the existing schema: SQLite.load only writes to tables and + # columns that actually exist, so unknown fields in the backup are ignored. + try: + for dry_run in (True, False): + for db_path, db_data in db_imports: + temp_db = SQLite(str(db_path)) + try: + temp_db.load(db_data, dry_run=dry_run) + except Exception as e: + if dry_run: + raise HTTPException( + status_code=400, + detail=f"Invalid backup: '{os.path.basename(db_path)}' cannot be " + f"loaded, so nothing was imported: {e}", + ) + raise + finally: + temp_db.disconnect() - if filter_imports: - os.makedirs('db/service_filters', exist_ok=True) - for filter_path, decoded in filter_imports: - with open(filter_path, 'wb') as script_file: - script_file.write(decoded) - - for key, value in kept.items(): - if value is not None: - db.put(key, value) + if filter_imports: + os.makedirs('db/service_filters', exist_ok=True) + for filter_path, decoded in filter_imports: + with open(filter_path, 'wb') as script_file: + script_file.write(decoded) + finally: + # Whatever happened above: these are this instance's, and an import that stopped + # half way must not be the end of them. + for key, value in kept.items(): + if value is not None and db.get(key) != value: + db.put(key, value) # Restart the application state, without re-deciding authentication: nothing about the # deployment changed, and deciding again from the environment is what a boot does. diff --git a/backend/routers/services.py b/backend/routers/services.py index b752c46..da3c926 100644 --- a/backend/routers/services.py +++ b/backend/routers/services.py @@ -536,6 +536,7 @@ async def refresh_frontend(extra: list[str] | None = None): async def startup(): db.init() + _prune_orphan_code() try: await firewall.init() except Exception as e: @@ -548,6 +549,32 @@ async def startup(): _note_filtering(row["service_id"]) +def _prune_orphan_code() -> None: + """Remove the filter files no filter refers to. + + The code lives beside the database rather than in it, so the two can drift apart, and + an import is where they did: it replaced the database and wrote the backup's files, + but left every file the backup did not have — the code of every Python filter created + since, kept on disk for filters that no longer existed. Done at every start, which is + also what an import ends with, so the directory holds exactly what the database uses. + Only `.py` files are touched; anything else in there is not ours to judge. + """ + if not os.path.isdir(CODE_DIR): + return + used = { + row["filter_id"] + for row in db.query("SELECT filter_id FROM filters WHERE kind = ?;", KIND.PYFILTER) + } + for name in os.listdir(CODE_DIR): + stem, ext = os.path.splitext(name) + if ext != ".py" or stem in used: + continue + try: + os.remove(os.path.join(CODE_DIR, name)) + except OSError: + traceback.print_exc() + + async def shutdown(): await firewall.close() # The capture interface outlives any one service but not the process: this is the diff --git a/backend/utils/sqlite.py b/backend/utils/sqlite.py index a07ff12..85f5122 100644 --- a/backend/utils/sqlite.py +++ b/backend/utils/sqlite.py @@ -144,7 +144,19 @@ class SQLite(): res[tname] = self.query(f"SELECT * FROM {tname};") return res - def load(self, data: dict): + def load(self, data: dict, dry_run: bool = False): + """Replace the tables named in `data` with its rows, all in one transaction. + + `dry_run` does every write and then rolls it back: what an import uses to find out + whether each of its databases *would* load before it changes any of them. Loaded + one after another for real, a refusal from the third left the first two replaced — + `keys_values` among them, which is where the password and the signing secret live. + + Every identifier is quoted. Columns are taken from the table itself rather than + from the backup, so quoting cannot let anything in, and leaving them bare made a + column called `table` — the firewall's — a syntax error: no backup holding a + single firewall rule could be imported at all. + """ self.connect() cur = self.conn.cursor() try: @@ -157,7 +169,7 @@ class SQLite(): # Ignore anything that isn't a proper list of row objects if not isinstance(rows, list) or not all(isinstance(r, dict) for r in rows): continue - cur.execute(f"DELETE FROM {table_name};") + cur.execute(f'DELETE FROM "{table_name}";') if rows: # Column names are identifiers and can't be parameterized, so only # keep the ones that actually exist on the table - this stops a @@ -168,11 +180,11 @@ class SQLite(): if not cols: continue placeholders = ", ".join(["?"] * len(cols)) - col_names = ", ".join(cols) - query = f"INSERT INTO {table_name} ({col_names}) VALUES ({placeholders})" + col_names = ", ".join(f'"{c}"' for c in cols) + query = f'INSERT INTO "{table_name}" ({col_names}) VALUES ({placeholders})' for row in rows: cur.execute(query, [row.get(c) for c in cols]) - cur.execute("COMMIT") + cur.execute("ROLLBACK" if dry_run else "COMMIT") except Exception as e: cur.execute("ROLLBACK") raise e diff --git a/tests/helpers/firegexapi.py b/tests/helpers/firegexapi.py index ad618e4..8af8088 100644 --- a/tests/helpers/firegexapi.py +++ b/tests/helpers/firegexapi.py @@ -111,6 +111,16 @@ class FiregexAPI: req = self.s.post(f"{self.address}api/import", json=backup) return verify(req) + # --- Firewall rules ---------------------------------------------------------- + def firewall_rules(self): + """`{policy, rules, enabled}`, as the firewall page reads them.""" + return self.s.get(f"{self.address}api/firewall/rules").json() + + def firewall_set_rules(self, rules: list, policy: str): + req = self.s.post(f"{self.address}api/firewall/rules", + json={"rules": rules, "policy": policy}) + return verify(req) + # --- Services: the network layer ------------------------------------------- def services_list(self): req = self.s.get(f"{self.address}api/services") diff --git a/tests/integration/test_backup.py b/tests/integration/test_backup.py index f8e64cd..929bf7a 100644 --- a/tests/integration/test_backup.py +++ b/tests/integration/test_backup.py @@ -1,14 +1,20 @@ -"""Export and import, and the two things a backup must never carry. +"""Export and import: what a backup brings back, and the two things it must never carry. -A backup is configuration. The password is not configuration — importing one must not +A backup is configuration, and restoring it has to bring that configuration back as it +was — services, addresses, chains, patterns, the Python files beside the database, the +firewall's rules — not merely be accepted. The password is not configuration — importing one must not lock the current session out — and neither is *whether a password is asked for*, which belongs to where firegex is deployed. A backup able to carry that second one is a backup able to turn a firewall's authentication off. """ +import time + import pytest +from integration.conftest import SETTLE, add_python_filter, add_regex_filter, start_and_settle from helpers.firegexapi import FiregexAPI +from helpers.traffic import Channel pytestmark = pytest.mark.instance @@ -51,3 +57,173 @@ def test_whether_authentication_is_asked_for_survives_an_import(api, auth_enable # the suite expects. assert api.set_auth_mode(False) assert api.status()["auth_disabled"] is False + + +# --- what a backup brings back ------------------------------------------------------ +# +# Everything above checks what an import must *not* touch. These check what it is for: +# that a configuration exported, then lost, comes back as it was — not only accepted. + + +#: Two functions in one file, so that a function switched off can be told apart from one +#: that was never there: the switch lives in the database, the code on disk, and a backup +#: has to bring both back for the filter to mean what it meant. +TWO_FUNCTIONS = """from firegex.pyfilters import pyfilter, ACCEPT, REJECT +from firegex.pyfilters.models import RawPacket + + +@pyfilter +def refuse_marker(packet: RawPacket): + return REJECT if b"PYBLOCK" in packet.data else ACCEPT + + +@pyfilter +def switched_off(packet: RawPacket): + return REJECT if b"OFFBLOCK" in packet.data else ACCEPT +""" + + +def _snapshot(api, service_id: str) -> dict: + """Everything the API says about a service and its chain, code included.""" + service = dict(api.services_get(service_id)) + # The live log is the process's, not the configuration's: a deleted service's goes + # with it, and a restored one starts a new one. + service.pop("problem", None) + filters = api.services_filters(service_id) + chain = [] + for flt in filters: + entry = {"filter": flt} + if flt["kind"] == "regex": + entry["regexes"] = sorted(api.services_regexes(service_id, flt["filter_id"]), + key=lambda r: r["regex_id"]) + else: + entry["functions"] = api.services_functions(service_id, flt["filter_id"]) + entry["code"] = api.services_get_code(service_id, flt["filter_id"]) + chain.append(entry) + return {"service": service, "addresses": api.services_addresses(service_id), + "chain": chain} + + +def _filters_as_configured(channel) -> None: + assert channel.gets_through(b"harmless"), "the service is not answering" + assert channel.is_blocked(b"carrying RESTORE_RX"), "the pattern is not blocking" + assert channel.is_blocked(b"carrying PYBLOCK"), "the Python filter is not blocking" + assert channel.gets_through(b"carrying OFFBLOCK"), \ + "a function switched off is running" + + +def test_a_deleted_service_comes_back_whole_and_filtering(api, protected, filtering_layer): + """Export, lose the service, import: it has to be the same service again. + + Same id, same addresses, same chain in the same order, the same patterns and code, + the same functions switched off, the certificate a TLS service needs to start — and + running and filtering, because it was when the backup was taken. The code is the half + most easily lost: it is a file beside the database, deleted with the service, and + nothing but the backup's own entry for it brings it back. + """ + service_id, server, port = protected(filtering_layer, name="restore") + add_regex_filter(api, service_id, "RESTORE_RX", name="patterns") + filter_id = add_python_filter(api, service_id, TWO_FUNCTIONS, name="python") + assert api.services_edit_function(service_id, filter_id, "switched_off", False) + start_and_settle(api, service_id) + channel = Channel(server, port, filtering_layer.ipv6, filtering_layer.tls) + _filters_as_configured(channel) + + # Read through the services API first, which writes the counters still waiting: what + # is compared afterwards includes them. + before = _snapshot(api, service_id) + backup = api.export_backup() + + assert api.services_delete(service_id) + assert service_id not in [s["service_id"] for s in api.services_list()] + + assert api.import_backup(backup), "the backup was not imported" + time.sleep(SETTLE) + + after = _snapshot(api, service_id) + assert after["service"]["status"] == "active", \ + "a service running when the backup was taken did not come back running" + assert after == before, "the restored service is not the one that was exported" + _filters_as_configured(channel) + + +def test_firewall_rules_and_policy_come_back(api): + """The firewall's rules and policy live in a database of their own, beside the + services', and travel in the same backup.""" + was = api.firewall_rules() + if was.get("enabled"): + pytest.skip("the firewall is enabled on this instance, and rewriting its rules " + "here would change what the host accepts") + rules = [ + {"active": True, "name": "ssh from the lab", "proto": "tcp", "table": "filter", + "src": "10.0.0.0/8", "dst": "", "port_src_from": 1, "port_src_to": 65535, + "port_dst_from": 22, "port_dst_to": 22, "action": "accept", "mode": "in"}, + {"active": False, "name": "dns out", "proto": "udp", "table": "filter", + "src": "", "dst": "eth0", "port_src_from": 1, "port_src_to": 65535, + "port_dst_from": 53, "port_dst_to": 53, "action": "drop", "mode": "out"}, + ] + try: + assert api.firewall_set_rules(rules, "accept") + configured = api.firewall_rules() + backup = api.export_backup() + + assert api.firewall_set_rules([], "reject") + assert api.firewall_rules()["rules"] == [] + + assert api.import_backup(backup), "the backup was not imported" + restored = api.firewall_rules() + assert restored == configured, "the firewall came back different from the backup" + assert restored["enabled"] is False + finally: + api.firewall_set_rules(was["rules"], was["policy"]) + + +def test_a_backup_that_cannot_be_loaded_changes_nothing(api, fg_address, fg_password, + auth_enabled): + """Every database is tried before any is replaced. + + They used to be loaded one after another, so a refusal from one left the ones before + it replaced and the rest of the import never ran. With `firegex.db` first — the order + an export lists them in — its `keys_values` were replaced and this instance's password + and secret never put back: the instance came back asking the next visitor to choose a + password. Found by the firewall's own rules, which no import could load at all. + """ + before = api.export_backup() + broken = { + "firegex.db": {"keys_values": before["firegex.db"]["keys_values"]}, + "services.db": {"services": [{"service_id": "x", "name": "x", "status": "bogus", + "proto": "tcp", "transport": "proxy"}]}, + } + why = api.s.post(f"{api.address}api/import", json=broken) + assert why.status_code == 400, why.text + assert "nothing was imported" in why.json().get("detail", ""), why.text + + assert api.status()["status"] == "run", "the instance lost its password to the import" + if auth_enabled: + assert api.status()["loggined"] is True, "the importing session was logged out" + assert FiregexAPI(fg_address).login(fg_password), "the password was lost or changed" + after = api.export_backup() + assert after["services.db"] == before["services.db"], "the services were changed" + + +def test_an_import_leaves_no_code_behind_from_after_the_backup(api, service): + """The database is replaced whole and the backup's filter files written, and the files + the backup did not have used to stay: the code of every Python filter created since, + on disk for filters that no longer existed after the import.""" + from helpers.net import free_port + + backup = api.export_backup() + port = free_port() + service_id = service(f"orphan-{port}", "127.0.0.1", port, "proxy") + filter_id = add_python_filter(api, service_id, TWO_FUNCTIONS, name="python") + assert f"{filter_id}.py" in api.export_backup().get("service_filters", {}) + + assert api.import_backup(backup), "the backup was not imported" + restored = api.export_backup() + files = set(restored.get("service_filters", {})) + assert f"{filter_id}.py" not in files, "the code of a filter the backup never had stayed" + used = {f"{row['filter_id']}.py" for row in restored["services.db"].get("filters", []) + if row["kind"] == "pyfilter"} + assert files <= used, f"filter files nothing refers to: {sorted(files - used)}" + kept = set(backup.get("service_filters", {})) & used + assert kept <= files, "the code of a filter in the backup was lost" diff --git a/tests/unit/test_backup_guards.py b/tests/unit/test_backup_guards.py index ac81ed5..ce16f69 100644 --- a/tests/unit/test_backup_guards.py +++ b/tests/unit/test_backup_guards.py @@ -112,6 +112,41 @@ def test_everything_else_in_a_table_is_carried(tmp_path): db.disconnect() +def test_a_column_named_like_a_keyword_is_loaded_back(tmp_path): + """The firewall's rules have a column called `table`. Left unquoted, loading it was a + syntax error, and no backup holding a single firewall rule could be imported.""" + db = SQLite(str(tmp_path / "firewall-rules.db"), {}) + db.connect() + db.create_schema({"rules": {"rule_id": "INT PRIMARY KEY", "`table`": "TEXT"}}) + db.query("INSERT INTO rules (rule_id, `table`) VALUES (0, 'filter');") + dumped = db.dump() + + db.query("DELETE FROM rules;") + db.load(dumped) + assert db.query("SELECT * FROM rules;") == [{"rule_id": 0, "table": "filter"}] + db.disconnect() + + +def test_a_dry_run_loads_nothing_and_still_finds_what_would_fail(tmp_path): + """What an import runs over every database before it changes any of them: one + refusing after others were replaced is how an import wiped this instance's + password and secret.""" + db = SQLite(str(tmp_path / "things.db"), {}) + db.connect() + db.create_schema({"things": {"id": "VARCHAR(10) PRIMARY KEY", + "v": "TEXT NOT NULL CHECK (v IN ('a', 'b'))"}}) + db.query("INSERT INTO things (id, v) VALUES ('kept', 'a');") + + db.load({"things": [{"id": "new", "v": "b"}]}, dry_run=True) + assert db.query("SELECT * FROM things;") == [{"id": "kept", "v": "a"}], \ + "a dry run changed the database" + + with pytest.raises(Exception): + db.load({"things": [{"id": "bad", "v": "not allowed"}]}, dry_run=True) + assert db.query("SELECT * FROM things;") == [{"id": "kept", "v": "a"}] + db.disconnect() + + # --- whether authentication starts off ---------------------------------------- # A container's environment is fixed when it is created. Docker starting it again by # itself — a reboot, a daemon restart, a crash under `restart: unless-stopped` — brings that @@ -142,3 +177,31 @@ def test_with_nothing_decided_since_the_environment_stands(): assert boot_auth_mode(None, False, False) == (False, False) assert boot_auth_mode(None, False, True) == (True, False) + + +# --- the code beside the database ---------------------------------------------- + + +def test_filter_files_no_filter_refers_to_are_removed_at_start(tmp_path, monkeypatch): + """An import replaced the database and wrote the backup's files, and left every file + the backup did not have: the code of each Python filter created since, on disk for a + filter that no longer existed.""" + import routers.services as router + + class Db: + def query(self, sql, *args): + return [{"filter_id": "kept"}] + + for name in ("kept.py", "orphan.py", "notes.txt"): + (tmp_path / name).write_text("x") + monkeypatch.setattr(router, "CODE_DIR", str(tmp_path)) + monkeypatch.setattr(router, "db", Db()) + router._prune_orphan_code() + assert sorted(p.name for p in tmp_path.iterdir()) == ["kept.py", "notes.txt"] + + +def test_no_code_directory_is_nothing_to_prune(tmp_path, monkeypatch): + import routers.services as router + + monkeypatch.setattr(router, "CODE_DIR", str(tmp_path / "missing")) + router._prune_orphan_code()