[__opendkim_key] Overall improvements in key management

While developing this, I noticed that the type was handling inconsistently the
expectation that a cdist object with the same __object_id gets *modified*.
Instead more and more lines were added to, e.g. SigningTable and KeyTable.

In order to solve this, some backwards compatibility breaking is necessary.

This is probably not too terrible since:

- the `--selector` parameter was mandatory, therefore the fallback for the key
location is triggered.
- OpenDKIM uses the first match in `SigningTable` and `KeyTable`
- __line and __block respectively append if they do not match

Closes #19 and #20.

type/__opendkim/files/opendkim.conf.sh

@@ -34,8 +34,8 @@ then
 fi
 
 # Key and Domain tables
-echo "KeyTable ${CFG_DIR}/KeyTable"
-echo "SigningTable ${CFG_DIR}/SigningTable"
+echo "KeyTable ${CFG_DIR:?}/KeyTable"
+echo "SigningTable ${CFG_DIR:?}/SigningTable"
 
 # Required socket to listen on
 printf "Socket %s\n" "${SOCKET:?}"

type/__opendkim/man.rst

@@ -41,21 +41,25 @@ subdomains
 umask
   Set the umask for the socket and PID file.
 
-userid
-  Change the user the opendkim program is to run as.
-  By default, Alpine Linux's OpenRC service will set this to `opendkim` on the
-  command-line and FreeBSD's rc will set it to `mailnull`.
-
 custom-config
   The string following this parameter is appended as-is in the configuration, to
   enable more complex configurations.
 
+
 BOOLEAN PARAMETERS
 ------------------
 syslog
   Log to syslog.
 
 
+DEPRECATED PARAMETERS
+---------------------
+userid
+  Change the user the opendkim program is to run as.
+  By default, Alpine Linux's OpenRC service will set this to `opendkim` on the
+  command-line and FreeBSD's rc will set it to `mailnull`.
+
+
 EXAMPLES
 --------
 

type/__opendkim/manifest

@@ -22,13 +22,17 @@ os=$(cat "${__global:?}/explorer/os")
 
 CFG_DIR="/etc/opendkim"
 service="opendkim"
+start_service="opendkim"
 case "$os" in
 'alpine')
 	:
 	;;
 'freebsd')
 	CFG_DIR="/usr/local/etc/mail"
+	# This is for 'service X start'
 	service="milter-opendkim"
+	# This is for __start_on_boot (it's inconsistent)
+	start_service="milteropendkim"
 	;;
 *)
 	printf "__opendkim does not yet support %s.\n" "$os" >&2
@@ -75,7 +79,7 @@ fi
 
 # Generate and deploy configuration file.
 source_file="${__object:?}/files/opendkim.conf"
-target_file="${CFG_DIR}/opendkim.conf"
+target_file="${CFG_DIR:?}/opendkim.conf"
 
 mkdir -p "${__object:?}/files"
 
@@ -90,11 +94,11 @@ fi
 require="__package/opendkim" __file "$target_file" \
 	--source "$source_file" --mode 0644
 
-require="__package/opendkim" __start_on_boot "${service}"
+require="__package/opendkim" __start_on_boot "${start_service:?}"
 
 # Ensure Key and Signing tables exist and have proper permissions
-key_table="${CFG_DIR}/KeyTable"
-signing_table="${CFG_DIR}/SigningTable"
+key_table="${CFG_DIR:?}/KeyTable"
+signing_table="${CFG_DIR:?}/SigningTable"
 
 require="__package/opendkim" \
 	__file "${key_table}" \
@@ -105,7 +109,7 @@ require="__package/opendkim" \
 	--mode 444
 
 require="__file${target_file} __file${key_table}
-		__file${signing_table} __start_on_boot/${service}" \
+		__file${signing_table} __start_on_boot/${start_service}" \
 	__check_messages opendkim \
 		--pattern "^__file${target_file}" \
 		--execute "service ${service} restart"

type/__opendkim/parameter/deprecated/userid

@@ -0,0 +1,2 @@
+This can cause inconsistencies with permissions and will stop being supported.
+If you still need this, you can use --custom-config 'UserId $USERID'.

type/__opendkim_genkey/explorer/key-state

