[RFC,1/2] Add vpn-ipsec functions

Message ID	1501090354-17617-1-git-send-email-jonatan.schlag@ipfire.org
State	New
Headers	From: Jonatan Schlag <jonatan.schlag@ipfire.org> To: network@lists.ipfire.org Subject: [RFC PATCH 1/2] Add vpn-ipsec functions Date: Wed, 26 Jul 2017 19:32:33 +0200 Message-Id: <1501090354-17617-1-git-send-email-jonatan.schlag@ipfire.org> Precedence: list Errors-To: network-bounces@lists.ipfire.org Sender: "network" <network-bounces@lists.ipfire.org>

Message ID

1501090354-17617-1-git-send-email-jonatan.schlag@ipfire.org

State

New

Headers

From: Jonatan Schlag <jonatan.schlag@ipfire.org>
To: network@lists.ipfire.org
Subject: [RFC PATCH 1/2] Add vpn-ipsec functions
Date: Wed, 26 Jul 2017 19:32:33 +0200
Message-Id: <1501090354-17617-1-git-send-email-jonatan.schlag@ipfire.org>
Precedence: list
Errors-To: network-bounces@lists.ipfire.org
Sender: "network" <network-bounces@lists.ipfire.org>

Message

Jonatan Schlag July 27, 2017, 3:32 a.m. UTC

  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

Comments

Michael Tremer July 28, 2017, 1:43 a.m. UTC | #1

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

Jonatan Schlag July 29, 2017, 1:04 a.m. UTC | #2

Hi,

Am Do, 27. Jul, 2017 um 5:43 schrieb Michael Tremer 
<michael.tremer@ipfire.org>:
> 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.

Unfourtunately I have to touch the file bevore I write to it.
This check fails without touch

 + if [ ! -w ${path} ]; then
 +	 log ERROR "${path} is not writeable"
 +	 return ${EXIT_ERROR}
 +	fi

The rest is ok i will send a improved version.

Jonatan

> 
> 
>>  +	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