From 45b85edb9773953109f05f05b0cd295dbca22121 Mon Sep 17 00:00:00 2001 From: Ryan Adams Date: Thu, 1 Oct 2026 19:47:35 -0600 Subject: [PATCH] fix: stop scenario metadata from reaching shells and host commands Several apps and SCORCH components splice scenario metadata into shell strings, minimega and ovs-vsctl commands, or run scenario-supplied scripts directly on the phenix host. - utils.run_command() executes an argument vector instead of a shell string; a str is shlex-split. collector, pcap, mm, tcpdump, trim_pcap() and pcap_capinfos() pass argument lists, and tshark output is redirected from Python rather than through 'bash -c ... >'. - Values that end up in ovs-vsctl or minimega commands are validated as single tokens: the mirror app's bridge, VLAN alias and HIL interfaces (Go), the mgmt_tap bridge, erspan bridges, interfaces, IPs, session key and excluded VLANs, tcpdump interfaces, mm capture filenames and filters, and the experiment and compute names in mm_compute_cmd(). - Validators for the art and cc components and the pipe 'via' program run on the phenix host, not in a VM, so they now need an explicit opt-in: PHENIX_SCORCH_HOST_VALIDATORS=1, read once in common/settings.py. Without it validators are skipped with a warning and a via fails the component. This is a breaking default. - The kafka PID file leaves world-writable /tmp for the experiment's SCORCH files directory (still independent of loop and count, so configure keeps detecting a running listener), and cleanup only kills the PID if it is still a kafka listener. - The ssh component takes an optional known_hosts file and rejects unknown host keys when one is given; without it, it warns before trusting the key. Documented in the affected READMEs, AGENTS.md and the changelog; Go and Python tests cover the new validation. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo --- AGENTS.md | 1 + CHANGELOG.md | 4 + src/go/cmd/phenix-app-mirror/main.go | 24 +-- src/go/cmd/phenix-app-mirror/util.go | 58 +++++++- src/go/cmd/phenix-app-mirror/util_test.go | 51 +++++++ src/python/phenix_apps/apps/mgmt_tap/app.py | 14 +- .../phenix_apps/apps/scorch/art/README.md | 5 + src/python/phenix_apps/apps/scorch/art/art.py | 36 +++-- .../phenix_apps/apps/scorch/cc/README.md | 5 + src/python/phenix_apps/apps/scorch/cc/cc.py | 26 ++-- .../apps/scorch/collector/collector.py | 12 +- .../phenix_apps/apps/scorch/erspan/erspan.py | 43 ++++-- .../apps/scorch/kafka/kafka_component.py | 52 +++++-- .../phenix_apps/apps/scorch/mm/README.md | 4 +- src/python/phenix_apps/apps/scorch/mm/mm.py | 139 ++++++++++++------ .../phenix_apps/apps/scorch/pcap/pcap.py | 50 +++++-- .../phenix_apps/apps/scorch/pipe/README.md | 5 + .../phenix_apps/apps/scorch/pipe/pipe.py | 15 +- .../phenix_apps/apps/scorch/ssh/README.md | 1 + src/python/phenix_apps/apps/scorch/ssh/ssh.py | 12 +- .../apps/scorch/tcpdump/tcpdump.py | 31 +++- src/python/phenix_apps/common/settings.py | 33 +++-- .../common/tests/test_command_helpers.py | 103 +++++++++++++ src/python/phenix_apps/common/utils.py | 86 ++++++++++- src/python/phenix_apps/testing/plugin.py | 4 +- 25 files changed, 646 insertions(+), 168 deletions(-) create mode 100644 src/go/cmd/phenix-app-mirror/util_test.go create mode 100644 src/python/phenix_apps/common/tests/test_command_helpers.py diff --git a/AGENTS.md b/AGENTS.md index 26a4ce7e..8f2f8972 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,6 +130,7 @@ Most environment variables are read in | `PHENIX_DIR`, `PHENIX_TEMP_DIR` | phēnix data and scratch directories | | `MM_FILEPATH`, `MM_SOCKET_PATH` | minimega file root and command socket | | `PHENIX_CC_*` | miniccc polling rates and timeout grace periods | +| `PHENIX_SCORCH_HOST_VALIDATORS` | Literal `0` stops the SCORCH `art` and `cc` validators and the `pipe` `via` from running scenario-supplied commands on the host; on otherwise | Read elsewhere: `PHENIX_DRYRUN` (literal `true`) in `src/python/phenix_apps/apps/__init__.py`, diff --git a/CHANGELOG.md b/CHANGELOG.md index 245881f9..b0a89526 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **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. +- **Common**: `run_command()` no longer uses a shell; `mm_compute_cmd()` rejects multi-token names. +- **Mirror, mgmt_tap, SCORCH**: Metadata is validated before reaching `ovs-vsctl`, minimega or `tshark`; commands run as argument vectors, not `bash -c`. +- **SCORCH**: kafka's PID file moves from `/tmp` to the experiment's `files_dir/scorch/`; ssh takes optional `known_hosts`. +- **SCORCH art/cc/pipe**: `PHENIX_SCORCH_HOST_VALIDATORS=0` stops validators and `via` from running scenario-supplied commands on the host; validator scripts are written with `tempfile`. ## [2.0.0] - 2026-03-04 diff --git a/src/go/cmd/phenix-app-mirror/main.go b/src/go/cmd/phenix-app-mirror/main.go index 93b05166..2d76e6d6 100644 --- a/src/go/cmd/phenix-app-mirror/main.go +++ b/src/go/cmd/phenix-app-mirror/main.go @@ -151,17 +151,11 @@ func runStage(log *slog.Logger, stage string, exp *types.Experiment, dryrun bool func configure(log *slog.Logger, exp *types.Experiment) error { app := util.ExtractApp(exp.Spec.Scenario(), "mirror") - amd, err := extractMetadata(app.Metadata()) + amd, err := extractMetadata(app.Metadata(), exp.Spec.DefaultBridge()) if err != nil { return fmt.Errorf("extracting app metadata: %w", err) } - amd.Init() - - if amd.MirrorBridge == "" { - amd.MirrorBridge = exp.Spec.DefaultBridge() - } - nw, err := mirrorNet(log, &amd) if err != nil { return fmt.Errorf("determining mirror network: %w", err) @@ -299,17 +293,11 @@ func postStart(log *slog.Logger, exp *types.Experiment, dryrun bool) error { app := util.ExtractApp(exp.Spec.Scenario(), "mirror") var amd MirrorAppMetadataV1 - amd, err = extractMetadata(app.Metadata()) + amd, err = extractMetadata(app.Metadata(), exp.Spec.DefaultBridge()) if err != nil { return fmt.Errorf("extracting app metadata: %w", err) } - amd.Init() - - if amd.MirrorBridge == "" { - amd.MirrorBridge = exp.Spec.DefaultBridge() - } - var nw netaddr.IPPrefix nw, err = mirrorNet(log, &amd) if err != nil { @@ -735,17 +723,11 @@ func cleanup(log *slog.Logger, exp *types.Experiment, dryrun bool) error { app := util.ExtractApp(exp.Spec.Scenario(), "mirror") - amd, err := extractMetadata(app.Metadata()) + _, err := extractMetadata(app.Metadata(), exp.Spec.DefaultBridge()) if err != nil { return fmt.Errorf("extracting app metadata: %w", err) } - amd.Init() - - if amd.MirrorBridge == "" { - amd.MirrorBridge = exp.Spec.DefaultBridge() - } - cluster := cluster(exp) var status MirrorAppStatus diff --git a/src/go/cmd/phenix-app-mirror/util.go b/src/go/cmd/phenix-app-mirror/util.go index 1c9cca5f..f33efbdd 100644 --- a/src/go/cmd/phenix-app-mirror/util.go +++ b/src/go/cmd/phenix-app-mirror/util.go @@ -2,13 +2,46 @@ package main import ( "fmt" + "regexp" + "strings" "github.com/mitchellh/mapstructure" "phenix-apps/util" ) -func extractMetadata(data map[string]any) (MirrorAppMetadataV1, error) { +// ovsTokenRegex matches a single safe token for interpolation into ovs-vsctl +// commands run on cluster hosts. Bridge and interface names are capped at 15 +// characters (IFNAMSIZ); VLAN aliases are phenix names with no such cap. +var ( + ovsTokenRegex = regexp.MustCompile(`^[A-Za-z0-9_.-]{1,15}$`) + vlanAliasRegex = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) +) + +// validateOVSToken ensures a metadata-provided value is a single, safe token +// before it gets interpolated into an ovs-vsctl command. It explicitly rejects +// whitespace and the `--` command separator on top of the allowlist regex. +func validateOVSToken(field, value string) error { + return validateToken(field, value, ovsTokenRegex) +} + +func validateToken(field, value string, re *regexp.Regexp) error { + if value == "--" || strings.ContainsAny(value, " \t\r\n") || !re.MatchString(value) { + return fmt.Errorf( + "invalid %s %q in mirror app metadata: must match %s and must not be `--`", + field, + value, + re, + ) + } + + return nil +} + +// extractMetadata decodes the app metadata, applies defaults (defaultBridge is +// the experiment's default bridge) and validates the result, so a defaulted +// value is checked like one from the scenario. +func extractMetadata(data map[string]any, defaultBridge string) (MirrorAppMetadataV1, error) { var ( amd MirrorAppMetadataV1 version = util.ExtractVersion(data) @@ -31,6 +64,22 @@ func extractMetadata(data map[string]any) (MirrorAppMetadataV1, error) { } } + amd.Init() + + if amd.MirrorBridge == "" { + amd.MirrorBridge = defaultBridge + } + + err := validateOVSToken("mirrorBridge", amd.MirrorBridge) + if err != nil { + return amd, err + } + + err = validateToken("mirrorVLAN", amd.MirrorVLAN, vlanAliasRegex) + if err != nil { + return amd, err + } + return amd, nil } @@ -49,5 +98,12 @@ func extractHostMetadata(data map[string]any) (MirrorHostMetadata, error) { } } + for _, hil := range hmd.HIL { + err := validateOVSToken("hilInterfaces entry", hil) + if err != nil { + return hmd, err + } + } + return hmd, nil } diff --git a/src/go/cmd/phenix-app-mirror/util_test.go b/src/go/cmd/phenix-app-mirror/util_test.go new file mode 100644 index 00000000..296063fd --- /dev/null +++ b/src/go/cmd/phenix-app-mirror/util_test.go @@ -0,0 +1,51 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateOVSToken(t *testing.T) { + t.Parallel() + + for _, value := range []string{"phenix", "br-0", "eth1.100", "mirror", "a_b", "abcdefghijklmno"} { + require.NoError(t, validateOVSToken("field", value), value) + } + + for _, value := range []string{"", "--", "two words", "tab\there", "new\nline", "br0;reboot", "abcdefghijklmnop", "$(id)"} { + require.Error(t, validateOVSToken("field", value), value) + } +} + +func TestExtractMetadataRejectsUnsafeTokens(t *testing.T) { + t.Parallel() + + _, err := extractMetadata(map[string]any{"version": "v1", "mirrorBridge": "phenix -- del-br phenix"}, "phenix") + require.Error(t, err) + + _, err = extractMetadata(map[string]any{"version": "v1", "mirrorVLAN": "mirror;id"}, "phenix") + require.Error(t, err) + + amd, err := extractMetadata(map[string]any{"version": "v1", "mirrorBridge": "phenix", "mirrorVLAN": "mirror"}, "") + require.NoError(t, err) + assert.Equal(t, "phenix", amd.MirrorBridge) + + // VLAN aliases are phenix names, not OVS interface names: no 15-character cap. + amd, err = extractMetadata(map[string]any{"version": "v1", "mirrorVLAN": "mirror_capture_net"}, "phenix") + require.NoError(t, err) + assert.Equal(t, "mirror_capture_net", amd.MirrorVLAN) + + // Defaults are applied before validation, so a bad default bridge is caught. + amd, err = extractMetadata(map[string]any{"version": "v1"}, "phenix") + require.NoError(t, err) + assert.Equal(t, "phenix", amd.MirrorBridge) + assert.Equal(t, "mirror", amd.MirrorVLAN) + + _, err = extractMetadata(map[string]any{"version": "v1"}, "br0 -- del-br br0") + require.Error(t, err) + + _, err = extractHostMetadata(map[string]any{"hilInterfaces": []any{"eth1", "eth2 eth3"}}) + require.Error(t, err) +} diff --git a/src/python/phenix_apps/apps/mgmt_tap/app.py b/src/python/phenix_apps/apps/mgmt_tap/app.py index 809a0f96..60a9f102 100644 --- a/src/python/phenix_apps/apps/mgmt_tap/app.py +++ b/src/python/phenix_apps/apps/mgmt_tap/app.py @@ -1,11 +1,17 @@ import ipaddress import socket +from typing import Final import minimega from phenix_apps.apps import AppBase from phenix_apps.common.logger import logger -from phenix_apps.common.utils import _mm_init, mm_compute_cmd, mm_host_info +from phenix_apps.common.utils import ( + _mm_init, + mm_compute_cmd, + mm_host_info, + validate_ovs_name, +) class MgmtTap(AppBase): @@ -24,7 +30,7 @@ class MgmtTap(AppBase): bridge: phenix """ - DEFAULT_BRIDGE = "phenix" + DEFAULT_BRIDGE: Final[str] = "phenix" def __init__(self, name: str, stage: str, dryrun: bool = False) -> None: super().__init__(name, stage, dryrun) @@ -57,12 +63,12 @@ def _get_bridge(self) -> str: bridge = self.metadata.get("bridge", None) if self.metadata else None if bridge: logger.debug(f"Using bridge '{bridge}' from app metadata") - return bridge + return validate_ovs_name(bridge, "bridge") bridge = self.experiment.spec.get("defaultBridge", None) if bridge: logger.debug(f"Using experiment default bridge '{bridge}'") - return bridge + return validate_ovs_name(bridge, "bridge") logger.debug(f"Falling back to bridge '{self.DEFAULT_BRIDGE}'") return self.DEFAULT_BRIDGE diff --git a/src/python/phenix_apps/apps/scorch/art/README.md b/src/python/phenix_apps/apps/scorch/art/README.md index f93bc8ae..9473d3a2 100644 --- a/src/python/phenix_apps/apps/scorch/art/README.md +++ b/src/python/phenix_apps/apps/scorch/art/README.md @@ -83,6 +83,11 @@ If exit non-zero and `abortOnError` is true, then the component exits as failed. If no validator is provided then it is assumed the test succeeded if the atomic executor exits cleanly. +> [!IMPORTANT] +> Validators run as shell scripts on the phenix host, not in the VM. To lock +> this down, set `PHENIX_SCORCH_HOST_VALIDATORS=0` in the phenix environment; +> each validator is then skipped with a warning and the result is not validated. + `vms` is a list of settings per VM to execute the test on. ## VM Settings diff --git a/src/python/phenix_apps/apps/scorch/art/art.py b/src/python/phenix_apps/apps/scorch/art/art.py index 6c824841..7db43de5 100644 --- a/src/python/phenix_apps/apps/scorch/art/art.py +++ b/src/python/phenix_apps/apps/scorch/art/art.py @@ -1,17 +1,19 @@ -import os import subprocess +import tempfile import time import uuid +from typing import Final from box import Box from phenix_apps.apps.scorch import ComponentBase from phenix_apps.common import utils from phenix_apps.common.logger import logger +from phenix_apps.common.settings import SCORCH_HOST_VALIDATORS # This can be changed here to reflect your directory structure on hosts as a default. # This is overwritten if a value is provided via goartPath -GOART_BASE = "/phenix/art" +GOART_BASE: Final[str] = "/phenix/art" class AtomicRedTeam(ComponentBase): @@ -102,34 +104,38 @@ def start(self): utils.mm_exec_wait(mm, hostname, cmd) time.sleep(5) logger.info(f"retrieving results: {out_file}") - results_file = os.path.join(self.base_dir, f"{hostname}.json") + results_file = self.base_dir / f"{hostname}.json" try: utils.mm_recv(mm, hostname, out_file, results_file) + if not results_file.is_file(): + raise FileNotFoundError(f"{results_file} not found after transfer") logger.info(f"results_file path: {results_file}") - logger.info(f"results_file exists: {os.path.exists(results_file)}") except Exception as ex: raise RuntimeError( f"failed to get results file from {hostname}: {ex}" ) from ex validator = self.metadata.get("validator", None) - if validator: + if validator and not SCORCH_HOST_VALIDATORS: + logger.warning( + f"skipping host-side validator for {hostname}: " + "host-side commands are disabled by PHENIX_SCORCH_HOST_VALIDATORS=0" + ) + elif validator: logger.info(f"validating results from {hostname}") - tempfile = f"/tmp/{uuid.uuid4()!s}.sh" - with open(tempfile, "w") as tf: - tf.write(validator) - results = Box.from_json(filename=results_file) - proc = subprocess.run( - ["sh", tempfile, hostname], - input=results.Executor.ExecutedCommand.results.encode(), - capture_output=True, - ) + with tempfile.NamedTemporaryFile("w", suffix=".sh") as tf: + tf.write(validator) + tf.flush() - os.remove(tempfile) + proc = subprocess.run( + ["sh", tf.name, hostname], + input=results.Executor.ExecutedCommand.results.encode(), + capture_output=True, + ) if proc.returncode != 0: stderr = proc.stderr.decode() diff --git a/src/python/phenix_apps/apps/scorch/cc/README.md b/src/python/phenix_apps/apps/scorch/cc/README.md index a1db3c68..2f01ee09 100644 --- a/src/python/phenix_apps/apps/scorch/cc/README.md +++ b/src/python/phenix_apps/apps/scorch/cc/README.md @@ -38,6 +38,11 @@ metadata: > validator script should be written to process STDIN. Anything the validator > script writes to STDERR will be available to the user if the validation fails. +> [!IMPORTANT] +> Validators run as shell scripts on the phenix host, not in the VM. To lock +> this down, set `PHENIX_SCORCH_HOST_VALIDATORS=0` in the phenix environment; +> each validator is then skipped with a warning and the result is not validated. + ## Types - VM-specific command types - `exec`: execute a command (`cc exec`) diff --git a/src/python/phenix_apps/apps/scorch/cc/cc.py b/src/python/phenix_apps/apps/scorch/cc/cc.py index 1cf69414..3ee1f708 100644 --- a/src/python/phenix_apps/apps/scorch/cc/cc.py +++ b/src/python/phenix_apps/apps/scorch/cc/cc.py @@ -1,10 +1,12 @@ import subprocess +import tempfile import uuid from pathlib import Path, PurePath from phenix_apps.apps.scorch import ComponentBase from phenix_apps.common import error, utils from phenix_apps.common.logger import logger +from phenix_apps.common.settings import SCORCH_HOST_VALIDATORS class CC(ComponentBase): @@ -112,21 +114,23 @@ def __run(self, stage: str) -> None: if results["stdout"]: logger.info(f"STDOUT Output: {results['stdout']}") - if validator: + if validator and not SCORCH_HOST_VALIDATORS: + logger.warning( + f"skipping host-side validator for command '{cmd.args}' on VM {vm.hostname}: " + "host-side commands are disabled by PHENIX_SCORCH_HOST_VALIDATORS=0" + ) + elif validator: logger.info(f"validating results from '{cmd.args}'") - tempfile = Path(f"/tmp/{uuid.uuid4()!s}.sh") - - with tempfile.open("w") as tf: + with tempfile.NamedTemporaryFile("w", suffix=".sh") as tf: tf.write(validator) + tf.flush() - proc = subprocess.run( - ["bash", str(tempfile), vm.hostname], - input=results["stdout"].encode(), - capture_output=True, - ) - - tempfile.unlink() + proc = subprocess.run( + ["bash", tf.name, vm.hostname], + input=results["stdout"].encode(), + capture_output=True, + ) if proc.returncode != 0: stderr = proc.stderr.decode() diff --git a/src/python/phenix_apps/apps/scorch/collector/collector.py b/src/python/phenix_apps/apps/scorch/collector/collector.py index 64087306..3d65b820 100644 --- a/src/python/phenix_apps/apps/scorch/collector/collector.py +++ b/src/python/phenix_apps/apps/scorch/collector/collector.py @@ -63,9 +63,15 @@ def stop(self): sceptre_topo = self.experiment.metadata.annotations.topology sceptre_scenario = self.experiment.metadata.annotations.scenario - topo_data = utils.run_command(f"phenix config get topology/{sceptre_topo}") - sc_data = utils.run_command(f"phenix config get scenario/{sceptre_scenario}") - exp_data = utils.run_command(f"phenix config get experiment/{self.exp_name}") + topo_data = utils.run_command( + ["phenix", "config", "get", f"topology/{sceptre_topo}"] + ) + sc_data = utils.run_command( + ["phenix", "config", "get", f"scenario/{sceptre_scenario}"] + ) + exp_data = utils.run_command( + ["phenix", "config", "get", f"experiment/{self.exp_name}"] + ) assert topo_data assert sc_data assert exp_data diff --git a/src/python/phenix_apps/apps/scorch/erspan/erspan.py b/src/python/phenix_apps/apps/scorch/erspan/erspan.py index 78469710..b12fbedf 100644 --- a/src/python/phenix_apps/apps/scorch/erspan/erspan.py +++ b/src/python/phenix_apps/apps/scorch/erspan/erspan.py @@ -11,7 +11,7 @@ import paramiko 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 @@ -225,24 +225,32 @@ def _get_args(self): and password are optional. """ # --- Top-level required --- - self.local_bridge = self.metadata.get("local_bridge") - if self.local_bridge is None: - raise ValueError("No 'local_bridge' provided in metadata") + self.local_bridge = utils.validate_ovs_name( + self.metadata.get("local_bridge"), "local_bridge" + ) + self.local_ip = utils.validate_ip(self.metadata.get("local_ip"), "local_ip") + self.remote_ip = utils.validate_ip(self.metadata.get("remote_ip"), "remote_ip") - self.local_ip = self.metadata.get("local_ip") - if self.local_ip is None: - raise ValueError("No 'local_ip' provided in metadata") + # --- Top-level optional with defaults --- + self.local_interface = utils.validate_ovs_name( + self.metadata.get("local_interface", "erspan1"), "local_interface" + ) - self.remote_ip = self.metadata.get("remote_ip") - if self.remote_ip is None: - raise ValueError("No 'remote_ip' provided in metadata") + try: + self.session_key = int(self.metadata.get("session_key", 100)) + except (TypeError, ValueError) as ex: + raise error.AppError( + f"invalid 'session_key' value in metadata: {ex}" + ) from ex - # --- Top-level optional with defaults --- - self.local_interface = self.metadata.get("local_interface", "erspan1") - self.session_key = self.metadata.get("session_key", 100) self.excluded_vlans = self.metadata.get("excluded_vlans", []) if isinstance(self.excluded_vlans, str): self.excluded_vlans = [self.excluded_vlans] + for vlan in self.excluded_vlans: + if not isinstance(vlan, str) or not vlan or len(vlan.split()) != 1: + raise error.AppError( + f"invalid 'excluded_vlans' value '{vlan}' in metadata" + ) # --- Optional remote_config section --- self.remote_config = self.metadata.get("remote_config") @@ -259,6 +267,15 @@ def _get_args(self): if not rc.get("remote_interface"): rc.remote_interface = "erspan1" + rc.remote_interface = utils.validate_ovs_name( + rc.remote_interface, "remote_interface" + ) + + if rc.get("remote_bridge") is not None: + rc.remote_bridge = utils.validate_ovs_name( + rc.remote_bridge, "remote_bridge" + ) + def _get_mirrored_vlans(self) -> list[int]: """ Return a sorted list of unique VLAN IDs (integers) for all VLANs that diff --git a/src/python/phenix_apps/apps/scorch/kafka/kafka_component.py b/src/python/phenix_apps/apps/scorch/kafka/kafka_component.py index 876ce007..99bb5b1f 100644 --- a/src/python/phenix_apps/apps/scorch/kafka/kafka_component.py +++ b/src/python/phenix_apps/apps/scorch/kafka/kafka_component.py @@ -19,16 +19,28 @@ def __init__(self): uuid.NAMESPACE_DNS, f"{self.exp_name}-{self.name}-{config_str}" ) + # Not under base_dir: that is per loop and count, and configure must see + # the PID file a previous loop wrote. Not /tmp, which is world-writable. self.pid_file = ( - f"/tmp/phenix-scorch-kafka-{self.exp_name}-{component_uuid.hex}.pid" + Path(self.files_dir) + / "scorch" + / f"phenix-scorch-kafka-{self.exp_name}-{component_uuid.hex}.pid" ) self.execute_stage() def configure(self): - # if the deterministic PID file already exists, don't configure the component again - if os.path.exists(self.pid_file): - logger.info(f"User component {self.name} already configured, skipping") - return + # if the deterministic PID file names a running listener, don't configure + # the component again; a stale file (crash, reboot) is discarded + if self.pid_file.exists(): + try: + pid = int(self.pid_file.read_text().strip()) + except (OSError, ValueError): + pid = 0 + if pid and self._is_listener(pid): + logger.info(f"User component {self.name} already configured, skipping") + return + logger.warning(f"Removing stale PID file {self.pid_file}") + self.pid_file.unlink(missing_ok=True) logger.info(f"Configuring user component: {self.name}") @@ -49,13 +61,13 @@ def configure(self): csv_bool = self.metadata.get("csv", True) # if false output a JSON # get and output the output directory to the logger - output_dir = self.base_dir + output_dir: Path = self.base_dir logger.info(f"Output Directory: {output_dir}") - os.makedirs(output_dir, exist_ok=True) + output_dir.mkdir(parents=True, exist_ok=True) if csv_bool: - self.path = os.path.join(output_dir, f"{self.name}_output.csv") + self.path = output_dir / f"{self.name}_output.csv" else: - self.path = os.path.join(output_dir, f"{self.name}_output.ndjson") + self.path = output_dir / f"{self.name}_output.ndjson" kafka_ips_str = ",".join(kafka_ips) topics_str = json.dumps(topics) @@ -89,9 +101,9 @@ def configure(self): logger.error(f"Error running listener executable. See: {e}") def _create_pid_file(self, pid): - # writes PID to unique .pid file under /tmp directory + # writes PID to the unique .pid file try: - with open(self.pid_file, "w+") as f: + with self.pid_file.open("w+") as f: f.write(str(pid)) except Exception as e: logger.error( @@ -104,9 +116,9 @@ def _consume_pid_file(self): # reads and deletes PID file, returns PID pid = 0 try: - with open(self.pid_file) as f: + with self.pid_file.open() as f: pid = int(f.readline().rstrip()) - os.remove(self.pid_file) + self.pid_file.unlink() return pid except Exception as e: logger.error( @@ -122,12 +134,26 @@ def cleanup(self): logger.info("No PID, component already cleaned up") exit() + if not self._is_listener(pid): + logger.error( + f"PID {pid} does not appear to be a kafka listener process, not killing" + ) + return + try: os.kill(pid, 9) logger.info(f"Cleaned up user component: {self.name}") except Exception as e: logger.error(f"Error terminating listener at PID {pid}. See: {e}") + @staticmethod + def _is_listener(pid: int) -> bool: + try: + cmdline = Path(f"/proc/{pid}/cmdline").read_bytes() + except OSError: + return False + return b"kafka_listener.py" in cmdline + def main(): Kafka() diff --git a/src/python/phenix_apps/apps/scorch/mm/README.md b/src/python/phenix_apps/apps/scorch/mm/README.md index 5f3b020b..af91bd1d 100644 --- a/src/python/phenix_apps/apps/scorch/mm/README.md +++ b/src/python/phenix_apps/apps/scorch/mm/README.md @@ -14,7 +14,7 @@ metadata: - type: start_capture capture: bridge: phenix - filename: capture.pcap # will be auto generated as -.pcap if not provided + filename: capture.pcap # will be auto generated as -.pcap if not provided; letters, digits and . _ : + - only filter: port 80 # typical BPF filter expression snaplen: null - type: stop_capture @@ -42,7 +42,7 @@ metadata: - type: start_capture capture: interface: 0 - filename: capture.pcap # will be auto generated as --.pcap if not provided + filename: capture.pcap # will be auto generated as --.pcap if not provided; letters, digits and . _ : + - only filter: port 80 # typical BPF filter expression snaplen: null - type: stop_capture # will stop captures on all interfaces (minimega limitation) diff --git a/src/python/phenix_apps/apps/scorch/mm/mm.py b/src/python/phenix_apps/apps/scorch/mm/mm.py index d30f0f9c..8aea03fb 100644 --- a/src/python/phenix_apps/apps/scorch/mm/mm.py +++ b/src/python/phenix_apps/apps/scorch/mm/mm.py @@ -1,12 +1,45 @@ -import os +import re import subprocess import time from datetime import datetime +from pathlib import Path, PurePath +from typing import Final 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 +FILENAME_REGEX: Final[re.Pattern[str]] = re.compile(r"^[A-Za-z0-9._:+-]+$") + + +def _validate_filename(filename: str) -> str: + # NOTE: os.path.basename() returned '' for names ending in '/'; PurePath.name + # would return the last component instead, so reject trailing '/' explicitly + # to keep the validation semantics identical. + filename = "" if filename.endswith("/") else PurePath(filename).name + if not FILENAME_REGEX.fullmatch(filename): + raise error.AppError(f"invalid capture filename '{filename}'") + return filename + + +def _validate_snaplen(snaplen) -> int: + try: + snaplen = int(str(snaplen).strip()) + except ValueError: + raise error.AppError(f"snaplen must be an integer, got '{snaplen}'") from None + if snaplen < 0: + raise error.AppError(f"snaplen must not be negative, got {snaplen}") + return snaplen + + +def _convert_pcap(pcap_in: str, json_out: str) -> None: + with Path(json_out).open("w") as jf: + subprocess.run( + ["tshark", "-r", pcap_in, "-T", "ek"], + stdout=jf, + stderr=subprocess.DEVNULL, + ) + class MM(ComponentBase): def __init__(self): @@ -45,10 +78,15 @@ def __run(self, stage: str) -> None: if not bridge: raise ValueError("bridge to capture traffic on not provided") + utils._validate_mm_token(bridge) + now = datetime.utcnow() - filename = os.path.basename( - cap.get("filename", f"{bridge}-{now:%Y-%m-%dT%H:%M:%SZ}.pcap") - ) + filename = cap.get("filename", None) + + if filename: + filename = _validate_filename(filename) + else: + filename = f"{bridge}-{now:%Y-%m-%dT%H:%M:%SZ}.pcap" if not filename.lower().endswith(".pcap"): filename += ".pcap" @@ -56,19 +94,20 @@ def __run(self, stage: str) -> None: cap_filter = cap.get("filter", None) if cap_filter: - mm.capture_pcap_filter(cap_filter) + mm.capture_pcap_filter(utils._validate_mm_filter(cap_filter)) snaplen = cap.get("snaplen", None) if snaplen: - mm.capture_pcap_snaplen(snaplen) + mm.capture_pcap_snaplen(_validate_snaplen(snaplen)) try: logger.info(f"starting pcap capture for bridge {bridge}") - mm.mesh_send("all", f"shell mkdir -p {self.base_dir}") - mm.capture_pcap_bridge( - bridge, os.path.join(self.base_dir, filename) + mm.mesh_send( + "all", + f"shell mkdir -p {utils._validate_mm_token(str(self.base_dir))}", ) + mm.capture_pcap_bridge(bridge, str(Path(self.base_dir) / filename)) logger.info(f"started pcap capture for bridge {bridge}") except Exception as ex: raise RuntimeError( @@ -87,12 +126,18 @@ def __run(self, stage: str) -> None: if not bridge: raise ValueError("bridge to stop capture traffic on not provided") + + utils._validate_mm_token(bridge) try: logger.info(f"stopping pcap capture on bridge {bridge}") mm.capture_pcap_delete_bridge(bridge) logger.info(f"stopped pcap capture on bridge {bridge}") - mm.file_get(os.path.relpath(self.base_dir, self.root_dir)) + mm.file_get( + str( + Path(self.base_dir).relative_to(self.root_dir, walk_up=True) + ) + ) if cap.get("convert", False): logger.info( @@ -117,21 +162,15 @@ def __run(self, stage: str) -> None: ) # convert pcap files - for file in os.listdir(self.base_dir): - if file.endswith(".pcap") and not os.path.exists( - f"{file}.jsonl" - ): + for entry in self.base_dir.iterdir(): + file = entry.name + json_out = entry.with_name(f"{file}.jsonl") + if file.endswith(".pcap") and not json_out.is_file(): logger.info( f"starting PCAP --> JSON conversion of {file}" ) - pcap_in = os.path.join(self.base_dir, file) - json_out = f"{pcap_in}.jsonl" - - subprocess.run( - f"bash -c 'tshark -r {pcap_in} -T ek > {json_out} 2>/dev/null'", - shell=True, - ) + _convert_pcap(str(entry), str(json_out)) logger.info( f"PCAP --> JSON conversion of {file} complete" @@ -191,6 +230,11 @@ def __run(self, stage: str) -> None: bridge = conn.get("bridge", None) + utils._validate_mm_token(str(iface)) + utils._validate_mm_token(str(vlan)) + if bridge: + utils._validate_mm_token(bridge) + try: logger.info( f"connecting interface {iface} on {vm.hostname} to VLAN {vlan}" @@ -218,6 +262,8 @@ def __run(self, stage: str) -> None: f"interface to disconnect not provided for vm {vm.hostname}" ) + utils._validate_mm_token(str(iface)) + try: logger.info(f"disconnecting interface {iface} on {vm.hostname}") mm.vm_net_disconnect(vm.hostname, iface) @@ -241,13 +287,17 @@ def __run(self, stage: str) -> None: f"interface to capture traffic on not provided for vm {vm.hostname}" ) + utils._validate_mm_token(str(iface)) + now = utils.utc_now() - filename = os.path.basename( - cap.get( - "filename", - f"{vm.hostname}-{iface}-{now:%Y-%m-%dT%H:%M:%SZ}.pcap", + filename = cap.get("filename", None) + + if filename: + filename = _validate_filename(filename) + else: + filename = ( + f"{vm.hostname}-{iface}-{now:%Y-%m-%dT%H:%M:%SZ}.pcap" ) - ) if not filename.lower().endswith(".pcap"): filename += ".pcap" @@ -255,20 +305,23 @@ def __run(self, stage: str) -> None: cap_filter = cap.get("filter", None) if cap_filter: - mm.capture_pcap_filter(cap_filter) + mm.capture_pcap_filter(utils._validate_mm_filter(cap_filter)) snaplen = cap.get("snaplen", None) if snaplen: - mm.capture_pcap_snaplen(snaplen) + mm.capture_pcap_snaplen(_validate_snaplen(snaplen)) try: logger.info( f"starting pcap capture for interface {iface} on {vm.hostname}" ) - mm.mesh_send("all", f"shell mkdir -p {self.base_dir}") + mm.mesh_send( + "all", + f"shell mkdir -p {utils._validate_mm_token(str(self.base_dir))}", + ) mm.capture_pcap_vm( - vm.hostname, iface, os.path.join(self.base_dir, filename) + vm.hostname, iface, str(Path(self.base_dir) / filename) ) logger.info( f"started pcap capture for interface {iface} on {vm.hostname}" @@ -288,7 +341,13 @@ def __run(self, stage: str) -> None: mm.capture_pcap_delete_vm(vm.hostname) logger.info(f"stopped pcap capture(s) on VM {vm.hostname}") - mm.file_get(os.path.relpath(self.base_dir, self.root_dir)) + mm.file_get( + str( + Path(self.base_dir).relative_to( + self.root_dir, walk_up=True + ) + ) + ) if cap and cap.get("convert", False): logger.info( @@ -309,21 +368,15 @@ def __run(self, stage: str) -> None: break # convert pcap files - for file in os.listdir(self.base_dir): - if file.endswith(".pcap") and not os.path.exists( - f"{file}.jsonl" - ): + for entry in self.base_dir.iterdir(): + file = entry.name + json_out = entry.with_name(f"{file}.jsonl") + if file.endswith(".pcap") and not json_out.is_file(): logger.info( f"starting PCAP --> JSON conversion of {file}" ) - pcap_in = os.path.join(self.base_dir, file) - json_out = f"{pcap_in}.jsonl" - - subprocess.run( - f"bash -c 'tshark -r {pcap_in} -T ek > {json_out} 2>/dev/null'", - shell=True, - ) + _convert_pcap(str(entry), str(json_out)) logger.info( f"PCAP --> JSON conversion of {file} complete" diff --git a/src/python/phenix_apps/apps/scorch/pcap/pcap.py b/src/python/phenix_apps/apps/scorch/pcap/pcap.py index 24c56cee..1cbd1ba2 100644 --- a/src/python/phenix_apps/apps/scorch/pcap/pcap.py +++ b/src/python/phenix_apps/apps/scorch/pcap/pcap.py @@ -1,4 +1,5 @@ import shutil +import subprocess from pathlib import Path from phenix_apps.apps.scorch import ComponentBase @@ -47,9 +48,8 @@ def start(self): bpf_filter = self.metadata.get("filter") if bpf_filter: logger.info(f"applying pcap BPF filter: '{bpf_filter}'") - self.mm.capture_pcap_filter( - f"'{bpf_filter}'" - ) # single quotes are needed here + # single quotes are needed here + self.mm.capture_pcap_filter(f"'{utils._validate_mm_filter(bpf_filter)}'") # https://wiki.wireshark.org/SnapLen snaplen = self.metadata.get("snaplen") @@ -191,7 +191,7 @@ def stop(self): frag_path = t_path.with_stem(f"{t_path.stem}_fragmented") # give tshark 2 minutes to complete utils.run_command( - f'tshark -r {t_path} -w {frag_path} -o ip.defragment:FALSE -o tcp.desegment_tcp_streams:FALSE -n -Y "ip.frag_offset > 0"', + self._tshark_split(t_path, frag_path, "ip.frag_offset > 0"), timeout=120.0, ) fragmented.append(frag_path) @@ -199,7 +199,9 @@ def stop(self): nd_path = t_path.with_stem(f"{t_path.stem}_no_dos_fragments") # give tshark 2 minutes to complete utils.run_command( - f'tshark -r {t_path} -w {nd_path} -o ip.defragment:FALSE -o tcp.desegment_tcp_streams:FALSE -n -Y "not (ip.frag_offset > 0)"', + self._tshark_split( + t_path, nd_path, "not (ip.frag_offset > 0)" + ), timeout=120.0, ) non_dos.append(nd_path) @@ -253,7 +255,18 @@ def stop(self): logger.info( f"running PCAP --> JSON conversion (source={pcap_path.name}, dest={json_path.name})" ) - utils.run_command(f"tshark -r {pcap_path} -T ek > {json_path}") + with json_path.open("w") as json_file: + proc = subprocess.run( + ["tshark", "-r", str(pcap_path), "-T", "ek"], + stdout=json_file, + stderr=subprocess.PIPE, + check=False, + ) + if proc.returncode != 0: + raise RuntimeError( + f"PCAP --> JSON conversion of {pcap_path.name} failed: " + f"{proc.stderr.decode(errors='replace').strip()}" + ) else: logger.info("PCAP --> JSON conversion disabled") @@ -268,10 +281,8 @@ def _merge_pcaps( if overwriting: merged_path = merged_path.with_stem("temp_merged") - mergecap_cmd = f"mergecap -w {merged_path}" - for pcap_path in pcap_paths: - mergecap_cmd += f" {pcap_path}" - + mergecap_cmd = ["mergecap", "-w", str(merged_path)] + mergecap_cmd += [str(p) for p in pcap_paths] utils.run_command(mergecap_cmd, timeout=60.0) # this shouldn't take long if not merged_path.is_file(): @@ -285,9 +296,26 @@ def _merge_pcaps( return merged_path + @staticmethod + def _tshark_split(src: Path, dst: Path, display_filter: str) -> list[str]: + return [ + "tshark", + "-r", + str(src), + "-w", + str(dst), + "-o", + "ip.defragment:FALSE", + "-o", + "tcp.desegment_tcp_streams:FALSE", + "-n", + "-Y", + display_filter, + ] + def _dedupe_pcap(self, pcap_path: Path) -> None: dd_path = Path(self.base_dir, "deduped.pcap") - utils.run_command(f"editcap -d {pcap_path} {dd_path}", timeout=60.0) + utils.run_command(["editcap", "-d", str(pcap_path), str(dd_path)], timeout=60.0) if not dd_path.is_file(): raise RuntimeError( diff --git a/src/python/phenix_apps/apps/scorch/pipe/README.md b/src/python/phenix_apps/apps/scorch/pipe/README.md index 9ae47b89..1a5f6f2b 100644 --- a/src/python/phenix_apps/apps/scorch/pipe/README.md +++ b/src/python/phenix_apps/apps/scorch/pipe/README.md @@ -22,6 +22,11 @@ stages: start, cleanup > The `via`, `mode`, and `log` options are considered top-level commands and > if used, will be executed *before* any data is written to the pipe +> [!IMPORTANT] +> A `via` program runs on the phenix host. To lock this down, set +> `PHENIX_SCORCH_HOST_VALIDATORS=0` in the phenix environment; a `via` then +> fails the component. + ## Metadata Options ```yaml diff --git a/src/python/phenix_apps/apps/scorch/pipe/pipe.py b/src/python/phenix_apps/apps/scorch/pipe/pipe.py index a54779c6..1a5d5544 100644 --- a/src/python/phenix_apps/apps/scorch/pipe/pipe.py +++ b/src/python/phenix_apps/apps/scorch/pipe/pipe.py @@ -1,5 +1,7 @@ from phenix_apps.apps.scorch import ComponentBase +from phenix_apps.common import error, utils from phenix_apps.common.logger import logger +from phenix_apps.common.settings import SCORCH_HOST_VALIDATORS class Pipe(ComponentBase): @@ -23,7 +25,14 @@ def start(self): if not pipe: raise ValueError("pipe not specified but is required") + utils._validate_mm_token(pipe) + if via: + if not SCORCH_HOST_VALIDATORS: + raise error.AppError( + f"pipe 'via' runs '{via}' as a command on the host, and " + "host-side commands are disabled by PHENIX_SCORCH_HOST_VALIDATORS=0" + ) logger.info(f"setting via '{via}' for pipe '{pipe}'") self.mm.pipe_via(pipe, via) @@ -46,6 +55,10 @@ def start(self): if data: logger.info(f"writing data '{data}' to pipe '{pipe}'") + if any(c in str(data) for c in "'\n\r"): + raise error.AppError( + "pipe data may not contain single quotes or line breaks" + ) self.mm.pipe(pipe, f"'{data}'") logger.info(f"Started user component: {self.name}") @@ -59,7 +72,7 @@ def cleanup(self): raise ValueError("pipe not specified but is required") logger.info(f"clearing pipe '{pipe}'") - self.mm.clear_pipe(pipe) + self.mm.clear_pipe(utils._validate_mm_token(pipe)) logger.info(f"Cleaned up user component: {self.name}") diff --git a/src/python/phenix_apps/apps/scorch/ssh/README.md b/src/python/phenix_apps/apps/scorch/ssh/README.md index 1583722d..d4f50aab 100644 --- a/src/python/phenix_apps/apps/scorch/ssh/README.md +++ b/src/python/phenix_apps/apps/scorch/ssh/README.md @@ -15,6 +15,7 @@ metadata: ip: # (REQUIRED) Login IP of the Remote Collection Server. user: # (REQUIRED) User name of the Remote Collection Server. password: # (OPTIONAL) Password of the Remote Collection Server. If not provided, the connection will be attempted without a password, meaning and SSH key will need to be already established with the remote server and available at the default location ($HOME/.ssh). If running phenix in a container, the SSH key will need to be in /root/.ssh in the container. + known_hosts: # (OPTIONAL) Path to a known_hosts file holding the server's host key. If provided, unknown host keys are rejected. If not, the presented host key is trusted automatically and a warning is logged. cmds: - # (REQUIRED) Command to execute on the Remote Collection Server. - # (OPTIONAL) Additional Commands to execute on the Remote Collection Server. diff --git a/src/python/phenix_apps/apps/scorch/ssh/ssh.py b/src/python/phenix_apps/apps/scorch/ssh/ssh.py index 9514aab8..b3c2a359 100644 --- a/src/python/phenix_apps/apps/scorch/ssh/ssh.py +++ b/src/python/phenix_apps/apps/scorch/ssh/ssh.py @@ -132,7 +132,17 @@ def _connect(self) -> paramiko.SSHClient: paramiko.SSHException: If the connection or authentication fails. """ ssh = paramiko.SSHClient() - ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) + + known_hosts = self.metadata.get("known_hosts") if self.metadata else None + if known_hosts: + ssh.load_host_keys(known_hosts) + ssh.set_missing_host_key_policy(paramiko.RejectPolicy()) + else: + logger.warning( + f"no 'known_hosts' file provided in metadata; automatically " + f"trusting host key presented by {self.ip}" + ) + ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) connect_kwargs = { "hostname": self.ip, diff --git a/src/python/phenix_apps/apps/scorch/tcpdump/tcpdump.py b/src/python/phenix_apps/apps/scorch/tcpdump/tcpdump.py index 128d1dc7..3ea055bb 100644 --- a/src/python/phenix_apps/apps/scorch/tcpdump/tcpdump.py +++ b/src/python/phenix_apps/apps/scorch/tcpdump/tcpdump.py @@ -1,11 +1,22 @@ +import re import subprocess +from pathlib import Path +from typing import Final 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 # TODO: merge tcpdump's functionality into the 'pcap' component +IFACE_REGEX: Final[re.Pattern[str]] = re.compile(r"^[A-Za-z0-9_.:-]{1,32}$") + + +def _validate_iface(iface: str) -> str: + if not IFACE_REGEX.fullmatch(iface): + raise error.AppError(f"invalid interface name '{iface}'") + return iface + class TCPDump(ComponentBase): def __init__(self): @@ -32,6 +43,8 @@ def start(self): if not iface: raise ValueError("no interface name provided for VM config") + iface = _validate_iface(iface) + res = utils.mm_exec_wait(mm, hostname, "which tcpdump") if not res["stdout"]: raise RuntimeError(f"tcpdump is not installed in VM {hostname}") @@ -73,8 +86,10 @@ def stop(self): if not iface: raise ValueError("no interface name provided for VM config") - pcap_out = f"{self.base_dir}/{hostname}-{iface}.pcap" - json_out = f"{self.base_dir}/{hostname}-{iface}.pcap.jsonl" + iface = _validate_iface(iface) + + pcap_out = str(Path(self.base_dir) / f"{hostname}-{iface}.pcap") + json_out = Path(self.base_dir) / f"{hostname}-{iface}.pcap.jsonl" utils.mm_exec_wait(mm, hostname, "pkill tcpdump") @@ -88,10 +103,12 @@ def stop(self): if convert: logger.info(f"starting PCAP --> JSON conversion for node {hostname}...") - subprocess.run( - f"bash -c 'tshark -r {pcap_out} -T ek > {json_out} 2>/dev/null'", - shell=True, - ) + with json_out.open("w") as jf: + subprocess.run( + ["tshark", "-r", pcap_out, "-T", "ek"], + stdout=jf, + stderr=subprocess.DEVNULL, + ) logger.info(f"PCAP --> JSON conversion for node {hostname} complete") diff --git a/src/python/phenix_apps/common/settings.py b/src/python/phenix_apps/common/settings.py index 73fac805..4a1e9e76 100644 --- a/src/python/phenix_apps/common/settings.py +++ b/src/python/phenix_apps/common/settings.py @@ -1,41 +1,46 @@ import os +from typing import Final # phenix logging level. Options: ['DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL'] -PHENIX_LOG_LEVEL = os.getenv("PHENIX_LOG_LEVEL", "INFO") +PHENIX_LOG_LEVEL: Final = os.getenv("PHENIX_LOG_LEVEL", "INFO") # Path to phenix log file -PHENIX_LOG_FILE = os.getenv("PHENIX_LOG_FILE", "/var/log/phenix/phenix-apps.log") +PHENIX_LOG_FILE: Final = os.getenv("PHENIX_LOG_FILE", "/var/log/phenix/phenix-apps.log") # Base phenix data directory -PHENIX_DIR = os.getenv("PHENIX_DIR", "/phenix") +PHENIX_DIR: Final = os.getenv("PHENIX_DIR", "/phenix") # phenix temporary directory -PHENIX_TEMP_DIR = os.getenv("PHENIX_TEMP_DIR", "/tmp/phenix") +PHENIX_TEMP_DIR: Final = os.getenv("PHENIX_TEMP_DIR", "/tmp/phenix") # Base minimega filepath -MM_FILEPATH = os.getenv("MM_FILEPATH", "/phenix/images") +MM_FILEPATH: Final = os.getenv("MM_FILEPATH", "/phenix/images") # Base minimega filepath -MM_SOCKET_PATH = os.getenv("MM_SOCKET_PATH", "/tmp/minimega/minimega") +MM_SOCKET_PATH: Final = os.getenv("MM_SOCKET_PATH", "/tmp/minimega/minimega") # Seconds between cc client/command polls -CC_POLL_RATE = float(os.getenv("PHENIX_CC_POLL_RATE", 2.0)) +CC_POLL_RATE: Final = float(os.getenv("PHENIX_CC_POLL_RATE", 2.0)) # Seconds between client-liveness checks during an unbounded cc wait -CC_LIVENESS_INTERVAL = float(os.getenv("PHENIX_CC_LIVENESS_INTERVAL", 10.0)) +CC_LIVENESS_INTERVAL: Final = float(os.getenv("PHENIX_CC_LIVENESS_INTERVAL", 10.0)) # Seconds to wait for a cc exit code after the response is counted -CC_EXITCODE_GRACE = float(os.getenv("PHENIX_CC_EXITCODE_GRACE", 10.0)) +CC_EXITCODE_GRACE: Final = float(os.getenv("PHENIX_CC_EXITCODE_GRACE", 10.0)) # Seconds to wait for a miniccc client to register -CC_CLIENT_GRACE = float(os.getenv("PHENIX_CC_CLIENT_GRACE", 300.0)) +CC_CLIENT_GRACE: Final = float(os.getenv("PHENIX_CC_CLIENT_GRACE", 300.0)) # Seconds to wait for a cc file send or component start to be acknowledged -CC_SEND_GRACE = float(os.getenv("PHENIX_CC_SEND_GRACE", 300.0)) +CC_SEND_GRACE: Final = float(os.getenv("PHENIX_CC_SEND_GRACE", 300.0)) # Seconds to wait for a cc command response. 0 == unbounded -CC_CMD_GRACE = float(os.getenv("PHENIX_CC_CMD_GRACE", 0.0)) +CC_CMD_GRACE: Final = float(os.getenv("PHENIX_CC_CMD_GRACE", 0.0)) # Seconds before the first "still waiting" log line, doubling up to the max -CC_LOG_INTERVAL = float(os.getenv("PHENIX_CC_LOG_INTERVAL", 10.0)) -CC_LOG_MAX_INTERVAL = float(os.getenv("PHENIX_CC_LOG_MAX_INTERVAL", 320.0)) +CC_LOG_INTERVAL: Final = float(os.getenv("PHENIX_CC_LOG_INTERVAL", 10.0)) +CC_LOG_MAX_INTERVAL: Final = float(os.getenv("PHENIX_CC_LOG_MAX_INTERVAL", 320.0)) + +# Allow SCORCH components to run scenario-supplied commands on the phenix host +# (art and cc validators, pipe via). On unless set to exactly "0". +SCORCH_HOST_VALIDATORS: Final = os.getenv("PHENIX_SCORCH_HOST_VALIDATORS", "1") != "0" diff --git a/src/python/phenix_apps/common/tests/test_command_helpers.py b/src/python/phenix_apps/common/tests/test_command_helpers.py new file mode 100644 index 00000000..22d5bbf7 --- /dev/null +++ b/src/python/phenix_apps/common/tests/test_command_helpers.py @@ -0,0 +1,103 @@ +""" +Tests for the shared command helpers: values spliced into raw minimega +commands must be single tokens, and run_command never goes through a shell. +""" + +import importlib + +import pytest + +from phenix_apps.common import settings, utils +from phenix_apps.common.error import AppError + + +class TestValidateMmToken: + @pytest.mark.parametrize("value", ["exp1", "node-2", "a.b_c"]) + def test_accepts(self, value): + assert utils._validate_mm_token(value) == value + + @pytest.mark.parametrize( + "value", + ["", "two words", "new\nline", "tab\there", "quo'te", 'dou"ble'], + ) + def test_rejects(self, value): + with pytest.raises(AppError): + utils._validate_mm_token(value) + + +class TestRunCommand: + def test_string_form_is_shlex_split_not_shell(self): + # A shell would expand the glob and the semicolon; argv exec must not. + out = utils.run_command("echo 'a; touch /tmp/pwned' *") + assert out.strip() == "a; touch /tmp/pwned *" + + def test_list_form_passthrough(self): + assert utils.run_command(["echo", "hi"]).strip() == "hi" + + +@pytest.mark.parametrize( + ("value", "expected"), [(None, True), ("1", True), ("0", False)] +) +def test_scorch_host_validators_on_unless_zero(monkeypatch, value, expected): + if value is None: + monkeypatch.delenv("PHENIX_SCORCH_HOST_VALIDATORS", raising=False) + else: + monkeypatch.setenv("PHENIX_SCORCH_HOST_VALIDATORS", value) + try: + assert importlib.reload(settings).SCORCH_HOST_VALIDATORS is expected + finally: + monkeypatch.undo() + importlib.reload(settings) + + +class TestValidateMmFilter: + def test_accepts_spaces(self): + assert utils._validate_mm_filter("tcp port 80") == "tcp port 80" + + @pytest.mark.parametrize("value", ["x' or 1", 'a"b', "a\nb", "a\rb"]) + def test_rejects_quotes_and_line_breaks(self, value): + with pytest.raises(AppError): + utils._validate_mm_filter(value) + + +def test_mm_capture_filename_allows_timestamps(): + from phenix_apps.apps.scorch.mm import mm + + assert mm._validate_filename("br0-2024-01-01T10:00:00Z+x.pcap") + for bad in ["two words.pcap", "a.pcap\n"]: + with pytest.raises(AppError): + mm._validate_filename(bad) + + +def test_mm_snaplen_must_be_a_non_negative_integer(): + from phenix_apps.apps.scorch.mm import mm + + assert mm._validate_snaplen(" 96 ") == 96 + for bad in ["96 -w /x", -1]: + with pytest.raises(AppError): + mm._validate_snaplen(bad) + + +def test_pcap_tshark_split_keeps_the_display_filter_one_argument(): + from pathlib import Path + + from phenix_apps.apps.scorch.pcap.pcap import PCAP + + cmd = PCAP._tshark_split(Path("/r/a b.pcap"), Path("/r/out.pcap"), "not (x > 0)") + assert cmd[cmd.index("-r") + 1] == "/r/a b.pcap" + assert cmd[-2:] == ["-Y", "not (x > 0)"] + + +class TestMetadataValidators: + def test_ovs_name(self): + assert utils.validate_ovs_name("br-0", "bridge") == "br-0" + for bad in [None, "", "two words", "abcdefghijklmnop", 7, "br0\n"]: + with pytest.raises(AppError): + utils.validate_ovs_name(bad, "bridge") + + def test_ip(self): + assert utils.validate_ip("10.0.0.1", "ip") == "10.0.0.1" + assert utils.validate_ip("fe80::1", "ip") == "fe80::1" + for bad in [None, "10.0.0.1; reboot", "host"]: + with pytest.raises(AppError): + utils.validate_ip(bad, "ip") diff --git a/src/python/phenix_apps/common/utils.py b/src/python/phenix_apps/common/utils.py index ba1f122b..f1f09fb5 100644 --- a/src/python/phenix_apps/common/utils.py +++ b/src/python/phenix_apps/common/utils.py @@ -1,9 +1,11 @@ import csv import datetime +import ipaddress import json import math import random import re +import shlex import shutil import socket import stat @@ -16,7 +18,7 @@ from pathlib import Path from socket import inet_ntoa from struct import pack -from typing import IO +from typing import IO, Final import mako.lookup import mako.template @@ -368,13 +370,16 @@ def mm_recv( mm: minimega.minimega, vm: str, src: list[str] | str, - dst: str, + dst: str | Path, grace: float = phenix_settings.CC_CLIENT_GRACE, ) -> None: """ Transfer one or more files from a VM to a destination on the host using miniccc mounts. """ + # The normalization check below compares the caller's spelling of the path. + dst = str(dst) + # Use PHENIX_DIR as base directory to ensure minimega has access to it. This # assumes PHENIX_DIR is mounted into the containers if containers are being # used. @@ -1106,9 +1111,12 @@ def mm_compute_cmd(mm: minimega.minimega, **kwargs) -> list[dict]: if param not in kwargs: raise ValueError(f"Missing required parameter: {param}") - experiment = kwargs["experiment"] + experiment = _validate_mm_token(kwargs["experiment"]) computes = kwargs.get("computes", "all") # 'all' or comma-separated list computes_list = computes.split(",") + + for compute in computes_list: + _validate_mm_token(compute) command = kwargs["command"] command_type = kwargs.get("command_type", "shell") ignore_error = kwargs.get("ignore_error", False) @@ -1142,6 +1150,58 @@ def mm_compute_cmd(mm: minimega.minimega, **kwargs) -> list[dict]: return results +def _validate_mm_token(value: str) -> str: + """ + Ensure a value spliced into a raw minimega control-socket command is a + single benign token (no whitespace, newlines, or quotes). + """ + if not value or re.search(r"[\s'\"]", value): + raise AppError(f"invalid minimega command token '{value}'") + + return value + + +OVS_NAME_REGEX: Final[re.Pattern[str]] = re.compile(r"^[A-Za-z0-9_.-]{1,15}$") + + +def validate_ovs_name(value: object, field: str) -> str: + """ + A required metadata value that names an OVS bridge or interface: a single + token of at most 15 characters (IFNAMSIZ), safe to splice into ovs-vsctl + and minimega commands. + """ + if value is None: + raise AppError(f"no '{field}' provided in metadata") + if not isinstance(value, str) or not OVS_NAME_REGEX.fullmatch(value): + raise AppError( + f"invalid '{field}' value '{value}' in metadata: must match " + f"{OVS_NAME_REGEX.pattern}" + ) + + return value + + +def validate_ip(value: object, field: str) -> str: + """A required metadata value that is an IPv4 or IPv6 address.""" + if value is None: + raise AppError(f"no '{field}' provided in metadata") + try: + return str(ipaddress.ip_address(str(value))) + except ValueError as ex: + raise AppError(f"invalid '{field}' value '{value}' in metadata") from ex + + +def _validate_mm_filter(cap_filter: str) -> str: + """ + Ensure a capture filter can be wrapped in single quotes in a raw minimega + command: it may contain spaces, but no quotes or line breaks. + """ + if any(c in cap_filter for c in "'\"\n\r"): + raise AppError(f"invalid characters in capture filter '{cap_filter}'") + + return cap_filter + + def _mm_socket_cmd(cmd: str, ignore_error: bool = False) -> list[str]: """Send command to minimega socket and get a response. @@ -1305,8 +1365,10 @@ def mm_delete_file( ) -def run_command(cmd: str, timeout: float | None = None) -> str: - result = subprocess.check_output(cmd, shell=True, timeout=timeout) +def run_command(cmd: str | list[str], timeout: float | None = None) -> str: + if isinstance(cmd, str): + cmd = shlex.split(cmd) + result = subprocess.check_output(cmd, timeout=timeout) if isinstance(result, bytes): result = result.decode() return result @@ -1393,7 +1455,17 @@ def trim_pcap( # YYYY-MM-DDThh:mm:ss.nnnnnnnnn[Z|+-hh:mm] # editcap -A start-time -B stop-time run_command( - f"editcap -F {cap_type} -A {start_time.isoformat()} -B {end_time.isoformat()} {src.as_posix()} {edited.as_posix()}" + [ + "editcap", + "-F", + cap_type, + "-A", + start_time.isoformat(), + "-B", + end_time.isoformat(), + src.as_posix(), + edited.as_posix(), + ] ) trimmed_size = edited.stat().st_size @@ -1420,7 +1492,7 @@ def pcap_capinfos(pcap_path: str | Path) -> dict: {'File name': './br14-0.pcap', 'File type': 'pcap', 'File encapsulation': 'ether', 'File time precision': 'microseconds', 'Packet size limit': '1600', 'Packet size limit min (inferred)': 'n/a', 'Packet size limit max (inferred)': 'n/a', 'Number of packets': '37', 'File size (bytes)': '3862', 'Data size (bytes)': '3246', 'Capture duration (seconds)': '28.975097', 'Start time': '2024-02-21 22:27:36.592584', 'End time': '2024-02-21 22:28:05.567681', 'Data byte rate (bytes/sec)': '112.03', 'Data bit rate (bits/sec)': '896.22', 'Average packet size (bytes)': '87.73', 'Average packet rate (packets/sec)': '1.28', 'SHA256': '2b07c65ec9f00c6ea3334ccd1f49074c4f643c68776a3a8cae990e824cbbf72a', 'SHA1': 'dcc7cb3f070b8757693a30a6e75ddc5542686072', 'Strict time order': 'True', 'Capture hardware': '', 'Capture oper-sys': '', 'Capture application': '', 'Capture comment': ''} """ - capinfo_output = run_command(f"capinfos -T -M {pcap_path}") + capinfo_output = run_command(["capinfos", "-T", "-M", str(pcap_path)]) io_obj = StringIO(capinfo_output) reader = csv.DictReader(io_obj, delimiter="\t") # tab-delimited diff --git a/src/python/phenix_apps/testing/plugin.py b/src/python/phenix_apps/testing/plugin.py index 86eaf11e..1232ea0b 100644 --- a/src/python/phenix_apps/testing/plugin.py +++ b/src/python/phenix_apps/testing/plugin.py @@ -69,7 +69,9 @@ def pytest_configure(config): @pytest.fixture(scope="session", autouse=True) def _silence_phenix_log(): """Disable file logging across the test session.""" - settings.PHENIX_LOG_FILE = None + with pytest.MonkeyPatch.context() as mp: + mp.setattr(settings, "PHENIX_LOG_FILE", None) + yield @pytest.fixture