From d566dccebe38e6380ca596c72d3c6d2ba0a9fa0d Mon Sep 17 00:00:00 2001
From: Evilham <contact@evilham.com>
Date: Fri, 25 Mar 2022 10:56:53 +0100
Subject: [PATCH] [__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_genkey/explorer/key-state     | 32 +++++++
 type/__opendkim_genkey/gencode-remote         | 33 +++++---
 type/__opendkim_genkey/man.rst                | 83 ++++++++++++++-----
 type/__opendkim_genkey/manifest               | 74 ++++++++++++++---
 type/__opendkim_genkey/parameter/optional     |  4 +-
 .../parameter/optional_multiple               |  1 +
 type/__opendkim_genkey/parameter/required     |  2 -
 7 files changed, 183 insertions(+), 46 deletions(-)
 create mode 100755 type/__opendkim_genkey/explorer/key-state
 create mode 100644 type/__opendkim_genkey/parameter/optional_multiple
 delete mode 100644 type/__opendkim_genkey/parameter/required

diff --git a/type/__opendkim_genkey/explorer/key-state b/type/__opendkim_genkey/explorer/key-state
new file mode 100755
index 0000000..75998f9
--- /dev/null
+++ b/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
diff --git a/type/__opendkim_genkey/gencode-remote b/type/__opendkim_genkey/gencode-remote
index d8dfb4d..d2bea50 100755
--- a/type/__opendkim_genkey/gencode-remote
+++ b/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
diff --git a/type/__opendkim_genkey/man.rst b/type/__opendkim_genkey/man.rst
index b3fd013..0d52ca3 100644
--- a/type/__opendkim_genkey/man.rst
+++ b/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' \
-        __opendkim_genkey default \
-                --domain example.com \
-                --selector default
+    # Continue only after the service has been set up
+    export require="__opendkim"
 
-        __opendkim_genkey myfoo \
-                --domain foo.com \
-                --selector backup
+    # Generate a key for 'example.com' with selector 'default'
+    __opendkim_genkey default \
+        --domain example.com \
+        --selector default
+
+    # 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
diff --git a/type/__opendkim_genkey/manifest b/type/__opendkim_genkey/manifest
index 50dcee5..1fee0c1 100755
--- a/type/__opendkim_genkey/manifest
+++ b/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
diff --git a/type/__opendkim_genkey/parameter/optional b/type/__opendkim_genkey/parameter/optional
index e44793f..9d9b6d1 100644
--- a/type/__opendkim_genkey/parameter/optional
+++ b/type/__opendkim_genkey/parameter/optional
@@ -1,4 +1,6 @@
 bits
 directory
+domain
 unrestricted
-sigkey
+selector
+sigdomain
diff --git a/type/__opendkim_genkey/parameter/optional_multiple b/type/__opendkim_genkey/parameter/optional_multiple
new file mode 100644
index 0000000..35978a9
--- /dev/null
+++ b/type/__opendkim_genkey/parameter/optional_multiple
@@ -0,0 +1 @@
+sigkey
diff --git a/type/__opendkim_genkey/parameter/required b/type/__opendkim_genkey/parameter/required
deleted file mode 100644
index 4dacb77..0000000
--- a/type/__opendkim_genkey/parameter/required
+++ /dev/null
@@ -1,2 +0,0 @@
-domain
-selector