Hi,
Am Do, 7. Sep, 2017 um 9:44 schrieb Michael Tremer michael.tremer@ipfire.org:
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@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}fiTypo in the EXIT_ERROR variable.
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} infirst)echo "${IPSEC_RW_VALID_OPTIONS}";;dpd)echo "${IPSEC_DPD_RW_VALID_OPTIONS}";;esac;;net-to-net)case ${type} infirst)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?
I thought about it, there two ways to achieve that. One way would be to put the error messages here and remove the error message inside the function. Or we check what type we get a report a better error message back.
But from my point of view "Invalid argument" should be ok here, because we always get here when the user has some typos or use wrong commands. So keeping the generic error message should be cleaner.
case ${cmd} in action) ipsec_connection_dpd_action "${connection}" "$@"
-Michael
Hopefully, the WIFI in the ICE from Berlin to Munich is good (If there would be WIFI, it would be even better than my ride from Munich to Berlin without WIFI ) so I can correct this patch tomorrow afternoon.
Jonatan