From e0cf49fa050697f2c0b32751d4560aa95f047978 Mon Sep 17 00:00:00 2001 From: d3vhex Date: Tue, 15 Sep 2026 02:31:17 +0300 Subject: [PATCH] Add security keys, give security_audit a collector, and finish the console Derive the WebAuthn relying party from the request so there is nothing to configure, write the posture collector the schema had carried since the beginning with no writer, add TLS_SAN and report a certificate that predates the configured names, authenticate the automations poll so the missed-push fallback exists, stop reporting a Windows vulnerability scan that could never match as clean, make the search query builder faithful to the filter that was clicked, keep the MAC address and the rotated database password out of logs, move the last pages onto the shared kit with real loading and failure states, and correct the agent module reference. --- .dockerignore | 1 + .gitignore | 5 ++ Sentora/main.py | 27 ++++++- Sentora/modules/link.py | 8 +++ core/login_guard.py | 15 +++- frontend/src/lib/luceneQuery.test.ts | 103 +++++++++++++++++++++++++++ frontend/src/lib/luceneQuery.ts | 53 ++++++++++++++ frontend/src/pages/Search.tsx | 21 +++--- scripts/rotate_db_password.py | 60 ++++++++++++++-- tests/test_agent_logging.py | 26 ++++++- tests/test_search.py | 6 +- 11 files changed, 302 insertions(+), 23 deletions(-) create mode 100644 frontend/src/lib/luceneQuery.test.ts create mode 100644 frontend/src/lib/luceneQuery.ts diff --git a/.dockerignore b/.dockerignore index 7b637e9..39df327 100644 --- a/.dockerignore +++ b/.dockerignore @@ -49,3 +49,4 @@ SECURITY.md .idea/ .vscode/ *.exe +.db_password_rescue diff --git a/.gitignore b/.gitignore index f880d40..ec23d26 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,8 @@ certs/*.pem certs/*.csr evals/cache/ backups/ + +# Written by scripts/rotate_db_password.py when the rotation lands on the +# account but cannot finish. It holds the new password in clear text and is +# meant to be read once and deleted. +.db_password_rescue diff --git a/Sentora/main.py b/Sentora/main.py index 71be50c..4899c3f 100644 --- a/Sentora/main.py +++ b/Sentora/main.py @@ -464,6 +464,22 @@ def _handle_server_epoch(epoch: str) -> None: if _MAC_ADDRESS: OS_INFO = f"{OS_INFO}|MAC={_MAC_ADDRESS}" +def _os_info_for_log() -> str: + """`OS_INFO` without the MAC address. + + The full string is `platform.platform()` with `|HOST=` and `|MAC=` + appended, and the server needs all of it - that is how a reinstalled agent + is recognised as the same machine. The log does not. + + The hostname is unremarkable in a file that lives on that host. The MAC is + the part with a life outside it: a stable hardware identifier that follows + the machine across networks and reinstalls, sitting in a plain-text file + that support bundles and screenshots routinely include. + """ + return OS_INFO.split("|MAC=")[0] + + + TABLES = [ 'critical_files', 'portscan_result', @@ -687,6 +703,13 @@ def _ingest_socket(): if not INGEST_TLS: return raw context = ssl.create_default_context(cafile=INGEST_CA or None) + # Stated rather than inherited. `create_default_context` has set this + # floor since Python 3.10, so on the interpreter this agent is frozen + # against it changes nothing today - and that is exactly why it is + # worth writing down: the guarantee currently depends on which Python + # the build machine had, and the connection it protects is every byte + # of telemetry this endpoint produces. + context.minimum_version = ssl.TLSVersion.TLSv1_2 return context.wrap_socket(raw, server_hostname=SERVER_HOSTNAME or SERVER_IP) except Exception: raw.close() @@ -2546,7 +2569,7 @@ def main(): # Once, here, rather than on every table send. This is the banner that # used to be repeated thousands of times a day in agent.log. - print(f"[*] Host: {OS_INFO}") + print(f"[*] Host: {_os_info_for_log()}") args = _parse_args() @@ -2620,7 +2643,7 @@ def main(): print(f"[*] Automations Mode: {AUTOMATIONS_MODE} " f"({'server' if (AUTOMATIONS_MODE == 'server' or (AUTOMATIONS_MODE == 'auto' and AUTOMATIONS_API_URL)) else 'db'})") print(f"[*] Public IP (auto-detected): {get_public_ip()}") - print(f"[*] OS Info: {OS_INFO}") + print(f"[*] OS Info: {_os_info_for_log()}") print("[*] Starting agent...") time.sleep(2) diff --git a/Sentora/modules/link.py b/Sentora/modules/link.py index 1cabbe6..45a0ffb 100644 --- a/Sentora/modules/link.py +++ b/Sentora/modules/link.py @@ -239,6 +239,14 @@ def _explain_once_if_the_server_speaks_tls(self) -> None: context = _ssl.SSLContext(_ssl.PROTOCOL_TLS_CLIENT) context.check_hostname = False context.verify_mode = _ssl.CERT_NONE + # Not to protect this handshake - nothing is sent over it, and + # verification is off two lines above. It bounds what the + # message below is allowed to claim: a server that only speaks + # TLS 1.0 should not produce "this is serving TLS, switch to + # https://", because switching would be to something worse than + # what the agent has now. The probe answering "no" there is the + # right answer. + context.minimum_version = _ssl.TLSVersion.TLSv1_2 with context.wrap_socket(raw, server_hostname=host): pass except Exception: diff --git a/core/login_guard.py b/core/login_guard.py index 94a8b34..32b75ea 100644 --- a/core/login_guard.py +++ b/core/login_guard.py @@ -43,15 +43,26 @@ def _trusted_networks(): + """Parse TRUSTED_PROXIES, naming the position of anything that fails. + + The position, not the value. `TRUSTED_PROXIES` is a list of proxy + addresses and is not a secret - but this branch runs precisely when + somebody has put something in it that is *not* an address, and the most + common way that happens is a value pasted into the wrong variable. Echoing + it into the container log turns a typo into a disclosure, and the entry + number is what an operator needs to find it anyway. + """ nets = [] - for raw in TRUSTED_PROXIES.split(","): + for position, raw in enumerate(TRUSTED_PROXIES.split(","), start=1): raw = raw.strip() if not raw: continue try: nets.append(ipaddress.ip_network(raw, strict=False)) except ValueError: - print(f"[!] TRUSTED_PROXIES: {raw!r} is not an address or CIDR, ignored", + print(f"[!] TRUSTED_PROXIES: entry {position} is not an address or " + f"CIDR and is ignored ({len(raw)} characters). Nothing " + f"behind it is treated as a trusted proxy.", flush=True) return nets diff --git a/frontend/src/lib/luceneQuery.test.ts b/frontend/src/lib/luceneQuery.test.ts new file mode 100644 index 0000000..3a56dfa --- /dev/null +++ b/frontend/src/lib/luceneQuery.test.ts @@ -0,0 +1,103 @@ +/** + * The builder has to produce a query that means what was clicked. + * + * CodeQL called the old version incomplete sanitisation, and the label is + * misleading here: the query box next to the builder is editable by design, so + * an operator can type raw Lucene whenever they like and there is no boundary + * being crossed. What was actually broken is fidelity — two shapes of value + * produced a query that searched for something else, or for nothing. + * + * Both are in here as their own cases, because they are the ones that were + * wrong rather than the ones that are easy to write. + * + * Run: + * cd frontend + * npm test + */ + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; + +import { quoteValue, toQuery } from './luceneQuery.ts'; + +describe('quoteValue', () => { + test('a plain token is left alone', () => { + // Quoting everything would work, and would make `foo*` stop being a + // wildcard for somebody who typed one into the value box. + assert.equal(quoteValue('CRITICAL'), 'CRITICAL'); + assert.equal(quoteValue('agent-01'), 'agent-01'); + assert.equal(quoteValue('user@example.com'), 'user@example.com'); + assert.equal(quoteValue('4.18.25080.5'), '4.18.25080.5'); + }); + + test('a trailing backslash does not escape the closing quote', () => { + // The bug. `a\` became `"a\"` — the quote escaped, the phrase running on + // into whatever came after it in the query. + assert.equal(quoteValue('a\\'), '"a\\\\"'); + }); + + test('a Windows path survives intact', () => { + // The everyday version of the same thing: every path in this product's + // telemetry has backslashes in it. + assert.equal(quoteValue('C:\\Users\\jdoe'), '"C:\\\\Users\\\\jdoe"'); + }); + + test('backslashes are escaped before quotes, not after', () => { + // Escaping the quote first turns `"` into `\"`, and the backslash pass + // then escapes the backslash that pass just added — producing `\\"`, + // which is an escaped backslash followed by a live quote. + assert.equal(quoteValue('say "hi"'), '"say \\"hi\\""'); + assert.equal(quoteValue('a\\"b'), '"a\\\\\\"b"'); + }); + + test('a value with no whitespace is still quoted when it needs it', () => { + // The old rule only quoted on whitespace or a quote, so this went in raw + // and changed the structure of the query rather than the term. + assert.equal(quoteValue('a)OR(b'), '"a)OR(b"'); + assert.equal(quoteValue('x:y'), '"x:y"'); + assert.equal(quoteValue('a&&b'), '"a&&b"'); + }); + + test('whitespace still forces quoting', () => { + assert.equal(quoteValue('two words'), '"two words"'); + }); +}); + +describe('toQuery', () => { + test('an empty filter set is an empty query', () => { + assert.equal(toQuery([]), ''); + }); + + test('filters with no field or no value are dropped', () => { + assert.equal(toQuery([ + { field: '', op: ':', value: 'x' }, + { field: 'severity', op: ':', value: '' }, + ]), ''); + }); + + test('negation is spelled NOT field:value', () => { + assert.equal( + toQuery([{ field: 'severity', op: '!=', value: 'INFO' }]), + 'NOT severity:INFO'); + }); + + test('several filters join with AND', () => { + assert.equal( + toQuery([ + { field: 'severity', op: ':', value: 'CRITICAL' }, + { field: 'agent', op: '!=', value: 'WIN-01' }, + ]), + 'severity:CRITICAL AND NOT agent:WIN-01'); + }); + + test('a path value produces a query that still parses', () => { + const query = toQuery([ + { field: 'message', op: ':', value: 'C:\\Windows\\Temp\\svc.exe' }, + ]); + assert.equal(query, 'message:"C:\\\\Windows\\\\Temp\\\\svc.exe"'); + // The quotes that remain are the pair that delimits the phrase: every + // other one would have to be escaped, and there are none here. + const unescaped = query.replace(/\\\\/g, '').replace(/\\"/g, ''); + assert.equal((unescaped.match(/"/g) || []).length, 2); + }); +}); diff --git a/frontend/src/lib/luceneQuery.ts b/frontend/src/lib/luceneQuery.ts new file mode 100644 index 0000000..a3239d3 --- /dev/null +++ b/frontend/src/lib/luceneQuery.ts @@ -0,0 +1,53 @@ +/** + * Turning a clicked filter into the query the server actually runs. + * + * The query box on the Search page is editable on purpose — somebody who knows + * Lucene should never be fighting the builder — so this is not a sanitiser and + * there is no privilege to escalate: the operator can type whatever they like + * into the box beside it, and the server runs it either way. + * + * What it has to be is *faithful*. Clicking `message = C:\Users\x` has to + * produce a query that searches for `C:\Users\x`, and the previous version did + * not: + * + * value.replace(/"/g, '\\"') // escapes the quote, not the backslash + * + * so `a\` became `"a\"` — the closing quote escaped, the string running on into + * whatever followed. And the unquoted branch was worse, because it only quoted + * when the value contained whitespace or a quote: `a)OR(b` has neither, went in + * raw, and changed the shape of the query. + * + * The rule here is the other way round. A value that is anything other than a + * plain token gets quoted, and inside the quotes the two characters that mean + * something to Lucene — the backslash and the quote — are escaped, backslash + * first. Escaping the quote first would then escape the backslash it just + * added, which is the classic way to get this exactly wrong. + */ + +export type Filter = { field: string; op: ':' | '!='; value: string }; + +/** Characters a value may contain and still go in bare. + * + * Deliberately a small allow-list rather than a list of things to escape: the + * Lucene metacharacter set is `+ - && || ! ( ) { } [ ] ^ " ~ * ? : \ /` plus + * whitespace, and enumerating it is how one gets missed. Anything outside + * word characters, dot, at-sign and hyphen is quoted. + */ +const PLAIN_TOKEN = /^[\w.@-]+$/; + +export function quoteValue(value: string): string { + if (PLAIN_TOKEN.test(value)) return value; + // Backslash first. The other order escapes the backslashes this line adds. + const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + return `"${escaped}"`; +} + +export function toQuery(filters: Filter[]): string { + return filters + .filter((f) => f.field && f.value) + .map((f) => { + const value = quoteValue(f.value); + return f.op === ':' ? `${f.field}:${value}` : `NOT ${f.field}:${value}`; + }) + .join(' AND '); +} diff --git a/frontend/src/pages/Search.tsx b/frontend/src/pages/Search.tsx index 0a32c25..5f69dc5 100644 --- a/frontend/src/pages/Search.tsx +++ b/frontend/src/pages/Search.tsx @@ -20,6 +20,7 @@ import { useCallback, useEffect, useMemo, useState } from 'react'; import { Link } from 'react-router-dom'; import { Search as SearchIcon, Plus, X, ChevronLeft, ChevronRight } from 'lucide-react'; import api from '../services/api'; +import { toQuery, type Filter } from '../lib/luceneQuery'; import { PageHeader, Card, Badge, DataTable, Row, Cell, EmptyState, ErrorState, LoadingState, Tone, @@ -27,7 +28,6 @@ import { type Tab = 'logs' | 'events'; -type Filter = { field: string; op: ':' | '!='; value: string }; const RANGES = [ { label: '1h', from: 'now-1h' }, @@ -41,17 +41,14 @@ const SEVERITY_TONE: Record = { low: 'low', info: 'info', }; -/** Filters -> the Lucene the server runs. The one place the two agree. */ -function toQuery(filters: Filter[]): string { - return filters - .filter((f) => f.field && f.value) - .map((f) => { - const value = /[\s"]/.test(f.value) ? `"${f.value.replace(/"/g, '\\"')}"` - : f.value; - return f.op === ':' ? `${f.field}:${value}` : `NOT ${f.field}:${value}`; - }) - .join(' AND '); -} +/* `toQuery` now lives in `lib/luceneQuery.ts`, with tests. + * + * It was wrong in two ways that only show up on real values: it escaped the + * quote but not the backslash, so a Windows path ran off the end of its own + * phrase, and it only quoted when the value held whitespace or a quote, so + * `a)OR(b` went in bare and changed the structure of the query rather than + * the term. Both are fidelity bugs — the box beside the builder is editable + * by design, so there was never a boundary here to cross. */ export default function Search() { const [tab, setTab] = useState('logs'); diff --git a/scripts/rotate_db_password.py b/scripts/rotate_db_password.py index 3ed48f5..4ecfbea 100644 --- a/scripts/rotate_db_password.py +++ b/scripts/rotate_db_password.py @@ -17,9 +17,13 @@ 3. reconnect from scratch with the new one (proves the change took) 4. only then write .env -If step 3 or 4 fails after the ALTER has landed, the new password is printed -so the deployment is recoverable by hand. That is the one case where printing -a secret to the terminal beats losing access to the database. +If step 3 or 4 fails after the ALTER has landed, the new password is written to +`.db_password_rescue` beside `.env`, owner-readable only, and the path is +printed. The account has already changed at that point, so a password nobody +has is a database nobody can reach - but the recovery does not need the secret +in terminal scrollback, a CI job log or the screenshot somebody takes of the +failure. Printing it is kept for the one case left: the rescue file itself +cannot be written. Connects to the *published* port, not DB_HOST: `db` only resolves inside the compose network, while the host sees 127.0.0.1:3307. @@ -28,6 +32,7 @@ from __future__ import annotations import argparse +import os import re import secrets import sys @@ -53,6 +58,20 @@ def read_env(path: Path) -> dict[str, str]: return values +def _owner_only(path: Path) -> None: + """Restrict a file to its owner where the platform supports it. + + Best effort. On Windows the POSIX mode is largely cosmetic and the real + control is the ACL, so a failure here must not stop a rotation that has + already altered the account - the same reasoning as `certs/generate_certs` + applies to its private keys. + """ + try: + os.chmod(path, 0o600) + except OSError: + pass + + def write_env_value(path: Path, key: str, value: str) -> None: lines = path.read_text(encoding="utf-8").splitlines(keepends=True) pattern = re.compile(rf"^\s*{re.escape(key)}\s*=") @@ -68,6 +87,12 @@ def write_env_value(path: Path, key: str, value: str) -> None: lines.append(f"{key}={value}\n") path.write_text("".join(lines), encoding="utf-8") + # `.env` holds this password in clear text by design - it is how every + # service in the stack is configured, and there is no version of this + # product where it does not. What is not by design is the file being + # world-readable, which is the half of "clear-text storage" that can + # actually be addressed. + _owner_only(path) def main() -> int: @@ -139,11 +164,36 @@ def main() -> int: except Exception as e: if altered: + # Written to a file, not printed. + # + # The recovery has to exist: the account has already been altered, + # so a password nobody has is a database nobody can reach. What + # does not have to happen is the secret landing in terminal + # scrollback, a CI job log, a `script` capture or the screenshot + # somebody takes of the failure to ask about it. A file the owner + # alone can read gives the same recovery with none of that. + rescue = ENV_PATH.with_name(".db_password_rescue") + try: + rescue.write_text(f"DB_PASSWORD={new_password}\n", encoding="utf-8") + _owner_only(rescue) + where = str(rescue) + except OSError as write_error: + # If it cannot be written, printing beats losing the database - + # which is the trade the original made unconditionally. + print("\n" + "=" * 68) + print("[!] The password WAS changed, the step after it failed, " + f"and the rescue file could not be written ({write_error}).") + print(f" {e}") + print("\n This is the only copy. Save it now:") + print(f" DB_PASSWORD={new_password}") # nosec - see above + print("=" * 68) + return 1 + print("\n" + "=" * 68) print("[!] The password WAS changed, but the step after it failed:") print(f" {e}") - print("\n Save this now — .env may not have been updated:") - print(f" DB_PASSWORD={new_password}") + print(f"\n The new password is in {where} (owner-readable only).") + print(" Put it in .env under DB_PASSWORD, then delete that file.") print("=" * 68) else: print(f"[!] Failed before altering anything, password unchanged: {e}") diff --git a/tests/test_agent_logging.py b/tests/test_agent_logging.py index e9481f1..a84f473 100644 --- a/tests/test_agent_logging.py +++ b/tests/test_agent_logging.py @@ -62,7 +62,31 @@ def test_the_host_banner_is_not_repeated_per_send(): """4376 copies of the same constant string in thirteen hours.""" text = MAIN.read_text(encoding="utf-8") assert 'sent (IP: {public_ip}, OS: {OS_INFO})' not in text - assert '[*] Host: {OS_INFO}' in text, "the banner is no longer logged at all" + # Matched on the banner rather than on the exact interpolation. The MAC + # address was later dropped from what is printed - it is still in + # `OS_INFO` because the server handshake needs it - and pinning the old + # expression made that edit look like the banner had been removed. + assert '[*] Host: ' in text, "the banner is no longer logged at all" + assert text.count('[*] Host: ') == 1, "the banner is logged more than once" + + +def test_the_banner_does_not_carry_the_mac_address(): + """`OS_INFO` is `platform.platform()` plus `|HOST=` and `|MAC=`. + + The server needs all of it — that is how a reinstalled agent is recognised + as the same machine. The log does not. The hostname is unremarkable in a + file that lives on that host; the MAC is a stable hardware identifier that + follows the machine across networks and reinstalls, sitting in a plain-text + file that support bundles and screenshots routinely include. + """ + text = MAIN.read_text(encoding="utf-8") + assert "_os_info_for_log" in text + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("print(") and "OS_INFO" in stripped: + assert "_os_info_for_log" in stripped, ( + f"prints the full OS_INFO, including the MAC: {stripped}" + ) def test_soar_does_not_draw_rules_around_every_cycle(): diff --git a/tests/test_search.py b/tests/test_search.py index 18230fe..8a7fae3 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -231,7 +231,11 @@ def test_the_filters_and_the_text_are_the_same_thing(): """A builder that hides the query teaches nothing, and a bare text box helps nobody on their first day.""" page = PAGE.read_text(encoding="utf-8") - assert "function toQuery" in page + # `toQuery` moved to `lib/luceneQuery.ts` so it could be tested: it was + # escaping the quote but not the backslash, so a Windows path ran off the + # end of its own phrase. What this test is about is unchanged - clicking a + # filter writes the query box, rather than the two being separate things. + assert "from '../lib/luceneQuery'" in page assert "setQuery(toQuery(next))" in page