Skip to content

Docker: run as non-root user#30878

Open
naltatis wants to merge 7 commits into
masterfrom
fix/docker-nonroot
Open

Docker: run as non-root user#30878
naltatis wants to merge 7 commits into
masterfrom
fix/docker-nonroot

Conversation

@naltatis

@naltatis naltatis commented Jun 15, 2026

Copy link
Copy Markdown
Member

Runs the evcc container process as a non-root user (default uid 1000) instead of root, limiting the impact of any in-container code execution. Existing installs keep working unchanged.

  • Entrypoint drops privileges via su-exec after re-chowning the database mount
  • Serial devices keep working: entrypoint auto-joins the group(s) owning mounted --device ports (ttyUSB/ACM/AMA, serial/by-id); su-exec runs uid-only so the groups survive, EVCC_DEVICE_GIDS overrides
  • Database path and /root/.evcc mount unchanged; auto-migrates on first start
  • PUID/PGID env (default 1000) to match host ownership
  • Home Assistant addon path unchanged (stays root)
  • Docs: Docker: document non-root user and PUID/PGID docs#1088

Real world testing wanted

Please try this change in the naltatis/evcc:non-root image (nightly 25.6.2026) on docker hub and give feedback.

Run the long-lived evcc process as a non-root user (default uid 1000) instead of root, reducing the blast radius of any in-container code execution.

The entrypoint starts as root only to repair ownership of the database mount, then drops privileges via su-exec. Existing installations keep working without manual steps: the /root/.evcc mount is re-owned on first start and the database path is unchanged. PUID/PGID env vars (default 1000) align the runtime user with host ownership. The Home Assistant addon path stays root.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • The unconditional chown -R on /root/.evcc (and on dirname "$EVCC_DATABASE_DSN") on every start may be expensive and could unintentionally recurse over large/shared directories; consider narrowing the path or only fixing ownership when it’s actually wrong.
  • With the new logic, even arbitrary commands passed as docker run ... evcc-image <cmd> now run under the non-root user; if you still want a way to drop into a root shell for debugging, you may want to special-case common shells or provide an explicit override.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The unconditional `chown -R` on `/root/.evcc` (and on `dirname "$EVCC_DATABASE_DSN"`) on every start may be expensive and could unintentionally recurse over large/shared directories; consider narrowing the path or only fixing ownership when it’s actually wrong.
- With the new logic, even arbitrary commands passed as `docker run ... evcc-image <cmd>` now run under the non-root user; if you still want a way to drop into a root shell for debugging, you may want to special-case common shells or provide an explicit override.

## Individual Comments

### Comment 1
<location path="packaging/docker/bin/entrypoint.sh" line_range="92-93" />
<code_context>
+	# user's home is set to /root so ~/.evcc/evcc.db keeps resolving to the
+	# existing mount (su-exec sets HOME from the passwd entry, not the env).
+	if [ "$(id -u)" = "0" ]; then
+		getent group evcc > /dev/null 2>&1 || addgroup -g "$PGID" evcc 2> /dev/null || addgroup evcc
+		getent passwd evcc > /dev/null 2>&1 || adduser -D -H -h /root -u "$PUID" -G evcc evcc 2> /dev/null || adduser -D -H -h /root -G evcc evcc
+
+		mkdir -p "$DATA_DIR"
</code_context>
<issue_to_address>
**issue:** The fallback addgroup/adduser behavior can ignore the requested PUID/PGID and lead to mismatched IDs.

If the requested PGID/UID are already in use under different names, `addgroup -g "$PGID" evcc` / `adduser ... -u "$PUID"` will fail, and the fallbacks will create a new user/group with different IDs. This breaks the expectation that PUID/PGID match the container user/group and the IDs used in `chown -R "$PUID:$PGID"`. It would be safer to fail loudly on this misconfiguration, or to look up and reuse existing user/group entries by UID/GID instead of creating new ones with mismatched IDs.
</issue_to_address>

