Security and correctness hardening pass - #5
Open
solomonneas wants to merge 13 commits into
Open
Conversation
Bring the README and maintainer-health files up to the adoption standard. README: - Prominent website link to lidless.dev/soc-stack near the top - Live CI and release status badges plus the existing tool badges - Sharpened first paragraph: what / why / how it differs - New keyword-rich "What it does" section (SOC lab, Proxmox, Wazuh, TheHive, Cortex, MISP, Zeek, Suricata, homelab, MCP) - Redacted result-JSON example block (RFC 5737 documentation IPs) - "Why not something else?" and "What soc-stack is not" sections - Local clone quickstart and an honest single-maintainer status note Health files: - Add CODE_OF_CONDUCT.md (Contributor Covenant 2.1) - Add .github/ISSUE_TEMPLATE/config.yml (no blank issues, route security and questions off the issue tracker) - Add a no-PII / no-leak checkbox to the pull request template - Add an Unreleased CHANGELOG entry for the docs work
…daction bugs
Remove the unreferenced legacy scripts/setup/** tree (Plan-3 leftover). It was
excluded from the shellcheck gate and carried real smells that contradicted the
security posture: verify=False forwarding alerts to TheHive, a MISP installer
pulled from the mutable 2.4 branch and run as root, cleartext password printing,
stale default-credential text, and an allow-all firewall. Nothing live sourced
it (only design docs referenced it).
Shared-lib correctness and secret-handling fixes, all covered by new unit tests:
- secrets.sh: gen_password returned 141 on success under `set -o pipefail`
because tr takes SIGPIPE when head closes the pipe. Capture, tolerate the
expected failure, and assert the length so a genuine short read still fails.
- json-out.sh state_set: `jq > tmp; mv tmp f` ran the mv even when jq failed,
truncating good state to empty (broke idempotency). Only replace on success,
reinitialize a corrupt file instead of destroying it, mktemp in the target
dir for an atomic same-fs rename, and pass the key via --arg (no jq injection).
- json-out.sh state_get: tolerate a corrupt state file (read as empty, do not
abort a `set -e` caller); pass key via --arg.
- json-out.sh redact_json: broaden the key heuristic (pwd/passwd/bearer/
credential/private_key) and scrub credentials embedded in URL values, while
only collapsing a matching key when its value is scalar so nested shapes and
their per-key redaction are preserved.
- manifest.sh validate_manifest: exact-token component match; `grep -qw` had
false-accepted "cortex" and treated names as regex ("m.p" matched "mcp").
- install.sh parse_args: accept the --flag=value form (was rejected as unknown).
…t status The orchestrator validated and stored several network settings but never applied them, and a successful deploy could fail to record itself. All covered by new dry-run unit tests. - --vlan was parsed, validated, and written to the manifest but the tag never reached the container net config, so it silently did nothing. Append ,tag=<vlan> to --net0. - --ip-mode static built ip= with no gw=, so containers came up with an address but no default route; lxc_wait_network then timed out pinging 8.8.8.8 and every static deploy failed. Derive the gateway (new --gateway flag, else first host of the range) and append ,gw=<gw>. Adds default_gateway() to lib/network.sh. - Static IPs were allocated by the component's position in the selected subset, so `--components misp` (index 0) collided with an already-deployed wazuh (also index 0). Allocate by the fixed canonical ordinal via component_ordinal(). - A successful deploy only got status=deployed if the component's in-LXC deploy.sh wrote it and the pct pull merged it; otherwise the component was re-deployed on every rerun. Record status=deployed authoritatively after verify passes.
Secrets passed on a command line are readable via /proc/<pid>/cmdline by any
local UID for the life of the process. Two provisioning paths did this; move
both to stdin, matching the pattern misp/deploy.sh already uses.
- install.sh: set the LXC root password with `chpasswd` over stdin after start
instead of `pct create --password <pw>` (which exposed it on the Proxmox host
process list). Dry-run message updated to match.
- thehive-cortex/deploy.sh: the four TheHive/Cortex admin-credential requests
used `curl -d "{...password...}"`, exposing the password inside an LXC that
also runs untrusted container workloads. Feed the JSON body via `printf | curl
--data @-`; printf is a shell builtin so the secret never reaches an argv.
…vilege The integration-component job runs on the self-hosted Proxmox runner (which can create/destroy LXCs, and whose gh-runner account has broad sudo including bash) on every pull_request with no fork guard. A fork PR editing the CI helper or a deploy script could run as root on the host. - Add `if: github.event_name == 'push' || pull_request.head.repo.full_name == github.repository` so only same-repo PRs and pushes reach the self-hosted runner; fork PRs run only the sandboxed ubuntu-latest jobs. (integration-full was already push+main guarded.) - Pin actions/checkout@v4 to its commit SHA (v4.2.2) across all jobs. - Add top-level `permissions: contents: read` (the workflow only reads the repo). - Correct the setup-ci-runner.sh header comment, which claimed the sudoers was scoped to pct/qm/pvesm/pveam only; it also grants bash. Document that the fork guard is what makes that acceptable.
The per-server MCP bearer token was generated, written to the env file, and advertised to clients as `Authorization: Bearer ...`, but mcp-proxy has no server-side auth, so nothing enforced it: the 9 SSE endpoints (admin-reach tools like Cortex block-IP, Sophos isolate-endpoint, TheHive mutation) accepted any request. SECURITY.md and the READMEs implied the token guarded them, which induced the unsafe `--mcp-bind-host 0.0.0.0` config. - mcp/deploy.sh: mcp-proxy now binds 127.0.0.1 on an internal port; nginx owns the public port (3001-3009) and returns 401 unless the Authorization header is the exact bearer token. `nginx -t` gates the reload so a bad config fails the deploy instead of silently disabling auth. - mcp/verify.sh: assert nginx is enabled and its config valid. - assert-mcp.sh: assert every endpoint returns 401 without a token (proves enforcement; the accept path needs the unredacted token so it is not tested). - install.sh: warn (in logs + result JSON) when mcp binds a non-loopback host. Extracted as warn_if_mcp_exposed() with unit tests. - Docs updated to describe the enforced model accurately. NEEDS LIVE SMOKE TEST: the nginx gateway runs inside the MCP LXC and cannot be exercised by the unit suite or (fork-gated) CI without a Proxmox host. Deploy the mcp component once and confirm `curl :3001/sse` returns 401 and a request with the token from /root/mcp-clients.json succeeds before relying on it.
- thehive-cortex/deploy.sh: Cortex admin password hash now uses a full-length random salt (openssl rand -hex 16) with no fixed "s3-" prefix. The old 24-bit prefixed salt lowered offline cracking cost if the ES admin doc leaked. - mcp/integrate.sh: parse rapid7.env/sophos.env as KEY=value data instead of sourcing them, which executed arbitrary shell as root. - CHANGELOG: document the hardening pass (Security/Removed/Fixed).
misp-core and misp-modules ran from floating :latest tags, so a re-pushed tag could silently change what runs on the next deploy with no version record. Pin both to the digest :latest resolves to today; bump deliberately. (mariadb, redis, and the TheHive/Cortex images were already pinned.) The dashboards git clones are intentionally left alone: bro-hunter was renamed (solomonneas/bro-hunter -> lidless-labs/vervet) and playbook-forge no longer resolves at the referenced URL, so those need canonical URLs confirmed before pinning, not a blind SHA.
The dashboards component cloned stale URLs: solomonneas/bro-hunter was renamed to lidless-labs/vervet (only still worked via GitHub's rename redirect), and solomonneas/playbook-forge no longer resolved at all, so that clone was failing. - bro-hunter -> lidless-labs/vervet (confirmed rename; same web/ Vite layout). - playbook-forge -> lidless-labs/hotwash (its successor: a visual IR-playbook builder with the same standalone web/ Vite app). Commented VERIFY since this is an inferred rename, not a redirect. - Pin both to a commit and switch the re-run path from `git pull` to `git fetch` + `git checkout --detach <ref>`, matching how the mcp component pins its sources (a compromised upstream cannot change what gets built).
Fill the coverage gaps the audit flagged (134 unit tests total): - lib/lxc.sh: lxc_stop idempotency, lxc_destroy (absent / running / stopped), and lxc_push_script, via the pct mock. - components/wazuh/destroy.sh: the per-component teardown contract (no state -> no-op; VMID present -> stop+destroy+remove state; no VMID -> remove state only). Representative of all six destroy scripts. - deploy_one records status=failed and returns non-zero when static IP allocation overflows the host range. - emit_final_json keeps mcp endpoint tokens raw under --include-secrets-json and redacts them by default.
Testing the repointed sources locally surfaced that vervet and hotwash now have different build layouts, so the single `cd web && npm install && vite build` pattern was still wrong for one of them: - vervet (bro-hunter) keeps package.json at the repo ROOT with the Vite app under web/ (root: web/, outDir: dist). `cd web && npm install` fails there (no web/package.json). Build from root: npm install + vite build --config web/vite.config.ts, which still emits web/dist. - hotwash (playbook-forge) keeps package.json under web/, so it builds from web/. Both verified locally against the pinned commits: each produces web/dist/index.html, and `vite preview` from web/ serves it (200). The systemd/nginx serve path is unchanged (still web/dist via vite preview). Confirms hotwash IS the Playbook Forge successor (its build even emits a flowchart chunk).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security and correctness hardening pass
A read-only audit of the installer (which runs as root and provisions LXCs) turned up real weaknesses the existing tests did not catch. This branch fixes them. The baseline was already green; every change here is gated through
./scripts/verifyand covered by new unit tests where testable. Unit tests went from 105 to 123.Security
mcp-proxyhas no server-side auth. The 9 SSE endpoints (admin-reach tools) accepted unauthenticated requests while the docs implied otherwise. Each endpoint is now fronted by an nginx gateway that returns401unless theAuthorizationheader is the exact token;mcp-proxybinds loopback behind it. Docs corrected to match; installer warns on a non-loopback bind.integration-componentran on the self-hosted Proxmox runner for every PR with no fork guard, and the runner account has broad host sudo. Added anif:so only same-repo PRs and pushes reach it; pinnedactions/checkoutto a commit SHA; addedpermissions: contents: read.scripts/setup/**). It was excluded from the shellcheck gate and carried TLS-off alert forwarding, an installer fetched from a mutable branch and run as root, cleartext password printing, and an allow-all firewall. Nothing live referenced it./proc): the LXC root password is set viachpasswdover stdin, and the TheHive/Cortex API calls usecurl --data @-, matching the pattern MISP already used.rapid7.env/sophos.envas data instead of sourcing them as shell.Correctness
gen_passwordreturned exit 141 on success underset -o pipefail(atr | headSIGPIPE), a latent abort for anyset -ecaller. Reproduced and fixed.state_setoverwrote a good state file with empty content whenjqfailed, breaking idempotency. It now only replaces on success, tolerates a corrupt file instead of aborting, and writes the temp file in the target directory for an atomic rename.--ip-mode staticset no gateway, so static containers had no default route and failed the network wait. Added a--gatewayflag (defaults to the first host of the range).--vlanwas validated and stored but never applied to the container network config.status=deployedauthoritatively, so it is not re-deployed on every re-run.--flag=valueis accepted;validate_manifestexact-matches component names.Testing
./scripts/verifypasses (134 unit tests, shellcheck clean, manifests valid). New coverage fills gaps the audit flagged: the destroy path (lxc_destroy/lxc_stop/lxc_push_scriptand a componentdestroy.sh), thedeploy_onestatic-IP failure branch, and--include-secrets-jsonredact-vs-emit.Validation done (off-host), plus one remaining live check
Both items that needed verification were tested locally:
nginx -tclean; noAuthorizationheader →401; wrong token →401; correct token →200proxied to the backend. The bearer token is genuinely enforced now.hotwashis the Playbook Forge successor (its build even emits a flowchart chunk). Testing surfaced thatvervetandhotwashnow have different build layouts (vervet'spackage.jsonmoved to the repo root), so the singlecd web && npm install && vite buildpattern was still wrong for vervet. The build commands were corrected and re-validated against the pinned commits: both produceweb/dist/index.htmlandvite previewserves them (HTTP 200).Remaining live check (needs a Proxmox host): the in-LXC deploy plumbing can't be exercised off-host — that
mcp-proxybinds127.0.0.1:<port+10000>behind nginx and its systemd unit starts, and that the dashboardsvite previewunits come up. One-shot smoke test after deployingmcpanddashboards: