Re: [RFC PATCH 1/2] Add vpn-ipsec functions

Hi,

On Wed, 2017-07-26 at 19:32 +0200, Jonatan Schlag wrote:

Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org

Makefile.am                       |   1 +  src/functions/functions.vpn-ipsec | 550 ++++++++++++++++++++++++++++++++++++++  2 files changed, 551 insertions(+)  create mode 100644 src/functions/functions.vpn-ipsec

diff --git a/Makefile.am b/Makefile.am index 761e849..c14bb17 100644 --- a/Makefile.am +++ b/Makefile.am @@ -160,6 +160,7 @@ dist_network_SCRIPTS = \   src/functions/functions.usb \   src/functions/functions.util \   src/functions/functions.vlan \

• src/functions/functions.vpn-ipsec \

src/functions/functions.vpn-security-policies \   src/functions/functions.wireless \   src/functions/functions.wpa_supplicant \ diff --git a/src/functions/functions.vpn-ipsec b/src/functions/functions.vpn-ipsec new file mode 100644 index 0000000..2bfa3a2 --- /dev/null +++ b/src/functions/functions.vpn-ipsec @@ -0,0 +1,550 @@ +#!/bin/bash +#################################################################### ########### +#                                                                              # +# IPFire.org - A linux based firewall                                         # +# Copyright (C) 2017  IPFire Network Development Team                         # +#                                                                              # +# This program is free software: you can redistribute it and/or modify        # +# it under the terms of the GNU General Public License as published by        # +# the Free Software Foundation, either version 3 of the License, or           # +# (at your option) any later version.                                         # +#                                                                              # +# This program is distributed in the hope that it will be useful,             # +# but WITHOUT ANY WARRANTY; without even the implied warranty of              # +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the               # +# GNU General Public License for more details.                                # +#                                                                              # +# You should have received a copy of the GNU General Public License           # +# along with this program.  If not, see http://www.gnu.org/licenses /.       # +#                                                                              # +#################################################################### ###########

+VPN_IPSEC_CONNECTION_CONFIG_SETTINGS="AUTH_MODE INACTIVITY_TIMEOUT LOCAL_ID LOCAL_PREFIX" +VPN_IPSEC_CONNECTION_CONFIG_SETTINGS="${VPN_IPSEC_CONNECTION_CONFIG_ SETTINGS} MODE PEER PSK" +VPN_IPSEC_CONNECTION_CONFIG_SETTINGS="${VPN_IPSEC_CONNECTION_CONFIG_ SETTINGS} REMOTE_ID REMOTE_PREFIX" +VPN_IPSEC_CONNECTION_CONFIG_SETTINGS="${VPN_IPSEC_CONNECTION_CONFIG_ SETTINGS} SECURITY_POLICY"

LOL. This is a big paragraph to open...

+# Default values +MODE="tunnel" +AUTH_MODE="psk" +INACTIVITY_TIMEOUT="0" +SECURITY_POLICY="system"

I think there is no need for these to be set here. Especially since variable names like MODE are quite generic.

I think they should be copied into local variables in the *_new() function. You can have something like IPSEC_DEFAULT_MODE=... in the header maybe to make them easier to modify if we need to. But I am not sure if that is necessary.

Should the functions be called vpn_ipsec_* or is just ipsec_* not enough?

+# This function writes all values to a via ${connection} specificated VPN IPsec configuration file +vpn_ipsec_connection_write_config() {

• assert [ $# -ge 1 ]
• local connection="${1}"
• if ! vpn_ipsec_connection_exists "${connection}"; then

• log ERROR "No such VPN IPsec connection:
  

${connection}"

• return ${EXIT_ERROR}
  

• fi
• local path="$(vpn_ipsec_connection_path "${connection}")"
• if [ ! -w ${path} ]; then

• log ERROR "${path} is not writeable"
  

• return ${EXIT_ERROR}
  

• fi
• if ! settings_write "${path}"

${VPN_IPSEC_CONNECTION_CONFIG_SETTINGS}; then

• log ERROR "Could not write configuration settings
  

for VPN IPsec connection ${connection}"

• return ${EXIT_ERROR}
  

• fi
• # TODO everytime we successfully write a config we should

call some trigger to take the changes into effect +}

I think you can add the trigger already as an empty function called vpn_ipsec_reload that takes the connection name as an argument.

+# This funtion writes the value for one key to a via ${connection} specificated VPN IPsec connection configuration file +vpn_ipsec_connection_write_config_key() {

• assert [ $# -ge 3 ]
• local connection=${1}
• local key=${2}
• shift 2
• local value="$@"
• if ! vpn_ipsec_connection_exists "${connection}"; then

• log ERROR "No such VPN ipsec connection:
  

${connection}"

• return ${EXIT_ERROR}
  

• fi
• log DEBUG "Set '${key}' to new value '${value}' in VPN ipsec

connection '${connection}'"

• local ${VPN_IPSEC_CONNECTION_CONFIG_SETTINGS}
• # Read the config settings
• if ! vpn_ipsec_connection_read_config "${connection}"; then

• return ${EXIT_ERROR}
  

• fi
• # Set the key to a new value
• assign "${key}" "${value}"
• if ! vpn_ipsec_connection_write_config "${connection}"; then

• return ${EXIT_ERROR}
  

• fi
• return ${EXIT_TRUE}

+}

+# Reads one or more keys out of a settings file or all if no key is provided. +vpn_ipsec_connection_read_config() {

• assert [ $# -ge 1 ]
• local connection="${1}"
• shift 1
• if ! vpn_ipsec_connection_exists "${connection}"; then

• log ERROR "No such VPN IPsec connection :
  

${connection}"

• return ${EXIT_ERROR}
  

• fi
• local args
• if [ $# -eq 0 ] && [ -n

"${VPN_IPSEC_CONNECTION_CONFIG_SETTINGS}" ]; then

• list_append args
  

${VPN_IPSEC_CONNECTION_CONFIG_SETTINGS}

• else

• list_append args $@
  

• fi
• local path="$(vpn_ipsec_connection_path ${connection})"
• if ! settings_read "${path}" ${args}; then

• log ERROR "Could not read settings for VPN IPsec
  

connection ${connection}"

• return ${EXIT_ERROR}
  

• fi

+}

+# Returns the path to a the configuration for given connection +vpn_ipsec_connection_path() {

• assert [ $# -eq 1 ]
• local connection=${1}
• echo

"${NETWORK_CONFIG_DIR}/vpn/ipsec/connection/${connection}" +}

Should we not use a directory to store certificates, a description and so on?

+# This function checks if a vpn ipsec connection exists +# Returns True when yes and false when not +vpn_ipsec_connection_exists() {

• assert [ $# -eq 1 ]
• local connection=${1}
• local path=$(vpn_ipsec_connection_path "${connection}")
• [ -f ${path} ] && return ${EXIT_TRUE} || return

${EXIT_FALSE} +}

Always use "" when you are passing an argument to [.

+# Handle the cli after authentification +vpn_ipsec_connection_authentication() {

• if [ ! $# -gt 1 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local cmd=${2}
• shift 2
• case ${cmd} in

• mode)
  

• 	vpn_ipsec_connection_authentication_mode
  

${connection} $@

• 	;;
  

• pre-shared-key)
  

• 	vpn_ipsec_connection_authentication_psk
  

${connection} $@

• 	;;
  

• *)
  

• 	log ERROR "Unrecognized argument: ${cmd}"
  

• 	return ${EXIT_ERROR}
  

• 	;;
  

• esac

+}

+# Set the authentification mode +vpn_ipsec_connection_authentication_mode() {

• if [ ! $# -eq 2]; then

Space missing after the 2.

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local mode=${2}
• if ! isoneof mode "psk" "PSK"; then

• log ERROR "Auth mode '${mode}' is not valid"
  

• return ${EXIT_ERROR}
  

• fi
• if ! vpn_ipsec_connection_write_config_key ${connection}

"AUTH_MODE" ${mode,,}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

+# Set the psk +vpn_ipsec_connection_authentication_psk() {

• if [ ! $# -eq 2]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local psk=${2}
• # TODO Check if psk is valid
• if ! vpn_ipsec_connection_write_config_key ${connection}

"PSK" ${psk}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

I think there is a return missing at the end.

What is a valid PSK?

+# Handle the cli after local +vpn_ipsec_connection_local() {

• if [ ! $# -ge 2 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local cmd=${2}
• shift 2
• case ${cmd} in

• prefix)
  

• 	vpn_ipsec_connection_prefix ${connection}
  

"LOCAL" $@

• 	;;
  

• id)
  

• 	vpn_ipsec_connection_id ${connection}
  

"LOCAL" $@

• 	;;
  

Ideally I would like these in alphabetical order. Makes solving merge conflicts easier.

• *)
  

• 	log ERROR "Unrecognized argument: ${cmd}"
  

• 	return ${EXIT_ERROR}
  

• 	;;
  

• esac
• return ${EXIT_OK}

+}

+# Set the connection mode +vpn_ipsec_connection_mode() {

