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

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

[Jonatan Schlag]

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

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"
+
+# Valid options for net-to-net
+IPSEC_NET_TOT_NET_VALID_OPTIONS="${IPSEC_ALL_VALID_OPTIONS} \
+	peer"
+
+# 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"
+
+# Valid dpd options for RW
+IPSEC_DPD_RW_VALID_OPTIONS="${IPSEC_DPD_ALL_VALID_OPTIONS}"
+
+
+
 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
+
 		case "${key}" in
 			authentication|down|disable|dpd|enable|inactivity_timeout|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
+
+	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
+
 	case ${cmd} in
 		action)
 			ipsec_connection_dpd_action "${connection}" "$@"
-- 
2.6.3

[PATCH 2/2] ipsec: also show only valid options in the autocompletion

[Jonatan Schlag]

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

The options for the different types of connections should be also printed
correctly when we use autocompletion

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/bash-completion/network | 17 ++++++++++++++++-
 src/network                 |  3 +++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/bash-completion/network b/src/bash-completion/network
index 71bf245..25cf6aa 100644
--- a/src/bash-completion/network
+++ b/src/bash-completion/network
@@ -413,7 +413,7 @@ _network_vpn_ipsec_connection_subcommands() {
 	shift
 	local words=( $@ )
 
-	local commands="authentication color description down inactivity-timeout local mode peer remote security-policy show up"
+	local commands="$(network raw list-valid-ipsec-options "${connection}" "first")"
 	local cmd="$(_network_find_on_cmdline "${commands}")"
 	if [[ -z "${cmd}" ]]; then
 		COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )
@@ -432,6 +432,9 @@ _network_vpn_ipsec_connection_subcommands() {
 		description)
 			_network_description ${args}
 			;;
+		dpd)
+			_network_vpn_ipsec_connection_subcommands_dpd "${connection}" ${args}
+			;;
 		local)
 			_network_vpn_ipsec_connection_subcommands_local_remote ${connection} "local" ${args}
 			;;
@@ -529,6 +532,18 @@ _network_vpn_ipsec_connection_subcommands_security_policy() {
 	fi
 }
 
+_network_vpn_ipsec_connection_subcommands_dpd() {
+	local connection=${1}
+	shift
+	local words=( $@ )
+
+	local commands="$(network raw list-valid-ipsec-options "${connection}" "dpd")"
+	local cmd="$(_network_find_on_cmdline "${commands}")"
+	if [[ -z "${cmd}" ]]; then
+		COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )
+		return 0
+	fi
+}
 _network_vpn_security_policies() {
 	local words=( $@ )
 
diff --git a/src/network b/src/network
index 9a2d480..379a71d 100644
--- a/src/network
+++ b/src/network
@@ -1334,6 +1334,9 @@ cli_raw() {
 		list-next-free-zones)
 			zones_get_next_free
 			;;
+		list-valid-ipsec-options)
+			ipsec_get_valid_options "$@"
+			;;
 		list-zone-config-ids)
 			zone_config_list_ids "$@"
 			;;
-- 
2.6.3

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

[Michael Tremer]

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

Re: [PATCH 2/2] ipsec: also show only valid options in the autocompletion

[Michael Tremer]

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

Hi,

technically this work, but is it not better to have a command that checks what
type a connection is (rw or n2n) and then the right list of commands will be
selected?

I think that is cleaner instead of heaving some auto-completion stuff in the
main library.

Best,
-Michael

On Mon, 2017-08-28 at 17:32 +0200, Jonatan Schlag wrote:
> The options for the different types of connections should be also printed
> correctly when we use autocompletion
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/bash-completion/network | 17 ++++++++++++++++-
>  src/network                 |  3 +++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bash-completion/network b/src/bash-completion/network
> index 71bf245..25cf6aa 100644
> --- a/src/bash-completion/network
> +++ b/src/bash-completion/network
> @@ -413,7 +413,7 @@ _network_vpn_ipsec_connection_subcommands() {
>  	shift
>  	local words=( $@ )
>  
> -	local commands="authentication color description down inactivity-
> timeout local mode peer remote security-policy show up"
> +	local commands="$(network raw list-valid-ipsec-options
> "${connection}" "first")"
>  	local cmd="$(_network_find_on_cmdline "${commands}")"
>  	if [[ -z "${cmd}" ]]; then
>  		COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )
> @@ -432,6 +432,9 @@ _network_vpn_ipsec_connection_subcommands() {
>  		description)
>  			_network_description ${args}
>  			;;
> +		dpd)
> +			_network_vpn_ipsec_connection_subcommands_dpd
> "${connection}" ${args}
> +			;;
>  		local)
>  			_network_vpn_ipsec_connection_subcommands_local_remot
> e ${connection} "local" ${args}
>  			;;
> @@ -529,6 +532,18 @@
> _network_vpn_ipsec_connection_subcommands_security_policy() {
>  	fi
>  }
>  
> +_network_vpn_ipsec_connection_subcommands_dpd() {
> +	local connection=${1}
> +	shift
> +	local words=( $@ )
> +
> +	local commands="$(network raw list-valid-ipsec-options
> "${connection}" "dpd")"
> +	local cmd="$(_network_find_on_cmdline "${commands}")"
> +	if [[ -z "${cmd}" ]]; then
> +		COMPREPLY=( $(compgen -W "${commands}" -- "${cur}") )
> +		return 0
> +	fi
> +}
>  _network_vpn_security_policies() {
>  	local words=( $@ )
>  
> diff --git a/src/network b/src/network
> index 9a2d480..379a71d 100644
> --- a/src/network
> +++ b/src/network
> @@ -1334,6 +1334,9 @@ cli_raw() {
>  		list-next-free-zones)
>  			zones_get_next_free
>  			;;
> +		list-valid-ipsec-options)
> +			ipsec_get_valid_options "$@"
> +			;;
>  		list-zone-config-ids)
>  			zone_config_list_ids "$@"
>  			;;

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