Skip to content

Commit

Permalink
nixos-rebuild: fix the maybeSudo usage
Browse files Browse the repository at this point in the history
* properly expand the command using arrays instead of strings
* also handle sudo on the localhost
  • Loading branch information
zimbatm committed Jan 21, 2020
1 parent 93204f1 commit ab10bac
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions nixos/modules/installer/tools/nixos-rebuild.sh
Expand Up @@ -22,7 +22,7 @@ repair=
profile=/nix/var/nix/profiles/system
buildHost=
targetHost=
maybeSudo=
maybeSudo=()

while [ "$#" -gt 0 ]; do
i="$1"; shift 1
Expand Down Expand Up @@ -92,7 +92,7 @@ while [ "$#" -gt 0 ]; do
;;
--use-remote-sudo)
# note the trailing space

This comment has been minimized.

Copy link
@bjornfor

bjornfor Jan 29, 2020

Contributor

Comment doesn't apply anymore.

maybeSudo="sudo "
maybeSudo=(sudo --)
shift 1
;;
*)
Expand All @@ -102,6 +102,10 @@ while [ "$#" -gt 0 ]; do
esac
done

if [ -n "$SUDO_USER" ]; then

This comment has been minimized.

Copy link
@bjornfor

bjornfor Jan 29, 2020

Contributor

Did you mean to add this? It was explicitly disabled in 2c09cfc.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Jan 29, 2020

Author Member

yes it's convenient for my use-case

This comment has been minimized.

Copy link
@bjornfor

bjornfor Jan 29, 2020

Contributor

Right, it was convenient for my use-case too, hence that was the initial implementation. I changed it only after someone disliked the idea of remote sudo being "automatic" when using local sudo. But oh, well, it's fewer keystrokes now to get remote sudo :-)

This comment has been minimized.

Copy link
@bjornfor

bjornfor Jan 29, 2020

Contributor

With this feature back in the docs should be updated. Currently the docs claim that only the flag enables remote sudo.

This comment has been minimized.

Copy link
@yorickvP

yorickvP Feb 19, 2020

Contributor

Seconding removal, this causes nixos-rebuild to require sudo in the path even on local deployment in some cases. You don't always want to run this as root locally.

This comment has been minimized.

Copy link
@bjornfor

bjornfor Feb 20, 2020

Contributor

@yorickvP: AFAICT, it doesn't require local sudo. But if you do use local sudo, you get remote sudo too. If you only want remote sudo, use the flag.

This comment has been minimized.

Copy link
@yorickvP

yorickvP Feb 25, 2020

Contributor

if you do sudo -u somebody nixos-rebuild, this will cause it to sudo back to root, requiring sudo.

maybeSudo=(sudo --)
fi

if [ -z "$buildHost" -a -n "$targetHost" ]; then
buildHost="$targetHost"
fi
Expand All @@ -116,17 +120,17 @@ buildHostCmd() {
if [ -z "$buildHost" ]; then
"$@"
elif [ -n "$remoteNix" ]; then
ssh $SSHOPTS "$buildHost" env PATH="$remoteNix:$PATH" "$maybeSudo$@"
ssh $SSHOPTS "$buildHost" env PATH="$remoteNix:$PATH" "${maybeSudo[@]}" "$@"
else
ssh $SSHOPTS "$buildHost" "$maybeSudo$@"
ssh $SSHOPTS "$buildHost" "${maybeSudo[@]}" "$@"
fi
}

targetHostCmd() {
if [ -z "$targetHost" ]; then
"$@"
"${maybeSudo[@]}" "$@"
else
ssh $SSHOPTS "$targetHost" "$maybeSudo$@"
ssh $SSHOPTS "$targetHost" "${maybeSudo[@]}" "$@"
fi
}

Expand Down

0 comments on commit ab10bac

Please sign in to comment.