[PATCH 1/6] vpnmain.cgi: Fixes bug13737 - remove unneeded &cleanssldatabase calls

[PATCH 1/6] vpnmain.cgi: Fixes bug13737 - remove unneeded &cleanssldatabase calls

[Adolf Belka]

- This first part removes all usages of &cleanssldatabase with the client certificates.
   This is not needed here. If used then the serial number would be moved back to 01 when
   an existing client certificate is removged or a new one created, even if no errors
   occurred.
- The usage of &cleanssldatabase has also been removed from the root/host cert creation
   if it was successful, otherwise the index file is moved back to being empty and the
   serial file to containing 01.
- The only usage now of the &cleanssldatabase is for when the root/host cert set is
   being created or if an uploaded cert has been checked as good to install.
- This now means that each time a new client certificate is created the serial number
   is incremented.
- The removal of the x509 root/host cert also unlinks all .pem files in the certs
   directory and therefore also all the 01.pem, 02.pem etc files so the
   &cleanssldatabase routine no longer needs to unlink the 01.pem file
- The &newcleanssldatabase script is no longer needed, as the &cleanssldatabase commands
   used covers the required cleaning, so it has been removed.
- This patch together with the others from this set have been tested out on my vm system
   and I was able to create a new root/host cert set and then new client certs and make
   an ipsec certificate connection successfully. I could then renew the host cert and
   the client connection still worked.

Fixes: bug13737
Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 html/cgi-bin/vpnmain.cgi | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
index e30506fdf..85119a81d 100644
--- a/html/cgi-bin/vpnmain.cgi
+++ b/html/cgi-bin/vpnmain.cgi
@@ -2,7 +2,7 @@
 ###############################################################################
 #                                                                             #
 # IPFire.org - A linux based firewall                                         #
-# Copyright (C) 2007-2022  IPFire Team  <info@ipfire.org>                     #
+# Copyright (C) 2007-2025  IPFire Team  <info@ipfire.org>                     #
 #                                                                             #
 # This program is free software: you can redistribute it and/or modify        #
 # it under the terms of the GNU General Public License as published by        #
@@ -200,27 +200,6 @@ sub cleanssldatabase {
 	unlink ("${General::swroot}/certs/index.txt.old");
 	unlink ("${General::swroot}/certs/index.txt.attr.old");
 	unlink ("${General::swroot}/certs/serial.old");
-	unlink ("${General::swroot}/certs/01.pem");
-}
-sub newcleanssldatabase {
-	if (! -s "${General::swroot}/certs/serial" ) {
-		open(FILE, ">${General::swroot}/certs/serial");
-		print FILE "01";
-		close FILE;
-	}
-	if (! -s ">${General::swroot}/certs/index.txt") {
-		open(FILE, ">${General::swroot}/certs/index.txt");
-		close(FILE);
-	}
-	if (! -s ">${General::swroot}/certs/index.txt.attr") {
-		open(FILE, ">${General::swroot}/certs/index.txt.attr");
-		print FILE "unique_subject = yes";
-		close(FILE);
-	}
-	unlink ("${General::swroot}/certs/index.txt.old");
-	unlink ("${General::swroot}/certs/index.txt.attr.old");
-	unlink ("${General::swroot}/certs/serial.old");
-#	unlink ("${General::swroot}/certs/01.pem");		numbering evolves. Wrong place to delete
 }
 
 ###