### Comment 2
<location path="packaging/docker/bin/entrypoint.sh" line_range="95-100" />
<code_context>
+		mkdir -p "$DATA_DIR"
+		chown -R "$PUID:$PGID" "$DATA_DIR"
+		# fix a custom database directory if EVCC_DATABASE_DSN points elsewhere
+		[ -n "$EVCC_DATABASE_DSN" ] && chown -R "$PUID:$PGID" "$(dirname "$EVCC_DATABASE_DSN")" 2> /dev/null || true
+
+		RUNAS="su-exec $PUID:$PGID"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `chown` on `dirname "$EVCC_DATABASE_DSN"` can target unexpected paths and relies on subtle `&&`/`||` precedence.

1) If `EVCC_DATABASE_DSN` isn’t a plain filesystem path (e.g. `postgres://...` or `.`), `dirname` may resolve to an unexpected directory (including `.`), and `chown -R` will recursively change ownership there. Consider restricting this to known-safe locations (e.g. under `$DATA_DIR`) or validating that the resolved directory is within the intended mount.

2) The `[ -n ... ] && chown ... || true` pattern relies on `&&`/`||` precedence to behave correctly. An explicit `if [ -n "$EVCC_DATABASE_DSN" ]; then chown -R "$PUID:$PGID" "$(dirname "$EVCC_DATABASE_DSN")" || true; fi` is clearer and less error-prone if this line is modified later.

