fix nginx security headers not being applied to https responses

nginx add_header inheritance meant X-Content-Type-Options and X-Frame-Options were silently dropped from all https server blocks. moved all security headers into a shared snippet (security-headers.inc) included per server block. added pytest-based header verification that auto-discovers sites from conf files.
2026-03-20 15:11:56 -04:00
parent 3b41653c04
commit cc07a34489
13 changed files with 622 additions and 11 deletions
@@ -0,0 +1,538 @@
+# Security Analysis
+
+Vulnerability assessment of the hantim-server infrastructure. Each finding
+includes severity, affected files, explanation, and a proposed fix. Sorted
+from highest to lowest severity.
+
+---
+
+## HIGH
+
+### 1. Security headers silently dropped on all HTTPS responses
+
+**Affected:** `docker/nginx/nginx.conf`, all `conf.d/*.conf`
+
+The `http` block in `nginx.conf` defines:
+
+```nginx
+add_header X-Content-Type-Options nosniff;
+add_header X-Frame-Options DENY;
+```
+
+But nginx's `add_header` inheritance rule means these are **dropped** from
+any block that defines its own `add_header`. Every HTTPS server block adds
+`Strict-Transport-Security`, which causes the parent headers to stop being
+inherited. The result: all site responses have HSTS but **no**
+`X-Content-Type-Options` and **no** `X-Frame-Options`.
+
+This is a well-documented nginx gotcha. Sites are currently vulnerable to
+clickjacking and MIME-type sniffing attacks.
+
+**Fix:** Add all security headers in every server block that uses
+`add_header`, or move them into a shared snippet. The simplest approach is
+a snippet file included from each HTTPS server block:
+
+```nginx
+# docker/nginx/conf.d/security-headers.inc
+add_header Strict-Transport-Security "max-age=63072000; preload" always;
+add_header X-Content-Type-Options "nosniff" always;
+add_header X-Frame-Options "DENY" always;
+add_header Referrer-Policy "strict-origin-when-cross-origin" always;
+add_header Permissions-Policy "geolocation=(), microphone=(), camera=()" always;
+```
+
+Then in each HTTPS server block, replace the standalone HSTS line with:
+
+```nginx
+include /etc/nginx/conf.d/security-headers.inc;
+```
+
+And remove the (now-dead) `add_header` lines from `nginx.conf`'s `http`
+block.
+
+---
+
+### 2. Garage admin API exposed to the internet with only token auth
+
+**Affected:** `docker/nginx/conf.d/garage.hantim.net.conf`
+
+The Garage admin API (port 3903) is proxied through nginx to the public
+internet at `garage.hantim.net`. The only protection is the Bearer token.
+No IP restriction, no rate limiting, no fail2ban. Anyone who obtains or
+brute-forces the admin token can create/delete buckets, manage access keys,
+and read/write all stored data.
+
+**Fix:** Restrict access by source IP. Only the developer machine and
+argento need access:
+
+```nginx
+location / {
+    allow <your-ip>/32;
+    allow <argento-ip>/32;
+    deny all;
+
+    proxy_pass http://garage:3903;
+}
+```
+
+If IP restriction isn't practical, add rate limiting at minimum:
+
+```nginx
+limit_req_zone $binary_remote_addr zone=admin:1m rate=5r/s;
+
+location / {
+    limit_req zone=admin burst=10 nodelay;
+    proxy_pass http://garage:3903;
+}
+```
+
+---
+
+### 3. Docker group membership gives deploy user root-equivalent access
+
+**Affected:** `scripts/bootstrap.sh:98`
+
+```bash
+usermod -aG docker deploy
+```
+
+Any member of the `docker` group can run
+`docker run -v /:/host alpine chroot /host` and get full root access. This
+undermines the SSH `command=` restriction and the scoped sudoers entry.
+
+The docker group is unnecessary here: the SSH `authorized_keys` entry runs
+`sudo /opt/hantim/scripts/deploy.sh`, so deploy.sh already executes as
+root. Docker commands within it don't need the deploy user in the docker
+group.
+
+**Fix:** Remove the line. The sudoers entry already covers the deploy
+flow:
+
+```bash
+# Remove this line from bootstrap.sh:
+# usermod -aG docker deploy
+```
+
+If the user is already in the group on an existing server:
+
+```bash
+gpasswd -d deploy docker
+```
+
+---
+
+### 4. All secrets fetched on every deploy (bws secret list in a loop)
+
+**Affected:** `scripts/deploy.sh:62`, `tools/app.sh:98`
+
+`deploy.sh` calls `bws secret list` (which returns **every** secret for
+the machine account) once per line in `.env.keys`:
+
+```bash
+while IFS= read -r line; do
+    ...
+    value=$(bws secret list | jq -r --arg key "$secret_key" ...)
+done < "docker/$APP/.env.keys"
+```
+
+For garage with 3 secrets, this fetches the full secret list 3 times. All
+secrets are materialized in a pipe buffer each time. A core dump, `/proc`
+read, or process trace during this window exposes everything.
+
+**Fix:** Fetch the list once, filter locally:
+
+```bash
+ALL_SECRETS=$(bws secret list)
+while IFS= read -r line || [ -n "$line" ]; do
+    [ -z "$line" ] && continue
+    var_name="${line%%=*}"
+    secret_key="${line#*=}"
+    value=$(echo "$ALL_SECRETS" | jq -r --arg key "$secret_key" \
+        '.[] | select(.key == $key) | .value')
+    if [ -z "$value" ]; then
+        echo "ERROR: Secret '$secret_key' not found in bws."
+        exit 1
+    fi
+    env_content="${env_content}${var_name}=${value}"$'\n'
+done < "docker/$APP/.env.keys"
+unset ALL_SECRETS
+```
+
+Even better, use `bws secret get <uuid>` to fetch individual secrets by ID
+instead of listing all.
+
+---
+
+### 5. Garage RPC port open to the internet
+
+**Affected:** `docker/garage/compose.yml:12`, `scripts/configure.sh:20`
+
+Port 3901 is bound to the host and opened in the firewall. This is needed
+for cluster replication with argento, but it means anyone on the internet
+can attempt RPC connections. The only protection is the `rpc_secret`.
+
+**Fix:** Restrict the firewall rule to argento's IP only:
+
+```bash
+# In configure.sh, replace:
+firewall-cmd --permanent --add-port=3901/tcp
+
+# With:
+firewall-cmd --permanent --add-rich-rule='rule family="ipv4" source address="<argento-ip>/32" port port="3901" protocol="tcp" accept'
+```
+
+---
+
+## MEDIUM
+
+### 6. No SSL/TLS hardening in nginx
+
+**Affected:** `docker/nginx/nginx.conf`, all `conf.d/*.conf`
+
+No `ssl_protocols`, `ssl_ciphers`, `ssl_session_cache`, or OCSP stapling
+is configured. The nginx:alpine image's defaults may allow TLS 1.0/1.1 and
+weak cipher suites depending on the version.
+
+Certbot writes `options-ssl-nginx.conf` to `/etc/letsencrypt/` (which is
+mounted into the container), but no conf file includes it.
+
+**Fix:** Add TLS hardening to `nginx.conf` inside the `http` block:
+
+```nginx
+ssl_protocols TLSv1.2 TLSv1.3;
+ssl_ciphers ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384;
+ssl_prefer_server_ciphers off;
+ssl_session_cache shared:SSL:10m;
+ssl_session_timeout 1d;
+ssl_session_tickets off;
+ssl_stapling on;
+ssl_stapling_verify on;
+resolver 1.1.1.1 8.8.8.8 valid=300s;
+resolver_timeout 5s;
+```
+
+Or include certbot's config:
+
+```nginx
+include /etc/letsencrypt/options-ssl-nginx.conf;
+ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem;
+```
+
+---
+
+### 7. No default server block to catch unknown Host headers
+
+**Affected:** `docker/nginx/conf.d/`
+
+Without a catch-all server block, requests with unknown `Host` headers are
+served by whichever conf file nginx loads first (alphabetically). This
+leaks which sites are hosted and can be used for reconnaissance.
+
+**Fix:** Add a default server block:
+
+```nginx
+# docker/nginx/conf.d/00-default.conf
+server {
+    listen 80 default_server;
+    listen [::]:80 default_server;
+    listen 443 ssl default_server;
+    listen [::]:443 ssl default_server;
+    http2 on;
+    server_name _;
+
+    ssl_certificate     /etc/letsencrypt/live/hantim.net/fullchain.pem;
+    ssl_certificate_key /etc/letsencrypt/live/hantim.net/privkey.pem;
+
+    return 444;
+}
+```
+
+The `444` status silently drops the connection.
+
+---
+
+### 8. No rate limiting on any endpoint
+
+**Affected:** `docker/nginx/nginx.conf`, all `conf.d/*.conf`
+
+No `limit_req` or `limit_conn` zones are defined. The server is vulnerable
+to brute-force attacks against the S3 and admin APIs, and to connection
+exhaustion / slowloris attacks.
+
+**Fix:** Add rate limiting zones to `nginx.conf`:
+
+```nginx
+http {
+    limit_req_zone $binary_remote_addr zone=general:10m rate=10r/s;
+    limit_req_zone $binary_remote_addr zone=api:10m rate=2r/s;
+    ...
+}
+```
+
+Apply them in server blocks:
+
+```nginx
+# In s3.hantim.net.conf and garage.hantim.net.conf:
+limit_req zone=api burst=20 nodelay;
+
+# In site server blocks:
+limit_req zone=general burst=30 nodelay;
+```
+
+---
+
+### 9. envsubst replaces all environment variables in garage.toml
+
+**Affected:** `docker/garage/Dockerfile:8`
+
+```dockerfile
+CMD envsubst < /etc/garage.toml.tpl > /etc/garage.toml && exec /garage server
+```
+
+`envsubst` without arguments replaces every `$VAR` or `${VAR}` pattern. If
+any other environment variable (e.g., `$HOME`, `$PATH`) matches a pattern
+in the template, it will be silently substituted. This is fragile and could
+break the config in unexpected ways.
+
+**Fix:** Specify the variables explicitly:
+
+```dockerfile
+CMD envsubst '${RPC_SECRET} ${ARGENTO_NODE_ID} ${ADMIN_TOKEN}' < /etc/garage.toml.tpl > /etc/garage.toml && exec /garage server
+```
+
+---
+
+### 10. StrictHostKeyChecking=accept-new trusts first connection blindly
+
+**Affected:** `.gitea/workflows/*.yml`, `tools/app.sh:367`
+
+All SSH connections use `-o StrictHostKeyChecking=accept-new`, which trusts
+whatever host key is presented on first contact. A man-in-the-middle during
+the first CI run (or after server reprovisioning) could intercept the
+deploy connection and capture the SSH private key.
+
+**Fix:** Pin the server's host key in workflows. Add a step before SSH:
+
+```yaml
+- name: Configure SSH
+  run: |
+    mkdir -p ~/.ssh
+    echo "${{ secrets.DEPLOY_SSH_KEY }}" > ~/.ssh/deploy_key
+    chmod 600 ~/.ssh/deploy_key
+    echo "${{ vars.DEPLOY_HOST_KEY }}" >> ~/.ssh/known_hosts
+    ssh -o StrictHostKeyChecking=yes -i ~/.ssh/deploy_key deploy@${{ vars.DEPLOY_HOST }} deploy-nginx
+```
+
+Store the server's host public key as a Gitea variable (`DEPLOY_HOST_KEY`).
+
+---
+
+### 11. API tokens visible in process listings
+
+**Affected:** `tools/app.sh` (lines 134, 139, 225, etc.)
+
+Secrets passed as curl header arguments appear in `ps aux` and
+`/proc/<pid>/cmdline` for the duration of the curl call:
+
+```bash
+VULTR_AUTH="Authorization: Bearer $VULTR_API_KEY"
+curl -s "$VULTR_API/domains/$APP" -H "$VULTR_AUTH"
+```
+
+**Fix:** Use curl's `--header @-` or `-H @<file>` to read headers from
+stdin/file instead of command arguments:
+
+```bash
+VULTR_AUTH_FILE=$(mktemp)
+echo "Authorization: Bearer $VULTR_API_KEY" > "$VULTR_AUTH_FILE"
+chmod 600 "$VULTR_AUTH_FILE"
+trap "rm -f $VULTR_AUTH_FILE" EXIT
+curl -s "$VULTR_API/domains/$APP" -H "@$VULTR_AUTH_FILE"
+```
+
+Or use a `.netrc` file for authentication.
+
+---
+
+### 12. No container resource limits
+
+**Affected:** All `docker/*/compose.yml`
+
+No memory or CPU limits are set on any container. A runaway container
+(memory leak, fork bomb, or compromised process) can consume all host
+resources and take down every service.
+
+**Fix:** Add resource limits to compose files:
+
+```yaml
+services:
+  app:
+    ...
+    deploy:
+      resources:
+        limits:
+          memory: 128M
+          cpus: '0.5'
+```
+
+For nginx and garage, use higher limits appropriate to their workload.
+
+---
+
+### 13. Brief nginx downtime during initial cert issuance
+
+**Affected:** `scripts/deploy.sh:87-114`
+
+When deploying nginx with new domains that need certs, the script stops
+nginx, runs `certbot --standalone`, then starts nginx. During this window
+(potentially minutes if multiple certs are needed), all sites are down.
+
+**Fix:** Issue certs via the `cert-<domain>` SSH command (which uses
+`--webroot` and causes zero downtime) before deploying the nginx config.
+The `app.sh` already does this (`cmd_cert` runs before nginx gets the new
+config). For the standalone path, consider using certbot's `--preferred-challenges dns`
+or pre-issuing certs in a separate step.
+
+---
+
+### 14. Hardcoded Bitwarden secret UUIDs
+
+**Affected:** `scripts/bootstrap.sh:21-22`
+
+```bash
+SECRET_REGISTRY_TOKEN="43f4a77d-7bb4-49ad-8044-b411016c5c6e"
+SECRET_DEPLOY_PUBKEY="945760dd-90e2-46e5-98ad-b411016e3f3b"
+```
+
+UUIDs identify specific secrets. If this repo is exposed, an attacker
+knows exactly which secret IDs to target. Using `bws secret list` with
+name lookup (like the rest of the codebase does) is more consistent and
+doesn't leak UUIDs.
+
+**Fix:** Use name-based lookup like `deploy.sh` and `app.sh` do:
+
+```bash
+REGISTRY_TOKEN=$(bws secret list | jq -r '.[] | select(.key == "hantim-ci-registry-push") | .value')
+DEPLOY_PUBKEY=$(bws secret list | jq -r '.[] | select(.key == "hantim-deploy-ssh-public-key") | .value')
+```
+
+---
+
+### 15. certbot registers without email
+
+**Affected:** `scripts/deploy.sh:29, 109`
+
+```bash
+certbot certonly --standalone --non-interactive --agree-tos \
+    --register-unsafely-without-email ...
+```
+
+Without an email, Let's Encrypt cannot send expiration warnings. If the
+certbot renewal timer/cron breaks silently, certificates expire without
+notice, causing site outages.
+
+**Fix:** Register with an email:
+
+```bash
+certbot certonly --standalone --non-interactive --agree-tos \
+    -m admin@hantim.net ...
+```
+
+---
+
+## LOW
+
+### 16. Certbot renewal hook has no error handling
+
+**Affected:** `scripts/configure.sh:26-30`
+
+```bash
+cat > /etc/letsencrypt/renewal-hooks/deploy/reload-nginx.sh <<'HOOK'
+#!/bin/bash
+docker exec nginx nginx -s reload
+HOOK
+```
+
+If the nginx container is down when certs renew, the reload fails silently.
+The new cert won't take effect until nginx is manually restarted.
+
+**Fix:**
+
+```bash
+#!/bin/bash
+set -e
+if ! docker exec nginx nginx -s reload; then
+    logger -t certbot "ERROR: Failed to reload nginx after cert renewal"
+    exit 1
+fi
+```
+
+---
+
+### 17. No audit logging for deploy operations
+
+**Affected:** `scripts/deploy.sh`
+
+There is no logging of who triggered a deploy, what changed, or when
+secrets were accessed. If the server is compromised, there's no audit trail.
+
+**Fix:** Add logging to deploy.sh:
+
+```bash
+logger -t hantim-deploy "deploy started: cmd=$CMD user=$(whoami) ssh_client=${SSH_CLIENT:-local}"
+```
+
+And at the end:
+
+```bash
+logger -t hantim-deploy "deploy finished: cmd=$CMD"
+```
+
+---
+
+### 18. git pull without explicit branch
+
+**Affected:** `scripts/deploy.sh:39, 51`
+
+```bash
+git pull
+```
+
+If the repo's default tracking branch changes or is misconfigured, `git
+pull` could pull the wrong branch.
+
+**Fix:**
+
+```bash
+git pull origin main
+```
+
+---
+
+## Positive security practices already in place
+
+- SSH `command=` restriction limits deploy user to specific commands
+- Deploy user has `nologin` shell and scoped sudoers entry
+- All scripts use `set -euo pipefail`
+- HTTPS enforced everywhere with HTTP-to-HTTPS redirects
+- HSTS with preload on all main server blocks
+- `.env` files are `chmod 600` and gitignored
+- BWS token file is `chmod 600`
+- Docker network isolation: only nginx binds host ports
+- Input validation on domain names with regex in `deploy.sh` and `app.sh`
+- Idempotent scripts safe to re-run
+- Certbot auto-renewal configured via timer or cron
+- `nginx -t` config test before applying changes
+
+---
+
+## Recommended priority
+
+1. Fix security headers (item 1) — actual bug, every site affected now
+2. Restrict Garage admin API access (item 2)
+3. Remove deploy user from docker group (item 3)
+4. Add SSL/TLS hardening (item 6)
+5. Add default server block (item 7)
+6. Restrict Garage RPC firewall rule to argento IP (item 5)
+7. Fix bws secret list loop (item 4)
+8. Everything else as time permits