@@ -889,8 +868,6 @@ END
 } elsif ($cgiparams{'ACTION'} eq $Lang::tr{'generate root/host certificates'} ||
 	$cgiparams{'ACTION'} eq $Lang::tr{'upload p12 file'}) {
 
-	&newcleanssldatabase();
-
 	if (-f "${General::swroot}/ca/cacert.pem") {
 		$errormessage = $Lang::tr{'valid root certificate already exists'};
 		goto ROOTCERT_SKIP;
@@ -1004,7 +981,6 @@ END
 		# IPFire can only import certificates
 
 		&General::log("charon", "p12 import completed!");
-		&cleanssldatabase();
 		goto ROOTCERT_SUCCESS;
 
 	} elsif ($cgiparams{'ROOTCERT_COUNTRY'} ne '') {
@@ -1170,7 +1146,6 @@ END
 
 		# Successfully build CA / CERT!
 		if (!$errormessage) {
-			&cleanssldatabase();
 			goto ROOTCERT_SUCCESS;
 		}
 
@@ -1933,11 +1908,9 @@ END
 		if ( $errormessage = &callssl ($opt) ) {
 			unlink ($filename);
 			unlink ("${General::swroot}/certs/$cgiparams{'NAME'}cert.pem");
-			&cleanssldatabase();
 			goto VPNCONF_ERROR;
 		} else {
 			unlink ($filename);
-			&cleanssldatabase();
 		}
 
 		$cgiparams{'CERT_NAME'} = getCNfromcert ("${General::swroot}/certs/$cgiparams{'NAME'}cert.pem");
@@ -2220,7 +2193,6 @@ END
 		} else {
 			unlink ($v3extname);
 			unlink ("${General::swroot}/certs/$cgiparams{'NAME'}req.pem");
-			&cleanssldatabase();
 		}
 
 		# Create the pkcs12 file
-- 
2.49.0

[PATCH 2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate

[Adolf Belka]

- As the serial number is incremented now for each new cert that is created, then when a
   client cert is deleted from the ipsec list in the wui then that cert must be revoked
   otherwise it will still be listed in the .index file as a valid certificate and then
   the certificate name and DN could never be used again.
- Running the revoke command when deleting a client cert leaves the details in the .index
   file but the same name can then be re-used and will get a new serial number etc.

Fixes: bug13737
Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
index 85119a81d..1c9f9243b 100644
--- a/html/cgi-bin/vpnmain.cgi
+++ b/html/cgi-bin/vpnmain.cgi
@@ -1595,17 +1595,25 @@ END
 	&General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
 	&General::readhasharray("${General::swroot}/vpn/config", \%confighash);
 
-	if ($confighash{$cgiparams{'KEY'}}) {
-		unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
-		unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
-		delete $confighash{$cgiparams{'KEY'}};
-		&General::writehasharray("${General::swroot}/vpn/config", \%confighash);
-		&writeipsecfiles();
-		&General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
-	} else {
-		$errormessage = $Lang::tr{'invalid key'};
-	}
-	&General::firewall_reload();
+        if ($confighash{$cgiparams{'KEY'}}) {
+                # Revoke the removed certificate
+                if (!$errormessage) {
+                        &General::log("charon", "Revoking the removed client cert...");
+                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
+                        $errormessage = &callssl($opt);
+                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
+                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
+                        delete $confighash{$cgiparams{'KEY'}};
+                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
+                        &writeipsecfiles();
+                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
+                } else {
+                        goto VPNCONF_ERROR;
+                }
+        } else {
+                $errormessage = $Lang::tr{'invalid key'};
+        }
+        &General::firewall_reload();
 ###
 ### Choose between adding a host-net or net-net connection
 ###
-- 
2.49.0

[PATCH 3/6] include: Add the contents of the ipsec certs directory to the backup

[Adolf Belka]

- Previously only the .pem files were bacdked up from the /var/ipfire/certs/ directory.
   That was okay in the past as the serial and index files never changed after the
   root/host cert set waqs created.
- With the renew process then the serial and index files get updated and these are needed
   to match with the cert status that was backed up. Otherwise you could end up with one
   set of values in the serial and index files that did not match with the restored
   certs.
- This patch adds all the contents of the certs directory to the backup.
- Tested out on my vm testbed and successfully restored a backup and was able to connect
   with the same client settings.

Fixes: bug13737
Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 config/backup/include | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config/backup/include b/config/backup/include
index 0bf9440d3..7e1e9a76a 100644
--- a/config/backup/include
+++ b/config/backup/include
@@ -28,6 +28,7 @@ var/ipfire/backup/addons/backup
 var/ipfire/backup/exclude.user
 var/ipfire/backup/include.user
 var/ipfire/captive/*
+var/ipfire/certs
 var/ipfire/*/*.conf
 var/ipfire/*/config
 var/ipfire/dhcp/*
-- 
2.49.0

[PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Adolf Belka]

- This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
   restart ipsec and ensure that the restored certs are all being used.
- Tested this out on my vm testbed and confirmed that with this I could restore a backup
   and make the client connection as previously set up.
- Without this I had to press the Save button on the ipsec WUI page to get the certs
   etc being used.

Fixes: bug13737
Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 config/backup/backup.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config/backup/backup.pl b/config/backup/backup.pl
index 1c8c87d0a..a6d1467fd 100644
--- a/config/backup/backup.pl
+++ b/config/backup/backup.pl
@@ -307,6 +307,9 @@ restore_backup() {
 	# start collectd after restore
 	/etc/rc.d/init.d/collectd start
 
+	# Reload ipsec certificates and secrets after doing a restore
+	&General::system('/usr/local/bin/ipsecctrl', 'R');
+
 	return 0
 }
 
-- 
2.49.0

[PATCH 5/6] core194: Ship the vpnmain.cgi changes

[Adolf Belka]

Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 config/rootfiles/core/194/filelists/files | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config/rootfiles/core/194/filelists/files b/config/rootfiles/core/194/filelists/files
index e615ef92e..96ac20573 100644
--- a/config/rootfiles/core/194/filelists/files
+++ b/config/rootfiles/core/194/filelists/files
@@ -2,3 +2,4 @@ etc/rc.d/init.d/firewall
 etc/rc.d/init.d/functions
 srv/web/ipfire/cgi-bin/aliases.cgi
 srv/web/ipfire/cgi-bin/pakfire.cgi
+srv/web/ipfire/cgi-bin/vpnmain.cgi
-- 
2.49.0

[PATCH 6/6] core194: Ship the backup file changes

[Adolf Belka]

Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 config/rootfiles/core/194/filelists/files | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config/rootfiles/core/194/filelists/files b/config/rootfiles/core/194/filelists/files
index 96ac20573..9f709db26 100644
--- a/config/rootfiles/core/194/filelists/files
+++ b/config/rootfiles/core/194/filelists/files
@@ -3,3 +3,5 @@ etc/rc.d/init.d/functions
 srv/web/ipfire/cgi-bin/aliases.cgi
 srv/web/ipfire/cgi-bin/pakfire.cgi
 srv/web/ipfire/cgi-bin/vpnmain.cgi
+var/ipfire/backup/include
+var/ipfire/backup/bin/backup.pl
-- 
2.49.0

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Adolf Belka]

Hi Tom,


On 01/04/2025 21:00, Tom Rymes wrote:
> I’m sure that this has been considered already, and Abigail’s is probably a better place to ask, but: Is there a warning of any type so that the user knows existing connections will be dropped, or is this only used in a situation where existing connections would not exist?

I am not sure what "Abigail's" is a reference to.

If you do a restore then currently the root/host certs and the client certs and the PSK secrets etc will all be replaced in the /var/ipfire/vpn/config and /var/ipfire/vpn/ipsec.secrets files anyway. So I have the feeling that you currently could also have a problem if you do a restore from an old backup which had different ipsec client connections.

I am not sure that any existing connections would continue working currently if a backup was restored even if the ipsec daemon was not restarted.

It was just that I found doing an x509 clearout and then doing the restore so all the client certs etc came back, did not give a system where the connections would work.

I will try and find some time to see what happens if a restore on a non x509 cleared system replaces things with an old version but the server is not restarted. Do the existing client connections still work if they were already ongoing, and what happens if I try and newly connect with the previously working connection.

The only alternative would be to not use that patch to do a restart but then after a restore has been done then none of the restored connections will work unless the user remembers to press the save button in the global section of the enabled ipsec wui page. That I definitely know, because I went through a period of restoring the backup and then having the connection from the client continuously failing to work until I though to try out pressing the Save button in the Global options section.

We don't have to press any buttons for restores on any other WUI page so that doesn't seem consistent to me that it would need to be done for the ipsec page restore.

> 
> I’m thinking of a PSK connection that would be reset if you restore certificates, perhaps?

The PSK in the ipsec.secrets file will be replaced with whatever was in the backup being restored. That is the same for all of IPFire. It also happens with OpenVPN, where the existing certs will be cleared out and replaced with the new ones.

I think the user needs to understand what version of backup they are restoring from and that there might be an interruption in service, in the same way as when doing an update and you have to do a reboot. All existing connections will be lost at that point.

I will try and do some testing but I may not have time before I am going off travelling.

Something does need to be done to fix this bug because currently if your host cert expires the renew function is not working properly and will fail and the only option there is to replace the root/host cert, which will also remove all the client connections based on certificates. The PSK connection will stay in place, although I am not sure if it still works with a new root/host x509 cert set or not.

Having said all the above, I need to do a v2 version of this patch 4/6 anyway as I have just noticed that I didn't update the backup.pl with the final version I got working. The patch actually submitted won't work anyway.

Regards,

Adolf.

> 
> Tom
> 
> 
>> On Apr 1, 2025, at 2:08 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>
>> - This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
>>    restart ipsec and ensure that the restored certs are all being used.
>> - Tested this out on my vm testbed and confirmed that with this I could restore a backup
>>    and make the client connection as previously set up.
>> - Without this I had to press the Save button on the ipsec WUI page to get the certs
>>    etc being used.
>>
>> Fixes: bug13737
>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>> ---
>> config/backup/backup.pl | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>> index 1c8c87d0a..a6d1467fd 100644
>> --- a/config/backup/backup.pl
>> +++ b/config/backup/backup.pl
>> @@ -307,6 +307,9 @@ restore_backup() {
>>     # start collectd after restore
>>     /etc/rc.d/init.d/collectd start
>>
>> +    # Reload ipsec certificates and secrets after doing a restore
>> +    &General::system('/usr/local/bin/ipsecctrl', 'R');
>> +
>>     return 0
>> }
>>
>> --
>> 2.49.0
>>
>>

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Adolf Belka]

Hi Tom,

On 01/04/2025 22:44, Adolf Belka wrote:
> Hi Tom,
> 
> 
> On 01/04/2025 21:00, Tom Rymes wrote:
>> I’m sure that this has been considered already, and Abigail’s is probably a better place to ask, but: Is there a warning of any type so that the user knows existing connections will be dropped, or is this only used in a situation where existing connections would not exist?
> 
> I am not sure what "Abigail's" is a reference to.
> 
> If you do a restore then currently the root/host certs and the client certs and the PSK secrets etc will all be replaced in the /var/ipfire/vpn/config and /var/ipfire/vpn/ipsec.secrets files anyway. So I have the feeling that you currently could also have a problem if you do a restore from an old backup which had different ipsec client connections.
> 
> I am not sure that any existing connections would continue working currently if a backup was restored even if the ipsec daemon was not restarted.

I have tested this and the old ipsec certificate connection kept on working, even though the restore had only a PSK connection and no certificate connection.
So not doing a restart would leave the existing connections going but the connections that were restored would not work. So the question would be why do a restore if you don't intend to replace the existing connections.

You don't need to do a restart on OpenVPN to be able to use the restored client connections.

So if I don't do the ipsec restart, then users will need to remember that for IPSec, they have to manually restart the daemon to be able to access the restored connections, but not for OpenVPN.

Seems a bit inconsistent with that approach.

Would be good to get @Michaels input on this.

Regards,

Adolf.

> 
> It was just that I found doing an x509 clearout and then doing the restore so all the client certs etc came back, did not give a system where the connections would work.
> 
> I will try and find some time to see what happens if a restore on a non x509 cleared system replaces things with an old version but the server is not restarted. Do the existing client connections still work if they were already ongoing, and what happens if I try and newly connect with the previously working connection.
> 
> The only alternative would be to not use that patch to do a restart but then after a restore has been done then none of the restored connections will work unless the user remembers to press the save button in the global section of the enabled ipsec wui page. That I definitely know, because I went through a period of restoring the backup and then having the connection from the client continuously failing to work until I though to try out pressing the Save button in the Global options section.
> 
> We don't have to press any buttons for restores on any other WUI page so that doesn't seem consistent to me that it would need to be done for the ipsec page restore.
> 
>>
>> I’m thinking of a PSK connection that would be reset if you restore certificates, perhaps?
> 
> The PSK in the ipsec.secrets file will be replaced with whatever was in the backup being restored. That is the same for all of IPFire. It also happens with OpenVPN, where the existing certs will be cleared out and replaced with the new ones.
> 
> I think the user needs to understand what version of backup they are restoring from and that there might be an interruption in service, in the same way as when doing an update and you have to do a reboot. All existing connections will be lost at that point.
> 
> I will try and do some testing but I may not have time before I am going off travelling.
> 
> Something does need to be done to fix this bug because currently if your host cert expires the renew function is not working properly and will fail and the only option there is to replace the root/host cert, which will also remove all the client connections based on certificates. The PSK connection will stay in place, although I am not sure if it still works with a new root/host x509 cert set or not.
> 
> Having said all the above, I need to do a v2 version of this patch 4/6 anyway as I have just noticed that I didn't update the backup.pl with the final version I got working. The patch actually submitted won't work anyway.
> 
> Regards,
> 
> Adolf.
> 
>>
>> Tom
>>
>>
>>> On Apr 1, 2025, at 2:08 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>>
>>> - This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
>>>    restart ipsec and ensure that the restored certs are all being used.
>>> - Tested this out on my vm testbed and confirmed that with this I could restore a backup
>>>    and make the client connection as previously set up.
>>> - Without this I had to press the Save button on the ipsec WUI page to get the certs
>>>    etc being used.
>>>
>>> Fixes: bug13737
>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>> ---
>>> config/backup/backup.pl | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>> index 1c8c87d0a..a6d1467fd 100644
>>> --- a/config/backup/backup.pl
>>> +++ b/config/backup/backup.pl
>>> @@ -307,6 +307,9 @@ restore_backup() {
>>>     # start collectd after restore
>>>     /etc/rc.d/init.d/collectd start
>>>
>>> +    # Reload ipsec certificates and secrets after doing a restore
>>> +    &General::system('/usr/local/bin/ipsecctrl', 'R');
>>> +
>>>     return 0
>>> }
>>>
>>> -- 
>>> 2.49.0
>>>
>>>
> 

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Tom Rymes]

Adolf,

I wouldn’t worry about any additional testing. So long as it’s obvious to the user that any established tunnels will be torn down and re-established, that’s all I was worried about.

It sounds (as I suspected) as if the nature of the operation (restoring a backup) should make this obvious.

Lastly, “Abagail” was just autocorrect’s fun way of changing “Bugzilla”, for whatever reason, so please disregard.

Tom

> On Apr 1, 2025, at 4:44 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi Tom,
> 
> 
>> On 01/04/2025 21:00, Tom Rymes wrote:
>> I’m sure that this has been considered already, and Abigail’s is probably a better place to ask, but: Is there a warning of any type so that the user knows existing connections will be dropped, or is this only used in a situation where existing connections would not exist?
> 
> I am not sure what "Abigail's" is a reference to.
> 
> If you do a restore then currently the root/host certs and the client certs and the PSK secrets etc will all be replaced in the /var/ipfire/vpn/config and /var/ipfire/vpn/ipsec.secrets files anyway. So I have the feeling that you currently could also have a problem if you do a restore from an old backup which had different ipsec client connections.
> 
> I am not sure that any existing connections would continue working currently if a backup was restored even if the ipsec daemon was not restarted.
> 
> It was just that I found doing an x509 clearout and then doing the restore so all the client certs etc came back, did not give a system where the connections would work.
> 
> I will try and find some time to see what happens if a restore on a non x509 cleared system replaces things with an old version but the server is not restarted. Do the existing client connections still work if they were already ongoing, and what happens if I try and newly connect with the previously working connection.
> 
> The only alternative would be to not use that patch to do a restart but then after a restore has been done then none of the restored connections will work unless the user remembers to press the save button in the global section of the enabled ipsec wui page. That I definitely know, because I went through a period of restoring the backup and then having the connection from the client continuously failing to work until I though to try out pressing the Save button in the Global options section.
> 
> We don't have to press any buttons for restores on any other WUI page so that doesn't seem consistent to me that it would need to be done for the ipsec page restore.
> 
>> I’m thinking of a PSK connection that would be reset if you restore certificates, perhaps?
> 
> The PSK in the ipsec.secrets file will be replaced with whatever was in the backup being restored. That is the same for all of IPFire. It also happens with OpenVPN, where the existing certs will be cleared out and replaced with the new ones.
> 
> I think the user needs to understand what version of backup they are restoring from and that there might be an interruption in service, in the same way as when doing an update and you have to do a reboot. All existing connections will be lost at that point.
> 
> I will try and do some testing but I may not have time before I am going off travelling.
> 
> Something does need to be done to fix this bug because currently if your host cert expires the renew function is not working properly and will fail and the only option there is to replace the root/host cert, which will also remove all the client connections based on certificates. The PSK connection will stay in place, although I am not sure if it still works with a new root/host x509 cert set or not.
> 
> Having said all the above, I need to do a v2 version of this patch 4/6 anyway as I have just noticed that I didn't update the backup.pl with the final version I got working. The patch actually submitted won't work anyway.
> 
> Regards,
> 
> Adolf.
> 
>> Tom
>>>> On Apr 1, 2025, at 2:08 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>> 
>>> - This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
>>>   restart ipsec and ensure that the restored certs are all being used.
>>> - Tested this out on my vm testbed and confirmed that with this I could restore a backup
>>>   and make the client connection as previously set up.
>>> - Without this I had to press the Save button on the ipsec WUI page to get the certs
>>>   etc being used.
>>> 
>>> Fixes: bug13737
>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>> ---
>>> config/backup/backup.pl | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>> index 1c8c87d0a..a6d1467fd 100644
>>> --- a/config/backup/backup.pl
>>> +++ b/config/backup/backup.pl
>>> @@ -307,6 +307,9 @@ restore_backup() {
>>>    # start collectd after restore
>>>    /etc/rc.d/init.d/collectd start
>>> 
>>> +    # Reload ipsec certificates and secrets after doing a restore
>>> +    &General::system('/usr/local/bin/ipsecctrl', 'R');
>>> +
>>>    return 0
>>> }
>>> 
>>> --
>>> 2.49.0
>>> 
>>> 
> 

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Tom Rymes]

> On Apr 1, 2025, at 5:47 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:

<snip>

> I have tested this and the old ipsec certificate connection kept on working, even though the restore had only a PSK connection and no certificate connection.
> So not doing a restart would leave the existing connections going but the connections that were restored would not work. So the question would be why do a restore if you don't intend to replace the existing connections.

The restart seems wise to me. My only concern was that the user should be aware that initiating the process will cause existing connections to be dropped. Perhaps this could be an explicit notice to the user, or perhaps that’s unneeded because it should be obvious.

Tom

Re: [PATCH 2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate

[Michael Tremer]

> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> - As the serial number is incremented now for each new cert that is created, then when a
>   client cert is deleted from the ipsec list in the wui then that cert must be revoked
>   otherwise it will still be listed in the .index file as a valid certificate and then
>   the certificate name and DN could never be used again.
> - Running the revoke command when deleting a client cert leaves the details in the .index
>   file but the same name can then be re-used and will get a new serial number etc.
> 
> Fixes: bug13737
> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
> ---
> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
> index 85119a81d..1c9f9243b 100644
> --- a/html/cgi-bin/vpnmain.cgi
> +++ b/html/cgi-bin/vpnmain.cgi
> @@ -1595,17 +1595,25 @@ END
> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
> 
> - if ($confighash{$cgiparams{'KEY'}}) {
> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
> - delete $confighash{$cgiparams{'KEY'}};
> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
> - &writeipsecfiles();
> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
> - } else {
> - $errormessage = $Lang::tr{'invalid key'};
> - }
> - &General::firewall_reload();
> +        if ($confighash{$cgiparams{'KEY'}}) {
> +                # Revoke the removed certificate
> +                if (!$errormessage) {
> +                        &General::log("charon", "Revoking the removed client cert...");
> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
> +                        $errormessage = &callssl($opt);

This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.

Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.

> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
> +                        delete $confighash{$cgiparams{'KEY'}};
> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
> +                        &writeipsecfiles();
> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
> +                } else {
> +                        goto VPNCONF_ERROR;
> +                }
> +        } else {
> +                $errormessage = $Lang::tr{'invalid key'};
> +        }
> +        &General::firewall_reload();
> ###
> ### Choose between adding a host-net or net-net connection
> ###
> -- 
> 2.49.0
> 
> 

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Adolf Belka]

Hi Tom,

On 01/04/2025 23:52, Tom Rymes wrote:
> Adolf,
> 
> I wouldn’t worry about any additional testing. So long as it’s obvious to the user that any established tunnels will be torn down and re-established, that’s all I was worried about.
> 
> It sounds (as I suspected) as if the nature of the operation (restoring a backup) should make this obvious.
> 
> Lastly, “Abagail” was just autocorrect’s fun way of changing “Bugzilla”, for whatever reason, so please disregard.

LOL. I have suffered some of those on my phone but that one is a beauty.

Regards,

Adolf.

> 
> Tom
> 
>> On Apr 1, 2025, at 4:44 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>
>> Hi Tom,
>>
>>
>>> On 01/04/2025 21:00, Tom Rymes wrote:
>>> I’m sure that this has been considered already, and Abigail’s is probably a better place to ask, but: Is there a warning of any type so that the user knows existing connections will be dropped, or is this only used in a situation where existing connections would not exist?
>>
>> I am not sure what "Abigail's" is a reference to.
>>
>> If you do a restore then currently the root/host certs and the client certs and the PSK secrets etc will all be replaced in the /var/ipfire/vpn/config and /var/ipfire/vpn/ipsec.secrets files anyway. So I have the feeling that you currently could also have a problem if you do a restore from an old backup which had different ipsec client connections.
>>
>> I am not sure that any existing connections would continue working currently if a backup was restored even if the ipsec daemon was not restarted.
>>
>> It was just that I found doing an x509 clearout and then doing the restore so all the client certs etc came back, did not give a system where the connections would work.
>>
>> I will try and find some time to see what happens if a restore on a non x509 cleared system replaces things with an old version but the server is not restarted. Do the existing client connections still work if they were already ongoing, and what happens if I try and newly connect with the previously working connection.
>>
>> The only alternative would be to not use that patch to do a restart but then after a restore has been done then none of the restored connections will work unless the user remembers to press the save button in the global section of the enabled ipsec wui page. That I definitely know, because I went through a period of restoring the backup and then having the connection from the client continuously failing to work until I though to try out pressing the Save button in the Global options section.
>>
>> We don't have to press any buttons for restores on any other WUI page so that doesn't seem consistent to me that it would need to be done for the ipsec page restore.
>>
>>> I’m thinking of a PSK connection that would be reset if you restore certificates, perhaps?
>>
>> The PSK in the ipsec.secrets file will be replaced with whatever was in the backup being restored. That is the same for all of IPFire. It also happens with OpenVPN, where the existing certs will be cleared out and replaced with the new ones.
>>
>> I think the user needs to understand what version of backup they are restoring from and that there might be an interruption in service, in the same way as when doing an update and you have to do a reboot. All existing connections will be lost at that point.
>>
>> I will try and do some testing but I may not have time before I am going off travelling.
>>
>> Something does need to be done to fix this bug because currently if your host cert expires the renew function is not working properly and will fail and the only option there is to replace the root/host cert, which will also remove all the client connections based on certificates. The PSK connection will stay in place, although I am not sure if it still works with a new root/host x509 cert set or not.
>>
>> Having said all the above, I need to do a v2 version of this patch 4/6 anyway as I have just noticed that I didn't update the backup.pl with the final version I got working. The patch actually submitted won't work anyway.
>>
>> Regards,
>>
>> Adolf.
>>
>>> Tom
>>>>> On Apr 1, 2025, at 2:08 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>>>
>>>> - This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
>>>>    restart ipsec and ensure that the restored certs are all being used.
>>>> - Tested this out on my vm testbed and confirmed that with this I could restore a backup
>>>>    and make the client connection as previously set up.
>>>> - Without this I had to press the Save button on the ipsec WUI page to get the certs
>>>>    etc being used.
>>>>
>>>> Fixes: bug13737
>>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>>> ---
>>>> config/backup/backup.pl | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>>> index 1c8c87d0a..a6d1467fd 100644
>>>> --- a/config/backup/backup.pl
>>>> +++ b/config/backup/backup.pl
>>>> @@ -307,6 +307,9 @@ restore_backup() {
>>>>     # start collectd after restore
>>>>     /etc/rc.d/init.d/collectd start
>>>>
>>>> +    # Reload ipsec certificates and secrets after doing a restore
>>>> +    &General::system('/usr/local/bin/ipsecctrl', 'R');
>>>> +
>>>>     return 0
>>>> }
>>>>
>>>> --
>>>> 2.49.0
>>>>
>>>>
>>
> 

Re: [PATCH 4/6] backup.pl: Fixes bug13737 - restarts ipsec to use the restored certs etc

[Michael Tremer]

I call things wrong all of the time, and I am very sure it is not me, but auto-correct.

It happens, but this one was indeed a riddle that was very hard to solve.

> On 2 Apr 2025, at 11:24, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi Tom,
> 
> On 01/04/2025 23:52, Tom Rymes wrote:
>> Adolf,
>> I wouldn’t worry about any additional testing. So long as it’s obvious to the user that any established tunnels will be torn down and re-established, that’s all I was worried about.
>> It sounds (as I suspected) as if the nature of the operation (restoring a backup) should make this obvious.
>> Lastly, “Abagail” was just autocorrect’s fun way of changing “Bugzilla”, for whatever reason, so please disregard.
> 
> LOL. I have suffered some of those on my phone but that one is a beauty.
> 
> Regards,
> 
> Adolf.
> 
>> Tom
>>> On Apr 1, 2025, at 4:44 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>> 
>>> Hi Tom,
>>> 
>>> 
>>>> On 01/04/2025 21:00, Tom Rymes wrote:
>>>> I’m sure that this has been considered already, and Abigail’s is probably a better place to ask, but: Is there a warning of any type so that the user knows existing connections will be dropped, or is this only used in a situation where existing connections would not exist?
>>> 
>>> I am not sure what "Abigail's" is a reference to.
>>> 
>>> If you do a restore then currently the root/host certs and the client certs and the PSK secrets etc will all be replaced in the /var/ipfire/vpn/config and /var/ipfire/vpn/ipsec.secrets files anyway. So I have the feeling that you currently could also have a problem if you do a restore from an old backup which had different ipsec client connections.
>>> 
>>> I am not sure that any existing connections would continue working currently if a backup was restored even if the ipsec daemon was not restarted.
>>> 
>>> It was just that I found doing an x509 clearout and then doing the restore so all the client certs etc came back, did not give a system where the connections would work.
>>> 
>>> I will try and find some time to see what happens if a restore on a non x509 cleared system replaces things with an old version but the server is not restarted. Do the existing client connections still work if they were already ongoing, and what happens if I try and newly connect with the previously working connection.
>>> 
>>> The only alternative would be to not use that patch to do a restart but then after a restore has been done then none of the restored connections will work unless the user remembers to press the save button in the global section of the enabled ipsec wui page. That I definitely know, because I went through a period of restoring the backup and then having the connection from the client continuously failing to work until I though to try out pressing the Save button in the Global options section.
>>> 
>>> We don't have to press any buttons for restores on any other WUI page so that doesn't seem consistent to me that it would need to be done for the ipsec page restore.
>>> 
>>>> I’m thinking of a PSK connection that would be reset if you restore certificates, perhaps?
>>> 
>>> The PSK in the ipsec.secrets file will be replaced with whatever was in the backup being restored. That is the same for all of IPFire. It also happens with OpenVPN, where the existing certs will be cleared out and replaced with the new ones.
>>> 
>>> I think the user needs to understand what version of backup they are restoring from and that there might be an interruption in service, in the same way as when doing an update and you have to do a reboot. All existing connections will be lost at that point.
>>> 
>>> I will try and do some testing but I may not have time before I am going off travelling.
>>> 
>>> Something does need to be done to fix this bug because currently if your host cert expires the renew function is not working properly and will fail and the only option there is to replace the root/host cert, which will also remove all the client connections based on certificates. The PSK connection will stay in place, although I am not sure if it still works with a new root/host x509 cert set or not.
>>> 
>>> Having said all the above, I need to do a v2 version of this patch 4/6 anyway as I have just noticed that I didn't update the backup.pl with the final version I got working. The patch actually submitted won't work anyway.
>>> 
>>> Regards,
>>> 
>>> Adolf.
>>> 
>>>> Tom
>>>>>> On Apr 1, 2025, at 2:08 PM, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>>>> 
>>>>> - This adds a check if the ipsec server is enabled. If it is then ipsecctrl is run to
>>>>>   restart ipsec and ensure that the restored certs are all being used.
>>>>> - Tested this out on my vm testbed and confirmed that with this I could restore a backup
>>>>>   and make the client connection as previously set up.
>>>>> - Without this I had to press the Save button on the ipsec WUI page to get the certs
>>>>>   etc being used.
>>>>> 
>>>>> Fixes: bug13737
>>>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>>>> ---
>>>>> config/backup/backup.pl | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>> 
>>>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>>>> index 1c8c87d0a..a6d1467fd 100644
>>>>> --- a/config/backup/backup.pl
>>>>> +++ b/config/backup/backup.pl
>>>>> @@ -307,6 +307,9 @@ restore_backup() {
>>>>>    # start collectd after restore
>>>>>    /etc/rc.d/init.d/collectd start
>>>>> 
>>>>> +    # Reload ipsec certificates and secrets after doing a restore
>>>>> +    &General::system('/usr/local/bin/ipsecctrl', 'R');
>>>>> +
>>>>>    return 0
>>>>> }
>>>>> 
>>>>> --
>>>>> 2.49.0
>>>>> 
>>>>> 
>>> 
> 
> 

Re: [PATCH 2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate

[Adolf Belka]

Hi Michael,

On 02/04/2025 12:21, Michael Tremer wrote:
> 
> 
>> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>
>> - As the serial number is incremented now for each new cert that is created, then when a
>>    client cert is deleted from the ipsec list in the wui then that cert must be revoked
>>    otherwise it will still be listed in the .index file as a valid certificate and then
>>    the certificate name and DN could never be used again.
>> - Running the revoke command when deleting a client cert leaves the details in the .index
>>    file but the same name can then be re-used and will get a new serial number etc.
>>
>> Fixes: bug13737
>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>> ---
>> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
>> index 85119a81d..1c9f9243b 100644
>> --- a/html/cgi-bin/vpnmain.cgi
>> +++ b/html/cgi-bin/vpnmain.cgi
>> @@ -1595,17 +1595,25 @@ END
>> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
>> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
>>
>> - if ($confighash{$cgiparams{'KEY'}}) {
>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>> - delete $confighash{$cgiparams{'KEY'}};
>> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>> - &writeipsecfiles();
>> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>> - } else {
>> - $errormessage = $Lang::tr{'invalid key'};
>> - }
>> - &General::firewall_reload();
>> +        if ($confighash{$cgiparams{'KEY'}}) {
>> +                # Revoke the removed certificate
>> +                if (!$errormessage) {
>> +                        &General::log("charon", "Revoking the removed client cert...");
>> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
>> +                        $errormessage = &callssl($opt);
> 
> This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.
> 
> Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.

I will look at doing that. However it might have to wait till I am back from visiting my family. I might be able to do it while visiting my son but can't be sure I will get the time.

Will put it on my list.

Regards,

Adolf.

> 
>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>> +                        delete $confighash{$cgiparams{'KEY'}};
>> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>> +                        &writeipsecfiles();
>> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>> +                } else {
>> +                        goto VPNCONF_ERROR;
>> +                }
>> +        } else {
>> +                $errormessage = $Lang::tr{'invalid key'};
>> +        }
>> +        &General::firewall_reload();
>> ###
>> ### Choose between adding a host-net or net-net connection
>> ###
>> -- 
>> 2.49.0
>>
>>
> 
> 

Re: [PATCH 2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate

[Michael Tremer]

Hello Adolf,

> On 2 Apr 2025, at 11:41, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 02/04/2025 12:21, Michael Tremer wrote:
>>> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>> 
>>> - As the serial number is incremented now for each new cert that is created, then when a
>>>   client cert is deleted from the ipsec list in the wui then that cert must be revoked
>>>   otherwise it will still be listed in the .index file as a valid certificate and then
>>>   the certificate name and DN could never be used again.
>>> - Running the revoke command when deleting a client cert leaves the details in the .index
>>>   file but the same name can then be re-used and will get a new serial number etc.
>>> 
>>> Fixes: bug13737
>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>> ---
>>> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
>>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
>>> index 85119a81d..1c9f9243b 100644
>>> --- a/html/cgi-bin/vpnmain.cgi
>>> +++ b/html/cgi-bin/vpnmain.cgi
>>> @@ -1595,17 +1595,25 @@ END
>>> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
>>> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
>>> 
>>> - if ($confighash{$cgiparams{'KEY'}}) {
>>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>>> - delete $confighash{$cgiparams{'KEY'}};
>>> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>>> - &writeipsecfiles();
>>> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>>> - } else {
>>> - $errormessage = $Lang::tr{'invalid key'};
>>> - }
>>> - &General::firewall_reload();
>>> +        if ($confighash{$cgiparams{'KEY'}}) {
>>> +                # Revoke the removed certificate
>>> +                if (!$errormessage) {
>>> +                        &General::log("charon", "Revoking the removed client cert...");
>>> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
>>> +                        $errormessage = &callssl($opt);
>> This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.
>> Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.
> 
> I will look at doing that. However it might have to wait till I am back from visiting my family. I might be able to do it while visiting my son but can't be sure I will get the time.

No rush, this is not that urgent, but of course we should not forget about it…

-Michael

> 
> Will put it on my list.
> 
> Regards,
> 
> Adolf.
> 
>>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>>> +                        delete $confighash{$cgiparams{'KEY'}};
>>> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>>> +                        &writeipsecfiles();
>>> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>>> +                } else {
>>> +                        goto VPNCONF_ERROR;
>>> +                }
>>> +        } else {
>>> +                $errormessage = $Lang::tr{'invalid key'};
>>> +        }
>>> +        &General::firewall_reload();
>>> ###
>>> ### Choose between adding a host-net or net-net connection
>>> ###
>>> -- 
>>> 2.49.0