Hello,

this patch was line-wrapped and cannot be merged, but nevertheless,
here are my thoughts:

On Mon, 2015-12-07 at 17:35 +0100, IT Superhack wrote:
> Signed-off-by: Timmothy Wilson <itsuperhack(a)web.de>
> ---
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index 62af54e..15385f1 100644
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -1316,7 +1316,7 @@ END
>  				<option value='1024'
> $selected{'DHLENGHT'}{'1024'}>1024
> $Lang::tr{'bit'}</option>
>  				<option value='2048'
> $selected{'DHLENGHT'}{'2048'}>2048
> $Lang::tr{'bit'}</option>
>  				<option value='3072'
> $selected{'DHLENGHT'}{'3072'}>3072
> $Lang::tr{'bit'}</option>
> -				<option value='4096'
> $selected{'DHLENGHT'}{'4096'}>4096
> $Lang::tr{'bit'}</option>
> +				<option value='4096'
> $selected{'DHLENGHT'}{'4096'}>4096
> $Lang::tr{'bit'} ($Lang::tr{'recommended'})</option>
>  			</select>
>  		</td>
>  	</tr>

I agree, that it is desirable to use longer keys. However, I am not
sure if it is a good idea to go all the way for 4096 bit and not only
for e.g. 2048 bit. Why not 8192 even?

I would like to read some justification for the values that are picked.

Furthermore, I think that we the upper bound should be something that
the average IPFire box is able to handle.

