Skip to content

Conversation

@guruswarupa
Copy link
Contributor

@guruswarupa guruswarupa commented Jan 7, 2026

User description

Issue

Failed to install openssh-server error when it is already installed and running through apt #886

Description

In scripts/install.sh at line 430, the command nixopus install "${cli_args[@]}" is executed without sudo privileges. This causes failures when installing system packages like openssh-server, even though the Python code in deps.py uses sudo for individual package commands.


Scope of Change

Select all applicable areas impacted by this PR:

  • View (UI/UX)
  • API
  • CLI
  • Infra / Deployment
  • Docs
  • Other (specify): installation

Screenshot / Video / GIF (if applicable)

before fix:
image

after fix:
image


Developer Checklist

To be completed by the developer who raised the PR.

  • Add valid/relevant title for the PR
  • Self-review done
  • Manual dev testing done
  • No secrets exposed
  • No merge conflicts
  • Docs added/updated (if applicable)
  • Removed debug prints / secrets / sensitive data
  • Unit / Integration tests passing
  • Follows all standards defined in Nixopus Docs

Reviewer Checklist

To be completed by the reviewer before merge.

  • Peer review done
  • No console.logs / fmt.prints left
  • No secrets exposed
  • If any DB migrations, migration changes are verified
  • Verified release changes are production-ready

PR Type

Bug fix


Description

  • Add sudo privilege check for normal users in install script

  • Execute nixopus install with sudo when EUID is not root

  • Prevent permission failures for system package installations


Diagram Walkthrough

flowchart LR
  A["Check EUID"] --> B{Is root?}
  B -->|No| C["Execute with sudo"]
  B -->|Yes| D["Execute without sudo"]
  C --> E["nixopus install"]
  D --> E
Loading

File Walkthrough

Relevant files
Bug fix
install.sh
Add conditional sudo execution for install command             

scripts/install.sh

  • Added EUID check to determine if script runs as root user
  • Execute nixopus install with sudo when EUID is not 0
  • Maintain direct execution when already running as root
  • Prevents permission errors for system package installations
+5/-1     

Summary by CodeRabbit

  • Bug Fixes
    • Installation now intelligently handles permission requirements by automatically detecting your current privilege level. When elevated permissions are needed, the installer requests them automatically. When running with root access, it executes directly. This ensures a seamless setup experience for all users.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

The scripts/install.sh file is modified to check if the script is running as root. If not root, it invokes the nixopus install command with sudo; otherwise, it runs directly with elevated privileges already present.

Changes

Cohort / File(s) Summary
Privilege elevation wrapper
scripts/install.sh
Adds root privilege check that conditionally wraps nixopus install invocation with sudo when not running as root, ensuring elevated permissions for the install operation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

Review effort 2/5

Poem

🐰 A rabbit's ode to elevated rights:

Root or not, the script now knows,
Sudo wraps when privilege flows,
Permissions squared with wisdom's touch,
Install commands don't ask for much! 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: run with sudo for normal users' directly summarizes the main change: wrapping the nixopus install invocation with a sudo check for non-root users to ensure elevated privileges for system package installation.

✏️ Tip: You can configure your own custom Pre-merge Checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c4be3e and 1c41c10.

📒 Files selected for processing (1)
  • scripts/install.sh
🔇 Additional comments (1)
scripts/install.sh (1)

430-434: Critical privilege escalation flaw: Python code still hardcodes sudo despite shell-level fix.

The shell script's EUID check (lines 430-434) only partially addresses the issue. The Python code in cli/app/commands/install/deps.py unconditionally hardcodes sudo in all subprocess calls (lines 40, 44, 46, 48, 50 for package manager updates and lines 75, 79, 81, 83, 85 for package installations). This creates two problematic scenarios:

  1. Normal user: sudo nixopus install → Python adds another sudo, resulting in nested sudo calls
  2. Root user: Direct execution → Python still tries to use sudo, which will fail or behave unexpectedly

The install_all_deps() function called from install_dependencies() in run.py (line 167) has no privilege awareness. Either the Python code needs to check os.geteuid() and conditionally prepend sudo to commands, or environment variables should be used to signal when sudo is unnecessary. The current split between shell-level and Python-level privilege handling is inconsistent and violates proper design principles.

⛔ Skipped due to learnings
Learnt from: raghavyuva
Repo: raghavyuva/nixopus PR: 0
File: :0-0
Timestamp: 2025-09-14T14:31:19.830Z
Learning: The Nixopus update functionality has a critical flaw: the update process in cli/app/commands/update/run.py calls BaseDockerService for "pull" and "up" operations without passing the env_file parameter. This causes containers to start without environment variables, leading to health check failures. Specifically, the Postgres health check "pg_isready -U ${USERNAME} -d ${DB_NAME}" fails when USERNAME and DB_NAME environment variables are missing, eventually causing Postgres to enter recovery mode and potentially lose production data.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Privileged PATH execution

