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

[Michael Tremer <michael.tremer@ipfire.org>]

[-- 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
>