Skip to content

V 1.5.3 - #1

Closed
rdwr-rahulk wants to merge 2 commits into
mainfrom
beta
Closed

rdwr-rahulk wants to merge 2 commits into
mainfrom
beta

Conversation

@rdwr-rahulk

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical Compose/resource-limit and Docker GID portability issues, plus upgrade migration failures, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

V 1.5.3 hardens the watchdog container, adds resource limits, and configures Docker socket access for non-root execution.

Changes:

  • Adds non-root execution, reduced capabilities, and a read-only filesystem.
  • Applies memory, CPU, swap, and PID limits.
  • Adds Docker GID detection, log ownership handling, and deployment documentation.
File summaries
File Summary and findings
README.md Documents hardening and resource limits. Critical (3 votes): Update memswap_limit in both Compose files when increasing mem_limit (lines 96 and 100).
install.sh Configures log ownership and Docker GID. Moderate (3 votes): Migrate existing log-file ownership (lines 122 and 125). Moderate (1 vote): Migrate missing DOCKER_GID before the existing-config early return (line 378). Moderate (1 vote): Fail when the socket/GID cannot be detected or derive the configured socket path instead of defaulting to 999 (line 139).
Dockerfile Adds the non-root runtime user.
docker-compose.yaml Applies production hardening and limits. Critical (2 votes): Require and validate DOCKER_GID instead of defaulting to 999 (line 38).
docker-compose.build.yaml Applies equivalent build-stack settings. Moderate (1 vote): Require the detected DOCKER_GID instead of using the 999 fallback (line 32).
DEPLOYMENT.md Documents Docker GID setup. Nit (1 vote): Replace the copy-ready 999 value with a replaceable, host-specific value (line 122).
.env.example Adds the Docker GID setting. Moderate (1 vote): Use a replaceable placeholder and make Compose fail fast until the actual GID is configured (line 45).
Review details

Suppressed comments (8)

.env.example:45

  • The copy-ready example uses 999 as if it were a universal socket GID. Following the documented cp .env.example .env flow therefore breaks on hosts where stat -c '%g' /var/run/docker.sock returns another value, because this explicit value overrides Compose's fallback. Use a clearly replaceable placeholder and make the Compose configuration fail fast until the actual GID is set.
DOCKER_GID=999

DEPLOYMENT.md:122

  • The documented manual .env block presents 999 as a usable value even though the preceding text says the socket GID is host-specific. Copying this block verbatim breaks socket access on hosts with another GID; show a replace-me value and require the actual detected GID before starting.
DOCKER_GID=999

README.md:100

  • group_add grants the process access to the Docker Engine socket, which is effectively root-equivalent on the host. A compromised or runaway code path can use that API to create privileged containers or bind-mount host files, so cap_drop, no-new-privileges, and the cgroup limits do not contain it to this container as this paragraph claims. Please qualify the hardening scope or put the monitor behind a restricted Docker API proxy.
The container runs as a non-root user (UID 1000) with a read-only root filesystem, no extra Linux capabilities (`cap_drop: ALL`), and `no-new-privileges` set. Access to `/var/run/docker.sock` is granted by adding the container's user to the host's `docker` group via `group_add` — the GID is auto-detected by `install.sh` and stored as `DOCKER_GID` in `.env` (see [DEPLOYMENT.md](DEPLOYMENT.md#22-configure-environment-variables-env)). CPU and process count are also capped (`cpus: "0.50"`, `pids_limit: 200`) so a leak or runaway condition is contained to this container instead of the host.

docker-compose.build.yaml:32

  • This duplicate fallback has the same portability failure in the build/developer deployment: 999 may not be the group owning the host Docker socket, and the container then starts without API access. Keep this file consistent with the production Compose file by requiring the detected DOCKER_GID rather than choosing a silent default.
      - "${DOCKER_GID:-999}"   # host docker.sock group GID — auto-detected into .env by install.sh

docker-compose.yaml:38

  • Although the process is non-root, adding it to the host docker group grants unrestricted Docker API access; a read-only bind mount of the Unix socket, dropped capabilities, and no-new-privileges do not make that API read-only. A compromised watchdog can still create a privileged container or mount the host filesystem, so the claim that a runaway is contained to this container is not true for a compromise. Use a least-privilege Docker API proxy, or document this residual host-root-equivalent risk explicitly.
      - "${DOCKER_GID:-999}"   # host docker.sock group GID — auto-detected into .env by install.sh

install.sh:126

  • If the installer runs as a non-root user whose UID/GID is not 1000, this fallback leaves the directory owned by that user's group. Mode 775 only grants write access to the owner/group, while the container runs as UID/GID 1000 and only adds DOCKER_GID, so it may be unable to create watchdog.log and will fail to start. Either stop with an actionable ownership/privilege error or configure a shared group that includes the container user.
        print_warning "Could not chown ${LOG_DIR} to UID 1000 (not running as root?) — falling back to group-writable permissions"
        chmod 775 "$LOG_DIR"

install.sh:378

  • On an upgrade where .env or watchdog-config.yaml already exists, configure_all returns at lines 242–245 when the default N is accepted, so this new value is never written. Compose then falls back to GID 999; on hosts whose Docker socket has a different group, the non-root container cannot connect to Docker. Add a migration path for an existing .env that lacks DOCKER_GID before the early return.
        printf "# Docker socket access (non-root container)\nDOCKER_GID=%s\n\n" "$DOCKER_GID"

install.sh:141

  • If the socket is absent, or both stat forms fail, this silently writes 999 and continues. The prerequisite check only verifies that Docker responds through the caller's configured client, so a nonstandard/rootless setup can pass it while Compose still mounts only /var/run/docker.sock; installation then reports a detected GID but deployment cannot access Docker. Treat this as a prerequisite error or derive the configured socket path instead of guessing.
        stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock 2>/dev/null || echo "999"
    else
        echo "999"
  • Files reviewed: 7/7 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread README.md Outdated
| `watchdog.tar` export | ~170 MB |

> **Note:** The values above are approximate and may vary depending on the host operating system, Docker version, and installed dependencies.
> **Note:** The values above are approximate and may vary depending on the host operating system, Docker version, and installed dependencies. If usage approaches the 256 MB limit (check with `docker stats docker-container-watchdog`), raise `mem_limit`/`mem_reservation` in `docker-compose.yaml` rather than removing the limit.
Comment thread docker-compose.yaml Outdated
# ── Hardening ────────────────────────────────────────────────────────────
user: "1000:1000" # non-root
group_add:
- "${DOCKER_GID:-999}" # host docker.sock group GID — auto-detected into .env by install.sh
Comment thread install.sh Outdated
print_info "Log directory already exists: ${LOG_DIR}"
fi
# Container runs as non-root (UID 1000) — own it directly instead of opening it to all local users
if chown 1000:1000 "$LOG_DIR" 2>/dev/null; then
@rdwr-rahulk
rdwr-rahulk removed the request for review from rdwr-egore September 17, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants