There is already a root test and the installer sudo can be interactive, so we don’t force users to set a NOPASSWD sudo user.
Because it can return output, such as asking for a password, this test is not valid for user machines, but only on a controlled environment such as a build pipeline.
Because it can return output, such as asking for a password, this test is not valid for user machines, but only on a controlled environment such as a build pipeline.
Let’s add a sudo test that checks if output by sudo is empty but that
only runs inside CI?
I still think this is would be a very worthwhile feature:
The problem is, that this would only work for users that allow sudo to cache passwords.
If caching the sudo password is completely disabled, same in other words if timestamp_timeout=0 is set in sudo settings, then such a test does not seem possible in any sane way.
(Except inventing our own password prompt and caching which we should probably avoid. Specifically for the nice use case of timestamp_timeout=0)
The same in other words, if the user re-enters the sudo password for every sudo command, then this test does not seam possible in any sane way.
Other nice use cases could be where the user doesn’t (only) use a sudo password but configured some other mechanism (smartcards…).
And that is for every other script that requires sudo at certain stages instead of running the script as root or with sudo.
I don’t think we should handle the password, it should be handled by sudo prompt or with sudo’s --stdin option to write the password to stdin and receive the prompt from stderr. That would also have to be deatl with su and doas.
I don’t know about sudo with smartcards, maybe the credentials can also be cached?
Yes we can separate it with the --stdin option, prompt will be sent to stderr. The output will be returned in the file descriptors the commands want it to return.
But, it doesn’t solve the problem with caching passwords, which I don’t think would be safe for us to handle it. Instead, it should be handled before-hand when the user is installing his own operating system. I understand some users might not do this, but then should we disrespect user will to not cache the password?
Yes. I would guess these are independent sub components. The sudo caching period being independent from the authentication itself. Maybe done with the sudo askpass configuration file option. Most likely nothing we need to worry about.
Sounds good.
Yes, we should probably stay away from caching user passwords.
No. Respect user settings regarding cached credentials.
sudo_output now contains stdout and stderr of '$actual_command_to_run_with_sudo` but
user was still able to see sudo’s request for authentication,
its result (if it failed).
Possible?
--stdin
Write the prompt to the standard error and read the password from the standard input instead of using the terminal device.
I don’t see how that helps? Ok, the sudo prompt goes to stderr. stdin will work either way. But then we cannot catch the stderr into a variable or file? (Which is needed to compare it later to see if it’s as expected.)
Password comes from /dev/stdin.
sudo command >/this-is-stdout 2>/this-is-stderr
Shout stderr to the user as it is the prompt. Save stdout as it is the command output.
This is rather complex and all of this for the corner case of maybe handful of users who fully disabled sudo credentials caching. So I doubt we should go for a complex and therefore error-prone solution. Got another idea…
## This code we already have:
log info "Testing root login"
root_cmd echo "Successful root login" ||
die 1 "Failed to run test command as root."
## Below we could add:
user_using_sudo_credential_caching=""
if sudo --non-interactive test -d /usr ; then
user_using_sudo_credential_caching=yes
fi
if [ ! "$user_using_sudo_credential_caching" = "yes" ]; then
log info "credential caching detected: no"
return 0
fi
log info "credential caching detected: yes"
sudo_output=$(root_cmd timeout --kill-after 5 5 test -d /usr 2>&1)
if [ "$sudo_output" = "" ]; then
log info "sudo output test success."
return 0
fi
log error "sudo output: '$sudo_output'"
die 105 "sudo output was expected to be empty but is actually non-empty. This is likely a system configuration issue."
In function virtualbox_start_failed there is currently an issue:
The installer succeeded with download and import, but
But that function doesn’t really know that. If previously downloaded/imported, this is actually false.
I thought maybe I’ll get the information if the installer was downloading from should_download but that function might produce extraneous terminal output. Maybe should_download should be split into two functions, one for test only and one for output? Or set a variable there to indicate if an actual download took place so it can be checked later?
That function also doesn’t know if any import or re-import was done.
Another option would be to simplify the output of that function and just drop it but I think it’s quite useful as it’s supposed to be now?
--redownload (without additional options) is broken, doesn’t actually re-download if previously downloaded and already imported.
if test "${has_vboxmanage}" = "1" && test "${has_linux_headers}" = "1"; then
log notice "vboxmanage and ${linux_headers}"
install_virtualbox_debian_common_end
return 0
fi
This could use a comment why it’s done like this.
It might happen that vboxmanage is installed but VirtualBox-Qt is not.
I guess that’s to cover case where a user installed VirtualBox from the oracle repository already?
if test "${has_vboxmanage}" != "1"; then
log notice "Preparing to install VirtualBox"
install_backports_and_fasttrack_repository_debian
fasttrack_added=1
install_pkg virtualbox
fi
if test "${has_virtualbox_qt}" != "1"; then
log notice "Preparing to install VirtualBox-QT"
test "${fasttrack_added-}" != 1 && install_backports_and_fasttrack_repository_debian
install_pkg virtualbox-qt
fi
I think we don’t need both, because since virtualbox is a dependency of virtualbox-qt, installing virtualbox-qt alone would be sufficient?
log info "If you would like to redownload the image, read about --redownload (safe)."
log info "If you would like to reimport the image, read about --reimport (danger)."
Should that be log notice so its shown by default and easier to learn about for users?