@@ -0,0 +1,32 @@
+#!/bin/sh -e
+DIRECTORY="/var/db/dkim/"
+if [ -f "${__object:?}/parameter/directory" ];
+then
+	# Be forgiving about a lack of trailing slash
+	DIRECTORY="$(sed -E 's!([^/])$!\1/!' < "${__object:?}/parameter/directory")"
+fi
+
+
+KEY_ID="$(echo "${__object_id:?)}" | tr '/' '_')"
+DEFAULT_PATH="${DIRECTORY:?}${KEY_ID:?}.private"
+if [ -s "${DEFAULT_PATH}" ]; then
+	# This is the main location for the key
+	FOUND_PATH="${DEFAULT_PATH}"
+else
+	# This is a backwards-compatible location for the key
+	# Keys generated post March 2022 should not land here
+	if [ -f "${__object:?}/parameter/selector" ]; then
+		SELECTOR="$(cat "${__object:?}/parameter/selector")"
+		if [ -s "${DIRECTORY}${SELECTOR:?}.private" ]; then
+			FOUND_PATH="${DIRECTORY}${SELECTOR:?}.private"
+		fi
+	fi
+fi
+
+if [ -n "${FOUND_PATH}" ]; then
+	printf "present\t%s" "${FOUND_PATH}"
+else
+	# We didn't find the key
+	# We pass the default path here, to easen logic in the rest of the type
+	printf "absent\t%s" "${DEFAULT_PATH}"
+fi

type/__opendkim_genkey/gencode-remote

@@ -19,8 +19,8 @@
 #
 
 # Required parameters
-DOMAIN="$(cat "${__object:?}/parameter/domain")"
-SELECTOR="$(cat "${__object:?}/parameter/selector")"
+DOMAIN="$(cat "${__object:?}/domain")"
+SELECTOR="$(cat "${__object:?}/selector")"
 
 # Optional parameters
 BITS=
@@ -28,12 +28,6 @@ if [ -f "${__object:?}/parameter/bits" ]; then
 	BITS="-b $(cat "${__object:?}/parameter/bits")"
 fi
 
-DIRECTORY="/var/db/dkim/"
-if [ -f "${__object:?}/parameter/directory" ]; then
-	# Be forgiving about a lack of trailing slash
-	DIRECTORY="$(sed -E 's!([^/])$!\1/!' < "${__object:?}/parameter/directory")"
-fi
-
 # Boolean parameters
 SUBDOMAINS=
 if [ -f "${__object:?}/parameter/no-subdomains" ]; then
@@ -48,9 +42,24 @@ fi
 user="$(cat "${__object:?}/user")"
 group="$(cat "${__object:?}/group")"
 
-if ! [ -f "${DIRECTORY}${SELECTOR}.private" ]; then
-	echo "opendkim-genkey $BITS --domain=$DOMAIN --directory=$DIRECTORY $RESTRICTED --selector=$SELECTOR $SUBDOMAINS"
-	echo "chown ${user}:${group} ${DIRECTORY}${SELECTOR}.private"
+KEY_STATE="$(cut -f 1 "${__object:?}/explorer/key-state")"
+KEY_LOCATION="$(cut -f 2- "${__object:?}/explorer/key-state")"
+
+if [ "${KEY_STATE:?}" = "absent" ]; then
+	# opendkim-genkey(8) does not allow specifying the file name.
+	# To err on the safe side (and avoid potentially killing other keys)
+	# we operate on a temporary directory first, then move the resulting key
+	cat <<-EOF
+	tmp_dir="\$(mktemp -d cdist-dkim.XXXXXXXXXXX)"
+	opendkim-genkey $BITS --domain=${DOMAIN:?} --directory=\${tmp_dir:?} $RESTRICTED --selector=${SELECTOR:?} $SUBDOMAINS
+	# Relocate and ensure permissions
+	mv "\${tmp_dir:?}/${SELECTOR:?}.private" '${KEY_LOCATION:?}'
+	chown ${user}:${group} '${KEY_LOCATION}'
+	chmod 0600 '${KEY_LOCATION}'
 	# This is usually generated, if it weren't we do not want to fail
-	echo "chown ${user}:${group} ${DIRECTORY}${SELECTOR}.txt || true"
+	mv "\${tmp_dir:?}/${SELECTOR:?}.txt" '${KEY_LOCATION%.private}.txt' || true
+	chown ${user}:${group} '${KEY_LOCATION%.private}.txt' || true
+	# Cleanup after ourselves
+	rmdir "\${tmp_dir:?}" || true
+	EOF
 fi

type/__opendkim_genkey/man.rst

@@ -10,23 +10,27 @@ DESCRIPTION
 -----------
 
 This type uses the `opendkim-genkey(8)` to generate signing keys suitable for
