From 9ba9dceb1a709f47c059d2e21d1e4e5b1b81d00f Mon Sep 17 00:00:00 2001
From: Evilham <cvs@evilham.com>
Date: Sun, 7 Feb 2021 02:29:52 +0100
Subject: [PATCH 1/3] [__letsencrypt_cert] Revamp explorers, add locking.

This would fix #839

Certbot uses locking [1] even for read-only operations and does not properly
use exit codes, which means that sometimes it would print:
"Another instance of Certbot is already running" and exit with success.

However, the previous explorers would take that as the certificate being absent
and would trigger code generation.

The issue was made worse by having many explorers running certbot, so for N
certificates, we'd run certbot N*4 times, potentially "in parallel".

[1]: https://certbot.eff.org/docs/using.html#id5

This patch joins all explorers in one to avoid starting multiple remote python
processes and uses a cdist-specific lock in /tmp/certbot.cdist.lock with a
60 seconds timeout.

It has been tested with certbot 0.31.0 and 0.17 that the:

    from certbot.main import main

trick works. It is somewhat well documented so it can be somewhat relied upon.
---
 .../__letsencrypt_cert/explorer/certbot-path  |  3 -
 .../explorer/certificate-data                 | 78 +++++++++++++++++++
 .../explorer/certificate-domains              |  8 --
 .../explorer/certificate-exists               | 13 ----
 .../explorer/certificate-is-test              | 14 ----
 .../type/__letsencrypt_cert/gencode-remote    | 11 ++-
 cdist/conf/type/__letsencrypt_cert/manifest   |  2 +-
 7 files changed, 87 insertions(+), 42 deletions(-)
 delete mode 100755 cdist/conf/type/__letsencrypt_cert/explorer/certbot-path
 create mode 100755 cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
 delete mode 100755 cdist/conf/type/__letsencrypt_cert/explorer/certificate-domains
 delete mode 100755 cdist/conf/type/__letsencrypt_cert/explorer/certificate-exists
 delete mode 100755 cdist/conf/type/__letsencrypt_cert/explorer/certificate-is-test

diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certbot-path b/cdist/conf/type/__letsencrypt_cert/explorer/certbot-path
deleted file mode 100755
index 3c6076df..00000000
--- a/cdist/conf/type/__letsencrypt_cert/explorer/certbot-path
+++ /dev/null
@@ -1,3 +0,0 @@
-#!/bin/sh -e
-
-command -v certbot 2>/dev/null || true
diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
new file mode 100755
index 00000000..13f0d4d1
--- /dev/null
+++ b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
@@ -0,0 +1,78 @@
+#!/bin/sh -e
+certbot_path="$(command -v certbot 2>/dev/null || true)"
+# Defaults
+certificate_exists="no"
+certificate_is_test="no"
+
+if [ -n "${certbot_path}" ]; then
+	# Find python executable that has access to certbot's module
+	python_path="$(head -n 1 "${certbot_path}" | cut -d '!' -f 2)"
+
+	# Use a lock for cdist due to certbot not exiting with failure
+	# or having any flags for concurrent use.
+	_certbot() {
+		${python_path} - 2>/dev/null <<EOF
+from certbot.main import main
+import fcntl
+lock_file = "/tmp/certbot.cdist.lock"
+timeout=60
+with open(lock_file, 'w') as fd:
+    for i in range(timeout):
+        try:
+            # Get exclusive lock
+            fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+            break
+        except:
+            # Wait if that fails
+            import time
+            time.sleep(1)
+    else:
+        # Timed out, exit with failure
+        import sys
+        sys.exit(1)
+    # Do list certificates
+    main(["certificates", "--cert-name", "${__object_id:?}"])
+EOF
+	}
+
+
+	_certificate_exists() {
+		if grep -q "  Certificate Name: ${__object_id:?}$"; then
+			echo yes
+		else
+			echo no
+		fi
+	}
+
+	_certificate_is_test() {
+		if grep -q 'INVALID: TEST_CERT'; then
+			echo yes
+		else
+			echo no
+		fi
+	}
+
+	_certificate_domains() {
+		grep '    Domains: ' | cut -d ' ' -f 6- | tr ' ' '\n'
+	}
+
+	# Get data about all available certificates
+	certificates="$(_certbot)"
+
+	# Check whether or not the certificate exists
+	certificate_exists="$(echo "${certificates}" | _certificate_exists)"
+
+	# Check whether or not the certificate is for testing
+	certificate_is_test="$(echo "${certificates}" | _certificate_is_test)"
+
+	# Get domains for certificate
+	certificate_domains="$(echo "${certificates}" | _certificate_domains)"
+fi
+
+# Return received data
+cat <<EOF
+certbot_path:${certbot_path}
+certificate_exists:${certificate_exists}
+certificate_is_test:${certificate_is_test}
+${certificate_domains}
+EOF
diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-domains b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-domains
deleted file mode 100755
index db605b63..00000000
--- a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-domains
+++ /dev/null
@@ -1,8 +0,0 @@
-#!/bin/sh -e
-
-certbot_path=$("${__type_explorer}/certbot-path")
-if [ -n "${certbot_path}" ]
-then
-	certbot certificates --cert-name "${__object_id:?}" | grep '    Domains: ' | \
-		cut -d ' ' -f 6- | tr ' ' '\n'
-fi
diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-exists b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-exists
deleted file mode 100755
index 4e6f44db..00000000
--- a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-exists
+++ /dev/null
@@ -1,13 +0,0 @@
-#!/bin/sh -e
-
-certbot_path=$("${__type_explorer}/certbot-path")
-if [ -n "${certbot_path}" ]
-then
-	if certbot certificates | grep -q "  Certificate Name: ${__object_id:?}$"; then
-		echo yes
-	else
-		echo no
-	fi
-else
-	echo no
-fi
diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-is-test b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-is-test
deleted file mode 100755
index 9b445059..00000000
--- a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-is-test
+++ /dev/null
@@ -1,14 +0,0 @@
-#!/bin/sh -e
-
-certbot_path=$("${__type_explorer}/certbot-path")
-if [ -n "${certbot_path}" ]
-then
-	if certbot certificates --cert-name "${__object_id:?}" | \
-		grep -q 'INVALID: TEST_CERT'; then
-		echo yes
-	else
-		echo no
-	fi
-else
-	echo no
-fi
diff --git a/cdist/conf/type/__letsencrypt_cert/gencode-remote b/cdist/conf/type/__letsencrypt_cert/gencode-remote
index 375570a4..801ac033 100755
--- a/cdist/conf/type/__letsencrypt_cert/gencode-remote
+++ b/cdist/conf/type/__letsencrypt_cert/gencode-remote
@@ -1,6 +1,10 @@
 #!/bin/sh -e
 
