fix(security): run controller as non-root 'taos' user (#639) - #677
Merged
Conversation
The tinyagentos.service unit now runs as a dedicated system user 'taos' instead of root. The installer still runs as root (via sudo bash); only the resulting systemd runtime drops to the unprivileged user. Changes: - scripts/install-server.sh: create system user 'taos' (idempotent), add to incus + docker groups (warn if absent, don't fail), substitute User=taos/Group=taos into the system unit template, chown data/ to taos:taos (0700) and chmod 0600 sensitive credential files after setup - scripts/systemd/tinyagentos.service: unchanged — TAOS_USER/TAOS_GROUP placeholders already present, ExecStartPre=+ debugfs lines already have the root-override prefix and remain correct - os-build/userpatches/overlay/etc/systemd/system/tinyagentos.service: User=root → User=taos, Group=taos added - os-build/userpatches/extensions/tinyagentos.sh: create taos user in the Armbian image chroot, add to incus/docker groups, chown data dir - tinyagentos/deployer.py: one-line comment clarifying os.getuid() returns the taos UID when running under systemd (no behaviour change)
|
Warning Review limit reached
More reviews will be available in 14 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughMigrates TinyAgentOS to run as a non-root system user ( ChangesNon-Root Service Migration (root → taos)
Sequence DiagramsequenceDiagram
participant Installer as install-server.sh
participant TaosSetup as TaosProvisioning
participant SystemdConfig as SystemdUnitConfig
participant DataOwnership as DataDirSetup
Installer->>TaosSetup: ensure_taos_user()
TaosSetup->>TaosSetup: create taos user, add to groups
Installer->>SystemdConfig: substitute User=taos Group=taos
Installer->>DataOwnership: set_data_dir_ownership()
DataOwnership->>DataOwnership: chown/chmod to taos:taos, tighten secrets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-root taos, data-preserving) (#639)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/install-server.sh (1)
1292-1293: ⚡ Quick winSilent
chownfailure masks issues that would cause service startup failure.If
chownfails (e.g., taos user doesn't exist due to an earlier failure), the service won't be able to write todata/and will fail at runtime with a confusing error. The migration script (pre-beta-to-beta.sh) warns on this same failure:chown -R taos:taos "$NEW_TAOS_DIR/data" 2>/dev/null \ || warn " chown failed (taos user may not exist) — service will fail to start"Consider matching that pattern here for consistency and better diagnostics.
♻️ Proposed fix
- chown -R taos:taos "$INSTALL_DIR/data" 2>/dev/null || true + chown -R taos:taos "$INSTALL_DIR/data" 2>/dev/null \ + || warn "chown failed (taos user may not exist) — service will fail to start"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-server.sh` around lines 1292 - 1293, The silent chown in install-server.sh masks failures; replace the suppressed fallback on the chown -R taos:taos "$INSTALL_DIR/data" 2>/dev/null || true with the same diagnostic behavior used in pre-beta-to-beta.sh—i.e., if chown fails, call the existing warn function (or print a stderr warning if warn is not available) with a clear message like "chown failed (taos user may not exist) — service will fail to start" so operators see the cause instead of silently continuing.scripts/pre-beta-to-beta.sh (1)
300-303: 💤 Low valueMinor: Comment mentions "group" but code applies to "others".
The comment says "group execute+read" but
o+rXmodifies permissions for "others", not "group". The code is correct (taos isn't in root's group, so others-read is needed), but the comment is misleading.📝 Suggested comment fix
-# The venv and source tree must be readable by the taos user (group -# execute+read) but owned by root is fine — taos does not need write access. +# The venv and source tree must be readable by the taos user (world +# read+execute) but owned by root is fine — taos does not need write access. if [[ -d "$NEW_TAOS_DIR/.venv" ]]; then chmod -R o+rX "$NEW_TAOS_DIR/.venv" 2>/dev/null || true fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pre-beta-to-beta.sh` around lines 300 - 303, Update the misleading comment above the chmod line that currently states "group execute+read" to accurately describe the permission change being applied to others; reference the chmod invocation that uses chmod -R o+rX on "$NEW_TAOS_DIR/.venv" and change the comment to state that the venv and source tree must be readable/executable by others (o+rX) because the taos user is not in the owning group, or alternatively clarify both cases if group membership is possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@os-build/userpatches/extensions/tinyagentos.sh`:
- Around line 37-40: The usermod call currently swallows failures with "||
true", preventing the outer display_alert from running; remove the "|| true" so
that a failing usermod inside the chroot (the block that uses
getent/groupadd/usermod with variables ${SDCARD}, ${grp}, taos) returns non-zero
and triggers the outer display_alert "TinyAgentOS" path; ensure you still
suppress expected stderr if desired (e.g., keep "2>/dev/null" only if you intend
to ignore noisy output) but do not mask the exit status of usermod so membership
update failures are detectable.
- Around line 47-51: The success alert is always shown even though the
chown/chmod are masked with "|| true"; change the block that runs chroot and the
chown/chmod so it captures their exit status and only calls display_alert
"TinyAgentOS" "Set /opt/tinyagentos/data ownership to taos:taos (0700)" "info"
when both commands succeed, otherwise call display_alert with an error/failure
message; specifically modify the chroot "... chown -R taos:taos
/opt/tinyagentos/data ... chmod 0700 ..." invocation to remove the unconditional
"|| true" masking and check the combined exit code (or inspect individual exits)
before invoking display_alert, referencing the chroot invocation and the
chown/chmod commands and the display_alert call to locate the change.
---
Nitpick comments:
In `@scripts/install-server.sh`:
- Around line 1292-1293: The silent chown in install-server.sh masks failures;
replace the suppressed fallback on the chown -R taos:taos "$INSTALL_DIR/data"
2>/dev/null || true with the same diagnostic behavior used in
pre-beta-to-beta.sh—i.e., if chown fails, call the existing warn function (or
print a stderr warning if warn is not available) with a clear message like
"chown failed (taos user may not exist) — service will fail to start" so
operators see the cause instead of silently continuing.
In `@scripts/pre-beta-to-beta.sh`:
- Around line 300-303: Update the misleading comment above the chmod line that
currently states "group execute+read" to accurately describe the permission
change being applied to others; reference the chmod invocation that uses chmod
-R o+rX on "$NEW_TAOS_DIR/.venv" and change the comment to state that the venv
and source tree must be readable/executable by others (o+rX) because the taos
user is not in the owning group, or alternatively clarify both cases if group
membership is possible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1299927e-1b18-46ec-b59f-257454b34455
📒 Files selected for processing (5)
os-build/userpatches/extensions/tinyagentos.shos-build/userpatches/overlay/etc/systemd/system/tinyagentos.servicescripts/install-server.shscripts/pre-beta-to-beta.shtinyagentos/deployer.py
…+ move pending-restart flag to data dir (#639) Two updater-compatibility fixes for the non-root taos service user introduced in #677: 1. chown the whole INSTALL_DIR to taos (not just data/) so the in-app updater can write to .git/, .venv/, and static/desktop/ during git pull, pip install -e ., and npm run build. Mirrored in install-server.sh, pre-beta-to-beta.sh, and the Armbian image extension. Data dir and secret files are tightened AFTER the broad chown so restrictive perms win. Security trade-off noted in comments: full update-privilege-separation is post-beta hardening. 2. Replace the hardcoded ~/.config/taos/pending-restart.json path with a _pending_restart_path() helper in restart_orchestrator.py. Resolution order: TAOS_DATA_DIR env (already used by __main__.py for Mac) → <install_dir>/data (derived from __file__, matches PROJECT_DIR/"data" in app.py) → ~/.config/taos fallback for root/dev installs. All write/read/clear calls updated to use the helper.
…on instead of masking them (CodeRabbit on #677)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/restart_orchestrator.py`:
- Around line 56-59: The current broad except blocks around path.read_text() and
json.loads (and the similar block around path.unlink()) swallow all errors;
change them to handle only FileNotFoundError (return None for read, ignore for
unlink) and let other errors surface: replace "except Exception:" with "except
FileNotFoundError: return None" for the read path (keeping json.JSONDecodeError
handled explicitly if desired) and for the clear/unlink path catch
FileNotFoundError and re-raise or log and re-raise any other exceptions so
ownership/permission/filesystem errors are not hidden; refer to the occurrences
using path.read_text(), json.loads(...) and path.unlink() to locate the two
fixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b080440-d327-4b0e-a8c7-1883a1855f3a
📒 Files selected for processing (4)
os-build/userpatches/extensions/tinyagentos.shscripts/install-server.shscripts/pre-beta-to-beta.shtinyagentos/restart_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (3)
- os-build/userpatches/extensions/tinyagentos.sh
- scripts/pre-beta-to-beta.sh
- scripts/install-server.sh
Comment on lines
56
to
59
| try: | ||
| return json.loads(PENDING_RESTART_PATH.read_text()) | ||
| return json.loads(path.read_text()) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Avoid swallowing filesystem failures in pending-restart read/clear paths.
Line 58 and Line 65 currently hide all errors, which can mask ownership/permission regressions after the root→taos migration and produce false “no pending restart” states in update-status/boot-time checks.
Suggested fix
def read_pending_restart() -> dict | None:
path = _pending_restart_path()
if not path.exists():
return None
try:
return json.loads(path.read_text())
- except Exception:
+ except json.JSONDecodeError:
+ logger.warning("Invalid pending restart JSON at %s", path)
+ return None
+ except OSError:
+ logger.exception("Failed reading pending restart file at %s", path)
return None
def clear_pending_restart() -> None:
try:
_pending_restart_path().unlink(missing_ok=True)
- except Exception:
- pass
+ except OSError:
+ logger.exception("Failed clearing pending restart file")Also applies to: 63-66
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 58-58: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/restart_orchestrator.py` around lines 56 - 59, The current broad
except blocks around path.read_text() and json.loads (and the similar block
around path.unlink()) swallow all errors; change them to handle only
FileNotFoundError (return None for read, ignore for unlink) and let other errors
surface: replace "except Exception:" with "except FileNotFoundError: return
None" for the read path (keeping json.JSONDecodeError handled explicitly if
desired) and for the clear/unlink path catch FileNotFoundError and re-raise or
log and re-raise any other exceptions so ownership/permission/filesystem errors
are not hidden; refer to the occurrences using path.read_text(), json.loads(...)
and path.unlink() to locate the two fixes.
Source: Linters/SAST tools
…ad/clear (CodeRabbit on #677)
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This was referenced Jun 10, 2026
Merged
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
1 participant
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.
Summary
scripts/install-server.sh): creates a dedicatedtaossystem user (idempotent), adds it to theincusanddockergroups (warns instead of failing if either group is absent), substitutesUser=taos/Group=taosinto the rendered system unit (was$USER), thenchown -R taos:taos data/(mode 0700) andchmod 0600the sensitive credential files after the venv/data setup.scripts/systemd/tinyagentos.service): no change needed —TAOS_USER/TAOS_GROUPplaceholders were already there; theExecStartPre=+chmoddebugfs lines already carry the+root-override prefix and continue to work as-is.os-build/userpatches/overlay/etc/systemd/system/tinyagentos.service):User=root→User=taos,Group=taosadded.os-build/userpatches/extensions/tinyagentos.sh): createstaosuser in the image chroot, adds toincus/dockergroups (creates them if absent so the user record is correct even before packages land at first-boot),chown -R taos:taos /opt/tinyagentos/data(0700).tinyagentos/deployer.py: one-line comment only —os.getuid()returning thetaosUID is correct forraw.idmap both {uid} 0(maps container-root to the hosttaosUID for the trace bind-mount). No behaviour change.systemd/tinyagentos-host-firewall.service: left as-is — it is a separate root oneshot for iptables rules and does not need to change.scripts/pre-beta-to-beta.sh): run-once helper for existing users upgrading from a root-based pre-beta install to this non-root beta layout (see below).What was not changed
sudo bash) — the drop totaosis in the systemd unit only.install_linux_systemd_userpath (no-sudo fallback) still uses$USERas before — it runs as the calling user by design.Root-only assumptions checked
The only genuine concern was
os.getuid()indeployer.pybeing passed ashost_uidto incusraw.idmap. Verified:raw.idmap both {uid} 0maps container-root to whatever UID the controller runs as. When running astaos(a non-zero UID) this is correct and required — it ensures the trace bind-mount directory owned bytaosis writable by the container. No code change needed.Migration script — pre-beta → beta (
scripts/pre-beta-to-beta.sh)Existing users who ran a root-based pre-beta install (typically at
/root/tinyagentoson the Pi) need to copy their data to the new non-root layout. Run this after the beta installer has been applied:Or with explicit paths if auto-detection is ambiguous:
Pass
--yesto skip the interactive confirmation prompt.What it does (in order):
tinyagentos.serviceNEW/data/to a timestamped.tgzif it already has content (never overwrites without a backup)OLD/data/→NEW/data/(cp -a, preserving timestamps/perms)taossystem user exists and hasincus/dockergroup membership (same logic as the installer)chown -R taos:taos NEW/(whole install dir) +chmod 0700 data/+chmod 0600credential filesUser=root→User=taosif the old unit is still in placesystemctl daemon-reload && systemctl start tinyagentosactiveand running astaos; prints PASS/FAIL with next stepsThe OLD install is never modified or deleted — it is left intact for rollback.
Updater compatibility
Two additional fixes so the in-app updater (Settings → Updates) works correctly when running as non-root
taos:Fix 1 — taos owns the whole install dir, not just data/
The updater needs to write to
.git/(git pull),.venv/(pip install -e .), andstatic/desktop/(npm run build).set_data_dir_ownership()ininstall-server.shpreviously only chowneddata/; it now chowns the entireINSTALL_DIRfirst, then tightensdata/to 0700 and secret files to 0600 on top (order matters — restrictive perms win). The same change is mirrored inpre-beta-to-beta.shand the Armbian build extension.Security trade-off:
taosowning its own code directory is the minimum required for non-root in-app self-update without a privileged helper. Full update-privilege-separation (a signed updater suid binary that verifies integrity before writing) is a post-beta hardening task.Fix 2 — pending-restart flag moved out of ~/.config
restart_orchestrator.pypreviously hardcoded~/.config/taos/pending-restart.json. Thetaosservice user is created with-M(no home directory), so~does not resolve to a writable path under systemd.Replaced the module-level constant with a
_pending_restart_path()helper. Resolution order:$TAOS_DATA_DIR/pending-restart.json— when the env var is set (already used by__main__.pyfor the Mac app).<install_dir>/data/pending-restart.json— derived fromPath(__file__).parent.parent / "data", which is the samePROJECT_DIR / "data"convention used throughoutapp.py. This is the path used on every standard Linux install.~/.config/taos/pending-restart.json— backward-compatible fallback for root-based or developer installs where~resolves correctly.The systemd unit template does not need a new
Environment=TAOS_DATA_DIR=…line: the module-location fallback (__file__→../../data) resolves to the correct<install_dir>/datadirectory automatically, because the process is always launched withWorkingDirectory=<install_dir>and the module lives inside that tree.All three functions (
write_pending_restart,read_pending_restart,clear_pending_restart) now call_pending_restart_path()so the path is consistent across write/read/clear.Pi test plan (deploy-time — validate on Pi before relying on it)
This is a deploy-time change with no automated CI coverage. Validate on the Orange Pi 5 Plus:
sudo bash scripts/install-server.sh) on the Pi to create thetaosuser and update the unit.id taos— expectuid=<N>(taos) gid=<N>(taos) groups=...,incus,docker.systemctl show -p User tinyagentos— expectUser=taos.systemctl status tinyagentos— Active: active (running).ps -o user= -p $(systemctl show -p MainPID tinyagentos | cut -d= -f2)— expecttaos.ls -la /opt/tinyagentos/— expecttaos:taosowner throughout;data/should bedrwx------.http://taos.local:6969, log in.incusgroup.data/trace/<slug>/is created and populated by the container.systemctl status tinyagentos-host-firewall— still applies iptables rules (runs as root, unaffected by this change).ExecStartPre=+chmoddebugfs lines run as root regardless ofUser=taos).data/pending-restart.jsonis created (not~/.config/taos/)./root/tinyagentosinstall, runsudo bash scripts/pre-beta-to-beta.sh --yesand verify the 8-step output ends withMigration PASSED, the service is active astaos, and data is intact.Summary by CodeRabbit
New Features
Security Improvements
Install/Upgrade