Several minor issues in openvpn.nsi

[mattock]

While reviewing PR#58 I noticed several small issues not related to that PR.

1. Location of !addplugindir

Right now !addplugindir command in openvpn.nsi is right before nsProcess.nsh is included

; nsProcess.nsh to detect whether OpenVPN process is running ( http://nsis.sourceforge.net/NsProcess_plugin )
!addplugindir .
!include "nsProcess.nsh"

This is technically correct, as nsProcess.nsh is the only .nsh file that ships a DLL. See !addplugindir reference for details.

In my opinion, however, it !addplugindir should be to the top, before all !include lines. This way we would not have to remember to add new !include lines (that may bring DLL's with them) after !include "nsProcess.nsh".

2. Some links are http instead of https

See lines 400 and 401.

3. Confusingly named labels

Labels "guiEndYes" and "guiEndNo" are used in both -pre and .onInit section. According to NSIS documentation labels are local (per-section) by default, so this is safe. However, the use of those labels in .onInit function is confusing, as the section has nothing to do with stopping the GUI.

4. Selecting OpenVPN GUI section does not select OpenVPN User-space components

As the header says. In addition, selecting the "OpenVPN Service" section does select OpenVPN User-space component section. So we're not being consistent.

---

I can provide a PR that fixes all/some of these, depending on what we want.

EDIT: added two other minor issues. EDIT 2: added yet another minor issue

[selvanair]

1. Yes, let's move !include "nsProcess.nsh" to the top
2. I prefer https if available -- for me its more than optics
3. Those labels be better changed to something more meaningful like WInXPNo and WinXPYes Also indentation is wrong in the beginning of .OnInit -- fixing would make it easier to read the code..
4. Good point: I think we should select user-space when GUI is selected. When service is selected, we already force-select user-space components, don't we?

To add to that, the biggest damage we do in section -pre is to remove the services even if service installation is not selected. As we allow options like install only TAP or EasyRSA, this is bad as the user will lose the service setup if any such option is selected. We also allow to uncheck dependencies (advanced) which could likely lead to a broken installation with no or mismatched openssl dlls.

Personally I would prefer to reduce the number of check boxes. Also do not remove or stop the service, GUI or openvpn.exe if only TAP or EasyRSA is selected. TAP install will update the driver which causes a SIGHUP restart on any connected tunnels that we can warn the user about. Anyone who needs more fine control can roll their own installer -- I know an admin who patches openvpn.nsi to make installers with with no options.

[mattock]

I will implement the four fixes after PR#58 is merged.

I think we could solve most of the problems you noted by reducing user choice, as discussed on the mailing list. Right now we have the following sections:

• SecAddShortcutsWorkaround
• SecOpenVPNUserSpace
• SecService
• SecInteractiveService
• SecTAP
• SecOpenVPNGUI
• SecFileAssociation
• SecOpenSSLUtilities
• SecOpenVPNEasyRSA
• SecAddPath
• SecAddShortcuts

Out of these SecOpenVPNEasyRSA is clearly optional. I think we could even strip it out of the installer completely, as it is only needed for OpenVPN servers on Windows. We would then just have to point users to documentation about using alternative CAs on Windows.

The rest of the sections seem to be "necessary" or "reasonable defaults". For example, I think most people would want to install OpenVPN GUI, add OpenVPN to PATH, add Shortcuts, etc. Enterprise users might want some flexibility, but those should be capable of building their own installers, or to remove / modify the configurations post-install (e.g. using Puppet, PowerShell DSC). Some power users might complain if they lose their checkboxes, but I'm not sure if they really need them, or just like to have the choice.

Alternatively we could keep some flexibility internally in the installer, but hide the sections from view. The user could then use command-line switches to control which components get installed.

Installing everything by default would of course overwrite customized files (e.g. openvpn.exe, openvpn-gui.exe), but that should affect only a very small subset of users who have very special needs.

[selvanair]

I like the option of keeping some flexibility but always installing a wider range of components. As you say, then there is less cause for unchecking one breaking the other. Code-wise its useful to have tasks separated into sections or functions so let's keep all the sections but simplify installation options, by hiding some. The tasks involved falls into 4 groups:

1. Hidden sections (name prefixed with "-": these are always selected) SecOpenUserSpace (with PR58 includes iservice), SecService, SecOpenSSLUtilities, current advanced sections (i.e., dlls): includes setting up registry Rationale --> these are useful for everyone and no reason to unselect those: SecService is debatable, but I would include it [1]
2. Selected by default (but could be unchecked) SecOpenVPNGUI, SecTap Ratioanale --> a power user may want to keep using ndis5 or some oleder tap driver, or a different GUI.
3. Optional (not selected by default) SecOpenVPNEasyRSA (or throw this out?)
4. Advanced (a group of options selected by default and tucked away as advanced) SecAddPath, SecFileAssociation, SecAddShortCuts, LaunchOnLogon Rationale --> though most users need these, some are invasive: overwrites any existing shortcuts to the GUI on desktop, make visible changes to the startmenu etc. So let power users uncheck these if they wish. SecAddShortCutsWorkAround --> this could be removed and relevant code added to other sections?

[1] I suspect no one ever installed with SecService unchecked -- if they did, config and log folders are not created (pre-PR58) but GUI 10 needed those to be present.

[selvanair]

A simplified installer menu could look like this (here UserSpace components is listed for clarity, but grayed out -- its supposed to include all essential stuff: openevpn, dlls and services)

[chipitsine]

as for "I suspect no one ever installed with SecService unchecked -- if they did, config and log folders are not created (pre-PR58) but GUI 10 needed those to be present." .... what if we will write some powershell code (@mattock has created pretty openvpn-test), which will install with various combination of parameters ?

[selvanair]

On Tue, Dec 6, 2016 at 2:20 PM, Ilya Shipitsin [email protected] wrote:

as for "I suspect no one ever installed with SecService unchecked -- if they did, config and log folders are not created (pre-PR58) but GUI 10 needed those to be present." .... what if we will write some powershell code (@mattock https://github.com/mattock has created pretty openvpn-test), which will install with various combination of parameters ?

Why go into all that? Just install everything that a normal user wants and leave commandline install to power users. Keep it simple.

[mattock]

@selvanair : I would add a checkbox for SecService, but select it by default. Then all the bundled non-core OpenVPN projects (openvpn-gui, openvpnserv2, tap-windows6) would be optional. Besides that I'm fine with your proposal.

@chipitsine : later on, when things calm down a bit, I think we could fairly easily add basic installer smoketesting to the openvpn-windows-test project. For example something like this:

1. Installer file is given as an optional command-line parameter
2. OpenVPN version (if any) is uninstalled silently
3. (Ensure that OpenVPN was uninstalled successfully)
4. Install given on the command-line runs silently
5. Basic test run (what we have now)
6. OpenVPN is uninstalled silently
7. (Ensure that OpenVPN was uninstalled successfully)

The other command-line parameters (e.g. TestService, TestGui) could also determine which components would get installed. The "Ensure that OpenVPN was uninstalled successfully" parts can be somewhat tricky to implement, so I would not implement it right away.

[mattock]

Do you guys think the refactorings we've discussed here would be safe to include in OpenVPN 2.4 installers? I actually added that as a topic for today's community meeting.

[selvanair]

@mattock: I think its pretty safe as we are not removing anything, just having less choices. However, if we allow SecService as an option as you suggested (which is a fair idea), we have to stop removing (only stop it) in Section -pre. Such changes would need more testing, so leave for 2.4.1?

Let's first try to get PR58 and .net installation sorted out. Any comments on PR60 which is minor tweak of @chipitsine 's approach to make it less of a blackbox? My only remaining concern about it is how it would handles silent installation. To be safe .NET installation may be disabled IfSilent.

[mattock]

@selvanair : sounds reasonable. I will have a look at PR#60 later today/tomorrow. I agree that in silent installs .NET should not be installed. People who do silent installs should be able to figure out any problems with openvpnserv2.exe.

[selvanair]

@mattock: while at the topic of pending PRs, the active-setup PR must be flawless :-) with the last changes, isn't it? Shall I squash those commits?

EDIT: oh. my bad: it was already squashed..

[selvanair]

On Wed, Dec 7, 2016 at 4:12 AM, Samuli Seppänen [email protected] wrote:

@chipitsine https://github.com/chipitsine : later on, when things calm down a bit, I think we could fairly easily add basic installer smoketesting to the openvpn-windows-test project. For example something like this:

1. Installer file is given as an optional command-line parameter
2. OpenVPN version (if any) is uninstalled silently
3. (Ensure that OpenVPN was uninstalled successfully)
4. Install given on the command-line runs silently
5. Basic test run (what we have now)
6. OpenVPN is uninstalled silently
7. (Ensure that OpenVPN was uninstalled successfully)

Yes this is useful, but getting all possible combinations of command-line options work sensibly is hard to implement and test. We could still allow a power user to enable and disable any section (even hidden ones) on silent install, with the caveat that a combination is possible doesn't mean that it makes sense.

[mattock]

@selvanair : I'm not thinking of covering all the installer options as there are quite a few combinations and we would have to define what "success" or "failure" means in all of them. However, the basic install with default settings -> test -> uninstall process is something I need to do on every release, so automating that makes sense imho.

[mattock]

I issued a PR that fixes the four original issues identified here. One of them had been fixed in an earlier PR from @chipitsine (which I managed to overlook).

[flichtenheld]

We have decided to remove the nsis/mingw buildsystem since it is not maintained anymore. Only the msi/msvc buildsystem will remain, unless someone steps up to maintain the other parts. This issue will be closed when the removal actually happens.

[flichtenheld]

NSIS installer is not supported anymore. Closing issues.