From: Michael A Cassaniti Date: Mon, 22 Jun 2026 03:17:52 +0000 (+1000) Subject: acme-common: move nftables rule to hook X-Git-Url: http://git.99rst.org/?a=commitdiff_plain;h=ba08c8cb51102dc5575575fb72073170b6ef1a4b;p=openwrt-packages.git 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 --- diff --git a/net/acme-acmesh/files/hook.sh b/net/acme-acmesh/files/hook.sh index d051d1afb..7466594aa 100644 --- 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 b25384ef6..0419ba8b6 100644 --- 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 853782a60..3baffcfb5 100644 --- 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 bd51d105e..df836793b 100755 --- 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)