Description: The script runs sudo nixopus install ... without using an absolute path to nixopus, which
can be risky if sudo does not enforce a safe secure_path and a malicious nixopus earlier
in PATH is executed with elevated privileges. install.sh [430-434]

Referred Code
if [ $EUID -ne 0 ]; then
    sudo nixopus install "${cli_args[@]}"
else
    nixopus install "${cli_args[@]}"
fi
Ticket Compliance
🟡
🎫 #886
🔴 If openssh-server is already installed and running (via apt), the installer should skip
attempting to install it and proceed to the next step instead of failing.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing failure handling: The script executes sudo nixopus install/nixopus install without checking the exit status
and then unconditionally logs success, which can mask installation failures.

Referred Code
if [ $EUID -ne 0 ]; then
    sudo nixopus install "${cli_args[@]}"
else
    nixopus install "${cli_args[@]}"
fi
log_success "nixopus install completed successfully!"

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Incomplete audit context: The new privileged execution path logs the command options but does not clearly record the
executing user identity, timestamp, and the actual command outcome in a verifiable way
within this diff.

Referred Code
    log_info "Running 'nixopus install' with subcommand and options: ${cli_args[*]}"
else
    log_info "Running 'nixopus install' with options: ${cli_args[*]}"
fi
if [ $EUID -ne 0 ]; then
    sudo nixopus install "${cli_args[@]}"
else
    nixopus install "${cli_args[@]}"
fi
log_success "nixopus install completed successfully!"

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Potential sensitive logging: The new log lines print ${cli_args[*]} which may include sensitive values
(tokens/keys/password-like args) depending on how install options are provided.

Referred Code
    log_info "Running 'nixopus install' with subcommand and options: ${cli_args[*]}"
else
    log_info "Running 'nixopus install' with options: ${cli_args[*]}"
fi

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add install error handling

Add error handling for the nixopus install command to detect failures and
prevent the script from showing a misleading success message.

scripts/install.sh [430-434]

-if [ $EUID -ne 0 ]; then
-    sudo nixopus install "${cli_args[@]}"
+if [ "$EUID" -ne 0 ]; then
+    if ! sudo nixopus install "${cli_args[@]}"; then
+        log_error "nixopus install failed"
+        exit 1
+    fi
 else
-    nixopus install "${cli_args[@]}"
+    if ! nixopus install "${cli_args[@]}"; then
+        log_error "nixopus install failed"
+        exit 1
+    fi
 fi
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This is a critical improvement. The script currently prints a success message regardless of whether the nixopus install command succeeds or fails. Adding error handling makes the script's output reliable.

Medium
General
Improve script portability for user check

Replace the bash-specific $EUID variable with the more portable id -u command to
check the user ID.

scripts/install.sh [430]

-if [ $EUID -ne 0 ]; then
+if [ "$(id -u)" -ne 0 ]; then
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that $EUID is a bash-specific variable and proposes using the more portable id -u command, which improves the script's reliability across different shell environments.

Medium
Check sudo availability

Add a check to verify that the sudo command is available on the system before
executing it.

scripts/install.sh [431]

+command -v sudo >/dev/null 2>&1 || { log_error "sudo not found, please install sudo or run as root."; exit 1; }
 sudo nixopus install "${cli_args[@]}"
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This suggestion improves the script's robustness by checking for the existence of the sudo command before attempting to use it, providing a better user experience on systems where it might not be installed.

Low
Quote variable in test

Enclose the $EUID variable in double quotes within the if condition to prevent
potential shell parsing issues.

scripts/install.sh [430]

-if [ $EUID -ne 0 ]; then
+if [ "$EUID" -ne 0 ]; then
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: Quoting variables in shell script tests is a good practice to prevent word splitting and globbing issues, enhancing script robustness, although the immediate risk with $EUID is low.

Low
  • More

@raghavyuva
Copy link
Owner

LGTM!

Comment on lines +430 to +434
if [ $EUID -ne 0 ]; then
sudo nixopus install "${cli_args[@]}"
else
nixopus install "${cli_args[@]}"
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets exit and throw error if not root.

For installing docker and setting related permissions for docker and docker daemon, we need root permission mandatorily.

nixopus install works fine if docker is already there, but if docker is not installed, then we have to run with sudo.

So advised to run it as sudo. If you check docs, here also by default we expect user to run the script with sudo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this escalation, normal users would fail midway because certain installs require root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even after we enter sudo password, it doesn't continue installation.
to fix that, sudo is added here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants