For sudo that requires a password, this PR takes the pw from a file #37
Conversation
|
One other thing to look at while validating this PR is to check on machine that these changes run on password-less setup |
There was a problem hiding this comment.
Pull request overview
This PR adds support for systems that require a password for sudo operations by introducing the --become-password-file flag to all ansible-playbook commands and sudo -S with password file redirection for direct sudo commands. The default password file location is configured as core/inventory/.become-passfile.
Key Changes:
- Added
BECOME_PASSWORD_FILEvariable in config-vars.sh pointing to a default password file location - Updated all ansible-playbook commands that use
--becometo include the--become-password-fileparameter - Modified direct sudo commands to use
sudo -Swith input redirection from the password file
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| core/lib/system/config-vars.sh | Defines the BECOME_PASSWORD_FILE variable with default path |
| core/lib/system/precheck/readiness-check.sh | Adds --become-password-file to ansible-playbook command |
| core/lib/system/precheck/prereq-check.sh | Updates sudo commands to use sudo -S with password file input |
| core/lib/components/observability-controller.sh | Adds --become-password-file to observability playbook deployment |
| core/lib/components/intel-base-operator.sh | Adds --become-password-file to Habana AI operator deployment |
| core/lib/cluster/state/cluster-state-check.sh | Adds --become-password-file to cluster state check and wait commands |
| core/lib/cluster/nodes/remove-node.sh | Adds --become-password-file to node removal playbook |
| core/lib/cluster/nodes/add-node.sh | Adds --become-password-file to node addition playbook |
| core/lib/cluster/deployment/fresh-install.sh | Adds --become-password-file to fresh installation playbook |
| core/lib/cluster/deployment/cluster-purge.sh | Adds --become-password-file to cluster reset playbook |
| core/lib/cluster/config/cluster-config-init.sh | Adds --become-password-file to cluster config deployment |
| core/lib/add-node.sh | Adds --become-password-file to node addition playbook |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif command -v apt &> /dev/null; then | ||
| echo "Installing dependencies using apt Ubuntu..." | ||
| sudo apt update && sudo apt install -y "${other_deps[@]}" | ||
| sudo -S apt update < "${BECOME_PASSWORD_FILE}" && sudo -S apt install -y "${other_deps[@]}" < "${BECOME_PASSWORD_FILE}" |
There was a problem hiding this comment.
Using sudo -S with password file redirection exposes the password in the process command line and can leave it in shell history. Additionally, reading from the same file twice in a compound command may cause issues if the file contains only one password line. Consider using ansible-playbook with --become-password-file for consistency with the rest of the PR, or use a more secure method like sudo's NOPASSWD configuration or askpass helper.
| if command -v apt &> /dev/null; then | ||
| echo "Updating package lists using apt Ubuntu..." | ||
| if sudo apt update; then | ||
| if sudo -S apt update < "${BECOME_PASSWORD_FILE}"; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| elif command -v dnf &> /dev/null; then | ||
| echo "Updating package lists using dnf (RHEL/CentOS)..." | ||
| if sudo dnf check-update || [ $? -eq 100 ]; then | ||
| if sudo -S dnf check-update < "${BECOME_PASSWORD_FILE}" || [ $? -eq 100 ]; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| if command -v dnf &> /dev/null; then | ||
| echo "Installing dependencies using dnf RHEL..." | ||
| sudo dnf install -y "${other_deps[@]}" | ||
| sudo -S dnf install -y "${other_deps[@]}" < "${BECOME_PASSWORD_FILE}" |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| if [[ "$python_version" == "3.11" ]]; then | ||
| echo "Installing python3.11-pip using dnf (RHEL 9)..." | ||
| if ! sudo dnf install -y python3.11-pip; then | ||
| if ! sudo -S dnf install -y python3.11-pip < "${BECOME_PASSWORD_FILE}"; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| elif [[ "$python_version" == "3.12" ]]; then | ||
| echo "Installing python3.12-pip using dnf (RHEL 9)..." | ||
| if ! sudo dnf install -y python3.12-pip; then | ||
| if ! sudo -S dnf install -y python3.12-pip < "${BECOME_PASSWORD_FILE}"; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| else | ||
| echo "Installing python3-pip using dnf (RHEL 9)..." | ||
| if ! sudo dnf install -y python3-pip; then | ||
| if ! sudo -S dnf install -y python3-pip < "${BECOME_PASSWORD_FILE}"; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
| elif command -v apt &> /dev/null; then | ||
| echo "Installing python3-pip using apt (Ubuntu 22/24)..." | ||
| if ! sudo apt install -y python3-pip; then | ||
| if ! sudo -S apt install -y python3-pip < "${BECOME_PASSWORD_FILE}"; then |
There was a problem hiding this comment.
The sudo -S approach with file redirection is less secure than the ansible --become-password-file method used elsewhere. The password could be exposed in process listings and shell history. Consider refactoring these package management operations into an ansible playbook to maintain consistent security practices throughout the codebase.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
This adds "--become-password-file" to all ansible-playbook commands that become root. The default file location is set in core/lib/system/config-vars.sh to core/inventory/.become-passfile.
For systems that require the user's password to sudo, this update allows for clean execution of inference-stack-deploy without sudo permission errors or interactive sudo password prompts.