Re: [PATCH 1/2] ipsec: only accept valid options for a connection type

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

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

Hi,

sorry for the very late review. See the comments below:

On Mon, 2017-08-28 at 17:32 +0200, Jonatan Schlag wrote:
> The valid options for a host-to-net ipsec connection and a net-to-net
> are different so we nedd to check if we get a valid option for the connection
> type.
> 
> Fixes: #11444
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.ipsec | 129
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/src/functions/functions.ipsec b/src/functions/functions.ipsec
> index 6f14c8e..11a1d9a 100644
> --- a/src/functions/functions.ipsec
> +++ b/src/functions/functions.ipsec
> @@ -54,6 +54,46 @@ IPSEC_DEFAULT_TYPE="net-to-net"
>  IPSEC_VALID_MODES="gre-transport tunnel vti"
>  IPSEC_VALID_AUTH_MODES="PSK"
>  
> +# Valid options for all types of connetions
> +IPSEC_ALL_VALID_OPTIONS="\
> +	authentication \
> +	down \
> +	disable \
> +	dpd \
> +	enable \
> +	inactivity_timeout \
> +	local \
> +	mode \
> +	remote \
> +	security_policy \
> +	start_action \
> +	up \
> +	color \
> +	description \
> +	show"

Could we order those alphabetically?

> +
> +# Valid options for net-to-net
> +IPSEC_NET_TOT_NET_VALID_OPTIONS="${IPSEC_ALL_VALID_OPTIONS} \
> +	peer"

I think it should be "TO" in the variable name. You could also use "N2N" to make
it a little bit shorter maybe.

> +
> +# Valid options for RW
> +IPSEC_RW_VALID_OPTIONS="${IPSEC_ALL_VALID_OPTIONS} \
> +	pool"
> +
> +# Valid dpd options for all types of connetions
> +IPSEC_DPD_ALL_VALID_OPTIONS="\
> +	delay \
> +	timeout"
> +
> +# Valid dpd options for net-to-net
> +IPSEC_DPD_NET_TOT_NET_VALID_OPTIONS="${IPSEC_DPD_ALL_VALID_OPTIONS} \
> +	action"

Same as above.

> +
> +# Valid dpd options for RW
> +IPSEC_DPD_RW_VALID_OPTIONS="${IPSEC_DPD_ALL_VALID_OPTIONS}"
> +
> +
> +

Three lines of whitespace. One should do.

>  cli_ipsec() {
>  	local action=${1}
>  	shift 1
> @@ -79,6 +119,10 @@ cli_ipsec_connection() {
>  		key=${key//-/_}
>  		shift 2
>  
> +		if ! ipsec_connection_check_option "${connection}" "first"
> "${key}"; then
> +			return ${EXITR_ERROR}
> +		fi
> +

Typo in the EXIT_ERROR variable.

>  		case "${key}" in
>  			authentication|down|disable|dpd|enable|inactivity_tim
> eout|local|mode|peer|pool|remote|security_policy|start_action|up)
>  				ipsec_connection_${key} ${connection} "$@"
> @@ -148,6 +192,67 @@ ipsec_connection_get_description_title() {
>  	description_title_read $(description_format_filename "ipsec-
> connection" "${name}")
>  }
>  
> +# This functions returns all options which are valid for this type of
> connection
> +ipsec_get_valid_options() {
> +	assert [ $# -eq 2 ]
> +
> +	local connection="${1}"
> +	local type="${2}"
> +
> +	local con_type=$(ipsec_connection_get_type "${connection}")
> +	assert isset con_type
> +
> +	case ${con_type} in
> +		host-to-net)
> +			case ${type} in
> +				first)
> +					echo "${IPSEC_RW_VALID_OPTIONS}"
> +					;;
> +				dpd)
> +					echo "${IPSEC_DPD_RW_VALID_OPTIONS}"
> +					;;
> +			esac
> +			;;
> +		net-to-net)
> +			case ${type} in
> +				first)
> +					echo
> "${IPSEC_NET_TOT_NET_VALID_OPTIONS}"
> +					;;
> +				dpd)
> +					echo
> "${IPSEC_DPD_NET_TOT_NET_VALID_OPTIONS}"
> +					;;
> +			esac
> +			;;
> +	esac
> +}
> +
> +# This Function checks if a option is valid for a given type
> +ipsec_connection_check_option() {
> +	assert [ $# -eq 3 ]
> +
> +	local connection="${1}"
> +	local type="${2}"
> +	local key="${3}"
> +
> +	local con_type=$(ipsec_connection_get_type "${connection}")
> +	assert isset con_type
> +
> +	case ${con_type} in
> +		host-to-net)
> +			if ! isoneof key $(ipsec_get_valid_options
> "${connection}" "${type}"); then
> +				log ERROR "Invalid Argument: ${key}"
> +				return ${EXIT_ERROR}
> +			fi
> +			;;
> +		net-to-net)
> +			if ! isoneof key $(ipsec_get_valid_options
> "${connection}" "${type}"); then
> +				log ERROR "Invalid Argument: ${key}"
> +				return ${EXIT_ERROR}
> +			fi
> +		;;
> +	esac
> +}
> +
>  cli_ipsec_connection_show() {
>  	local connection="${1}"
>  
> @@ -443,6 +548,26 @@ ipsec_reload() {
>  	ipsec_strongswan_load
>  }
>  
> +# Returns the type of a connection
> +ipsec_connection_get_type() {
> +	assert [ $# -eq 1 ]
> +
> +	local connection=${1}
> +
> +	if ! ipsec_connection_exists "${connection}"; then
> +		log ERROR "No such VPN ipsec connection: ${connection}"
> +		return ${EXIT_ERROR}
> +	fi

It is called a "IPsec VPN connection".

IPsec is spelled like this and the rest sounds like Yoda :)

> +
> +	local TYPE
> +
> +	if ! ipsec_connection_read_config "${connection}" TYPE; then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	echo ${TYPE}
> +}
> +
>  # Handle the cli after authentification
>  ipsec_connection_authentication() {
>  	if [ ! $# -gt 1 ]; then
> @@ -551,6 +676,10 @@ ipsec_connection_dpd() {
>  	local cmd=${2}
>  	shift 2
>  
> +	if ! ipsec_connection_check_option "${connection}" "dpd" "${cmd}";
> then
> +		return ${EXITR_ERROR}
> +	fi
> +

Should the user not get a better message when DPD commands are not available?

>  	case ${cmd} in
>  		action)
>  			ipsec_connection_dpd_action "${connection}" "$@"

-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]