new feature vpn-security-policies

new feature vpn-security-policies

[Jonatan Schlag]

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

The following two patches add the base for the new vpn security policies feature.
They are defenitely not perfetc and because of that i want to ask for commands.
This would help me to bring this patches into a mergeable format.

[RFC 1/2] vpn-security-policies: add new feature vpn security-policies

[Jonatan Schlag]

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

For further explanation see:
http://wiki.ipfire.org/devel/network/security-policies

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/functions/functions.vpn-security-policies | 437 ++++++++++++++++++++++++++
 1 file changed, 437 insertions(+)
 create mode 100644 src/functions/functions.vpn-security-policies

diff --git a/src/functions/functions.vpn-security-policies b/src/functions/functions.vpn-security-policies
new file mode 100644
index 0000000..9e24e4f
--- /dev/null
+++ b/src/functions/functions.vpn-security-policies
@@ -0,0 +1,437 @@
+#!/bin/bash
+###############################################################################
+#                                                                             #
+# IPFire.org - A linux based firewall                                         #
+# Copyright (C) 2013  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_SECURITY_POLICIES_CONFIG_SETTINGS="CIPHER COMPRESSION GROUP_TYPE INTEGRITY KEY_EXCHANGE LIFETIME PFS"
+VPN_SECURITY_POLICIES_READONLY="system"
+
+VPN_SUPPORTED_CIPHERS="AES192 AES256 AES128"
+VPN_SUPPORTED_INTEGRITY="SHA512 SHA256 SHA128"
+VPN_SUPPORTED_GROUP_TYPES="MODP4096 MODP8192"
+
+vpn_security_policies_check_readonly() {
+	# This functions checks if a policy is readonly
+	# returns true when yes and false when no
+
+	if isoneof name ${VPN_SECURITY_POLICIES_READONLY}; then
+		return ${EXIT_TRUE}
+	else
+		return ${EXIT_FALSE}
+	fi
+}
+
+vpn_security_policies_write_config() {
+	# This function writes all values to a via ${name} specificated vpn security policy configuration file
+	assert [ $# -ge 1 ]
+
+	local name="${1}"
+
+	if ! vpn_security_policy_exists ${name}; then
+		log ERROR "No such policy: ${name}"
+		return ${EXIT_ERROR}
+	fi
+
+	if vpn_security_policies_check_readonly ${name}; then
+		log ERROR "${name} is a readable only vpn security policy"
+		return ${EXIT_ERROR}
+	fi
+
+	local args
+	list_append args ${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}
+
+	local path="$(vpn_security_policies_path ${name})"
+	if [ ! -w ${path} ]; then
+		log ERROR "${path} is not writeable"
+		return ${EXIT_ERROR}
+	fi
+
+	settings_write "${path}" ${args}
+	# TODO everytime we successfully write a config we should call some trigger to take the changes into effect
+}
+
+vpn_security_policies_write_config_key() {
+	# This funtion writes the value for one key to a via ${name} specificated vpn security policy configuration file
+	assert [ $# -ge 3 ]
+	local name=${1}
+
+	if ! vpn_security_policy_exists ${name}; then
+		log ERROR "No such policy: ${name}"
+		return ${EXIT_ERROR}
+	fi
+
+	local key=${2}
+	shift 2
+	local value="$@"
+	log DEBUG "Set '${key}' to new value '${value}'"
+
+	# Read the config settings
+	if ! vpn_security_policies_read_config ${name}; then
+		return ${EXIT_ERROR}
+	fi
+
+	# Set the key to a new value
+	eval "${key}=\"${value}\""
+
+	if ! vpn_security_policies_write_config ${name}; then
+		return ${EXIT_ERROR}
+	fi
+
+	return ${EXIT_TRUE}
+
+}
+
+vpn_security_policies_read_config() {
+	# Reads one or more keys out of a settings file or all if no key is provided.
+	assert [ $# -ge 1 ]
+
+	local name="${1}"
+	shift 1
+
+	if ! vpn_security_policy_exists ${name}; then
+		log ERROR "No such policy: ${name}"
+		return ${EXIT_ERROR}
+	fi
+
+
+	local args
+	if [ $# -eq 0 ] && [ -n "${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}" ]; then
+		list_append args ${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}
+	else
+		list_append args $@
+	fi
+
+	local path="$(vpn_security_policies_path ${name})"
+	if [ ! -r ${path} ]; then
+		log ERROR "${path} is not readable"
+		return ${EXIT_ERROR}
+	fi
+
+	settings_read "${path}" ${args}
+}
+
+vpn_security_policies_path() {
+	# Returns the path to a the configuration fora given name
+	assert [ $# -eq 1 ]
+	local name=${1}
+
+	if vpn_security_policies_check_readonly ${name}; then
+		echo "/usr/share/network/vpn/security-policies/${name}"
+	else
+		echo "/etc/network/vpn/security-policies/${name}"
+	fi
+}
+
+vpn_security_policies_show() {
+	# Print the content of a vpn security policy configuration file in a nice way
+	assert [ $# -eq 1 ]
+	local name=${1}
+
+	# Break if read fails
+	if ! vpn_security_policies_read_config ${name}; then
+		return ${EXIT_ERROR}
+	fi
+
+	cli_print_fmt1 0 "Security Policy: ${name}"
+	cli_space
+
+	# This could be done in a loop but a loop is much more complicated
+	# because we print 'Group Types' but the variable is named 'GROUP_TYPES'
+	cli_print_fmt1 1 "Ciphers:"
+	cli_print_fmt1 2 "${CIPHER}"
+	cli_space
+	cli_print_fmt1 1 "Integrity:"
+	cli_print_fmt1 2 "${INTEGRITY}"
+	cli_space
+	cli_print_fmt1 1 "Group Types:"
+	cli_print_fmt1 2 "${GROUP_TYPE}"
+	cli_space
+
+	cli_print_fmt1 1 "Key exchange:" "${KEY_EXCHANGE}"
+	cli_print_fmt1 1 "Key Lifetime in seconds:" "${LIFETIME}"
+	if enabled PFS; then
+		cli_print_fmt1 1 "Perfect Forward Secrecy:" "enabled"
+	else
+		cli_print_fmt1 1 "Perfect Forward Secrecy:" "disabled"
+	fi
+	cli_space
+	if enabled COMPRESSION; then
+		cli_print_fmt1 1 "Compression:" "enabled"
+	else
+		cli_print_fmt1 1 "Compression:" "disabled"
+	fi
+	cli_space
+}
+
+vpn_security_policy_exists() {
+	# This function checks if a vpn security policy exists
+	# Returns True when yes and false when not
+	assert [ $# -eq 1 ]
+	local name=${1}
+
+	local path=$(vpn_security_policies_path ${name})
+	[ -f ${path} ]
+}
+
+
+vpn_security_policies_cipher(){
+	# This function parses the parameters for the 'cipher' command
+	local name=${1}
+	shift
+
+	if [ $# -eq 0 ]; then
+		log ERROR "You must pass at least one value after cipher"
+		return ${EXIT_ERROR}
+	fi
+
+	if ! vpn_security_policies_read_config ${name} "CIPHER"; then
+		return ${EXIT_ERROR}
+	fi
+
+	# Remove duplicated entries to proceed the list safely
+	CIPHER="$(list_unique ${CIPHER})"
+
+	while [ $# -gt 0 ]; do
+		case "${1}" in
+			-*)
+				value=${1#-}
+				# Check if the cipher is in the list of ciphers and
+				# check if the list has after removing this cipher at least one valid value
+				if list_match ${value} ${CIPHER} && [ $(list_length ${CIPHER}) -gt 1 ]; then
+					list_remove CIPHER ${value}
+				else
+					log ERROR "Can not remove ${value} from the list of Ciphers because ${value} is not in the list or the list would be empty after removing this cipher"
+				fi
+				;;
+			+*)
+				value=${1#+}
+				# Check if the Ciphers is in the list of supported ciphers.
+				if ! isoneof value ${VPN_SUPPORTED_CIPHERS}; then
+					log ERROR "${value} is not a supported cipher and can thats why not added to the list of ciphers"
+				else
+					if list_match ${value} ${CIPHER}; then
+						log WARNING "${value} is already in the list of ciphers of this policy"
+					else
+						list_append CIPHER ${value}
+					fi
+				fi
+				;;
+		esac
+		shift
+	done
+
+	vpn_security_policies_write_config_key ${name} "CIPHER" ${CIPHER}
+}
+
+vpn_security_policies_compression(){
+	# This function parses the parameters for the 'compression' command
+	local name=${1}
+	local value=${2}
+	# Check if we get only one argument after compression <name>
+	if [ ! $# -eq 2 ] || ! isbool value; then
+		log ERROR "You can pass only one parameter after compression and this one must be 'on' or 'off'"
+		return ${EXIT_ERROR}
+	fi
+
+	vpn_security_policies_write_config_key "${name}" "COMPRESSION" "${value}"
+}
+
+vpn_security_policies_group_type(){
+	# This function parses the parameters for the 'group-type' command.
+	local name=${1}
+	shift
+
+	if [ $# -eq 0 ]; then
+		log ERROR "You must pass at least one value after group-type"
+		return ${EXIT_ERROR}
+	fi
+
+	if ! vpn_security_policies_read_config ${name} "GROUP_TYPE"; then
+		return ${EXIT_ERROR}
+	fi
+
+	# Remove duplicated entries to proceed the list safely
+	GROUP_TYPE="$(list_unique ${GROUP_TYPE})"
+
+	while [ $# -gt 0 ]; do
+		case "${1}" in
+			-*)
+				value=${1#-}
+				# Check if the group type is in the list of group types and
+				# check if the list has after removing this group type at leatst one valid value
+				if list_match ${value} ${GROUP_TYPE} && [ $(list_length ${GROUP_TYPE}) -gt 1 ]; then
+					list_remove GROUP_TYPE ${value}
+				else
+					log ERROR "Can not remove ${value} from the list of group types because ${value} is not in the list or the list would be empty after removing this group type"
+				fi
+				;;
+			+*)
+				value=${1#+}
+				# Check if the group type is in the list of supported group types.
+				if ! isoneof value ${VPN_SUPPORTED_GROUP_TYPES}; then
+					log ERROR "${value} is not a supported group type and can thats why not added to the list of group types"
+				else
+					if list_match ${value} ${GROUP_TYPE}; then
+						log WARNING "${value} is already in the list of group-types of this policy"
+					else
+						list_append GROUP_TYPES ${value}
+					fi
+				fi
+				;;
+		esac
+		shift
+	done
+
+	vpn_security_policies_write_config_key ${name} "GROUP_TYPE" ${GROUP_TYPE}
+}
+
+vpn_security_policies_integrity(){
+	# This function parses the parameters for the 'integrity' command
+	local name=${1}
+	shift
+
+	if [ $# -eq 0 ]; then
+		log ERROR "You must pass at least one value after integrity"
+		return ${EXIT_ERROR}
+	fi
+
+	if ! vpn_security_policies_read_config ${name} "INTEGRITY"; then
+		return ${EXIT_ERROR}
+	fi
+
+	# Remove duplicated entries to proceed the list safely
+	INTEGRITY="$(list_unique ${INTEGRITY})"
+
+	while [ $# -gt 0 ]; do
+		case "${1}" in
+			-*)
+				value=${1#-}
+				# Check if the integrity hash is in the list of integrity hashes and
+				# check if the list has after removing this  integrity hash at least one valid value
+				if list_match ${value} ${INTEGRITY} && [ $(list_length ${INTEGRITY}) -gt 1 ]; then
+					list_remove INTEGRITY ${value}
+				else
+					log ERROR "Can not remove ${value} from the list of integrity hashes because ${value} is not in the list or the list would be empty after removing this integrity hash"
+				fi
+				;;
+			+*)
+				value=${1#+}
+				# Check if the Ciphers is in the list of supported integrity hashes.
+				if ! isoneof value ${VPN_SUPPORTED_INTEGRITY}; then
+					log ERROR "${value} is not a supported integrity hashes and can thats why not added to the list of integrity hashes"
+				else
+					if list_match ${value} ${INTEGRITY}; then
+						log WARNING "${value} is already in the list of integrety hashes of this policy"
+					else
+						list_append INTEGRITY ${value}
+					fi
+				fi
+				;;
+		esac
+		shift
+	done
+
+	vpn_security_policies_write_config_key ${name} "INTEGRITY" ${INTEGRITY}
+}
+
+vpn_security_policies_key_exchange() {
+	# This function parses the parameters for the 'key-exchange' command
+	local name=${1}
+	local value=${2}
+	# Check if we get only one argument after compression <name>
+	if [ ! $# -eq 2 ] || ! isoneof value "ikev1" "ikev2"; then
+		log ERROR "You can pass only one parameter after key-exchange and this one must be 'ikev1' or 'ikev2'"
+		return ${EXIT_ERROR}
+	fi
+
+	vpn_security_policies_write_config_key "${name}" "KEY_EXCHANGE" "${value}"
+}
+
+vpn_security_policies_lifetime(){
+	# This function parses the parameters for the 'lifetime' command
+	local name=${1}
+	local value=${2}
+	# Check if we get only one argument after compression <name>
+	if [ ! $# -eq 2 ] || ! isinteger value || [ ${value} -le 0 ]; then
+		log ERROR "You can pass only one parameter after liftime and this one must be an valid integer greater zero"
+		return ${EXIT_ERROR}
+	fi
+
+	vpn_security_policies_write_config_key "${name}" "LIFETIME" "${value}"
+}
+
+vpn_security_policies_pfs(){
+	# This function parses the parameters for the 'pfs' command
+	local name=${1}
+	local value=${2}
+	# Check if we get only one argument after compression <name>
+	if [ ! $# -eq 2 ] || ! isbool value; then
+		log ERROR "You can pass only one parameter after pfs and this one must be 'on' or 'off'"
+		return ${EXIT_ERROR}
+	fi
+
+	vpn_security_policies_write_config_key "${name}" "PFS" "${value}"
+}
+
+vpn_security_policies_check_name() {
+	# This function checks if a vpn security policy name is valid
+	# Allowed are only A-Za-z0-9
+	assert [ $# -eq 1 ]
+	local name=${1}
+	[[ ${name} =~ [[:alnum:]] ]]
+}
+
+vpn_security_policies_new() {
+	# Function that creates based on the paramters one ore more new vpn security policies
+	local name
+	for name in $@; do
+		if vpn_security_policy_exists ${name}; then
+			log ERROR "The vpn security policy ${name} does already exist"
+			continue
+		fi
+
+		if vpn_security_policies_check_name ${name}; then
+			log ERROR "'${name}' contains illegal characters. Allowed are only A-Za-z0-9 are allowed"
+			continue
+		fi
+
+		log DEBUG "Creating vpn security policy ${name}"
+		cp "$(vpn_security_policies_path "system")" "$(vpn_security_policies_path ${name})"
+	done
+
+}
+
+vpn_security_policies_destroy() {
+	# Function that deletes based on the passed parameters one ore more vpn security policies
+	local name
+	for name in $@; do
+		if ! vpn_security_policy_exists ${name}; then
+			log ERROR "The vpn security policy ${name} does not exist"
+			continue
+		fi
+
+		if vpn_security_policies_check_readonly ${name}; then
+			log ERROR "The vpn security policy ${name} is readonly and can thats why not deleted"
+			continue
+		fi
+
+		log DEBUG "Deleting vpn security policy ${name}"
+		rm -f $(vpn_security_policies_path ${name})
+	done
+}
-- 
2.6.3

[RFC 2/2] network: add vpn security policies commands

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/network | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/src/network b/src/network
index 154d253..e7b637c 100644
--- a/src/network
+++ b/src/network
@@ -1194,7 +1194,6 @@ cli_reset() {
 	fi
 
 	warning_log "Will reset the whole network configuration!!!"
-
 	# Force mode is disabled by default
 	local force=0
 
@@ -1384,13 +1383,77 @@ cli_raw() {
 	exit ${EXIT_OK}
 }
 
+cli_vpn() {
+
+	local action
+	action=${1}
+	shift 1
+
+	case "${action}" in
+		security-policies)
+			cli_vpn_security-policies $@
+			;;
+		*)
+			error "Unrecognized argument: ${action}"
+			exit ${EXIT_ERROR}
+			;;
+	esac
+}
+
+cli_vpn_security-policies() {
+
+	local action
+	local security_policy
+
+	if vpn_security_policy_exists ${1}; then
+
+		security_policy=${1}
+		key=${2}
+		shift 2
+
+		case "${key}" in
+			cipher|compression|integrity|lifetime|pfs|show)
+				vpn_security_policies_${key} ${security_policy} $@
+				;;
+			group-type)
+				vpn_security_policies_group_type ${security_policy} $@
+				;;
+			key-exchange)
+				vpn_security_policies_key_exchange ${security_policy} $@
+				;;
+			*)
+				error "Unrecognized argument: ${key}"
+				exit ${EXIT_ERROR}
+				;;
+		esac
+	else
+		action=${1}
+		shift
+
+		case "${action}" in
+			new)
+				vpn_security_policies_new $@
+				;;
+			destroy)
+				vpn_security_policies_destroy $@
+				;;
+			""|*)
+				if [ -n "${action}" ]; then
+					error "Unrecognized argument: '${action}'"
+				fi
+				exit ${EXIT_ERROR}
+				;;
+		esac
+	fi
+}
+
 # Process the given action
 case "${action}" in
 	init)
 		init_run
 		;;
 
-	settings|hostname|port|device|zone|start|stop|restart|status|reset|route)
+	settings|hostname|port|device|zone|start|stop|restart|status|reset|route|vpn)
 		cli_${action} $@
 		;;
 
-- 
2.6.3

Re: [RFC 1/2] vpn-security-policies: add new feature vpn security-policies

[Michael Tremer]

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

Hi,

On Thu, 2017-07-13 at 20:33 +0200, Jonatan Schlag wrote:
> For further explanation see:
> http://wiki.ipfire.org/devel/network/security-policies
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.vpn-security-policies | 437
> ++++++++++++++++++++++++++
>  1 file changed, 437 insertions(+)
>  create mode 100644 src/functions/functions.vpn-security-policies
> 
> diff --git a/src/functions/functions.vpn-security-policies
> b/src/functions/functions.vpn-security-policies
> new file mode 100644
> index 0000000..9e24e4f
> --- /dev/null
> +++ b/src/functions/functions.vpn-security-policies
> @@ -0,0 +1,437 @@
> +#!/bin/bash
> +####################################################################
> ###########
> +#                                                                   
>           #
> +# IPFire.org - A linux based
> firewall                                         #
> +# Copyright (C) 2013  IPFire Network Development
> Team                         #
> +#                                                                   
>           #

It's 2017 :)

> +# 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_SECURITY_POLICIES_CONFIG_SETTINGS="CIPHER COMPRESSION GROUP_TYPE
> INTEGRITY KEY_EXCHANGE LIFETIME PFS"
> +VPN_SECURITY_POLICIES_READONLY="system"
> +
> +VPN_SUPPORTED_CIPHERS="AES192 AES256 AES128"
> +VPN_SUPPORTED_INTEGRITY="SHA512 SHA256 SHA128"
> +VPN_SUPPORTED_GROUP_TYPES="MODP4096 MODP8192"

Obviously these need to be extended. But this will suit as a proof-of-
concept. Just please order stuff alphabetically. In this case it is
best to do it descending.

> +vpn_security_policies_check_readonly() {
> +	# This functions checks if a policy is readonly
> +	# returns true when yes and false when no
> +
> +	if isoneof name ${VPN_SECURITY_POLICIES_READONLY}; then
> +		return ${EXIT_TRUE}
> +	else
> +		return ${EXIT_FALSE}
> +	fi
> +}

Ok.

> +vpn_security_policies_write_config() {
> +	# This function writes all values to a via ${name}
> specificated vpn security policy configuration file
> +	assert [ $# -ge 1 ]
> +
> +	local name="${1}"
> +
> +	if ! vpn_security_policy_exists ${name}; then
> +		log ERROR "No such policy: ${name}"
> +		return ${EXIT_ERROR}
> +	fi

I think you need to improve some of the error messages. A sentence is
usually good. The shorter the sweeter. Not sure if "no such policy" is
helpful when they are actually called security policies. So, don't go
too short.

> +
> +	if vpn_security_policies_check_readonly ${name}; then
> +		log ERROR "${name} is a readable only vpn security
> policy"
> +		return ${EXIT_ERROR}
> +	fi

Here I would suggest: "The system security policy cannot be changed"

> +
> +	local args
> +	list_append args ${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}

No need to call list_append here. Settings args to
VPN_SECURITY_POLICIES_CONFIG_SETTINGS is enough. You just just pass it
to settings_write without using $args.

> +
> +	local path="$(vpn_security_policies_path ${name})"
> +	if [ ! -w ${path} ]; then
> +		log ERROR "${path} is not writeable"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	settings_write "${path}" ${args}
> +	# TODO everytime we successfully write a config we should
> call some trigger to take the changes into effect
> +}

Space between the settings_write and comments, please.

What happens when the settings could not be written?

> +vpn_security_policies_write_config_key() {
> +	# This funtion writes the value for one key to a via ${name}
> specificated vpn security policy configuration file
> +	assert [ $# -ge 3 ]
> +	local name=${1}
> +
> +	if ! vpn_security_policy_exists ${name}; then
> +		log ERROR "No such policy: ${name}"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	local key=${2}
> +	shift 2
> +	local value="$@"
> +	log DEBUG "Set '${key}' to new value '${value}'"

Please group the argument parsing together at the top and then test if
the policy exists. This is hard to read.

Also the logging message is a bit random since it doesn't mention the
security policy here.

> +	# Read the config settings
> +	if ! vpn_security_policies_read_config ${name}; then
> +		return ${EXIT_ERROR}
> +	fi

This will overwrite any global variables. So you will need to
call

  local VPN_SECURITY_POLICIES_CONFIG_SETTINGS

before you read the settings from file.

> +	# Set the key to a new value
> +	eval "${key}=\"${value}\""

Never ever use eval. This will allow someone to pass shell commands
that may get executed. At no time ever we would want that.

There is a function called assign() that takes care of assigning a
variable.

> +	if ! vpn_security_policies_write_config ${name}; then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	return ${EXIT_TRUE}
> +
> +}
> +
> +vpn_security_policies_read_config() {
> +	# Reads one or more keys out of a settings file or all if no
> key is provided.
> +	assert [ $# -ge 1 ]
> +
> +	local name="${1}"
> +	shift 1
> +
> +	if ! vpn_security_policy_exists ${name}; then
> +		log ERROR "No such policy: ${name}"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +
> +	local args
> +	if [ $# -eq 0 ] && [ -n
> "${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}" ]; then
> +		list_append args
> ${VPN_SECURITY_POLICIES_CONFIG_SETTINGS}
> +	else
> +		list_append args $@
> +	fi
> +
> +	local path="$(vpn_security_policies_path ${name})"
> +	if [ ! -r ${path} ]; then
> +		log ERROR "${path} is not readable"
> +		return ${EXIT_ERROR}
> +	fi

Why do you need to do this every time? Doesn't settings_read do this?

> +
> +	settings_read "${path}" ${args}
> +}
> +
> +vpn_security_policies_path() {
> +	# Returns the path to a the configuration fora given name
> +	assert [ $# -eq 1 ]
> +	local name=${1}
> +
> +	if vpn_security_policies_check_readonly ${name}; then
> +		echo "/usr/share/network/vpn/security-
> policies/${name}"
> +	else
> +		echo "/etc/network/vpn/security-policies/${name}"
> +	fi
> +}

Hard-coded paths :(

> +
> +vpn_security_policies_show() {
> +	# Print the content of a vpn security policy configuration
> file in a nice way
> +	assert [ $# -eq 1 ]
> +	local name=${1}

Here you will need to do the local variable initialization before you
red this in - as above.

> +	# Break if read fails
> +	if ! vpn_security_policies_read_config ${name}; then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	cli_print_fmt1 0 "Security Policy: ${name}"
> +	cli_space
> +
> +	# This could be done in a loop but a loop is much more
> complicated
> +	# because we print 'Group Types' but the variable is named
> 'GROUP_TYPES'
> +	cli_print_fmt1 1 "Ciphers:"
> +	cli_print_fmt1 2 "${CIPHER}"
> +	cli_space

This does not show any broken ciphers as broken. But we could add this
later as well. This patch is quite huge already.

> +	cli_print_fmt1 1 "Integrity:"
> +	cli_print_fmt1 2 "${INTEGRITY}"
> +	cli_space
> +	cli_print_fmt1 1 "Group Types:"
> +	cli_print_fmt1 2 "${GROUP_TYPE}"
> +	cli_space
> +
> +	cli_print_fmt1 1 "Key exchange:" "${KEY_EXCHANGE}"

Capital E in Exchange

> +	cli_print_fmt1 1 "Key Lifetime in seconds:" "${LIFETIME}"

The lifetime should be formatted nicely because seconds are hard to
understand. There is a function that formats these nicely.

> +	if enabled PFS; then
> +		cli_print_fmt1 1 "Perfect Forward Secrecy:"
> "enabled"
> +	else
> +		cli_print_fmt1 1 "Perfect Forward Secrecy:"
> "disabled"
> +	fi
> +	cli_space
> +	if enabled COMPRESSION; then
> +		cli_print_fmt1 1 "Compression:" "enabled"
> +	else
> +		cli_print_fmt1 1 "Compression:" "disabled"
> +	fi
> +	cli_space
> +}
> +
> +vpn_security_policy_exists() {
> +	# This function checks if a vpn security policy exists
> +	# Returns True when yes and false when not
> +	assert [ $# -eq 1 ]
> +	local name=${1}
> +
> +	local path=$(vpn_security_policies_path ${name})
> +	[ -f ${path} ]
> +}
> +
> +
> +vpn_security_policies_cipher(){
> +	# This function parses the parameters for the 'cipher'
> command
> +	local name=${1}
> +	shift
> +
> +	if [ $# -eq 0 ]; then
> +		log ERROR "You must pass at least one value after
> cipher"
> +		return ${EXIT_ERROR}
> +	fi
> +

local CIPHER

> +	if ! vpn_security_policies_read_config ${name} "CIPHER";
> then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	# Remove duplicated entries to proceed the list safely
> +	CIPHER="$(list_unique ${CIPHER})"
> +
> +	while [ $# -gt 0 ]; do
> +		case "${1}" in
> +			-*)
> +				value=${1#-}
> +				# Check if the cipher is in the list
> of ciphers and
> +				# check if the list has after
> removing this cipher at least one valid value
> +				if list_match ${value} ${CIPHER} &&
> [ $(list_length ${CIPHER}) -gt 1 ]; then
> +					list_remove CIPHER ${value}
> +				else
> +					log ERROR "Can not remove
> ${value} from the list of Ciphers because ${value} is not in the list
> or the list would be empty after removing this cipher"
> +				fi

There is no return here which ends the function on this error.

I think you should check this after the loop to support things like:

-3DES +AES256

This would crash if 3DES is the last cipher but the user does not
intend to remove all ciphers.

> +				;;
> +			+*)
> +				value=${1#+}
> +				# Check if the Ciphers is in the
> list of supported ciphers.
> +				if ! isoneof value
> ${VPN_SUPPORTED_CIPHERS}; then
> +					log ERROR "${value} is not a
> supported cipher and can thats why not added to the list of ciphers"
> +				else
> +					if list_match ${value}
> ${CIPHER}; then
> +						log WARNING
> "${value} is already in the list of ciphers of this policy"
> +					else
> +						list_append CIPHER
> ${value}
> +					fi
> +				fi
> +				;;
> +		esac
> +		shift
> +	done
> +
> +	vpn_security_policies_write_config_key ${name} "CIPHER"
> ${CIPHER}

Are there no errors to catch here?

> +}
> +
> +vpn_security_policies_compression(){
> +	# This function parses the parameters for the 'compression'
> command
> +	local name=${1}
> +	local value=${2}
> +	# Check if we get only one argument after compression <name>
> +	if [ ! $# -eq 2 ] || ! isbool value; then
> +		log ERROR "You can pass only one parameter after
> compression and this one must be 'on' or 'off'"
> +		return ${EXIT_ERROR}
> +	fi

Probably not the nicest handling in case of no argument or so. This
also suggests two values but actually more would be accepted as well.
> +
> +	vpn_security_policies_write_config_key "${name}"
> "COMPRESSION" "${value}"
> +}
> +
> +vpn_security_policies_group_type(){
> +	# This function parses the parameters for the 'group-type'
> command.
> +	local name=${1}
> +	shift
> +
> +	if [ $# -eq 0 ]; then
> +		log ERROR "You must pass at least one value after
> group-type"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	if ! vpn_security_policies_read_config ${name} "GROUP_TYPE";
> then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	# Remove duplicated entries to proceed the list safely
> +	GROUP_TYPE="$(list_unique ${GROUP_TYPE})"
> +
> +	while [ $# -gt 0 ]; do
> +		case "${1}" in
> +			-*)
> +				value=${1#-}
> +				# Check if the group type is in the
> list of group types and
> +				# check if the list has after
> removing this group type at leatst one valid value
> +				if list_match ${value} ${GROUP_TYPE}
> && [ $(list_length ${GROUP_TYPE}) -gt 1 ]; then
> +					list_remove GROUP_TYPE
> ${value}
> +				else
> +					log ERROR "Can not remove
> ${value} from the list of group types because ${value} is not in the
> list or the list would be empty after removing this group type"
> +				fi
> +				;;
> +			+*)
> +				value=${1#+}
> +				# Check if the group type is in the
> list of supported group types.
> +				if ! isoneof value
> ${VPN_SUPPORTED_GROUP_TYPES}; then
> +					log ERROR "${value} is not a
> supported group type and can thats why not added to the list of group
> types"
> +				else
> +					if list_match ${value}
> ${GROUP_TYPE}; then
> +						log WARNING
> "${value} is already in the list of group-types of this policy"
> +					else
> +						list_append
> GROUP_TYPES ${value}
> +					fi
> +				fi
> +				;;
> +		esac
> +		shift
> +	done
> +
> +	vpn_security_policies_write_config_key ${name} "GROUP_TYPE"
> ${GROUP_TYPE}
> +}

Same as above.

> +
> +vpn_security_policies_integrity(){
> +	# This function parses the parameters for the 'integrity'
> command
> +	local name=${1}
> +	shift
> +
> +	if [ $# -eq 0 ]; then
> +		log ERROR "You must pass at least one value after
> integrity"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	if ! vpn_security_policies_read_config ${name} "INTEGRITY";
> then
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	# Remove duplicated entries to proceed the list safely
> +	INTEGRITY="$(list_unique ${INTEGRITY})"
> +
> +	while [ $# -gt 0 ]; do
> +		case "${1}" in
> +			-*)
> +				value=${1#-}
> +				# Check if the integrity hash is in
> the list of integrity hashes and
> +				# check if the list has after
> removing this  integrity hash at least one valid value
> +				if list_match ${value} ${INTEGRITY}
> && [ $(list_length ${INTEGRITY}) -gt 1 ]; then
> +					list_remove INTEGRITY
> ${value}
> +				else
> +					log ERROR "Can not remove
> ${value} from the list of integrity hashes because ${value} is not in
> the list or the list would be empty after removing this integrity
> hash"
> +				fi
> +				;;
> +			+*)
> +				value=${1#+}
> +				# Check if the Ciphers is in the
> list of supported integrity hashes.
> +				if ! isoneof value
> ${VPN_SUPPORTED_INTEGRITY}; then
> +					log ERROR "${value} is not a
> supported integrity hashes and can thats why not added to the list of
> integrity hashes"
> +				else
> +					if list_match ${value}
> ${INTEGRITY}; then
> +						log WARNING
> "${value} is already in the list of integrety hashes of this policy"
> +					else
> +						list_append
> INTEGRITY ${value}
> +					fi
> +				fi
> +				;;
> +		esac
> +		shift
> +	done
> +
> +	vpn_security_policies_write_config_key ${name} "INTEGRITY"
> ${INTEGRITY}
> +}

Likewise.

> +
> +vpn_security_policies_key_exchange() {
> +	# This function parses the parameters for the 'key-exchange' 
> command
> +	local name=${1}
> +	local value=${2}
> +	# Check if we get only one argument after compression <name>
> +	if [ ! $# -eq 2 ] || ! isoneof value "ikev1" "ikev2"; then
> +		log ERROR "You can pass only one parameter after
> key-exchange and this one must be 'ikev1' or 'ikev2'"
> +		return ${EXIT_ERROR}
> +	fi

Should we accept values like "IKEv2", too?

> +
> +	vpn_security_policies_write_config_key "${name}"
> "KEY_EXCHANGE" "${value}"
> +}
> +
> +vpn_security_policies_lifetime(){
> +	# This function parses the parameters for the 'lifetime'
> command
> +	local name=${1}
> +	local value=${2}
> +	# Check if we get only one argument after compression <name>
> +	if [ ! $# -eq 2 ] || ! isinteger value || [ ${value} -le 0
> ]; then
> +		log ERROR "You can pass only one parameter after
> liftime and this one must be an valid integer greater zero"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	vpn_security_policies_write_config_key "${name}" "LIFETIME"
> "${value}"
> +}

This one should accept values like "1h". There is a function for it.

> +
> +vpn_security_policies_pfs(){
> +	# This function parses the parameters for the 'pfs' command
> +	local name=${1}
> +	local value=${2}
> +	# Check if we get only one argument after compression <name>
> +	if [ ! $# -eq 2 ] || ! isbool value; then
> +		log ERROR "You can pass only one parameter after pfs
> and this one must be 'on' or 'off'"
> +		return ${EXIT_ERROR}
> +	fi
> +
> +	vpn_security_policies_write_config_key "${name}" "PFS"
> "${value}"
> +}

See above.

> +
> +vpn_security_policies_check_name() {
> +	# This function checks if a vpn security policy name is
> valid
> +	# Allowed are only A-Za-z0-9
> +	assert [ $# -eq 1 ]
> +	local name=${1}
> +	[[ ${name} =~ [[:alnum:]] ]]

This only checks if there is at least one alphanumerical character in
the string. But it does not exclusively check for them I think.

> +}
> +
> +vpn_security_policies_new() {
> +	# Function that creates based on the paramters one ore more
> new vpn security policies
> +	local name
> +	for name in $@; do
> +		if vpn_security_policy_exists ${name}; then
> +			log ERROR "The vpn security policy ${name}
> does already exist"
> +			continue
> +		fi
> +
> +		if vpn_security_policies_check_name ${name}; then
> +			log ERROR "'${name}' contains illegal
> characters. Allowed are only A-Za-z0-9 are allowed"
> +			continue
> +		fi
> +
> +		log DEBUG "Creating vpn security policy ${name}"
> +		cp "$(vpn_security_policies_path "system")"
> "$(vpn_security_policies_path ${name})"

Don't use cp.

> +	done
> +
> +}

What happens when I create a new policy called "system"?

> +
> +vpn_security_policies_destroy() {
> +	# Function that deletes based on the passed parameters one
> ore more vpn security policies
> +	local name
> +	for name in $@; do
> +		if ! vpn_security_policy_exists ${name}; then
> +			log ERROR "The vpn security policy ${name}
> does not exist"
> +			continue
> +		fi
> +
> +		if vpn_security_policies_check_readonly ${name};
> then
> +			log ERROR "The vpn security policy ${name}
> is readonly and can thats why not deleted"
> +			continue
> +		fi
> +
> +		log DEBUG "Deleting vpn security policy ${name}"
> +		rm -f $(vpn_security_policies_path ${name})
> +	done
> +}

This is overall good work. See the comments above and go through them
one by one and please send an updated version of the patch when ever
you are ready.

Best,
-Michael

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

Re: [RFC 2/2] network: add vpn security policies commands

[Michael Tremer]

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

Hi,

On Thu, 2017-07-13 at 20:33 +0200, Jonatan Schlag wrote:
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/network | 67
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network b/src/network
> index 154d253..e7b637c 100644
> --- a/src/network
> +++ b/src/network
> @@ -1194,7 +1194,6 @@ cli_reset() {
>  	fi
>  
>  	warning_log "Will reset the whole network configuration!!!"
> -
>  	# Force mode is disabled by default
>  	local force=0
>  

This change doesn't have anything to do with the sec pols.

> @@ -1384,13 +1383,77 @@ cli_raw() {
>  	exit ${EXIT_OK}
>  }
>  
> +cli_vpn() {
> +
> +	local action
> +	action=${1}
> +	shift 1

Just no :)

> +
> +	case "${action}" in
> +		security-policies)
> +			cli_vpn_security-policies $@
> +			;;
> +		*)
> +			error "Unrecognized argument: ${action}"
> +			exit ${EXIT_ERROR}
> +			;;
> +	esac
> +}
> +
> +cli_vpn_security-policies() {

Try to avoid a dash in the function name.

> +
> +	local action
> +	local security_policy
> +
> +	if vpn_security_policy_exists ${1}; then
> +
> +		security_policy=${1}
> +		key=${2}
> +		shift 2
> +
> +		case "${key}" in
> +			cipher|compression|integrity|lifetime|pfs|sh
> ow)
> +				vpn_security_policies_${key}
> ${security_policy} $@
> +				;;
> +			group-type)
> +				vpn_security_policies_group_type
> ${security_policy} $@
> +				;;
> +			key-exchange)
> +				vpn_security_policies_key_exchange
> ${security_policy} $@
> +				;;
> +			*)
> +				error "Unrecognized argument:
> ${key}"
> +				exit ${EXIT_ERROR}
> +				;;
> +		esac
> +	else
> +		action=${1}
> +		shift
> +
> +		case "${action}" in
> +			new)
> +				vpn_security_policies_new $@
> +				;;
> +			destroy)
> +				vpn_security_policies_destroy $@
> +				;;
> +			""|*)
> +				if [ -n "${action}" ]; then
> +					error "Unrecognized
> argument: '${action}'"
> +				fi
> +				exit ${EXIT_ERROR}
> +				;;
> +		esac
> +	fi
> +}
> +
>  # Process the given action
>  case "${action}" in
>  	init)
>  		init_run
>  		;;
>  
> -	settings|hostname|port|device|zone|start|stop|restart|status
> |reset|route)
> +	settings|hostname|port|device|zone|start|stop|restart|status
> |reset|route|vpn)
>  		cli_${action} $@
>  		;;
>  

-Michael

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