tools,win: refactor install_tools.bat

[refack]

• Ask for credentials upfront (just for an opaque token)
• Try to -DisableReboots

Refs: #22645
Refs: #23838
Refs: nodejs/security-wg#439

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1500/pipeline

[joaocgreis]

I see 3 changes here:

• Ask for credentials: I'm not sure I understand the point of this. Boxstarter already stores the password in a SecureString, same as here. This makes the script display a dialog asking for credentials, without any explanation why. Given the reports we've been getting, I believe this will make things worst and more confusing. Furthermore, credentials are only used by Boxstarter for rebooting, so if disabling reboots goes forward this is not necessary.
• Move much of the script to Powershell: I don't think it's worth the added complexity for now if the above is removed, but it's fine either way. Needs to be added to the MSI.
• Disable reboots: reboots are there for reliability, so this might leave us in a worst place. The text in the .bat file needs to be adapted, the prompt no longer makes sense.

So, points 1 and 3 are mutually exclusive, it doesn't make sense to move forward with both.

I believe we should leave handling of credentials to Boxstarter, and not try to duplicate the feature here.

I'd like to see if #23987 has a positive effect. If it doesn't help, disabling reboots is probably the best thing to try next.

[joaocgreis]

It would be great to to know if disabling reboots can never make the installation fail, or if there is some concrete scenario where it will make the installation fail for sure. (cc @gep13 @ferventcoder @jberezanski if you know, that info would be very welcome here. Thanks!)

I've only seen the installation of updates fail, but Visual Studio still installed correctly and is usable. However, it fails in a way that makes it look like the whole installation failed, so it's sure to bring more confusion.

[refack]

• Ask for credentials: This makes the script display a dialog asking for credentials, without any explanation why.

AFAICT this is the recommended way to use boxstarter (Refs:https://boxstarter.org/InstallingPackages)
The small benefit I see with this is it uses an WIN32 api to get the credentials, and doesn't ask for the password via command prompt.

• Move much of the script to Powershell: I don't think it's worth the added complexity for now if the above is removed, but it's fine either way. Needs to be added to the MSI.

IMHO it reduces complexity (PS code in .ps1 file, instead of a mixed language .bat), but I agree it make most sense if we accept all 3 changes.

• Disable reboots: reboots are there for reliability, so this might leave us in a worst place.

Hoping for feedback on this for boxstarter people

• The text in the .bat file needs to be adapted, the prompt no longer makes sense.

Will do.

[joaocgreis]

AFAICT this is the recommended way to use boxstarter (Refs:https://boxstarter.org/InstallingPackages)

This makes a lot of sense for longer installations, if you need to leave the computer running overnight and don't know if it'll need to reboot or not. Using just Install-BoxstarterPackage without credentials is also present in that page.

The small benefit I see with this is it uses an WIN32 api to get the credentials, and doesn't ask for the password via command prompt.

I don't see how that is a benefit. When asking in the command line, Boxstarter also uses a SecureString, which is arguably irrelevant because it can be decrypted so easily.

To be clear, the two disadvantages I see in this are:

• If a credentials window just pops up, it will probably "look like a virus" to many users. We should at least display some text in the console with a pause, before the window pops up.
• Boxstarter does not ask for password if autologon is enabled, which is a nice feature for anyone using VMs. We should make sure this works with autologon enabled and no passwords.

[gep13]

The short answer is, it depends :-(

While on a fully patched machine, the three Chocolatey Packages that are choosing to install along with your node application might not need a reboot, on another machine, it might be required. There is no real way of knowing. Chocolatey, first and foremost is a application package manager. It will note that a reboot may be required to continue with further installations, but it won't do anything about it. As a result, attempting to install further packages, if a reboot is pending, might result in a failed package installation. That is where Boxstarter came from in the first place, to detect when a pending reboot is required, and to take the necessary steps to reboot the machine, and then continue with the installation. Disabling reboots might have the unfortunate side effect of failing the later packages, if an earlier one requires a reboot.

I don't know what testing infrastructure, but do you test the installation of NodeJS on a suite of machines prior to releasing?

[refack]

@gep13, thank you for the explanation.

I don't know what testing infrastructure, but do you test the installation of NodeJS on a suite of machines prior to releasing?

We don't test the installer, just the binary.
Just thinking about the variable-space makes me dizzy 😵 (versions X builds X patch-levels X locales X other-installed-products, oh my).

I think we might need help from the MSVS team, to clarify what are the prerequisites for installing the MSVS Build Tools SKU...

[gep13]

@refack said...
Just thinking about the variable-space makes me dizzy 😵 (versions X builds X patch-levels X locales X other-installed-products, oh my).

Those are some of the things that Boxstarter and Chocolatey are trying to cover for you, however, they can only do that if they are allowed to, i.e. by rebooting the machine when required. Otherwise, the application installations are going to fail. Based on that, and assuming that you are going to continue to shell out to Boxstarter and Chocolatey to perform these installations, I think you have to allow the reboots to happen. That is just my opinion though.

[refack]

Those are some of the things that Boxstarter and Chocolatey are trying to cover for you, however, they can only do that if they are allowed to, i.e. by rebooting the machine when required.

Ack - 56e913c

Although ATM we have to balance the chance of a failed dependency, with the chance of a degraded UX for the baseline installer ⚖️

[jberezanski]

Chocolatey, first and foremost is a application package manager. It will note that a reboot may be required to continue with further installations, but it won't do anything about it.

I'll expand upon that. If Package A signals that a reboot is required, not only will Chocolatey not do anything about it (other than displaying a message to the user), Chocolatey will (unfortunately) immediately proceed to install packages dependent on Package A - whose installation may fail, depending on the exact packages in question and the state of the machine. For example, if the .NET Framework needs to be upgraded (as a dependency of another package) and its installation requires a reboot, applications depending on that .NET version may fail to run until the machine is rebooted.

That is why, in order to achieve a more robust machine configuration experience, it is better to install known low-level dependencies (vcredist140, dotnetfx, Windows Updates - KB2919355 and others etc.) explicitly in separate choco invocations and allow the machine configuration framework (such as Boxstarter, PowerShell DSC or simply a custom script with autologon and runonce) to ensure the machine is rebooted if necessary in between.

[jberezanski]

See also: chocolatey/choco#1038