• if [ ! $# -eq 2]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local mode=${2}
• if ! isoneof mode "tunnel" "vti" "gre-transport"; then

• log ERROR "Mode '${mode}' is not valid"
  

• return ${EXIT_ERROR}
  

• fi

We should have a variable for all valid modes at the top of the file.

• if ! vpn_ipsec_connection_write_config_key ${connection}

"MODE" ${mode}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

Same here. Either no empty line or a return...

+# Set the peer to connect to +vpn_ipsec_connection_peer() {

• if [ ! $# -eq 2]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local peer=${2}
• if ! vpn_ipsec_connection_check_peer ${peer}; then

• log ERROR "Peer '${peer}' is not valid"
  

• return ${EXIT_ERROR}
  

• fi

There is a word for "not valid". It is "invalid".

• if ! vpn_ipsec_connection_write_config_key ${connection}

"PEER" ${peer}; then

At the top you used "" around ${connection}. I think that would be a good idea generally.

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

+#Set the local or remote id +vpn_ipsec_connection_id() {

• if [ ! $# -eq 3]; then

Space.

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local type=${2}
• local id=${3}
• if ! vpn_ipsec_connection_check_id ${id}; then

• log ERROR "Id '${id}' is not valid"
  

• return ${EXIT_ERROR}
  

• fi
• if ! vpn_ipsec_connection_write_config_key ${connection}

"${type}_ID" ${id}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi
• return ${EXIT_OK}

+}

+# Set the local or remote prefix  +vpn_ipsec_connection_prefix() {

• if [ ! $# -ge 3 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local type=${2}
• shift 2
• local _prefix="${type}_PREFIX"
• local "${_prefix}"
• if ! vpn_ipsec_connection_read_config ${connection}

"${_prefix}"; then

• return ${EXIT_ERROR}
  

• fi
• # Remove duplicated entries to proceed the list safely
• assign "${_prefix}" "$(list_unique ${!_prefix} )"
• local prefixes_added
• local prefixes_removed
• local prefixes_set
• while [ $# -gt 0 ]; do

• local arg="${1}"
  

• case "${arg}" in
  

• 	+*)
  

• 		list_append prefixes_added
  

"${arg:1}"

• 		;;
  

• 	-*)
  

• 		list_append prefixes_removed
  

"${arg:1}"

• 		;;
  

• 	[A-Z0-9]*)
  

• 		list_append prefixes_set "${arg}"
  

• 		;;
  

A prefix would never start with anything after G-Z.

• 	*)
  

• 		error "Invalid argument: ${arg}"
  

• 		return ${EXIT_ERROR}
  

• 		;;
  

• esac
  

• shift
  

• done
• # Check if the user is trying a mixed operation
• if ! list_is_empty prefixes_set && (! list_is_empty

prefixes_added || ! list_is_empty prefixes_removed); then

• error "You cannot reset the prefix list and add or
  

remove prefixes at the same time"

• return ${EXIT_ERROR}
  

• fi
• # Set new prefixe list

Typo

• if ! list_is_empty prefixes_set; then

• # Check if all prefixes are valid
  

• local prefix
  

• for prefix in ${prefixes_set}; do
  

• 	if ! ip_net_is_valid ${prefix}; then
  

• 		error "Unsupported prefix:
  

${prefix}"

• 		return ${EXIT_ERROR}
  

• 	fi
  

• done
  

• assign "${_prefix}" "${prefixes_set}"
  

• # Perform incremental updates
• else

• local prefix
  

• # Perform all removals
  

• for prefix in ${prefixes_removed}; do
  

• 	if ! list_remove "${_prefix}" ${prefix};
  

then

• 		warning "${prefix} was not on the
  

list and could not be removed"

• 	fi
  

• done
  

• for prefix in ${prefixes_added}; do
  

• 	if ip_net_is_valid ${prefix}; then
  

• 		if ! list_append_unique "${_prefix}"
  

${prefix}; then

• 			warning "${prefix} is
  

already on the prefix list"

• 		fi
  

• 	else
  

• 		warning "${prefix} is not a valiv IP
  

net and could not be added"

• 	fi
  

• done
  

• fi
• # Check if the list contain at least one valid prefixe

Typo

• if list_is_empty ${_prefix}; then

• error "Cannot save an empty prefix list"
  

• return ${EXIT_ERROR}
  

• fi
• # Save everything
• if ! vpn_ipsec_connection_write_config_key ${connection}

"${_prefix}" ${!_prefix}; then

• log ERROR "Could not write configuration settings"
  

• fi

+}

+# Handle the cli after remote +vpn_ipsec_connection_remote() {