-usage by `opendkim(8)` to sign outgoing emails. Then, a line with the domain,
-selector and keyname in the `$selector._domainkey.$domain` format will be added
-to the OpenDKIM key table located at `/etc/opendkim/KeyTable`. Finally, a line
-will be added to the OpenDKIM signing table, using either the domain or the
-provided key for the `domain:selector:keyfile` value in the table. An existing
-key will not be overwritten.
+usage by `opendkim(8)` to sign outgoing emails.
+
+It also manages the key, identified by its `$__object_id` in OpenDKIM's
+KeyTable and sets its `s=` and `d=` parameters (see: `--selector` and
+`--sigdomain` respectively).
+
+This type will also manage the entries in the OpenDKIM's SigningTable by
+associating any given `sigkey` values to this key.
+
+Take into account that if you use this type without the `--domain` and
+`--selector` parameters, the `$__object_id` must be in form `$domain/$selector`.
 
 Currently, this type is only implemented for Alpine Linux and FreeBSD.
 Please contribute an implementation if you can.
 
-REQUIRED PARAMETERS
--------------------
-domain
-  The domain to generate the key for.
-
-selector
-  The DKIM selector to generate the key for.
+NOTE: the name of the key file under `--directory` will default to
+`$__object_id.private`, but if that fails and `--selector` is used,
+`SELECTOR.private` will be considered.
+Take care when using unrelated keys that might collide this way.
+For more information see:
+https://code.ungleich.ch/ungleich-public/cdist-contrib/issues/20
 
 
 OPTIONAL PARAMETERS
@@ -38,10 +42,36 @@ bits
 directory
   The directory in which to generate the key, `/var/db/dkim/` by default.
 
+domain
+  The domain to generate the key for.
+  If omitted, `--selector` must be omitted as well and `$__object_id` must be
+  in form: `$domain/$selector`.
+
+selector
+  The DKIM selector to generate the key for.
+  If omitted, `--domain` must be omitted as well and `$__object_id` must be
+  in form: `$domain/$selector`.
+
+sigdomain
+  Specified in the KeyTable, the domain to use in the signature's "d=" value.
+  Defaults to the specified domain. If `%`, it will be replaced by the apparent
+  domain of the sender when generating a signature.
+  Note you probably don't want to set both `--sigdomain` and `--sigkey` to `%`.
+  See `KeyTable` in `opendkim.conf(5)` for more information.
+
+
+OPTIONAL MULTIPLE PARAMETERS
+----------------------------
 sigkey
-  The key used in the SigningTable for this signing key. Defaults to the
+  The key used in the `SigningTable` for this signing key. Defaults to the
   specified domain. If `%`, OpenDKIM will replace it with the domain found
   in the `From:` header. See `opendkim.conf(5)` for more options.
+  Note you probably don't want to set both `--sigdomain` and `--sigkey` to `%`.
+  This can be passed multiple times, resulting in multiple lines in the
+  SigningTable, which can be used to support signing of subdomains or multiple
+  domains with the same key; in that case, you probably want to set
+  `--sigdomain` to `%`, else the domains will not be aligned.
+
 
 BOOLEAN PARAMETERS
 ------------------
@@ -57,6 +87,7 @@ EXAMPLES
 
 .. code-block:: sh
 
+    # Setup the OpenDKIM service
     __opendkim \
         --socket inet:8891@localhost \
         --basedir /var/lib/opendkim \
@@ -65,14 +96,24 @@ EXAMPLES
         --umask 002 \
         --syslog
 
-    require='__opendkim' \
+    # Continue only after the service has been set up
+    export require="__opendkim"
+
+    # Generate a key for 'example.com' with selector 'default'
     __opendkim_genkey default \
         --domain example.com \
         --selector default
 
-        __opendkim_genkey myfoo \
-                --domain foo.com \
-                --selector backup
+    # Generate a key for 'foo.com' with selector 'backup'
+    __opendkim_genkey 'foo.com/backup'
+
+    # Generate a key for 'example.org' with selector 'main'
+    # that can also sign 'cdi.st' and subdomains of 'example.org'
+    __opendkim_genkey 'example.org/main' \
+        --sigdomain '%' \
+        --sigkey 'example.org' \
+        --sigkey '.example.org' \
+        --sigkey 'cdi.st'
 
 
 SEE ALSO

type/__opendkim_genkey/manifest

@@ -38,14 +38,45 @@ case "$os" in
 	__opendkim_genkey currently only supports Alpine Linux. Please
 	contribute an implementation for $os if you can.
 	EOF
+	exit 1
 ;;
 esac
-# Persist user and group for gencode-remote
-printf '%s' "${user}" > "${__object:?}/user"
-printf '%s' "${group}" > "${__object:?}/group"
 
