fix switch - #404
Conversation
- Keep flexible run_root_cmd function that supports both sudo and doas - Resolves conflict between hardcoded /usr/bin/sudo and flexible approach 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request introduces a new Kyber host configuration for Ubuntu Linux with Tailscale VPN integration, removes launchctl dependency from the Makefile, adds k3s configuration module, introduces comprehensive Tailscale Home Manager module with systemd services and activation scripts, expands package management with Linux-specific tools, and adds supporting shell utilities and configuration files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the dotfiles' capabilities by integrating a new 'Kyber' server configuration, complete with Tailscale VPN setup and robust secret management. It also introduces a dedicated Tailscale Home Manager module for flexible deployment across different systems. Additionally, the changes refine existing Nix configurations, enhance the CLI proxy API's accessibility, and update various development dependencies and shell aliases to streamline workflows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRThis PR introduces new server configurations for Kyber (k3s, Tailscale), adds several system utilities and packages (doppler, enhanced Tailscale service, git fetch), enables remote management, and includes a minor Makefile fix. What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new host configuration for 'Kyber', including K3s and Tailscale setup, along with several dependency updates and new shell aliases. Key changes involve enabling remote management for cliproxyapi and integrating sshpass for SSH access to the Kyber server. While the new host configuration is well-structured, there are a few areas related to security and consistency that warrant attention.
| # Whether to allow remote (non-localhost) management access. | ||
| # When false, only localhost can access management endpoints (a key is still required). | ||
| allow-remote: false | ||
| allow-remote: true |
There was a problem hiding this comment.
Changing allow-remote to true enables remote management access for cliproxyapi. This is a security-sensitive change that could expose the management API to external networks. Please ensure this is intentional and that appropriate network-level access controls (e.g., firewall rules) are in place to restrict access to trusted sources only. If this is for development or specific internal use, consider adding a prominent warning or making it configurable via environment variables rather than a static true in the default config.
| SERVICE_FILE="/etc/systemd/system/tailscaled.service" | ||
| NIX_SERVICE="${tailscaledServiceFile}" | ||
| SUDOERS_FILE="/etc/sudoers.d/nix-tailscale" | ||
|
|
||
| # Resolve an elevated command helper | ||
| SUDO_CMD="" | ||
| if command -v sudo >/dev/null 2>&1; then | ||
| SUDO_CMD="sudo" | ||
| elif [ -x /usr/bin/sudo ]; then | ||
| SUDO_CMD="/usr/bin/sudo" | ||
| elif command -v doas >/dev/null 2>&1; then | ||
| SUDO_CMD="doas" | ||
| elif [ -x /usr/bin/doas ]; then | ||
| SUDO_CMD="/usr/bin/doas" | ||
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "Tailscale system service installation requires root privileges, but sudo/doas is not available." >&2 | ||
| echo "Either install sudo, configure doas, or run home-manager as root." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| run_root_cmd() { | ||
| if [ -n "$SUDO_CMD" ]; then | ||
| ''${DRY_RUN_CMD:-} "$SUDO_CMD" "$@" | ||
| else | ||
| ''${DRY_RUN_CMD:-} "$@" | ||
| fi | ||
| } | ||
|
|
||
| # Only install if service file differs from nix-generated one | ||
| if ! cmp -s "$NIX_SERVICE" "$SERVICE_FILE" 2>/dev/null; then | ||
| echo "Installing tailscaled systemd service (requires root)..." | ||
| run_root_cmd cp "$NIX_SERVICE" "$SERVICE_FILE" | ||
| run_root_cmd systemctl daemon-reload | ||
| run_root_cmd systemctl enable tailscaled | ||
| echo "Tailscaled service installed." | ||
| fi | ||
|
|
||
| # Configure sudo to include Nix profile paths | ||
| echo "Configuring sudo PATH for Nix packages..." | ||
| SUDOERS_CONTENT="# Added by home-manager for Nix Tailscale | ||
| Defaults secure_path=\"${config.home.homeDirectory}/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\" | ||
| " | ||
|
|
||
| # Create temporary file with correct content | ||
| TEMP_SUDOERS=$(mktemp) | ||
| echo "$SUDOERS_CONTENT" > "$TEMP_SUDOERS" | ||
|
|
||
| # Only update if different or doesn't exist | ||
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | ||
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | ||
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | ||
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | ||
| fi | ||
|
|
||
| rm -f "$TEMP_SUDOERS" | ||
| '' |
There was a problem hiding this comment.
This activation script performs significant system-level modifications, including copying service files to /etc/systemd/system and modifying /etc/sudoers.d. While DRY_RUN_CMD is used, direct manipulation of /etc by a home-manager module is a powerful operation. It's crucial to ensure these operations are thoroughly tested and that the sudoers file modification is strictly necessary and correctly configured to prevent unintended privilege escalation or system misconfiguration. Consider if there's a more declarative NixOS-native way to manage systemd services and sudoers entries if this configuration is intended for NixOS systems, or if this is strictly for non-NixOS Linux where home-manager acts more like a script runner.
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | ||
| if test -n "$password" | ||
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | ||
| else | ||
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" | ||
| ssh ubuntu@$KYBER_IP_ADDR | ||
| end |
There was a problem hiding this comment.
This function retrieves an SSH password from the macOS Keychain and uses sshpass for SSH authentication. While convenient, using sshpass can be less secure than SSH agent forwarding or key-based authentication without a passphrase, as it can expose the password in process lists or history. Additionally, the IP address 91.242.214.231 is hardcoded. It would be more robust and secure to use an SSH configuration file (~/.ssh/config) with IdentityFile and ProxyJump (if applicable) or to make the IP address a configurable variable. If a password is truly necessary, consider using a more secure method than sshpass if available, or at least ensure the Keychain is strongly protected.
| if ! command -v tailscale &>/dev/null; then | ||
| curl -fsSL https://tailscale.com/install.sh | sh | ||
| fi |
There was a problem hiding this comment.
Piping curl output directly to sh (curl -fsSL ... | sh) is a common practice but carries a security risk. It executes arbitrary code downloaded from the internet without prior inspection. While Tailscale's install script is generally trusted, for critical server setups, it's safer to download the script, review its contents, and then execute it. Consider replacing this with a two-step process: download, then execute.
|
|
||
| .PHONY: switch | ||
| switch: nix-switch launchctl ## Apply Nix configuration and restart launchd agents. | ||
| switch: nix-switch ## Apply Nix configuration and restart launchd agents. |
There was a problem hiding this comment.
The comment for the switch target still mentions "restart launchd agents", but the launchctl dependency has been removed. This creates an inconsistency between the target's description and its actual actions.
switch: nix-switch ## Apply Nix configuration.
| if [ -n "$(HOST)" ]; then \ | ||
| echo "Switching named home config: $(HOST)"; \ | ||
| USER=$(NIX_USERNAME) $(NIX_ALLOW_UNFREE) $(NIX_EXEC) run $(NIX_FLAGS) --impure .#homeConfigurations.$(HOST).activationPackage; \ | ||
| else \ | ||
| USER=$(NIX_USERNAME) $(NIX_ALLOW_UNFREE) $(NIX_EXEC) run $(NIX_FLAGS) --impure .#$(NIX_CONFIG_TYPE)."$(NIX_USERNAME)@$(NIX_SYSTEM)".activationPackage; \ | ||
| fi; \ |
There was a problem hiding this comment.
In the homeConfigurations block, the if [ -n "$(HOST)" ] branch hardcodes homeConfigurations.$(HOST).activationPackage, while the else branch uses $(NIX_CONFIG_TYPE)."$(NIX_USERNAME)@$(NIX_SYSTEM)".activationPackage. For consistency and to avoid potential issues if NIX_CONFIG_TYPE were to change its value unexpectedly within this context, it would be clearer to use $(NIX_CONFIG_TYPE) in both branches if it's guaranteed to be homeConfigurations at this point, or explicitly state homeConfigurations in both if that's the intent.
if [ -n "$(HOST)" ]; then \
echo "Switching named home config: $(HOST)"; \
USER=$(NIX_USERNAME) $(NIX_ALLOW_UNFREE) $(NIX_EXEC) run $(NIX_FLAGS) --impure .#$(NIX_CONFIG_TYPE).$(HOST).activationPackage; \
else \
USER=$(NIX_USERNAME) $(NIX_ALLOW_UNFREE) $(NIX_EXEC) run $(NIX_FLAGS) --impure .#$(NIX_CONFIG_TYPE)."$(NIX_USERNAME)@$(NIX_SYSTEM)".activationPackage; \
fi;
|
|
||
| # Declare directories and files for home-manager | ||
| home.file.".local/share/tailscale/tailscaled.state".source = | ||
| config.lib.file.mkOutOfStoreSymlink "${config.home.homeDirectory}/.local/state/tailscale/tailscaled.state"; |
There was a problem hiding this comment.
The tailscaled.state file is configured to be a symlink from .local/share/tailscale/tailscaled.state to ${config.home.homeDirectory}/.local/state/tailscale/tailscaled.state. While xdg.dataHome defaults to ~/.local/share, the target path uses ~/.local/state. This creates an inconsistency in where state data is expected to reside according to XDG Base Directory Specification. It would be clearer and more consistent to either use xdg.stateHome for the target path or keep the state file directly within xdg.dataHome without symlinking to a different XDG directory type.
| ```bash | ||
| kyber # Fish abbreviation that runs: ssh ubuntu@kyber | ||
| ``` |
There was a problem hiding this comment.
The kyber abbreviation is documented here, but the underlying _kyber_function uses sshpass with a password retrieved from the Keychain and a hardcoded IP. The README should either explicitly mention these details or, ideally, guide users towards a more secure SSH setup (e.g., key-based authentication without sshpass and using SSH config for host aliases and identity files) to align with best security practices.
| sudo tailscale up | ||
|
|
There was a problem hiding this comment.
The sudo tailscale up command will prompt for user authentication. Given that the _kyber_function.fish uses sshpass for SSH access, it implies a password-based setup. For server environments, it's generally recommended to use key-based authentication for tailscale up (e.g., via an auth key file managed by agenix, as commented out in named-hosts/kyber/default.nix) to avoid interactive password prompts and enhance automation and security.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 20 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Makefile">
<violation number="1" location="Makefile:138">
P2: Comment is now misleading: the `launchctl` dependency was removed but the comment still says "restart launchd agents". Update the comment to reflect the actual behavior.</violation>
</file>
<file name="home-manager/programs/fish/functions/_kyber_function.fish">
<violation number="1" location="home-manager/programs/fish/functions/_kyber_function.fish:4">
P1: Security vulnerability: Password passed as command-line argument is visible in process listings. Use `sshpass -e` with `SSHPASS` environment variable instead, or prefer SSH key authentication.</violation>
</file>
<file name="home-manager/modules/tailscale/default.nix">
<violation number="1" location="home-manager/modules/tailscale/default.nix:175">
P1: Auth key passed directly on command line is visible in process listings via `ps aux`. Consider deprecating the `authKey` option in favor of `authKeyFile` only, or use environment variables with `EnvironmentFile` to pass the secret securely.</violation>
</file>
<file name="config/k3s/default.nix">
<violation number="1" location="config/k3s/default.nix:17">
P2: Consider using more robust sudo detection like the tailscale module does. The hardcoded `/usr/bin/sudo` path may fail on systems where sudo is located elsewhere (e.g., NixOS), and there's no error handling if the command fails.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| .PHONY: switch | ||
| switch: nix-switch launchctl ## Apply Nix configuration and restart launchd agents. | ||
| switch: nix-switch ## Apply Nix configuration and restart launchd agents. |
There was a problem hiding this comment.
P2: Comment is now misleading: the launchctl dependency was removed but the comment still says "restart launchd agents". Update the comment to reflect the actual behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 138:
<comment>Comment is now misleading: the `launchctl` dependency was removed but the comment still says "restart launchd agents". Update the comment to reflect the actual behavior.</comment>
<file context>
@@ -135,7 +135,7 @@ setup: nix-setup ## Basic Nix setup (alias for nix-setup).
.PHONY: switch
-switch: nix-switch launchctl ## Apply Nix configuration and restart launchd agents.
+switch: nix-switch ## Apply Nix configuration and restart launchd agents.
.PHONY: test
</file context>
| switch: nix-switch ## Apply Nix configuration and restart launchd agents. | |
| switch: nix-switch ## Apply Nix configuration. |
| function _kyber_function --description "SSH to Kyber server" | ||
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | ||
| if test -n "$password" | ||
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR |
There was a problem hiding this comment.
P1: Security vulnerability: Password passed as command-line argument is visible in process listings. Use sshpass -e with SSHPASS environment variable instead, or prefer SSH key authentication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/fish/functions/_kyber_function.fish, line 4:
<comment>Security vulnerability: Password passed as command-line argument is visible in process listings. Use `sshpass -e` with `SSHPASS` environment variable instead, or prefer SSH key authentication.</comment>
<file context>
@@ -0,0 +1,9 @@
+function _kyber_function --description "SSH to Kyber server"
+ set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null)
+ if test -n "$password"
+ sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR
+ else
+ echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w"
</file context>
| let | ||
| authKeyArg = | ||
| if cfg.authKey != "" then | ||
| "--authkey=${cfg.authKey}" |
There was a problem hiding this comment.
P1: Auth key passed directly on command line is visible in process listings via ps aux. Consider deprecating the authKey option in favor of authKeyFile only, or use environment variables with EnvironmentFile to pass the secret securely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/modules/tailscale/default.nix, line 175:
<comment>Auth key passed directly on command line is visible in process listings via `ps aux`. Consider deprecating the `authKey` option in favor of `authKeyFile` only, or use environment variables with `EnvironmentFile` to pass the secret securely.</comment>
<file context>
@@ -0,0 +1,277 @@
+ let
+ authKeyArg =
+ if cfg.authKey != "" then
+ "--authkey=${cfg.authKey}"
+ else if cfg.authKeyFile != "" then
+ "--authkey-file=${cfg.authKeyFile}"
</file context>
| home.activation.k3s-config = lib.mkIf pkgs.stdenv.isLinux ( | ||
| lib.hm.dag.entryAfter [ "writeBoundary" ] '' | ||
| if [ -f "$HOME/.config/k3s/config.yaml" ]; then | ||
| $DRY_RUN_CMD /usr/bin/sudo mkdir -p /etc/rancher/k3s |
There was a problem hiding this comment.
P2: Consider using more robust sudo detection like the tailscale module does. The hardcoded /usr/bin/sudo path may fail on systems where sudo is located elsewhere (e.g., NixOS), and there's no error handling if the command fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/k3s/default.nix, line 17:
<comment>Consider using more robust sudo detection like the tailscale module does. The hardcoded `/usr/bin/sudo` path may fail on systems where sudo is located elsewhere (e.g., NixOS), and there's no error handling if the command fails.</comment>
<file context>
@@ -0,0 +1,22 @@
+ home.activation.k3s-config = lib.mkIf pkgs.stdenv.isLinux (
+ lib.hm.dag.entryAfter [ "writeBoundary" ] ''
+ if [ -f "$HOME/.config/k3s/config.yaml" ]; then
+ $DRY_RUN_CMD /usr/bin/sudo mkdir -p /etc/rancher/k3s
+ $DRY_RUN_CMD /usr/bin/sudo cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml
+ fi
</file context>
There was a problem hiding this comment.
Pull request overview
This PR adds configuration for a new Ubuntu Linux host called "Kyber" with Tailscale VPN integration, along with several related improvements to the Makefile, package management, and configuration files. The changes enable managing remote Linux servers using home-manager with secure networking through Tailscale.
- Adds comprehensive Kyber host configuration with home-manager, Tailscale, and agenix secrets management
- Implements a reusable Tailscale home-manager module with system service installation support
- Enhances the Makefile's
switchtarget to support named host configurations via HOST parameter - Adds k3s and cliproxyapi configuration support
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Removes launchctl dependency from switch target (macOS-specific); adds HOST parameter support for named configurations |
| package.json | Adds @beads/bd package dependency |
| bun.lock | Locks @beads/bd@0.29.0 with binary support |
| nix-darwin/config/homebrew.nix | Adds sshpass for password-based SSH connections |
| home-manager/modules/tailscale/default.nix | New comprehensive Tailscale module with system service installation and sudo/doas detection |
| home-manager/modules/default.nix | Registers the new Tailscale module |
| home-manager/packages/default.nix | Adds doppler package |
| home-manager/programs/fish/default.nix | Adds cliproxyapi alias and kyber abbreviation for SSH access |
| home-manager/programs/fish/functions/_kyber_function.fish | New SSH function with macOS Keychain password integration |
| named-hosts/kyber/default.nix | Main configuration for Kyber Ubuntu host with Tailscale and agenix |
| named-hosts/kyber/secrets.nix | Template for agenix secrets (currently empty/commented) |
| named-hosts/kyber/setup.sh | Bootstrap script for initial Tailscale installation on Kyber |
| named-hosts/kyber/README.md | Documentation for Kyber setup, secrets management, and SSH access |
| config/k3s/default.nix | K3s configuration management with sudo-based activation |
| config/k3s/config.yaml | K3s config to disable traefik |
| config/default.nix | Registers k3s configuration module |
| config/cliproxyapi/default.nix | Adds example config file for cliproxyapi bootstrap |
| config/cliproxyapi/config.yaml | Enables remote management access |
| config/claude/settings.json | Adds git fetch to allowed Bash commands |
| flake.nix | Registers kyber as a new homeConfigurations entry |
Comments suppressed due to low confidence (1)
home-manager/programs/fish/functions/_kyber_function.fish:4
- Passing password as a command-line argument (
-p $password) exposes it in process listings and system logs. This is a security vulnerability as any user on the system can see the password by runningps auxor similar commands. Consider using SSH key-based authentication instead, or usesshpasswith the-eflag and pass the password via the SSHPASS environment variable.
sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Whether to allow remote (non-localhost) management access. | ||
| # When false, only localhost can access management endpoints (a key is still required). | ||
| allow-remote: false | ||
| allow-remote: true |
There was a problem hiding this comment.
Enabling allow-remote: true exposes the management API to non-localhost connections. While authentication is still required (line 11 shows secret-key: ""), the empty secret key means the Management API is currently disabled (404 for all routes). If a secret key is later added, this configuration will allow remote access. Ensure this is intentional and that strong authentication is in place before deploying.
| allow-remote: true | |
| allow-remote: false |
| $DRY_RUN_CMD /usr/bin/sudo mkdir -p /etc/rancher/k3s | ||
| $DRY_RUN_CMD /usr/bin/sudo cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml |
There was a problem hiding this comment.
The hardcoded path /usr/bin/sudo may not work on all Linux distributions (e.g., NixOS places sudo in a different location). Consider using just sudo without the full path to let the shell's PATH resolution find it, or follow the pattern used in the Tailscale module (lines 223-237 in home-manager/modules/tailscale/default.nix) which checks for sudo availability before using it.
| $DRY_RUN_CMD /usr/bin/sudo mkdir -p /etc/rancher/k3s | |
| $DRY_RUN_CMD /usr/bin/sudo cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml | |
| $DRY_RUN_CMD sudo mkdir -p /etc/rancher/k3s | |
| $DRY_RUN_CMD sudo cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml |
| ```bash | ||
| kyber # Fish abbreviation that runs: ssh ubuntu@kyber | ||
| ``` |
There was a problem hiding this comment.
The documentation mentions using the kyber Fish abbreviation for SSH access, but doesn't document that the KYBER_IP_ADDR environment variable must be set for the function to work. Add a note about setting this environment variable (e.g., in the Initial Setup section or SSH Access section).
| SERVICE_FILE="/etc/systemd/system/tailscaled.service" | ||
| NIX_SERVICE="${tailscaledServiceFile}" | ||
| SUDOERS_FILE="/etc/sudoers.d/nix-tailscale" | ||
|
|
||
| # Resolve an elevated command helper | ||
| SUDO_CMD="" | ||
| if command -v sudo >/dev/null 2>&1; then | ||
| SUDO_CMD="sudo" | ||
| elif [ -x /usr/bin/sudo ]; then | ||
| SUDO_CMD="/usr/bin/sudo" | ||
| elif command -v doas >/dev/null 2>&1; then | ||
| SUDO_CMD="doas" | ||
| elif [ -x /usr/bin/doas ]; then | ||
| SUDO_CMD="/usr/bin/doas" | ||
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "Tailscale system service installation requires root privileges, but sudo/doas is not available." >&2 | ||
| echo "Either install sudo, configure doas, or run home-manager as root." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| run_root_cmd() { | ||
| if [ -n "$SUDO_CMD" ]; then | ||
| ''${DRY_RUN_CMD:-} "$SUDO_CMD" "$@" | ||
| else | ||
| ''${DRY_RUN_CMD:-} "$@" | ||
| fi | ||
| } | ||
|
|
||
| # Only install if service file differs from nix-generated one | ||
| if ! cmp -s "$NIX_SERVICE" "$SERVICE_FILE" 2>/dev/null; then | ||
| echo "Installing tailscaled systemd service (requires root)..." | ||
| run_root_cmd cp "$NIX_SERVICE" "$SERVICE_FILE" | ||
| run_root_cmd systemctl daemon-reload | ||
| run_root_cmd systemctl enable tailscaled | ||
| echo "Tailscaled service installed." | ||
| fi | ||
|
|
||
| # Configure sudo to include Nix profile paths | ||
| echo "Configuring sudo PATH for Nix packages..." | ||
| SUDOERS_CONTENT="# Added by home-manager for Nix Tailscale | ||
| Defaults secure_path=\"${config.home.homeDirectory}/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\" | ||
| " | ||
|
|
||
| # Create temporary file with correct content | ||
| TEMP_SUDOERS=$(mktemp) | ||
| echo "$SUDOERS_CONTENT" > "$TEMP_SUDOERS" | ||
|
|
||
| # Only update if different or doesn't exist | ||
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | ||
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | ||
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | ||
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | ||
| fi | ||
|
|
||
| rm -f "$TEMP_SUDOERS" |
There was a problem hiding this comment.
[nitpick] The shell script has inconsistent and excessive indentation (using many leading spaces). This makes the code harder to read and maintain. Consider aligning the script content to the left margin with minimal indentation, similar to the style used in the createTailscaleDirs activation script (lines 202-213).
| SERVICE_FILE="/etc/systemd/system/tailscaled.service" | |
| NIX_SERVICE="${tailscaledServiceFile}" | |
| SUDOERS_FILE="/etc/sudoers.d/nix-tailscale" | |
| # Resolve an elevated command helper | |
| SUDO_CMD="" | |
| if command -v sudo >/dev/null 2>&1; then | |
| SUDO_CMD="sudo" | |
| elif [ -x /usr/bin/sudo ]; then | |
| SUDO_CMD="/usr/bin/sudo" | |
| elif command -v doas >/dev/null 2>&1; then | |
| SUDO_CMD="doas" | |
| elif [ -x /usr/bin/doas ]; then | |
| SUDO_CMD="/usr/bin/doas" | |
| elif [ "$(id -u)" -ne 0 ]; then | |
| echo "Tailscale system service installation requires root privileges, but sudo/doas is not available." >&2 | |
| echo "Either install sudo, configure doas, or run home-manager as root." >&2 | |
| exit 1 | |
| fi | |
| run_root_cmd() { | |
| if [ -n "$SUDO_CMD" ]; then | |
| ''${DRY_RUN_CMD:-} "$SUDO_CMD" "$@" | |
| else | |
| ''${DRY_RUN_CMD:-} "$@" | |
| fi | |
| } | |
| # Only install if service file differs from nix-generated one | |
| if ! cmp -s "$NIX_SERVICE" "$SERVICE_FILE" 2>/dev/null; then | |
| echo "Installing tailscaled systemd service (requires root)..." | |
| run_root_cmd cp "$NIX_SERVICE" "$SERVICE_FILE" | |
| run_root_cmd systemctl daemon-reload | |
| run_root_cmd systemctl enable tailscaled | |
| echo "Tailscaled service installed." | |
| fi | |
| # Configure sudo to include Nix profile paths | |
| echo "Configuring sudo PATH for Nix packages..." | |
| SUDOERS_CONTENT="# Added by home-manager for Nix Tailscale | |
| Defaults secure_path=\"${config.home.homeDirectory}/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\" | |
| " | |
| # Create temporary file with correct content | |
| TEMP_SUDOERS=$(mktemp) | |
| echo "$SUDOERS_CONTENT" > "$TEMP_SUDOERS" | |
| # Only update if different or doesn't exist | |
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | |
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | |
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | |
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | |
| fi | |
| rm -f "$TEMP_SUDOERS" | |
| SERVICE_FILE="/etc/systemd/system/tailscaled.service" | |
| NIX_SERVICE="${tailscaledServiceFile}" | |
| SUDOERS_FILE="/etc/sudoers.d/nix-tailscale" | |
| # Resolve an elevated command helper | |
| SUDO_CMD="" | |
| if command -v sudo >/dev/null 2>&1; then | |
| SUDO_CMD="sudo" | |
| elif [ -x /usr/bin/sudo ]; then | |
| SUDO_CMD="/usr/bin/sudo" | |
| elif command -v doas >/dev/null 2>&1; then | |
| SUDO_CMD="doas" | |
| elif [ -x /usr/bin/doas ]; then | |
| SUDO_CMD="/usr/bin/doas" | |
| elif [ "$(id -u)" -ne 0 ]; then | |
| echo "Tailscale system service installation requires root privileges, but sudo/doas is not available." >&2 | |
| echo "Either install sudo, configure doas, or run home-manager as root." >&2 | |
| exit 1 | |
| fi | |
| run_root_cmd() { | |
| if [ -n "$SUDO_CMD" ]; then | |
| ''${DRY_RUN_CMD:-} "$SUDO_CMD" "$@" | |
| else | |
| ''${DRY_RUN_CMD:-} "$@" | |
| fi | |
| } | |
| # Only install if service file differs from nix-generated one | |
| if ! cmp -s "$NIX_SERVICE" "$SERVICE_FILE" 2>/dev/null; then | |
| echo "Installing tailscaled systemd service (requires root)..." | |
| run_root_cmd cp "$NIX_SERVICE" "$SERVICE_FILE" | |
| run_root_cmd systemctl daemon-reload | |
| run_root_cmd systemctl enable tailscaled | |
| echo "Tailscaled service installed." | |
| fi | |
| # Configure sudo to include Nix profile paths | |
| echo "Configuring sudo PATH for Nix packages..." | |
| SUDOERS_CONTENT="# Added by home-manager for Nix Tailscale | |
| Defaults secure_path=\"${config.home.homeDirectory}/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\" | |
| " | |
| # Create temporary file with correct content | |
| TEMP_SUDOERS=$(mktemp) | |
| echo "$SUDOERS_CONTENT" > "$TEMP_SUDOERS" | |
| # Only update if different or doesn't exist | |
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | |
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | |
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | |
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | |
| fi | |
| rm -f "$TEMP_SUDOERS" |
| "license": "ISC", | ||
| "packageManager": "bun@1.3.0", | ||
| "dependencies": { | ||
| "@beads/bd": "^0.29.0", |
There was a problem hiding this comment.
The @beads/bd package includes a binary (bin/bd.js according to bun.lock line 49) but is not listed in the trustedDependencies array. Packages with install scripts or binaries should typically be included in trustedDependencies to explicitly declare trust for execution during installation. Consider adding "@beads/bd" to the trustedDependencies array (line 26-37).
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | ||
| else | ||
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" | ||
| ssh ubuntu@$KYBER_IP_ADDR |
There was a problem hiding this comment.
The variable $KYBER_IP_ADDR is undefined and not set anywhere in the codebase. This will cause the SSH command to fail or connect to an unintended host. The function references the IP 91.242.214.231 in the keychain lookup (line 2) but uses the undefined variable for the actual connection. Either define KYBER_IP_ADDR as an environment variable or use the IP address directly.
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | |
| else | |
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" | |
| ssh ubuntu@$KYBER_IP_ADDR | |
| sshpass -p $password ssh ubuntu@91.242.214.231 | |
| else | |
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" | |
| ssh ubuntu@91.242.214.231 |
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | ||
| if test -n "$password" | ||
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | ||
| else | ||
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" |
There was a problem hiding this comment.
The function contains a hardcoded IP address (91.242.214.231) in the keychain service name. This makes the function less maintainable and could lead to confusion if the IP changes. Consider using a hostname or a constant variable instead.
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | |
| if test -n "$password" | |
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | |
| else | |
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@91.242.214.231' -a ubuntu -w" | |
| set -l password (security find-generic-password -s "ssh ubuntu@$KYBER_IP_ADDR" -w 2>/dev/null) | |
| if test -n "$password" | |
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | |
| else | |
| echo "Password not found in Keychain. Run: security add-generic-password -s 'ssh ubuntu@$KYBER_IP_ADDR' -a ubuntu -w" |
| function _kyber_function --description "SSH to Kyber server" | ||
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | ||
| if test -n "$password" | ||
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR |
There was a problem hiding this comment.
The use of sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR exposes the SSH password as a command-line argument, which can be read by other local users or malware via process listings and system logs, leaking credentials for the Kyber server. An attacker with local user access could monitor running processes to capture the password and then reuse it to log into the remote host. To avoid exposing credentials, prefer key-based SSH authentication, or if sshpass is required, use a more secure mechanism (e.g., reading the password from a protected file or environment variable with sshpass -e) instead of passing it directly on the command line.
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR | |
| set -lx SSHPASS $password | |
| sshpass -e ssh ubuntu@$KYBER_IP_ADDR |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 3d9f07b...3cd48e9
Analysis
-
System-Level Operations via Home Manager: Using a user-space tool (Home Manager) to manage system-level services creates security boundary issues, risks partial configuration on sudo failures, and leaves persistent system changes that violate declarative principles.
-
Incomplete Automation Chain: Critical elements are missing or commented out (Tailscale auth keys, SSH key validation, undefined environment variables), requiring manual intervention and undermining the declarative configuration approach.
-
Expanded Attack Surface: Remote management for cliproxyapi is enabled without visible authentication configuration, and password-based SSH authentication is used instead of more secure alternatives like Tailscale SSH.
-
Cross-Platform Compatibility Issues: The configuration mixes Darwin-specific elements with Linux-focused additions without proper platform guards, including hard-coded paths that will fail on different systems.
-
Silent Failures: Multiple sudo operations during home-manager activation could fail silently, and the k3s configuration sync lacks error handling and validation.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
20 files reviewed | 6 comments | Edit Agent Settings • Read Docs
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "Tailscale system service installation requires root privileges, but sudo/doas is not available." >&2 | ||
| echo "Either install sudo, configure doas, or run home-manager as root." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Using exit 1 here will abort the entire home-manager activation if sudo/doas is unavailable, potentially leaving the system in a partially configured state. Consider using a warning instead of a hard error, or add a configuration option to make system service installation optional. This is especially problematic since the module has installSystemService = true by default.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: home-manager/modules/tailscale/default.nix#L236
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Using `exit 1` here will abort the entire home-manager activation if sudo/doas is unavailable, potentially leaving the system in a partially configured state. Consider using a warning instead of a hard error, or add a configuration option to make system service installation optional. This is especially problematic since the module has `installSystemService = true` by default.
| }; | ||
|
|
||
| # Agenix configuration | ||
| age.identityPaths = [ "/home/${username}/.ssh/id_ed25519" ]; |
There was a problem hiding this comment.
The SSH key path /home/${username}/.ssh/id_ed25519 is assumed to exist but is never created or validated. If this key doesn't exist when agenix tries to decrypt secrets, the entire home-manager activation will fail. Consider adding an activation script to generate the SSH key if it doesn't exist, or document this as a prerequisite in the README.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: named-hosts/kyber/default.nix#L47
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The SSH key path `/home/${username}/.ssh/id_ed25519` is assumed to exist but is never created or validated. If this key doesn't exist when agenix tries to decrypt secrets, the entire home-manager activation will fail. Consider adding an activation script to generate the SSH key if it doesn't exist, or document this as a prerequisite in the README.
| function _kyber_function --description "SSH to Kyber server" | ||
| set -l password (security find-generic-password -s "ssh ubuntu@91.242.214.231" -w 2>/dev/null) | ||
| if test -n "$password" | ||
| sshpass -p $password ssh ubuntu@$KYBER_IP_ADDR |
There was a problem hiding this comment.
The variable $KYBER_IP_ADDR is undefined and will cause SSH connection failures. Additionally, using sshpass -p $password exposes the password in process arguments (visible via ps). Consider either: 1) Define KYBER_IP_ADDR as an environment variable in the fish config, or 2) Use the Tailscale hostname directly (e.g., kyber.tailnet-name.ts.net), which would be more secure and eliminate the need for password-based auth entirely.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: home-manager/programs/fish/functions/_kyber_function.fish#L4
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The variable `$KYBER_IP_ADDR` is undefined and will cause SSH connection failures. Additionally, using `sshpass -p $password` exposes the password in process arguments (visible via `ps`). Consider either: 1) Define `KYBER_IP_ADDR` as an environment variable in the fish config, or 2) Use the Tailscale hostname directly (e.g., `kyber.tailnet-name.ts.net`), which would be more secure and eliminate the need for password-based auth entirely.
| # Whether to allow remote (non-localhost) management access. | ||
| # When false, only localhost can access management endpoints (a key is still required). | ||
| allow-remote: false | ||
| allow-remote: true |
There was a problem hiding this comment.
Enabling remote management access significantly expands the attack surface. While line 10 mentions that a management key is required, the actual key configuration is not visible in this diff. Ensure that: 1) A strong management key is configured (not left empty/default), 2) The service is only accessible via Tailscale VPN (not exposed to the public internet), and 3) This change is intentional and documented in the PR description with security implications.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: config/cliproxyapi/config.yaml#L7
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Enabling remote management access significantly expands the attack surface. While line 10 mentions that a management key is required, the actual key configuration is not visible in this diff. Ensure that: 1) A strong management key is configured (not left empty/default), 2) The service is only accessible via Tailscale VPN (not exposed to the public internet), and 3) This change is intentional and documented in the PR description with security implications.
|
|
||
| # Configure sudo to include Nix profile paths | ||
| echo "Configuring sudo PATH for Nix packages..." | ||
| SUDOERS_CONTENT="# Added by home-manager for Nix Tailscale |
There was a problem hiding this comment.
Modifying /etc/sudoers.d/ during every home-manager activation creates a permanent system-level change that persists even after uninstalling the module. This violates the principle of declarative configuration management. Consider: 1) Documenting this as a manual setup step, 2) Adding a cleanup mechanism in a deactivation hook, or 3) Using a different approach that doesn't require persistent sudoers modifications.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: home-manager/modules/tailscale/default.nix#L258
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Modifying `/etc/sudoers.d/` during every home-manager activation creates a permanent system-level change that persists even after uninstalling the module. This violates the principle of declarative configuration management. Consider: 1) Documenting this as a manual setup step, 2) Adding a cleanup mechanism in a deactivation hook, or 3) Using a different approach that doesn't require persistent sudoers modifications.
| home.activation.k3s-config = lib.mkIf pkgs.stdenv.isLinux ( | ||
| lib.hm.dag.entryAfter [ "writeBoundary" ] '' | ||
| if [ -f "$HOME/.config/k3s/config.yaml" ]; then | ||
| $DRY_RUN_CMD /usr/bin/sudo mkdir -p /etc/rancher/k3s |
There was a problem hiding this comment.
The hard-coded /usr/bin/sudo path will fail on systems where sudo is installed elsewhere (e.g., NixOS where it's in /run/wrappers/bin/sudo, or some distributions using /bin/sudo). Use command -v sudo or check multiple paths as done in the Tailscale module (lines 225-236 of home-manager/modules/tailscale/default.nix). Additionally, there's no error handling if the sudo commands fail.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#404
File: config/k3s/default.nix#L17
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The hard-coded `/usr/bin/sudo` path will fail on systems where sudo is installed elsewhere (e.g., NixOS where it's in `/run/wrappers/bin/sudo`, or some distributions using `/bin/sudo`). Use `command -v sudo` or check multiple paths as done in the Tailscale module (lines 225-236 of `home-manager/modules/tailscale/default.nix`). Additionally, there's no error handling if the sudo commands fail.
Summary by cubic
Fixes the Makefile switch command and adds a full Kyber (Ubuntu) host setup with Tailscale and k3s config sync. Also enables remote management for cliproxyapi and updates tooling.
New Features
Bug Fixes
Written for commit 3cd48e9. Summary will update automatically on new commits.