> @@ -4687,7 +4687,7 @@ if ($cgiparams{'TYPE'} eq 'net') {
>  				<option value='CAMELLIA-256-CBC'
> $selected{'DCIPHER'}{'CAMELLIA-256-CBC'}>CAMELLIA-CBC (256
> $Lang::tr{'bit'})</option>
>  				<option value='CAMELLIA-192-CBC'
> $selected{'DCIPHER'}{'CAMELLIA-192-CBC'}>CAMELLIA-CBC (192
> $Lang::tr{'bit'})</option>
>  				<option value='CAMELLIA-128-CBC'
> $selected{'DCIPHER'}{'CAMELLIA-128-CBC'}>CAMELLIA-CBC (128
> $Lang::tr{'bit'})</option>
> -				<option value='AES-256-CBC' 	
> $selected{'DCIPHER'}{'AES-256-CBC'}>AES-CBC (256 $Lang::tr{'bit'},
> $Lang::tr{'default'})</option>
> +				<option value='AES-256-CBC' 	
> $selected{'DCIPHER'}{'AES-256-CBC'}>AES-CBC (256 $Lang::tr{'bit'},
> $Lang::tr{'default'}, $Lang::tr{'recommended'})</option>
>  				<option value='AES-192-CBC' 	
> $selected{'DCIPHER'}{'AES-192-CBC'}>AES-CBC (192
> $Lang::tr{'bit'})</option>
>  				<option value='AES-128-CBC' 	
> $selected{'DCIPHER'}{'AES-128-CBC'}>AES-CBC (128
> $Lang::tr{'bit'})</option>
>  				<option value='DES-EDE3-CBC'	
> $selected{'DCIPHER'}{'DES-EDE3-CBC'}>DES-EDE3-CBC (192
> $Lang::tr{'bit'})</option>

I can agree with that since it is already selected by default. This
makes it just more explicit.

I would have merged this if this was an independent patch in a patch
set.

> @@ -4702,7 +4702,7 @@ if ($cgiparams{'TYPE'} eq 'net') {
>  		<td class='boldbase'>$Lang::tr{'ovpn ha'}:</td>
>  		<td><select name='DAUTH'>
>  				<option value='whirlpool'	
> $selected{'DAUTH'}{'whirlpool'}>Whirlpool (512
> $Lang::tr{'bit'})</option>
> -				<option value='SHA512'		
> 	$selected{'DAUTH'}{'SHA512'}>SHA2 (512
> $Lang::tr{'bit'})</option>
> +				<option value='SHA512'		
> 	$selected{'DAUTH'}{'SHA512'}>SHA2 (512
> $Lang::tr{'bit'}, $Lang::tr{'recommended'})</option>
>  				<option value='SHA384'		
> 	$selected{'DAUTH'}{'SHA384'}>SHA2 (384
> $Lang::tr{'bit'})</option>
>  				<option value='SHA256'		
> 	$selected{'DAUTH'}{'SHA256'}>SHA2 (256
> $Lang::tr{'bit'})</option>
>  				<option value='SHA1'			
> $selected{'DAUTH'}{'SHA1'}>SHA1 (160
> $Lang::tr{'bit'} Default)</option>

Same as above. SHA2 is considered to be "secure enough for now". Why is
256 bit not enough? This has also a rather huge performance impact.
Things like these should also be mentioned in the commit message.

> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
> index f1cffb8..9aa50f5 100644
> --- a/html/cgi-bin/vpnmain.cgi
> +++ b/html/cgi-bin/vpnmain.cgi
> @@ -2424,7 +2424,7 @@ if(($cgiparams{'ACTION'} eq
> $Lang::tr{'advanced'}) ||
>  			<td>$Lang::tr{'vpn keyexchange'}:</td>
>  			<td>
>  				<select name='IKE_VERSION'>
> -					<option value='ikev2'
> $selected{'IKE_VERSION'}{'ikev2'}>IKEv2</option>
> +					<option value='ikev2'
> $selected{'IKE_VERSION'}{'ikev2'}>IKEv2
> ($Lang::tr{'recommended'})</option>
>  					<option value='ikev1'
> $selected{'IKE_VERSION'}{'ikev1'}>IKEv1</option>
>  				</select>
>  			</td>

Why should IKEv2 be recommended? AFAIK there are no known design issues
with IKEv1. Some algorithms might not be available, but this is not an
issue for now since AES, SHA2, (AKA the strong ones) are supported.

> @@ -2434,7 +2434,7 @@ if(($cgiparams{'ACTION'} eq
> $Lang::tr{'advanced'}) ||
>  			<td class='boldbase'
> width="15%">$Lang::tr{'encryption'}</td>
>  			<td class='boldbase'>
>  				<select name='IKE_ENCRYPTION'
> multiple='multiple' size='6'
> style='width: 100%'>
> -					<option value='aes256gcm128'
> $checked{'IKE_ENCRYPTION'}{'aes256gcm128'}>256 bit AES-GCM/128 bit
> ICV</option>
> +					<option value='aes256gcm128'
> $checked{'IKE_ENCRYPTION'}{'aes256gcm128'}>256 bit AES-GCM/128 bit
> ICV
> ($Lang::tr{'recommended'})</option>
>  					<option value='aes256gcm96'
> $checked{'IKE_ENCRYPTION'}{'aes256gcm96'}>256 bit AES-GCM/96 bit
> ICV</option>
>  					<option value='aes256gcm64'
> $checked{'IKE_ENCRYPTION'}{'aes256gcm64'}>256 bit AES-GCM/64 bit
> ICV</option>
>  					<option value='aes256'
> $checked{'IKE_ENCRYPTION'}{'aes256'}>256
> bit AES-CBC</option>
> @@ -2454,7 +2454,7 @@ if(($cgiparams{'ACTION'} eq
> $Lang::tr{'advanced'}) ||
>  			</td>
>  			<td class='boldbase'>
>  				<select name='ESP_ENCRYPTION'
> multiple='multiple' size='6'
> style='width: 100%'>
> -					<option value='aes256gcm128'
> $checked{'ESP_ENCRYPTION'}{'aes256gcm128'}>256 bit AES-GCM/128 bit
> ICV</option>
> +					<option value='aes256gcm128'
> $checked{'ESP_ENCRYPTION'}{'aes256gcm128'}>256 bit AES-GCM/128 bit
> ICV
> ($Lang::tr{'recommended'})</option>
>  					<option value='aes256gcm96'
> $checked{'ESP_ENCRYPTION'}{'aes256gcm96'}>256 bit AES-GCM/96 bit
> ICV</option>
>  					<option value='aes256gcm64'
> $checked{'ESP_ENCRYPTION'}{'aes256gcm64'}>256 bit AES-GCM/64 bit
> ICV</option>
>  					<option value='aes256'
> $checked{'ESP_ENCRYPTION'}{'aes256'}>256
> bit AES-CBC</option>

Why are the AES-GCM cipher suites with smaller IVs not recommended?

> @@ -2478,7 +2478,7 @@ if(($cgiparams{'ACTION'} eq
> $Lang::tr{'advanced'}) ||
>  			<td class='boldbase'
> width="15%">$Lang::tr{'integrity'}</td>
>  			<td class='boldbase'>
>  				<select name='IKE_INTEGRITY'
> multiple='multiple' size='6'
> style='width: 100%'>
> -					<option value='sha2_512'
> $checked{'IKE_INTEGRITY'}{'sha2_512'}>SHA2 512 bit</option>
> +					<option value='sha2_512'
> $checked{'IKE_INTEGRITY'}{'sha2_512'}>SHA2 512 bit
> ($Lang::tr{'recommended'})</option>
>  					<option value='sha2_384'
> $checked{'IKE_INTEGRITY'}{'sha2_384'}>SHA2 384 bit</option>
>  					<option value='sha2_256'
> $checked{'IKE_INTEGRITY'}{'sha2_256'}>SHA2 256 bit</option>
>  					<option value='aesxcbc'
> $checked{'IKE_INTEGRITY'}{'aesxcbc'}>AES
> XCBC</option>

Same as above.

> @@ -2488,7 +2488,7 @@ if(($cgiparams{'ACTION'} eq
> $Lang::tr{'advanced'}) ||
>  			</td>
>  			<td class='boldbase'>
>  				<select name='ESP_INTEGRITY'
> multiple='multiple' size='6'
> style='width: 100%'>
> -					<option value='sha2_512'
> $checked{'ESP_INTEGRITY'}{'sha2_512'}>SHA2 512 bit</option>
> +					<option value='sha2_512'
> $checked{'ESP_INTEGRITY'}{'sha2_512'}>SHA2 512 bit
> ($Lang::tr{'recommended'})</option>
>  					<option value='sha2_384'
> $checked{'ESP_INTEGRITY'}{'sha2_384'}>SHA2 384 bit</option>
>  					<option value='sha2_256'
> $checked{'ESP_INTEGRITY'}{'sha2_256'}>SHA2 256 bit</option>
>  					<option value='aesxcbc'
> $checked{'ESP_INTEGRITY'}{'aesxcbc'}>AES
> XCBC</option>

Same again.

> diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl
> index 2bca854..b18cace 100644
> --- a/langs/de/cgi-bin/de.pl
> +++ b/langs/de/cgi-bin/de.pl
> @@ -1914,6 +1914,7 @@
>  'rebooting ipfire' => 'Starte IPFire neu',
>  'reconnect' => 'Neu Verbinden',
>  'reconnection' => 'Wiederverbindung',
> +'recommended' => 'empfohlen',
>  'red' => 'Internet',
>  'red1' => 'ROT',
>  'references' => 'Referenzen',
> 
> 

The English translation is missing.

Best,
-Michael