From 4a68d9aa911a34ad85dd6ec52b01b114032d8523 Mon Sep 17 00:00:00 2001 From: Ryan Adams Date: Thu, 1 Oct 2026 08:51:19 -0600 Subject: [PATCH] fix: validate hostnames and paths built from scenario metadata Apps and SCORCH components build host-side file paths from scenario metadata (hostnames, device names, filenames from VMs) without checking them, so a value containing '/' or '..' could write outside the experiment directory. Add two shared helpers in common/utils.py: - validate_hostname(): phenix core's node-name rules as of v2026.10.02 (2 to 63 letters, digits and interior hyphens, not all digits, not 'all' or 'phenix'). external_node hosts in particular only exist in scenario metadata and bypass core's check. - safe_join(): joins parts onto a base and rejects a result that resolves outside it, including absolute parts that would replace the base. Use them in AppBase.add_node, caldera, helics, ignition, otsim, protonuke, scale and its plugins, sceptre, wireguard, and the cc, ssh and providerdata SCORCH components. mm_send/mm_recv keep VM-side paths inside the mount and mm_recv requires a normalized absolute host destination. Generated configs also stop accepting embedded newlines (protonuke args, wireguard fields), wireguard configs holding the private key are written 0600, and the sceptre Windows startup scripts drop from 0777 to 0755. Files touched here, including all of common/utils.py, also move from os.path to pathlib, since safe_join returns a Path. utils.abs_path() now always returns a Path. test_path_helpers.py pins the helpers' behavior. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo --- CHANGELOG.md | 6 + src/python/phenix_apps/apps/__init__.py | 7 +- src/python/phenix_apps/apps/caldera/app.py | 70 ++++++----- src/python/phenix_apps/apps/helics/app.py | 26 +++-- .../phenix_apps/apps/ignition/README.md | 2 +- src/python/phenix_apps/apps/ignition/app.py | 15 ++- .../apps/ignition/tests/test_ignition.py | 6 + src/python/phenix_apps/apps/otsim/app.py | 36 +++--- src/python/phenix_apps/apps/protonuke/app.py | 29 +++-- src/python/phenix_apps/apps/scale/app.py | 31 ++--- .../apps/scale/plugins/builtin/README.md | 2 +- .../apps/scale/plugins/builtin/plugin.py | 6 +- .../apps/scale/plugins/wind_turbine/README.md | 1 + .../apps/scale/plugins/wind_turbine/plugin.py | 26 +++-- .../wind_turbine/tests/test_wind_turbine.py | 12 +- .../apps/scale/tests/test_scale.py | 27 +++-- src/python/phenix_apps/apps/sceptre/app.py | 31 +++-- .../phenix_apps/apps/sceptre/configure.py | 4 +- src/python/phenix_apps/apps/scorch/cc/cc.py | 34 +++--- .../apps/scorch/providerdata/providerdata.py | 22 ++-- src/python/phenix_apps/apps/scorch/ssh/ssh.py | 16 ++- src/python/phenix_apps/apps/wireguard/app.py | 45 ++++++-- .../common/tests/test_path_helpers.py | 109 ++++++++++++++++++ src/python/phenix_apps/common/utils.py | 85 ++++++++++---- 24 files changed, 473 insertions(+), 175 deletions(-) create mode 100644 src/python/phenix_apps/common/tests/test_path_helpers.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 55fe62fc..245881f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **SCEPTRE App**: `configure()` and `pre_start()` are stage classes, `ConfigureStage` and `PreStart`, sharing pre-start state through `PreStartState`. - **SCEPTRE App**: The device-type table is `configs/infrastructures.yaml`; adding a device type needs no code change. - **SCEPTRE App**: Injections are declared with `Sceptre.inject()`, and all of them in the configure stage. +- **Common**: `utils.abs_path()` always returns a `Path`. - **SCEPTRE App**: Type annotations on every app function, `Final` on module constants. - **SCEPTRE App**: Validation failures raise `error.AppError` instead of calling `sys.exit(1)`, matching the app contract. - **SCEPTRE App**: `metadata.simulator` matches case-insensitively in both stages, as validation already did; a miscased name used to silently get the default config. @@ -37,6 +38,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **SCEPTRE App**: A `fep` without a mgmt interface raised `UnboundLocalError`, or reused the previous fep's endpoints. - **SCEPTRE App**: A historian on a subnet with no OPC server was configured with an unrelated OPC's tag list and no address to collect from. It now gets no tags and a warning naming the subnet. +### Security +- **Common**: New `validate_hostname()` (phenix v2026.10.02 rules) and `safe_join()` guard hostnames and paths built from metadata; `mm_send()`/`mm_recv()` stay inside the miniccc mount. **Breaking:** rejects `_`, 1-char, all-digit, `all` and `phenix` hostnames. +- **Apps**: Scale, wind turbine and Ignition names are restricted; protonuke and wireguard reject embedded newlines; wireguard configs are `0600`, sceptre startup scripts `0755`. +- **SCORCH**: `cc`, `ssh` and `providerdata` transfers stay inside the run directory. + ## [2.0.0] - 2026-03-04 ### Changed diff --git a/src/python/phenix_apps/apps/__init__.py b/src/python/phenix_apps/apps/__init__.py index 62785d7c..2195584e 100644 --- a/src/python/phenix_apps/apps/__init__.py +++ b/src/python/phenix_apps/apps/__init__.py @@ -3,6 +3,7 @@ import os import re import sys +from pathlib import Path from typing import Any from box import Box @@ -45,7 +46,7 @@ def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: self.topo = self.get_annotation("topology") # Create the experiment directory if it doesn't exist - os.makedirs(self.exp_dir, exist_ok=True) + Path(self.exp_dir).mkdir(parents=True, exist_ok=True) # Mako templates directory inside the app's code folder py_path = sys.modules[self.__class__.__module__].__file__ @@ -347,6 +348,8 @@ def extract_node_hostname_for_ip(self, address: str) -> str | None: return None def add_node(self, new_node: Box | dict, overwrite: bool = False) -> None: + utils.validate_hostname(new_node["general"]["hostname"]) + found = None for idx, node in enumerate(self.experiment.spec.topology.nodes): @@ -435,7 +438,7 @@ def render(self, template_name: str, file_path: str, **kwargs) -> str: Returns the file path written to. """ - with open(file_path, "w") as fp: + with Path(file_path).open("w") as fp: utils.mako_serve_template( template_name=template_name, templates_dir=self.templates_dir, diff --git a/src/python/phenix_apps/apps/caldera/app.py b/src/python/phenix_apps/apps/caldera/app.py index 5962ab23..da7df357 100644 --- a/src/python/phenix_apps/apps/caldera/app.py +++ b/src/python/phenix_apps/apps/caldera/app.py @@ -1,8 +1,9 @@ import ipaddress as ipaddr -import os +from pathlib import Path from phenix_apps.apps import AppBase from phenix_apps.common import settings, utils +from phenix_apps.common.error import AppError from phenix_apps.common.logger import logger @@ -10,11 +11,13 @@ class Caldera(AppBase): def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) - self.app_dir: str = f"{self.exp_dir}/caldera" - os.makedirs(self.app_dir, exist_ok=True) + self.app_dir: Path = Path(self.exp_dir) / "caldera" + self.app_dir.mkdir(parents=True, exist_ok=True) - self.files_dir: str = f"{settings.PHENIX_DIR}/images/{self.exp_name}/caldera" - os.makedirs(self.files_dir, exist_ok=True) + self.files_dir: Path = ( + Path(settings.PHENIX_DIR) / "images" / self.exp_name / "caldera" + ) + self.files_dir.mkdir(parents=True, exist_ok=True) def configure(self): logger.info(f"Configuring user app: {self.name}") @@ -22,10 +25,12 @@ def configure(self): md = self.metadata for idx, server in enumerate(md.get("servers", [])): + hostname = utils.validate_hostname(server.get("hostname", f"caldera-{idx}")) + node = { "type": "VirtualMachine", "general": { - "hostname": server.get("hostname", f"caldera-{idx}"), + "hostname": hostname, "vm_type": "kvm", }, "hardware": { @@ -65,7 +70,7 @@ def pre_start(self): md = self.metadata for idx, server in enumerate(md.get("servers", [])): - hostname = server.get("hostname", f"caldera-{idx}") + hostname = utils.validate_hostname(server.get("hostname", f"caldera-{idx}")) node = self.extract_node(hostname) addr = node.network.interfaces[0].address @@ -73,7 +78,7 @@ def pre_start(self): for fact in server.get("facts", []): inject = { "src": fact, - "dst": f"/opt/caldera/data/sources/{os.path.basename(fact)}", + "dst": f"/opt/caldera/data/sources/{Path(fact).name}", } self.add_inject(hostname, inject) @@ -81,7 +86,7 @@ def pre_start(self): for adversary in server.get("adversaries", []): inject = { "src": adversary, - "dst": f"/opt/caldera/data/adversaries/{os.path.basename(adversary)}", + "dst": f"/opt/caldera/data/adversaries/{Path(adversary).name}", } self.add_inject(hostname, inject) @@ -89,45 +94,45 @@ def pre_start(self): if server.get("config"): config_file = server.get("config") else: - config_file = f"{self.app_dir}/{hostname}-config.yml" + config_file = utils.safe_join(self.app_dir, f"{hostname}-config.yml") - with open(config_file, "w") as f: + with config_file.open("w") as f: utils.mako_serve_template("default_config.mako", templates, f) inject = { - "src": config_file, + "src": str(config_file), "dst": "/opt/caldera/conf/default.yml", } self.add_inject(hostname, inject) - firefox_bookmark_config_file = ( - f"{self.app_dir}/{hostname}-firefox-policies.json" + firefox_bookmark_config_file = utils.safe_join( + self.app_dir, f"{hostname}-firefox-policies.json" ) - with open(firefox_bookmark_config_file, "w") as f: + with firefox_bookmark_config_file.open("w") as f: utils.mako_serve_template( "firefox_bookmark.mako", templates, f, addr=addr ) inject = { - "src": firefox_bookmark_config_file, + "src": str(firefox_bookmark_config_file), "dst": "/etc/firefox/policies/policies.json", } self.add_inject(hostname, inject) - firefox_autostart_config_file = ( - f"{self.app_dir}/{hostname}-firefox-autostart.json" + firefox_autostart_config_file = utils.safe_join( + self.app_dir, f"{hostname}-firefox-autostart.json" ) - with open(firefox_autostart_config_file, "w") as f: + with firefox_autostart_config_file.open("w") as f: utils.mako_serve_template( "firefox_autostart.mako", templates, f, addr=addr ) inject = { - "src": firefox_autostart_config_file, + "src": str(firefox_autostart_config_file), "dst": "/root/.config/autostart/Caldera.desktop", } @@ -136,6 +141,8 @@ def pre_start(self): hosts = self.extract_all_nodes(False) for host in hosts: + utils.validate_hostname(host.hostname) + try: addr = ipaddr.ip_address(host.metadata.server) except ValueError: @@ -151,9 +158,18 @@ def pre_start(self): addr = node.network.interfaces[iface].address if host.topology.hardware.os_type == "windows": - agent_file = f"{self.app_dir}/{host.hostname}-sandcat-agent.ps1" + if not (Path(templates) / "windows_agent.mako").exists(): + raise AppError( + f"cannot generate sandcat agent for Windows host " + f"'{host.hostname}': windows_agent.mako template is missing " + f"from {templates}" + ) - with open(agent_file, "w") as f: + agent_file = utils.safe_join( + self.app_dir, f"{host.hostname}-sandcat-agent.ps1" + ) + + with agent_file.open("w") as f: utils.mako_serve_template( "windows_agent.mako", templates, f, addr=addr ) @@ -161,14 +177,16 @@ def pre_start(self): self.add_inject( hostname=host.hostname, inject={ - "src": agent_file, + "src": str(agent_file), "dst": "/phenix/startup/90-sandcat-agent.ps1", }, ) elif host.topology.hardware.os_type == "linux": - agent_file = f"{self.app_dir}/{host.hostname}-sandcat-agent.sh" + agent_file = utils.safe_join( + self.app_dir, f"{host.hostname}-sandcat-agent.sh" + ) - with open(agent_file, "w") as f: + with agent_file.open("w") as f: utils.mako_serve_template( "linux_agent.mako", templates, f, addr=addr ) @@ -176,7 +194,7 @@ def pre_start(self): self.add_inject( hostname=host.hostname, inject={ - "src": agent_file, + "src": str(agent_file), "dst": "/etc/phenix/startup/90-sandcat-agent.sh", }, ) diff --git a/src/python/phenix_apps/apps/helics/app.py b/src/python/phenix_apps/apps/helics/app.py index fe3a50de..e31bf380 100644 --- a/src/python/phenix_apps/apps/helics/app.py +++ b/src/python/phenix_apps/apps/helics/app.py @@ -1,4 +1,4 @@ -import os +from pathlib import Path from phenix_apps.apps import AppBase from phenix_apps.common import utils @@ -9,8 +9,8 @@ class Helics(AppBase): def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) - self.helics_dir: str = f"{self.exp_dir}/helics" - os.makedirs(self.helics_dir, exist_ok=True) + self.helics_dir: Path = Path(self.exp_dir) / "helics" + self.helics_dir.mkdir(parents=True, exist_ok=True) def pre_start(self): logger.info(f"Starting user application: {self.name}") @@ -47,8 +47,8 @@ def pre_start(self): # create the wait script to be injected into federates templates = utils.abs_path(__file__, "templates/") - wait_file = f"{self.helics_dir}/wait-broker.sh" - with open(wait_file, "w") as f: + wait_file = self.helics_dir / "wait-broker.sh" + with wait_file.open("w") as f: utils.mako_serve_template( "wait_broker.mako", templates, f, rootbroker_ip=root_ip ) @@ -72,7 +72,8 @@ def pre_start(self): if configs and configs[0].get("broker-wait", True): dst = "/etc/phenix/startup/5-wait-broker.sh" self.add_inject( - hostname=fed.general.hostname, inject={"src": wait_file, "dst": dst} + hostname=fed.general.hostname, + inject={"src": str(wait_file), "dst": dst}, ) for config in configs: @@ -129,7 +130,7 @@ def pre_start(self): "feds": total_fed_count, "endpoint": root_ip, "log-level": broker_md.get("log-level", "summary"), - "log-file": os.path.join(log_dir, "helics-root-broker.log"), + "log-file": str(Path(log_dir) / "helics-root-broker.log"), } # per-host broker configs, initialized with root broker @@ -153,19 +154,22 @@ def pre_start(self): "parent": root_ip, "endpoint": endpoint, "log-level": level, - "log-file": os.path.join(log_dir, "helics-sub-broker.log"), + "log-file": str(Path(log_dir) / "helics-sub-broker.log"), } ) configs[hostname] = broker_configs for hostname, broker_configs in configs.items(): - start_file = f"{self.helics_dir}/{hostname}-broker.sh" + utils.validate_hostname(hostname) + start_file = utils.safe_join(self.helics_dir, f"{hostname}-broker.sh") - with open(start_file, "w") as f: + with start_file.open("w") as f: utils.mako_serve_template( "broker.mako", templates, f, configs=broker_configs ) dst = "/etc/phenix/startup/90-helics-broker.sh" - self.add_inject(hostname=hostname, inject={"src": start_file, "dst": dst}) + self.add_inject( + hostname=hostname, inject={"src": str(start_file), "dst": dst} + ) diff --git a/src/python/phenix_apps/apps/ignition/README.md b/src/python/phenix_apps/apps/ignition/README.md index 0e9f382a..c8432455 100644 --- a/src/python/phenix_apps/apps/ignition/README.md +++ b/src/python/phenix_apps/apps/ignition/README.md @@ -78,7 +78,7 @@ spec: | Option | Default | Description | |-----------------------|------------------|------------------------------------------------------| | `hostname` | (required) | Topology hostname of the outstation. | -| `name` | hostname | Ignition device name; tags reference it (e.g. `[custom-name]AnalogInput0`). | +| `name` | hostname | Ignition device name; tags reference it (e.g. `[custom-name]AnalogInput0`). Letters, digits, spaces, `_` and `-`, starting with a letter or digit, 2 to 63 characters, not `all` or `phenix`. | | `port` | `20000` | Outstation TCP port. | | `source_address` | `1` | DNP3 master address (ot-sim default). | | `destination_address` | `1024` | DNP3 outstation address (ot-sim default). | diff --git a/src/python/phenix_apps/apps/ignition/app.py b/src/python/phenix_apps/apps/ignition/app.py index b3ef5f34..b162feb9 100644 --- a/src/python/phenix_apps/apps/ignition/app.py +++ b/src/python/phenix_apps/apps/ignition/app.py @@ -73,6 +73,19 @@ class RtuDeviceConfig(BaseModel): model_config = {"extra": "ignore"} + @field_validator("name") + @classmethod + def _validate_device_name(cls, v: str | None) -> str | None: + if v is not None and ( + not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9_ -]{1,62}", v) + or v.lower() in utils.RESERVED_HOSTNAMES + ): + raise ValueError( + "device names must be 2 to 63 letters, digits, spaces, '_' and " + "'-', start with a letter or digit, and not be 'all' or 'phenix'" + ) + return v + @property def resolved_name(self) -> str: return self.name or self.hostname @@ -359,7 +372,7 @@ def _write_device_tree( injects = [] for device in devices: - device_dir = Path(host_dir, "devices", device["name"]) + device_dir = utils.safe_join(host_dir, "devices", device["name"]) device_dir.mkdir(parents=True, exist_ok=True) with Path(device_dir, "config.json").open("w") as f: diff --git a/src/python/phenix_apps/apps/ignition/tests/test_ignition.py b/src/python/phenix_apps/apps/ignition/tests/test_ignition.py index aff73be0..c61b0a89 100644 --- a/src/python/phenix_apps/apps/ignition/tests/test_ignition.py +++ b/src/python/phenix_apps/apps/ignition/tests/test_ignition.py @@ -68,6 +68,12 @@ def test_rtu_name_override_wins(): ) +@pytest.mark.parametrize("name", ["x", "all", "Phenix", "../etc"]) +def test_rtu_name_rejects_short_reserved_and_unsafe(name): + with pytest.raises(ValidationError, match="device names"): + RtuDeviceConfig(hostname="rtu-1", name=name) + + def test_host_config_defaults(): cfg = IgnitionHostConfig() assert cfg.gwbk is None diff --git a/src/python/phenix_apps/apps/otsim/app.py b/src/python/phenix_apps/apps/otsim/app.py index 28cf91df..df73ddf1 100644 --- a/src/python/phenix_apps/apps/otsim/app.py +++ b/src/python/phenix_apps/apps/otsim/app.py @@ -1,4 +1,4 @@ -import os +from pathlib import Path import lxml.etree as ET @@ -16,8 +16,8 @@ class OTSim(AppBase): def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) - self.otsim_dir: str = f"{self.exp_dir}/ot-sim" - os.makedirs(self.otsim_dir, exist_ok=True) + self.otsim_dir: Path = Path(self.exp_dir) / "ot-sim" + self.otsim_dir.mkdir(parents=True, exist_ok=True) self.__init_defaults() @@ -249,12 +249,14 @@ def pre_start(self): # if we are not using the helics app add the wait script if not self.extract_app("helics") and ":24000" not in addr: if addr not in broker_addr_wait: - wait_file = f"{self.otsim_dir}/wait-broker-{len(broker_addr_wait)}.sh" # just need a unique name - with open(wait_file, "w") as f: + wait_file = ( + self.otsim_dir / f"wait-broker-{len(broker_addr_wait)}.sh" + ) # just need a unique name + with wait_file.open("w") as f: utils.mako_serve_template( "wait_broker.mako", templates, f, rootbroker_ip=addr ) - broker_addr_wait[addr] = wait_file + broker_addr_wait[addr] = str(wait_file) dst = "/etc/phenix/startup/5-wait-broker.sh" self.add_inject( @@ -274,12 +276,13 @@ def pre_start(self): self.__config_node_red(server, config) - config_file = f"{self.otsim_dir}/{server.hostname}.xml" + utils.validate_hostname(server.hostname) + config_file = utils.safe_join(self.otsim_dir, f"{server.hostname}.xml") config.to_file(config_file) self.add_inject( hostname=server.hostname, - inject={"src": config_file, "dst": "/etc/ot-sim/config.xml"}, + inject={"src": str(config_file), "dst": "/etc/ot-sim/config.xml"}, ) # Front-end processor (FEP), assumed to act as a protocol gateway or @@ -320,12 +323,13 @@ def pre_start(self): self.__config_node_red(fep, config) - config_file = f"{self.otsim_dir}/{fep.hostname}.xml" + utils.validate_hostname(fep.hostname) + config_file = utils.safe_join(self.otsim_dir, f"{fep.hostname}.xml") config.to_file(config_file) self.add_inject( hostname=fep.hostname, - inject={"src": config_file, "dst": "/etc/ot-sim/config.xml"}, + inject={"src": str(config_file), "dst": "/etc/ot-sim/config.xml"}, ) # Field device client, acting as a protocol client via one or more protocol @@ -360,27 +364,29 @@ def pre_start(self): self.__config_node_red(client, config) - config_file = f"{self.otsim_dir}/{client.hostname}.xml" + utils.validate_hostname(client.hostname) + config_file = utils.safe_join(self.otsim_dir, f"{client.hostname}.xml") config.to_file(config_file) self.add_inject( hostname=client.hostname, - inject={"src": config_file, "dst": "/etc/ot-sim/config.xml"}, + inject={"src": str(config_file), "dst": "/etc/ot-sim/config.xml"}, ) # Create and inject config files for any brokers specified in the app # metadata. for hostname, cfg in self.brokers.items(): - start_file = f"{self.otsim_dir}/{hostname}-helics-broker.sh" + utils.validate_hostname(hostname) + start_file = utils.safe_join(self.otsim_dir, f"{hostname}-helics-broker.sh") - with open(start_file, "w") as f: + with start_file.open("w") as f: utils.mako_serve_template("helics_broker.mako", templates, f, cfg=cfg) self.add_inject( hostname=hostname, inject={ - "src": start_file, + "src": str(start_file), "dst": "/etc/phenix/startup/90-helics-broker.sh", }, ) diff --git a/src/python/phenix_apps/apps/protonuke/app.py b/src/python/phenix_apps/apps/protonuke/app.py index 441a277f..4d7a8c1b 100644 --- a/src/python/phenix_apps/apps/protonuke/app.py +++ b/src/python/phenix_apps/apps/protonuke/app.py @@ -1,5 +1,8 @@ +from pathlib import Path + from phenix_apps.apps import AppBase from phenix_apps.common import utils +from phenix_apps.common.error import AppError from phenix_apps.common.logger import logger @@ -7,7 +10,7 @@ class Protonuke(AppBase): def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) - self.startup_dir: str = f"{self.exp_dir}/startup" + self.startup_dir: Path = Path(self.exp_dir) / "startup" def pre_start(self): logger.info(f"Starting user application: {self.name}") @@ -15,32 +18,40 @@ def pre_start(self): nukes = self.extract_all_nodes() for vm in nukes: - path = f"{self.startup_dir}/{vm.hostname}-protonuke" + hostname = utils.validate_hostname(vm.hostname) + + args = str(vm.metadata.args) + if "\n" in args or "\r" in args: + raise AppError( + f"protonuke args for host '{hostname}' must not contain newlines" + ) + + path = utils.safe_join(self.startup_dir, f"{hostname}-protonuke") if vm.topology.hardware.os_type.upper() == "WINDOWS": kwargs = { - "src": path, + "src": str(path), "dst": "/phenix/startup/90-protonuke.ps1", } templates = utils.abs_path(__file__, "templates/") - with open(path, "w") as f: + with path.open("w") as f: utils.mako_serve_template( "protonuke.ps1.mako", templates, f, - protonuke_args=vm.metadata.args, + protonuke_args=args, ) else: kwargs = { - "src": path, + "src": str(path), "dst": "/etc/default/protonuke", } - with open(path, "w") as f: - f.write(f"PROTONUKE_ARGS = {vm.metadata.args}") + with path.open("w") as f: + f.write(f"PROTONUKE_ARGS = {args}") - self.add_inject(hostname=vm.hostname, inject=kwargs) + self.add_inject(hostname=hostname, inject=kwargs) logger.info(f"Started user application: {self.name}") diff --git a/src/python/phenix_apps/apps/scale/app.py b/src/python/phenix_apps/apps/scale/app.py index 99abc342..86dfea47 100644 --- a/src/python/phenix_apps/apps/scale/app.py +++ b/src/python/phenix_apps/apps/scale/app.py @@ -3,6 +3,7 @@ import os import sys from importlib.metadata import entry_points +from pathlib import Path from typing import Any import minimega @@ -23,16 +24,16 @@ def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) # Setup standard directories for the scale app - self.app_dir: str = f"{self.exp_dir}/{self.name}" - os.makedirs(self.app_dir, exist_ok=True) + self.app_dir: Path = Path(self.exp_dir) / self.name + self.app_dir.mkdir(parents=True, exist_ok=True) - self.files_dir: str + self.files_dir: Path if self.dryrun: - self.files_dir = f"/tmp/phenix/images/{self.exp_name}" + self.files_dir = Path("/tmp/phenix/images") / self.exp_name else: - self.files_dir = f"{settings.PHENIX_DIR}/images/{self.exp_name}" + self.files_dir = Path(settings.PHENIX_DIR) / "images" / self.exp_name - os.makedirs(self.files_dir, exist_ok=True) + self.files_dir.mkdir(parents=True, exist_ok=True) # Load all available plugins self._discover_plugins() @@ -150,9 +151,9 @@ def _configure_node_common( self, plugin: ScalePlugin, index: int, hostname: str, profile: dict[str, Any] ) -> None: """Adds standard startup script and injections.""" - startup_config = f"{self.app_dir}/{hostname}-startup.sh" - mm_dir = f"/tmp/miniccc/files/{self.exp_name}" - mm_file = f"{mm_dir}/{hostname}.mm" + startup_config = utils.safe_join(self.app_dir, f"{hostname}-startup.sh") + mm_dir = Path("/tmp/miniccc/files") / self.exp_name + mm_file = mm_dir / f"{hostname}.mm" additional_cmds = plugin.get_additional_startup_commands(index, hostname) @@ -166,13 +167,13 @@ def _configure_node_common( echo 'DONE!' """ - with open(startup_config, "w") as f: + with startup_config.open("w") as f: f.write(startup_script_content) self.add_inject( hostname=hostname, inject={ - "src": startup_config, + "src": str(startup_config), "dst": "/etc/phenix/startup/999-scale.sh", }, ) @@ -264,7 +265,7 @@ def post_start(self) -> None: if not self.dryrun: # minimega.connect prints to stdout on version mismatch, which corrupts JSON output - with open(os.devnull, "w") as devnull: + with Path(os.devnull).open("w") as devnull: old_stdout = sys.stdout sys.stdout = devnull try: @@ -331,7 +332,7 @@ def post_start(self) -> None: ] ) - mm_config = f"{self.files_dir}/{hostname}.mm" + mm_config = utils.safe_join(self.files_dir, f"{hostname}.mm") cfg = { "NAMESPACE": self.name, @@ -356,14 +357,14 @@ def post_start(self) -> None: plugin, "templates_dir", self.templates_dir ) - with open(mm_config, "w") as file_: + with mm_config.open("w") as file_: utils.mako_serve_template( template_name, plugin_templates_dir, file_, config=cfg ) if not self.dryrun: mm.cc_filter(filter=f"name={hostname}") - mm.cc_send(mm_config) + mm.cc_send(str(mm_config)) # Increase all nets' starting IP for next loop if net_info: diff --git a/src/python/phenix_apps/apps/scale/plugins/builtin/README.md b/src/python/phenix_apps/apps/scale/plugins/builtin/README.md index dcb67079..4ed5f101 100644 --- a/src/python/phenix_apps/apps/scale/plugins/builtin/README.md +++ b/src/python/phenix_apps/apps/scale/plugins/builtin/README.md @@ -13,7 +13,7 @@ The plugin is configured via the `profiles` list in the Scale App metadata. | `count` | Integer | `1` | The number of VMs to deploy. Ignored if `containers` > 0. | | `containers` | Integer | `0` | The total number of application containers required. | | `containers_per_node` | Integer | `0` | The number of containers to pack onto a single VM. | -| `hostname_prefix` | String | `"node"` | The prefix used for generating VM hostnames (e.g., `node-1`). | +| `hostname_prefix` | String | `"node"` | The prefix used for generating VM hostnames (e.g., `node-1`). Letters, digits and `-`, starting with a letter or digit, at most 58 characters. | | `node_template` | Dict | `{}` | Overrides for VM hardware (`cpu`, `memory`, `image`, `network`). | | `container_template`| Dict | `{}` | Configuration for containers (`rootfs`, `networks`, `gateway`, `cpu`, `memory`). | diff --git a/src/python/phenix_apps/apps/scale/plugins/builtin/plugin.py b/src/python/phenix_apps/apps/scale/plugins/builtin/plugin.py index 0f922a39..46226b5d 100644 --- a/src/python/phenix_apps/apps/scale/plugins/builtin/plugin.py +++ b/src/python/phenix_apps/apps/scale/plugins/builtin/plugin.py @@ -13,7 +13,11 @@ class BuiltinConfig(BaseModel): count: int = Field(default=1, ge=1) containers: int = Field(default=0, ge=0) containers_per_node: int = Field(default=0, ge=0) - hostname_prefix: str = "node" + # Prefix must be a valid phenix hostname fragment: the "-{index}" suffix is + # appended later, and the result feeds file paths and minimega commands. + hostname_prefix: str = Field( + default="node", pattern=r"^[a-zA-Z0-9][a-zA-Z0-9-]{0,57}$" + ) # Ignore extra fields (like node_template, container_template) that are # handled by the core Scale app, not this plugin. model_config = {"extra": "ignore", "validate_assignment": True} diff --git a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/README.md b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/README.md index 333f7c0a..db0da9e1 100644 --- a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/README.md +++ b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/README.md @@ -35,6 +35,7 @@ graph TD | Field | Type | Default | Description | | :--- | :--- | :--- | :--- | +| `name` | String | `"wind-turbine"` | The prefix used for generating VM hostnames (e.g., `wind-turbine-1`). Letters, digits and `-`, starting with a letter or digit, at most 58 characters. | | `count` | Integer | `1` | The number of **Wind Turbines** to simulate (not VMs). | | `containers_per_node` | Integer | `6` | The number of containers to run per VM. Since one turbine = 6 containers, set this to multiples of 6 (e.g., 18 for 3 turbines/VM). | | `node_template` | Dict | `{}` | VM hardware specifications (CPU, RAM, Network). | diff --git a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/plugin.py b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/plugin.py index bf6ce614..ca309011 100644 --- a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/plugin.py +++ b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/plugin.py @@ -1,10 +1,10 @@ import copy import ipaddress import math -import os import shutil import sys import tarfile +from pathlib import Path from typing import Any import lxml.etree as ET @@ -25,7 +25,11 @@ class WindTurbineConfig(BaseModel): - name: str = "wind-turbine" + # Name must be a valid phenix hostname fragment: the "-{index}" suffix is + # appended later, and the result feeds file paths and minimega commands. + name: str = Field( + default="wind-turbine", pattern=r"^[a-zA-Z0-9][a-zA-Z0-9-]{0,57}$" + ) count: int = Field(default=1, ge=1) containers_per_node: int = 6 node_template: dict[str, Any] = Field(default_factory=dict) @@ -86,7 +90,7 @@ class WindTurbine(ScalePlugin): def __init__(self) -> None: # Set the template directory for this plugin py_path = sys.modules[self.__class__.__module__].__file__ - self.templates_dir: str = utils.abs_path(py_path, "templates") + self.templates_dir: Path = Path(utils.abs_path(py_path, "templates")) self.brokers: dict[str, dict[str, Any]] = {} def _resolve_ext_start_ip(self) -> ipaddress.IPv4Address: @@ -288,8 +292,8 @@ def on_node_configured(self, app: AppBase, index: int, hostname: str) -> None: ips = d["component_ips"] # Prepare directory - cfg_dir = f"{self.app.app_dir}/{hostname}/{cnt_num}" - os.makedirs(cfg_dir, exist_ok=True) + cfg_dir = utils.safe_join(self.app.app_dir, hostname, str(cnt_num)) + cfg_dir.mkdir(parents=True, exist_ok=True) # Generate Config # The otsim.Config class expects the raw app metadata structure. @@ -348,18 +352,18 @@ def on_node_configured(self, app: AppBase, index: int, hostname: str) -> None: self._generate_blade_controller(config, node_meta) # Write config - config.to_file(f"{cfg_dir}/config.xml") + config.to_file(cfg_dir / "config.xml") # Create tarball of configs - tgz_path = f"{self.app.exp_dir}/wind-configs.tgz" + tgz_path = Path(self.app.exp_dir) / "wind-configs.tgz" with tarfile.open(tgz_path, "w:gz") as tar: - tar.add(self.app.app_dir, arcname=os.path.basename(self.app.app_dir)) + tar.add(self.app.app_dir, arcname=Path(self.app.app_dir).name) # Inject tarball self.app.add_inject( hostname=hostname, inject={ - "src": tgz_path, + "src": str(tgz_path), "dst": "/wind-configs.tgz", }, ) @@ -380,7 +384,7 @@ def _generate_main_controller( node: dict[str, Any], ips: dict[str, str], turbine_num: int, - cfg_dir: str, + cfg_dir: Path, ) -> None: tmpl = self.config.templates.get("default", {}).get("main-controller", {}) anemo_tmpl = self.config.templates.get("default", {}).get("anemometer", {}) @@ -484,7 +488,7 @@ def _generate_main_controller( if inject: shutil.copy( inject["src"], - os.path.join(cfg_dir, os.path.basename(inject["dst"])), + cfg_dir / Path(inject["dst"]).name, ) # Logic Module diff --git a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/tests/test_wind_turbine.py b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/tests/test_wind_turbine.py index c9d9659c..146f542f 100644 --- a/src/python/phenix_apps/apps/scale/plugins/wind_turbine/tests/test_wind_turbine.py +++ b/src/python/phenix_apps/apps/scale/plugins/wind_turbine/tests/test_wind_turbine.py @@ -1,3 +1,4 @@ +from pathlib import Path from unittest.mock import MagicMock import pytest @@ -198,9 +199,6 @@ def test_on_node_configured(wind_turbine, mocker): plugin.pre_configure(mock_app, profile) # Mock dependencies - mock_makedirs = mocker.patch( - "phenix_apps.apps.scale.plugins.wind_turbine.plugin.os.makedirs" - ) mock_tarfile = mocker.patch( "phenix_apps.apps.scale.plugins.wind_turbine.plugin.tarfile.open" ) @@ -216,14 +214,16 @@ def test_on_node_configured(wind_turbine, mocker): plugin.on_node_configured(mock_app, 1, "test-wtg-1") # Assertions - # 1. Check directories created (1 for each of 6 containers) - assert mock_makedirs.call_count >= 6 + # 1. Check directories created on disk (1 for each of 6 containers) + node_dir = Path(mock_app.app_dir) / "test-wtg-1" + assert node_dir.is_dir() + assert len([d for d in node_dir.iterdir() if d.is_dir()]) >= 6 # 2. Check config files generated (6 configs) assert mock_config_instance.to_file.call_count == 6 # 3. Check tarball creation - mock_tarfile.assert_called_with(f"{mock_app.exp_dir}/wind-configs.tgz", "w:gz") + mock_tarfile.assert_called_with(Path(mock_app.exp_dir) / "wind-configs.tgz", "w:gz") # 4. Check injection mock_app.add_inject.assert_any_call( diff --git a/src/python/phenix_apps/apps/scale/tests/test_scale.py b/src/python/phenix_apps/apps/scale/tests/test_scale.py index 7814c867..3f417210 100644 --- a/src/python/phenix_apps/apps/scale/tests/test_scale.py +++ b/src/python/phenix_apps/apps/scale/tests/test_scale.py @@ -3,6 +3,7 @@ """ import logging +from pathlib import Path from unittest.mock import MagicMock import pytest @@ -11,6 +12,7 @@ from phenix_apps.apps.scale.app import Scale from phenix_apps.apps.scale.interface import ScalePlugin from phenix_apps.apps.scale.registry import PLUGIN_REGISTRY +from phenix_apps.common import utils as real_utils pytestmark = pytest.mark.app_class(cls=Scale, name="scale") @@ -58,11 +60,15 @@ def test_post_start_logic(mocker, mock_app, mock_minimega, tmp_path): """Test post_start logic with mocked minimega and file operations.""" mocker.patch("phenix_apps.apps.scale.app.logger") mocker.patch("phenix_apps.apps.scale.app.Progress") - mocker.patch("phenix_apps.apps.scale.app.utils") + mock_utils = mocker.patch("phenix_apps.apps.scale.app.utils") + # keep real path semantics so the generated .mm paths stay honest + mock_utils.safe_join.side_effect = real_utils.safe_join + mock_utils.validate_hostname.side_effect = real_utils.validate_hostname mock_mm_conn = mock_minimega app = mock_app + (tmp_path / "images").mkdir(exist_ok=True) app.files_dir = str(tmp_path / "images") app.metadata = {"name": "default", "plugin": "builtin", "count": 2} app.dryrun = False @@ -161,11 +167,10 @@ def test_startup_script_generation(mocker, mock_app): # Case 1: With additional commands mock_plugin.get_additional_startup_commands.return_value = "echo 'custom command'" - mock_file = mocker.patch("builtins.open", mocker.mock_open()) app._configure_node_common(mock_plugin, 1, "node-1", {}) - mock_file.assert_called_with(f"{app.app_dir}/node-1-startup.sh", "w") - handle = mock_file() + startup_file = Path(app.app_dir) / "node-1-startup.sh" + assert startup_file.is_file() expected_content = """echo 'STARTING...' echo 'custom command' @@ -176,15 +181,13 @@ def test_startup_script_generation(mocker, mock_app): mm read /tmp/miniccc/files/test_exp/node-1.mm echo 'DONE!' """ - handle.write.assert_called_with(expected_content) + assert startup_file.read_text() == expected_content # Case 2: Without additional commands (None or empty string) mock_plugin.get_additional_startup_commands.return_value = None - mock_file = mocker.patch("builtins.open", mocker.mock_open()) app._configure_node_common(mock_plugin, 1, "node-2", {}) - handle = mock_file() expected_content_empty = """echo 'STARTING...' while [ ! -S /tmp/minimega/minimega ]; do sleep 1; done @@ -194,15 +197,15 @@ def test_startup_script_generation(mocker, mock_app): mm read /tmp/miniccc/files/test_exp/node-2.mm echo 'DONE!' """ - handle.write.assert_called_with(expected_content_empty) + assert ( + Path(app.app_dir) / "node-2-startup.sh" + ).read_text() == expected_content_empty # Case 3: With multiline additional commands mock_plugin.get_additional_startup_commands.return_value = "cmd1\ncmd2" - mock_file = mocker.patch("builtins.open", mocker.mock_open()) app._configure_node_common(mock_plugin, 1, "node-3", {}) - handle = mock_file() expected_content_multiline = """echo 'STARTING...' cmd1 cmd2 @@ -213,7 +216,9 @@ def test_startup_script_generation(mocker, mock_app): mm read /tmp/miniccc/files/test_exp/node-3.mm echo 'DONE!' """ - handle.write.assert_called_with(expected_content_multiline) + assert ( + Path(app.app_dir) / "node-3-startup.sh" + ).read_text() == expected_content_multiline def test_apply_node_defaults(mocker, mock_app): diff --git a/src/python/phenix_apps/apps/sceptre/app.py b/src/python/phenix_apps/apps/sceptre/app.py index d52baaa0..e3edf334 100644 --- a/src/python/phenix_apps/apps/sceptre/app.py +++ b/src/python/phenix_apps/apps/sceptre/app.py @@ -53,6 +53,16 @@ def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: # Override files that matched an injection, for _warn_unused_overrides. self._used_overrides: set[str] = set() + def _validate_hostnames(self) -> None: + """Reject hostnames the app would build file paths from. + + phenix core validates topology hostnames, but external_node hosts only + exist in the scenario metadata and bypass that check. + """ + + for host in self.extract_all_nodes(): + utils.validate_hostname(host.hostname) + def _log_inventory(self, stage: str) -> None: """Log what the app found in the scenario, by device type.""" @@ -167,7 +177,8 @@ def inject( def host_dir(self, hostname: str) -> Path: """A host's output directory under sceptre/, created. Pre-start only.""" - path = Path(self.sceptre_dir) / hostname + utils.validate_hostname(hostname) + path = utils.safe_join(self.sceptre_dir, hostname) path.mkdir(parents=True, exist_ok=True) return path @@ -181,7 +192,9 @@ def render_sceptre_start(self, device: Box, **kwargs: Any) -> None: ext = ".ps1" if hosts.os_type(device) == "windows" else ".sh" - startup_file = Path(self.startup_dir) / f"{device.hostname}-start{ext}" + startup_file = utils.safe_join( + self.startup_dir, f"{device.hostname}-start{ext}" + ) self.render("sceptre_start.mako", startup_file, **kwargs) utils.mark_executable(startup_file) @@ -191,13 +204,13 @@ def add_sceptre_startup_injects_windows(self, hostname: str) -> None: self.inject( hostname, - f"{self.startup_dir}/sceptre-startup-scheduler.cmd", + self.startup_dir / "sceptre-startup-scheduler.cmd", "ProgramData/Microsoft/Windows/Start Menu/Programs/Startup/sceptre-startup_scheduler.cmd", "sceptre startup scheduler", ) self.inject( hostname, - f"{self.startup_dir}/sceptre-startup.ps1", + self.startup_dir / "sceptre-startup.ps1", "sceptre/sceptre-startup.ps1", "sceptre startup script", ) @@ -210,6 +223,7 @@ def configure(self) -> None: """ validation.enforce(self) + self._validate_hostnames() self._log_inventory("configure") started = self._injection_count() @@ -231,17 +245,18 @@ def pre_start(self) -> None: # Re-checked: phenix may run stages in separate processes. validation.enforce(self) + self._validate_hostnames() self._log_inventory("pre-start") # Rendered unconditionally: add_sceptre_startup_injects_windows() # injects both by path during configure, for any Windows host. - scheduler_file = Path(self.startup_dir) / "sceptre-startup-scheduler.cmd" + scheduler_file = self.startup_dir / "sceptre-startup-scheduler.cmd" self.render("sceptre-startup-scheduler.mako", scheduler_file) - scheduler_file.chmod(0o0777) + scheduler_file.chmod(0o755) - startup_file = Path(self.startup_dir) / "sceptre-startup.ps1" + startup_file = self.startup_dir / "sceptre-startup.ps1" self.render("sceptre-startup.mako", startup_file) - startup_file.chmod(0o777) + startup_file.chmod(0o755) started = self._generated_file_count() diff --git a/src/python/phenix_apps/apps/sceptre/configure.py b/src/python/phenix_apps/apps/sceptre/configure.py index 4c9dd5c4..10ccc896 100644 --- a/src/python/phenix_apps/apps/sceptre/configure.py +++ b/src/python/phenix_apps/apps/sceptre/configure.py @@ -264,7 +264,9 @@ def helics(self) -> None: "helics_broker_logfile", "/etc/sceptre/log/helics_broker.log" ) - startup_file = self.startup_dir / f"{bkr.hostname}-helics.sh" + startup_file = utils.safe_join( + self.startup_dir, f"{bkr.hostname}-helics.sh" + ) startup_file.write_text( f"helics_broker --autorestart --ipv4 -f {len(feds)}" f" --logfile={logfile}" diff --git a/src/python/phenix_apps/apps/scorch/cc/cc.py b/src/python/phenix_apps/apps/scorch/cc/cc.py index 8971dac3..1cf69414 100644 --- a/src/python/phenix_apps/apps/scorch/cc/cc.py +++ b/src/python/phenix_apps/apps/scorch/cc/cc.py @@ -1,10 +1,9 @@ -import os import subprocess import uuid -from pathlib import PurePath +from pathlib import Path, PurePath from phenix_apps.apps.scorch import ComponentBase -from phenix_apps.common import utils +from phenix_apps.common import error, utils from phenix_apps.common.logger import logger @@ -116,18 +115,18 @@ def __run(self, stage: str) -> None: if validator: logger.info(f"validating results from '{cmd.args}'") - tempfile = f"/tmp/{uuid.uuid4()!s}.sh" + tempfile = Path(f"/tmp/{uuid.uuid4()!s}.sh") - with open(tempfile, "w") as tf: + with tempfile.open("w") as tf: tf.write(validator) proc = subprocess.run( - ["bash", tempfile, vm.hostname], + ["bash", str(tempfile), vm.hostname], input=results["stdout"].encode(), capture_output=True, ) - os.remove(tempfile) + tempfile.unlink() if proc.returncode != 0: stderr = proc.stderr.decode() @@ -184,10 +183,10 @@ def __run(self, stage: str) -> None: f"too many files provided for send command for VM {vm.hostname}: {cmd.args}" ) - if not os.path.isabs(src): + if not PurePath(src).is_absolute(): src = "/phenix/" + src - if not os.path.isabs(dst): + if not PurePath(dst).is_absolute(): dst = "/phenix/" + dst logger.info( @@ -211,7 +210,14 @@ def __run(self, stage: str) -> None: dst = str(self.base_dir / PurePath(src).name) elif len(args) == 2: src = args[0] - dst = args[1] + dst_path = Path(args[1]) + if dst_path.is_absolute(): + if not dst_path.is_relative_to(self.base_dir): + raise error.AppError( + f"path '{dst_path}' escapes base directory '{self.base_dir}'" + ) + dst_path = dst_path.relative_to(self.base_dir) + dst = str(utils.safe_join(self.base_dir, dst_path)) else: raise ValueError( f"too many files provided for recv command for VM {vm.hostname}: {cmd.args}" @@ -246,16 +252,16 @@ def __send_cmd_as_file(self, hostname, cmd): else: cmd_file += ".sh" - cmd_src = os.path.join(self.root_dir, self.exp_name, cmd_file) - cmd_dst = os.path.join("/tmp/miniccc/files", self.exp_name, cmd_file) + cmd_src = str(Path(self.root_dir) / self.exp_name / cmd_file) + cmd_dst = str(PurePath("/tmp/miniccc/files") / self.exp_name / cmd_file) - with open(cmd_src, "w") as f: + with Path(cmd_src).open("w") as f: f.write(cmd) utils.mm_cc_send_wait(self.mm, hostname, cmd_src, self.exp_name) self.mm.clear_cc_filter() - os.remove(cmd_src) + Path(cmd_src).unlink() if node.hardware.os_type.lower() == "windows": return f"powershell.exe -ExecutionPolicy Bypass -File {cmd_dst}" diff --git a/src/python/phenix_apps/apps/scorch/providerdata/providerdata.py b/src/python/phenix_apps/apps/scorch/providerdata/providerdata.py index 9ab136c9..63561af9 100644 --- a/src/python/phenix_apps/apps/scorch/providerdata/providerdata.py +++ b/src/python/phenix_apps/apps/scorch/providerdata/providerdata.py @@ -1,12 +1,18 @@ import configparser -import os.path +from pathlib import Path, PurePosixPath from time import sleep from phenix_apps.apps.scorch import ComponentBase -from phenix_apps.common import utils +from phenix_apps.common import error, utils from phenix_apps.common.logger import logger +def _check_fetch_path(path: str) -> str: + if ".." in PurePosixPath(path).parts: + raise error.AppError(f"invalid provider config path '{path}'") + return path + + class ProviderData(ComponentBase): """ SCORCH component for data collection from generalized bennu provider. @@ -30,11 +36,11 @@ def configure(self): # get the ini config self.recv_file(vm=host, src="/etc/sceptre/config.ini") pconf = configparser.ConfigParser() - pconf.read(os.path.join(self.base_dir, "config.ini")) + pconf.read(Path(self.base_dir) / "config.ini") # read the yaml file based on what's in the config - config_path = pconf.get( - section="power-solver-service", option="config-file" + config_path = _check_fetch_path( + pconf.get(section="power-solver-service", option="config-file") ) self.recv_file(vm=host, src=config_path) @@ -133,11 +139,11 @@ def stop(self): # get the ini config self.recv_file(vm=host, src="/etc/sceptre/config.ini") pconf = configparser.ConfigParser() - pconf.read(os.path.join(self.base_dir, "config.ini")) + pconf.read(Path(self.base_dir) / "config.ini") # read the yaml file based on what's in the config - config_path = pconf.get( - section="power-solver-service", option="config-file" + config_path = _check_fetch_path( + pconf.get(section="power-solver-service", option="config-file") ) self.recv_file(vm=host, src=config_path) diff --git a/src/python/phenix_apps/apps/scorch/ssh/ssh.py b/src/python/phenix_apps/apps/scorch/ssh/ssh.py index b5dad774..9514aab8 100644 --- a/src/python/phenix_apps/apps/scorch/ssh/ssh.py +++ b/src/python/phenix_apps/apps/scorch/ssh/ssh.py @@ -5,7 +5,6 @@ """ import json -import os import stat from pathlib import Path, PurePath @@ -223,7 +222,7 @@ def _fetch_remote( if stat.S_ISDIR(rstat.st_mode): self._sftp_get_dir(sftp, remote_path, local_path) else: - os.makedirs(os.path.dirname(local_path) or ".", exist_ok=True) + Path(local_path).parent.mkdir(parents=True, exist_ok=True) sftp.get(remote_path, local_path) except Exception as e: @@ -249,11 +248,18 @@ def _sftp_get_dir( remote_dir (str): Path to the remote directory to download. local_dir (str): Local destination directory path. """ - os.makedirs(local_dir, exist_ok=True) + Path(local_dir).mkdir(parents=True, exist_ok=True) for entry in sftp.listdir_attr(remote_dir): - remote_path = f"{remote_dir.rstrip('/')}/{entry.filename}" - local_path = os.path.join(local_dir, entry.filename) + name = entry.filename + if not name or name in (".", "..") or "/" in name: + logger.error( + f"skipping remote entry with unsafe filename '{name}' in '{remote_dir}'" + ) + continue + + remote_path = f"{remote_dir.rstrip('/')}/{name}" + local_path = str(utils.safe_join(local_dir, name)) if stat.S_ISDIR(entry.st_mode): self._sftp_get_dir(sftp, remote_path, local_path) diff --git a/src/python/phenix_apps/apps/wireguard/app.py b/src/python/phenix_apps/apps/wireguard/app.py index a1c3a841..2b6cccfc 100644 --- a/src/python/phenix_apps/apps/wireguard/app.py +++ b/src/python/phenix_apps/apps/wireguard/app.py @@ -1,13 +1,26 @@ +from pathlib import Path + from phenix_apps.apps import AppBase from phenix_apps.common import utils +from phenix_apps.common.error import AppError from phenix_apps.common.logger import logger +def _validate_single_line(hostname: str, field: str, value) -> None: + """Reject config values that could inject extra lines into wg0.conf.""" + + if "\n" in str(value) or "\r" in str(value): + raise AppError( + f"wireguard metadata field '{field}' for host '{hostname}' " + "must not contain newlines" + ) + + class Wireguard(AppBase): def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) - self.startup_dir: str = f"{self.exp_dir}/startup" + self.startup_dir: Path = Path(self.exp_dir) / "startup" def pre_start(self): logger.info(f"Starting user application: {self.name}") @@ -17,31 +30,45 @@ def pre_start(self): guards = self.extract_all_nodes() for vm in guards: - path = f"{self.startup_dir}/{vm.hostname}-wireguard.conf" + hostname = utils.validate_hostname(vm.hostname) + + for key, value in vm.metadata.get("interface", {}).items(): + _validate_single_line(hostname, f"interface.{key}", value) + + for idx, peer in enumerate(vm.metadata.get("peers", [])): + for key, value in peer.items(): + _validate_single_line(hostname, f"peers[{idx}].{key}", value) + + path = utils.safe_join(self.startup_dir, f"{hostname}-wireguard.conf") kwargs = { - "src": path, + "src": str(path), "dst": "/etc/wireguard/wg0.conf", } - self.add_inject(hostname=vm.hostname, inject=kwargs) + self.add_inject(hostname=hostname, inject=kwargs) - with open(path, "w") as f: + with path.open("w") as f: utils.mako_serve_template( "wireguard_config.mako", templates, f, wireguard=vm.metadata ) + # the config contains the interface's private key + path.chmod(0o600) + if vm.metadata.get("boot", False): - path = f"{self.startup_dir}/{vm.hostname}-wireguard-enable.sh" + path = utils.safe_join( + self.startup_dir, f"{hostname}-wireguard-enable.sh" + ) kwargs = { - "src": path, + "src": str(path), "dst": "/etc/phenix/startup/wireguard-enable.sh", } - self.add_inject(hostname=vm.hostname, inject=kwargs) + self.add_inject(hostname=hostname, inject=kwargs) - with open(path, "w") as f: + with path.open("w") as f: utils.mako_serve_template( "wireguard_enable.mako", templates, f, name="wg0" ) diff --git a/src/python/phenix_apps/common/tests/test_path_helpers.py b/src/python/phenix_apps/common/tests/test_path_helpers.py new file mode 100644 index 00000000..fd2bb515 --- /dev/null +++ b/src/python/phenix_apps/common/tests/test_path_helpers.py @@ -0,0 +1,109 @@ +""" +Characterization tests for the shared path-handling primitives. + +These lock in the trust-boundary behavior the hardening work established: +``safe_join`` is the canonical way to join untrusted path components, +``validate_hostname`` gates hostnames at ingestion, and ``mm_recv`` refuses +non-normalized host destination paths before touching minimega. +""" + +from unittest.mock import MagicMock + +import pytest + +from phenix_apps.common import utils +from phenix_apps.common.error import AppError + + +class TestValidateHostname: + @pytest.mark.parametrize( + "name", + ["node-1", "A9", "web-server-01", "rtu1", "x" * 63], + ) + def test_accepts(self, name): + assert utils.validate_hostname(name) == name + + @pytest.mark.parametrize( + "name", + [ + "", + "a", + "42", + "all", + "Phenix", + "web_server", + "-leading", + "trailing-", + "_leading", + "has space", + "has/slash", + "../traversal", + "semi;colon", + "new\nline", + "x" * 64, + "/etc/cron.d/x", + ], + ) + def test_rejects(self, name): + with pytest.raises(AppError): + utils.validate_hostname(name) + + +class TestSafeJoin: + def test_plain_join_stays_inside(self, tmp_path): + assert utils.safe_join(tmp_path, "a", "b") == tmp_path.resolve() / "a" / "b" + + def test_base_itself_is_allowed(self, tmp_path): + assert utils.safe_join(tmp_path) == tmp_path.resolve() + + def test_internal_dotdot_that_stays_inside_is_allowed(self, tmp_path): + assert utils.safe_join(tmp_path, "a/../b") == tmp_path.resolve() / "b" + + @pytest.mark.parametrize( + "part", + ["..", "../x", "a/../../x", "../../../../etc/cron.d/x"], + ) + def test_traversal_out_of_base_rejected(self, tmp_path, part): + with pytest.raises(AppError): + utils.safe_join(tmp_path, part) + + def test_absolute_part_replacing_base_rejected(self, tmp_path): + # pathlib semantics: an absolute component replaces the base entirely. + # safe_join must catch exactly that footgun. + with pytest.raises(AppError): + utils.safe_join(tmp_path, "/etc/cron.d/x") + + +class TestMmRecvHostDstGuard: + """mm_recv validates its host-side dst before any minimega interaction, + so rejection cases need no minimega and acceptance cases stop at the + first (mocked) minimega call instead of an AppError.""" + + @pytest.mark.parametrize( + "dst", + [ + "relative/path", + "/a/../b", + "/a/..", + "..", + "/..", + "/a/b/", + "/a/b/.", + "/a//b", + "///a", + ], + ) + def test_rejects_non_normalized_dst(self, dst): + with pytest.raises(AppError): + utils.mm_recv(MagicMock(), "vm", "/src", dst) + + @pytest.mark.parametrize("dst", ["/a/b", "//a"]) + def test_normalized_absolute_dst_passes_guard(self, dst, monkeypatch): + # "//a" documents the POSIX exactly-two-leading-slashes quirk the + # guard inherited from os.path.normpath and deliberately preserves. + def stop_after_guard(*args, **kwargs): + raise RuntimeError("stop after guard") + + monkeypatch.setattr(utils, "mm_cc_client_active", stop_after_guard) + with pytest.raises(RuntimeError, match="stop after guard"): + utils.mm_recv(MagicMock(), "vm", "/src", dst) diff --git a/src/python/phenix_apps/common/utils.py b/src/python/phenix_apps/common/utils.py index 4ea170ef..ba1f122b 100644 --- a/src/python/phenix_apps/common/utils.py +++ b/src/python/phenix_apps/common/utils.py @@ -2,8 +2,6 @@ import datetime import json import math -import os -import os.path import random import re import shutil @@ -26,6 +24,7 @@ from elasticsearch import Elasticsearch import phenix_apps.common.settings as phenix_settings +from phenix_apps.common.error import AppError from phenix_apps.common.logger import logger @@ -42,6 +41,44 @@ def kibana_format_time(ts: datetime.datetime) -> str: return ts.strftime("%b %d, %Y @ %H:%M:%S.%f").replace(".000000", ".000") +# minimega's wildcard VM target, and the name phenix's Windows startup +# wrapper and built images use. +RESERVED_HOSTNAMES = frozenset({"all", "phenix"}) + + +def validate_hostname(name: str) -> str: + """ + Validate a hostname against phenix core's node-name rules as of v2026.10.02: + 2 to 63 letters, digits and interior hyphens, not all digits, and not a + reserved name. + """ + if ( + not re.fullmatch(r"[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]", name) + or name.isdigit() + or name.lower() in RESERVED_HOSTNAMES + ): + raise AppError(f"invalid hostname '{name}'") + + return name + + +def safe_join(base: str | Path, *parts: str | Path) -> Path: + """ + Join base with parts and ensure the resolved result stays within the + resolved base directory (rejects '..' traversal and absolute components). + + This is the canonical pathlib containment idiom for this codebase: all + guest-controlled relative parts must be joined through here. + """ + resolved_base = Path(base).resolve() + joined = resolved_base.joinpath(*parts).resolve() + + if joined != resolved_base and not joined.is_relative_to(resolved_base): + raise AppError(f"path '{joined}' escapes base directory '{resolved_base}'") + + return joined + + def mako_render(script_path: str, **kwargs) -> str: """Generate a mako template from a file and render it using provided args. @@ -83,8 +120,8 @@ def mark_executable(file_path: str) -> None: """ Add executable by owner bit to file mode. """ - st_ = os.stat(file_path) - os.chmod(file_path, st_.st_mode | stat.S_IEXEC) + path = Path(file_path) + path.chmod(path.stat().st_mode | stat.S_IEXEC) def generate_mac_addr() -> str: @@ -126,7 +163,7 @@ def validate_mac_addr(macs: list[str]) -> bool: return True -def abs_path(file_: str, relative_path: str | None = None) -> str | Path: +def abs_path(file_: str, relative_path: str | None = None) -> Path: """Return absolute path to file_ with optional relative resource. Args: @@ -134,11 +171,11 @@ def abs_path(file_: str, relative_path: str | None = None) -> str | Path: relative_path (str): Optional relative path of resource. Returns: - str: Full path to file_ (and optional relative resource). + Path: Full path to file_ (and optional relative resource). """ base_path = Path(file_).parent.absolute() - return f"{base_path}/{relative_path}" if relative_path else base_path + return base_path / relative_path if relative_path else base_path def cidr_to_netmask(cidr: int) -> str: @@ -288,7 +325,7 @@ def mm_send( dst: str, grace: float = phenix_settings.CC_CLIENT_GRACE, ) -> None: - if not os.path.exists(src): + if not Path(src).exists(): raise ValueError(f"{src} not found locally") # Use PHENIX_DIR as base directory to ensure minimega has access to it. This @@ -306,17 +343,17 @@ def mm_send( mm_cc_client_active(mm, vm, grace=grace) with tempfile.TemporaryDirectory(dir=base) as tmp: - vm_dst = os.path.join(tmp, dst.strip("/")) - dst_dir = os.path.dirname(vm_dst) + vm_dst = safe_join(tmp, dst.strip("/")) + dst_dir = vm_dst.parent try: mm.cc_mount(vm, tmp) time.sleep(1.0) - if not os.path.exists(dst_dir): - os.makedirs(dst_dir, exist_ok=True) + if not dst_dir.exists(): + dst_dir.mkdir(parents=True, exist_ok=True) - if os.path.isdir(src): + if Path(src).is_dir(): shutil.copytree(src, vm_dst, dirs_exist_ok=True) else: shutil.copyfile(src, vm_dst) @@ -350,24 +387,32 @@ def mm_recv( if Path("/tmp/miniccc-mounts").is_dir(): base = "/tmp/miniccc-mounts" + # PurePath construction collapses spurious slashes and single-dot segments + # (but not '..'), so str(Path(dst)) == dst is equivalent to the old + # os.path.normpath(dst) == dst normalization check. + if not Path(dst).is_absolute() or str(Path(dst)) != dst or ".." in Path(dst).parts: + raise AppError( + f"invalid host destination path '{dst}' (must be absolute and normalized, with no '..' segments)" + ) + mm_cc_client_active(mm, vm, grace=grace) with tempfile.TemporaryDirectory(dir=base) as tmp: if isinstance(src, str): src = [src] - vm_sources = [os.path.join(tmp, s.strip("/")) for s in src] - dst_dir = os.path.dirname(dst) + vm_sources = [safe_join(tmp, s.strip("/")) for s in src] + dst_dir = Path(dst).parent - if not os.path.exists(dst_dir): - os.makedirs(dst_dir, exist_ok=True) + if not dst_dir.exists(): + dst_dir.mkdir(parents=True, exist_ok=True) try: mm.cc_mount(vm, tmp) for vm_src in vm_sources: tries = 0 - while not os.path.exists(vm_src): + while not vm_src.exists(): tries += 1 if tries >= 5: @@ -375,7 +420,7 @@ def mm_recv( raise ValueError(f"{src} not found in VM {vm}") time.sleep(0.5) - if os.path.isdir(vm_src): + if vm_src.is_dir(): shutil.copytree(vm_src, dst, dirs_exist_ok=True) else: # shutil.copyfile(vm_src, dst) @@ -523,7 +568,7 @@ def mm_cc_send_wait( client=uuid, host=host, match_column="sent", - match_value=f"[{exp_name}/{os.path.basename(src)}]", + match_value=f"[{exp_name}/{Path(src).name}]", )