INTO-CPS-Association / INTO-CPS-Association/workspace
[BUG]Potential Vulnerabilities in Build Scripts
- Dominant language
- Python
- Stars
- 0
- Forks
- 5
- PR merge metrics
- No merged PRs in 30d
Description
### Describe the feature
As a **maintainer**, I want to **harden the startup, nginx, install, and CI/CD code outside of `workspaces/src/admin`** so that **container startup, routing, and quality gates behave reliably and match what the documentation promises**.
This tracks a re-verification of `PENDING_ISSUES.md` (unresolved PR review comments) and `POTENTIAL_IMPROVEMENTS.md` (codebase scan) against the current `main` branch, scoped to everything **outside** `workspaces/src/admin` (the FastAPI admin service already has its own follow-up work tracked separately). Each item below was re-checked against the current source before inclusion.
### Describe the problems your feature request solves
1. **Unsafe/unvalidated env var interpolation in `configure_nginx.py`** (`workspaces/src/startup/configure_nginx.py:12-77`)
Every placeholder substitution does `os.getenv(...)` and immediately string-concatenates the result into a `subprocess.call(..., shell=True)` command, e.g.:
```python
main_user = os.getenv("MAIN_USER")
call("sed -i 's@{MAIN_USER}@" + main_user + "@g' " + NGINX_FILE, shell=True)
```
If the variable is unset, `main_user` is `None` and the `+` concatenation raises `TypeError`, crashing nginx configuration at container startup. If the variable contains shell metacharacters, it can break out of the `sed` command (shell injection) since `shell=True` is used with string concatenation instead of an argument list.
**Security reference:** [CWE-78](https://cwe.mitre.org/data/definitions/78.html) (OS Command Injection), [CWE-88](https://cwe.mitre.org/data/definitions/88.html) (Argument Injection), [CWE-20](https://cwe.mitre.org/data/definitions/20.html) (Improper Input Validation), [CWE-252](https://cwe.mitre.org/data/definitions/252.html) (Unchecked Return Value, for the untested `None`). Directly matches SEI CERT's **ENV33-C — Do not call `system()`** and **ENV03-C — Sanitize the environment when invoking external programs**: never build a shell command string from unsanitized external input; pass arguments as a list to a non-shell `exec`-style call instead.
2. **`workspace-admin` is not reliably on `PATH` at runtime** (`workspaces/src/install/admin/install_admin.sh:40-43`, `workspaces/src/startup/custom_startup.sh:46-54`)
`install_admin.sh` installs the CLI with `pipx install`, then does `pipx ensurepath && source ~/.bashrc` to get `command -v workspace-admin` to succeed *within that single RUN step*. But:
- Each Dockerfile `RUN` is its own shell process, so the `.bashrc` `PATH` edit doesn't survive into later layers.
- The final `PATH` snapshot (`workspaces/Dockerfile.ubuntu.noble.xfce:79`, written to `.docker_set_envs`) is captured in a later, separate `RUN` step that never sourced `.bashrc`, so it very likely does **not** include the pipx bin directory.
- `custom_startup.sh`'s `start_admin_server` then calls bare `workspace-admin` with no absolute-path fallback.
Net effect: the admin server can fail to start at runtime with `workspace-admin: command not found`, despite the install script "verifying" it works at build time.
**Security reference:** [CWE-703](https://cwe.mitre.org/data/definitions/703.html) (Improper Check for Unusual or Exceptional Conditions) and [CWE-250](https://cwe.mitre.org/data/definitions/250.html) (Execution with Unnecessary Privileges) — the CLI is installed and `pipx ensurepath`-verified while running as **root** during the build (see `USER root` / no user switch in `workspaces/Dockerfile.ubuntu.noble.xfce` before this step), which is broader privilege than needed for a pure install step and obscures the runtime `PATH` gap. Prefer installing to a fixed, unprivileged system location (least privilege) rather than relying on a root-owned pipx home.
3. **`/services` nginx location isn't anchored** (`workspaces/src/startup/nginx.conf:48`)
```nginx
location ~* "^{WORKSPACE_BASE_URL_DECODED}/services" {
```
The regex has no end anchor, so after substitution it can unintentionally match unrelated paths like `/user1/servicesXYZ`.
**Security reference:** [CWE-625](https://cwe.mitre.org/data/definitions/625.html) (Permissive Regular Expression) and [CWE-284](https://cwe.mitre.org/data/definitions/284.html) (Improper Access Control) — an unanchored, case-insensitive route match on an internal service-discovery endpoint is a routing/access-control boundary that should fail closed (exact or explicitly delimited match), not rely on the substituted prefix happening not to collide with other paths.
4. **`custom_startup.sh` has no pre-flight command checks and swallows restart failures** (`workspaces/src/startup/custom_startup.sh`)
The monitoring loop detects a dead process and calls `start_*` again, but none of the `start_*` functions validate that the underlying binary exists first, and failures to relaunch aren't surfaced (no differentiation between "restarted successfully" and "failed to restart"). Combined with issue #2, a missing `workspace-admin` binary would silently loop-fail forever without ever logging why.
**Security reference:** [CWE-390](https://cwe.mitre.org/data/definitions/390.html) (Detection of Error Condition Without Action) and [CWE-703](https://cwe.mitre.org/data/definitions/703.html) — a service silently failing to restart is an availability/monitoring gap (CIA triad: Availability); fail loudly (non-zero exit, clear log line) so operators/orchestrators can detect and act on it.
5. **Docker socket mounted in the dev Traefik compose with no documented mitigation** (`workspaces/test/dtaas/compose.traefik.yml:25`)
```yaml
volumes:
- /var/run/docker.sock:/var/run/docker.sock
```
This grants Traefik root-equivalent access to the Docker daemon. `TRAEFIK.md` currently has no mention of this risk or a recommended mitigation (e.g. a Docker socket proxy) for anyone adapting this file for production.
**Security reference:** [CWE-269](https://cwe.mitre.org/data/definitions/269.html) (Improper Privilege Management) and [CWE-250](https://cwe.mitre.org/data/definitions/250.html) (Execution with Unnecessary Privileges) — the [CIS Docker Benchmark](https://www.cisecurity.org/benchmark/docker) explicitly recommends against mounting `docker.sock` into a container's filesystem, since it is equivalent to unrestricted root on the host. If it can't be removed, gate it behind a scoped proxy (e.g. `tecnativa/docker-socket-proxy`) and call this out prominently, not silently, in `TRAEFIK.md`.
6. **Leftover template cruft in `compose.traefik.yml`**
- Lines 20-21 define Traefik router/service labels for `myservice`, which doesn't correspond to any service in the file.
- The `frontend` network (line 27, defined line 67-68) is attached to `traefik` but no other service uses it, with no comment explaining why it's reserved.
**Security reference:** Minor by itself, but stale/unexplained routing rules and network attachments are exactly the kind of configuration drift the CIS Docker Benchmark's "minimize attack surface" guidance warns about — every label, network, and open route should be either used or removed so a later reviewer isn't left guessing whether it's dead config or a forgotten access path.
7. **CI Docker Compose lint step can pass without linting anything** (`.github/workflows/docker-lint.yml:35-43`)
```bash
for file in $(find . -name "compose*.yaml" -o -name "compose*.yml"); do
docker compose -f "$file" ... config --quiet
done
```
If `find` returns nothing (e.g. after a restructure that changes the naming convention), the loop silently does nothing and the step still reports success.
**Security reference:** [CWE-390](https://cwe.mitre.org/data/definitions/390.html) (Detection of Error Condition Without Action) — a quality gate that can report green while checking zero files is a broken control, the CI equivalent of a security check that's been silently disabled; it should fail loudly if its input set is empty.
8. **README documents a markdownlint CI gate that doesn't exist** (`README.md:232`, `.markdownlint.yaml`)
README states "Markdown files: Checked with markdownlint," and a `.markdownlint.yaml` config is present, but no workflow under `.github/workflows/` actually runs markdownlint. Docs and enforced CI are out of sync.
**Note:** primarily a documentation-accuracy issue rather than a direct vulnerability, but a documented control that isn't actually enforced is worth closing for the same reason security policies should match implemented controls (don't let stated guarantees silently drift from reality).
9. **No Dockerfile `HEALTHCHECK`** (`workspaces/Dockerfile.ubuntu.noble.xfce`)
nginx already exposes a `/ping` endpoint (`workspaces/src/startup/nginx.conf:43-46`) that returns 200, but the Dockerfile never wires up a `HEALTHCHECK` to use it, so container orchestrators have no built-in way to detect an unhealthy workspace container.
**Security reference:** [CIS Docker Benchmark](https://www.cisecurity.org/benchmark/docker) §4.6, "Ensure that HEALTHCHECK instructions have been added to container images" — beyond operational convenience, a missing health check delays detection of a hung/compromised container, an availability (CIA triad) gap.
### Describe the solution you'd like
- `configure_nginx.py`: validate every required env var up front (fail fast with a clear message if unset/empty — addresses CWE-20/CWE-252) and stop building shell strings by concatenation — use `subprocess.run([...])` argument lists (no `shell=True`) or plain Python file read/replace/write instead of `shell=True` + `sed`, per CERT ENV33-C/ENV03-C and to eliminate CWE-78/CWE-88 injection paths outright.
- Admin install/startup: install `workspace-admin` to a location guaranteed to be on `PATH` at runtime (e.g. a system bin dir via `PIPX_HOME`/`PIPX_BIN_DIR`, or an explicit `ENV PATH=...` set before the `.docker_set_envs` snapshot is taken) rather than a root-owned pipx home (least privilege, CWE-250), and/or have `custom_startup.sh` resolve an absolute path with a clear error if the binary is missing.
- `nginx.conf`: anchor the `/services` location regex (e.g. `/services$` or an exact/prefix match) so it can't over-match (closes the CWE-625/CWE-284 gap).
- `custom_startup.sh`: validate required commands exist before first use, and log clearly (non-silent failure, CWE-390) when a restart attempt itself fails instead of looping silently.
- `compose.traefik.yml`: remove the unused `myservice` labels, and either drop the unused `frontend` network or add a comment explaining its future purpose (attack-surface minimization); document the Docker-socket risk (CWE-269) and a socket-proxy mitigation, per the CIS Docker Benchmark, in `TRAEFIK.md`.
- `docker-lint.yml`: fail the step explicitly if no compose files are found (CWE-390 — a check that can't fail isn't a check).
- Either add a markdownlint job to CI, or remove the claim from `README.md` until one exists — keep documented controls and enforced controls in sync.
- Add a `HEALTHCHECK` instruction to `workspaces/Dockerfile.ubuntu.noble.xfce` pointing at `/ping`, per CIS Docker Benchmark §4.6.
### Describe alternatives you've considered
Filing each item as its own issue was considered, but since they were already catalogued together in `PENDING_ISSUES.md`/`POTENTIAL_IMPROVEMENTS.md` and share the same "startup/routing/CI reliability" theme, one consolidated issue keeps the tracking overhead low; individual items can be split into separate PRs as they're picked up.
### Additional context
Source docs: `PENDING_ISSUES.md` (unresolved PR review comments) and `POTENTIAL_IMPROVEMENTS.md` (codebase scan). Every item above was independently re-verified against current `main` — a few originally-listed items (e.g. "missing `apt-get update` in `install_admin.sh`", "Poetry/lock-version compatibility") were checked and either already mitigated or not confirmable from static inspection, so they were left out of this issue. Items specific to `workspaces/src/admin` itself are intentionally out of scope here and should be tracked separately.
**Security standards referenced** (for quick reviewer scanning): CWE-78 (OS Command Injection), CWE-88 (Argument Injection), CWE-20 (Improper Input Validation), CWE-252 (Unchecked Return Value), CWE-703 (Improper Check for Unusual Conditions), CWE-269 / CWE-250 (Improper Privilege Management / Unnecessary Privileges), CWE-625 (Permissive Regular Expression), CWE-284 (Improper Access Control), CWE-390 (Detection of Error Condition Without Action); SEI CERT Secure Coding Standard rules ENV33-C ("Do not call `system()`") and ENV03-C ("Sanitize the environment when invoking external programs"); [CIS Docker Benchmark](https://www.cisecurity.org/benchmark/docker) guidance on Docker-socket mounting and container `HEALTHCHECK` (§4.6).
### Success Criterion
- [ ] `configure_nginx.py` fails fast with a clear error on missing env vars and no longer builds shell commands via string concatenation
- [ ] `workspace-admin` reliably starts at container runtime (verified via a fresh `docker compose up`)
- [ ] `/services` nginx location is anchored and no longer over-matches
- [ ] `custom_startup.sh` validates required commands and logs restart failures
- [ ] `compose.traefik.yml` has no unused labels/networks (or they're documented), and `TRAEFIK.md` documents the Docker socket risk
- [ ] CI compose-lint step fails if no compose files are found
- [ ] Markdownlint is either enforced in CI or the README claim is removed
- [ ] `workspaces/Dockerfile.ubuntu.noble.xfce` has a working `HEALTHCHECK`
- [ ] Documentation Updated
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by splitting the work across the named files: workspaces/src/startup/configure_nginx.py, custom_startup.sh, nginx.conf, the admin install script, compose.traefik.yml, docker-lint.yml, README.md, and the Dockerfile. Re-read each listed failure and its proposed remedy, then verify the relevant startup, Compose, CI, and health-check behavior. Done means the security and reliability gaps are fixed, documented controls match CI, and each affected check fails clearly when its input or command is unavailable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- docker, docker-compose, github-actions, nginx, python, shell
- Domain
- ci-cd, devops, documentation, infrastructure, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 32/100