acme-common: move nftables rule to hook

The nftables rule in acme-common partially works, but it races against the
acme.sh and uacme client. While the client is performing the renew the rule
is being deleted because the client is run in the background. This change
moves the rule management to the hook instead. While duplicate rules could
be created, the benefits outway the potential costs. It is unknown how many
installations issue/renew multiple certificates.

Signed-off-by: Michael A Cassaniti <redacted>

diff --git a/net/acme-acmesh/files/hook.sh b/net/acme-acmesh/files/hook.sh
index d051d1afb64841c0999e194e44bd288464e33cf2..7466594aad4513c58636440ba440d124ecd8283b 100644 (file)
--- a/net/acme-acmesh/files/hook.sh
+++ b/net/acme-acmesh/files/hook.sh
@@ -56,19 +56,19 @@ handle_signal() {
        if [[ "$cgroup" == '/services/acme/*' ]]; then
                # send SIGTERM to all processes in this process's cgroup. this
                # relies on procd's having set up the cgroup for the instance.
-               read -r -d '' pids < /sys/fs/cgroup${cgroup}/cgroup.procs 
+               read -r -d '' pids < /sys/fs/cgroup${cgroup}/cgroup.procs
                kill -TERM $pids 2> /dev/null
        fi
 
-       # if we're here, either the cgroup wasn't as exected to be set up by
-       # procd or killing the cgroup PIDs failed. try to kill the process
-       # group, assuming this process is the group leader. this is actually
+       # If we're here, either the cgroup wasn't as expected to be set up by
+       # procd or killing the cgroup PIDs failed. Try to kill the process
+       # group, assuming this process is the group leader. This is actually
        # unlikely since procd doesn't set service PGIDs (so they aren't group
        # leaders).
        kill -TERM -$$ 2> /dev/null
 
-       # if we're here, cgroup-based killing was avoided or didn't work and
-       # PGID-based killing didn't work. fall back to the raciest option.
+       # If we're here, cgroup-based killing was avoided or didn't work and
+       # PGID-based killing didn't work. Fall back to the raciest option.
        trap "" TERM
        term_descendants() {
                local pids=$@
@@ -83,6 +83,7 @@ handle_signal() {
        }
        term_descendants $(jobs -p)
 
+       del_nft_rule
        wait_notify
 }
 
@@ -115,10 +116,12 @@ get)
                        set -- "$@" --renew --home "$state_dir" -d "$main_domain"
                        log info "$ACME $*"
                        trap "handle_signal renew Renewal" INT TERM
+                       add_nft_rule "$main_domain" "$listen_port"
                        $ACME "$@" &
                        wait $!
                        status=$?
                        trap - INT TERM
+                       del_nft_rule
 
                        case $status in
                        0)
@@ -175,13 +178,16 @@ get)
                fi
                ;;
        "standalone")
+               add_nft_rule "$main_domain" "$listen_port"
                set -- "$@" --standalone --listen-v6 --httpport "$listen_port"
                ;;
        "alpn")
+               add_nft_rule "$main_domain" "$listen_port"
                set -- "$@" --alpn --listen-v6 --tlsport "$listen_port"
                ;;
        "webroot")
                mkdir -p "$CHALLENGE_DIR"
+               add_nft_rule "$main_domain" "$listen_port"
                set -- "$@" --webroot "$CHALLENGE_DIR"
                ;;
        *)
@@ -199,6 +205,7 @@ get)
        wait $!
        status=$?
        trap - INT TERM
+       del_nft_rule
 
        case $status in
        0)

diff --git a/net/acme-common/files/acme.init b/net/acme-common/files/acme.init
index b25384ef6d3be88dffb085aa829609793e4f2bfc..0419ba8b6ce39b93bb10ea36418f93c743bfb0c7 100644 (file)
--- a/net/acme-common/files/acme.init
+++ b/net/acme-common/files/acme.init
@@ -5,8 +5,6 @@ USE_PROCD=1
 run_dir=/var/run/acme
 export CHALLENGE_DIR=$run_dir/challenge
 export CERT_DIR=/etc/ssl/acme
-LAST_LISTEN_PORT=
-NFT_HANDLE=
 HOOK=/usr/lib/acme/hook
 LOG_TAG=acme
 
@@ -16,19 +14,6 @@ LOG_TAG=acme
 extra_command "abort" "Abort running certificate issuances/renewals"
 extra_command "renew" "Run certificate issuances/renewals"
 
