From efb265fc3a8aed8fc9cd30201dfeeccd409e7bbe Mon Sep 17 00:00:00 2001 From: Guillermo Rojo Date: Thu, 8 Oct 2026 14:38:26 +0200 Subject: [PATCH 1/3] fix: least privilege and quoting for what reaches root - Certificates: operator paths quoted; files staged beside the live ones, key/cert/match/chain checked, then moved (previous kept). - Venv packages: the venv pip runs as the instance user with no login shell; root only passes the requirements file on stdin; pip options refused; commas split only before a package name. - Report reads pyvenv.cfg instead of running the instance interpreter; git runs as the instance user (git refuses root in another user's checkout), and both OCB origin forms are detected. - Neutralisation and copy identity run as the copy's owner role with a fixed search_path, not as the superuser. - Venv replication and wkhtmltopdf use a private mktemp directory; the replication keeps exact name==version lines and fails with pip. - tar runs as the instance user for backups (manual and scheduled) and restores. - Host text is sanitised before it reaches the terminal; typed control characters are refused; typed unit names go after --. - mask covers escaped secrets; the role SQL uses a unique dollar tag; exports are new private files; run(check=True) no longer prints the command. - tools/verify_privilege.py (CI) executes the certificate, download, venv and owner-role neutralisation steps. --- .github/workflows/ci.yml | 3 + .gitignore | 4 +- CHANGELOG.md | 25 ++ CONTRIBUTING.md | 10 +- SECURITY.md | 6 +- docs/installation.md | 2 +- docs/operations/instance-management.md | 7 +- instance_manager/i18n.py | 5 + instance_manager/planners.py | 205 +++++++-------- instance_manager/prompts.py | 22 +- instance_manager/system.py | 24 +- instance_manager/ui.py | 22 +- instance_manager/workflows/addons.py | 6 +- instance_manager/workflows/backup_restore.py | 85 ++++-- instance_manager/workflows/common.py | 10 + instance_manager/workflows/manage.py | 41 ++- instance_manager/workflows/report.py | 21 +- instance_manager/workflows/services.py | 22 +- .../proposal.md | 54 ++++ .../specs/addon-inventory/spec.md | 19 ++ .../specs/data-backup-restore/spec.md | 234 +++++++++++++++++ .../specs/execution-safety/spec.md | 52 ++++ .../specs/instance-configuration/spec.md | 26 ++ .../specs/instance-provisioning/spec.md | 38 +++ .../specs/server-audit/spec.md | 30 +++ .../specs/web-proxy-tls/spec.md | 23 ++ .../tasks.md | 25 ++ openspec/specs/addon-inventory/spec.md | 3 +- openspec/specs/data-backup-restore/spec.md | 21 +- openspec/specs/execution-safety/spec.md | 23 +- openspec/specs/instance-configuration/spec.md | 11 +- openspec/specs/instance-provisioning/spec.md | 4 +- openspec/specs/server-audit/spec.md | 8 +- openspec/specs/web-proxy-tls/spec.md | 14 +- tests/test_backup_restore.py | 10 +- tests/test_injection_privilege.py | 162 ++++++++++++ tools/verify_data_safety.py | 4 +- tools/verify_neutralisation.py | 6 +- tools/verify_privilege.py | 243 ++++++++++++++++++ tools/verify_secrets.py | 2 +- 40 files changed, 1309 insertions(+), 223 deletions(-) create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/proposal.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/addon-inventory/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-configuration/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-provisioning/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/server-audit/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/web-proxy-tls/spec.md create mode 100644 openspec/changes/archive/2026-10-08-least-privilege-and-quoting/tasks.md create mode 100644 tests/test_injection_privilege.py create mode 100644 tools/verify_privilege.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e05c2c..e4e72a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,9 @@ jobs: - name: Verify secrets stay out of process arguments, previews and errors run: python tools/verify_secrets.py + - name: Verify what reaches root or a superuser (certificates, downloads, venvs, neutralisation) + run: python tools/verify_privilege.py + - name: Verify nginx and fail2ban accept the generated configuration run: | sudo apt-get update -qq diff --git a/.gitignore b/.gitignore index dd15ec4..18a0eee 100644 --- a/.gitignore +++ b/.gitignore @@ -178,4 +178,6 @@ cython_debug/ # exclude from AI features like autocomplete and code analysis. Recommended for sensitive data # refer to https://docs.cursor.com/context/ignore-files .cursorignore -.cursorindexingignore \ No newline at end of file +.cursorindexingignore +# Exported reports (they name databases, users and paths) +reports/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0baacd4..ebd8d3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,31 @@ All notable changes to this project are documented here. The format is based on ### Fixed +- **Root shell and superuser exposure.** + - The certificate paths and the venv package names reached a root shell unescaped, so a file or package named + `$(…)` ran as root. + - The server report ran the instance's own Python interpreter as root. + - Neutralisation ran as the PostgreSQL superuser on a database copied from another instance, where a trigger + could raise its own role. + + Now: + - every operator value is quoted; + - pip, tar and git run as the instance user; + - the report reads `pyvenv.cfg` instead of running the interpreter; + - neutralisation runs as the copy's owner role; + - a key that does not match its certificate no longer replaces the live files, which would have broken the + next nginx restart for every instance. +- **Fixed `/tmp` names written as root** (the venv replication's requirements, the wkhtmltopdf package) are now + private temporary directories. The venv replication fails when pip fails, keeps only `name==version` lines + and shows them. +- Escape sequences in database names, file names or command output are printed inert, and typed values with + control characters are refused. +- A replica now detects its source's branch: git refused to run as root in the instance's checkout. +- Smaller fixes: + - a secret escaped inside SQL is masked in the preview; + - a password containing `$$` no longer breaks the role SQL; + - exports are new private files that never follow a link. + - **Installing over an existing instance removed it.** The install never checked the name was free, and its cleanup ran on a failure — or on Ctrl+C at the confirmation prompt — removing that instance's home, config, unit and vhosts. A new instance now needs a free name (no system account or service such as `backup`, whose diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c9e9f9a..27aebc2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,14 +53,22 @@ python3 tools/verify_install_runtime.py --clone 14=/path/odoo-14.0 --clone 16=/p python3 tools/verify_neutralisation.py --odoo 18=/path/venv/bin/python:/path/odoo-18.0 python3 tools/verify_ops_configs.py # nginx -t and fail2ban on the generated config python3 tools/verify_secrets.py # secrets in no process argument, preview or error +python3 tools/verify_privilege.py # certificates, downloads, venv replication, neutralisation as owner ``` -`tools/verify_data_safety.py`, `tools/verify_secrets.py` and `tools/verify_ops_configs.py` also run in CI. +`tools/verify_data_safety.py`, `tools/verify_secrets.py`, `tools/verify_privilege.py` and +`tools/verify_ops_configs.py` also run in CI. `tools/verify_data_safety.py` executes the generated scheduled-backup script, the retention prune and the delete step against stub binaries, and the template copy, forced drop and purge discovery against a PostgreSQL cluster of its own (it needs `initdb` and refuses to run as root). Run it after changing any of those. +`tools/verify_privilege.py` runs the certificate steps with real `openssl` (a key that does not match must leave +the live files untouched, a hostile file name must stay a name), the wkhtmltopdf and venv-replication steps +against stubs, and the neutralisation as the copy's owner against a PostgreSQL cluster of its own, where a +trigger in the copy tries to make its owner a superuser. Run it after changing those planners or +`_post_db_mode_commands`. + `tools/verify_install_runtime.py` is not part of CI: it downloads the pinned uv (and refuses a wrong checksum), installs the interpreter the matrix picks, builds a real venv for each Odoo checkout you pass and starts `odoo-bin --version`; for Odoo <= 16 it also shows the unpinned setuptools breaks it. It needs network diff --git a/SECURITY.md b/SECURITY.md index 862badc..6a2e55a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -32,8 +32,10 @@ A reproduction against a disposable VM is ideal. ## Handling credentials -- Database and admin passwords are entered interactively and passed to `psql`/`pg_dump` via `PGPASSWORD` in - the generated commands. Treat shell history and process listings on the host as sensitive. +- Database and admin passwords are entered interactively without echo. They reach `psql`/`pg_dump` through + the step's environment (`PGPASSWORD`), never its command line, and the plan preview masks them; a file holding + a secret is written from the environment too. Treat the host's root account as sensitive: root can read any + process's environment. - Do not commit real credentials, dumps, or filestore archives to the repository. - The recommended permissions baseline in [`docs/security/safe-controls.md`](docs/security/safe-controls.md) denies reads of common secret files (`.env`, `*.pem`, SSH keys). diff --git a/docs/installation.md b/docs/installation.md index 85bd55e..076f9bd 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -139,7 +139,7 @@ For HTTPS you pick a certificate strategy: | **Leave certificates untouched** | Adds no certificate commands. | | **Self-signed** | Reuses an existing key/fullchain or generates a 2048-bit self-signed cert for the domain. | | **Let's Encrypt (managed externally)** | Adds no certificate commands — you manage LE outside the tool. | -| **Copy your own certificates** | Installs your CRT/KEY (+ optional intermediate), builds the fullchain, and **validates that the key matches the certificate** before Nginx is reconfigured. | +| **Copy your own certificates** | Copies your CRT/KEY (+ optional intermediate) beside the live files, builds the fullchain, and **checks that the key matches the certificate** before they replace the current ones (kept as `.previous`); a wrong file never reaches Nginx. | ## The instance name must be free diff --git a/docs/operations/instance-management.md b/docs/operations/instance-management.md index bb75a8f..37ce41e 100644 --- a/docs/operations/instance-management.md +++ b/docs/operations/instance-management.md @@ -70,7 +70,8 @@ To go back, copy the files from the `config_preupdate/` directory and (`www-data:adm`, mode `640`) and reopens Nginx's logs. - **Log rotation** — see [Log rotation](log-rotation.md). - **Install Python packages in the venv** installs into the instance's virtualenv from a requirements file or an - inline package list, running pip as the instance user. + inline package list, running pip as the instance user. Separate packages with commas (`requests, lxml`); a + version range keeps its own comma (`babel>=2.14,<3`), and pip options (`--index-url …`) are refused. ## Backups & duplication @@ -109,7 +110,9 @@ for a refresh in place): - SII, TicketBAI, EDI proxy and Peppol in test mode; - website domain and CDN cleared, `web.base.url` pointing at the target, the "neutralised" banner and flag. - The rules, each with its source, are in `instance_manager/neutralise.py`. + The rules, each with its source, are in `instance_manager/neutralise.py`. They run as the copy's owner role, + not as the PostgreSQL superuser, so code the source database carries (a trigger, a function) gets no more + rights than its own role has. A copy is never visible to a running Odoo before it is neutralised. An Odoo's cron worker lists every database its role owns, whatever the `dbfilter`, so a duplication goes this way: diff --git a/instance_manager/i18n.py b/instance_manager/i18n.py index fa5362d..21afd5a 100644 --- a/instance_manager/i18n.py +++ b/instance_manager/i18n.py @@ -969,4 +969,9 @@ def tf(template: str, *args: object, **kwargs: object) -> str: 'That name is a system account or service name. Choose another instance name.': 'Ese nombre es de una cuenta o un servicio del sistema. Elige otro nombre de instancia.', 'Data dirs': 'Data dirs', 'Filestore folders': 'Carpetas de filestore', + 'Install the certificate files, kept only if the key and chain check out': 'Instalar los certificados, solo si la clave y la cadena son correctas', + 'Download, verify (SHA-256) and install patched wkhtmltopdf ({})': 'Descargar, verificar (SHA-256) e instalar wkhtmltopdf parcheado ({})', + 'Control characters are not allowed.': 'No se admiten caracteres de control.', + 'Ignored (not a package): {}': 'Ignorado (no es un paquete): {}', + 'Invalid service name. Use letters, digits and @ . _ : -': 'Nombre de servicio no válido. Usa letras, dígitos y @ . _ : -', } diff --git a/instance_manager/planners.py b/instance_manager/planners.py index 02064ea..104418f 100644 --- a/instance_manager/planners.py +++ b/instance_manager/planners.py @@ -26,11 +26,21 @@ def _is_local_db_host(db_host: str) -> bool: } +def _dollar_tag(*bodies: str) -> str: + """A dollar-quote tag that none of ``bodies`` contains: a password holding ``$$`` + would otherwise end the quoted block early.""" + index = 0 + while any(f"$oim{index}$" in body for body in bodies): + index += 1 + return f"$oim{index}$" + + def _db_role_create_if_missing_sql(config: InstanceConfig) -> str: db_user_literal = _sql_literal(config.db_user) db_password_literal = _sql_literal(config.db_password) + tag = _dollar_tag(db_password_literal) return ( - "DO $$ " + f"DO {tag} " "BEGIN " f"IF NOT EXISTS (SELECT FROM pg_roles WHERE rolname='{db_user_literal}') THEN " f"CREATE ROLE {config.db_user} WITH LOGIN CREATEDB PASSWORD '{db_password_literal}'; " @@ -39,7 +49,7 @@ def _db_role_create_if_missing_sql(config: InstanceConfig) -> str: f"RAISE NOTICE 'Role {db_user_literal} already exists; reusing it without changing the password.'; " "END IF; " "END " - "$$;" + f"{tag};" ) @@ -1128,128 +1138,89 @@ def plan_nginx_https( ] +_PEM_CERTS_AWK = ( + "awk 'BEGIN{c=0} /-----BEGIN CERTIFICATE-----/{c=1} c{gsub(/\\r$/,\"\"); print} " + "/-----END CERTIFICATE-----/{c=0; print \"\"}'" +) + + def plan_copy_custom_certs( config: InstanceConfig, cert_src: str, key_src: str, intermediate_src: str | None, ) -> list[Command]: - commands: list[Command] = [ - Command( - 'Create dedicated SSL directory', - f"install -d -m 750 -o root -g www-data '{config.nginx_ssl_dir}'", - ), - Command( - 'Copy server.crt', - f"install -m 644 -o root -g www-data '{cert_src}' '{config.ssl_cert_file}'", - ), - Command( - 'Copy server.key', - f"install -m 640 -o root -g www-data '{key_src}' '{config.ssl_key_file}'", - ), - ] + """Install the operator's certificate, key and optional chain in one step. + They are copied into a staging directory beside the live files and checked there + — the key, the certificate, that they match, the full chain — and only then + moved into place, the previous files kept as ``.previous``. A wrong file never + replaces a working one: the next nginx restart, of any instance, would fail.""" + q = shlex.quote + ssl_dir = q(config.nginx_ssl_dir) + targets = [("crt", config.ssl_cert_file, "644"), ("key", config.ssl_key_file, "640"), + ("fullchain", config.ssl_fullchain_file, "644")] + lines = [ + "set -e", + f"install -d -m 750 -o root -g www-data {ssl_dir}", + f'stage=$(mktemp -d {q(config.nginx_ssl_dir + "/.stage.XXXXXX")})', + 'trap \'rm -rf "$stage"\' EXIT', + f'cp -- {q(cert_src)} "$stage/crt"', + f'cp -- {q(key_src)} "$stage/key"', + ] if intermediate_src: - commands.append( - Command( - 'Copy intermediate.crt', - f"install -m 644 -o root -g www-data '{intermediate_src}' '{config.ssl_intermediate_file}'", - ) - ) - commands.append( - Command( - 'Build fullchain (cert + intermediate)', - "{ " - + "awk 'BEGIN{in_cert=0} /-----BEGIN CERTIFICATE-----/{in_cert=1} in_cert{gsub(/\\r$/,\"\"); print} /-----END CERTIFICATE-----/{in_cert=0; print \"\"}' " - + f"'{config.ssl_cert_file}'; " - + "awk 'BEGIN{in_cert=0} /-----BEGIN CERTIFICATE-----/{in_cert=1} in_cert{gsub(/\\r$/,\"\"); print} /-----END CERTIFICATE-----/{in_cert=0; print \"\"}' " - + f"'{config.ssl_intermediate_file}'; " - + f"}} > '{config.ssl_fullchain_file}'", - ) - ) - else: - commands.append( - Command( - 'Use server.crt as fullchain', - f"cp '{config.ssl_cert_file}' '{config.ssl_fullchain_file}'", - ) - ) - - commands.extend( - [ - Command( - 'Validate TLS private key', - f"openssl pkey -in '{config.ssl_key_file}' -noout >/dev/null", - ), - Command( - 'Validate main TLS certificate', - f"openssl x509 -in '{config.ssl_cert_file}' -noout >/dev/null", - ), - Command( - 'Validate KEY/CRT match', - "CERT_FP=$(openssl x509 -in '" - + config.ssl_cert_file - + "' -pubkey -noout | openssl pkey -pubin -outform PEM 2>/dev/null | sha256sum | awk '{print $1}') && " - + "KEY_FP=$(openssl pkey -in '" - + config.ssl_key_file - + "' -pubout -outform PEM 2>/dev/null | sha256sum | awk '{print $1}') && " - + "test -n \"$CERT_FP\" -a -n \"$KEY_FP\" -a \"$CERT_FP\" = \"$KEY_FP\" " - + "|| (echo '[ERROR] The private key does not match the selected public certificate.' && exit 1)", - ), - Command( - 'Validate TLS fullchain', - f"openssl x509 -in '{config.ssl_fullchain_file}' -noout >/dev/null", - ), + targets.append(("intermediate", config.ssl_intermediate_file, "644")) + lines += [ + f'cp -- {q(intermediate_src)} "$stage/intermediate"', + f'{{ {_PEM_CERTS_AWK} "$stage/crt"; {_PEM_CERTS_AWK} "$stage/intermediate"; }} > "$stage/fullchain"', ] - ) - - commands.extend( - [ - Command( - "Owner fullchain", f"chown root:www-data '{config.ssl_fullchain_file}'" - ), - Command('Permissions on fullchain', f"chmod 644 '{config.ssl_fullchain_file}'"), + else: + lines.append('cp "$stage/crt" "$stage/fullchain"') + lines += [ + 'openssl pkey -in "$stage/key" -noout', + 'openssl x509 -in "$stage/crt" -noout', + 'cert_fp=$(openssl x509 -in "$stage/crt" -pubkey -noout | openssl pkey -pubin -outform PEM | sha256sum)', + 'key_fp=$(openssl pkey -in "$stage/key" -pubout -outform PEM | sha256sum)', + 'if [ "$cert_fp" != "$key_fp" ]; then ' + "echo '[ERROR] The private key does not match the selected public certificate.' >&2; exit 1; fi", + 'openssl x509 -in "$stage/fullchain" -noout', + ] + for name, target, mode in targets: + lines += [ + f'chown root:www-data "$stage/{name}" && chmod {mode} "$stage/{name}"', + f"if [ -e {q(target)} ]; then cp -a {q(target)} {q(target + '.previous')}; fi", ] - ) - return commands + lines += [f'mv -f "$stage/{name}" {q(target)}' for name, target, _mode in targets] + return [ + Command( + 'Install the certificate files, kept only if the key and chain check out', + "\n".join(lines), + ) + ] def plan_ensure_self_signed_certs(config: InstanceConfig) -> list[Command]: + q = shlex.quote + key, cert, chain = q(config.ssl_key_file), q(config.ssl_cert_file), q(config.ssl_fullchain_file) return [ Command( 'Ensure dedicated SSL directory', - f"install -d -m 750 -o root -g www-data '{config.nginx_ssl_dir}'", + f"install -d -m 750 -o root -g www-data {q(config.nginx_ssl_dir)}", ), Command( 'Generate self-signed if missing (server.key/fullchain)', - "if [ -s '" - + config.ssl_key_file - + "' ] && [ -s '" - + config.ssl_fullchain_file - + "' ]; then " - + "echo '[INFO] Existing self-signed certificate, reusing it.'; " - + "else " - + "command -v openssl >/dev/null 2>&1 || (apt-get update && DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::=--force-confold -o DPkg::Lock::Timeout=600 install openssl); " - + "openssl req -x509 -nodes -newkey rsa:2048 -sha256 -days 825 " - + "-keyout '" - + config.ssl_key_file - + "' " - + "-out '" - + config.ssl_cert_file - + "' " - + "-subj '/CN=" - + config.domain - + "'; " - + "cp '" - + config.ssl_cert_file - + "' '" - + config.ssl_fullchain_file - + "'; " - + "fi", + f"if [ -s {key} ] && [ -s {chain} ]; then " + "echo '[INFO] Existing self-signed certificate, reusing it.'; " + "else " + "command -v openssl >/dev/null 2>&1 || (apt-get update && DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::=--force-confold -o DPkg::Lock::Timeout=600 install openssl); " + "openssl req -x509 -nodes -newkey rsa:2048 -sha256 -days 825 " + f"-keyout {key} -out {cert} -subj {q('/CN=' + config.domain)} && " + f"cp {cert} {chain}; " + "fi", ), Command( 'Adjust self-signed certificate permissions', - f"chown root:www-data '{config.ssl_key_file}' '{config.ssl_cert_file}' '{config.ssl_fullchain_file}' && chmod 640 '{config.ssl_key_file}' && chmod 644 '{config.ssl_cert_file}' '{config.ssl_fullchain_file}'", + f"chown root:www-data {key} {cert} {chain} && chmod 640 {key} && chmod 644 {cert} {chain}", ), ] @@ -1364,8 +1335,11 @@ def _scheduled_backup_script( 'if [ ! -d "$FILESTORE" ]; then echo "Filestore not found: $FILESTORE" >&2; exit 1; fi', 'PARTIAL="$BACKUP_DIR/$PREFIX$TS.filestore.tar.gz.partial"', # Exit 1 is GNU tar's "a file changed while read": Odoo keeps writing - # attachments, and every file already archived is complete. - 'rc=0; tar --warning=no-file-changed -czf "$PARTIAL" -C "$FILESTORE" . || rc=$?', + # attachments, and every file already archived is complete. tar reads as + # the instance user (a link it planted reaches nothing root alone could + # read); root writes the archive. + f"rc=0; sudo -u {shlex.quote(config.odoo_user)} -H " + 'tar --warning=no-file-changed -czf - -C "$FILESTORE" . > "$PARTIAL" || rc=$?', 'if [ "$rc" -gt 1 ]; then exit "$rc"; fi', f'mv -- "$PARTIAL" "$BACKUP_DIR/$PREFIX$TS{BACKUP_FILESTORE_SUFFIX}"', 'PARTIAL=""', @@ -1564,25 +1538,24 @@ def plan_install_wkhtmltopdf(mode: str, codename: str = "") -> list[Command]: if asset is None: return [] url, filename, sha256 = asset - tmp = f"/tmp/{filename}" + # One step in a private temporary directory: a fixed /tmp name could be planted, + # or swapped between the check and the install. + deb = f'"$tmp"/{shlex.quote(filename)}' return [ Command( 'Ensure curl is available', "command -v curl >/dev/null 2>&1 || (apt-get update && DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::=--force-confold -o DPkg::Lock::Timeout=600 install curl)", ), Command( - tf('Download patched wkhtmltopdf ({})', filename), - f"curl -fSL -o {shlex.quote(tmp)} {shlex.quote(url)}", - ), - Command( - 'Verify wkhtmltopdf SHA-256 (aborts on mismatch)', - f"echo {shlex.quote(sha256 + ' ' + tmp)} | sha256sum -c -", - ), - Command( - 'Install verified wkhtmltopdf .deb', - f"apt-get update && DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::=--force-confold -o DPkg::Lock::Timeout=600 install {shlex.quote(tmp)}", + tf('Download, verify (SHA-256) and install patched wkhtmltopdf ({})', filename), + "tmp=$(mktemp -d) && trap 'rm -rf \"$tmp\"' EXIT && " + f"curl -fSL -o {deb} {shlex.quote(url)} && " + f'echo {shlex.quote(sha256)}" "{deb} | sha256sum -c - && ' + # apt reads local packages as _apt: the verified file is made readable. + f'chmod 755 "$tmp" && chmod 644 {deb} && ' + "apt-get update && DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::=--force-confold " + f"-o DPkg::Lock::Timeout=600 install {deb}", ), - Command('Remove downloaded wkhtmltopdf .deb', f"rm -f {shlex.quote(tmp)}"), ] diff --git a/instance_manager/prompts.py b/instance_manager/prompts.py index 032c27d..b543045 100644 --- a/instance_manager/prompts.py +++ b/instance_manager/prompts.py @@ -6,7 +6,7 @@ from pathlib import Path from .i18n import current_language, t, tf -from .ui import level_text, prompt_label, style, title +from .ui import level_text, prompt_label, sanitize, style, title _last_selected_dir: Path | None = None @@ -46,6 +46,12 @@ def _validate_selected_path_extension( return ask_bool('Use this file anyway?', False) +def _has_control(value: str) -> bool: + """A control character (a pasted carriage return, an escape) in a typed value + would end up in odoo.conf, a unit or a shell line as something else.""" + return any(ord(char) < 32 or ord(char) == 127 for char in value) + + def ask_text( label: str, default: str | None = None, @@ -64,7 +70,8 @@ def ask_text( return "" print(level_text("ERROR", 'Value is required.')) continue - error = validate(value) if validate else None + error = 'Control characters are not allowed.' if _has_control(value) else None + error = error or (validate(value) if validate else None) if error: print(level_text("ERROR", error)) continue @@ -92,6 +99,9 @@ def ask_secret(label: str, required: bool = True) -> str: """Prompt for a secret without echoing it to the screen (via getpass).""" while True: value = getpass.getpass(f"{prompt_label(label)}: ").strip() + if value and _has_control(value): + print(level_text("ERROR", 'Control characters are not allowed.')) + continue if value: return value if not required: @@ -142,7 +152,7 @@ def choose(label: str, options: list[str], default_index: int | None = None) -> else "" ) marker = style(f"{index})", "blue", "bold") - print(f" {marker} {t(option)}{style(default_tag, 'dim')}") + print(f" {marker} {sanitize(t(option))}{style(default_tag, 'dim')}") while True: raw = input(f"{prompt_label('Select an option')}: ").strip() @@ -176,8 +186,8 @@ def select_file_path( while True: if requested_label: - print(f"\n{title('Select required file')}: {requested_label}") - print(f"{title('Current directory')}: {current}") + print(f"\n{title('Select required file')}: {sanitize(t(requested_label))}") + print(f"{title('Current directory')}: {sanitize(str(current))}") if _last_selected_dir and _last_selected_dir.is_dir(): print(level_text("INFO", tf('Last folder used: {}', _last_selected_dir))) entries = sorted( @@ -188,7 +198,7 @@ def select_file_path( print(t(' q) Cancel')) for index, entry in enumerate(entries, start=1): marker = "/" if entry.is_dir() else "" - print(f" {index}) {entry.name}{marker}") + print(f" {index}) {sanitize(entry.name)}{marker}") raw = input(f"{prompt_label("Choose a number, '..', 'q' or a manual path")}: ").strip() if raw.lower() in {"q", "cancelar"}: diff --git a/instance_manager/system.py b/instance_manager/system.py index 2addd64..0d91bdd 100644 --- a/instance_manager/system.py +++ b/instance_manager/system.py @@ -9,7 +9,7 @@ from dataclasses import dataclass, field from .i18n import t, tf -from .ui import level_text, style, title, wrap_plain_block +from .ui import level_text, sanitize, style, title, wrap_plain_block @dataclass @@ -41,8 +41,10 @@ def pg_env(password: str) -> dict[str, str]: def mask(text: str, secrets: tuple[str, ...] | list[str]) -> str: - """``text`` with every non-empty secret replaced by ``********``.""" - for secret in sorted((s for s in secrets if s), key=len, reverse=True): + """``text`` with every non-empty secret replaced by ``********``, also where it + appears escaped: inside an SQL literal (``'`` doubled) or a shell word.""" + forms = {form for s in secrets if s for form in (s, s.replace("'", "''"), shlex.quote(s))} + for secret in sorted(forms, key=len, reverse=True): text = text.replace(secret, "********") return text @@ -63,12 +65,8 @@ def run( env=_environment(env), ) if check and result.returncode != 0: - raise RuntimeError( - f"Command failed: {command}\n" - f"exit={result.returncode}\n" - f"stdout={result.stdout}\n" - f"stderr={result.stderr}" - ) + # Not the command: it may hold what a preview masks. + raise RuntimeError(f"Command failed (exit {result.returncode}): {result.stderr.strip()}") return result @@ -94,7 +92,7 @@ def run_streaming(command: str, env: dict[str, str] | None = None) -> subprocess captured: list[str] = [] assert process.stdout is not None for line in process.stdout: - print(line, end="", flush=True) + print(sanitize(line, keep_cr=True), end="", flush=True) captured.append(line) process.stdout.close() returncode = process.wait() @@ -184,14 +182,14 @@ def preview_commands(commands: list[Command]) -> None: indent = " " body_width = max(20, shutil.get_terminal_size((100, 24)).columns - len(indent)) for index, item in enumerate(commands, start=1): - print(f"\n{style(f'[{index:02d}]', 'blue', 'bold')} {t(item.description)}") - for chunk in wrap_plain_block(item.shown, body_width): + print(f"\n{style(f'[{index:02d}]', 'blue', 'bold')} {sanitize(t(item.description))}") + for chunk in wrap_plain_block(sanitize(item.shown), body_width): print(style(f"{indent}{chunk}", "dim")) def apply_commands(commands: list[Command], stop_on_error: bool = True) -> None: for index, item in enumerate(commands, start=1): - print(f"\n{style(f'[{index}/{len(commands)}]', 'blue', 'bold')} {t(item.description)}") + print(f"\n{style(f'[{index}/{len(commands)}]', 'blue', 'bold')} {sanitize(t(item.description))}") # Stream output live so long steps (apt/pip/pg_restore) aren't silent. result = run_streaming(item.command, item.env) if result.returncode != 0: diff --git a/instance_manager/ui.py b/instance_manager/ui.py index 8dfb7ea..5c6207c 100644 --- a/instance_manager/ui.py +++ b/instance_manager/ui.py @@ -38,6 +38,20 @@ def supports_color() -> bool: return True +# What a host string (a database or file name, a command's output) must not send to +# the terminal: every control character but tab and newline, and any escape +# sequence except a colour (SGR). An OSC or CSI sequence can retitle the window, +# write the clipboard, or move the cursor to hide what a plan really does. +_UNSAFE_RE = re.compile(r"\x1b(?!\[[0-9;]*m)|[\x00-\x08\x0b-\x1a\x1c-\x1f\x7f-\x9f]") +_UNSAFE_KEEP_CR_RE = re.compile(r"\x1b(?!\[[0-9;]*m)|[\x00-\x08\x0b\x0c\x0e-\x1a\x1c-\x1f\x7f-\x9f]") + + +def sanitize(text: str, keep_cr: bool = False) -> str: + """``text`` safe to print: unsafe controls shown as ``?``. ``keep_cr`` keeps + carriage returns, which progress bars in a command's output use.""" + return (_UNSAFE_KEEP_CR_RE if keep_cr else _UNSAFE_RE).sub("?", text) + + def style(text: str, *tokens: str) -> str: if not supports_color() or not tokens: return text @@ -49,7 +63,7 @@ def style(text: str, *tokens: str) -> str: def level_text(level: str, message: str) -> str: - message = t(message) + message = sanitize(t(message)) token_map = { "INFO": ("blue",), "WARN": ("yellow", "bold"), @@ -71,7 +85,7 @@ def prompt_label(label: str) -> str: def title(text: str) -> str: - return style(t(text), "bold") + return style(sanitize(t(text)), "bold") _ANSI_RE = re.compile(r"\x1b\[[0-9;]*m") @@ -139,8 +153,8 @@ def _wrap_cell(cell: str, cap: int) -> list[str]: def render_table( headers: list[str], rows: list[list[str]], max_width: int | None = None ) -> str: - safe_headers = [t(str(item)) for item in headers] - safe_rows = [[t(str(cell)) for cell in row] for row in rows] + safe_headers = [sanitize(t(str(item))) for item in headers] + safe_rows = [[sanitize(t(str(cell))) for cell in row] for row in rows] column_count = len(safe_headers) if column_count == 0: return "" diff --git a/instance_manager/workflows/addons.py b/instance_manager/workflows/addons.py index 63efe31..25d8361 100644 --- a/instance_manager/workflows/addons.py +++ b/instance_manager/workflows/addons.py @@ -12,7 +12,7 @@ from ..prompts import ask_bool, ask_text from ..system import pg_env, read_odoo_conf, run from ..ui import level_tag, level_text, render_table, strip_ansi, title -from .common import DbCredentials, _ask_db_credentials, _quote +from .common import DbCredentials, _ask_db_credentials, _quote, _write_export _VERSION_RE = re.compile(r"""["']version["']\s*:\s*["']([^"']+)["']""") @@ -268,9 +268,7 @@ def _maybe_export_inventory(config: InstanceConfig, sections: list[str]) -> None header = tf('Addon inventory: {}', config.instance) try: - os.makedirs(os.path.dirname(export_path) or ".", exist_ok=True) - with open(export_path, "w", encoding="utf-8") as file_handle: - file_handle.write(f"{header}\n\n" + "\n\n".join(sections) + "\n") + _write_export(export_path, f"{header}\n\n" + "\n\n".join(sections) + "\n") except OSError as error: print(level_text("ERROR", tf('Could not write {}: {}', export_path, error.strerror or error))) return diff --git a/instance_manager/workflows/backup_restore.py b/instance_manager/workflows/backup_restore.py index 58df6f5..9ca96c2 100644 --- a/instance_manager/workflows/backup_restore.py +++ b/instance_manager/workflows/backup_restore.py @@ -18,6 +18,7 @@ from ..planners import ( BACKUP_DUMP_SUFFIX, BACKUP_FILESTORE_SUFFIX, + _as_instance_user, _is_local_db_host, backup_basename, plan_ensure_db_role, @@ -113,6 +114,7 @@ def _post_db_mode_commands( neutralize: bool, local_url: str, env: dict[str, str] | None = None, + role: str = "", ) -> list[Command]: """Apply Odoo migration semantics to an already-restored target database. @@ -121,14 +123,19 @@ def _post_db_mode_commands( both the credential-based restore and the local, superuser-driven duplication. Each step is one statement that stops the plan when it fails: a copy left half-neutralised must not be handed to a running Odoo. + + With ``role`` (the copy's owner), the statements run as that role with a fixed + ``search_path``: a trigger or function the source database carries then runs with + the owner's rights, never the superuser's. Names MUST be safe. """ psql = f"{psql_target} -X -q -v ON_ERROR_STOP=1 -c" + prefix = f'SET ROLE "{role}"; SET search_path = pg_catalog, public; ' if role else "" commands: list[Command] = [] if migration_mode == 'Copied (new UUID on target)': commands.append( Command( 'Give the copy its own identity: new database.uuid, database.secret, create date', - f"{psql} {_quote(neutralise.copied_identity_sql())}", + f"{psql} {_quote(prefix + neutralise.copied_identity_sql())}", env=dict(env or {}), ) ) @@ -136,12 +143,12 @@ def _post_db_mode_commands( commands += [ Command( 'Neutralise the copy (crons, mail, payment, EDI/SII, calendars, webhooks, IAP, base URL)', - f"{psql} {_quote(neutralise.apply_sql(local_url))}", + f"{psql} {_quote(prefix + neutralise.apply_sql(local_url))}", env=dict(env or {}), ), Command( 'Check nothing in the copy can still act on the outside', - f"{psql} {_quote(neutralise.guard_sql(local_url))}", + f"{psql} {_quote(prefix + neutralise.guard_sql(local_url))}", env=dict(env or {}), ), ] @@ -319,8 +326,12 @@ def _backup_instance( f"umask 077 && TMP={_quote(archive_path + '.partial')} && " f"test -d {_quote(filestore_dir)} && " # GNU tar exits 1 when a file changed while read (Odoo keeps - # writing attachments); anything above 1 is a real failure. - f"{{ tar --warning=no-file-changed -czf \"$TMP\" -C {_quote(filestore_dir)} . || [ $? -eq 1 ]; }} && " + # writing attachments); anything above 1 is a real failure. tar + # reads as the instance user, so a link it planted reaches nothing + # root alone could read; root writes the archive. + "{ " + _as_instance_user( + config, f"tar --warning=no-file-changed -czf - -C {_quote(filestore_dir)} ." + ) + " > \"$TMP\" || [ $? -eq 1 ]; } && " f"mv \"$TMP\" {_quote(archive_path)} || {{ rm -f \"$TMP\"; exit 1; }}", ) ) @@ -435,15 +446,21 @@ def _restore_backup( f"mv -- {_quote(target_filestore)} {_quote(kept)}", ) ) + # Extracted as the instance user, root only reading the archive: an archive + # or a path the instance controls can write nowhere the instance cannot. commands.append( Command( - 'Create the filestore base path', f"mkdir -p {_quote(target_filestore)}" + 'Create the filestore base path', + _as_instance_user(config, f"mkdir -p {_quote(target_filestore)}"), ) ) commands.append( Command( 'Restore the filestore into the target', - f"tar --no-same-owner -xzf {_quote(filestore_backup)} -C {_quote(target_filestore)}", + _as_instance_user( + config, + f"tar --no-same-owner --no-same-permissions -xzf - -C {_quote(target_filestore)}", + ) + f" < {_quote(filestore_backup)}", ) ) commands.extend(_own_filestore_commands(config, target_db)) @@ -459,25 +476,30 @@ def _restore_backup( return creds -def _detect_source_repo_branch(config: InstanceConfig) -> str: - """Best-effort detection of the source instance's checked-out Odoo branch, so a - replica clones the same version.""" +def _source_git(config: InstanceConfig, args: str) -> str: + """``git `` in the source checkout, as its owner: git refuses a repository + another user owns ("dubious ownership"), and root has no reason to run the + instance's hooks or config.""" result = run( - f"git -C {_quote(config.odoo_home + '/odoo')} rev-parse --abbrev-ref HEAD 2>/dev/null", + _as_instance_user(config, f"git -C {_quote(config.odoo_home + '/odoo')} {args}") + " 2>/dev/null", check=False, ) - branch = result.stdout.strip() + return result.stdout.strip() if result.returncode == 0 else "" + + +def _detect_source_repo_branch(config: InstanceConfig) -> str: + """Best-effort detection of the source instance's checked-out Odoo branch, so a + replica clones the same version.""" + branch = _source_git(config, "rev-parse --abbrev-ref HEAD") return branch if branch and branch != "HEAD" else "" def _detect_source_core(config: InstanceConfig) -> str: """``ocb`` when the source checkout's origin is OCA's OCB, else ``odoo``, so a replica runs the same core.""" - result = run( - f"git -C {_quote(config.odoo_home + '/odoo')} remote get-url origin 2>/dev/null", - check=False, - ) - return "ocb" if "/oca/ocb" in result.stdout.strip().lower() else "odoo" + origin = _source_git(config, "remote get-url origin").lower() + # https://github.com/OCA/OCB(.git) or git@github.com:OCA/OCB.git + return "ocb" if re.search(r"[/:]oca/ocb(\.git)?/?$", origin) else "odoo" def _nginx_server_name_in_use(domain: str, directory: str = "/etc/nginx/sites-enabled") -> bool: @@ -550,14 +572,20 @@ def _replicate_venv_packages_command( target as the target user.""" source_pip = f"{source_config.odoo_home}/venv/bin/pip" target_pip = f"{target_config.odoo_home}/venv/bin/pip" - reqs = f"/tmp/{target_config.instance}_venv_reqs.txt" - script = ( - f"sudo -u {_quote(source_config.odoo_user)} {_quote(source_pip)} freeze 2>/dev/null " - f"| grep -E '^[A-Za-z0-9_.-]+==' > {_quote(reqs)} || true; " - f"if [ -s {_quote(reqs)} ]; then " - f"sudo -u {_quote(target_config.odoo_user)} {_quote(target_pip)} install -r {_quote(reqs)}; " - f"fi; rm -f {_quote(reqs)}" - ) + # A private temporary directory (never a fixed /tmp name another user could + # plant), only exact `name==version` lines, shown before they are installed; + # the step fails when the freeze or the install does. + script = "\n".join([ + "set -eo pipefail", + 'tmp=$(mktemp -d)', + 'trap \'rm -rf "$tmp"\' EXIT', + _as_instance_user(source_config, f"{_quote(source_pip)} freeze --exclude-editable") + ' > "$tmp/freeze"', + "grep -E '^[A-Za-z0-9][A-Za-z0-9_.-]*==[A-Za-z0-9_.+!-]+$' \"$tmp/freeze\" > \"$tmp/reqs\" || true", + 'if [ -s "$tmp/reqs" ]; then', + ' echo "Packages to install:"; cat "$tmp/reqs"', + " " + _as_instance_user(target_config, f"{_quote(target_pip)} install -r /dev/stdin") + ' < "$tmp/reqs"', + "fi", + ]) return Command('Replicate the source venv Python packages', script) @@ -636,7 +664,7 @@ def _plan_refresh_target( commands.extend(_seed_db_commands(source_db, target_db, target_owner, method)) commands.extend(_post_db_mode_commands( _psql_target_local(target_db), migration_mode, neutralize, - _local_url(_config_with_port(target_config, existing)), + _local_url(_config_with_port(target_config, existing)), role=target_owner, )) commands.extend(_hand_over_commands(target_db, target_owner)) if duplicate_filestore: @@ -753,7 +781,8 @@ def _plan_replica_target( _filestore_copy_commands(source_config, source_db, target_config, target_db) ) commands.extend(_post_db_mode_commands( - _psql_target_local(target_db), migration_mode, neutralize, _local_url(target_config) + _psql_target_local(target_db), migration_mode, neutralize, _local_url(target_config), + role=target_db, )) commands.extend(_hand_over_commands(target_db, target_db)) commands.append( @@ -843,7 +872,7 @@ def _duplicate_database( commands.extend(_seed_db_commands(source_db, target_db, target_owner, method)) commands.extend(_post_db_mode_commands( _psql_target_local(target_db), migration_mode, neutralize, - _local_url(_config_with_port(config, existing)), + _local_url(_config_with_port(config, existing)), role=target_owner, )) commands.extend(_hand_over_commands(target_db, target_owner)) if duplicate_filestore: diff --git a/instance_manager/workflows/common.py b/instance_manager/workflows/common.py index 8dc2529..ff1e531 100644 --- a/instance_manager/workflows/common.py +++ b/instance_manager/workflows/common.py @@ -238,6 +238,16 @@ def _backup_dir_error(path: str) -> str | None: return None +def _write_export(path: str, text: str) -> None: + """Write a report the operator asked for, as root, without following a link: the + file must be new (``O_EXCL``, which also refuses a link in its place) and is + private (``600``) — reports name databases, users and paths.""" + os.makedirs(os.path.dirname(path) or ".", exist_ok=True) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as file_handle: + file_handle.write(text) + + def _existing_instance_artifacts(config: InstanceConfig) -> list[str]: """What already exists under this instance's names: an install must not take them over, and its cleanup could otherwise remove them.""" diff --git a/instance_manager/workflows/manage.py b/instance_manager/workflows/manage.py index 85913da..9a5ab67 100644 --- a/instance_manager/workflows/manage.py +++ b/instance_manager/workflows/manage.py @@ -3,10 +3,12 @@ from __future__ import annotations import datetime +import re from ..i18n import t, tf from ..models import InstanceConfig, db_host_error, domain_error, is_valid_db_name from ..planners import ( + _as_instance_user, plan_nginx_http, plan_nginx_https, plan_remove_scheduled_backup, @@ -240,8 +242,28 @@ def _repair_instance_nginx_logs(config: InstanceConfig) -> None: _execute_plan(commands) +# A new requirement starts after a comma only where a name follows: `requests, lxml` +# is two, `babel>=2.14,<3` is one (its comma joins two version clauses). +_REQUIREMENT_SPLIT = re.compile(r",\s*(?=[A-Za-z0-9])") + + +def _split_requirements(chunks: list[str]) -> tuple[list[str], list[str]]: + """Requirements typed by the operator, and the ones refused: an option (``-…``, + such as ``--index-url``) or a control character is not a package.""" + packages: list[str] = [] + refused: list[str] = [] + for chunk in chunks: + for item in (part.strip() for part in _REQUIREMENT_SPLIT.split(chunk)): + if not item: + continue + if item.startswith("-") or any(ord(char) < 32 for char in item): + refused.append(item) + else: + packages.append(item) + return packages, refused + + def _install_python_packages_in_instance_venv(config: InstanceConfig) -> None: - venv_activate = f"{config.odoo_home}/venv/bin/activate" venv_pip = f"{config.odoo_home}/venv/bin/pip" print(f"\n{title('Install Python packages in the instance venv')}") @@ -279,10 +301,13 @@ def _install_python_packages_in_instance_venv(config: InstanceConfig) -> None: f"test -f {_quote(req_path)}", ) ) + # Root reads the file (the instance user may not reach it) and pip, run as the + # instance user, reads it on stdin. commands.append( Command( 'Install packages from requirements into the venv', - f"sudo -u {_quote(config.odoo_user)} bash -lc \"source {_quote(venv_activate)} && pip install -r {_quote(req_path)}\"", + _as_instance_user(config, f"{_quote(venv_pip)} install -r /dev/stdin") + + f" < {_quote(req_path)}", ) ) else: @@ -312,11 +337,9 @@ def _install_python_packages_in_instance_venv(config: InstanceConfig) -> None: chunks.append(raw_packages) chunks.extend(extra_lines) - packages: list[str] = [] - for chunk in chunks: - parts = [item.strip() for item in chunk.split(",") if item.strip()] - packages.extend(parts) - + packages, refused = _split_requirements(chunks) + if refused: + print(level_text("WARN", tf('Ignored (not a package): {}', ", ".join(refused)))) if not packages: print(level_text("WARN", 'No valid packages detected. Operation cancelled.')) return @@ -325,14 +348,14 @@ def _install_python_packages_in_instance_venv(config: InstanceConfig) -> None: commands.append( Command( 'Install packages into the venv', - f"sudo -u {_quote(config.odoo_user)} bash -lc \"source {_quote(venv_activate)} && pip install {package_args}\"", + _as_instance_user(config, f"{_quote(venv_pip)} install {package_args}"), ) ) commands.append( Command( 'Show installed packages (summary)', - f"sudo -u {_quote(config.odoo_user)} bash -lc \"source {_quote(venv_activate)} && pip list\"", + _as_instance_user(config, f"{_quote(venv_pip)} list"), ) ) diff --git a/instance_manager/workflows/report.py b/instance_manager/workflows/report.py index 7aeb583..2b67034 100644 --- a/instance_manager/workflows/report.py +++ b/instance_manager/workflows/report.py @@ -35,6 +35,7 @@ _odoo_conf_candidates, _quote, _read_text_file, + _write_export, ) @@ -518,6 +519,20 @@ def _nginx_matches_instance( return False +def _venv_python_version(python_path: str) -> str: + """The Python of the venv holding ``python_path``, read from its ``pyvenv.cfg``. + Read, not run: the interpreter belongs to the instance user, and the report runs + as root.""" + if not python_path: + return "" + venv = os.path.dirname(os.path.dirname(python_path)) + for line in _read_text_file(os.path.join(venv, "pyvenv.cfg")).splitlines(): + key, _, value = line.partition("=") + if key.strip() in {"version", "version_info"} and re.fullmatch(r"[0-9.]+(\.final\.0)?", value.strip()): + return f"Python {value.strip().removesuffix('.final.0')}" + return "" + + def _detect_odoo_release_version(odoo_home: str) -> str: if not odoo_home: return "" @@ -649,7 +664,7 @@ def _collect_external_instance_rows( context = service_context_by_name.get(config.odoo_service, {}) python_path = context.get("python_path", "") - python_version = _command_output(f"{_quote(python_path)} --version") if python_path else "" + python_version = _venv_python_version(python_path) odoo_home = context.get("odoo_home", "") or config.odoo_home odoo_version = _detect_odoo_release_version(odoo_home) @@ -1093,9 +1108,7 @@ def external_server_report() -> None: export_path = ask_text('Report export path', default_path, required=True) try: - os.makedirs(os.path.dirname(export_path) or ".", exist_ok=True) - with open(export_path, "w", encoding="utf-8") as file_handle: - file_handle.write("\n\n".join(report_sections) + "\n") + _write_export(export_path, "\n\n".join(report_sections) + "\n") except OSError as error: print(level_text("ERROR", tf('Could not write {}: {}', export_path, error.strerror or error))) return diff --git a/instance_manager/workflows/services.py b/instance_manager/workflows/services.py index e6576cf..59cc95e 100644 --- a/instance_manager/workflows/services.py +++ b/instance_manager/workflows/services.py @@ -2,6 +2,8 @@ from __future__ import annotations +import re + from ..i18n import tf from ..models import InstanceConfig from ..prompts import ask_text, choose @@ -16,6 +18,12 @@ from .common import _execute_plan, _quote +def _unit_name_error(name: str) -> str | None: + if re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9@._:-]{0,254}", name): + return None + return 'Invalid service name. Use letters, digits and @ . _ : -' + + def _list_existing_instance_services() -> list[str]: services: list[str] = [] for instance in list_instances(InstanceConfig.base_instances_dir): @@ -63,11 +71,11 @@ def manage_instance_services() -> None: if pick in {"", 'Cancel'}: continue if pick == 'Type a name': - service_name = ask_text('Service name', "", required=True) + service_name = ask_text('Service name', "", required=True, validate=_unit_name_error) else: service_name = pick else: - service_name = ask_text('Service name', "", required=True) + service_name = ask_text('Service name', "", required=True, validate=_unit_name_error) service_action = choose( tf('Action for service {}', service_name), @@ -85,14 +93,14 @@ def manage_instance_services() -> None: continue if service_action == 'Start': - commands = [Command(tf('Start service {}', service_name), f"systemctl start {_quote(service_name)}")] + commands = [Command(tf('Start service {}', service_name), f"systemctl start -- {_quote(service_name)}")] elif service_action == 'Stop': - commands = [Command(tf('Stop service {}', service_name), f"systemctl stop {_quote(service_name)}")] + commands = [Command(tf('Stop service {}', service_name), f"systemctl stop -- {_quote(service_name)}")] elif service_action == 'Restart': - commands = [Command(tf('Restart service {}', service_name), f"systemctl restart {_quote(service_name)}")] + commands = [Command(tf('Restart service {}', service_name), f"systemctl restart -- {_quote(service_name)}")] elif service_action == 'Enable autostart': - commands = [Command(tf('Enable autostart {}', service_name), f"systemctl enable {_quote(service_name)}")] + commands = [Command(tf('Enable autostart {}', service_name), f"systemctl enable -- {_quote(service_name)}")] else: - commands = [Command(tf('Disable autostart {}', service_name), f"systemctl disable {_quote(service_name)}")] + commands = [Command(tf('Disable autostart {}', service_name), f"systemctl disable -- {_quote(service_name)}")] _execute_plan(commands) diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/proposal.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/proposal.md new file mode 100644 index 0000000..d31b7ad --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/proposal.md @@ -0,0 +1,54 @@ +# Least privilege and quoting for what reaches root + +## Why + +The second audit ran the generated steps with hostile inputs: + +- **Certificates:** the operator's file paths went into the root shell in single quotes, so a file named + `x'$(…)'.crt` ran its name. The files also replaced the live certificate before the key was checked, so a + wrong key broke the next nginx restart for every instance on the host. +- **Venv packages:** `sudo -u bash -lc "… pip install ''"` was expanded by the outer root shell + first, so a package typed as `$(…)` ran as root. Splitting on commas also broke `babel>=2.14,<3`. +- **Report:** the server report ran the instance-owned venv interpreter (`python3 --version`) as root. +- **Neutralisation:** it ran as the PostgreSQL superuser on a database copied from another instance, so a + trigger in that database ran with superuser rights. +- **Fixed `/tmp` names, written as root:** the venv replication used `/tmp/_venv_reqs.txt`, which a local + user could plant. The wkhtmltopdf `.deb` was downloaded, checked and installed in separate steps. +- **Filestore archives:** tar ran as root inside instance-controlled directories, both to back up and to + restore. +- **Terminal:** escape sequences in database names, file names or command output reached the terminal, and could + rewrite what the preview showed. +- **Smaller ones:** + - a secret escaped inside an SQL literal was not masked; + - a password containing `$$` broke the role SQL; + - `git` ran as root in the instance checkout, which modern git refuses ("dubious ownership"), so a replica + never detected its source's branch; + - `--` was missing before typed unit names; + - a typed carriage return reached `odoo.conf`; + - exports followed links. + +## What changes + +- **Certificates:** the operator's files are staged, checked and then moved into place, quoted, with the old + files kept as `.previous`. +- **pip:** runs as the instance user with no login shell. Root only reads the requirements file and passes it on + stdin. Options are refused, and requirements are split on commas only before a package name. +- **Report:** reads `pyvenv.cfg` instead of running the interpreter. +- **Neutralisation and identity:** run as the copy's owner, with `SET ROLE` and a fixed `search_path`. +- **Temporary files:** a private `mktemp -d` holds the venv replication's requirements and the wkhtmltopdf + package. The replication keeps only exact `name==version` lines, shows them, and fails when pip fails. +- **tar and git:** tar runs as the instance user, both for backups (manual and scheduled) and for restores; git + runs as the instance user too. +- **Terminal:** host text is sanitised before it is printed, colours excepted. Typed values must not contain + control characters. +- **Smaller ones:** `mask` covers escaped forms, the role SQL uses a unique dollar tag, typed unit names go after + `--`, and exports are new private files that never follow a link. + +## Impact + +- Specs: `web-proxy-tls`, `instance-configuration`, `instance-provisioning`, `data-backup-restore`, + `execution-safety`, `server-audit`, `addon-inventory`. +- Code: `planners.py`, `system.py`, `ui.py`, `prompts.py`, `workflows/{manage,backup_restore,report,addons, + services,common}.py`. +- Tests: `tests/test_injection_privilege.py`. A new `tools/verify_privilege.py`, which runs in CI, executes the + certificate, wkhtmltopdf, venv-replication and owner-role neutralisation steps. diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/addon-inventory/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/addon-inventory/spec.md new file mode 100644 index 0000000..dfef225 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/addon-inventory/spec.md @@ -0,0 +1,19 @@ +## MODIFIED Requirements + +### Requirement: Optional inventory export + +After rendering the inventory, the tool SHALL offer to export it to a single text file at an operator-chosen +path (defaulting under `./reports/`), mirroring the server-audit report export. The exported content reflects +the active installed/all filter. Declining writes nothing. The file SHALL be new (an existing file, or a link in +its place, is refused) and private (`600`): the export runs as root. + +#### Scenario: Inventory is exported on request + +- **WHEN** the operator opts to export the inventory +- **THEN** the tool writes the rendered grouped tables (with the active filter) to the chosen file, creating + the parent directory if needed + +#### Scenario: Export can be declined + +- **WHEN** the operator declines the export +- **THEN** no file is written diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md new file mode 100644 index 0000000..d5641a8 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md @@ -0,0 +1,234 @@ +## MODIFIED Requirements + +### Requirement: Copied vs moved database semantics + +Restore and duplication SHALL apply Odoo migration semantics: in "copied" mode give the target its own +`database.uuid`, `database.secret` and `database.create_date`, as Odoo's own copy does; when neutralisation is +requested, apply every rule of the neutralisation catalogue (Odoo's `neutralize.sql` 16.0-19.0, extended to +12.0-19.0 and the OCA modules it lists), each guarded by the existence of its table and columns, as one statement +that stops the plan on failure, and then verify that nothing can still act on the outside, failing the plan when +something can. On the local server these statements SHALL run as the copy's owner role with +`search_path = pg_catalog, public`, never as the superuser: a trigger or function the source database carries +then runs with that role's rights. The neutralisation SHALL leave exactly one active outgoing mail server, pointing at a host that +does not resolve, so Odoo never falls back to the `smtp_server` of `odoo.conf`, and SHALL drop production's SMTP +credentials from the copy. + +#### Scenario: Copied mode regenerates the database UUID + +- **WHEN** the operator selects the copied mode +- **THEN** the plan writes a fresh `database.uuid`, `database.secret` and `database.create_date` into + `ir_config_parameter` on the target database + +#### Scenario: Neutralization deactivates automation in the target + +- **WHEN** the operator opts to neutralize the target +- **THEN** the plan deactivates crons (except Odoo's autovacuum and queue_job's cleanup), mail servers (dropping + their credentials), fetchmail, payment providers, external carriers and their production mode, OAuth + providers, calendar tokens, webhooks, IAP tokens, EDI/SII/TicketBAI/Peppol production modes and queued jobs, + points `web.base.url` at the target, adds the mail sink, and sets `database.is_neutralized` + +#### Scenario: A failed or incomplete neutralisation stops the plan + +- **WHEN** the neutralisation statement fails, or the check finds something that can still act on the outside +- **THEN** the step fails and the plan stops; no step tolerates its own failure + +#### Scenario: Code in the copy runs with its owner's rights + +- **WHEN** the copied database has a trigger that tries to make its owner a superuser +- **THEN** the trigger fails for lack of privilege, the step fails, and the role is unchanged + +### Requirement: Duplication + +The tool SHALL duplicate a source instance's database — and optionally its filestore — into a target, +end-to-end and existence-aware, requiring phrase confirmation and validating the source and target database +names as safe before using them in SQL. The target instance SHALL differ from the source instance, and the +target database SHALL differ from the source database. + +The operator SHALL choose the database copy method: a fast PostgreSQL **template** copy, or a robust +**`pg_dump | pg_restore --no-owner`** that reassigns ownership to the target role. A template copy keeps the +source's role as owner of every table, so the tool SHALL refuse it when the target role differs from the source +database's owner (always the case for a new replica). The template copy SHALL refuse new connections to the +source, terminate its sessions, copy, and reopen the source whatever the outcome. + +When the **target instance does not exist**, duplication SHALL provision it fully before seeding: create the +target role, run the base setup (system user, home, Odoo checkout at the source's version, virtualenv, +`odoo.conf`, systemd unit) **without starting the service**, optionally configure Nginx, seed the database and +filestore, then start the service. The replica SHALL follow the **same production-hardening prompts as a fresh +install** (secrets with informed choice, `list_db`, `dbfilter`, worker sizing, `db_sslmode`, wkhtmltopdf) with +auto-suggested non-colliding internal ports, and — when it fronts Nginx — SHALL require a domain that is **not +already served by another vhost** (or the replica would be unreachable behind a shared `server_name`). It SHALL +also offer to **replicate the source venv's Python packages** into the target venv, so addon dependencies the +source installed beyond `requirements.txt` are present in the replica. + +When the **target instance already exists**, duplication SHALL refresh it in place: stop the target service, +drop and recreate its database from the source and replace its filestore, apply the migration semantics, and +restart — **without** recreating the target's config, service, or system user. It SHALL drop an existing +target database only when that database belongs to the target's own role (read from its `odoo.conf`) and the +operator explicitly confirms the overwrite. + +Migration semantics (copied vs moved, neutralize) SHALL apply in both cases, and the duplicated filestore SHALL +be placed under the **target** instance's data directory — the one its configuration names, also for a replica +whose `odoo.conf` is not written yet — and owned by the target system user (the whole data dir when it is the +target's own, only the copied filestore otherwise). A filestore already at the target SHALL be moved aside to +`.replaced-`, never deleted, and the copy SHALL run only when the source filestore exists. +A target that is not a whole instance of this tool (home and `odoo.conf`) SHALL be provisioned only under a free +name, and never refreshed. Every database the tool seeds SHALL have its access +**restricted to its owner** — `CONNECT` revoked from `PUBLIC` and granted to the owning role — so an instance's +database role cannot reach other instances' databases. Local drops SHALL use `dropdb --force`, which closes the +database's sessions and refuses new ones in one statement. + +#### Scenario: Replica replicates the source venv Python packages + +- **WHEN** a replica is provisioned and the operator opts to replicate packages +- **THEN** the plan installs the source venv's packages into the target venv, so the replica has the same addon + Python dependencies as the source: the source's `pip freeze` runs as the source user, only exact + `name==version` lines are kept (in a private temporary directory) and shown, the install runs as the target + user, and the step fails when either fails + +#### Scenario: New target is provisioned and seeded as a replica + +- **WHEN** the target instance does not exist +- **THEN** the plan creates the target role, runs the same production-hardening prompts as a fresh install with + auto-suggested non-colliding ports, provisions the target (base setup without starting, optional Nginx), + seeds the database and filestore from the source with the `pg_dump` copy, and then starts the target service + +#### Scenario: Replica domain must not collide with another vhost + +- **WHEN** the replica is configured to front Nginx and the chosen domain is already a `server_name` in an + enabled vhost +- **THEN** the tool rejects it and re-prompts for a different domain, so the replica is reachable + +#### Scenario: Existing target is refreshed in place + +- **WHEN** the target instance already exists +- **THEN** the plan stops the target service, drops and recreates its database from the source, replaces its + filestore, applies the migration semantics, and restarts the service without recreating its config or unit + +#### Scenario: A database that is not the target's is never dropped + +- **WHEN** the target database exists and belongs to a role other than the target's, or the target equals the + source instance or the source database +- **THEN** the tool refuses with a descriptive error and builds no plan + +#### Scenario: Overwriting the target's database is confirmed + +- **WHEN** the target database exists and belongs to the target's role +- **THEN** the operator must explicitly confirm that it will be dropped and replaced before the plan is built + +#### Scenario: Operator selects the database copy method + +- **WHEN** the duplication plan is assembled +- **THEN** the operator chooses a template copy or a `pg_dump | pg_restore --no-owner` copy, and the plan uses + the selected method, refusing a template copy across roles + +#### Scenario: Template copy frees the source; dump copy leaves it untouched + +- **WHEN** the template method is chosen +- **THEN** the plan blocks and terminates the source's sessions before the copy and re-enables them afterward + even on failure; **WHEN** the dump method is chosen, the source is read live without terminating its sessions + +#### Scenario: Migration semantics and phrase confirmation + +- **WHEN** duplication runs +- **THEN** copied/moved and neutralize semantics are applied to the target and execution proceeds only after + the operator types the exact `DUPLICATE ` phrase for a replica, or `REPLACE ` for a refresh, + whose data it replaces + +#### Scenario: Seeded database is restricted to its owner + +- **WHEN** the tool seeds a target database +- **THEN** the plan revokes `CONNECT` on that database from `PUBLIC` and grants it to the owning role, so other + instances' roles cannot connect to it + +#### Scenario: Target data dir is owned by the target user + +- **WHEN** the filestore is copied into the target data directory (created as root) +- **THEN** the plan chowns the target's own data directory to the target system user, so Odoo can create its + `sessions` and `filestore` entries; in a data directory that may be shared, only the copied filestore + +#### Scenario: A previous target filestore is moved aside + +- **WHEN** the target filestore already exists +- **THEN** it is moved to `.replaced-` and the copy lands in the target, not nested in it + +#### Scenario: Unsafe database name is rejected + +- **WHEN** the source or target database name is not a safe name +- **THEN** the plan is not built and the operation reports the problem instead of interpolating it into SQL + +### Requirement: Backup + +The tool SHALL back up an instance's database (as a compressed custom-format dump) and/or its filestore (as a +gzipped tar) into the chosen backup directory, using a **single timestamp for the whole operation**, naming +each artifact `----`, and writing each artifact **atomically** (a temporary file +promoted only on success). The backup directory SHALL be private to root (`700`) and every artifact SHALL be +written private (`umask 077`): a dump holds password hashes, API keys and mail/payment secrets. The backup +directory SHALL be an absolute, dedicated directory: a shared system directory (`/`, `/tmp`, `/var/backups`, +`/etc`, `/opt/odoo`, …) SHALL be refused, since the tool makes the directory private and prunes in it. + +#### Scenario: Database backup produces an atomic timestamped custom dump + +- **WHEN** the operator selects a backup that includes the database +- **THEN** the plan runs `pg_dump -Fc` to a temporary file and renames it to + `/----.dump` only on success, removing the temporary file and failing + the step otherwise + +#### Scenario: Filestore backup archives the resolved filestore path atomically + +- **WHEN** the operator selects a backup that includes the filestore +- **THEN** the plan tars the resolved filestore directory to a temporary file and renames it to + `/----.filestore.tar.gz` only on success; a file that changed while + being read (GNU tar exit 1) does not fail the archive, any other tar failure does. tar reads as the instance + user and root writes the archive, so a link the instance planted reaches nothing only root could read + +#### Scenario: DB dump and filestore archive share one timestamp + +- **WHEN** the operator selects a "DB + Filestore" backup +- **THEN** the `.dump` and `.filestore.tar.gz` names carry the **same** timestamp so the pair can be matched + +#### Scenario: Backups are private + +- **WHEN** a backup is written +- **THEN** the backup directory is mode `700` and the artifacts are readable by root only + +#### Scenario: A shared directory is not a backup directory + +- **WHEN** the operator gives `/tmp` or `/var/backups` as the backup directory +- **THEN** the tool refuses it and asks for a dedicated one, such as `/var/backups/` + +### Requirement: Restore + +Restoring SHALL create the target database from a selected dump and/or restore a filestore archive, refusing to +clobber an existing target database, and require phrase confirmation before applying. The pre-restore +existence check for the target database is evaluated against the **local** PostgreSQL server. An existing +target filestore SHALL be moved aside, never deleted, and the restored files SHALL belong to the instance user. + +#### Scenario: Existing target database blocks restore + +- **WHEN** the restore includes the database and the target database already exists +- **THEN** the operation reports the conflict and stops without executing + +#### Scenario: Target-database existence check is local-only + +- **WHEN** the operator targets a remote database host for the restore +- **THEN** the pre-restore existence check runs against the local server and may not detect a remote + collision; the subsequent `createdb` still fails safely if the remote target exists + +#### Scenario: Existing target filestore requires explicit overwrite + +- **WHEN** the restore includes the filestore and the target filestore exists +- **THEN** the operator must confirm overwrite; on confirmation the previous filestore is moved to + `.replaced-` before extracting, otherwise the restore is cancelled + +#### Scenario: Restored files belong to the instance user + +- **WHEN** a filestore archive is extracted +- **THEN** tar runs as the instance user (root only reads the archive), restores neither the archive's owners + nor its permission bits, and the plan gives the files to the + instance user: the whole data dir when it is the instance's own (`/var/lib/odoo/` or inside its + home), only that database's filestore when the data dir may be shared + +#### Scenario: Restore is phrase-confirmed + +- **WHEN** the restore plan is assembled +- **THEN** it executes only after the operator types the exact `RESTORE ` phrase diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md new file mode 100644 index 0000000..da78a9d --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md @@ -0,0 +1,52 @@ +## ADDED Requirements + +### Requirement: Host text cannot drive the terminal + +Text that comes from the host — database, file and service names, a command's output, values read from files — +SHALL be printed with every control character except tab and newline, and every escape sequence except colour +(SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be +refused when it holds a control character, so a pasted carriage return or escape never reaches `odoo.conf`, a +unit, or a shell line. + +#### Scenario: A hostile database name cannot rewrite the screen + +- **WHEN** a listed database name, a file name or a step's output holds an OSC or CSI sequence +- **THEN** the sequence is printed inert, and the plan preview shows what will really run + +#### Scenario: A typed control character is refused + +- **WHEN** the operator types or pastes a value with a carriage return or an escape +- **THEN** the prompt refuses it and asks again + +## MODIFIED Requirements + +### Requirement: Secrets out of sight + +A secret — a database password, the master password, a file holding them — SHALL never be part of a plan step's +command text: it SHALL travel in the step's environment (which only root can read), a libpq client SHALL take +its password from `PGPASSWORD` there, and SQL holding a password SHALL be fed to `psql` on stdin. The plan +preview SHALL show such a step with its secrets masked — also where a secret appears escaped, as in an SQL +literal — and a failed step's error SHALL name the step rather than print its command. SQL that carries a +password inside a dollar-quoted block SHALL use a tag the password does not contain. Database probes SHALL give up connecting after 10 seconds (`PGCONNECT_TIMEOUT`). + +#### Scenario: A password is not visible to other users while a step runs + +- **WHEN** a step connects to PostgreSQL with a password +- **THEN** the password is in the step's environment and in no process's arguments + +#### Scenario: The preview masks secrets + +- **WHEN** the plan writes `odoo.conf` or sets a role's password +- **THEN** the preview shows the file or statement with `admin_passwd`, `db_password` and the role password as + `********` + +#### Scenario: Files are written atomically + +- **WHEN** a plan writes a file +- **THEN** it writes a private temporary file in the same directory, sets the mode, and renames it over the + target, so the file is never seen half-written or with a wider mode + +#### Scenario: A failure does not print the command + +- **WHEN** a step fails +- **THEN** the error names the step's description diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-configuration/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-configuration/spec.md new file mode 100644 index 0000000..24312ac --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-configuration/spec.md @@ -0,0 +1,26 @@ +## MODIFIED Requirements + +### Requirement: Virtualenv package installation + +The tool SHALL install Python packages into the instance virtualenv either from +a selected requirements file or from an operator-provided package list, running +the venv's pip directly as the instance user (no login shell). Root SHALL only read the requirements file and +pass it on stdin. A typed entry starting with `-` (a pip option such as `--index-url`) SHALL be refused, and a +comma SHALL separate two entries only when a package name follows it, so a version range such as +`babel>=2.14,<3` stays one requirement. + +#### Scenario: Install from a requirements file + +- **WHEN** the operator selects a requirements file +- **THEN** the plan validates the venv and the file, then installs the requirements into the venv as the instance user and prints the resulting package list + +#### Scenario: Install from a manual package list + +- **WHEN** the operator provides packages inline and/or one per line +- **THEN** the plan installs the parsed, non-empty package set into the venv as the instance user; if no valid package is found the operation is cancelled + +#### Scenario: A typed value is a package, not a shell word or an option + +- **WHEN** the operator types `x$(id)`, `--index-url https://…` or `babel>=2.14,<3` +- **THEN** the first is passed to pip as one quoted argument, the second is refused, and the third is one + requirement diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-provisioning/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-provisioning/spec.md new file mode 100644 index 0000000..4ac1195 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/instance-provisioning/spec.md @@ -0,0 +1,38 @@ +## MODIFIED Requirements + +### Requirement: Optional wkhtmltopdf provisioning + +During provisioning the tool SHALL offer to install `wkhtmltopdf`, explaining that Odoo's PDF reports +(invoices, quotations, and similar) require it. The recommended option SHALL install the Qt-patched +0.12.6 build selected for the detected OS codename from a pinned table (asset URL + SHA-256), verified +by checksum before installation; the tool SHALL also offer the distribution package as a clearly +labelled reduced-fidelity fallback, and a skip option. When the operator skips, the tool SHALL warn +that PDF report generation will fail until wkhtmltopdf is installed. + +#### Scenario: Operator is offered wkhtmltopdf with the reports rationale + +- **WHEN** the operator provisions an Odoo instance +- **THEN** the tool offers to install wkhtmltopdf and states that PDF reports require it + +#### Scenario: Recommended install uses the checksum-verified patched build for the codename + +- **WHEN** the operator accepts the recommended wkhtmltopdf option +- **THEN** the plan selects the patched 0.12.6 asset mapped to the detected OS codename (or the closest + ABI-compatible build when the codename has no native asset), downloads it into a private temporary + directory, and installs it only if its SHA-256 matches the pinned checksum, in the same step + +#### Scenario: Distribution package is offered as a reduced-fidelity fallback + +- **WHEN** the operator chooses the distribution package instead +- **THEN** the plan installs the distro `wkhtmltopdf`, labelled as un-patched/reduced-fidelity + +#### Scenario: Unmapped codename avoids guessing a download + +- **WHEN** the detected codename has no mapped patched asset and no ABI-compatible fallback +- **THEN** the tool does not fabricate a download URL and instead recommends the distribution package + or skipping + +#### Scenario: Skipping warns that PDF reports will fail + +- **WHEN** the operator skips wkhtmltopdf installation +- **THEN** the tool warns that PDF report generation will fail until wkhtmltopdf is installed diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/server-audit/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/server-audit/spec.md new file mode 100644 index 0000000..6e44696 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/server-audit/spec.md @@ -0,0 +1,30 @@ +## MODIFIED Requirements + +### Requirement: Read-only guarantee + +The audit SHALL be read-only with respect to server configuration: it runs discovery and inspection commands +and never builds or applies a plan that installs, configures, or deletes anything. It MAY, at the operator's +request, write a single report file and run active (read-only) TLS certificate checks. + +#### Scenario: Report never mutates server configuration + +- **WHEN** the full report runs +- **THEN** only inspection/discovery commands execute and no install, configuration, or deletion command is + produced or applied + +#### Scenario: Optional report export is operator-initiated + +- **WHEN** the operator opts to export the report +- **THEN** the tool writes a single report file to the chosen path (default under `./reports/`), which is the + only file the audit creates; the file must be new (an existing file or a link there is refused) and is + private (`600`) + +#### Scenario: Instance binaries are read, not run + +- **WHEN** the report shows an instance's Python version +- **THEN** it reads the venv's `pyvenv.cfg` instead of running the instance-owned interpreter as root + +#### Scenario: Optional active TLS checks are read-only + +- **WHEN** the operator opts into active TLS checks with an expiry threshold +- **THEN** the tool runs read-only `openssl` certificate checks and does not modify any certificate or service diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/web-proxy-tls/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/web-proxy-tls/spec.md new file mode 100644 index 0000000..8f35096 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/web-proxy-tls/spec.md @@ -0,0 +1,23 @@ +## MODIFIED Requirements + +### Requirement: Custom certificate validation + +When copying operator-supplied certificates, the plan SHALL copy them into a staging directory beside the +per-instance SSL directory, build the fullchain there, validate the key, the certificate, that the private key +matches the certificate, and the fullchain, and only then move them into place, keeping the previous files as +`.previous`. Operator-supplied paths SHALL be passed to the shell quoted. + +#### Scenario: Fullchain is built from cert and intermediate + +- **WHEN** an intermediate certificate is supplied +- **THEN** the plan concatenates the leaf certificate and the intermediate into the fullchain file; when no intermediate is supplied it uses the leaf as the fullchain + +#### Scenario: Key/certificate mismatch aborts the plan + +- **WHEN** the supplied private key's public key does not match the certificate's public key +- **THEN** the step fails before any live file is replaced, and execution stops before Nginx is reconfigured + +#### Scenario: A file name is a name + +- **WHEN** a selected file's name holds shell syntax such as `$(…)` or a quote +- **THEN** it is copied as that file and nothing in its name runs diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/tasks.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/tasks.md new file mode 100644 index 0000000..c2abd55 --- /dev/null +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/tasks.md @@ -0,0 +1,25 @@ +# Tasks + +## 1. Spec deltas + +- [x] 1.1 web-proxy-tls, instance-configuration, instance-provisioning, data-backup-restore, execution-safety, + server-audit, addon-inventory. +- [x] 1.2 `openspec validate --specs` passes. + +## 2. Code + +- [x] 2.1 Certificates staged, checked, moved; quoted paths and subject. +- [x] 2.2 Venv pip as the instance user, stdin requirements, option refusal, comma split. +- [x] 2.3 Report reads `pyvenv.cfg`; git as the instance user; OCB origin forms. +- [x] 2.4 Neutralisation and identity under `SET ROLE` with a fixed `search_path`. +- [x] 2.5 Private temporary directories (venv replication, wkhtmltopdf); tar as the instance user. +- [x] 2.6 `sanitize` for host text; control characters refused in prompts; `--` before unit names. +- [x] 2.7 `mask` escaped forms; unique dollar tag; `run(check=True)` error without the command; private + exports; `SECURITY.md`. +- [x] 2.8 i18n entries. + +## 3. Verification + +- [x] 3.1 Unit tests (`tests/test_injection_privilege.py`). +- [x] 3.2 `tools/verify_privilege.py` (CI): certificates with real openssl, wkhtmltopdf and venv replication with + stubs, owner-role neutralisation against a PostgreSQL of its own with a superuser negative control. diff --git a/openspec/specs/addon-inventory/spec.md b/openspec/specs/addon-inventory/spec.md index 13301d8..972274a 100644 --- a/openspec/specs/addon-inventory/spec.md +++ b/openspec/specs/addon-inventory/spec.md @@ -39,7 +39,8 @@ list **only installed** modules or **all** discovered modules. After rendering the inventory, the tool SHALL offer to export it to a single text file at an operator-chosen path (defaulting under `./reports/`), mirroring the server-audit report export. The exported content reflects -the active installed/all filter. Declining writes nothing. +the active installed/all filter. Declining writes nothing. The file SHALL be new (an existing file, or a link in +its place, is refused) and private (`600`): the export runs as root. #### Scenario: Inventory is exported on request diff --git a/openspec/specs/data-backup-restore/spec.md b/openspec/specs/data-backup-restore/spec.md index e497d11..4eaf86b 100644 --- a/openspec/specs/data-backup-restore/spec.md +++ b/openspec/specs/data-backup-restore/spec.md @@ -32,7 +32,8 @@ directory SHALL be an absolute, dedicated directory: a shared system directory ( - **WHEN** the operator selects a backup that includes the filestore - **THEN** the plan tars the resolved filestore directory to a temporary file and renames it to `/----.filestore.tar.gz` only on success; a file that changed while - being read (GNU tar exit 1) does not fail the archive, any other tar failure does + being read (GNU tar exit 1) does not fail the archive, any other tar failure does. tar reads as the instance + user and root writes the archive, so a link the instance planted reaches nothing only root could read #### Scenario: DB dump and filestore archive share one timestamp @@ -76,7 +77,8 @@ target filestore SHALL be moved aside, never deleted, and the restored files SHA #### Scenario: Restored files belong to the instance user - **WHEN** a filestore archive is extracted -- **THEN** tar does not restore the archive's owners (`--no-same-owner`) and the plan gives the files to the +- **THEN** tar runs as the instance user (root only reads the archive), restores neither the archive's owners + nor its permission bits, and the plan gives the files to the instance user: the whole data dir when it is the instance's own (`/var/lib/odoo/` or inside its home), only that database's filestore when the data dir may be shared @@ -128,8 +130,10 @@ database's sessions and refuses new ones in one statement. #### Scenario: Replica replicates the source venv Python packages - **WHEN** a replica is provisioned and the operator opts to replicate packages -- **THEN** the plan installs the source venv's packages (a filtered `pip freeze`) into the target venv, so the - replica has the same addon Python dependencies as the source +- **THEN** the plan installs the source venv's packages into the target venv, so the replica has the same addon + Python dependencies as the source: the source's `pip freeze` runs as the source user, only exact + `name==version` lines are kept (in a private temporary directory) and shown, the install runs as the target + user, and the step fails when either fails #### Scenario: New target is provisioned and seeded as a replica @@ -209,7 +213,9 @@ Restore and duplication SHALL apply Odoo migration semantics: in "copied" mode g requested, apply every rule of the neutralisation catalogue (Odoo's `neutralize.sql` 16.0-19.0, extended to 12.0-19.0 and the OCA modules it lists), each guarded by the existence of its table and columns, as one statement that stops the plan on failure, and then verify that nothing can still act on the outside, failing the plan when -something can. The neutralisation SHALL leave exactly one active outgoing mail server, pointing at a host that +something can. On the local server these statements SHALL run as the copy's owner role with +`search_path = pg_catalog, public`, never as the superuser: a trigger or function the source database carries +then runs with that role's rights. The neutralisation SHALL leave exactly one active outgoing mail server, pointing at a host that does not resolve, so Odoo never falls back to the `smtp_server` of `odoo.conf`, and SHALL drop production's SMTP credentials from the copy. @@ -232,6 +238,11 @@ credentials from the copy. - **WHEN** the neutralisation statement fails, or the check finds something that can still act on the outside - **THEN** the step fails and the plan stops; no step tolerates its own failure +#### Scenario: Code in the copy runs with its owner's rights + +- **WHEN** the copied database has a trigger that tries to make its owner a superuser +- **THEN** the trigger fails for lack of privilege, the step fails, and the role is unchanged + ### Requirement: Database name path safety An operator-entered database name SHALL be a name Odoo's own database manager accepts diff --git a/openspec/specs/execution-safety/spec.md b/openspec/specs/execution-safety/spec.md index 4c4f5bb..eeaa9b5 100644 --- a/openspec/specs/execution-safety/spec.md +++ b/openspec/specs/execution-safety/spec.md @@ -187,13 +187,32 @@ to the menu rather than terminating with an uncaught error. raises an error on a failing command - **THEN** the tool reports the failure and returns to the main menu, keeping the session alive +### Requirement: Host text cannot drive the terminal + +Text that comes from the host — database, file and service names, a command's output, values read from files — +SHALL be printed with every control character except tab and newline, and every escape sequence except colour +(SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be +refused when it holds a control character, so a pasted carriage return or escape never reaches `odoo.conf`, a +unit, or a shell line. + +#### Scenario: A hostile database name cannot rewrite the screen + +- **WHEN** a listed database name, a file name or a step's output holds an OSC or CSI sequence +- **THEN** the sequence is printed inert, and the plan preview shows what will really run + +#### Scenario: A typed control character is refused + +- **WHEN** the operator types or pastes a value with a carriage return or an escape +- **THEN** the prompt refuses it and asks again + ### Requirement: Secrets out of sight A secret — a database password, the master password, a file holding them — SHALL never be part of a plan step's command text: it SHALL travel in the step's environment (which only root can read), a libpq client SHALL take its password from `PGPASSWORD` there, and SQL holding a password SHALL be fed to `psql` on stdin. The plan -preview SHALL show such a step with its secrets masked, and a failed step's error SHALL name the step rather than -print its command. Database probes SHALL give up connecting after 10 seconds (`PGCONNECT_TIMEOUT`). +preview SHALL show such a step with its secrets masked — also where a secret appears escaped, as in an SQL +literal — and a failed step's error SHALL name the step rather than print its command. SQL that carries a +password inside a dollar-quoted block SHALL use a tag the password does not contain. Database probes SHALL give up connecting after 10 seconds (`PGCONNECT_TIMEOUT`). #### Scenario: A password is not visible to other users while a step runs diff --git a/openspec/specs/instance-configuration/spec.md b/openspec/specs/instance-configuration/spec.md index 8cfb291..daba4cd 100644 --- a/openspec/specs/instance-configuration/spec.md +++ b/openspec/specs/instance-configuration/spec.md @@ -70,7 +70,10 @@ correct ownership and permissions, then reopen Nginx logs. The tool SHALL install Python packages into the instance virtualenv either from a selected requirements file or from an operator-provided package list, running -pip as the instance user. +the venv's pip directly as the instance user (no login shell). Root SHALL only read the requirements file and +pass it on stdin. A typed entry starting with `-` (a pip option such as `--index-url`) SHALL be refused, and a +comma SHALL separate two entries only when a package name follows it, so a version range such as +`babel>=2.14,<3` stays one requirement. #### Scenario: Install from a requirements file @@ -82,6 +85,12 @@ pip as the instance user. - **WHEN** the operator provides packages inline and/or one per line - **THEN** the plan installs the parsed, non-empty package set into the venv as the instance user; if no valid package is found the operation is cancelled +#### Scenario: A typed value is a package, not a shell word or an option + +- **WHEN** the operator types `x$(id)`, `--index-url https://…` or `babel>=2.14,<3` +- **THEN** the first is passed to pip as one quoted argument, the second is refused, and the third is one + requirement + ### Requirement: Security and production posture audit The management status SHALL provide an on-demand security-and-production posture view that inspects the diff --git a/openspec/specs/instance-provisioning/spec.md b/openspec/specs/instance-provisioning/spec.md index eaea1be..eb192c0 100644 --- a/openspec/specs/instance-provisioning/spec.md +++ b/openspec/specs/instance-provisioning/spec.md @@ -273,8 +273,8 @@ that PDF report generation will fail until wkhtmltopdf is installed. - **WHEN** the operator accepts the recommended wkhtmltopdf option - **THEN** the plan selects the patched 0.12.6 asset mapped to the detected OS codename (or the closest - ABI-compatible build when the codename has no native asset), downloads it, and installs it only if - its SHA-256 matches the pinned checksum + ABI-compatible build when the codename has no native asset), downloads it into a private temporary + directory, and installs it only if its SHA-256 matches the pinned checksum, in the same step #### Scenario: Distribution package is offered as a reduced-fidelity fallback diff --git a/openspec/specs/server-audit/spec.md b/openspec/specs/server-audit/spec.md index 5d2a1a9..6c6463c 100644 --- a/openspec/specs/server-audit/spec.md +++ b/openspec/specs/server-audit/spec.md @@ -82,7 +82,13 @@ request, write a single report file and run active (read-only) TLS certificate c - **WHEN** the operator opts to export the report - **THEN** the tool writes a single report file to the chosen path (default under `./reports/`), which is the - only file the audit creates + only file the audit creates; the file must be new (an existing file or a link there is refused) and is + private (`600`) + +#### Scenario: Instance binaries are read, not run + +- **WHEN** the report shows an instance's Python version +- **THEN** it reads the venv's `pyvenv.cfg` instead of running the instance-owned interpreter as root #### Scenario: Optional active TLS checks are read-only diff --git a/openspec/specs/web-proxy-tls/spec.md b/openspec/specs/web-proxy-tls/spec.md index a37aa65..7514001 100644 --- a/openspec/specs/web-proxy-tls/spec.md +++ b/openspec/specs/web-proxy-tls/spec.md @@ -93,9 +93,10 @@ managed), and copy operator-supplied CRT/KEY with an optional intermediate. ### Requirement: Custom certificate validation -When copying operator-supplied certificates, the plan SHALL install them into a -dedicated per-instance SSL directory, build a fullchain, and validate the key, -the certificate, and that the private key matches the certificate before use. +When copying operator-supplied certificates, the plan SHALL copy them into a staging directory beside the +per-instance SSL directory, build the fullchain there, validate the key, the certificate, that the private key +matches the certificate, and the fullchain, and only then move them into place, keeping the previous files as +`.previous`. Operator-supplied paths SHALL be passed to the shell quoted. #### Scenario: Fullchain is built from cert and intermediate @@ -105,4 +106,9 @@ the certificate, and that the private key matches the certificate before use. #### Scenario: Key/certificate mismatch aborts the plan - **WHEN** the supplied private key's public key does not match the certificate's public key -- **THEN** the corresponding validation command fails and execution stops before Nginx is reconfigured +- **THEN** the step fails before any live file is replaced, and execution stops before Nginx is reconfigured + +#### Scenario: A file name is a name + +- **WHEN** a selected file's name holds shell syntax such as `$(…)` or a quote +- **THEN** it is copied as that file and nothing in its name runs diff --git a/tests/test_backup_restore.py b/tests/test_backup_restore.py index e2c113c..f51babb 100644 --- a/tests/test_backup_restore.py +++ b/tests/test_backup_restore.py @@ -28,9 +28,13 @@ def test_freezes_source_and_installs_into_target(self) -> None: cmd = _replicate_venv_packages_command( InstanceConfig(instance="prod"), InstanceConfig(instance="dev") ).command - self.assertIn("sudo -u prod /opt/odoo/prod/venv/bin/pip freeze", cmd) - self.assertIn("grep -E '^[A-Za-z0-9_.-]+=='", cmd) - self.assertIn("sudo -u dev /opt/odoo/dev/venv/bin/pip install -r", cmd) + self.assertIn("sudo -u prod -H bash -c '/opt/odoo/prod/venv/bin/pip freeze --exclude-editable'", cmd) + self.assertIn("grep -E '^[A-Za-z0-9][A-Za-z0-9_.-]*==[A-Za-z0-9_.+!-]+$'", cmd) + self.assertIn("sudo -u dev -H bash -c '/opt/odoo/dev/venv/bin/pip install -r /dev/stdin' < \"$tmp/reqs\"", cmd) + # A private temporary directory, never a fixed /tmp name; failures propagate. + self.assertIn("tmp=$(mktemp -d)", cmd) + self.assertNotIn("/tmp/", cmd) + self.assertIn("set -eo pipefail", cmd) class SafeDbNameTests(unittest.TestCase): diff --git a/tests/test_injection_privilege.py b/tests/test_injection_privilege.py new file mode 100644 index 0000000..dab8abd --- /dev/null +++ b/tests/test_injection_privilege.py @@ -0,0 +1,162 @@ +"""What reaches a root shell, a superuser session or the terminal: quoting, the user a +step runs as, private temporary files, and escape sequences from host text.""" + +from __future__ import annotations + +import os +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +from instance_manager import planners, system +from instance_manager.models import InstanceConfig +from instance_manager.prompts import _has_control +from instance_manager.ui import sanitize +from instance_manager.workflows import backup_restore, common, report +from instance_manager.workflows.manage import _split_requirements +from instance_manager.workflows.services import _unit_name_error + +HOSTILE = "/tmp/x$(touch /tmp/pwned)'y.crt" + + +class CertificateTests(unittest.TestCase): + def _config(self) -> InstanceConfig: + return InstanceConfig(instance="shop", domain="erp.example.com") + + def test_operator_paths_are_quoted(self) -> None: + [step] = planners.plan_copy_custom_certs(self._config(), HOSTILE, HOSTILE, HOSTILE) + self.assertIn("cp -- '/tmp/x$(touch /tmp/pwned)'\"'\"'y.crt' \"$stage/crt\"", step.command) + + def test_files_are_checked_before_they_replace_the_live_ones(self) -> None: + [step] = planners.plan_copy_custom_certs(self._config(), "/a.crt", "/a.key", None) + lines = step.command.splitlines() + first_move = next(i for i, line in enumerate(lines) if line.startswith("mv -f")) + for check in ("openssl pkey -in", "openssl x509 -in \"$stage/crt\"", 'if [ "$cert_fp" != "$key_fp" ]'): + self.assertLess(next(i for i, line in enumerate(lines) if check in line), first_move, check) + self.assertEqual(lines[0], "set -e") + + def test_self_signed_domain_and_paths_are_quoted(self) -> None: + config = self._config() + command = planners.plan_ensure_self_signed_certs(config)[1].command + self.assertIn("-subj /CN=erp.example.com", command) + self.assertNotIn("'/CN=", command.replace("-subj /CN=", "")) + + +class RequirementSplitTests(unittest.TestCase): + def test_a_comma_inside_a_version_range_does_not_split(self) -> None: + self.assertEqual(_split_requirements(["babel>=2.14,<3"]), (["babel>=2.14,<3"], [])) + + def test_comma_separated_names_split(self) -> None: + self.assertEqual(_split_requirements(["requests, lxml==5.2"]), (["requests", "lxml==5.2"], [])) + + def test_options_are_not_packages(self) -> None: + packages, refused = _split_requirements(["--index-url https://evil.example", "-e ."]) + self.assertEqual(packages, []) + self.assertEqual(refused, ["--index-url https://evil.example", "-e ."]) + + +class InstanceUserTests(unittest.TestCase): + def test_replica_freeze_and_install_run_as_their_owners(self) -> None: + command = backup_restore._replicate_venv_packages_command( + InstanceConfig(instance="prod"), InstanceConfig(instance="dev") + ).command + self.assertIn("sudo -u prod -H", command) + self.assertIn("sudo -u dev -H", command) + + def test_source_git_runs_as_the_source_user(self) -> None: + with mock.patch.object(backup_restore, "run") as run: + run.return_value = mock.Mock(returncode=0, stdout="18.0\n") + backup_restore._detect_source_repo_branch(InstanceConfig(instance="prod")) + self.assertTrue(run.call_args.args[0].startswith("sudo -u prod -H bash -c 'git -C /opt/odoo/prod/odoo")) + + def test_ocb_origin_forms(self) -> None: + for origin, core in (("https://github.com/OCA/OCB.git", "ocb"), ("git@github.com:OCA/OCB.git", "ocb"), + ("https://github.com/OCA/OCB", "ocb"), ("https://github.com/odoo/odoo.git", "odoo"), + ("https://github.com/oca/ocb-fork.git", "odoo")): + with mock.patch.object(backup_restore, "run", return_value=mock.Mock(returncode=0, stdout=origin)): + self.assertEqual(backup_restore._detect_source_core(InstanceConfig(instance="p")), core, origin) + + def test_the_report_reads_the_venv_python_instead_of_running_it(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + (Path(tmp) / "bin").mkdir() + (Path(tmp) / "pyvenv.cfg").write_text("home = /usr/bin\nversion = 3.12.3\n", encoding="utf-8") + with mock.patch.object(report, "_command_output") as run: + self.assertEqual(report._venv_python_version(f"{tmp}/bin/python3"), "Python 3.12.3") + run.assert_not_called() + (Path(tmp) / "pyvenv.cfg").write_text("version_info = 3.8.20.final.0\n", encoding="utf-8") + self.assertEqual(report._venv_python_version(f"{tmp}/bin/python3"), "Python 3.8.20") + + def test_neutralisation_runs_as_the_copy_owner(self) -> None: + commands = backup_restore._post_db_mode_commands( + "sudo -u postgres psql -d copy", 'Copied (new UUID on target)', True, "http://127.0.0.1:8069", + role="dev", + ) + self.assertEqual(len(commands), 3) + for command in commands: + self.assertIn("SET ROLE \"dev\"; SET search_path = pg_catalog, public; ", command.command) + + +class TemporaryFileTests(unittest.TestCase): + def test_wkhtmltopdf_uses_a_private_directory_in_one_step(self) -> None: + with mock.patch.object(planners, "resolve_wkhtmltopdf_asset", + return_value=("https://x/w.deb", "w.deb", "ab" * 32)): + commands = planners.plan_install_wkhtmltopdf("patched", "noble") + joined = "\n".join(c.command for c in commands) + self.assertNotIn("/tmp/", joined) + step = commands[-1].command + self.assertIn("tmp=$(mktemp -d)", step) + self.assertLess(step.index("sha256sum -c"), step.index("install \"$tmp\"/w.deb")) + + def test_an_export_never_follows_a_link(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + target = Path(tmp) / "victim" + target.write_text("keep", encoding="utf-8") + link = Path(tmp) / "report.txt" + link.symlink_to(target) + with self.assertRaises(OSError): + common._write_export(str(link), "x") + self.assertEqual(target.read_text(encoding="utf-8"), "keep") + common._write_export(str(Path(tmp) / "new.txt"), "x") + self.assertEqual(os.stat(Path(tmp) / "new.txt").st_mode & 0o777, 0o600) + + +class TerminalTests(unittest.TestCase): + def test_escape_sequences_from_the_host_are_neutralised(self) -> None: + osc = "db\x1b]0;owned\x07name" + self.assertEqual(sanitize(osc), "db?]0;owned?name") + self.assertEqual(sanitize("a\x1b[2Jb"), "a?[2Jb") + self.assertEqual(sanitize("\x9b31m"), "?31m") + + def test_colours_newlines_and_tabs_stay(self) -> None: + self.assertEqual(sanitize("\x1b[1;31mred\x1b[0m\n\tx"), "\x1b[1;31mred\x1b[0m\n\tx") + self.assertEqual(sanitize("50%\r", keep_cr=True), "50%\r") + self.assertEqual(sanitize("50%\r"), "50%?") + + def test_typed_control_characters_are_refused(self) -> None: + self.assertTrue(_has_control("^shop$\r")) + self.assertTrue(_has_control("a\x1b[2J")) + self.assertFalse(_has_control("p4ss w0rd!")) + + def test_unit_names(self) -> None: + self.assertIsNone(_unit_name_error("odoo-backup-shop.timer")) + self.assertIsNotNone(_unit_name_error("--now")) + self.assertIsNotNone(_unit_name_error("a b")) + + +class SecretFormTests(unittest.TestCase): + def test_an_escaped_secret_is_masked_too(self) -> None: + secret = "it's" + self.assertEqual(system.mask("PASSWORD 'it''s'", (secret,)), "PASSWORD '********'") + + def test_a_password_with_dollar_quotes_keeps_the_block_whole(self) -> None: + config = InstanceConfig(instance="shop") + config.normalize_defaults() + config.db_password = "a$$b$oim0$c" + sql = planners._db_role_create_if_missing_sql(config) + self.assertTrue(sql.startswith("DO $oim1$ ")) + self.assertTrue(sql.endswith("$oim1$;")) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/verify_data_safety.py b/tools/verify_data_safety.py index eda0633..50b12d9 100644 --- a/tools/verify_data_safety.py +++ b/tools/verify_data_safety.py @@ -49,7 +49,7 @@ def _stub_bin(root: Path) -> Path: """`sudo -u ` runs the rest as us; pg_dump/pg_restore are scripted by env.""" bindir = root / "stubs" bindir.mkdir() - _write_exe(bindir / "sudo", 'if [ "$1" = "-u" ]; then shift 2; fi\nexec "$@"\n') + _write_exe(bindir / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\nexec "$@"\n') _write_exe( bindir / "pg_dump", 'case "${STUB_PG_DUMP:-ok}" in\n' @@ -414,7 +414,7 @@ def _against_a_server(root: Path) -> None: return stubs = root / "pgstubs" stubs.mkdir() - _write_exe(stubs / "sudo", 'if [ "$1" = "-u" ]; then shift 2; fi\nexec "$@"\n') + _write_exe(stubs / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\nexec "$@"\n') cluster = Cluster(root, bindir) try: cluster.start() diff --git a/tools/verify_neutralisation.py b/tools/verify_neutralisation.py index b01cbef..9bd1c19 100644 --- a/tools/verify_neutralisation.py +++ b/tools/verify_neutralisation.py @@ -110,7 +110,7 @@ def _one(major: int, python: str, clone: Path, cluster: Cluster, env: dict[str, plan = (backup_restore._seed_db_commands(src, dst, "dev", "dump") + backup_restore._post_db_mode_commands( - backup_restore._psql_target_local(dst), "Copied (new UUID on target)", True, URL)) + backup_restore._psql_target_local(dst), "Copied (new UUID on target)", True, URL, role="dev")) for command in plan: result = subprocess.run(["bash", "-c", command.command], capture_output=True, text=True, env=env) output = result.stdout + result.stderr @@ -126,7 +126,7 @@ def _one(major: int, python: str, clone: Path, cluster: Cluster, env: dict[str, check(f"Odoo {major}: the copy is visible to its role after the hand-over", _visible_to(cluster, "dev", dst)) again = backup_restore._post_db_mode_commands( - backup_restore._psql_target_local(dst), "Moved (keep UUID)", True, URL) + backup_restore._psql_target_local(dst), "Moved (keep UUID)", True, URL, role="dev") results = [subprocess.run(["bash", "-c", c.command], capture_output=True, text=True, env=env) for c in again] check(f"Odoo {major}: neutralising again is harmless", all(r.returncode == 0 for r in results), " ".join(r.stderr for r in results)) @@ -175,7 +175,7 @@ def main() -> int: root = Path(tmp) stubs = root / "stubs" stubs.mkdir() - _write_exe(stubs / "sudo", 'if [ "$1" = "-u" ]; then shift 2; fi\nexec "$@"\n') + _write_exe(stubs / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\nexec "$@"\n') cluster = Cluster(root, bindir) try: cluster.start() diff --git a/tools/verify_privilege.py b/tools/verify_privilege.py new file mode 100644 index 0000000..7d6b8b8 --- /dev/null +++ b/tools/verify_privilege.py @@ -0,0 +1,243 @@ +#!/usr/bin/env python3 +"""Run what reaches a root shell or a superuser session, instead of reading it. + +- the certificate step with real ``openssl``: a matching pair is installed (the + previous files kept as ``.previous``), a key that does not match leaves the live + files as they were, a hostile file name is taken literally, a chain is joined; +- the self-signed step with real ``openssl``; +- the wkhtmltopdf step with stub ``curl``/``apt-get``: a wrong SHA-256 installs + nothing, a right one installs from a private directory that is gone afterwards; +- the replica's venv replication with stub ``pip``s: only ``name==version`` lines + reach the target, and a failing install fails the step; +- neutralisation as the copy's owner, against a PostgreSQL of our own: a trigger the + source database carries cannot raise its owner to superuser, while the same step + run as the superuser lets it (negative control). + +Nothing touches the host's services or databases (``initdb`` refuses root, so the +server-backed check is a developer-machine one). + + python3 tools/verify_privilege.py +""" + +from __future__ import annotations + +import hashlib +import os +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent)) +sys.path.insert(0, str(HERE)) + +from verify_data_safety import Cluster, _postgres_bindir, _rebase, _write_exe # noqa: E402 + +from instance_manager import planners # noqa: E402 +from instance_manager.models import InstanceConfig # noqa: E402 +from instance_manager.workflows import backup_restore # noqa: E402 + +FAILURES: list[str] = [] + + +def check(name: str, ok: bool, detail: str = "") -> None: + print(f"{'ok ' if ok else 'FAIL'} {name}" + (f" — {detail[-400:]}" if detail and not ok else ""), flush=True) + if not ok: + FAILURES.append(name) + + +def _run(script: str, env: dict[str, str]) -> subprocess.CompletedProcess[str]: + return subprocess.run(["bash", "-c", script], capture_output=True, text=True, env=env) + + +def _stub_env(root: Path, names: tuple[str, ...]) -> dict[str, str]: + """Each named command records its arguments and succeeds.""" + bindir = root / "stubs" + bindir.mkdir(exist_ok=True) + for name in names: + _write_exe(bindir / name, f'echo "{name} $*" >> {root}/calls\n') + _write_exe(bindir / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\n' + 'exec "$@"\n') + return dict(os.environ, PATH=f"{bindir}:{os.environ['PATH']}") + + +def _local(command: str, root: Path) -> str: + """A cert step runnable without root: host paths under ``root``, and the + directory made without a root owner (chown itself is a stub).""" + return _rebase(command, root).replace("install -d -m 750 -o root -g www-data", "install -d -m 750") + + +def _openssl(*args: str) -> None: + subprocess.run(["openssl", *args], check=True, capture_output=True) + + +def _pair(directory: Path, name: str) -> tuple[Path, Path]: + key, cert = directory / f"{name}.key", directory / f"{name}.crt" + _openssl("req", "-x509", "-nodes", "-newkey", "rsa:2048", "-days", "2", "-subj", f"/CN={name}", + "-keyout", str(key), "-out", str(cert)) + return key, cert + + +def _certificates(root: Path) -> None: + if shutil.which("openssl") is None: + check("openssl found", not os.environ.get("CI")) + print("skip openssl not found") + return + env = _stub_env(root, ("chown",)) + src = root / "src" + src.mkdir() + key, cert = _pair(src, "one") + other_key = _pair(src, "two")[0] + ca = _pair(src, "ca")[1] + config = InstanceConfig(instance="shop", domain="erp.example.com") + live_cert = Path(_rebase(config.ssl_cert_file, root)) + + [step] = planners.plan_copy_custom_certs(config, str(cert), str(key), None) + result = _run(_local(step.command, root), env) + check("a matching pair is installed", result.returncode == 0 and live_cert.read_bytes() == cert.read_bytes(), + result.stderr) + check("the key is installed private (640)", oct(Path(_rebase(config.ssl_key_file, root)).stat().st_mode & 0o777) + == "0o640") + check("no staging directory is left", not list(live_cert.parent.glob(".stage.*"))) + + before = {p.name: p.read_bytes() for p in live_cert.parent.iterdir()} + [step] = planners.plan_copy_custom_certs(config, str(cert), str(other_key), None) + result = _run(_local(step.command, root), env) + after = {p.name: p.read_bytes() for p in live_cert.parent.iterdir()} + check("a key that does not match fails the step", result.returncode != 0) + check("and leaves the live files as they were", before == after, str(sorted(after))) + + hostile = src / "x$(touch PWNED)'y.crt" + shutil.copy(cert, hostile) + [step] = planners.plan_copy_custom_certs(config, str(hostile), str(key), str(ca)) + result = subprocess.run(["bash", "-c", _local(step.command, root)], capture_output=True, text=True, env=env, + cwd=root) + check("a hostile file name is taken literally", not (root / "PWNED").exists(), result.stderr) + check("the step still runs", result.returncode == 0, result.stderr) + chain = Path(_rebase(config.ssl_fullchain_file, root)).read_text(encoding="utf-8") + check("the full chain holds the certificate then the intermediate", chain.count("BEGIN CERTIFICATE") == 2) + check("the previous files are kept", Path(_rebase(config.ssl_cert_file + ".previous", root)).is_file()) + + self_signed = InstanceConfig(instance="self", domain="self.example.com") + failed = [c.description for c in planners.plan_ensure_self_signed_certs(self_signed) + if _run(_local(c.command, root), env).returncode != 0] + check("the self-signed steps run", not failed, str(failed)) + check("they leave a key and a full chain", + Path(_rebase(self_signed.ssl_key_file, root)).is_file() + and Path(_rebase(self_signed.ssl_fullchain_file, root)).is_file()) + + +def _wkhtmltopdf(root: Path) -> None: + payload = b"fake deb" + calls = root / "calls" + env = _stub_env(root, ("apt-get",)) + _write_exe(root / "stubs" / "curl", 'while [ "$1" != "-o" ]; do shift; done\nprintf "fake deb" > "$2"\n') + for digest, should_install in (("0" * 64, False), (hashlib.sha256(payload).hexdigest(), True)): + calls.unlink(missing_ok=True) + planners_asset = ("https://example.invalid/w.deb", "w.deb", digest) + original = planners.resolve_wkhtmltopdf_asset + planners.resolve_wkhtmltopdf_asset = lambda codename, asset=planners_asset: asset + try: + step = planners.plan_install_wkhtmltopdf("patched", "noble")[-1] + finally: + planners.resolve_wkhtmltopdf_asset = original + result = _run(step.command, env) + installed = calls.exists() and "install " in calls.read_text(encoding="utf-8") + label = "a matching SHA-256 installs" if should_install else "a wrong SHA-256 installs nothing" + check(label, installed == should_install and (result.returncode == 0) == should_install, + (calls.read_text(encoding="utf-8") if calls.exists() else "") + result.stderr) + if should_install: + deb = calls.read_text(encoding="utf-8").split()[-1] + check("from a private directory that is gone afterwards", not deb.startswith("/tmp/w") + and not Path(deb).exists(), deb) + + +def _venv_replication(root: Path) -> None: + opt = root / "opt" + env = _stub_env(root, ()) + source, target = InstanceConfig(instance="prod"), InstanceConfig(instance="dev") + freeze = "Babel==2.14.0\n-e git+https://example.invalid/x.git#egg=x\nevil @ file:///tmp/evil\nlxml==5.2.1\n" + for config in (source, target): + (opt / config.instance / "venv" / "bin").mkdir(parents=True) + _write_exe(opt / "prod/venv/bin/pip", f"cat <<'EOF'\n{freeze}EOF\n") + received = root / "received" + for target_exit, label in ((0, "succeeds"), (1, "fails")): + _write_exe(opt / "dev/venv/bin/pip", f'cat > {received}\nexit {target_exit}\n') + command = backup_restore._replicate_venv_packages_command(source, target).command + result = _run(command.replace("/opt/odoo", str(opt)), env) + if target_exit == 0: + got = received.read_text(encoding="utf-8").split() + check("only name==version lines reach the target", got == ["Babel==2.14.0", "lxml==5.2.1"], str(got)) + check("the replication succeeds", result.returncode == 0, result.stderr) + else: + check(f"a target install that {label} fails the step", result.returncode != 0, result.stdout) + + +def _neutralise_as_owner(root: Path) -> None: + bindir = _postgres_bindir() + if bindir is None or os.geteuid() == 0: + check("initdb and a non-root user are available", not os.environ.get("CI")) + print("skip the neutralisation check needs initdb and a non-root user") + return + stubs = root / "pgstubs" + stubs.mkdir() + _write_exe(stubs / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\n' + 'exec "$@"\n') + (root / "pg").mkdir() + cluster = Cluster(root / "pg", bindir) + try: + cluster.start() + env = cluster.env(stubs) + cluster.value("CREATE ROLE dev LOGIN") + for db in ("copy_owner", "copy_superuser"): + cluster.value(f'CREATE DATABASE "{db}"') + for sql in ( + "CREATE TABLE ir_config_parameter (id serial PRIMARY KEY, key varchar UNIQUE NOT NULL, value text, " + "create_uid int, create_date timestamp, write_uid int, write_date timestamp)", + "ALTER TABLE ir_config_parameter OWNER TO dev", + # What a hostile source database could carry: a trigger raising its owner. + "CREATE FUNCTION escalate() RETURNS trigger LANGUAGE plpgsql AS " + "$$ BEGIN EXECUTE 'ALTER ROLE dev SUPERUSER'; RETURN NEW; END $$", + "ALTER FUNCTION escalate() OWNER TO dev", + "CREATE TRIGGER escalate BEFORE INSERT OR UPDATE ON ir_config_parameter " + "FOR EACH ROW EXECUTE FUNCTION escalate()", + ): + cluster.value_in(db, sql) + + [step] = backup_restore._post_db_mode_commands( + backup_restore._psql_target_local("copy_owner"), 'Copied (new UUID on target)', False, + "http://127.0.0.1:8069", role="dev", + ) + result = _run(step.command, env) + superuser = cluster.value("SELECT rolsuper FROM pg_roles WHERE rolname = 'dev'") + check("run as the owner, the trigger cannot raise its role", superuser == "f", result.stderr) + check("and the step fails instead of going on", result.returncode != 0) + + [step] = backup_restore._post_db_mode_commands( + backup_restore._psql_target_local("copy_superuser"), 'Copied (new UUID on target)', False, + "http://127.0.0.1:8069", + ) + _run(step.command, env) + superuser = cluster.value("SELECT rolsuper FROM pg_roles WHERE rolname = 'dev'") + check("negative control: run as the superuser, the trigger raises it", superuser == "t") + finally: + cluster.stop() + + +def main() -> int: + with tempfile.TemporaryDirectory(prefix="oim-verify-privilege-") as tmp: + root = Path(tmp) + for part in ("certs", "wk", "venv", "pg"): + (root / part).mkdir() + _certificates(root / "certs") + _wkhtmltopdf(root / "wk") + _venv_replication(root / "venv") + _neutralise_as_owner(root / "pg") + print(f"\n{len(FAILURES)} failure(s)" if FAILURES else "\nall checks passed") + return 1 if FAILURES else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/verify_secrets.py b/tools/verify_secrets.py index 23d5e7f..c6fbe97 100644 --- a/tools/verify_secrets.py +++ b/tools/verify_secrets.py @@ -102,7 +102,7 @@ def _role(root: Path) -> None: return stubs = root / "stubs" stubs.mkdir() - _write_exe(stubs / "sudo", 'if [ "$1" = "-u" ]; then shift 2; fi\nexec "$@"\n') + _write_exe(stubs / "sudo", 'while [ "${1#-}" != "$1" ]; do if [ "$1" = "-u" ]; then shift; fi; shift; done\nexec "$@"\n') cluster = Cluster(root / "pg", bindir) (root / "pg").mkdir() try: From 2bbabb73cbbc4ad65710522ce76e3ced1442198c Mon Sep 17 00:00:00 2001 From: Guillermo Rojo Date: Thu, 8 Oct 2026 14:43:52 +0200 Subject: [PATCH 2/3] fix: neutralise as the owner with search_path public; fail the check without Odoo tables With search_path pg_catalog, public the current schema was pg_catalog, so no rule found its table and the check passed on nothing. pg_catalog is searched first anyway; the check now fails when the current schema has no ir_cron. Table-existence tests are nested so PL/pgSQL never plans a query on a missing table. --- instance_manager/neutralise.py | 19 +++++++--- instance_manager/workflows/backup_restore.py | 10 +++--- .../specs/data-backup-restore/spec.md | 7 +++- openspec/specs/data-backup-restore/spec.md | 7 +++- tests/test_injection_privilege.py | 2 +- tools/verify_privilege.py | 36 +++++++++++++++++++ 6 files changed, 69 insertions(+), 12 deletions(-) diff --git a/instance_manager/neutralise.py b/instance_manager/neutralise.py index c415eba..01f6276 100644 --- a/instance_manager/neutralise.py +++ b/instance_manager/neutralise.py @@ -231,11 +231,13 @@ def apply_sql(local_url: str) -> str: + f" WHERE ({rule.where}) AND ({_armed(rule, local_url)}); END IF; " for rule in CATALOGUE ) + # Nested, not `AND`: PL/pgSQL plans the whole condition, so a query on a table + # that is not there fails even behind a false `to_regclass` test. flag = ( - "IF to_regclass('ir_config_parameter') IS NOT NULL AND NOT EXISTS (SELECT 1 FROM " + "IF to_regclass('ir_config_parameter') IS NOT NULL THEN IF NOT EXISTS (SELECT 1 FROM " "ir_config_parameter WHERE key = 'database.is_neutralized') THEN " "INSERT INTO ir_config_parameter (key, value) VALUES ('database.is_neutralized', 'True'); " - "END IF; " + "END IF; END IF; " ) return f"DO $$ BEGIN {blocks}{_mail_sink_block()}{flag}END $$;" @@ -250,13 +252,20 @@ def guard_sql(local_url: str) -> str: for rule in CATALOGUE ) sink = ( - "IF to_regclass('ir_mail_server') IS NOT NULL AND NOT EXISTS (SELECT 1 FROM ir_mail_server " + "IF to_regclass('ir_mail_server') IS NOT NULL THEN IF NOT EXISTS (SELECT 1 FROM ir_mail_server " f"WHERE active AND name = {_lit(MAIL_SINK_NAME)} AND smtp_host = {_lit(MAIL_SINK_HOST)}) THEN " - "found := found || 'mail-sink (missing) '; END IF; " + "found := found || 'mail-sink (missing) '; END IF; END IF; " + ) + # Every rule looks in the current schema: if Odoo's tables are not there, no rule + # applied and the check above saw nothing, so it must not pass. + odoo = ( + "IF NOT EXISTS (SELECT 1 FROM pg_class c JOIN pg_namespace s ON s.oid = c.relnamespace " + "WHERE s.nspname = current_schema() AND c.relname = 'ir_cron') THEN " + "found := found || 'no Odoo tables in schema ' || current_schema() || ' '; END IF; " ) return ( "DO $$ DECLARE n integer; found text := ''; BEGIN " - f"{blocks}{sink}" + f"{odoo}{blocks}{sink}" "IF found <> '' THEN RAISE EXCEPTION 'the copy can still act on the outside: %', found; " "END IF; END $$;" ) diff --git a/instance_manager/workflows/backup_restore.py b/instance_manager/workflows/backup_restore.py index 9ca96c2..3dfe688 100644 --- a/instance_manager/workflows/backup_restore.py +++ b/instance_manager/workflows/backup_restore.py @@ -124,12 +124,14 @@ def _post_db_mode_commands( Each step is one statement that stops the plan when it fails: a copy left half-neutralised must not be handed to a running Odoo. - With ``role`` (the copy's owner), the statements run as that role with a fixed - ``search_path``: a trigger or function the source database carries then runs with - the owner's rights, never the superuser's. Names MUST be safe. + With ``role`` (the copy's owner), the statements run as that role with + ``search_path = public``: a trigger or function the source database carries then + runs with the owner's rights, never the superuser's. ``pg_catalog`` stays searched + first (it is, unless listed), and ``public`` stays the current schema the + catalogue's guards look in. Names MUST be safe. """ psql = f"{psql_target} -X -q -v ON_ERROR_STOP=1 -c" - prefix = f'SET ROLE "{role}"; SET search_path = pg_catalog, public; ' if role else "" + prefix = f'SET ROLE "{role}"; SET search_path = public; ' if role else "" commands: list[Command] = [] if migration_mode == 'Copied (new UUID on target)': commands.append( diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md index d5641a8..cad5b21 100644 --- a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/data-backup-restore/spec.md @@ -8,7 +8,7 @@ requested, apply every rule of the neutralisation catalogue (Odoo's `neutralize. 12.0-19.0 and the OCA modules it lists), each guarded by the existence of its table and columns, as one statement that stops the plan on failure, and then verify that nothing can still act on the outside, failing the plan when something can. On the local server these statements SHALL run as the copy's owner role with -`search_path = pg_catalog, public`, never as the superuser: a trigger or function the source database carries +`search_path = public` (`pg_catalog` is still searched first), never as the superuser: a trigger or function the source database carries then runs with that role's rights. The neutralisation SHALL leave exactly one active outgoing mail server, pointing at a host that does not resolve, so Odoo never falls back to the `smtp_server` of `odoo.conf`, and SHALL drop production's SMTP credentials from the copy. @@ -37,6 +37,11 @@ credentials from the copy. - **WHEN** the copied database has a trigger that tries to make its owner a superuser - **THEN** the trigger fails for lack of privilege, the step fails, and the role is unchanged +#### Scenario: A check that sees no Odoo table fails + +- **WHEN** the check runs where the current schema holds no Odoo table (`ir_cron`) +- **THEN** it fails instead of passing because no rule applied + ### Requirement: Duplication The tool SHALL duplicate a source instance's database — and optionally its filestore — into a target, diff --git a/openspec/specs/data-backup-restore/spec.md b/openspec/specs/data-backup-restore/spec.md index 4eaf86b..d072be3 100644 --- a/openspec/specs/data-backup-restore/spec.md +++ b/openspec/specs/data-backup-restore/spec.md @@ -214,7 +214,7 @@ requested, apply every rule of the neutralisation catalogue (Odoo's `neutralize. 12.0-19.0 and the OCA modules it lists), each guarded by the existence of its table and columns, as one statement that stops the plan on failure, and then verify that nothing can still act on the outside, failing the plan when something can. On the local server these statements SHALL run as the copy's owner role with -`search_path = pg_catalog, public`, never as the superuser: a trigger or function the source database carries +`search_path = public` (`pg_catalog` is still searched first), never as the superuser: a trigger or function the source database carries then runs with that role's rights. The neutralisation SHALL leave exactly one active outgoing mail server, pointing at a host that does not resolve, so Odoo never falls back to the `smtp_server` of `odoo.conf`, and SHALL drop production's SMTP credentials from the copy. @@ -243,6 +243,11 @@ credentials from the copy. - **WHEN** the copied database has a trigger that tries to make its owner a superuser - **THEN** the trigger fails for lack of privilege, the step fails, and the role is unchanged +#### Scenario: A check that sees no Odoo table fails + +- **WHEN** the check runs where the current schema holds no Odoo table (`ir_cron`) +- **THEN** it fails instead of passing because no rule applied + ### Requirement: Database name path safety An operator-entered database name SHALL be a name Odoo's own database manager accepts diff --git a/tests/test_injection_privilege.py b/tests/test_injection_privilege.py index dab8abd..4dd9b51 100644 --- a/tests/test_injection_privilege.py +++ b/tests/test_injection_privilege.py @@ -94,7 +94,7 @@ def test_neutralisation_runs_as_the_copy_owner(self) -> None: ) self.assertEqual(len(commands), 3) for command in commands: - self.assertIn("SET ROLE \"dev\"; SET search_path = pg_catalog, public; ", command.command) + self.assertIn("SET ROLE \"dev\"; SET search_path = public; ", command.command) class TemporaryFileTests(unittest.TestCase): diff --git a/tools/verify_privilege.py b/tools/verify_privilege.py index 7d6b8b8..43cfd34 100644 --- a/tools/verify_privilege.py +++ b/tools/verify_privilege.py @@ -222,6 +222,42 @@ def _neutralise_as_owner(root: Path) -> None: _run(step.command, env) superuser = cluster.value("SELECT rolsuper FROM pg_roles WHERE rolname = 'dev'") check("negative control: run as the superuser, the trigger raises it", superuser == "t") + cluster.value("ALTER ROLE dev NOSUPERUSER") + + # As the owner, the rules still find the tables: the copy is really neutralised. + cluster.value('CREATE DATABASE "copy_plain"') + for sql in ( + "CREATE TABLE ir_config_parameter (id serial PRIMARY KEY, key varchar UNIQUE NOT NULL, value text)", + "CREATE TABLE ir_cron (id serial PRIMARY KEY, active boolean, cron_name varchar)", + "CREATE TABLE ir_model_data (id serial PRIMARY KEY, module varchar, name varchar, model varchar, " + "res_id int)", + "INSERT INTO ir_config_parameter (key, value) VALUES ('web.base.url', 'https://erp.example.com')", + "INSERT INTO ir_cron (active, cron_name) VALUES (true, 'send invoices')", + "ALTER TABLE ir_config_parameter OWNER TO dev", + "ALTER TABLE ir_cron OWNER TO dev", + "ALTER TABLE ir_model_data OWNER TO dev", + ): + cluster.value_in("copy_plain", sql) + steps = backup_restore._post_db_mode_commands( + backup_restore._psql_target_local("copy_plain"), "Moved (keep UUID)", True, + "http://127.0.0.1:8069", role="dev", + ) + results = [_run(c.command, env) for c in steps] + check("as the owner, the neutralisation and its check run", + all(r.returncode == 0 for r in results), " ".join(r.stderr for r in results)) + check("and reach the tables (base URL and crons changed)", + cluster.value_in("copy_plain", "SELECT value FROM ir_config_parameter WHERE key = 'web.base.url'") + == "http://127.0.0.1:8069" + and cluster.value_in("copy_plain", "SELECT count(*) FROM ir_cron WHERE active") == "0") + + cluster.value('CREATE DATABASE "not_odoo"') + check_step = backup_restore._post_db_mode_commands( + backup_restore._psql_target_local("not_odoo"), "Moved (keep UUID)", True, + "http://127.0.0.1:8069", role="dev", + )[-1] + result = _run(check_step.command, env) + check("the check fails where it sees no Odoo table, instead of passing on nothing", + result.returncode != 0 and "no Odoo tables" in result.stderr, result.stderr) finally: cluster.stop() From 58cfb92b9968d0c1209ebb40d54ffad65857d8ab Mon Sep 17 00:00:00 2001 From: Guillermo Rojo Date: Thu, 8 Oct 2026 14:45:37 +0200 Subject: [PATCH 3/3] docs(spec): state the terminal requirement with SHALL in its first line --- .../specs/execution-safety/spec.md | 6 +++--- openspec/specs/execution-safety/spec.md | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md index da78a9d..9cb38fa 100644 --- a/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md +++ b/openspec/changes/archive/2026-10-08-least-privilege-and-quoting/specs/execution-safety/spec.md @@ -2,9 +2,9 @@ ### Requirement: Host text cannot drive the terminal -Text that comes from the host — database, file and service names, a command's output, values read from files — -SHALL be printed with every control character except tab and newline, and every escape sequence except colour -(SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be +The tool SHALL print text that comes from the host (database, file and service names, a command's output, +values read from files) with every control character except tab and newline, and every escape sequence except +colour (SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be refused when it holds a control character, so a pasted carriage return or escape never reaches `odoo.conf`, a unit, or a shell line. diff --git a/openspec/specs/execution-safety/spec.md b/openspec/specs/execution-safety/spec.md index eeaa9b5..500d8b7 100644 --- a/openspec/specs/execution-safety/spec.md +++ b/openspec/specs/execution-safety/spec.md @@ -189,9 +189,9 @@ to the menu rather than terminating with an uncaught error. ### Requirement: Host text cannot drive the terminal -Text that comes from the host — database, file and service names, a command's output, values read from files — -SHALL be printed with every control character except tab and newline, and every escape sequence except colour -(SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be +The tool SHALL print text that comes from the host (database, file and service names, a command's output, +values read from files) with every control character except tab and newline, and every escape sequence except +colour (SGR), shown as `?`; a command's streamed output keeps its carriage returns. A value the operator types SHALL be refused when it holds a control character, so a pasted carriage return or escape never reaches `odoo.conf`, a unit, or a shell line.