From c83f76cb1a7a611ad9eb4e6ad029e3085c7d3a30 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Tue, 6 Oct 2026 13:30:05 +0000 Subject: [PATCH] feat(docker): stricter request validation and bind mount rules Signed-off-by: Oleksandr Piskun --- .github/workflows/tests-unit.yml | 35 ++ README.md | 33 +- haproxy.cfg.template | 8 + haproxy_agent.py | 462 ++++++++++++++++++++--- pyproject.toml | 5 + start.sh | 14 +- tests/unit/__init__.py | 2 + tests/unit/conftest.py | 22 ++ tests/unit/payloads.py | 29 ++ tests/unit/test_bind_mounts.py | 289 +++++++++++++++ tests/unit/test_payload_validation.py | 119 ++++++ tests/unit/test_requests.py | 516 ++++++++++++++++++++++++++ 12 files changed, 1480 insertions(+), 54 deletions(-) create mode 100644 .github/workflows/tests-unit.yml create mode 100644 tests/unit/__init__.py create mode 100644 tests/unit/conftest.py create mode 100644 tests/unit/payloads.py create mode 100644 tests/unit/test_bind_mounts.py create mode 100644 tests/unit/test_payload_validation.py create mode 100644 tests/unit/test_requests.py diff --git a/.github/workflows/tests-unit.yml b/.github/workflows/tests-unit.yml new file mode 100644 index 0000000..ad378f4 --- /dev/null +++ b/.github/workflows/tests-unit.yml @@ -0,0 +1,35 @@ +name: Tests - Unit + +on: + pull_request: + branches: [main] + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: tests-unit-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + unit: + runs-on: ubuntu-22.04 + name: Unit tests (in the HaRP image) + + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Build HaRP image + run: docker build -t harp-unit-tests . + + # The tests run inside the image, so they use the Python and library versions HaRP ships with. + - name: Run unit tests + run: | + docker run --rm -v "$PWD":/src:ro -w /src --entrypoint sh harp-unit-tests -c \ + "pip install --break-system-packages --root-user-action=ignore --quiet pytest==9.1.1 && python3 -m pytest tests/unit -p no:cacheprovider" diff --git a/README.md b/README.md index e2a758b..72b155e 100644 --- a/README.md +++ b/README.md @@ -298,11 +298,39 @@ HaRP is configured via several environment variables. Here are the key variables - **Description:** The base URL of your Nextcloud instance. - **Requirement:** Must be accessible from the HaRP container. +- **`HP_DOCKER_ENGINE_PORTS`** + - **Description:** Comma-separated ports and ranges on HaRP's loopback interface that AppAPI may address with its `docker-engine-port` header. Requests for any other port are refused (`403` on the Docker API passthrough, `400` on the `/docker/exapp/*` endpoints). + - **Default:** `HP_DOCKER_ENGINE_PORTS="24000-24099"`, the range the FRP server's configuration reserves for Docker Engine tunnels (`24000` is the bundled engine, `24001-24099` are remote engines). + - **Example:** `HP_DOCKER_ENGINE_PORTS="24000-24099,2375"` + - **Note:** Add a port when AppAPI reaches a Docker Engine through another one: a remote engine whose FRP tunnel uses a port in `23000-23999` (the FRP server accepts these as well, although they are meant for ExApps), or a socket proxy that HaRP reaches over host networking. HaRP passes no credentials to these ports (it removes `harp-shared-key` and `Authorization`), so such a proxy must accept requests without them and should listen on 127.0.0.1 only. An empty value means the default; HaRP does not start with a malformed one. + - **`HP_FRP_DISABLE_TLS`** - **Description:** Disables TLS for the FRP service. - **Default:** `HP_FRP_DISABLE_TLS="false"` - **Advanced:** Use only for specialized setups where TCP TLS termination is managed externally. +- **`HP_EXAPP_BIND_MOUNTS_DISABLED`** / **`HP_EXAPP_BIND_MOUNTS_ALLOWED`** / **`HP_EXAPP_BIND_MOUNTS_DENIED`** / **`HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL`** + - **Description:** Rules for the host paths that may be bind mounted into ExApp containers on a Docker Engine (the mounts an administrator adds in the deploy options of an ExApp). A container whose mounts break a rule is not created, and the answer names the rule and the mount; AppAPI shows it to the administrator and HaRP writes it to its log. + - `HP_EXAPP_BIND_MOUNTS_DISABLED`: set to `true` (or `1`, `yes`) to refuse every bind mount. + - `HP_EXAPP_BIND_MOUNTS_ALLOWED`: comma-separated list of host paths. When set, only these paths and the paths below them may be mounted. + - `HP_EXAPP_BIND_MOUNTS_DENIED`: comma-separated list of host paths that may not be mounted. A mount is refused when its source is a listed path, lies below one, or is a parent of one. Add `:rw` to an entry to refuse only read-write mounts of it. Setting this variable replaces the default list, and an empty value removes it. + - `HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL`: in this release, a mount refused only by `/dev`, `/run`, `/var/run`, `/root` or a `:rw` entry of the default list is permitted with a warning naming the mount, the ExApp container, the rule and how to keep the mount, and so is a mount mode other than `ro` or `rw` (such as `z` or `RO`, which mounts read-write as it always did); a later release will refuse both. Set to `true` to refuse such mounts now. A mount that contains one of the other entries, such as `/run` or `/var/run` themselves, is always refused. The entries of a list you set in `HP_EXAPP_BIND_MOUNTS_DENIED` are always enforced. + - **Default:** + - `HP_EXAPP_BIND_MOUNTS_DISABLED="false"` + - `HP_EXAPP_BIND_MOUNTS_ALLOWED=""` (any path that is not denied) + - `HP_EXAPP_BIND_MOUNTS_DENIED="/proc,/sys,/boot,/var/lib/docker,/var/lib/containerd,/var/lib/containers,/run/docker.sock,/var/run/docker.sock,/run/containerd,/var/run/containerd,/run/podman,/var/run/podman,/run/crio,/var/run/crio,/etc/shadow,/etc/gshadow,/etc/sudoers,/etc/sudoers.d,/etc/ssh,/dev,/run,/var/run,/root,/etc:rw,/usr:rw,/bin:rw,/sbin:rw,/lib:rw,/lib64:rw,/var/spool:rw"` (kernel interfaces, the container runtimes' sockets and state, and host credentials; then device and runtime trees, root's home, and read-write access to the host's programs and spool directories) + - `HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL="false"` + - **Example:** `HP_EXAPP_BIND_MOUNTS_ALLOWED="/mnt/models,/srv/exapps"` + - **Note:** + - When an allowed and a denied entry both cover a mount, the more specific (longer) one decides, and a denied entry wins a tie. With the default list, `HP_EXAPP_BIND_MOUNTS_ALLOWED="/mnt/models,/var/run/mysqld"` permits `/var/run/mysqld/mysqld.sock` while `/var/run/docker.sock` stays refused. Because an allowed list restricts every other mount too, list all the sources you mount when you add an exception. + - `HP_EXAPP_BIND_MOUNTS_DENIED="/"` together with an allowed list permits nothing but the listed paths. + - A mount without a mode is read-write (`occ app_api:app:register --mount SRC:DST` means `rw`), so add `:ro` to read-only mounts of system paths such as `/etc/localtime` or `/usr/share/zoneinfo`. + - On Docker Desktop for Windows, host paths look like `/run/desktop/mnt/host/c/...` and fall under `/run`; set `HP_EXAPP_BIND_MOUNTS_ALLOWED="/run/desktop/mnt/host"` there. The sockets of rootless Docker and Podman (`/run/user//...`) also fall under `/run` only, so in this release they are permitted with a warning. + - Paths are normalized (`//`, `/./` and a trailing `/` are removed), compared as text, and sent to Docker in that form. HaRP cannot see symbolic links on the Docker host, so set `HP_EXAPP_BIND_MOUNTS_ALLOWED` when you need a strict rule. + - Entries are separated by commas; quotes around the whole value or around an entry are ignored. Every path must be absolute and must not contain a `..` segment, `:` or control characters (apart from the `:rw` suffix, which only `HP_EXAPP_BIND_MOUNTS_DENIED` accepts); a path may contain blanks, but several paths separated by blanks are an error. HaRP does not start with a malformed entry or a true/false value it does not recognize. + - The source and the target of a mount must be absolute paths without a `..` segment, `:` or control characters. The mode should be `ro` or `rw`; any other mode mounts read-write (see `HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL`). The Kubernetes backend does not support bind mounts. + - **Upgrading:** AppAPI stores the mounts given when an ExApp is installed and sends them again on every update of that ExApp, after it has removed the old container. If a stored mount is refused (a denied path, or a path that is relative or contains a `..` segment, `:` or control characters), the update fails and the ExApp is left without a container. All entries of the default list except `/dev`, `/run`, `/var/run`, `/root` and the `:rw` entries are refused from this release on, and so is a mount that contains one of them (such as `/run`, `/var/run`, `/etc`, `/var` or `/` themselves). For example, a mount of a Docker volume's data directory (`/var/lib/docker/volumes//_data`) needs an allowed entry below `/var/lib/docker`. Every request for a daemon whose Docker Engine port (`--harp_docker_socket_port`) is not in `HP_DOCKER_ENGINE_PORTS` is refused as well (deploying, updating, enabling, disabling, removing and AppAPI's daemon check; ExApps that are already running keep serving), so add such a port to that variable first. If you mount your own `/haproxy.cfg`, copy the new `http-request del-header` lines of `haproxy.cfg.template` into it. Before upgrading HaRP, check the mounts of your ExApps (shown in their deploy options) against these rules. AppAPI has already stored the new version when such an update fails, so `occ app_api:app:update` will not retry it, and changing the rule alone does nothing until the next ExApp update. To recover, unregister the ExApp with `occ app_api:app:unregister --force` (its data is kept unless you add `--rm-data`) and register it again, with corrected mounts or after changing the rule. + - **`HP_LOG_LEVEL`** - **Default:** `warning` - **Possible Values:** `debug`, `info`, `warning`, `error` @@ -786,13 +814,14 @@ curl -fsS \ ``` * `24000` is the **default** FRP remote port used by the HaRP container for the **built‑in/local** Docker Engine (enabled when `/var/run/docker.sock` is mounted). -* If you have connected **additional** Docker Engines via FRP, replace `24000` with the corresponding `remotePort` you configured (typically `24001–24099`). +* If you have connected **additional** Docker Engines via FRP, replace `24000` with the corresponding `remotePort` you configured (`24001-24099`). * A response body of `OK` means the Docker Engine API is reachable from HaRP. **Common outcomes** * `OK` – Success: HaRP can reach the Docker Engine on the given port. * `401 Unauthorized` – The `harp-shared-key` header does not match `HP_SHARED_KEY`. +* `403 Forbidden` - The `docker-engine-port` header is not a port listed in `HP_DOCKER_ENGINE_PORTS` (default `24000-24099`). * `503 Service Unavailable` / `504 Gateway Timeout` – Wrong docker-engine-port, FRP tunnel is down, or the Docker Engine is not reachable. * Connection errors – The address in `HP_EXAPPS_ADDRESS` (port `8780` by default) is not reachable from where you ran curl. @@ -821,7 +850,7 @@ These checks run **inside the HaRP container** (e.g., `docker exec -it appapi-ha **2) Remote Docker Engine over FRP** - For each external Docker Engine you connected via FRP (each with a unique `remotePort`, typically **24001–24099**), test the TCP port that FRP exposes **on the HaRP container**: + For each external Docker Engine you connected via FRP (each with a unique `remotePort` in the range **24001-24099**), test the TCP port that FRP exposes **on the HaRP container**: ``` # Replace 24001 with the remotePort you configured in that engine's frpc.toml diff --git a/haproxy.cfg.template b/haproxy.cfg.template index f91ea30..7718674 100644 --- a/haproxy.cfg.template +++ b/haproxy.cfg.template @@ -79,6 +79,8 @@ backend ex_apps_backend http-request set-header EX-APP-VERSION %[var(txn.exapps.exapp_version)] http-request set-header AUTHORIZATION-APP-API %[var(txn.exapps.exapp_token)] http-request set-header AA-VERSION "32" # TO-DO: temporary, remove it after we update all ExApps. + # The shared key is only meant for HaRP itself, so it is not passed on to the ExApp. + http-request del-header harp-shared-key backend ex_apps_backend_w_bruteforce mode http @@ -90,6 +92,8 @@ backend ex_apps_backend_w_bruteforce http-request set-header EX-APP-VERSION %[var(txn.exapps.exapp_version)] http-request set-header AUTHORIZATION-APP-API %[var(txn.exapps.exapp_token)] http-request set-header AA-VERSION "32" # TO-DO: temporary, remove it after we update all ExApps. + # The shared key is only meant for HaRP itself, so it is not passed on to the ExApp. + http-request del-header harp-shared-key filter spoe engine exapps-bruteforce-protection-spoe config /etc/haproxy/spoe-agent.conf ############################################################################### @@ -108,6 +112,10 @@ backend docker_engine_backend server frp_server 127.0.0.1 http-request set-dst-port var(txn.exapps.target_port) http-request set-path %[var(txn.exapps.target_path)] + # Neither the shared key nor AppAPI's Basic credentials (which carry the same key) are meant for the engine. + # These must come before the `allow` rules below, which end the processing of `http-request` rules. + http-request del-header harp-shared-key + http-request del-header authorization # docker system _ping http-request allow if { path,url_dec -m reg -i ^(/v[\d\.]+)?/_ping$ } METH_GET diff --git a/haproxy_agent.py b/haproxy_agent.py index 6c8da53..332c282 100644 --- a/haproxy_agent.py +++ b/haproxy_agent.py @@ -5,11 +5,14 @@ import asyncio import collections +import hashlib +import hmac import io import ipaddress import json import logging import os +import posixpath import re import socket import ssl @@ -25,7 +28,14 @@ from aiohttp import web from haproxyspoa.payloads.ack import AckPayload from haproxyspoa.spoa_server import SpoaServer -from pydantic import BaseModel, Field, ValidationError, computed_field, model_validator +from pydantic import ( + BaseModel, + Field, + ValidationError, + computed_field, + field_validator, + model_validator, +) # Reported by `/info`; AppAPI compares it against its minimum supported HaRP version. # Keep it in sync with the release tag and use a full `x.y.z` string: a bare float like `0.4` @@ -82,6 +92,11 @@ SPOA_AGENT = SpoaServer() DOCKER_API_HOST = "127.0.0.1" +# The ports the FRP server accepts Docker Engine tunnels on (see `allowPorts` in start.sh); HP_DOCKER_ENGINE_PORTS +# can add others for setups where HaRP shares its network namespace with the Docker Engine or a socket proxy. +DEFAULT_DOCKER_ENGINE_PORTS = "24000-24099" +# Headers whose values are never written to the log. +REDACTED_LOG_HEADERS = frozenset({"harp-shared-key", "authorization-app-api", "authorization", "cookie"}) def _parse_trusted_proxies(value: str) -> list[ipaddress.IPv4Network | ipaddress.IPv6Network]: @@ -144,6 +159,141 @@ def _parse_trusted_proxies(value: str) -> list[ipaddress.IPv4Network | ipaddress TRUSTED_PROXIES = _parse_trusted_proxies(os.environ.get("HP_TRUSTED_PROXY_IPS", "")) +# The entries of the default list after the hard core only log a warning in this release (see +# HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL): AppAPI re-sends the mounts stored at install on every ExApp update, after it +# has removed the old container, so refusing a mount that used to work would leave the ExApp without a container. +_STAGED_BIND_MOUNTS_DENIED = "/dev,/run,/var/run,/root,/etc:rw,/usr:rw,/bin:rw,/sbin:rw,/lib:rw,/lib64:rw,/var/spool:rw" +# Host paths ExApp containers may not bind mount unless HP_EXAPP_BIND_MOUNTS_DENIED says otherwise: the hard core +# (kernel interfaces, the container runtimes' sockets and state, host credentials), which no ExApp has a reason to +# mount; then device and runtime trees, root's home, and read-write access to the host's programs and spool directories. +DEFAULT_BIND_MOUNTS_DENIED = ( + "/proc,/sys,/boot,/var/lib/docker,/var/lib/containerd,/var/lib/containers," + "/run/docker.sock,/var/run/docker.sock,/run/containerd,/var/run/containerd," + "/run/podman,/var/run/podman,/run/crio,/var/run/crio," + "/etc/shadow,/etc/gshadow,/etc/sudoers,/etc/sudoers.d,/etc/ssh," + _STAGED_BIND_MOUNTS_DENIED +) +DEFAULT_BIND_MOUNTS_STAGED = frozenset(_STAGED_BIND_MOUNTS_DENIED.split(",")) + + +def _has_control_characters(value: str) -> bool: + return any(ord(char) < 32 or ord(char) == 127 for char in value) + + +def _setting_items(value: str) -> list[str]: + """Split a comma-separated setting; quotes reach us literally from an env-file or a compose list entry.""" + return [item for raw in value.split(",") if (item := raw.strip().strip("'\"").strip())] + + +def _normalize_mount_path(value: str) -> str: + """Return the normalized form of an absolute path; raise ValueError for anything else.""" + if not value.startswith("/"): + raise ValueError("must be an absolute path") + if ":" in value: + raise ValueError("must not contain ':'") + if _has_control_characters(value): + raise ValueError("must not contain control characters") + if ".." in value.split("/"): + raise ValueError("must not contain '..' segments") + return posixpath.normpath("/" + value.lstrip("/")) + + +def _parse_mount_rules(env_name: str, value: str, rw_suffix: bool = False) -> list[tuple[str, bool]]: + """Parse a comma-separated list of host paths into (path, read-write only) rules. + + A malformed entry stops the agent: silently dropping it would change which mounts are permitted. + """ + rules: list[tuple[str, bool]] = [] + for item in _setting_items(value): + rw_only = rw_suffix and item.endswith(":rw") + if rw_only: + item = item.removesuffix(":rw").strip().strip("'\"").strip() + words = item.split() + if len(words) > 1 and all(word.startswith("/") for word in words): + raise SystemExit(f"ERROR: invalid entry {item!r} in {env_name}: separate the paths with commas.") + try: + rules.append((_normalize_mount_path(item), rw_only)) + except ValueError as e: + raise SystemExit(f"ERROR: invalid entry {item!r} in {env_name}: {e}.") from None + return rules + + +def _parse_bool(env_name: str) -> bool: + """Read a true/false setting that defaults to false; anything else stops the agent instead of meaning false.""" + value = os.environ.get(env_name, "") + item = value.strip().strip("'\"").strip().lower() + if item in {"", "0", "false", "no"}: + return False + if item in {"1", "true", "yes"}: + return True + raise SystemExit(f"ERROR: invalid value {value!r} in {env_name}: expected true or false.") + + +def _rule_label(path: str, rw_only: bool) -> str: + return f"{path}:rw" if rw_only else path + + +BIND_MOUNTS_DISABLED = _parse_bool("HP_EXAPP_BIND_MOUNTS_DISABLED") +BIND_MOUNTS_ENFORCE_ALL = _parse_bool("HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL") +BIND_MOUNTS_ALLOWED = [ + path + for path, _ in _parse_mount_rules( + "HP_EXAPP_BIND_MOUNTS_ALLOWED", os.environ.get("HP_EXAPP_BIND_MOUNTS_ALLOWED", "") + ) +] +_denied_setting = os.environ.get("HP_EXAPP_BIND_MOUNTS_DENIED") +BIND_MOUNTS_DENIED = _parse_mount_rules( + "HP_EXAPP_BIND_MOUNTS_DENIED", + DEFAULT_BIND_MOUNTS_DENIED if _denied_setting is None else _denied_setting, + rw_suffix=True, +) +# Rules that only log a warning: the staged entries of the default list. A list the administrator sets is +# always enforced as written. +BIND_MOUNTS_WARN_ONLY = ( + frozenset() if _denied_setting is not None or BIND_MOUNTS_ENFORCE_ALL else DEFAULT_BIND_MOUNTS_STAGED +) +if _denied_setting is not None: + if BIND_MOUNTS_DENIED: + LOGGER.warning("HP_EXAPP_BIND_MOUNTS_DENIED replaces the default list of denied host paths.") + else: + LOGGER.warning("HP_EXAPP_BIND_MOUNTS_DENIED is empty: no host path is denied to ExApp containers.") +if BIND_MOUNTS_DISABLED: + LOGGER.info("Bind mounts for ExApp containers are disabled.") +else: + LOGGER.info( + "Bind mounts for ExApp containers: allowed below %s, denied %s%s.", + BIND_MOUNTS_ALLOWED or "any path", + [_rule_label(path, rw_only) for path, rw_only in BIND_MOUNTS_DENIED] or "nothing", + f" (only a warning for {sorted(BIND_MOUNTS_WARN_ONLY)})" if BIND_MOUNTS_WARN_ONLY else "", + ) + + +def _parse_port_ranges(env_name: str, value: str) -> list[tuple[int, int]]: + """Parse comma-separated ports ("2375") and ranges ("24000-24099"); a malformed entry stops the agent.""" + ranges: list[tuple[int, int]] = [] + for item in _setting_items(value): + first, dash, last = item.partition("-") + try: + first_port = int(first) + last_port = int(last) if dash else first_port + except ValueError: + raise SystemExit( + f"ERROR: invalid entry {item!r} in {env_name}: expected a port or a range like 24000-24099." + ) from None + if not 1 <= first_port <= last_port <= 65535: + raise SystemExit(f"ERROR: invalid entry {item!r} in {env_name}: ports must be 1-65535, ranges ascending.") + ranges.append((first_port, last_port)) + return ranges + + +# An empty value (e.g. a template passing an unset variable) means the default. +DOCKER_ENGINE_PORTS = _parse_port_ranges( + "HP_DOCKER_ENGINE_PORTS", os.environ.get("HP_DOCKER_ENGINE_PORTS", "") +) or _parse_port_ranges("HP_DOCKER_ENGINE_PORTS", DEFAULT_DOCKER_ENGINE_PORTS) +DOCKER_ENGINE_PORTS_TEXT = ", ".join( + str(first) if first == last else f"{first}-{last}" for first, last in DOCKER_ENGINE_PORTS +) +LOGGER.info("Docker Engine ports accepted from AppAPI: %s.", DOCKER_ENGINE_PORTS_TEXT) + ############################################################################### # Definitions ############################################################################### @@ -172,7 +322,7 @@ def encode_bruteforce_protection_values(self) -> Self: class ExApp(BaseModel): - exapp_token: str = Field(...) + exapp_token: str = Field(..., repr=False) # the ExApp's secret; records are written to the log exapp_version: str = Field(...) host: str = Field(...) port: int = Field(...) @@ -198,9 +348,13 @@ def _sanitize_k8s_name(raw: str) -> str: class ExAppName(BaseModel): - name: str = Field(..., description="ExApp name.") - instance_id: str = Field("", description="Nextcloud instance ID.") - role_suffix: str = Field("", description="Role suffix, e.g. 'rp', 'idx'.") + # The three parts end up in container, volume and hostname values and in Docker API URLs, + # so they are limited to the characters Docker allows in a container name. + name: str = Field(..., pattern=r"^[a-zA-Z0-9][a-zA-Z0-9_.-]*$", max_length=63, description="ExApp name.") + instance_id: str = Field("", pattern=r"^[a-zA-Z0-9_.-]*$", max_length=63, description="Nextcloud instance ID.") + role_suffix: str = Field( + "", pattern=r"^[a-zA-Z0-9_.-]*$", max_length=63, description="Role suffix, e.g. 'rp', 'idx'." + ) @computed_field @property @@ -236,14 +390,22 @@ def exapp_k8s_volume_name(self) -> str: class CreateExAppMounts(BaseModel): source: str = Field(...) target: str = Field(...) + # Free text, as `occ app_api:app:register --mount SRC:DST:MODE` stores it; anything but "ro" mounts read-write. mode: str = Field("rw") + @field_validator("source", "target") + @classmethod + def normalize_path(cls, value: str) -> str: + return _normalize_mount_path(value) + class CreateExAppPayload(ExAppName): image_id: str = Field(..., description="Docker image ID.") network_mode: str = Field(..., description="Desired NetworkMode for the container.") environment_variables: list[str] = Field([], description="ExApp environment variables.") - restart_policy: str = Field("unless-stopped", description="Desired RestartPolicy for the container.") + restart_policy: Literal["", "no", "always", "unless-stopped", "on-failure"] = Field( + "unless-stopped", description="Desired RestartPolicy for the container." + ) compute_device: Literal["cpu", "rocm", "cuda"] = Field( "cpu", description="Possible values: 'cpu', 'rocm' or 'cuda'" ) @@ -264,6 +426,15 @@ def accept_k8s_friendly_payload(cls, data: Any) -> Any: data = {**data, "network_mode": "bridge"} # Default network_mode (used only for Docker) return data + @field_validator("network_mode") + @classmethod + def validate_network_mode(cls, value: str) -> str: + """Accept 'host', 'bridge' or a network name, but not the ':' forms like 'container:'.""" + separators = ":" in value or any(char.isspace() for char in value) or _has_control_characters(value) + if not value or len(value) > 255 or separators: + raise ValueError("must be 'host', 'bridge' or the name of a network") + return value + class RemoveExAppPayload(ExAppName): remove_data: bool = Field(False, description="Flag indicating whether the Docker ExApp volume should be deleted.") @@ -379,6 +550,13 @@ async def record_ip_failure(ip_address: str | IPv4Address | IPv6Address) -> None LOGGER.warning("Recorded failure for IP %s. Failures in window: %d", ip_str, len(attempts)) +def is_shared_key(value: Any) -> bool: + """Compare a presented key with HP_SHARED_KEY in constant time; an unset or empty key never matches.""" + if not SHARED_KEY or not isinstance(value, str): + return False + return hmac.compare_digest(value.encode("utf-8", "surrogatepass"), SHARED_KEY.encode("utf-8", "surrogatepass")) + + async def record_failure_unless_trusted( ip_address: str | IPv4Address | IPv6Address, request_headers: dict[str, str] ) -> None: @@ -388,7 +566,7 @@ async def record_failure_unless_trusted( on that trusted path (e.g. reverse-proxy stripping `/exapps/`) would otherwise cause the caller to self-DoS once the blacklist window fills up. """ - if request_headers.get("harp-shared-key") == SHARED_KEY: + if is_shared_key(request_headers.get("harp-shared-key")): return await record_ip_failure(ip_address) @@ -412,11 +590,16 @@ async def is_ip_banned(ip_address: str | IPv4Address | IPv6Address) -> bool: ############################################################################### +def session_log_id(pass_cookie: str) -> str: + """A short identifier for log lines about a session; the cookie value itself is never written.""" + return hashlib.sha256(pass_cookie.encode("utf-8", "surrogatepass")).hexdigest()[:12] + + async def record_session(pass_cookie: str, nc_user: NcUser) -> None: now = time.time() async with SESSION_CACHE_LOCK: SESSION_CACHE[pass_cookie] = (nc_user, now) - LOGGER.info("Recorded session for cookie %s, User %s", pass_cookie, nc_user.user_id) + LOGGER.info("Recorded session %s for user %s", session_log_id(pass_cookie), nc_user.user_id) async def get_session(pass_cookie: str) -> NcUser | None: @@ -431,7 +614,7 @@ async def get_session(pass_cookie: str) -> NcUser | None: return nc_user # Session expired, remove it del SESSION_CACHE[pass_cookie] - LOGGER.info("Session for cookie %s expired", pass_cookie) + LOGGER.info("Session %s expired", session_log_id(pass_cookie)) return None @@ -459,7 +642,7 @@ async def _exapps_msg( request_headers = parse_headers(headers) client_ip_str = str(get_true_client_ip(client_ip, request_headers)) reply = reply.set_txn_var("true_client_ip", client_ip_str) - LOGGER.debug("Incoming request to ExApp: path=%s, headers=%s, ip=%s", path, headers, client_ip_str) + log_headers("Incoming request to ExApp: path=%s, ip=%s, headers=%s", request_headers, path, client_ip_str) # Check if the IP is banned based on failed attempts in BLACKLIST_CACHE. if await is_ip_banned(client_ip_str): @@ -500,8 +683,8 @@ async def _exapps_msg( ] ): # This is a direct request from AppAPI to ExApp using AppAPI PHP functions "requestToExAppXXX" - if request_headers["harp-shared-key"] != SHARED_KEY: - await record_ip_failure(client_ip) + if not is_shared_key(request_headers["harp-shared-key"]): + await record_ip_failure(client_ip_str) return reply.set_txn_var("bad_request", 1) authorization_app_api = request_headers["authorization-app-api"] # Use unified fetch path (cache + singleflight + K8s resolution) @@ -526,7 +709,7 @@ async def _exapps_msg( await record_failure_unless_trusted(client_ip_str, request_headers) return reply.set_txn_var("not_found", 1) except ValidationError as e: - LOGGER.error("Invalid ExApp metadata from Nextcloud: %s", e) + LOGGER.error("Invalid ExApp metadata from Nextcloud: %s", describe_validation_error(e)) return reply.set_txn_var("not_found", 1) except Exception as e: LOGGER.exception("Failed to fetch ExApp metadata from Nextcloud", exc_info=e) @@ -550,7 +733,7 @@ async def _exapps_msg( if nc_user and pass_cookie: await record_session(pass_cookie, nc_user) except ValidationError as e: - LOGGER.error("Invalid user info from Nextcloud: %s", e) + LOGGER.error("Invalid user info from Nextcloud: %s", describe_validation_error(e)) return reply.set_txn_var("unauthorized", 1) except Exception as e: LOGGER.exception("Failed to fetch user info from Nextcloud", exc_info=e) @@ -643,16 +826,30 @@ async def handle_app_api_request( ) -> AckPayload: """Handle the special case where the ExApp ID is 'app_api'.""" LOGGER.debug("Request from AppAPI received: %s", target_path) - if request_headers.get("harp-shared-key") != SHARED_KEY: + if not is_shared_key(request_headers.get("harp-shared-key")): await record_ip_failure(str_client_ip) return reply.set_txn_var("unauthorized", 1) docker_engine_port = request_headers.get("docker-engine-port") if docker_engine_port and not target_path.startswith("/docker/"): - reply = reply.set_txn_var("target_port", int(docker_engine_port)) + engine_port = parse_docker_engine_port(docker_engine_port) + if engine_port is None: + return reply.set_txn_var("forbidden", 1) + reply = reply.set_txn_var("target_port", engine_port) return reply.set_txn_var("backend", "docker_engine_backend") return reply.set_txn_var("backend", "nextcloud_control_backend") +def redact_headers(headers: dict[str, str]) -> dict[str, str]: + """Return a copy of the headers fit for the log: credentials and cookies are replaced.""" + return {k: "[REDACTED]" if k.lower() in REDACTED_LOG_HEADERS else v for k, v in headers.items()} + + +def log_headers(message: str, headers: dict[str, str], *args: Any) -> None: + """Log request headers at debug level, with credentials and cookies redacted; the headers come last.""" + if LOGGER.isEnabledFor(logging.DEBUG): + LOGGER.debug(message, *args, redact_headers(headers)) + + def parse_headers(headers_str: str) -> dict[str, str]: """Parse a string containing HTTP headers into a dictionary. @@ -790,7 +987,7 @@ async def _get_or_fetch_exapp(exapp_id: str) -> ExApp | None: async def nc_get_user(app_id: str, all_headers: dict[str, str]) -> NcUser | None: ext_headers = {k: v for k, v in all_headers.items() if k.lower() not in EXCLUDE_HEADERS_USER_INFO} - LOGGER.debug("all_headers = %s\next_headers = %s", str(all_headers), str(ext_headers)) + log_headers("Requesting user info for ExApp '%s' with headers %s", ext_headers, app_id) async with _get_nc_session().get( USER_INFO_URL, headers={**ext_headers, "harp-shared-key": SHARED_KEY}, @@ -888,20 +1085,30 @@ async def delete_exapp(request: web.Request): ############################################################################### +def frp_client_host(address: str) -> str: + """Return the host of an FRP `client_address`: "1.2.3.4:5678" or "[2001:db8::1]:5678".""" + if address.startswith("["): + return address[1:].partition("]")[0] + host, separator, _ = address.rpartition(":") + return host if separator and ":" not in host else address + + async def frp_auth(request: web.Request): if request.method != "POST": raise web.HTTPBadRequest() try: json_data = await request.json() - client_ip = str(json_data["content"]["client_address"]).split(":")[0] + client_ip = frp_client_host(str(json_data["content"]["client_address"])) except Exception: raise web.HTTPBadRequest() from None if await is_ip_banned(client_ip): return web.json_response({"reject": True, "reject_reason": "banned"}) - auth_token = json_data["content"]["metas"].get("token", "") - if auth_token == SHARED_KEY: + # frpc sends `metas: null` when no `metadatas` are configured. + metas = json_data["content"].get("metas") + auth_token = metas.get("token") if isinstance(metas, dict) else None + if is_shared_key(auth_token): return web.json_response({"reject": False, "unchange": True}) await record_ip_failure(client_ip) @@ -913,20 +1120,160 @@ async def frp_auth(request: web.Request): ############################################################################### +def describe_validation_error(error: ValidationError) -> str: + """Summarize a pydantic error as 'field: message' pairs; the submitted values themselves are left out.""" + return "; ".join( + f"{'.'.join(str(part) for part in item['loc']) or 'payload'}: {item['msg'].removeprefix('Value error, ')}" + for item in error.errors() + ) + + +def parse_docker_engine_port(value: str) -> int | None: + """Return the port of a Docker Engine the agent may talk to, or None (and log why) when the value is not one.""" + try: + port = int(value) + except ValueError: + port = None + if port is not None and any(first <= port <= last for first, last in DOCKER_ENGINE_PORTS): + return port + LOGGER.error("Invalid 'docker-engine-port' header value: %s (allowed: %s).", value, DOCKER_ENGINE_PORTS_TEXT) + return None + + def get_docker_engine_port(request: web.Request) -> int: docker_engine_port_str = request.headers.get("docker-engine-port") if not docker_engine_port_str: LOGGER.error("Missing 'docker-engine-port' header.") raise web.HTTPBadRequest(text="Missing 'docker-engine-port' header.") - try: - docker_engine_port = int(docker_engine_port_str) - if not (0 < docker_engine_port < 65536): - raise ValueError("Port out of valid range") from None - return docker_engine_port - except ValueError: - LOGGER.error("Invalid 'docker-engine-port' header value: %s", docker_engine_port_str) - raise web.HTTPBadRequest(text=f"Invalid 'docker-engine-port' header value: {docker_engine_port_str}") from None + docker_engine_port = parse_docker_engine_port(docker_engine_port_str) + if docker_engine_port is None: + raise web.HTTPBadRequest( + text=f"Invalid 'docker-engine-port' header value: {docker_engine_port_str} " + f"(allowed: {DOCKER_ENGINE_PORTS_TEXT})" + ) + return docker_engine_port + + +def check_mount_modes(mounts: list[CreateExAppMounts], enforce: bool, container_name: str = "") -> str | None: + """Return why a mount mode is refused; until enforced, a mode other than 'ro' or 'rw' only logs a warning. + + AppAPI stores modes such as "z" or "RO" as typed and re-sends them on every ExApp update; they have always + mounted read-write, so refusing them at once would leave the ExApp without a container. + """ + for mount in mounts: + if mount.mode in ("ro", "rw"): + continue + if enforce: + return ( + f"HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL refuses mode '{mount.mode}' of '{mount.source}': use 'ro' or 'rw'." + ) + LOGGER.warning( + "Bind mount of '%s'%s has mode '%s', which mounts it read-write; a later release will refuse it. Use 'ro' " + "or 'rw'; HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL=true refuses it now.", + mount.source, + f" for '{container_name}'" if container_name else "", + mount.mode, + ) + return None + + +def check_docker_resource_limits(resource_limits: dict[str, Any]) -> str | None: + """Return why the limits cannot be passed to the Docker Engine, or None when they can.""" + for limit in ("memory", "nanoCPUs"): + value = resource_limits.get(limit) + if value is not None and (isinstance(value, bool) or not isinstance(value, int) or value < 0): + return f"Payload validation error: resource_limits.{limit} must be a non-negative integer." + return None + + +def _is_same_or_below(path: str, parent: str) -> bool: + return parent == "/" or path == parent or path.startswith(f"{parent}/") + + +def _governing_rule( + path: str, mode: str, allowed: list[str], denied: list[tuple[str, bool]] +) -> tuple[bool, str] | None: + """Return (is_denied, entry) for the most specific entry covering the path, or None when no entry covers it. + + The entry with the longest path decides, a denied entry wins a tie, and ":rw" entries do not apply to + read-only mounts. This is what lets an allowed entry below a denied one permit that part of it. + """ + best: tuple[tuple[int, bool], bool, str] | None = None + for rule_path, is_denied, rw_only in [(d, True, rw) for d, rw in denied] + [(a, False, False) for a in allowed]: + if (rw_only and mode == "ro") or not _is_same_or_below(path, rule_path): + continue + key = (len(rule_path), is_denied) + if best is None or key > best[0]: + best = (key, is_denied, _rule_label(rule_path, rw_only)) + return None if best is None else (best[1], best[2]) + + +def _mount_refusal(mount: CreateExAppMounts, allowed: list[str], denied: list[tuple[str, bool]]) -> str | None: + """Return why one bind mount is refused under these rules, or None when it is permitted.""" + covered = any(_is_same_or_below(mount.source, path) for path in allowed) + governing = _governing_rule(mount.source, mount.mode, allowed, denied) + # Strict mode (`DENIED=/` with an allowed list): the missing allowed entry is the reason, not the root rule. + if allowed and not covered and (governing is None or governing[1] in ("/", "/:rw")): + return f"HP_EXAPP_BIND_MOUNTS_ALLOWED does not cover '{mount.source}'." + if governing is not None and governing[0]: + return f"HP_EXAPP_BIND_MOUNTS_DENIED rule '{governing[1]}' forbids mounting '{mount.source}' ({mount.mode})." + for denied_path, rw_only in denied: + if rw_only and mount.mode == "ro": + continue + if _is_same_or_below(denied_path, mount.source): + return ( + f"HP_EXAPP_BIND_MOUNTS_DENIED rule '{_rule_label(denied_path, rw_only)}' forbids mounting " + f"'{mount.source}' ({mount.mode}): the mount contains it." + ) + return None + + +def check_bind_mounts( + mounts: list[CreateExAppMounts], + disabled: bool, + allowed: list[str], + denied: list[tuple[str, bool]], + warn_only: frozenset[str] = frozenset(), + container_name: str = "", +) -> str | None: + """Return why a bind mount is refused, or None when all of them are permitted. + + The most specific entry covering the source decides (see `_governing_rule`); with an allowed list, a source + no entry covers is refused; and a denied path below the source refuses the mount, because mounting a parent + exposes the path as well. Denied rules listed in `warn_only` are left out of the decision; a mount they would + refuse is permitted with a warning. Paths are compared after normalization: links on the host are not resolved. + """ + if disabled and mounts: + return "HP_EXAPP_BIND_MOUNTS_DISABLED is set, so no bind mount is permitted." + enforced = [rule for rule in denied if _rule_label(*rule) not in warn_only] + for mount in mounts: + refusal = _mount_refusal(mount, allowed, enforced) + if refusal: + return refusal + if later := _mount_refusal(mount, allowed, denied): + # Offer only the changes that make the mount pass once every rule is enforced. + remedies = [] + if mount.mode != "ro" and not _mount_refusal(mount.model_copy(update={"mode": "ro"}), allowed, denied): + remedies.append("mount it read-only") + if not _mount_refusal(mount, [*allowed, mount.source], denied): + # Setting an allowed list for the first time refuses every mount it does not cover. + remedies.append( + "list it in HP_EXAPP_BIND_MOUNTS_ALLOWED" + if allowed + else "list it, together with every other host path your ExApps mount, in " + "HP_EXAPP_BIND_MOUNTS_ALLOWED" + ) + LOGGER.warning( + "Bind mount of '%s' (%s)%s is permitted for now, but a later release will refuse it: %s " + "To keep it, %s; HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL=true refuses it now.", + mount.source, + mount.mode, + f" for '{container_name}'" if container_name else "", + later, + " or ".join(remedies) or "set HP_EXAPP_BIND_MOUNTS_DENIED to your own list without that rule", + ) + return None async def docker_exapp_exists(request: web.Request): @@ -938,7 +1285,7 @@ async def docker_exapp_exists(request: web.Request): try: payload = ExAppName.model_validate(payload_dict) except ValidationError as e: - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + raise web.HTTPBadRequest(text=f"Payload validation error: {describe_validation_error(e)}") from None container_name = payload.exapp_container_name docker_api_url = f"http://{DOCKER_API_HOST}:{docker_engine_port}/containers/{container_name}/json" @@ -1001,13 +1348,28 @@ async def docker_exapp_create(request: web.Request): try: payload = CreateExAppPayload.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/create: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/create: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name volume_name = payload.exapp_container_volume image_id = payload.image_id + # Checked before the volume is created, so a refused request leaves nothing behind. + refusal = check_bind_mounts( + payload.mount_points, + BIND_MOUNTS_DISABLED, + BIND_MOUNTS_ALLOWED, + BIND_MOUNTS_DENIED, + BIND_MOUNTS_WARN_ONLY, + container_name, + ) or check_mount_modes(payload.mount_points, BIND_MOUNTS_ENFORCE_ALL, container_name) + refusal = refusal or check_docker_resource_limits(payload.resource_limits) + if refusal: + LOGGER.warning("Refusing to create container '%s': %s", container_name, refusal) + raise web.HTTPBadRequest(text=refusal) + container_config = { "Image": image_id, "Hostname": payload.name, @@ -1160,8 +1522,9 @@ async def docker_exapp_start(request: web.Request): try: payload = ExAppName.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/start: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/start: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name start_container_url = f"http://{DOCKER_API_HOST}:{docker_engine_port}/containers/{container_name}/start" @@ -1224,8 +1587,9 @@ async def docker_exapp_stop(request: web.Request): try: payload = ExAppName.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/stop: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/stop: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name stop_container_url = f"http://{DOCKER_API_HOST}:{docker_engine_port}/containers/{container_name}/stop" @@ -1288,8 +1652,9 @@ async def docker_exapp_wait_for_start(request: web.Request): try: payload = ExAppName.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/wait_for_start: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/wait_for_start: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name inspect_url = f"http://{DOCKER_API_HOST}:{docker_engine_port}/containers/{container_name}/json" @@ -1461,8 +1826,9 @@ async def docker_exapp_remove(request: web.Request): try: payload = RemoveExAppPayload.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/remove: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/remove: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name volume_name = payload.exapp_container_volume @@ -1602,8 +1968,9 @@ async def docker_exapp_image_remove(request: web.Request): try: payload = RemoveImagePayload.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/image_remove: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/image_remove: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None image_ref = payload.image_ref encoded_ref = quote(image_ref, safe="") @@ -1723,8 +2090,9 @@ async def docker_exapp_install_certificates(request: web.Request): try: payload = InstallCertificatesPayload.model_validate(payload_dict) except ValidationError as e: - LOGGER.warning("Payload validation error for /docker/exapp/install_certificates: %s", e) - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + reason = describe_validation_error(e) + LOGGER.warning("Payload validation error for /docker/exapp/install_certificates: %s", reason) + raise web.HTTPBadRequest(text=f"Payload validation error: {reason}") from None container_name = payload.exapp_container_name async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=120.0)) as session: @@ -2156,7 +2524,7 @@ async def _parse_json_payload(request: web.Request, model: type[BaseModel]) -> A try: return model.model_validate(payload_dict) except ValidationError as e: - raise web.HTTPBadRequest(text=f"Payload validation error: {e}") from None + raise web.HTTPBadRequest(text=f"Payload validation error: {describe_validation_error(e)}") from None def _k8s_error_msg(data: dict[str, Any] | None, text: str) -> str: @@ -2613,10 +2981,8 @@ async def _k8s_resolve_exapp_upstream(app_name: str) -> tuple[str, int] | None: if not K8S_ENABLED or not K8S_API_SERVER or not _get_k8s_token(): return None - try: - exapp = ExAppName(name=app_name) - except Exception: - return None + # Not validated: the lookup must keep working for ExApps whose id predates the stricter name rules. + exapp = ExAppName.model_construct(name=app_name) svc: dict[str, Any] | None = None got_k8s_error = False diff --git a/pyproject.toml b/pyproject.toml index 4879531..3950194 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -23,3 +23,8 @@ lint.extend-per-file-ignores."tests/**/*.py" = [ [tool.isort] profile = "black" + +[tool.pytest.ini_options] +# tests/test_k8s_routing.py is an integration test the K8s workflows run against a live cluster. +testpaths = ["tests/unit"] +pythonpath = ["."] diff --git a/start.sh b/start.sh index 7fee5ec..1748e7b 100644 --- a/start.sh +++ b/start.sh @@ -99,14 +99,19 @@ wait_for_tcp() { } wait_for_http() { - # $1 url, $2 timeout(s), $3 interval(s) - url="$1"; timeout="${2:-60}"; interval="${3:-0.5}" + # $1 url, $2 timeout(s), $3 interval(s), $4 optional PID of the process that serves the URL + url="$1"; timeout="${2:-60}"; interval="${3:-0.5}"; pid="${4:-}" start_ts="$(date +%s)" while :; do # --noproxy: internal loopback probes must ignore any http_proxy environment. if curl -fsS --noproxy '*' --max-time 2 "$url" >/dev/null 2>&1; then return 0 fi + # A process that has exited will not become ready: stop at once instead of waiting for the timeout. + if [ -n "$pid" ] && ! kill -0 "$pid" 2>/dev/null; then + echo "ERROR: The process serving $url exited during startup; see the error above." + return 1 + fi now="$(date +%s)"; elapsed=$(( now - start_ts )) if [ "$elapsed" -ge "$timeout" ]; then echo "ERROR: Timeout waiting for HTTP $url to become ready (after ${timeout}s)." @@ -466,7 +471,8 @@ EOF fi if [ "$HP_VERBOSE_START" -eq 1 ]; then log "INFO: Generated ${CFG_DIR}/frpc-docker.toml:" - cat "${CFG_DIR}/frpc-docker.toml" + # The token is the shared key, so it is not printed. + sed 's/^metadatas\.token = .*/metadatas.token = "[REDACTED]"/' "${CFG_DIR}/frpc-docker.toml" fi else log "INFO: ${CFG_DIR}/frpc-docker.toml already exists. Skipping generation..." @@ -480,7 +486,7 @@ AGENT_PID=$! # Wait deterministically for the agent to be ready (HTTP) and for SPOA (TCP). # Probe /heartbeat, not /info: /info can block on a K8s API reachability check. log "INFO: Waiting for HaRP Agent HTTP (GET http://127.0.0.1:8200/heartbeat) to be ready..." -wait_for_http "http://127.0.0.1:8200/heartbeat" "$HP_WAIT_AGENT_HTTP" "$HP_WAIT_INTERVAL" +wait_for_http "http://127.0.0.1:8200/heartbeat" "$HP_WAIT_AGENT_HTTP" "$HP_WAIT_INTERVAL" "$AGENT_PID" log "INFO: Waiting for SPOA port ${HP_SPOA_ADDRESS}..." wait_for_tcp "$SPOA_HOST" "$SPOA_PORT" "$HP_WAIT_SPOA" "$HP_WAIT_INTERVAL" diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py new file mode 100644 index 0000000..12a0627 --- /dev/null +++ b/tests/unit/__init__.py @@ -0,0 +1,2 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py new file mode 100644 index 0000000..dadedf5 --- /dev/null +++ b/tests/unit/conftest.py @@ -0,0 +1,22 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Unit tests import the agent as a module, which reads its configuration from the environment at import time.""" + +import os + +import pytest + +# Nothing from the developer's shell may reach that configuration. +for name in [name for name in os.environ if name.startswith(("HP_", "KUBERNETES_SERVICE_"))]: + del os.environ[name] +os.environ["NC_INSTANCE_URL"] = "http://nextcloud.local" +os.environ["HP_SHARED_KEY"] = "unit-test-shared-key" + + +@pytest.fixture(autouse=True) +def fresh_agent_caches(): + """Bans, sessions and ExApp records recorded by one test must not leak into the next.""" + import haproxy_agent + + for cache in (haproxy_agent.BLACKLIST_CACHE, haproxy_agent.SESSION_CACHE, haproxy_agent.EXAPP_CACHE): + cache.clear() diff --git a/tests/unit/payloads.py b/tests/unit/payloads.py new file mode 100644 index 0000000..69ed831 --- /dev/null +++ b/tests/unit/payloads.py @@ -0,0 +1,29 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Payloads shaped like the ones AppAPI sends, shared by the tests.""" + +# What AppAPI sends to `/docker/exapp/create` (DockerActions::deployExAppHarp). +DOCKER_CREATE_PAYLOAD = { + "name": "app-skeleton-python", + "instance_id": "", + "image_id": "ghcr.io/nextcloud/app-skeleton-python:latest", + "network_mode": "host", + "environment_variables": ["APP_ID=app-skeleton-python", "APP_PORT=23000", "HP_FRP_PORT=8782"], + "restart_policy": "unless-stopped", + "compute_device": "cpu", + "mount_points": [], + "start_container": True, + "resource_limits": {"memory": 536870912, "nanoCPUs": 1500000000}, +} +# What AppAPI sends to `/k8s/exapp/create` (KubernetesActions); the resource limits may also be given in +# Kubernetes units, which `_k8s_build_resources` accepts. +K8S_CREATE_PAYLOAD = { + "name": "app-skeleton-python", + "instance_id": "", + "role_suffix": "rp", + "image": "ghcr.io/nextcloud/app-skeleton-python:latest", + "image_pull_policy": "Never", + "environment_variables": ["APP_ID=app-skeleton-python"], + "compute_device": "cpu", + "resource_limits": {"memory": "512Mi", "cpu": "500m"}, +} diff --git a/tests/unit/test_bind_mounts.py b/tests/unit/test_bind_mounts.py new file mode 100644 index 0000000..5a7766d --- /dev/null +++ b/tests/unit/test_bind_mounts.py @@ -0,0 +1,289 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""The HP_EXAPP_BIND_MOUNTS_* rules for bind mounts of ExApp containers.""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +import haproxy_agent as agent + +DEFAULT_DENIED = agent._parse_mount_rules("TEST", agent.DEFAULT_BIND_MOUNTS_DENIED, rw_suffix=True) +# The default list, written out independently of the agent: the hard core, and the entries that only warn for now. +HARD_CORE = frozenset( + "/proc,/sys,/boot,/var/lib/docker,/var/lib/containerd,/var/lib/containers,/run/docker.sock,/var/run/docker.sock," + "/run/containerd,/var/run/containerd,/run/podman,/var/run/podman,/run/crio,/var/run/crio,/etc/shadow,/etc/gshadow," + "/etc/sudoers,/etc/sudoers.d,/etc/ssh".split(",") +) +WARN_ONLY = frozenset( + "/dev,/run,/var/run,/root,/etc:rw,/usr:rw,/bin:rw,/sbin:rw,/lib:rw,/lib64:rw,/var/spool:rw".split(",") +) + + +def mount(source, mode="rw"): + return agent.CreateExAppMounts(source=source, target="/mnt/target", mode=mode) + + +def check(source, mode="rw", allowed=(), denied=DEFAULT_DENIED, warn_only=frozenset(), container_name=""): + """Without `warn_only` every rule is enforced, as with HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL=true.""" + return agent.check_bind_mounts([mount(source, mode)], False, list(allowed), list(denied), warn_only, container_name) + + +def test_no_mounts_are_always_permitted(): + assert agent.check_bind_mounts([], True, ["/mnt"], DEFAULT_DENIED) is None + + +def test_disabled_refuses_every_mount(): + assert agent.check_bind_mounts([mount("/mnt/models")], True, [], []) == ( + "HP_EXAPP_BIND_MOUNTS_DISABLED is set, so no bind mount is permitted." + ) + + +def test_the_agent_starts_with_the_default_rules(): + assert agent.BIND_MOUNTS_DISABLED is False + assert agent.BIND_MOUNTS_ENFORCE_ALL is False + assert agent.BIND_MOUNTS_ALLOWED == [] + assert agent.BIND_MOUNTS_DENIED == DEFAULT_DENIED + assert {agent._rule_label(*rule) for rule in DEFAULT_DENIED} == HARD_CORE | WARN_ONLY + assert agent.BIND_MOUNTS_WARN_ONLY == WARN_ONLY + + +@pytest.mark.parametrize( + ("source", "mode"), + [ + ("/mnt/models", "rw"), + ("/var/lib/dockerfiles", "rw"), # only shares a name prefix with /var/lib/docker + ("/runner", "rw"), + ("/etc/localtime", "ro"), + ("/var/spool", "ro"), + ], +) +def test_permitted(source, mode): + assert check(source, mode) is None + + +@pytest.mark.parametrize( + ("source", "mode", "rule"), + [ + ("/proc", "ro", "/proc"), + ("/var/lib/docker/volumes", "rw", "/var/lib/docker"), + ("/dev/sda", "ro", "/dev"), + ("/etc/passwd", "rw", "/etc:rw"), + ("/lib64", "rw", "/lib64:rw"), + ("/etc", "ro", "/etc/shadow"), # a parent of a denied path + ("/", "ro", "/proc"), + ], +) +def test_refused(source, mode, rule): + assert f"HP_EXAPP_BIND_MOUNTS_DENIED rule '{rule}' forbids mounting '{source}' ({mode})" in check(source, mode) + + +def test_hard_core_of_the_default_list_is_refused_although_some_entries_only_warn(): + for path in HARD_CORE: + assert f"rule '{path}' forbids" in check(f"{path}/x", "ro", warn_only=WARN_ONLY) + for parent in ("/", "/var", "/etc", "/run", "/var/run"): + assert check(parent, "ro", warn_only=WARN_ONLY).endswith("the mount contains it.") + + +def test_refusal_names_the_mount_and_the_rule(): + # The rule comes first: AppAPI shows only the first 120 characters of the answer to the administrator. + assert check("/var/run/docker.sock") == ( + "HP_EXAPP_BIND_MOUNTS_DENIED rule '/var/run/docker.sock' forbids mounting '/var/run/docker.sock' (rw)." + ) + + +@pytest.mark.parametrize( + ("source", "permitted"), [("/mnt/models/llm", True), ("/srv/data", True), ("/mnt", False), ("/mnt/models2", False)] +) +def test_allowed_list(source, permitted): + result = check(source, allowed=["/mnt/models", "/srv/data"]) + assert result is None if permitted else result == f"HP_EXAPP_BIND_MOUNTS_ALLOWED does not cover '{source}'." + + +def test_a_denied_entry_wins_a_tie(): + assert "rule '/var/run/docker.sock'" in check("/var/run/docker.sock", allowed=["/var/run/docker.sock"]) + assert "rule '/etc:rw'" in check("/etc/hosts", "rw", allowed=["/etc"]) + + +@pytest.mark.parametrize( + ("allowed", "source", "rule"), + [ + ("/var/run/mysqld", "/var/run/mysqld/mysqld.sock", None), + ("/usr/local/models", "/usr/local/models", None), + ("/var/run/mysqld", "/var/run/docker.sock", "/var/run/docker.sock"), # only its own subtree + ("/var/run/mysqld", "/var/run", "/var/run"), # the parent itself stays refused + ("/usr/local/models", "/usr/bin", "/usr:rw"), + ("/var", "/var", "/var/lib/docker"), # an allowed parent of a denied path + ], +) +def test_an_allowed_entry_below_a_denied_one_permits_only_its_own_part(allowed, source, rule): + result = check(source, allowed=[allowed]) + assert result is None if rule is None else f"rule '{rule}'" in result + + +def test_custom_denied_lists(): + strict = [("/", False)] + assert check("/mnt/models/llm", allowed=["/mnt/models"], denied=strict) is None + assert ( + check("/opt", allowed=["/mnt/models"], denied=strict) == "HP_EXAPP_BIND_MOUNTS_ALLOWED does not cover '/opt'." + ) + # Without an allowed list, or when an allowed entry ties with it, denying the root refuses and names the rule. + assert "rule '/'" in check("/opt", denied=strict) + assert "rule '/'" in check("/opt", allowed=["/"], denied=strict) + assert check("/opt", "ro", denied=[("/", True)]) is None + assert check("/", denied=[]) is None + # A ":rw" entry inside a read-only mount does not refuse it. + assert check("/srv/data", "ro", denied=[("/srv/data/config", True)]) is None + assert check("/srv/data", "rw", denied=[("/srv/data/config", True)]).endswith("the mount contains it.") + + +def test_rules_are_parsed_from_a_comma_separated_list(): + assert agent._parse_mount_rules("TEST", "") == [] + assert agent._parse_mount_rules("TEST", " /mnt/models/ ,,'/srv/data', \"/mnt/AI models\" ") == [ + ("/mnt/models", False), + ("/srv/data", False), + ("/mnt/AI models", False), + ] + assert agent._parse_mount_rules("TEST", '"/etc:rw,/proc"', rw_suffix=True) == [("/etc", True), ("/proc", False)] + assert agent._parse_mount_rules("TEST", "'/etc':rw, /usr :rw", rw_suffix=True) == [("/etc", True), ("/usr", True)] + + +@pytest.mark.parametrize( + ("value", "rw_suffix"), + [("models", False), ("/mnt/../etc", False), ("/mnt/models:rw", False), ("/etc:ro", True), ("/proc /sys", True)], +) +def test_a_malformed_rule_stops_the_agent(value, rw_suffix): + with pytest.raises(SystemExit, match="in TEST"): + agent._parse_mount_rules("TEST", value, rw_suffix=rw_suffix) + + +FIRST_ALLOWED = "list it, together with every other host path your ExApps mount, in HP_EXAPP_BIND_MOUNTS_ALLOWED" + + +@pytest.mark.parametrize( + ("source", "mode", "allowed", "rule", "remedy"), + [ + ("/etc/localtime", "rw", [], "/etc:rw", f"mount it read-only or {FIRST_ALLOWED}"), + ("/dev/shm", "ro", [], "/dev", FIRST_ALLOWED), + # With an allowed list already set, adding the path to it is enough. + ("/var/run/mysqld/mysqld.sock", "rw", ["/var"], "/var/run", "list it in HP_EXAPP_BIND_MOUNTS_ALLOWED"), + # An allowed entry at the rule's own path ties and loses, so only the fitting remedies are offered. + ("/usr", "rw", [], "/usr:rw", "mount it read-only"), + ("/dev", "ro", [], "/dev", "set HP_EXAPP_BIND_MOUNTS_DENIED to your own list without that rule"), + ], +) +def test_a_warn_only_rule_permits_the_mount_and_logs_a_warning(caplog, source, mode, allowed, rule, remedy): + with caplog.at_level("WARNING", logger=agent.LOGGER.name): + assert check(source, mode, allowed=allowed, warn_only=WARN_ONLY, container_name="nc_app_llm2") is None + assert caplog.text.rstrip().endswith( + f"Bind mount of '{source}' ({mode}) for 'nc_app_llm2' is permitted for now, but a later release will refuse " + f"it: HP_EXAPP_BIND_MOUNTS_DENIED rule '{rule}' forbids mounting '{source}' ({mode}). To keep it, {remedy}; " + "HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL=true refuses it now." + ) + # Every remedy offered really makes the mount pass once all rules are enforced. + if "read-only" in remedy: + assert check(source, "ro", allowed=allowed) is None + if "ALLOWED" in remedy: + assert check(source, mode, allowed=[*allowed, source]) is None + + +def test_warn_only_rules_log_nothing_for_a_mount_they_do_not_decide(caplog): + with caplog.at_level("WARNING", logger=agent.LOGGER.name): + assert check("/etc/localtime", "ro", warn_only=WARN_ONLY) is None + assert check("/usr/local/models", allowed=["/usr/local/models"], warn_only=WARN_ONLY) is None + assert check("/mnt/models/llm", allowed=["/mnt/models"], warn_only=WARN_ONLY) is None + # Not covered by the allowed list: refused, whatever the read-write rule says. + assert check("/etc/localtime", allowed=["/mnt/models"], warn_only=WARN_ONLY) == ( + "HP_EXAPP_BIND_MOUNTS_ALLOWED does not cover '/etc/localtime'." + ) + assert caplog.text == "" + + +def test_a_warn_only_rule_inside_the_mount_only_warns(caplog): + with caplog.at_level("WARNING", logger=agent.LOGGER.name): + assert ( + check("/srv/data", denied=[("/srv/data/config", True)], warn_only=frozenset({"/srv/data/config:rw"})) + is None + ) + assert "rule '/srv/data/config:rw' forbids mounting '/srv/data' (rw): the mount contains it." in caplog.text + + +def test_every_mount_of_a_request_is_checked(caplog): + mounts = [mount("/etc/localtime"), mount("/var/run/docker.sock")] + with caplog.at_level("WARNING", logger=agent.LOGGER.name): + assert "rule '/var/run/docker.sock'" in agent.check_bind_mounts(mounts, False, [], DEFAULT_DENIED, WARN_ONLY) + assert "'/etc/localtime' (rw) is permitted for now" in caplog.text + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + (None, False), + (" ", False), + ("TRUE", True), + ("'1'", True), + ('"yes"', True), + ("false", False), + ("No", False), + ("0", False), + ], +) +def test_boolean_settings(monkeypatch, value, expected): + if value is not None: + monkeypatch.setenv("HP_TEST_SETTING", value) + assert agent._parse_bool("HP_TEST_SETTING") is expected + + +@pytest.mark.parametrize("value", ["on", "2", "truee"]) +def test_an_unknown_boolean_value_stops_the_agent(monkeypatch, value): + monkeypatch.setenv("HP_TEST_SETTING", value) + with pytest.raises(SystemExit, match=f"invalid value '{value}' in HP_TEST_SETTING: expected true or false"): + agent._parse_bool("HP_TEST_SETTING") + + +def run_agent_import(**env): + """Import the agent in a fresh interpreter, the way HaRP starts it, and return its log.""" + repo = Path(__file__).resolve().parents[2] + return subprocess.run( # noqa: S603 + [sys.executable, "-c", "import haproxy_agent"], + env={**os.environ, "HP_LOG_LEVEL": "INFO", "PYTHONPATH": str(repo), "PYTHONDONTWRITEBYTECODE": "1", **env}, + capture_output=True, + text=True, + timeout=60, + check=False, + ) + + +def test_settings_are_read_at_startup(): + out = run_agent_import() + assert out.returncode == 0 + assert f"only a warning for {sorted(WARN_ONLY)}" in out.stderr + assert "Docker Engine ports accepted from AppAPI: 24000-24099." in out.stderr + + out = run_agent_import( + HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL="true", + HP_EXAPP_BIND_MOUNTS_ALLOWED="/srv/data", + HP_DOCKER_ENGINE_PORTS="24000-24099, 2375", + ) + assert out.returncode == 0 + assert "allowed below ['/srv/data']" in out.stderr + assert "only a warning" not in out.stderr + assert "Docker Engine ports accepted from AppAPI: 24000-24099, 2375." in out.stderr + + out = run_agent_import(HP_EXAPP_BIND_MOUNTS_DENIED="/proc,/data:rw") + assert "HP_EXAPP_BIND_MOUNTS_DENIED replaces the default list" in out.stderr + assert "denied ['/proc', '/data:rw']." in out.stderr # a list set by the administrator is enforced as written + + out = run_agent_import(HP_EXAPP_BIND_MOUNTS_DENIED="", HP_DOCKER_ENGINE_PORTS="''") + assert "HP_EXAPP_BIND_MOUNTS_DENIED is empty: no host path is denied" in out.stderr + assert "Docker Engine ports accepted from AppAPI: 24000-24099." in out.stderr # an empty value means the default + + out = run_agent_import(HP_EXAPP_BIND_MOUNTS_DISABLED="true") + assert "Bind mounts for ExApp containers are disabled." in out.stderr + + out = run_agent_import(HP_EXAPP_BIND_MOUNTS_DENIED="/proc /sys") + assert out.returncode != 0 + assert out.stderr.splitlines()[-1].startswith("ERROR: invalid entry '/proc /sys' in HP_EXAPP_BIND_MOUNTS_DENIED") diff --git a/tests/unit/test_payload_validation.py b/tests/unit/test_payload_validation.py new file mode 100644 index 0000000..3915a50 --- /dev/null +++ b/tests/unit/test_payload_validation.py @@ -0,0 +1,119 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Validation of the payloads AppAPI sends to the `/docker/exapp/*` and `/k8s/exapp/*` endpoints.""" + +import pytest +from pydantic import ValidationError + +import haproxy_agent as agent + +from .payloads import DOCKER_CREATE_PAYLOAD, K8S_CREATE_PAYLOAD + + +def create_payload(**changes): + return agent.CreateExAppPayload.model_validate({**DOCKER_CREATE_PAYLOAD, **changes}) + + +# AppAPI 32 sends no resource limits; the restart policy defaults to unless-stopped. +@pytest.mark.parametrize("left_out", [(), ("resource_limits", "mount_points", "restart_policy")]) +def test_docker_create_payload_is_accepted(left_out): + payload = {k: v for k, v in DOCKER_CREATE_PAYLOAD.items() if k not in left_out} + assert agent.CreateExAppPayload.model_validate(payload).restart_policy == "unless-stopped" + + +def test_k8s_create_payload_is_accepted(): + payload = agent.CreateExAppPayload.model_validate(K8S_CREATE_PAYLOAD) + assert (payload.network_mode, payload.role_suffix) == ("bridge", "rp") + + +@pytest.mark.parametrize("name", ["context_chat_backend", "test-deploy", "a.b", "A1", "x" * 63]) +def test_name_accepted(name): + assert agent.ExAppName(name=name).name == name + + +@pytest.mark.parametrize("name", ["", "../etc", "a b", "a?force=true", "-a", "_a", "a\n", "ä", "x" * 64]) +def test_name_rejected(name): + with pytest.raises(ValidationError): + agent.ExAppName(name=name) + + +@pytest.mark.parametrize("field", ["instance_id", "role_suffix"]) +@pytest.mark.parametrize( + ("value", "valid"), + [("", True), ("_idx.1", True), ("a/b", False), ("a b", False), ("a\n", False), ("x" * 64, False)], +) +def test_name_parts(field, value, valid): + if valid: + assert getattr(agent.ExAppName(name="app", **{field: value}), field) == value + else: + with pytest.raises(ValidationError): + agent.ExAppName(name="app", **{field: value}) + + +@pytest.mark.parametrize("network_mode", ["host", "bridge", "nextcloud-aio", "master_bridge", "my.net"]) +def test_network_mode_accepted(network_mode): + assert create_payload(network_mode=network_mode).network_mode == network_mode + + +@pytest.mark.parametrize("network_mode", ["", "container:nc_app_other", "my net", "net\x01", "net\x7f", "n" * 256]) +def test_network_mode_rejected(network_mode): + with pytest.raises(ValidationError): + create_payload(network_mode=network_mode) + + +@pytest.mark.parametrize("restart_policy", ["", "no", "always", "unless-stopped", "on-failure"]) +def test_restart_policy_accepted(restart_policy): + assert create_payload(restart_policy=restart_policy).restart_policy == restart_policy + + +@pytest.mark.parametrize("restart_policy", ["on-failure:3", None]) +def test_restart_policy_rejected(restart_policy): + with pytest.raises(ValidationError): + create_payload(restart_policy=restart_policy) + + +@pytest.mark.parametrize( + ("path", "normalized"), + [ + ("/mnt//models/", "/mnt/models"), + ("//mnt/./models", "/mnt/models"), + ("/", "/"), + ("/mnt/AI models/", "/mnt/AI models"), + ], +) +def test_mount_paths_are_normalized(path, normalized): + mount = agent.CreateExAppMounts(source=path, target=path) + assert (mount.source, mount.target, mount.mode) == (normalized, normalized, "rw") + + +@pytest.mark.parametrize( + ("field", "path"), + [ + ("source", ""), + ("source", "./models"), + ("source", "/mnt/../etc"), + ("source", "/mnt/a:b"), + ("source", "/mnt/a\nb"), + ("source", "/mnt/a\x7fb"), + ("target", "models"), + ], +) +def test_mount_path_rejected(field, path): + with pytest.raises(ValidationError): + agent.CreateExAppMounts(**{"source": "/mnt/a", "target": "/mnt/a", field: path}) + + +@pytest.mark.parametrize("mode", ["ro", "rw", "RO", "z", ""]) +def test_mount_mode_is_kept_as_stored(mode): + # `occ app_api:app:register --mount SRC:DST:MODE` stores any text; check_mount_modes decides about it. + assert agent.CreateExAppMounts(source="/mnt/a", target="/mnt/a", mode=mode).mode == mode + + +@pytest.mark.parametrize("limits", [{}, {"memory": 0, "nanoCPUs": 1500000000}, {"memory": None}, {"cpu": "500m"}]) +def test_docker_resource_limits_accepted(limits): + assert agent.check_docker_resource_limits(limits) is None + + +@pytest.mark.parametrize("limits", [{"memory": 1.5}, {"memory": -1}, {"memory": True}, {"nanoCPUs": "2"}]) +def test_docker_resource_limits_rejected(limits): + assert "must be a non-negative integer" in agent.check_docker_resource_limits(limits) diff --git a/tests/unit/test_requests.py b/tests/unit/test_requests.py new file mode 100644 index 0000000..3ed7322 --- /dev/null +++ b/tests/unit/test_requests.py @@ -0,0 +1,516 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Shared key comparison, the `docker-engine-port` header and what the endpoints answer to a refused request.""" + +import asyncio +import ipaddress +import logging +import re + +import pytest +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer, make_mocked_request +from haproxyspoa.payloads.ack import AckPayload + +import haproxy_agent as agent + +from .payloads import DOCKER_CREATE_PAYLOAD, K8S_CREATE_PAYLOAD + +KEY = "unit-test-shared-key" +AUTH = {"harp-shared-key": KEY} + + +def txn_vars(reply: AckPayload) -> dict: + return {action.name: action.value for action in reply.actions} + + +def app_api_request(path: str, headers: dict) -> dict: + return txn_vars(asyncio.run(agent.handle_app_api_request(path, headers, "192.0.2.1", AckPayload()))) + + +def post(path: str, payload: dict, headers: dict) -> tuple[int, str]: + async def run() -> tuple[int, str]: + async with TestClient(TestServer(agent.create_web_app())) as client: + response = await client.post(path, json=payload, headers=headers) + return response.status, await response.text() + + return asyncio.run(run()) + + +@pytest.fixture +def no_docker_engine(monkeypatch): + """Fail the test when a handler tries to reach the Docker Engine.""" + + def unexpected_session(*_args, **_kwargs): + raise AssertionError("the request must be refused before the Docker Engine is contacted") + + monkeypatch.setattr(agent.aiohttp, "ClientSession", unexpected_session) + + +def test_shared_key_matches_only_the_configured_key(): + assert agent.is_shared_key(KEY) is True + for value in (KEY + "x", KEY[:-1], KEY.upper(), "", None, KEY.encode(), "\udc80"): + assert agent.is_shared_key(value) is False + + +@pytest.mark.parametrize("unset", [None, ""]) +def test_an_unset_shared_key_never_matches(monkeypatch, unset): + monkeypatch.setattr(agent, "SHARED_KEY", unset) + for value in (None, "", KEY): + assert agent.is_shared_key(value) is False + + +@pytest.mark.parametrize(("value", "port"), [("24000", 24000), ("24099", 24099), (" 24042 ", 24042)]) +def test_docker_engine_port_accepted(value, port): + assert agent.parse_docker_engine_port(value) == port + request = make_mocked_request("POST", "/docker/exapp/exists", headers={"docker-engine-port": value}) + assert agent.get_docker_engine_port(request) == port + + +@pytest.mark.parametrize("value", ["23999", "24100", "abc", "24000.0"]) +def test_docker_engine_port_rejected(value): + assert agent.parse_docker_engine_port(value) is None + request = make_mocked_request("POST", "/docker/exapp/exists", headers={"docker-engine-port": value}) + with pytest.raises(web.HTTPBadRequest) as error: + agent.get_docker_engine_port(request) + assert "allowed: 24000-24099" in error.value.text + + +def test_docker_api_requests_go_to_the_engine_tunnel(): + assert app_api_request("/v1.44/_ping", {**AUTH, "docker-engine-port": "24001"}) == { + "target_port": 24001, + "backend": "docker_engine_backend", + } + + +@pytest.mark.parametrize("port", ["23000", "abc"]) +def test_docker_api_requests_to_other_ports_are_refused(port): + assert app_api_request("/v1.44/_ping", {**AUTH, "docker-engine-port": port}) == {"forbidden": 1} + + +def test_agent_endpoints_are_not_routed_by_the_port_header(): + for headers in (AUTH, {**AUTH, "docker-engine-port": "24000"}, {**AUTH, "docker-engine-port": "8200"}): + assert app_api_request("/docker/exapp/create", headers) == {"backend": "nextcloud_control_backend"} + assert app_api_request("/info", AUTH) == {"backend": "nextcloud_control_backend"} + + +@pytest.mark.parametrize("headers", [{}, {"harp-shared-key": "wrong"}]) +def test_requests_without_the_shared_key_are_unauthorized(headers): + assert app_api_request("/info", headers) == {"unauthorized": 1} + + +@pytest.mark.parametrize( + ("mount", "reason"), + [ + ({"source": "/var/run/docker.sock", "target": "/var/run/docker.sock"}, "HP_EXAPP_BIND_MOUNTS_DENIED"), + ({"source": "models", "target": "/models"}, "mount_points.0.source: must be an absolute path"), + ], +) +def test_create_refuses_a_mount_before_contacting_the_engine(no_docker_engine, mount, reason): + status, text = post( + "/docker/exapp/create", {**DOCKER_CREATE_PAYLOAD, "mount_points": [mount]}, {"docker-engine-port": "24000"} + ) + assert status == 400 + assert reason in text + + +@pytest.mark.parametrize( + "changes", + [ + {"name": "app/../other"}, + {"resource_limits": {"memory": "512m"}}, + ], +) +def test_create_refuses_invalid_values_before_contacting_the_engine(no_docker_engine, changes): + status, _ = post("/docker/exapp/create", {**DOCKER_CREATE_PAYLOAD, **changes}, {"docker-engine-port": "24000"}) + assert status == 400 + + +@pytest.mark.parametrize("endpoint", ["exists", "start", "stop", "wait_for_start", "remove", "install_certificates"]) +def test_endpoints_refuse_an_invalid_name_and_port(no_docker_engine, endpoint): + path = f"/docker/exapp/{endpoint}" + assert post(path, {"name": "app/../other"}, {"docker-engine-port": "24000"})[0] == 400 + assert post(path, {"name": "app"}, {"docker-engine-port": "8200"})[0] == 400 + + +def test_exapp_record_repr_hides_the_token(): + record = agent.ExApp(exapp_token="app-secret-value", exapp_version="1.0.0", host="app", port=23000) + assert "app-secret-value" not in repr(record) + assert "app-secret-value" not in str(record) + assert record.exapp_token == "app-secret-value" + + +def test_incoming_request_log_has_no_secrets(caplog): + headers = "\n".join( + [ + f"harp-shared-key: {KEY}", + "Authorization: Basic YWRtaW46YWRtaW4=", + "authorization-app-api: dXNlcjpzZWNyZXQ=", + "cookie: oc_sessionPassphrase=passphrase-value", + "ex-app-id: app", + ] + ) + with caplog.at_level(logging.DEBUG, logger=agent.LOGGER.name): + asyncio.run(agent._exapps_msg("/exapps/app_api/info", headers, ipaddress.ip_address("192.0.2.1"), "")) + assert "Incoming request to ExApp: path=/exapps/app_api/info, ip=192.0.2.1, headers=" in caplog.text + assert "'ex-app-id': 'app'" in caplog.text + assert caplog.text.count("[REDACTED]") == 4 + for secret in (KEY, "YWRtaW46YWRtaW4=", "dXNlcjpzZWNyZXQ=", "passphrase-value"): + assert secret not in caplog.text + + +def test_session_log_lines_have_no_cookie_value(caplog): + user = agent.NcUser(user_id="admin", access_level=agent.AccessLevel.ADMIN) + with caplog.at_level(logging.INFO, logger=agent.LOGGER.name): + asyncio.run(agent.record_session("passphrase-value", user)) + assert asyncio.run(agent.get_session("passphrase-value")) == user + agent.SESSION_CACHE["passphrase-value"] = (user, 0.0) # recorded long ago + assert asyncio.run(agent.get_session("passphrase-value")) is None + session_id = agent.session_log_id("passphrase-value") + assert re.fullmatch(r"[0-9a-f]{12}", session_id) + assert f"Recorded session {session_id} for user admin" in caplog.text + assert f"Session {session_id} expired" in caplog.text + assert "passphrase-value" not in caplog.text + + +def frp_login(content: dict) -> tuple[int, str]: + return post("/frp_handler", {"version": "0.1.0", "op": "Login", "content": content}, {}) + + +def test_frp_login_with_the_shared_key_is_accepted(): + status, text = frp_login({"client_address": "198.51.100.7:40000", "metas": {"token": KEY}}) + assert (status, text) == (200, '{"reject": false, "unchange": true}') + assert agent.BLACKLIST_CACHE.get("198.51.100.7", []) == [] # no failure recorded + + +@pytest.mark.parametrize( + "content", + [ + {"client_address": "198.51.100.7:40000", "metas": {"token": "wrong"}}, + {"client_address": "198.51.100.7:40000", "metas": {"token": 12345}}, + {"client_address": "198.51.100.7:40000", "metas": None}, # frpc without `metadatas` + ], +) +def test_frp_login_without_the_shared_key_is_refused_and_counted(content): + status, _ = frp_login(content) + assert status == 400 + assert len(agent.BLACKLIST_CACHE["198.51.100.7"]) == 1 + + +def test_validation_errors_name_the_field_and_leave_out_the_value(no_docker_engine): + payload = {**DOCKER_CREATE_PAYLOAD, "environment_variables": {"APP_SECRET": "secret-value"}, "name": "a/b"} + status, text = post("/docker/exapp/create", payload, {"docker-engine-port": "24000"}) + assert status == 400 + assert text.startswith("Payload validation error: name: ") + assert "; environment_variables: " in text + assert "secret-value" not in text + assert "errors.pydantic.dev" not in text + assert "\n" not in text + + +def test_engine_ports_are_parsed_from_a_comma_separated_list(): + assert agent._parse_port_ranges("TEST", "24000-24099") == [(24000, 24099)] + assert agent._parse_port_ranges("TEST", " '24000-24099', 2375 ,,") == [(24000, 24099), (2375, 2375)] + assert agent._parse_port_ranges("TEST", "1,1-65535") == [(1, 1), (1, 65535)] + + +@pytest.mark.parametrize("value", ["abc", "24099-24000", "0", "65536", "24000-"]) +def test_a_malformed_engine_port_list_stops_the_agent(value): + with pytest.raises(SystemExit, match="TEST"): + agent._parse_port_ranges("TEST", value) + + +def test_extra_engine_ports_are_accepted_when_configured(monkeypatch): + monkeypatch.setattr(agent, "DOCKER_ENGINE_PORTS", [(24000, 24099), (2375, 2375)]) + monkeypatch.setattr(agent, "DOCKER_ENGINE_PORTS_TEXT", "24000-24099, 2375") + assert agent.parse_docker_engine_port("2375") == 2375 + assert agent.parse_docker_engine_port("2376") is None + assert app_api_request("/v1.44/_ping", {**AUTH, "docker-engine-port": "2375"}) == { + "target_port": 2375, + "backend": "docker_engine_backend", + } + request = make_mocked_request("POST", "/docker/exapp/exists", headers={"docker-engine-port": "2376"}) + with pytest.raises(web.HTTPBadRequest) as error: + agent.get_docker_engine_port(request) + assert "allowed: 24000-24099, 2375" in error.value.text + + +@pytest.mark.parametrize( + ("address", "host"), + [ + ("198.51.100.7:40000", "198.51.100.7"), + ("[2001:db8::1]:40000", "2001:db8::1"), + ("198.51.100.7", "198.51.100.7"), + ("2001:db8::1", "2001:db8::1"), + ], +) +def test_frp_client_host(address, host): + assert agent.frp_client_host(address) == host + + +def test_failed_frp_logins_from_ipv6_clients_are_counted_per_address(): + for address in ("[2001:db8::1]:40000", "[2001:db8::2]:40000"): + assert frp_login({"client_address": address, "metas": {"token": "wrong"}})[0] == 400 + # `is_ip_banned` creates an empty entry for every address it sees, so count the recorded failures. + assert {ip: len(failures) for ip, failures in agent.BLACKLIST_CACHE.items()} == {"2001:db8::1": 1, "2001:db8::2": 1} + + +def test_a_wrong_key_on_an_appapi_request_counts_against_the_client_behind_the_proxy(monkeypatch): + monkeypatch.setattr(agent, "TRUSTED_PROXIES", [ipaddress.ip_network("10.0.0.0/8")]) + headers = "\n".join( + [ + "x-forwarded-for: 198.51.100.20", + "ex-app-version: 1.0.0", + "ex-app-id: app", + "ex-app-host: app", + "ex-app-port: 23000", + "authorization-app-api: eA==", + "harp-shared-key: wrong", + ] + ) + reply = asyncio.run(agent._exapps_msg("/exapps/app/heartbeat", headers, ipaddress.ip_address("10.0.0.5"), "")) + assert txn_vars(reply)["bad_request"] == 1 + assert len(agent.BLACKLIST_CACHE["198.51.100.20"]) == 1 + assert agent.BLACKLIST_CACHE.get("10.0.0.5", []) == [] # not the proxy + + +def test_k8s_create_refuses_an_invalid_role_before_contacting_the_cluster(no_docker_engine): + status, text = post("/k8s/exapp/create", {**K8S_CREATE_PAYLOAD, "role_suffix": "web ui"}, {}) + assert status == 400 + assert text.startswith("Payload validation error: role_suffix: ") + + +def test_user_info_requests_keep_the_credentials_but_the_log_does_not(monkeypatch, caplog): + sent = {} + + class Response: + ok = True + + async def json(self): + return {"user_id": "admin", "access_level": 2} + + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return False + + class Session: + def get(self, url, headers, params): + sent.update(headers) + return Response() + + monkeypatch.setattr(agent, "_get_nc_session", Session) + headers = {"authorization": "Basic YWRtaW46YWRtaW4=", "cookie": "oc_sessionPassphrase=passphrase-value"} + with caplog.at_level(logging.DEBUG, logger=agent.LOGGER.name): + user = asyncio.run(agent.nc_get_user("app", headers)) + assert user.user_id == "admin" + assert sent["authorization"] == "Basic YWRtaW46YWRtaW4=" # Nextcloud still gets them + assert sent["cookie"] == "oc_sessionPassphrase=passphrase-value" + assert "Requesting user info for ExApp 'app'" in caplog.text + assert "YWRtaW46YWRtaW4=" not in caplog.text + assert "passphrase-value" not in caplog.text + + +def test_create_refuses_a_read_write_system_mount_once_enforced(no_docker_engine, monkeypatch): + # By default the read-write entries only warn (see test_bind_mounts.py); HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL + # empties the warn-only set. + monkeypatch.setattr(agent, "BIND_MOUNTS_WARN_ONLY", frozenset()) + mount = {"source": "/etc/hosts", "target": "/etc/hosts", "mode": "rw"} + status, text = post( + "/docker/exapp/create", {**DOCKER_CREATE_PAYLOAD, "mount_points": [mount]}, {"docker-engine-port": "24000"} + ) + assert (status, text) == (400, "HP_EXAPP_BIND_MOUNTS_DENIED rule '/etc:rw' forbids mounting '/etc/hosts' (rw).") + + +def test_a_valid_shared_key_is_never_counted_as_a_failure(): + asyncio.run(agent.record_failure_unless_trusted("192.0.2.9", {"harp-shared-key": KEY})) + assert agent.BLACKLIST_CACHE.get("192.0.2.9", []) == [] + asyncio.run(agent.record_failure_unless_trusted("192.0.2.9", {"harp-shared-key": "wrong"})) + asyncio.run(agent.record_failure_unless_trusted("192.0.2.9", {})) + assert len(agent.BLACKLIST_CACHE["192.0.2.9"]) == 2 + + +@pytest.mark.parametrize( + ("payload", "port"), + [({"image_ref": ""}, "24000"), ({"image_ref": "alpine:latest"}, "8200")], +) +def test_image_remove_refuses_invalid_requests_before_contacting_the_engine(no_docker_engine, payload, port): + assert post("/docker/exapp/image_remove", payload, {"docker-engine-port": port})[0] == 400 + + +def test_create_permits_a_staged_mount_and_sends_the_normalized_mounts_to_the_engine(monkeypatch, caplog): + sent = [] + + class Response: + def __init__(self, status, body=None): + self.status, self.body = status, body or {} + + async def json(self): + return self.body + + async def text(self): + return "" + + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return False + + class Session: + def __init__(self, *_args, **_kwargs): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return False + + def get(self, url): + return Response(200) # the volume exists + + def post(self, url, json): + sent.append((url, json)) + return Response(201, {"Id": "abc"}) + + monkeypatch.setattr(agent.aiohttp, "ClientSession", Session) + mounts = [ + {"source": "/dev/shm/", "target": "/dev/shm"}, + {"source": "/etc//localtime", "target": "/etc/localtime", "mode": "ro"}, + {"source": "/srv/data", "target": "/data", "mode": "z"}, # an SELinux option typed into occ + ] + with caplog.at_level(logging.WARNING, logger=agent.LOGGER.name): + status, _ = post( + "/docker/exapp/create", {**DOCKER_CREATE_PAYLOAD, "mount_points": mounts}, {"docker-engine-port": "24000"} + ) + assert status == 201 + assert "Bind mount of '/dev/shm' (rw) for 'nc_app_app-skeleton-python' is permitted for now" in caplog.text + assert "Bind mount of '/srv/data' for 'nc_app_app-skeleton-python' has mode 'z', which mounts it read-write" in ( + caplog.text + ) + binds = [m for m in sent[-1][1]["HostConfig"]["Mounts"] if m["Type"] == "bind"] + assert [(m["Source"], m["Readonly"]) for m in binds] == [ + ("/dev/shm", False), + ("/etc/localtime", True), + ("/srv/data", False), + ] + + +def appapi_request_headers(key: str) -> str: + return "\n".join( + [ + "ex-app-version: 1.0.0", + "ex-app-id: app", + "ex-app-host: 127.0.0.1", + "ex-app-port: 23000", + "authorization-app-api: eA==", + f"harp-shared-key: {key}", + ] + ) + + +def test_an_appapi_request_with_the_shared_key_is_routed_to_the_exapp(monkeypatch): + async def no_record(_app_id): + return None + + monkeypatch.setattr(agent, "_get_or_fetch_exapp", no_record) + reply = asyncio.run( + agent._exapps_msg("/exapps/app/heartbeat", appapi_request_headers(KEY), ipaddress.ip_address("192.0.2.1"), "") + ) + assert txn_vars(reply) == { + "true_client_ip": "192.0.2.1", + "target_path": "/heartbeat", + "backend": "ex_apps_backend", + "target_ip": "127.0.0.1", + "target_port": 23000, + "exapp_token": "eA==", + "exapp_version": "1.0.0", + "exapp_id": "app", + } + assert agent.BLACKLIST_CACHE.get("192.0.2.1", []) == [] + + +@pytest.mark.parametrize( + ("lookup", "model", "path", "headers", "answer", "message"), + [ + ("_get_or_fetch_exapp", agent.ExApp, "/exapps/app/x", "", "not_found", "Invalid ExApp metadata from Nextcloud"), + ( + "nc_get_user", + agent.NcUser, + "/exapps/app/x", + "authorization: Basic eA==", + "unauthorized", + "Invalid user info", + ), + ], +) +def test_invalid_answers_from_nextcloud_are_logged_without_their_values( + monkeypatch, caplog, lookup, model, path, headers, answer, message +): + async def invalid_answer(*_args): + model.model_validate({"exapp_token": "secret-value", "access_level": "secret-value"}) + + async def record(_app_id): + return agent.ExApp(exapp_token="", exapp_version="1", host="127.0.0.1", port=23000) + + monkeypatch.setattr(agent, "_get_or_fetch_exapp", record) + monkeypatch.setattr(agent, lookup, invalid_answer) + with caplog.at_level(logging.ERROR, logger=agent.LOGGER.name): + reply = asyncio.run(agent._exapps_msg(path, headers, ipaddress.ip_address("192.0.2.1"), "")) + assert txn_vars(reply)[answer] == 1 + assert message in caplog.text + assert "secret-value" not in caplog.text + + +@pytest.mark.parametrize("mode", ["z", "RO", "ro,z", ""]) +def test_a_mode_other_than_ro_or_rw_mounts_read_write_until_enforced(caplog, mode): + mounts = [agent.CreateExAppMounts(source="/srv/data", target="/data", mode=mode)] + with caplog.at_level(logging.WARNING, logger=agent.LOGGER.name): + assert agent.check_mount_modes(mounts, False, "nc_app_x") is None + assert f"Bind mount of '/srv/data' for 'nc_app_x' has mode '{mode}', which mounts it read-write" in caplog.text + assert agent.check_mount_modes(mounts, True) == ( + f"HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL refuses mode '{mode}' of '/srv/data': use 'ro' or 'rw'." + ) + + +def test_ro_and_rw_modes_need_no_warning(caplog): + mounts = [agent.CreateExAppMounts(source="/srv/data", target="/data", mode=mode) for mode in ("ro", "rw")] + with caplog.at_level(logging.WARNING, logger=agent.LOGGER.name): + assert agent.check_mount_modes(mounts, True) is None + assert agent.check_mount_modes(mounts, False) is None + assert caplog.text == "" + + +def test_create_refuses_an_unknown_mode_once_enforced(no_docker_engine, monkeypatch): + monkeypatch.setattr(agent, "BIND_MOUNTS_ENFORCE_ALL", True) + mount = {"source": "/srv/data", "target": "/data", "mode": "RO"} + status, text = post( + "/docker/exapp/create", {**DOCKER_CREATE_PAYLOAD, "mount_points": [mount]}, {"docker-engine-port": "24000"} + ) + assert (status, text) == ( + 400, + "HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL refuses mode 'RO' of '/srv/data': use 'ro' or 'rw'.", + ) + + +def test_k8s_service_lookup_works_for_any_app_id(monkeypatch): + calls = [] + + async def k8s_request(method, path, **kwargs): + calls.append(path) + # No Service of that name, and none labelled for the ExApp. + return (200, {"items": []}, "") if "query" in kwargs else (404, None, "") + + async def no_manual_upstream(*_args): + return None + + monkeypatch.setattr(agent, "K8S_ENABLED", True) + monkeypatch.setattr(agent, "K8S_API_SERVER", "https://k8s.example") + monkeypatch.setattr(agent, "_get_k8s_token", lambda: "token") + monkeypatch.setattr(agent, "_k8s_request", k8s_request) + monkeypatch.setattr(agent, "_k8s_resolve_manual_upstream", no_manual_upstream) + # An id outside the name rules for new containers still gets its Service looked up. + assert asyncio.run(agent._k8s_resolve_exapp_upstream("_hidden")) is None + assert calls[0].endswith("/services/nc-app-hidden")