-SELECTOR="$(cat "${__object:?}/parameter/selector")"
-DOMAIN="$(cat "${__object:?}/parameter/domain")"
+# Logic to simplify the type as documented in
+# https://code.ungleich.ch/ungleich-public/cdist-contrib/issues/20#issuecomment-14711
+DOMAIN="$(cat "${__object:?}/parameter/domain" 2>/dev/null || true)"
+SELECTOR="$(cat "${__object:?}/parameter/selector" 2>/dev/null || true)"
+if [ -z "${DOMAIN}${SELECTOR}" ]; then
+	# Neither SELECTOR nor DOMAIN were passed, try to use __object_id
+	if echo "${__object_id:?}" | \
+			grep -qE '^[^/[:space:]]+/[^/[:space:]]+$'; then
+		# __object_id matches, let's get the data
+		DOMAIN="$(echo "${__object_id:?}" | cut -d '/' -f 1)"
+		SELECTOR="$(echo "${__object_id:?}" | cut -d '/' -f 2)"
+	else
+		# It doesn't match the pattern, this is sad
+		cat <<- EOF >&2
+		The arguments --domain and --selector were not used.
+		So __object_id must match DOMAIN/SELECTOR.
+		But instead the type got: ${__object_id:?}
+		EOF
+		exit 1
+	fi
+elif [ -z "${DOMAIN}" ] || [ -z "${SELECTOR}" ]; then
+	# Only one was passed, this is sad :-(
+	cat <<- EOF >&2
+	You must pass either both --selector and --domain or none of them.
+	If these arguments are absent, __object_id must match: DOMAIN/SELECTOR.
+	EOF
+	exit 1
+# else: both were passed
+fi
+
+# Persist data for gencode-remote
+printf '%s' "${user:?}" > "${__object:?}/user"
+printf '%s' "${group:?}" > "${__object:?}/group"
+printf '%s' "${DOMAIN:?}" > "${__object:?}/domain"
+printf '%s' "${SELECTOR:?}" > "${__object:?}/selector"
 
 DIRECTORY="/var/db/dkim/"
 if [ -f "${__object:?}/parameter/directory" ];
@@ -59,6 +90,11 @@ if [ -f "${__object:?}/parameter/sigkey" ];
 then
 	SIGKEY="$(cat "${__object:?}/parameter/sigkey")"
 fi
+SIGDOMAIN="${DOMAIN:?}"
+if [ -f "${__object:?}/parameter/sigdomain" ];
+then
+	SIGDOMAIN="$(cat "${__object:?}/parameter/sigdomain")"
+fi
 
 # Ensure the key-container directory exists with the proper permissions
 __directory "${DIRECTORY}" \
@@ -76,10 +112,28 @@ esac
 key_table="${CFG_DIR}/KeyTable"
 signing_table="${CFG_DIR}/SigningTable"
 
-__line "line-key-${__object_id:?}" \
-	--file "${key_table}" \
-	--line "${SELECTOR:?}._domainkey.${DOMAIN:?} ${DOMAIN:?}:${SELECTOR:?}:${DIRECTORY:?}${SELECTOR:?}.private"
+KEY_STATE="$(cut -f 1 "${__object:?}/explorer/key-state")"
+KEY_LOCATION="$(cut -f 2- "${__object:?}/explorer/key-state")"
 
-__line "line-sig-${__object_id:?}" \
+__line "__opendkim_genkey/${__object_id:?}" \
+	--file "${key_table}" \
+	--line "${__object_id:?} ${SIGDOMAIN:?}:${SELECTOR:?}:${KEY_LOCATION:?}" \
+	--regex "^${__object_id:?}[[:space:]]" \
+	--state 'replace'
+
+sigtable_block() {
+	for sigkey in ${SIGKEY:?}; do
+		echo "${sigkey:?} ${__object_id:?}"
+	done
+}
+__block "__opendkim_genkey/${__object_id:?}" \
 	--file "${signing_table}" \
-	--line "${SIGKEY:?} ${SELECTOR:?}._domainkey.${DOMAIN:?}"
+	--text "$(sigtable_block)"
+
+if [ "${KEY_STATE:?}" = "present" ]; then
+	# Ensure proper permissions for the key file
+	__file "${KEY_LOCATION}" \
+		--owner "${user}" \
+		--group "${group}" \
+		--mode 0600
+fi

type/__opendkim_genkey/parameter/optional

@@ -1,4 +1,6 @@
 bits
 directory
+domain
 unrestricted
-sigkey
+selector
+sigdomain

type/__opendkim_genkey/parameter/optional_multiple

@@ -0,0 +1 @@
+sigkey

type/__opendkim_genkey/parameter/required

@@ -1,2 +0,0 @@
-domain
-selector