diff --git a/Dockerfile b/Dockerfile index 41418c31..7a8317d9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -60,6 +60,7 @@ RUN echo "packages upgraded for ${APP_VERSION}" && \ COPY nginx/dashboard.conf /etc/nginx/http.d/dashboard.conf COPY nginx/security-headers.conf /etc/nginx/http.d/security-headers.conf COPY nginx/csp-default.conf /etc/nginx/http.d/csp-default.conf +COPY nginx/csp-api.conf /etc/nginx/http.d/csp-api.conf # Replaced at container start by docker-entrypoint.sh. Present so the config is # valid at build time. COPY nginx/realip.conf /etc/nginx/http.d/realip.conf @@ -68,11 +69,11 @@ COPY --from=assets /src/ui/ /usr/share/nginx/html/ # The image mirrors the repository layout. Shared modules keep the same relative # path in both places. -COPY --chown=node:node api/ /app/api/ -COPY --chown=node:node ui/js/link-url.js /app/ui/js/link-url.js -COPY --chown=node:node ui/js/limits.js /app/ui/js/limits.js -COPY --chown=node:node ui/js/config-recovery-logic.js /app/ui/js/config-recovery-logic.js -COPY --chown=node:node ui/js/host-names.js /app/ui/js/host-names.js +COPY api/ /app/api/ +COPY ui/js/link-url.js /app/ui/js/link-url.js +COPY ui/js/limits.js /app/ui/js/limits.js +COPY ui/js/config-recovery-logic.js /app/ui/js/config-recovery-logic.js +COPY ui/js/host-names.js /app/ui/js/host-names.js COPY scripts/exit-on-fatal.py /app/scripts/exit-on-fatal.py # Fails the build if python3 is no longer present. Without it the event listener # cannot start and no failure is reported. diff --git a/api/test/link-url.test.js b/api/test/link-url.test.js index 16f8d4d4..f927b672 100644 --- a/api/test/link-url.test.js +++ b/api/test/link-url.test.js @@ -114,8 +114,8 @@ test('the Dockerfile puts the shared rule where the server can require it', () = const dockerfile = fs.readFileSync(path.join(__dirname, '../../Dockerfile'), 'utf8'); /* The image mirrors the repository layout, so the same relative path resolves in both. If either line changes without the other, the API cannot start. */ - assert.match(dockerfile, /COPY --chown=node:node api\/ \/app\/api\//); - assert.match(dockerfile, /COPY --chown=node:node ui\/js\/link-url\.js \/app\/ui\/js\/link-url\.js/); + assert.match(dockerfile, /^COPY api\/ \/app\/api\/$/m); + assert.match(dockerfile, /^COPY ui\/js\/link-url\.js \/app\/ui\/js\/link-url\.js$/m); const supervisord = fs.readFileSync(path.join(__dirname, '../../supervisord.conf'), 'utf8'); assert.match(supervisord, /command=node \/app\/api\/src\/server\.js/); }); diff --git a/api/test/nginx-config.test.js b/api/test/nginx-config.test.js index ad61d524..8f913bdc 100644 --- a/api/test/nginx-config.test.js +++ b/api/test/nginx-config.test.js @@ -462,3 +462,23 @@ test('the image stamps assets itself, so a locally built image is not pinned to 'the web root must come from the stamped stage, not straight from the build context', ); }); + +/* ── the API policy ───────────────────────────────────────────────────────── */ + +/* /api/ sets its own headers, so the server-level policy does not reach it. The + icon proxy serves third-party SVG on this origin. */ +test('every /api/ location sends the locked-down API policy', () => { + const apiPolicy = (read('csp-api.conf').match(/add_header Content-Security-Policy "([^"]+)"/) || [])[1]; + assert.ok(apiPolicy, 'csp-api.conf has no policy'); + for (const directive of ["default-src 'none'", "frame-ancestors 'none'", 'sandbox']) { + assert.ok(apiPolicy.includes(directive), `the API policy lacks ${directive}`); + } + + const locations = [...dashboard.matchAll(/location (?:= )?(\/api\/\S*) \{/g)]; + assert.equal(locations.length, 3, 'an /api/ location was added or removed; check it sends the API policy'); + for (const m of locations) { + const block = dashboard.slice(m.index, dashboard.indexOf('\n }', m.index)); + assert.ok(block.includes('include /etc/nginx/http.d/csp-api.conf;'), `${m[1]} sends no API policy`); + assert.ok(!block.includes(INCLUDE), `${m[1]} also sends the page policy`); + } +}); diff --git a/docker-compose.yml b/docker-compose.yml index eb97d7db..e50dc2ce 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -13,6 +13,7 @@ services: - SETUID - SETGID - DAC_OVERRIDE + - KILL security_opt: - no-new-privileges:true privileged: false diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index e07778e5..4c78fb2f 100644 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -77,7 +77,7 @@ nginx -t -q # marker carries that failure out as the container's exit code, so a dead API # reads as a failure rather than a normal shutdown and `restart: on-failure` # behaves the same as `unless-stopped` here. -MARKER="${SUPERVISOR_FATAL_MARKER:-/tmp/stackyard-fatal}" +MARKER="${SUPERVISOR_FATAL_MARKER:-/run/stackyard-fatal}" rm -f "$MARKER" # Run in the background rather than exec, so this script survives to read the @@ -88,13 +88,14 @@ rm -f "$MARKER" child=$! trap 'kill -TERM "$child" 2>/dev/null' TERM INT -# `wait` returns early when a signal is handled, so wait again for the real exit. -wait "$child" -status=$? -if [ "$status" -gt 128 ]; then - wait "$child" 2>/dev/null - status=$? -fi +# Keep `|| status=$?`. A trapped signal makes `wait` return 143, and set -e +# would exit here: the kernel then kills supervisord mid-shutdown. +status=0 +wait "$child" || status=$? +while kill -0 "$child" 2>/dev/null; do + status=0 + wait "$child" || status=$? +done if [ -f "$MARKER" ]; then echo "stackyard: $(cat "$MARKER") could not be started; exiting so the container is restarted" >&2 diff --git a/nginx/csp-api.conf b/nginx/csp-api.conf new file mode 100644 index 00000000..81905dc4 --- /dev/null +++ b/nginx/csp-api.conf @@ -0,0 +1,3 @@ +# Policy for every /api/ location. Do not loosen it. A proxied SVG icon opened +# directly would run script on this origin. +add_header Content-Security-Policy "default-src 'none'; frame-ancestors 'none'; sandbox" always; diff --git a/nginx/dashboard.conf b/nginx/dashboard.conf index 1888c3ec..4138747d 100644 --- a/nginx/dashboard.conf +++ b/nginx/dashboard.conf @@ -162,6 +162,7 @@ server { proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; include /etc/nginx/http.d/security-headers.conf; + include /etc/nginx/http.d/csp-api.conf; add_header Cache-Control "no-store" always; proxy_read_timeout 60s; proxy_connect_timeout 5s; @@ -179,6 +180,7 @@ server { proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; include /etc/nginx/http.d/security-headers.conf; + include /etc/nginx/http.d/csp-api.conf; add_header Cache-Control "no-store" always; proxy_read_timeout 60s; proxy_connect_timeout 5s; @@ -199,6 +201,7 @@ server { # $scheme is always http here, and setting it would overwrite the https a # front proxy sent and drop the Secure flag from the session cookie. include /etc/nginx/http.d/security-headers.conf; + include /etc/nginx/http.d/csp-api.conf; add_header Cache-Control "no-store" always; # Give up after 15s and return 504 if Node hangs. proxy_read_timeout 15s; diff --git a/nginx/security-headers.conf b/nginx/security-headers.conf index 57eea175..f7a45e87 100644 --- a/nginx/security-headers.conf +++ b/nginx/security-headers.conf @@ -1,7 +1,7 @@ # Server-level security headers. nginx add_header does not inherit into a # location that sets any add_header of its own, so every such location includes # this file. X-Frame-Options is set per location. Content-Security-Policy comes -# from csp-default.conf, except in /admin and /widgets/. +# from csp-default.conf, except in /admin, /widgets/ and /api/ (csp-api.conf). add_header X-Content-Type-Options "nosniff" always; add_header Referrer-Policy "strict-origin-when-cross-origin" always; add_header Permissions-Policy "camera=(), microphone=(), geolocation=()" always; diff --git a/scripts/exit-on-fatal.py b/scripts/exit-on-fatal.py index 5b9caeef..8843540f 100644 --- a/scripts/exit-on-fatal.py +++ b/scripts/exit-on-fatal.py @@ -24,7 +24,7 @@ import sys # Read by docker-entrypoint.sh after supervisord exits. -FAILURE_MARKER = os.environ.get('SUPERVISOR_FATAL_MARKER', '/tmp/stackyard-fatal') +FAILURE_MARKER = os.environ.get('SUPERVISOR_FATAL_MARKER', '/run/stackyard-fatal') def write_stdout(s): diff --git a/supervisord.conf b/supervisord.conf index f71ada8b..494b295f 100644 --- a/supervisord.conf +++ b/supervisord.conf @@ -15,7 +15,7 @@ logfile_backups=3 ; this repo prints itself, from scripts/exit-on-fatal.py and ; docker-entrypoint.sh. Do not restore info without re-reading that trade. loglevel=warn -pidfile=/tmp/supervisord.pid +pidfile=/run/supervisord.pid [program:nginx] command=/usr/sbin/nginx -g "daemon off;" diff --git a/templates/stackyard.xml b/templates/stackyard.xml index b1ecccc7..67713ba2 100644 --- a/templates/stackyard.xml +++ b/templates/stackyard.xml @@ -5,6 +5,7 @@ https://ghcr.io bridge false + --init --cap-drop=ALL --cap-add=NET_BIND_SERVICE --cap-add=CHOWN --cap-add=SETUID --cap-add=SETGID --cap-add=DAC_OVERRIDE --cap-add=KILL --security-opt=no-new-privileges:true --pids-limit=256 --memory=384m --memory-reservation=96m --memory-swap=384m --cpus=0.5 https://github.com/SandObserver/Stackyard/issues https://github.com/SandObserver/Stackyard https://github.com/SandObserver/Stackyard/blob/main/README.md @@ -79,4 +80,5 @@ true + --max-old-space-size=192 diff --git a/test/repo/ca-template.test.js b/test/repo/ca-template.test.js index 1ffe9734..a27a13f8 100644 --- a/test/repo/ca-template.test.js +++ b/test/repo/ca-template.test.js @@ -118,3 +118,41 @@ test('the donation link is the canonical form', () => { assert.ok(tag(xml, 'DonateText')); } }); + +/* An Unraid install gets no other hardening: the Compose file does not apply. */ +test('the template applies the Compose hardening', () => { + const compose = fs.readFileSync(path.join(ROOT, 'docker-compose.yml'), 'utf8'); + const value = key => { + const m = new RegExp(`^\\s+${key}:\\s*"?([^"\\s]+)"?$`, 'm').exec(compose); + assert.ok(m, `docker-compose.yml has no ${key}`); + return m[1]; + }; + const list = key => { + const m = new RegExp(`^\\s+${key}:\\n((?:\\s+- .+\\n)+)`, 'm').exec(compose); + assert.ok(m, `docker-compose.yml has no ${key}`); + return [...m[1].matchAll(/- (\S+)/g)].map(i => i[1]); + }; + + const expected = [ + ...(value('init') === 'true' ? ['--init'] : []), + ...list('cap_drop').map(c => `--cap-drop=${c}`), + ...list('cap_add').map(c => `--cap-add=${c}`), + ...list('security_opt').map(o => `--security-opt=${o}`), + `--pids-limit=${value('pids_limit')}`, + `--memory=${value('mem_limit')}`, + `--memory-reservation=${value('mem_reservation')}`, + `--memory-swap=${value('memswap_limit')}`, + `--cpus=${value('cpus')}`, + ]; + const params = (tag(template, 'ExtraParams') || '').split(/\s+/); + assert.deepEqual(params.toSorted(), expected.toSorted()); + + /* Node sizes its heap from the host, not --memory. Without the cap the kernel + kills the API instead of it collecting. */ + const heap = /NODE_OPTIONS=\$\{NODE_OPTIONS:-([^}]+)\}/.exec(compose); + assert.ok(heap, 'docker-compose.yml has no NODE_OPTIONS default'); + const config = /]*Target="NODE_OPTIONS"[^>]*Default="([^"]+)"[^>]*>([^<]*)<\/Config>/.exec(template); + assert.ok(config, 'the template sets a memory limit but no heap cap'); + assert.equal(config[1], heap[1]); + assert.equal(config[2], heap[1]); +}); diff --git a/test/repo/compose-settings.test.js b/test/repo/compose-settings.test.js index 224e168b..7a46ab60 100644 --- a/test/repo/compose-settings.test.js +++ b/test/repo/compose-settings.test.js @@ -70,3 +70,9 @@ test('the Compose hardening is in place', () => { assert.ok(!/^\s*user:/m.test(compose), 'the Compose file now sets a user'); assert.match(read('supervisord.conf'), /^user=node$/m, 'the API should run as node'); }); + +/* supervisord runs as root and the API as node. Without KILL, root cannot signal + another user's process, so a stop never reaches the API. */ +test('supervisord can stop the API under the Compose capabilities', () => { + assert.match(compose, /cap_add:\s*\n(?:\s*- \w+\n)*\s*- KILL\n/); +}); diff --git a/test/repo/entrypoint-shutdown.test.js b/test/repo/entrypoint-shutdown.test.js new file mode 100644 index 00000000..c7e62bd0 --- /dev/null +++ b/test/repo/entrypoint-shutdown.test.js @@ -0,0 +1,110 @@ +/* `docker stop` sends SIGTERM to the entrypoint, which forwards it to + supervisord. The entrypoint must outlive supervisord's shutdown: once PID 1 + exits, the kernel kills every process left in the container. + + Runs the real entrypoint with a stand-in nginx and a stand-in supervisord + that takes a moment to shut down. */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { spawn } = require('node:child_process'); + +const { tmpDir } = require('../../api/test-support/tmp'); + +const ENTRYPOINT = path.join(__dirname, '../../docker-entrypoint.sh'); + +const stub = (dir, name, body) => { + const p = path.join(dir, name); + fs.writeFileSync(p, `#!/bin/sh\n${body}\n`); + fs.chmodSync(p, 0o755); + return p; +}; + +const waitFor = async (check, ms = 5000) => { + const end = Date.now() + ms; + while (!check()) { + if (Date.now() > end) throw new Error('timed out'); + await new Promise(r => setTimeout(r, 20)); + } +}; + +/* @returns {Promise<{ code: number|null, stopped: boolean, stderr: string }>} + `stopped` is whether the stand-in had finished its shutdown when the + entrypoint exited. */ +async function run({ child, signals = ['SIGTERM'], marker = false }) { + const dir = tmpDir('entrypoint-stop'); + const binDir = path.join(dir, 'bin'); + fs.mkdirSync(binDir); + stub(binDir, 'nginx', 'exit 0'); + const handoff = stub(binDir, 'handoff', child); + const started = path.join(dir, 'started'); + const done = path.join(dir, 'done'); + const fatal = path.join(dir, 'fatal'); + + const proc = spawn('sh', [ENTRYPOINT, handoff], { + env: { + PATH: `${binDir}:${process.env.PATH}`, + REALIP_CONF: path.join(dir, 'realip.conf'), + LISTEN_CONF: path.join(dir, 'listen-ipv6.inc'), + DATA_DIR: path.join(dir, 'no-data'), + ICONS_DIR: path.join(dir, 'no-icons'), + SUPERVISOR_FATAL_MARKER: fatal, + SY_STARTED: started, + SY_DONE: done, + SY_FATAL: marker ? fatal : '', + }, + stdio: ['ignore', 'ignore', 'pipe'], + }); + let stderr = ''; + proc.stderr.on('data', d => { + stderr += d; + }); + const exited = new Promise(resolve => proc.on('exit', status => resolve(status))); + + await waitFor(() => fs.existsSync(started)); + for (const [i, sig] of signals.entries()) { + if (i) await new Promise(r => setTimeout(r, 100)); + proc.kill(sig); + } + const code = await exited; + return { code, stopped: fs.existsSync(done), stderr }; +} + +/* Shuts down over 300 ms after SIGTERM, then exits 0 as supervisord does. */ +const SLOW_STOP = [ + 'trap \'sleep 0.3; [ -n "$SY_FATAL" ] && echo api > "$SY_FATAL"; touch "$SY_DONE"; exit 0\' TERM', + 'touch "$SY_STARTED"', + 'while :; do sleep 0.05; done', +].join('\n'); + +test('docker stop waits for supervisord to finish shutting down', async () => { + const r = await run({ child: SLOW_STOP }); + assert.ok(r.stopped, 'the entrypoint exited while supervisord was still shutting down'); + assert.equal(r.code, 0, 'a clean stop must exit with supervisord status'); +}); + +test('Ctrl-C in the foreground waits the same way', async () => { + const r = await run({ child: SLOW_STOP, signals: ['SIGINT'] }); + assert.ok(r.stopped); + assert.equal(r.code, 0); +}); + +test('a second signal during shutdown still waits', async () => { + const r = await run({ child: SLOW_STOP, signals: ['SIGTERM', 'SIGTERM'] }); + assert.ok(r.stopped, 'the entrypoint exited on the second signal'); + assert.equal(r.code, 0); +}); + +test('a program that could not be started still fails the container after a stop', async () => { + const r = await run({ child: SLOW_STOP, marker: true }); + assert.ok(r.stopped); + assert.equal(r.code, 1); + assert.match(r.stderr, /api could not be started/); +}); + +test('supervisord exiting on its own passes its status through', async () => { + const r = await run({ child: 'touch "$SY_STARTED"; sleep 0.1; exit 3', signals: [] }); + assert.equal(r.code, 3); +}); diff --git a/test/repo/image-hardening.test.js b/test/repo/image-hardening.test.js index 602458af..28d229b8 100644 --- a/test/repo/image-hardening.test.js +++ b/test/repo/image-hardening.test.js @@ -119,3 +119,12 @@ test('the upgrade cannot be served from a cache across releases', () => { const argAt = dockerfile.lastIndexOf('ARG APP_VERSION', dockerfile.indexOf(run)); assert.ok(argAt !== -1, 'APP_VERSION is read in the step but never declared above it'); }); + +/* The API runs as node. Code it owns, it can rewrite, and the change survives a + restart. */ +test('the API cannot rewrite its own code', () => { + const runtime = dockerfile.split(/^FROM /m).pop(); + const copies = runtime.split('\n').filter(l => /^COPY .*\/app\//.test(l)); + assert.ok(copies.length > 0); + for (const line of copies) assert.doesNotMatch(line, /--chown/, line); +}); diff --git a/test/repo/supervisord-fatal.test.js b/test/repo/supervisord-fatal.test.js index 7f12bc6f..248341e1 100644 --- a/test/repo/supervisord-fatal.test.js +++ b/test/repo/supervisord-fatal.test.js @@ -182,3 +182,13 @@ test('only the supervisor listener reaches the runtime image', () => { const copied = [...runtime.matchAll(/^COPY (?:--\S+ )*(scripts\/\S+)/gm)].map(m => m[1]); assert.deepEqual(copied, ['scripts/exit-on-fatal.py']); }); + +/* supervisord and the listener run as root, and the API user can write /tmp. A + link planted there would make root overwrite the file it points at. */ +test('root writes its marker and pid file outside world-writable /tmp', () => { + const marker = /SUPERVISOR_FATAL_MARKER',\s*'([^']+)'/.exec(listener)[1]; + const pidfile = /^pidfile=(\S+)$/m.exec(supervisord)[1]; + for (const file of [marker, pidfile]) { + assert.match(file, /^\/run\//, `${file} is not in a root-owned directory`); + } +});