• if [ ! $# -ge 2 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local cmd=${2}
• shift 2
• case ${cmd} in

• prefix)
  

• 	vpn_ipsec_connection_prefix ${connection}
  

"REMOTE" $@

• 	;;
  

• id)
  

• 	vpn_ipsec_connection_id ${connection}
  

"REMOTE" $@

• 	;;
  

Same as above.

• *)
  

• 	log ERROR "Unrecognized argument: ${cmd}"
  

• 	return ${EXIT_ERROR}
  

• 	;;
  

• esac
• return ${EXIT_OK}

+}

+# Set the inactivity timeout +vpn_ipsec_connection_inactivity_timeout() {

• if [ ! $# -ge 2 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• shift 1
• local value=$@
• if ! isinteger value; then

• value=$(parse_time $@)
  

• if [ ! $? -eq 0 ]; then
  

• 	log ERROR "Parsing the passed time was not
  

sucessful please check the passed values."

• 	return ${EXIT_ERROR}
  

• fi
  

• fi
• if [ ${value} -le 0 ]; then

• log ERROR "The passed time value must be in the sum
  

greater zero seconds."

• return ${EXIT_ERROR}
  

• fi
• if ! vpn_ipsec_connection_write_config_key ${connection}

"INACTIVITY_TIMEOUT" ${value}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

+# Set the security policy to use +vpn_ipsec_connection_security_policy() {

• if [ ! $# -eq 2 ]; then

• log ERROR "Not enough arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection=${1}
• local security_policy=${2}
• if ! vpn_security_policy_exists ${security_policy}; then

• log ERROR "No such vpn security policy
  

'${security_policy}'"

• return ${EXIT_ERROR}
  

• fi
• if ! vpn_ipsec_connection_write_config_key ${connection}

"SECURITY_POLICY" ${security_policy}; then

• log ERROR "Could not write configuration settings"
  

• return ${EXIT_ERROR}
  

• fi

+}

Actually we now also need to check for a security policy if it is in use when ever someone tries to delete it.

If someone deleted the file we need to fall back to "system".

+# Check if a id is valid +vpn_ipsec_connection_check_id() {

• assert [ $# -eq 1 ]
• local id=${1}
• if [[ ${id} =~ ^@[[:alnum:]]+$ ]] || ip_is_valid ${id}; then

• return ${EXIT_TRUE}
  

• else

• return ${EXIT_FALSE}
  

• fi

+}

+# Checks if a peer is valid +vpn_ipsec_connection_check_peer() {

• assert [ $# -eq 1 ]
• local peer=${1}
• # TODO Accept also FQDNs
• if ip_is_valid ${peer}; then

• return ${EXIT_TRUE}
  

• else

• return ${EXIT_FALSE}
  

• fi

+}

+# This function checks if a VPN IPsec connection name is valid +# Allowed are only A-Za-z0-9 +vpn_ipsec_connection_check_name() {

• assert [ $# -eq 1 ]
• local connection=${1}
• [[ ${connection} =~ [^[:alnum:]$] ]]

+}

+# Function that creates one VPN IPsec connection +vpn_ipsec_connection_new() {

• if [ $# -gt 1 ]; then

• error "Too many arguments"
  

• return ${EXIT_ERROR}
  

• fi
• local connection="${1}"
• if ! isset connection; then

• error "Please provide a connection name"
  

• return ${EXIT_ERROR}
  

• fi
• # Check for duplicates
• if vpn_ipsec_connection_exists "${connection}"; then

• error "The VPN IPsec connection ${connection}
  

already exists"

• return ${EXIT_ERROR}
  

• fi
• # Check if the name of the connection is valid
• if  vpn_ipsec_connection_check_name "${connection}"; then

• error "'${connection}' contains illegal characters"
  

• return ${EXIT_ERROR}
  

• fi
• log DEBUG "Creating VPN IPsec connection ${connection}"

I think the defaults should be set here.

• touch $(vpn_ipsec_connection_path ${connection})

You don't need to touch the file before you write to it.

• vpn_ipsec_connection_write_config "${connection}"

+}

+# Function that deletes based on the passed parameters one ore more vpn security policies +vpn_ipsec_connection_destroy() {

• local connection
• for connection in $@; do

• if ! vpn_ipsec_connection_exists ${connection}; then
  

• 	log ERROR "The VPN IPsec connection
  

${connection} does not exist."

• 	continue
  

• fi
  

• log DEBUG "Deleting VPN IPsec connection
  

${connection}"

• settings_remove $(vpn_ipsec_connection_path
  

${connection})

• done

+}

-Michael