Re: [PATCH] OpenVPN: Introduce Negotiable Crypto Parameters for roadwarriors

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

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

Hello,

hmm, I am not sure if I agree with the patch.

Could you answer some questions so that I understand better what the
implications are.

On Mon, 2018-08-06 at 09:25 +0200, Erik Kapfer wrote:
> The ncp-ciphers differs to the OpenVPN default value and has been adapted from Fedora.
> Please see explanations in https://fedoraproject.org/wiki/Changes/New_default_cipher_in_OpenVPN .
> ---
>  html/cgi-bin/ovpnmain.cgi | 38 +++++++++++++++++++++++++++-----------
>  langs/de/cgi-bin/de.pl    |  1 +
>  langs/en/cgi-bin/en.pl    |  1 +
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index 976300f..dc22ba5 100644
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -321,8 +321,13 @@ sub writeserverconf {
>      }	
>      print CONF "status-version 1\n";
>      print CONF "status /var/run/ovpnserver.log 30\n";
> -    print CONF "ncp-disable\n";
>      print CONF "cipher $sovpnsettings{DCIPHER}\n";
> +    # Enable Negotiable Crypto Parameters
> +    if ($sovpnsettings{'NCP'} eq 'on') {
> +         print CONF "ncp-ciphers AES-256-GCM:AES-256-CBC:AES-128-GCM:AES-128-CBC:BF-CBC\n";
> +    } else {
> +        print CONF "ncp-disable\n";
> +    }

Questions here:

1) Why do we hard-code the cipher list?

2) Who would want to disable this as it should always peacefully co-
exists with the "cipher" options.

>      if ($sovpnsettings{'DAUTH'} eq '') {
>          print CONF "";
>      } else {
> @@ -789,6 +794,7 @@ if ($cgiparams{'ACTION'} eq $Lang::tr{'save-adv-options'}) {
>      $vpnsettings{'ROUTES_PUSH'} = $cgiparams{'ROUTES_PUSH'};
>      $vpnsettings{'DAUTH'} = $cgiparams{'DAUTH'};
>      $vpnsettings{'TLSAUTH'} = $cgiparams{'TLSAUTH'};
> +    $vpnsettings{'NCP'} = $cgiparams{'NCP'};
>      my @temp=();
>      
>      if ($cgiparams{'FRAGMENT'} eq '') {
> @@ -2685,6 +2691,9 @@ ADV_ERROR:
>      $checked{'TLSAUTH'}{'off'} = '';
>      $checked{'TLSAUTH'}{'on'} = '';
>      $checked{'TLSAUTH'}{$cgiparams{'TLSAUTH'}} = 'CHECKED';
> +    $checked{'NCP'}{'off'} = '';
> +    $checked{'NCP'}{'on'} = '';
> +    $checked{'NCP'}{$cgiparams{'NCP'}} = 'CHECKED';
>     
>      &Header::showhttpheaders();
>      &Header::openpage($Lang::tr{'status ovpn'}, 1, '');
> @@ -2818,6 +2827,22 @@ print <<END;
>      <tr>
>  		<td class'base'><b>$Lang::tr{'ovpn crypt options'}</b></td>
>  	</tr>
> +
> +<table width='100%'>
> +    <tr>
> +    <td width='20%'></td> <td width='15%'> </td><td width='15%'> </td><td width='15%'></td><td width='35%'></td>
> +    </tr>
> +
> +    <tr>
> +         <td class='base'>$Lang::tr{'ovpn ncp'}</td>
> +         <td><input type='checkbox' name='NCP' $checked{'NCP'}{'on'} /></td>
> +    </tr>
> +
> +    <tr>
> +         <td class='base'>HMAC tls-auth</td>
> +         <td><input type='checkbox' name='TLSAUTH' $checked{'TLSAUTH'}{'on'} /></td>
> +    </tr>
> +
>  	<tr>
>  		<td width='20%'></td> <td width='30%'> </td><td width='25%'> </td><td width='25%'></td>
>      </tr>	
> @@ -2833,17 +2858,8 @@ print <<END;
>  		<td>$Lang::tr{'openvpn default'}: <span class="base">SHA1 (160 $Lang::tr{'bit'})</span></td>
>      </tr>
>  </table>
> +<hr size='1'>
>  
> -<table width='100%'>
> -    <tr>
> -	<td width='20%'></td> <td width='15%'> </td><td width='15%'> </td><td width='15%'></td><td width='35%'></td>
> -    </tr>
> -
> -    <tr>
> -	<td class='base'>HMAC tls-auth</td>
> -	<td><input type='checkbox' name='TLSAUTH' $checked{'TLSAUTH'}{'on'} /></td>
> -    </tr>
> -    </table><hr>
>  END
>  
>  if ( -e "/var/run/openvpn.pid"){
> diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl
> index 6e3dba4..9f0de6b 100644
> --- a/langs/de/cgi-bin/de.pl
> +++ b/langs/de/cgi-bin/de.pl
> @@ -1833,6 +1833,7 @@
>  'ovpn mtu-disc off' => 'Deaktiviert',
>  'ovpn mtu-disc with mssfix or fragment' => 'Path MTU Discovery kann nicht gemeinsam mit mssfix oder fragment verwendet werden.',
>  'ovpn mtu-disc yes' => 'Forciert',
> +'ovpn ncp' => 'Verschlüsselung aushandeln',
>  'ovpn no connections' => 'Keine aktiven OpenVPN Verbindungen',
>  'ovpn on blue' => 'OpenVPN auf BLAU:',
>  'ovpn on orange' => 'OpenVPN auf ORANGE:',
> diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
> index 3ec5af5..5cd47b1 100644
> --- a/langs/en/cgi-bin/en.pl
> +++ b/langs/en/cgi-bin/en.pl
> @@ -1866,6 +1866,7 @@
>  'ovpn mtu-disc off' => 'Disabled',
>  'ovpn mtu-disc with mssfix or fragment' => 'Path MTU Discovery cannot be used with mssfix or fragment.',
>  'ovpn mtu-disc yes' => 'Forced',
> +'ovpn ncp' => 'Negotiate encryption',

This doesn't fully explain to the user actually is being negotiated.
The control channel? The data channel? TLS?

>  'ovpn no connections' => 'No active OpenVPN connections',
>  'ovpn on blue' => 'OpenVPN on BLUE:',
>  'ovpn on orange' => 'OpenVPN on ORANGE:',

Best,
-Michael