-delete_nft_rule() {
-       if [ "$NFT_HANDLE" ]; then
-               # $NFT_HANDLE contains the string 'handle XX' so pass it unquoted to nft
-               nft delete rule inet fw4 input $NFT_HANDLE
-               NFT_HANDLE=
-       fi
-}
-
-cleanup() {
-       log debug "cleaning up"
-       delete_nft_rule
-}
-
 load_options() {
        section=$1
 
@@ -115,18 +100,6 @@ get_cert() {
                config_get listen_port "$section" listen_port
                ;;
        esac
-       if [ "$listen_port" != "$LAST_LISTEN_PORT" ]; then
-               delete_nft_rule
-
-               if [ "$listen_port" ]; then
-                       if ! NFT_HANDLE=$(nft -a -e insert rule inet fw4 input tcp dport "$listen_port" counter accept comment ACME | grep -o 'handle [0-9]\+'); then
-                               return 1
-                       fi
-                       log debug "added nft rule: $NFT_HANDLE"
-               fi
-
-               LAST_LISTEN_PORT="$listen_port"
-       fi
 
        procd_open_instance "$section"
        procd_set_param command "$HOOK" get
@@ -201,8 +174,6 @@ service_triggers() {
 }
 
 load_and_run() {
-       trap cleanup EXIT
-
        config_load acme
        config_foreach load_globals acme
 

diff --git a/net/acme-common/files/functions.sh b/net/acme-common/files/functions.sh
index 853782a60f28ed235d28cad4d6b9f4e6ab1173d2..3baffcfb500a21ef3b0f3e444947c836d2b244d8 100644 (file)
--- a/net/acme-common/files/functions.sh
+++ b/net/acme-common/files/functions.sh
@@ -5,3 +5,43 @@ log() {
                logger -t "$LOG_TAG" -p "daemon.$prio" -- "$@"
        fi
 }
+
+NFT_HANDLE=
+
+add_nft_rule() {
+       local main_domain
+       local listen_port
+       main_domain="$1"
+       listen_port="$2"
+
+       [ -n "$listen_port" ] || return
+       case "$listen_port" in
+               [0-9]*)
+                       ;;
+               *)
+                       log err "Invalid listen port $listen_port for $main_domain"
+                       return 1
+                       ;;
+       esac
+
+       if ! NFT_HANDLE=$(nft -a -e insert rule inet fw4 input tcp dport "$listen_port" counter accept comment \"ACME $main_domain\" | grep -o 'handle [0-9]\+'); then
+               log err "Failed to add nftables rule for port $listen_port"
+               return 1
+       else
+               log debug "Added nftables rule for port $listen_port with $NFT_HANDLE"
+               echo "$NFT_HANDLE"
+       fi
+}
+
+del_nft_rule() {
+       if [ "$NFT_HANDLE" ]; then
+               # $NFT_HANDLE contains the string 'handle XX' so pass it unquoted to nft
+               # shellcheck disable=SC2086
+               if ! nft delete rule inet fw4 input $NFT_HANDLE ; then
+                       log err "Failed to delete nftables rule $NFT_HANDLE"
+               else
+                       log debug "Deleted nftables rule with $NFT_HANDLE"
+               fi
+               NFT_HANDLE=""
+       fi
+}

diff --git a/net/uacme/files/hook.sh b/net/uacme/files/hook.sh
index bd51d105e7dc85cb46d8132113df969887bf1dc0..df836793b5a4af86f551659a9146af36fa193ecf 100755 (executable)
--- a/net/uacme/files/hook.sh
+++ b/net/uacme/files/hook.sh
@@ -89,7 +89,7 @@ get)
        ec*)
                keylength=${key_type#ec}
                domain_dir="$state_dir/$main_domain"
-               set -- "$@" -t EC 
+               set -- "$@" -t EC
                ;;
        rsa*)
                keylength=${key_type#rsa}
@@ -137,6 +137,7 @@ get)
        case "$validation_method" in
        "alpn")
                log info "using already running ualpn, it's user's duty to config ualpn server deamon"
+               add_nft_rule "$main_domain" "$listen_port"
                set -- "$@" -h "$HOOKDIR/client/ualpn.sh"
                ;;
        "dns")
@@ -161,10 +162,13 @@ get)
                set -- "$@" --standalone --listen-v6
                log err "standalone server is not implmented for uacme"
                exit 1
+               # In case this is implemented in the future, we need to add nft rule before uacme starts to listen
+               #add_nft_rule "$main_domain" "$listen_port"
                ;;
        "webroot")
                mkdir -p "$CHALLENGE_DIR"
                export CHALLENGE_DIR
+               add_nft_rule "$main_domain" "$listen_port"
                set -- "$@" -h "$HOOKDIR/client/httpchalhook.sh"
                ;;
        *)
@@ -178,10 +182,11 @@ get)
        done
 
        log info "$ACME $*"
-       trap '$NOTIFY issue-failed;exit 1' INT
+       trap 'del_nft_rule; $NOTIFY issue-failed; exit 1' INT TERM
        "$ACME" "$@" -k 2>&1
        status=$?
-       trap - INT
+       trap - INT TERM
+       del_nft_rule
 
        case $status in
        0)