-certificate_exists=$(cat "${__object:?}/explorer/certificate-exists")
+_explorer_var() {
+	grep "^$1:" "${__object:?}/explorer/certificate-data" | cut -d ':' -f 2-
+}
+
+certificate_exists="$(_explorer_var certificate_exists)"
 name="${__object_id:?}"
 state=$(cat "${__object}/parameter/state")
 
@@ -29,8 +33,9 @@ case "${state}" in
 		fi
 
 		if [ "${certificate_exists}" = "yes" ]; then
-			existing_domains="${__object}/explorer/certificate-domains"
-			certificate_is_test=$(cat "${__object}/explorer/certificate-is-test")
+			existing_domains="${__object}/existing_domains"
+			tail -n +4 "${__object:?}/explorer/certificate-data" | grep -v '^$' > "${existing_domains}"
+			certificate_is_test="$(_explorer_var certificate_is_test)"
 
 			sort -uo "${requested_domains}" "${requested_domains}"
 			sort -uo "${existing_domains}" "${existing_domains}"
diff --git a/cdist/conf/type/__letsencrypt_cert/manifest b/cdist/conf/type/__letsencrypt_cert/manifest
index b4464366..d28e78df 100755
--- a/cdist/conf/type/__letsencrypt_cert/manifest
+++ b/cdist/conf/type/__letsencrypt_cert/manifest
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-certbot_fullpath="$(cat "${__object:?}/explorer/certbot-path")"
+certbot_fullpath="$(grep "^certbot_path:" "${__object:?}/explorer/certificate-data" | cut -d ':' -f 2-)"
 
 if [ -z "${certbot_fullpath}" ]; then
 	os="$(cat "${__global:?}/explorer/os")"

From 01c9f793576811ef1b167a58f79e82aeabb45b78 Mon Sep 17 00:00:00 2001
From: Evilham <cvs@evilham.com>
Date: Sun, 7 Feb 2021 20:19:50 +0100
Subject: [PATCH 2/3] [__letsencrypt_cert] Simplify derivation of python path.

This also supports odd setups with '!' in the path.
---
 cdist/conf/type/__letsencrypt_cert/explorer/certificate-data | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
index 13f0d4d1..ff62e742 100755
--- a/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
+++ b/cdist/conf/type/__letsencrypt_cert/explorer/certificate-data
@@ -6,7 +6,7 @@ certificate_is_test="no"
 
 if [ -n "${certbot_path}" ]; then
 	# Find python executable that has access to certbot's module
-	python_path="$(head -n 1 "${certbot_path}" | cut -d '!' -f 2)"
+	python_path=$(sed -n '1s/^#! *//p' "${certbot_path}")
 
 	# Use a lock for cdist due to certbot not exiting with failure
 	# or having any flags for concurrent use.

From 60ed6f35fdf5111461c62e0df59e638ff4c5cf68 Mon Sep 17 00:00:00 2001
From: Evilham <cvs@evilham.com>
Date: Tue, 23 Feb 2021 13:41:00 +0100
Subject: [PATCH 3/3] [__letsencrypt_cert] Be more consistent with temp files

Noticed this while doing previous work on the type.
---
 cdist/conf/type/__letsencrypt_cert/gencode-remote | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cdist/conf/type/__letsencrypt_cert/gencode-remote b/cdist/conf/type/__letsencrypt_cert/gencode-remote
index 801ac033..60a77ec3 100755
--- a/cdist/conf/type/__letsencrypt_cert/gencode-remote
+++ b/cdist/conf/type/__letsencrypt_cert/gencode-remote
@@ -33,7 +33,7 @@ case "${state}" in
 		fi
 
 		if [ "${certificate_exists}" = "yes" ]; then
-			existing_domains="${__object}/existing_domains"
+			existing_domains=$(mktemp "${TMPDIR:-/tmp}/existing_domains.cdist.XXXXXXXXXX")
 			tail -n +4 "${__object:?}/explorer/certificate-data" | grep -v '^$' > "${existing_domains}"
 			certificate_is_test="$(_explorer_var certificate_is_test)"