Skip to content

Add sudoers rules and install copy for users.yaml and workspaces.yaml#105

Merged
dcellison merged 2 commits intomainfrom
fix/protected-yaml-sudoers
Mar 22, 2026
Merged

Add sudoers rules and install copy for users.yaml and workspaces.yaml#105
dcellison merged 2 commits intomainfrom
fix/protected-yaml-sudoers

Conversation

@dcellison
Copy link
Owner

Summary

install apply was missing sudoers rules and file copy logic for users.yaml and workspaces.yaml. Without NOPASSWD sudoers rules, _read_protected_file() in config.py fails silently (sudo -n cat exits non-zero), and the bot falls back to ALLOWED_USER_IDS / empty workspace configs with no error logged.

Changes

1. Sudoers rules (_generate_sudoers in install.py)

Added cat rules for /etc/kai/users.yaml and /etc/kai/workspaces.yaml between the existing services.yaml and totp.secret rules. Config files are now grouped together in the sudoers output.

2. File copy during install (_apply_secrets in install.py)

Refactored the single services.yaml copy block into a loop that handles all three YAML config files (services.yaml, users.yaml, workspaces.yaml) identically: copy if exists in source directory, chmod 0600, chown root:root.

3. Dry run message (_apply_secrets in install.py)

Updated the dry_run branch to report which YAML configs would be copied, matching the real code path.

Why this matters

On a protected installation (/etc/kai/ owned by root), the runtime code tries sudo -n cat /etc/kai/users.yaml first. Without a NOPASSWD rule, this fails silently and _load_user_configs() returns None, falling back to ALLOWED_USER_IDS. The multi-user features from PRs #97 and #98 would be completely ignored on a protected installation with no visible error.

Fixes #104

Test plan

  • test_contains_cat_rules asserts rules for all config files including users.yaml and workspaces.yaml
  • All 976 tests pass

install apply was missing sudoers rules for users.yaml and
workspaces.yaml. Without NOPASSWD rules, sudo -n cat fails
silently and the bot falls back to ALLOWED_USER_IDS / empty
workspace configs with no error logged.

- Add cat rules for users.yaml and workspaces.yaml in _generate_sudoers
- Refactor services.yaml copy in _apply_secrets into a loop that
  handles all three YAML config files identically
- Update dry_run message to reflect YAML configs
- Update test_contains_cat_rules to assert the two new rules

Fixes #104
@dcellison
Copy link
Owner Author

Review by Kai

PR looks clean overall. The fix is correct, minimal, and follows established patterns. No critical or warning-level issues.


Suggestion — install.py dry-run path is hardcoded

# dry_run branch (line ~1282):
print(f"[DRY RUN] Would copy: /etc/kai/{yaml_name} (mode 0600)")

# real branch below uses the variable:
yaml_dst = etc_kai / yaml_name
print(f"  Copied {yaml_dst}")

The dry-run message hardcodes /etc/kai/ while the real path uses etc_kai. The existing dry-run line above it correctly uses env_path (a variable). If etc_kai is ever non-default, the dry-run output would silently misreport the destination. Should be etc_kai / yaml_name for consistency.


No issues with:

  • Sudoers rules — correct placement, follows existing pattern exactly.
  • Loop refactor — chmod 0600 + chown root:root applied after every copy, exists() guard is correct.
  • shutil.copy2 — fine here; permissions are explicitly overwritten immediately after.
  • Tests — cover the specific regression (missing sudoers entries) that was the root cause.

@dcellison
Copy link
Owner Author

Review by Kai

PR looks good. The prior suggestion was addressed.

The dry-run branch now correctly uses etc_kai / yaml_name (line ~1283) instead of a hardcoded /etc/kai/ string — the fix from the prior review is in.

No new findings:

  • Sudoers rules — correct placement, no wildcards, specific paths, matches existing pattern exactly.
  • Loop refactorchmod 0600 + chown root:root applied after each copy, exists() guard preserved. shutil.copy2 is fine since permissions are immediately overwritten.
  • Dry-run branch — now uses etc_kai / yaml_name, consistent with the real code path.
  • Tests — cover the regression directly. No test for the copy loop, but that matches the existing test coverage level for the original services.yaml block.
  • Security0600 + root:root on workspaces.yaml is conservative but appropriate; no objection.

Clean to merge.

@dcellison dcellison closed this Mar 22, 2026
@dcellison dcellison deleted the fix/protected-yaml-sudoers branch March 22, 2026 14:42
@dcellison dcellison restored the fix/protected-yaml-sudoers branch March 22, 2026 14:43
@dcellison dcellison reopened this Mar 22, 2026
@dcellison dcellison merged commit 2c66d15 into main Mar 22, 2026
2 checks passed
@dcellison dcellison deleted the fix/protected-yaml-sudoers branch March 22, 2026 14:45
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.

install apply does not add sudoers rules for users.yaml and workspaces.yaml

2 participants