```suggestion
		mkdir -p "$DATA_DIR"
		chown -R "$PUID:$PGID" "$DATA_DIR"
		# fix a custom database directory if EVCC_DATABASE_DSN points to a path under $DATA_DIR
		if [ -n "$EVCC_DATABASE_DSN" ]; then
			dbdir="$(dirname "$EVCC_DATABASE_DSN")"
			case "$dbdir" in
				"$DATA_DIR" | "$DATA_DIR"/*)
					chown -R "$PUID:$PGID" "$dbdir" 2> /dev/null || true
					;;
			esac
		fi

		RUNAS="su-exec $PUID:$PGID"
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread packaging/docker/bin/entrypoint.sh Outdated
Comment thread packaging/docker/bin/entrypoint.sh Outdated
@andig

andig commented Jun 15, 2026

Copy link
Copy Markdown
Member

We‘ve tried this before. Should find old PR and cross-check before merging.

@naltatis

naltatis commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

Should find old PR and cross-check before merging.

I did.

see #4850, #4878 and #4859 (docker, not merged)
and #4901 for non-root introduction in Debian

@andig

andig commented Jun 15, 2026

Copy link
Copy Markdown
Member

@StefanSchoof

Copy link
Copy Markdown
Contributor

Will their a permissions issue when user mounts their /dev/ttyUSB0 in the container?

@andig andig added the infrastructure Basic functionality label Jun 15, 2026
@andig

andig commented Jun 16, 2026

Copy link
Copy Markdown
Member

Yes, and it's worth calling out explicitly, because the usual workaround doesn't work as the entrypoint currently stands.

A device passed with --device=/dev/ttyUSB0 keeps the host node's numeric owner, typically root:dialout and mode crw-rw----. Running as uid 1000 we're neither the owner nor a member of the device's group, so open() fails with EACCES. It worked before only because root bypasses file permissions. This hits every RS485 / USB‑serial setup (Modbus RTU, P1/SML readers, etc.) on upgrade.

The normal escape hatch (--group-add <gid> / compose group_add:) is defeated here, because su-exec $PUID:$PGID with an explicit gid calls setgroups(1, &gid) and drops all supplementary groups. Verified on alpine:3:

# su-exec 1000:1000  (as in this PR), started with --group-add 20
after drop: uid=1000 gid=1000 groups=1000              # dialout (20) is gone

# su-exec 1000       (user only), evcc added to dialout in /etc/group
after drop: uid=1000 gid=1000 groups=20(dialout),1000  # survives

So to keep serial devices working the entrypoint would need to:

  1. add the evcc user to the gid(s) owning the mounted device(s) — e.g. an opt‑in env for extra GIDs, or auto‑detecting the group of char devices under /dev, and
  2. invoke su-exec $PUID (user only, no :gid) so getgrouplist() picks the membership up.

Otherwise we should at least document that serial‑device users have to pin PGID to their dialout gid (and even then only if we stop passing the explicit gid to su-exec).

🤖 Generated with Claude Code

@andig

andig commented Jun 16, 2026

Copy link
Copy Markdown
Member

Sketch of the entrypoint change (against the fix/docker-nonroot version of packaging/docker/bin/entrypoint.sh). Two essential parts: (1) add the evcc user to the group(s) owning any mounted serial devices, and (2) drop the explicit gid from the su-exec call so those supplementary groups survive.

 		mkdir -p "$DATA_DIR"
 		chown -R "$PUID:$PGID" "$DATA_DIR"
 		[ -n "$EVCC_DATABASE_DSN" ] && chown -R "$PUID:$PGID" "$(dirname "$EVCC_DATABASE_DSN")" 2> /dev/null || true
 
+		# join the group(s) owning mounted serial devices (Modbus RTU, P1/SML readers);
+		# --device keeps the host owner (often root:dialout), so non-root needs the group
+		DEVICE_GIDS="$EVCC_DEVICE_GIDS"
+		for dev in /dev/ttyUSB* /dev/ttyACM* /dev/ttyAMA* /dev/serial/by-id/*; do
+			[ -e "$dev" ] || continue
+			DEVICE_GIDS="$DEVICE_GIDS $(stat -c '%g' "$dev")"
+		done
+		for gid in $DEVICE_GIDS; do
+			[ "$gid" = "$PGID" ] && continue
+			grp=$(getent group "$gid" | cut -d: -f1)
+			if [ -z "$grp" ]; then
+				grp="evccdev$gid"
+				addgroup -g "$gid" "$grp" 2> /dev/null || true
+			fi
+			addgroup evcc "$grp" 2> /dev/null || true
+		done
+
-		RUNAS="su-exec $PUID:$PGID"
+		# no explicit gid: su-exec uid:gid wipes supplementary groups (setgroups), the
+		# uid-only form keeps them via getgrouplist; primary gid stays PGID from passwd
+		RUNAS="su-exec $PUID"

Verified in alpine:3 (set EVCC_DEVICE_GIDS="20 987", no real device needed) — both the existing-group path (dialout) and the create-named-group path (987) work, and the groups survive the su-exec drop:

uid=1000(evcc) gid=1000(evcc) groups=20(dialout),987(evccdev987),1000(evcc)

Notes:

  • EVCC_DEVICE_GIDS is an opt-in escape hatch for devices the globs miss (or to skip auto-detection entirely).
  • stat -c '%g', addgroup and getent are busybox builtins, so this stays Alpine-only — fine for this image.
  • The globs cover the common serial adapters; widen if a setup needs more.

🤖 Generated with Claude Code

@naltatis

Copy link
Copy Markdown
Member Author

Added the suggested device group. Updated docs PR evcc-io/docs#1088 accordingly.

@naltatis naltatis marked this pull request as ready for review June 23, 2026 17:05

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • The unconditional chown -R "$PUID:$PGID" "$DATA_DIR" (and optional EVCC_DATABASE_DSN dir) on every startup could be expensive for large volumes; consider narrowing this to the actual DB file or gating it (e.g. only when ownership is mismatched) to avoid repeated recursive traversals.
  • The serial device group join logic may add the same gid multiple times and print repeated log lines when multiple device paths share the same group; you could deduplicate DEVICE_GIDS before the loop to keep the output and group management cleaner.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The unconditional `chown -R "$PUID:$PGID" "$DATA_DIR"` (and optional EVCC_DATABASE_DSN dir) on every startup could be expensive for large volumes; consider narrowing this to the actual DB file or gating it (e.g. only when ownership is mismatched) to avoid repeated recursive traversals.
- The serial device group join logic may add the same gid multiple times and print repeated log lines when multiple device paths share the same group; you could deduplicate `DEVICE_GIDS` before the loop to keep the output and group management cleaner.

## Individual Comments

### Comment 1
<location path="packaging/docker/bin/entrypoint.sh" line_range="93-96" />
<code_context>
+	# existing mount (su-exec sets HOME from the passwd entry, not the env).
+	if [ "$(id -u)" = "0" ]; then
+		getent group evcc > /dev/null 2>&1 || addgroup -g "$PGID" evcc 2> /dev/null || addgroup evcc
+		getent passwd evcc > /dev/null 2>&1 || adduser -D -H -h /root -u "$PUID" -G evcc evcc 2> /dev/null || adduser -D -H -h /root -G evcc evcc
+
+		mkdir -p "$DATA_DIR"
+		chown -R "$PUID:$PGID" "$DATA_DIR"
+		# fix a custom database directory if EVCC_DATABASE_DSN points elsewhere
+		[ -n "$EVCC_DATABASE_DSN" ] && chown -R "$PUID:$PGID" "$(dirname "$EVCC_DATABASE_DSN")" 2> /dev/null || true
</code_context>
<issue_to_address>
**issue:** Avoid potential mismatch between requested PUID/PGID and the actual evcc user/group IDs used for chown.

If `$PUID`/`$PGID` are already in use, `adduser -u "$PUID"` / `addgroup -g "$PGID"` may fail and the fallback will create `evcc` with different IDs. In that case, `chown -R "$PUID:$PGID"` won’t match the actual `evcc` user/group. To avoid this, derive the IDs from `id evcc` / `getent group evcc` after creation and use those values for `chown` instead of the requested PUID/PGID.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread packaging/docker/bin/entrypoint.sh Outdated
@naltatis

Copy link
Copy Markdown
Member Author

Pushed a docker image for testing docker hub.
https://hub.docker.com/repository/docker/naltatis/evcc/tags/non-root

docker pull naltatis/evcc:non-root

@StefanSchoof (and others): can you test if this is non-breaking for you?

@naltatis naltatis self-assigned this Jun 24, 2026
@naltatis naltatis added the waiting for feedback Suspended progress label Jun 24, 2026
@StefanSchoof

Copy link
Copy Markdown
Contributor

@naltatis I run my evcc not in docker.

@naltatis naltatis marked this pull request as draft June 24, 2026 13:49
@naltatis

Copy link
Copy Markdown
Member Author

Throws database: unable to open database file (14) on my unraid system :/

@naltatis

Copy link
Copy Markdown
Member Author

Fixed

@naltatis naltatis marked this pull request as ready for review June 24, 2026 14:04

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In the device group-detection loop, for gid in $DEVICE_GIDS; do will process duplicate gids (from multiple devices sharing a group), so consider deduplicating the list before the loop to avoid repeated addgroup calls and repeated log lines.
  • The chown -R "$PUID:$PGID" "$DATA_DIR" and subsequent chmod -R on /root/.evcc may be expensive and override existing permissions on large or pre-populated mounts; you might want to guard these with a more targeted check (e.g., only adjust ownership when mismatched) to minimize surprises for existing volumes.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In the device group-detection loop, `for gid in $DEVICE_GIDS; do` will process duplicate gids (from multiple devices sharing a group), so consider deduplicating the list before the loop to avoid repeated `addgroup` calls and repeated log lines.
- The `chown -R "$PUID:$PGID" "$DATA_DIR"` and subsequent `chmod -R` on `/root/.evcc` may be expensive and override existing permissions on large or pre-populated mounts; you might want to guard these with a more targeted check (e.g., only adjust ownership when mismatched) to minimize surprises for existing volumes.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Basic functionality waiting for feedback Suspended progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants