Revert some minor changes

[orazioedoardo]

• Unquoted ${PKG_INSTALL} and ${UPDATE_PKG_CACHE} as they seem to hang the script if quoted.
• Removed this code as set -e is not used in the test branch and even if it were used, the script would exit if debconf-apt-progress's exit code is NOT 100 (which is the case when no errors occur, exit code 0).
• vpnGw="${pivpnNET/.0/.1}" turns 10.8.0.0 into 10.8.1.0 however we need 10.8.0.1 so vpnGw="${pivpnNET/.0.0/.0.1}" is used.
• Create folder /var/lib/openvpn
• Removed code path related to OLD_UFW, originally used for jessie compatibility
• Other trivial changes
  @corbolais what do you think?

[corbolais]

• regarding set -e in test: good catch

the script would exit if debconf-apt-progress's exit code is NOT 100 (which is the case when no errors occur, exit code 0)

• true. ACK.
  Intention was: do NOT exit on return code 100. I got reproducibly ERR 100 but installation and configuration went fine. So, when installing on bullseye/sid the installation of pivpn will simply drop out. That's why for this distribution version a condition is required.

vpnGw="${pivpnNET/.0/.1}" turns 10.8.0.0 into 10.8.1.0 however we need 10.8.0.1 so vpnGw="${pivpnNET/.0.0/.0.1}" is used.

• That is strange. I must have tested this but inserted something else. You are right, of course.

Create folder /var/lib/openvpn

• Good idea. But just omit --no-create-home rather than use mkdir.

Unquoted ${PKG_INSTALL} and ${UPDATE_PKG_CACHE} as they seem to hang the script if quoted.

• Just merge your change. I think this was a shellcheck issue. I'll have a look at it at some point. Would you mind adding a FIXME for me as a reminder at the top of the file?

• scripts/openvpn/pivpnDebug.sh: use pivpnNET/subnetClass instead of IP/24

• I'm not sure how this got left out: c20e7d2#diff-299d9b30c952310aace4aba77c698874L1435 . Good catch.

• server_config.txt: Good catch, again.

• scripts/uninstall.sh: rm -rf Are those really directories? Also, I'm wondering whether it's a good idea to remove old configs or certs (ovpns). Maybe the user needs those but wants to just remove the service or cleanly reinstall it? Touching user's stuff is touchy..

Thank you.

[orazioedoardo]

Intention was: do NOT exit on return code 100. I got reproducibly ERR 100 but installation and configuration went fine. So, when installing on bullseye/sid the installation of pivpn will simply drop out. That's why for this distribution version a condition is required.

Without set -e (default case) it should not exit.

scripts/openvpn/pivpnDebug.sh: use pivpnNET/subnetClass instead of IP/24

I changed uninstall.sh to get started. Will do all other scripts that have /24 hardcoded later. subnetClass, pivpnDEV, pivpnNET, vpnGw could be sourced from setupVars.conf

scripts/uninstall.sh: rm -rf Are those really directories? Also, I'm wondering whether it's a good idea to remove old configs or certs (ovpns). Maybe the user needs those but wants to just remove the service or cleanly reinstall it? Touching user's stuff is touchy..

pivpn/auto_install/install.sh

Line 1401 in ca11f79

$SUDO mkdir -p /etc/wireguard/configs

pivpn/auto_install/install.sh

Line 1403 in ca11f79

$SUDO mkdir -p /etc/wireguard/keys

pivpn/auto_install/install.sh

Line 1268 in ca11f79

$SUDO mv /etc/openvpn/EasyRSA-v${easyrsaVer} /etc/openvpn/easy-rsa

If we delete those three directories as well as openvpn.conf and wg0.conf it makes little sense to retain the client certs and configs, they can't be used anymore.

Lol, wanted to say "Use ${subnetClass} variable, create openvpn home, add shellcheck reminder" as commit message but the shell expanded the variable, should have used single quotes.

[corbolais]

Intention was: do NOT exit on return code 100. I got reproducibly ERR 100 but installation and configuration went fine. So, when installing on bullseye/sid the installation of pivpn will simply drop out. That's why for this distribution version a condition is required.

Without set -e (default case) it should not exit.

Well then, OK.

scripts/openvpn/pivpnDebug.sh: use pivpnNET/subnetClass instead of IP/24

I changed uninstall.sh to get started. Will do all other scripts that have /24 hardcoded later. subnetClass, pivpnDEV, pivpnNET, vpnGw could be sourced from setupVars.conf

scripts/uninstall.sh: rm -rf Are those really directories? Also, I'm wondering whether it's a good idea to remove old configs or certs (ovpns). Maybe the user needs those but wants to just remove the service or cleanly reinstall it? Touching user's stuff is touchy..

pivpn/auto_install/install.sh

Line 1401 in ca11f79

$SUDO mkdir -p /etc/wireguard/configs

pivpn/auto_install/install.sh

Line 1403 in ca11f79

$SUDO mkdir -p /etc/wireguard/keys

pivpn/auto_install/install.sh

Line 1268 in ca11f79

$SUDO mv /etc/openvpn/EasyRSA-v${easyrsaVer} /etc/openvpn/easy-rsa

If we delete those three directories as well as openvpn.conf and wg0.conf it makes little sense to retain the client certs and configs, they can't be used anymore.

So, keep them alltogether. Most likely, we should ask what the user wants us to with them, anyway: Keep || do them away. This is true for any such stuff as rm stuff, fw rules included. But most importantly, discarding user stuff that cannot be restored or recreated is a big nono, IMO. If unsure, let the user clean up on it's own. That way nothing is lost, on our behalves, at the least.

[corbolais]

I changed uninstall.sh to get started. Will do all other scripts that have /24 hardcoded later. subnetClass, pivpnDEV, pivpnNET, vpnGw could be sourced from setupVars.conf

Perfect.

vpnGw would be a candidate for setupVars.conf only if it is customizable and actually implemented in the scripts that way, wouldn't it? Otherwise we'd just derive it from pivpnNET.

Ideally, pivpn would verify identity between settings in setupVars.conf and the actual system configuration, i.e. interfaces, network, netmask, iptables rules etc. That way, we wouldn't be scrapping the users' manual modifications, potentially rendering systems, networks and businesses inoperable. Verify and yell the discrepancies at the user for them to be checked and resolved.

[orazioedoardo]

So, keep them alltogether. Most likely, we should ask what the user wants us to with them, anyway: Keep || do them away.

I mean, take the openvpn server. If we remove the easy-rsa folder, which contains the public key infrastructure, the .ovpn files simply can’t be used because the certificates inside refer to the cerificates in the pki. Same with wireguard. If you delete the server config, which contains public/private keys, then client configs are useless. So why keep them anyways?

This is true for any such stuff as rm stuff, fw rules included.

In my opinion, the uninstall script should undo all changes to the system the install script did. What do you think an uninstall script should do?

vpnGw would be a candidate for setupVars.conf only if it is customizable and actually implemented in the scripts that way, wouldn't it? Otherwise we'd just derive it from pivpnNET.

Apart from subnetClass, all of the above variables can just be derived from the VPN variable. However if we decide not to hardcode CIDRs inside the script it makes sense to me to derive them once and source them in all script that use them (look at the openvpn and wireguard folder).

Ideally, pivpn would verify identity between settings in setupVars.conf and the actual system configuration, i.e. interfaces, network, netmask, iptables rules etc.

Good idea

[corbolais]

So, keep them alltogether. Most likely, we should ask what the user wants us to with them, anyway: Keep || do them away.

I mean, take the openvpn server. If we remove the easy-rsa folder, which contains the public key infrastructure, the .ovpn files simply can’t be used because the certificates inside refer to the cerificates in the pki. Same with wireguard. If you delete the server config, which contains public/private keys, then client configs are useless. So why keep them anyways?

I mean, at least ask and double-check whether the user really intents us to unlink her certs and stuff. I personally prefer having the "old" certs around in a versioned/dated backup-directory for some time longer. It happens that someone only realises the need after days/weeks passed by.

This is true for any such stuff as rm stuff, fw rules included.

In my opinion, the uninstall script should undo all changes to the system the install script did. What do you think an uninstall script should do?

:-D
My point is, we should verify whether what we rm is definitely our stuff and nothing handled by the user. I'd prefer having stuff left around in case. Nothing worse than rm'ing stuff only to realize, wait a minute, where did xy end up!?

Take for example a fw rule: we should verify user doesn't really need it any longer. Ask. Verify it's really the rule we installed etc. Same with files. If in doubt, leave the cruft and tell the user we leave it on purpose because it seems like it's something she might need (maybe).

vpnGw would be a candidate for setupVars.conf only if it is customizable and actually implemented in the scripts that way, wouldn't it? Otherwise we'd just derive it from pivpnNET.

Apart from subnetClass, all of the above variables can just be derived from the VPN variable. However if we decide not to hardcode CIDRs inside the script it makes sense to me to derive them once and source them in all script that use them (look at the openvpn and wireguard folder).

Yes, that's a way. If we verify stuff anyway, we can dump all config in setupVars.conf. Sure thing.

[orazioedoardo]

I mean, at least ask and double-check whether the user really intents us to unlink her certs and stuff. I personally prefer having the "old" certs around in a versioned/dated backup-directory for some time longer. It happens that someone only realises the need after days/weeks passed by.

OK

:-D
My point is, we should verify whether what we rm is definitely our stuff and nothing handled by the user. I'd prefer having stuff left around in case. Nothing worse than rm'ing stuff only to realize, wait a minute, where did xy end up!?
Take for example a fw rule: we should verify user doesn't really need it any longer. Ask. Verify it's really the rule we installed etc. Same with files. If in doubt, leave the cruft and tell the user we leave it on purpose because it seems like it's something she might need (maybe).

Yes delete stuff but make sure that it's actually what pivpn added. We could "watermark" files and configs (for example specific file name, disclaimer inside config files, iptables rule comment).

[corbolais]

Yes delete stuff but make sure that it's actually what pivpn added. We could "watermark" files and configs (for example specific file name, disclaimer inside config files, iptables rule comment).

That's a good point!