[PATCH 1/3] ovpnmain.cgi: Updated fix for Bug#13137

[PATCH 1/3] ovpnmain.cgi: Updated fix for Bug#13137

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 7324 bytes --]

- This now only adds "providers legacy default" to the config files of connections that
   have legacy certificates, both for n2n and roadwarrior.
- This new approach also removes the requirement to have code in the update.sh script
   or in backup.pl so those earlier modifications are removed in two additional patches
   combined with this one in a set.
- The -legacy option has been removed from the pkcs12 creation part of the code as
   otherwise this creates a certificate in legacy format, which is not wanted. All new
   connection certificates being created will be based on openssl-3.x

Fixes: Bug#13137
Suggested-by: Michael Tremer <michael.tremer(a)ipfire.org>
Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
---
 html/cgi-bin/ovpnmain.cgi | 70 ++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 15 deletions(-)

diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
index 88106251e..a210e0509 100755
--- a/html/cgi-bin/ovpnmain.cgi
+++ b/html/cgi-bin/ovpnmain.cgi
@@ -138,6 +138,17 @@ unless (-e "$local_clientconf") {
 ###
 ### Useful functions
 ###
+sub iscertlegacy
+{
+	my $file=$_[0];
+	my @certinfo = &General::system_output("/usr/bin/openssl", "pkcs12", "-info", "-nodes", 
+	"-in", "$file.p12", "-noout", "-passin", "pass:''");
+	if (index ($certinfo[0], "MAC: sha1") != -1) {
+		return 0;
+	}
+	return 1;
+}
+
 sub haveOrangeNet
 {
 	if ($netsettings{'CONFIG_TYPE'} == 2) {return 1;}
@@ -1115,7 +1126,9 @@ unless(-d "${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}"){mkdir "${General
   print CLIENTCONF "# Activate Management Interface and Port\n";
   if ($cgiparams{'OVPN_MGMT'} eq '') {print CLIENTCONF "management localhost $cgiparams{'DEST_PORT'}\n"}
   else {print CLIENTCONF "management localhost $cgiparams{'OVPN_MGMT'}\n"};
-  print CLIENTCONF "providers legacy default\n";
+  if (&iscertlegacy("${General::swroot}/ovpn/certs/$cgiparams{'NAME'}")) {
+    	print CLIENTCONF "providers legacy default\n";
+  }
   close(CLIENTCONF);
 
 }
@@ -1649,7 +1662,7 @@ END
 		goto ROOTCERT_ERROR;
 	    }
 	} else {	# child
-	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-cacerts', '-nokeys',
+	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-cacerts', '-nokeys',
 		    '-in', $filename,
 		    '-out', "$tempdir/cacert.pem")) {
 		$errormessage = "$Lang::tr{'cant start openssl'}: $!";
@@ -1672,7 +1685,7 @@ END
 		goto ROOTCERT_ERROR;
 	    }
 	} else {	# child
-	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-clcerts', '-nokeys',
+	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-clcerts', '-nokeys',
 		    '-in', $filename,
 		    '-out', "$tempdir/hostcert.pem")) {
 		$errormessage = "$Lang::tr{'cant start openssl'}: $!";
@@ -1695,7 +1708,7 @@ END
 		goto ROOTCERT_ERROR;
 	    }
 	} else {	# child
-	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-nocerts',
+	    unless (exec ('/usr/bin/openssl', 'pkcs12', '-nocerts',
 		    '-nodes',
 		    '-in', $filename,
 		    '-out', "$tempdir/serverkey.pem")) {
@@ -2157,7 +2170,10 @@ if ($confighash{$cgiparams{'KEY'}}[3] eq 'net'){
    if ($confighash{$cgiparams{'KEY'}}[22] eq '') {print CLIENTCONF "management localhost $confighash{$cgiparams{'KEY'}}[29]\n"}
     else {print CLIENTCONF "management localhost $confighash{$cgiparams{'KEY'}}[22]\n"};
    print CLIENTCONF "# remsub $confighash{$cgiparams{'KEY'}}[11]\n";
-   print CLIENTCONF "providers legacy default\n";
+  if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
+    	print CLIENTCONF "providers legacy default\n";
+  }
+
 
 
     close(CLIENTCONF);
@@ -2229,10 +2245,18 @@ else
 
 		# Extract the certificate
 		# This system call is safe, because all arguments are passed as an array.
-		system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
-			'-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
-		if ($?) {
-			die "openssl error: $?";
+		if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
+			system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
+				'-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
+			if ($?) {
+				die "openssl error: $?";
+			}
+		} else {
+			system('/usr/bin/openssl', 'pkcs12', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
+				'-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
+			if ($?) {
+				die "openssl error: $?";
+			}
 		}
 
 		$zip->addFile("$file_crt", "$confighash{$cgiparams{'KEY'}}[1].pem") or die;
@@ -2240,10 +2264,18 @@ else
 
 		# Extract the key
 		# This system call is safe, because all arguments are passed as an array.
-		system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
-			'-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
-		if ($?) {
-			die "openssl error: $?";
+		if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
+			system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
+				'-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
+			if ($?) {
+				die "openssl error: $?";
+			}
+		} else {
+			system('/usr/bin/openssl', 'pkcs12', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
+				'-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
+			if ($?) {
+				die "openssl error: $?";
+			}
 		}
 
 		$zip->addFile("$file_key", "$confighash{$cgiparams{'KEY'}}[1].key") or die;
@@ -2302,6 +2334,11 @@ else
     # If the server is asking for TOTP this needs to happen interactively
     print CLIENTCONF "auth-retry interact\r\n";
 
+    # Add provider line if certificate is legacy type
+    if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
+	print CLIENTCONF "providers legacy default\r\n";
+    }
+
     if ($include_certs) {
 	print CLIENTCONF "\r\n";
 
@@ -3298,7 +3335,10 @@ END
 	print FILE "# Logfile\n";
 	print FILE "status-version 1\n";
 	print FILE "status /var/run/openvpn/$n2nname[0]-n2n 10\n";
-	print FILE "providers legacy default\n";
+	if (&iscertlegacy("${General::swroot}/ovpn/certs/$cgiparams{'n2nname'}")) {
+	    	print CLIENTCONF "providers legacy default\n";
+	}
+
 	close FILE;
 
 	unless(move("$tempdir/$uplconffilename", "${General::swroot}/ovpn/n2nconf/$n2nname[0]/$uplconffilename2")) {
@@ -4245,7 +4285,7 @@ if ($cgiparams{'TYPE'} eq 'net') {
 
 	    # Create the pkcs12 file
 	    # The system call is safe, because all arguments are passed as an array.
-	    system('/usr/bin/openssl', 'pkcs12', '-legacy', '-export',
+	    system('/usr/bin/openssl', 'pkcs12', '-export',
 		'-inkey', "${General::swroot}/ovpn/certs/$cgiparams{'NAME'}key.pem",
 		'-in', "${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem",
 		'-name', $cgiparams{'NAME'},
-- 
2.40.1

[PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

- This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.

Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
---
 config/backup/backup.pl | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/config/backup/backup.pl b/config/backup/backup.pl
index 8d990c0f1..60138a58a 100644
--- a/config/backup/backup.pl
+++ b/config/backup/backup.pl
@@ -190,21 +190,6 @@ restore_backup() {
 	# Update OpenVPN CRL
 	/etc/fcron.daily/openvpn-crl-updater
 	
-	# Update OpenVPN N2N Client Configs
-	## Add providers legacy default line to n2n client config files
-	# Check if ovpnconfig exists and is not empty
-	if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
-	       # Identify all n2n connections
-	       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
-	           # Add the legacy option to all N2N client conf files if it does not already exist
-			if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
-				if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
-					echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
-				fi
-			fi
-	       done
-	fi
-
 	return 0
 }
 
-- 
2.40.1

Re: [PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

I did not merge this, as I believe we need this, because:

We won’t rewrite the OpenVPN configuration files on update, so it might be a good idea to just add the line and if someone edits the connection it might be removed.

That should work I believe and -legacy should not have any side effects when enabled but not needed.

Best,
-Michael

> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> - This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.
> 
> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
> ---
> config/backup/backup.pl | 15 ---------------
> 1 file changed, 15 deletions(-)
> 
> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
> index 8d990c0f1..60138a58a 100644
> --- a/config/backup/backup.pl
> +++ b/config/backup/backup.pl
> @@ -190,21 +190,6 @@ restore_backup() {
> # Update OpenVPN CRL
> /etc/fcron.daily/openvpn-crl-updater
> 
> - # Update OpenVPN N2N Client Configs
> - ## Add providers legacy default line to n2n client config files
> - # Check if ovpnconfig exists and is not empty
> - if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
> -       # Identify all n2n connections
> -       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
> -           # Add the legacy option to all N2N client conf files if it does not already exist
> - if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
> - if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
> - echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
> - fi
> - fi
> -       done
> - fi
> -
> return 0
> }
> 
> -- 
> 2.40.1
> 

Re: [PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

Hi Michael,

On 10/06/2023 12:16, Michael Tremer wrote:
> I did not merge this, as I believe we need this, because:
> 
> We won’t rewrite the OpenVPN configuration files on update, so it might be a good idea to just add the line and if someone edits the connection it might be removed.
The code in the backup.pl put the line into the config irrespective of 
the certificate being legacy or not.

With the ovpnmain.cgi code patch of this patch set, it now only adds the 
providers legacy default to the config file if the cert is legacy when 
downloading the connection set. This is now done for both n2n and 
roadwarrior connection sets.
> 
> That should work I believe and -legacy should not have any side effects when enabled but not needed.
That is something I have not tested out but I think you are correct, it 
shouldn't have any side affects.

I think it is good to go now and I can always do any additional minor 
tunings later in CU176 and onwards, otherwise we will be here for ever.

Regards,

Adolf.
> 
> Best,
> -Michael
> 
>> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>
>> - This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.
>>
>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> ---
>> config/backup/backup.pl | 15 ---------------
>> 1 file changed, 15 deletions(-)
>>
>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>> index 8d990c0f1..60138a58a 100644
>> --- a/config/backup/backup.pl
>> +++ b/config/backup/backup.pl
>> @@ -190,21 +190,6 @@ restore_backup() {
>> # Update OpenVPN CRL
>> /etc/fcron.daily/openvpn-crl-updater
>>
>> - # Update OpenVPN N2N Client Configs
>> - ## Add providers legacy default line to n2n client config files
>> - # Check if ovpnconfig exists and is not empty
>> - if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
>> -       # Identify all n2n connections
>> -       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
>> -           # Add the legacy option to all N2N client conf files if it does not already exist
>> - if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
>> - if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
>> - echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
>> - fi
>> - fi
>> -       done
>> - fi
>> -
>> return 0
>> }
>>
>> -- 
>> 2.40.1
>>
> 

-- 
Sent from my laptop

Re: [PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

Hello,

> On 10 Jun 2023, at 12:16, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 10/06/2023 12:16, Michael Tremer wrote:
>> I did not merge this, as I believe we need this, because:
>> We won’t rewrite the OpenVPN configuration files on update, so it might be a good idea to just add the line and if someone edits the connection it might be removed.
> The code in the backup.pl put the line into the config irrespective of the certificate being legacy or not.
> 
> With the ovpnmain.cgi code patch of this patch set, it now only adds the providers legacy default to the config file if the cert is legacy when downloading the connection set. This is now done for both n2n and roadwarrior connection sets.

Yes, this is true, but we won’t run the CGI during the update.

Any connections that have legacy certificates won’t work after installing the new version of OpenSSL. So we need the legacy provider enabled (just to be safe).

>> That should work I believe and -legacy should not have any side effects when enabled but not needed.
> That is something I have not tested out but I think you are correct, it shouldn't have any side affects.
> 
> I think it is good to go now and I can always do any additional minor tunings later in CU176 and onwards, otherwise we will be here for ever.

I would rather like to get it right than being fast, but at this point I don’t know what else we can do. So *fingers crossed*.

Let’s release either tomorrow or Monday. Depending on how much I am going to enjoy the nice weather this weekend :)

-Michael

> 
> Regards,
> 
> Adolf.
>> Best,
>> -Michael
>>> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>> 
>>> - This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.
>>> 
>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> ---
>>> config/backup/backup.pl | 15 ---------------
>>> 1 file changed, 15 deletions(-)
>>> 
>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>> index 8d990c0f1..60138a58a 100644
>>> --- a/config/backup/backup.pl
>>> +++ b/config/backup/backup.pl
>>> @@ -190,21 +190,6 @@ restore_backup() {
>>> # Update OpenVPN CRL
>>> /etc/fcron.daily/openvpn-crl-updater
>>> 
>>> - # Update OpenVPN N2N Client Configs
>>> - ## Add providers legacy default line to n2n client config files
>>> - # Check if ovpnconfig exists and is not empty
>>> - if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
>>> -       # Identify all n2n connections
>>> -       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
>>> -           # Add the legacy option to all N2N client conf files if it does not already exist
>>> - if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
>>> - if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
>>> - echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
>>> - fi
>>> - fi
>>> -       done
>>> - fi
>>> -
>>> return 0
>>> }
>>> 
>>> -- 
>>> 2.40.1
>>> 
> 
> -- 
> Sent from my laptop

Re: [PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

Hi Michael,

On 10/06/2023 13:28, Michael Tremer wrote:
> Hello,
> 
>> On 10 Jun 2023, at 12:16, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>
>> Hi Michael,
>>
>> On 10/06/2023 12:16, Michael Tremer wrote:
>>> I did not merge this, as I believe we need this, because:
>>> We won’t rewrite the OpenVPN configuration files on update, so it might be a good idea to just add the line and if someone edits the connection it might be removed.
>> The code in the backup.pl put the line into the config irrespective of the certificate being legacy or not.
>>
>> With the ovpnmain.cgi code patch of this patch set, it now only adds the providers legacy default to the config file if the cert is legacy when downloading the connection set. This is now done for both n2n and roadwarrior connection sets.
> 
> Yes, this is true, but we won’t run the CGI during the update.
> 
> Any connections that have legacy certificates won’t work after installing the new version of OpenSSL. So we need the legacy provider enabled (just to be safe).

Okay, understand where you are coming from.Good catch.

I have also now tested out a n2n connection created with openssl-3.x with and without the providers legacy default line in the client conf.
Can confirm that it works in both cases, so having the legacy line added dose not cause any problems with the openssl-3.x n2n client connection working.

> 
>>> That should work I believe and -legacy should not have any side effects when enabled but not needed.
>> That is something I have not tested out but I think you are correct, it shouldn't have any side affects.
>>
>> I think it is good to go now and I can always do any additional minor tunings later in CU176 and onwards, otherwise we will be here for ever.
> 
> I would rather like to get it right than being fast, but at this point I don’t know what else we can do. So *fingers crossed*.
> 
> Let’s release either tomorrow or Monday. Depending on how much I am going to enjoy the nice weather this weekend :)

Enjoy the nice weather.

Regards,
Adolf.

> 
> -Michael
> 
>>
>> Regards,
>>
>> Adolf.
>>> Best,
>>> -Michael
>>>> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>>
>>>> - This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.
>>>>
>>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> ---
>>>> config/backup/backup.pl | 15 ---------------
>>>> 1 file changed, 15 deletions(-)
>>>>
>>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>>> index 8d990c0f1..60138a58a 100644
>>>> --- a/config/backup/backup.pl
>>>> +++ b/config/backup/backup.pl
>>>> @@ -190,21 +190,6 @@ restore_backup() {
>>>> # Update OpenVPN CRL
>>>> /etc/fcron.daily/openvpn-crl-updater
>>>>
>>>> - # Update OpenVPN N2N Client Configs
>>>> - ## Add providers legacy default line to n2n client config files
>>>> - # Check if ovpnconfig exists and is not empty
>>>> - if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
>>>> -       # Identify all n2n connections
>>>> -       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
>>>> -           # Add the legacy option to all N2N client conf files if it does not already exist
>>>> - if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
>>>> - if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
>>>> - echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
>>>> - fi
>>>> - fi
>>>> -       done
>>>> - fi
>>>> -
>>>> return 0
>>>> }
>>>>
>>>> -- 
>>>> 2.40.1
>>>>
>>
>> -- 
>> Sent from my laptop
> 
> 

Re: [PATCH 2/3] backup.pl: Remove the previous code for adding legacty provider to n2n

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 4212 bytes --]

Thanks for confirming.

I will schedule the release for tomorrow then as there hasn’t been any issues any more.

Thanks to everyone who helped to *finally* get this over the line and I will keep my fingers crossed that we found all issues.

Best,
-Michael

> On 11 Jun 2023, at 14:17, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
>> On 10/06/2023 13:28, Michael Tremer wrote:
>> Hello,
>>>> On 10 Jun 2023, at 12:16, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>> 
>>> Hi Michael,
>>> 
>>> On 10/06/2023 12:16, Michael Tremer wrote:
>>>> I did not merge this, as I believe we need this, because:
>>>> We won’t rewrite the OpenVPN configuration files on update, so it might be a good idea to just add the line and if someone edits the connection it might be removed.
>>> The code in the backup.pl put the line into the config irrespective of the certificate being legacy or not.
>>> 
>>> With the ovpnmain.cgi code patch of this patch set, it now only adds the providers legacy default to the config file if the cert is legacy when downloading the connection set. This is now done for both n2n and roadwarrior connection sets.
>> Yes, this is true, but we won’t run the CGI during the update.
>> Any connections that have legacy certificates won’t work after installing the new version of OpenSSL. So we need the legacy provider enabled (just to be safe).
> 
> Okay, understand where you are coming from.Good catch.
> 
> I have also now tested out a n2n connection created with openssl-3.x with and without the providers legacy default line in the client conf.
> Can confirm that it works in both cases, so having the legacy line added dose not cause any problems with the openssl-3.x n2n client connection working.
> 
>>>> That should work I believe and -legacy should not have any side effects when enabled but not needed.
>>> That is something I have not tested out but I think you are correct, it shouldn't have any side affects.
>>> 
>>> I think it is good to go now and I can always do any additional minor tunings later in CU176 and onwards, otherwise we will be here for ever.
>> I would rather like to get it right than being fast, but at this point I don’t know what else we can do. So *fingers crossed*.
>> Let’s release either tomorrow or Monday. Depending on how much I am going to enjoy the nice weather this weekend :)
> 
> Enjoy the nice weather.
> 
> Regards,
> Adolf.
> 
>> -Michael
>>> 
>>> Regards,
>>> 
>>> Adolf.
>>>> Best,
>>>> -Michael
>>>>> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>>> 
>>>>> - This code is no longer needed with the code in the ovpnmain.cgi patch in this patch set.
>>>>> 
>>>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>>> ---
>>>>> config/backup/backup.pl | 15 ---------------
>>>>> 1 file changed, 15 deletions(-)
>>>>> 
>>>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>>>> index 8d990c0f1..60138a58a 100644
>>>>> --- a/config/backup/backup.pl
>>>>> +++ b/config/backup/backup.pl
>>>>> @@ -190,21 +190,6 @@ restore_backup() {
>>>>> # Update OpenVPN CRL
>>>>> /etc/fcron.daily/openvpn-crl-updater
>>>>> 
>>>>> - # Update OpenVPN N2N Client Configs
>>>>> - ## Add providers legacy default line to n2n client config files
>>>>> - # Check if ovpnconfig exists and is not empty
>>>>> - if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
>>>>> -       # Identify all n2n connections
>>>>> -       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
>>>>> -           # Add the legacy option to all N2N client conf files if it does not already exist
>>>>> - if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
>>>>> - if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
>>>>> - echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
>>>>> - fi
>>>>> - fi
>>>>> -       done
>>>>> - fi
>>>>> -
>>>>> return 0
>>>>> }
>>>>> 
>>>>> -- 
>>>>> 2.40.1
>>>>> 
>>> 
>>> -- 
>>> Sent from my laptop

[PATCH 3/3] update.sh: Remove code to add providers into n2n configs

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

- This code is no longer required with the code changes in the ovpnmain.cgi patch in this
   patch set.

Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
---
 config/rootfiles/core/175/update.sh | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/config/rootfiles/core/175/update.sh b/config/rootfiles/core/175/update.sh
index 82676bc72..5e45c819f 100644
--- a/config/rootfiles/core/175/update.sh
+++ b/config/rootfiles/core/175/update.sh
@@ -177,20 +177,6 @@ if [ -e /boot/pakfire-kernel-update ]; then
     /boot/pakfire-kernel-update ${KVER}
 fi
 
-## Add providers legacy default line to n2n client config files
-# Check if ovpnconfig exists and is not empty
-if [ -s /var/ipfire/ovpn/ovpnconfig ]; then
-       # Identify all n2n connections
-       for y in $(awk -F',' '/net/ { print $3 }' /var/ipfire/ovpn/ovpnconfig); do
-           # Add the legacy option to all N2N client conf files
-		if [ $(grep -c "Open VPN Client Config" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 1 ] ; then
-			if [ $(grep -c "providers legacy default" /var/ipfire/ovpn/n2nconf/${y}/${y}.conf) -eq 0 ] ; then
-				echo "providers legacy default" >> /var/ipfire/ovpn/n2nconf/${y}/${y}.conf
-			fi
-		fi
-       done
-fi
-
 # This update needs a reboot...
 touch /var/run/need_reboot
 
-- 
2.40.1

Re: [PATCH 1/3] ovpnmain.cgi: Updated fix for Bug#13137

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 7947 bytes --]

Hello Adolf,

Since no comments (neither good or bad) arrived me, I merge this into all branches yesterday.

Once again, thank you very much for putting so much extra time into this painful problem.

I believe that we should be good for a release now - although I am bracing myself for the corner cases that we will discover over the next couple of months or maybe even years. It is going to be fun!

-Michael

> On 7 Jun 2023, at 15:21, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> - This now only adds "providers legacy default" to the config files of connections that
>   have legacy certificates, both for n2n and roadwarrior.
> - This new approach also removes the requirement to have code in the update.sh script
>   or in backup.pl so those earlier modifications are removed in two additional patches
>   combined with this one in a set.
> - The -legacy option has been removed from the pkcs12 creation part of the code as
>   otherwise this creates a certificate in legacy format, which is not wanted. All new
>   connection certificates being created will be based on openssl-3.x
> 
> Fixes: Bug#13137
> Suggested-by: Michael Tremer <michael.tremer(a)ipfire.org>
> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
> ---
> html/cgi-bin/ovpnmain.cgi | 70 ++++++++++++++++++++++++++++++---------
> 1 file changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index 88106251e..a210e0509 100755
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -138,6 +138,17 @@ unless (-e "$local_clientconf") {
> ###
> ### Useful functions
> ###
> +sub iscertlegacy
> +{
> + my $file=$_[0];
> + my @certinfo = &General::system_output("/usr/bin/openssl", "pkcs12", "-info", "-nodes", 
> + "-in", "$file.p12", "-noout", "-passin", "pass:''");
> + if (index ($certinfo[0], "MAC: sha1") != -1) {
> + return 0;
> + }
> + return 1;
> +}
> +
> sub haveOrangeNet
> {
> if ($netsettings{'CONFIG_TYPE'} == 2) {return 1;}
> @@ -1115,7 +1126,9 @@ unless(-d "${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}"){mkdir "${General
>   print CLIENTCONF "# Activate Management Interface and Port\n";
>   if ($cgiparams{'OVPN_MGMT'} eq '') {print CLIENTCONF "management localhost $cgiparams{'DEST_PORT'}\n"}
>   else {print CLIENTCONF "management localhost $cgiparams{'OVPN_MGMT'}\n"};
> -  print CLIENTCONF "providers legacy default\n";
> +  if (&iscertlegacy("${General::swroot}/ovpn/certs/$cgiparams{'NAME'}")) {
> +     print CLIENTCONF "providers legacy default\n";
> +  }
>   close(CLIENTCONF);
> 
> }
> @@ -1649,7 +1662,7 @@ END
> goto ROOTCERT_ERROR;
>    }
> } else { # child
> -    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-cacerts', '-nokeys',
> +    unless (exec ('/usr/bin/openssl', 'pkcs12', '-cacerts', '-nokeys',
>    '-in', $filename,
>    '-out', "$tempdir/cacert.pem")) {
> $errormessage = "$Lang::tr{'cant start openssl'}: $!";
> @@ -1672,7 +1685,7 @@ END
> goto ROOTCERT_ERROR;
>    }
> } else { # child
> -    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-clcerts', '-nokeys',
> +    unless (exec ('/usr/bin/openssl', 'pkcs12', '-clcerts', '-nokeys',
>    '-in', $filename,
>    '-out', "$tempdir/hostcert.pem")) {
> $errormessage = "$Lang::tr{'cant start openssl'}: $!";
> @@ -1695,7 +1708,7 @@ END
> goto ROOTCERT_ERROR;
>    }
> } else { # child
> -    unless (exec ('/usr/bin/openssl', 'pkcs12', '-legacy', '-nocerts',
> +    unless (exec ('/usr/bin/openssl', 'pkcs12', '-nocerts',
>    '-nodes',
>    '-in', $filename,
>    '-out', "$tempdir/serverkey.pem")) {
> @@ -2157,7 +2170,10 @@ if ($confighash{$cgiparams{'KEY'}}[3] eq 'net'){
>    if ($confighash{$cgiparams{'KEY'}}[22] eq '') {print CLIENTCONF "management localhost $confighash{$cgiparams{'KEY'}}[29]\n"}
>     else {print CLIENTCONF "management localhost $confighash{$cgiparams{'KEY'}}[22]\n"};
>    print CLIENTCONF "# remsub $confighash{$cgiparams{'KEY'}}[11]\n";
> -   print CLIENTCONF "providers legacy default\n";
> +  if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
> +     print CLIENTCONF "providers legacy default\n";
> +  }
> +
> 
> 
>     close(CLIENTCONF);
> @@ -2229,10 +2245,18 @@ else
> 
> # Extract the certificate
> # This system call is safe, because all arguments are passed as an array.
> - system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> - '-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
> - if ($?) {
> - die "openssl error: $?";
> + if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
> + system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> + '-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
> + if ($?) {
> + die "openssl error: $?";
> + }
> + } else {
> + system('/usr/bin/openssl', 'pkcs12', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> + '-clcerts', '-nokeys', '-nodes', '-out', "$file_crt" , '-passin', 'pass:');
> + if ($?) {
> + die "openssl error: $?";
> + }
> }
> 
> $zip->addFile("$file_crt", "$confighash{$cgiparams{'KEY'}}[1].pem") or die;
> @@ -2240,10 +2264,18 @@ else
> 
> # Extract the key
> # This system call is safe, because all arguments are passed as an array.
> - system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> - '-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
> - if ($?) {
> - die "openssl error: $?";
> + if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
> + system('/usr/bin/openssl', 'pkcs12', '-legacy', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> + '-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
> + if ($?) {
> + die "openssl error: $?";
> + }
> + } else {
> + system('/usr/bin/openssl', 'pkcs12', '-in', "${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1].p12",
> + '-nocerts', '-nodes', '-out', "$file_key", '-passin', 'pass:');
> + if ($?) {
> + die "openssl error: $?";
> + }
> }
> 
> $zip->addFile("$file_key", "$confighash{$cgiparams{'KEY'}}[1].key") or die;
> @@ -2302,6 +2334,11 @@ else
>     # If the server is asking for TOTP this needs to happen interactively
>     print CLIENTCONF "auth-retry interact\r\n";
> 
> +    # Add provider line if certificate is legacy type
> +    if (&iscertlegacy("${General::swroot}/ovpn/certs/$confighash{$cgiparams{'KEY'}}[1]")) {
> + print CLIENTCONF "providers legacy default\r\n";
> +    }
> +
>     if ($include_certs) {
> print CLIENTCONF "\r\n";
> 
> @@ -3298,7 +3335,10 @@ END
> print FILE "# Logfile\n";
> print FILE "status-version 1\n";
> print FILE "status /var/run/openvpn/$n2nname[0]-n2n 10\n";
> - print FILE "providers legacy default\n";
> + if (&iscertlegacy("${General::swroot}/ovpn/certs/$cgiparams{'n2nname'}")) {
> +     print CLIENTCONF "providers legacy default\n";
> + }
> +
> close FILE;
> 
> unless(move("$tempdir/$uplconffilename", "${General::swroot}/ovpn/n2nconf/$n2nname[0]/$uplconffilename2")) {
> @@ -4245,7 +4285,7 @@ if ($cgiparams{'TYPE'} eq 'net') {
> 
>    # Create the pkcs12 file
>    # The system call is safe, because all arguments are passed as an array.
> -    system('/usr/bin/openssl', 'pkcs12', '-legacy', '-export',
> +    system('/usr/bin/openssl', 'pkcs12', '-export',
> '-inkey', "${General::swroot}/ovpn/certs/$cgiparams{'NAME'}key.pem",
> '-in', "${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem",
> '-name', $cgiparams{'NAME'},
> -- 
> 2.40.1
>