acme-common: Move nftables rule to hook by mcassaniti · Pull Request #29802 · openwrt/packages

mcassaniti · 2026-06-22T03:29:12Z

📦 Package Details

Maintainer: @tohojo

Description:

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.

I've done some basic testing against my production firewalls and confirmed this is working. Feel free to make changes as required.

🧪 Run Testing Details

OpenWrt Version: 25.12.4
OpenWrt Target/Subtarget: arm64
OpenWrt Device: Raspberry Pi 4

✅ Formalities

I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

I'll need some help with these ones

It can be applied using git am
It has been refreshed to avoid offsets, fuzzes, etc., using
```
make package/<your-package>/refresh V=s
```
It is structured in a way that it is potentially upstreamable
_{(e.g., subject line, commit description, etc.)}
_{We must try to upstream patches to reduce maintenance burden.}

orangepizza · 2026-06-22T03:36:24Z

@tohojo is actual maintainer of acme-acmesh

Fixes #29444

mcassaniti · 2026-06-22T03:40:12Z

Whoops, forgot to add the fixes tag. Thank you.

orangepizza · 2026-06-22T03:58:56Z

Could you add same func at uacme's hook script too?

mcassaniti · 2026-06-22T04:43:24Z

Could you add same func at uacme's hook script too?

Once this has a review and everyone is happy I will make the changes for uacme as well. This can be a condition of getting this merged. I'd prefer to get it right in one spot first.

mcassaniti · 2026-06-22T23:26:58Z

I've looked at uacme's hook script and it says that the standalone server is not implemented. Unless I'm missing something I won't be making any modifications to uacme.

orangepizza · 2026-06-22T23:31:43Z

Webroot/tls-alpn still need to open its port to public (I doubt most people open luci to internet 24/7)

orangepizza · 2026-06-23T00:49:02Z

do you have reason to webroot challenge to not open port? you can't expect uhttpd(which run luci too) to be open to public by default:

mcassaniti · 2026-06-23T01:44:20Z

do you have reason to webroot challenge to not open port? you can't expect uhttpd(which run luci too) to be open to public by default:

I'm a bit lost. Was this for uacme or acmesh?

orangepizza · 2026-06-23T01:49:09Z

Both; you don't set rule add when you select web root challenge solver

orangepizza · 2026-06-23T02:37:46Z

tested it with uacme with webroot challenge solver, it works as intended.

orangepizza · 2026-06-23T03:32:00Z

I forgot to mention but you now have to bump PKG_RELEASE(it's in packages makefile) at every package you touched : acme-common is only on here, so for it you need last number of PKG_VERSION

mcassaniti · 2026-06-23T12:18:14Z

Whoops. Was there anything else I need to fix?

tohojo · 2026-06-23T12:44:30Z

I suspect the bot is going to complain that you don't have a signoff in your commit message. Also, the message starts with "The nftables rule in acme-common works" - does it really, though? If it's being removed while the check is still ongoing it does not, in fact, work, does it? :)

tohojo

Code looks good to me; seems the bot has one more nit, please fix that and we should be good to go :)

mcassaniti · 2026-06-27T05:55:34Z

Nudging this one

stangri · 2026-06-27T20:48:59Z

Unsolicited comment: @mcassaniti @tohojo this nft rule manipulation feels like a crutch when OpenWrt already supports adding firewall objects to procd instances.

Would it not be more elegant to create a patch that overwrites _startserver/_stopserver (and tls functions too) to start procd socat instance + firewall object and tear them down? Just a food for thought for the future. Should be an easy work for an agent.

orangepizza · 2026-06-28T04:57:42Z

Unsolicited comment: @mcassaniti @tohojo this nft rule manipulation feels like a crutch when OpenWrt already supports adding firewall objects to procd instances.

Would it not be more elegant to create a patch that overwrites _startserver/_stopserver (and tls functions too) to start procd socat instance + firewall object and tear them down? Just a food for thought for the future. Should be an easy work for an agent.

IIRC old one before this PR did that, but _stopserver called too early and removed firewall rule before remote ACME server actually check challenge file

mcassaniti · 2026-07-01T00:29:51Z

LGTM?

orangepizza · 2026-07-01T02:31:15Z

@BKPepe could you take another look at this?

openwrt · 2026-07-01T06:23:26Z

Formality Check: Failed

We completed the verification flow. Please review the formatting overview logs below.

🛑 CRITICAL ERRORS

Commit a7e6963 - acme-common: move nftables rule to hook:

Makefile introduces PKG_VERSION '1.5.3', but this version string is missing in the commit subject line

⚠️ STYLISTIC WARNINGS & SUGGESTIONS

Package Release Audit:

⚠️ Package net/acme-acmesh content changed without a PKG_RELEASE or version bump

⚠️ Package net/acme-common version updated from '1.5.2' to '1.5.3', but PKG_RELEASE was not reset to 1 (currently: 'not defined')

_{Something broken? Consider reporting an issue.}
_{Running version f1c5a8a deployed on 2026-07-01 23:34:44 CEST}

BKPepe · 2026-07-01T06:24:57Z

You should wait for maintainer.

Copilot

Pull request overview

This PR aims to fix a race in acme-common firewall (nftables) rule management by moving rule insertion/deletion from the acme init script into the per-client hook scripts, so the rule lifetime more closely matches the actual ACME client runtime.

Changes:

Removed nftables rule lifecycle management from net/acme-common/files/acme.init.
Added shared add_nft_rule / del_nft_rule helpers in net/acme-common/files/functions.sh and invoked them from the acme-acmesh and uacme hooks.
Bumped package versions/releases to ship the behavior change.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
net/uacme/Makefile	Bumps `PKG_RELEASE` to ship hook behavior changes.
net/uacme/files/hook.sh	Moves nft rule management into the uacme hook (add before run, delete after).
net/acme-common/Makefile	Bumps `PKG_VERSION` for the acme-common behavior change.
net/acme-common/files/functions.sh	Introduces shared nft rule helper functions.
net/acme-common/files/acme.init	Removes prior init-script-managed nft rule handling to avoid races.
net/acme-acmesh/Makefile	Bumps `PKG_RELEASE` to ship hook behavior changes.
net/acme-acmesh/files/hook.sh	Moves nft rule management into the acme.sh hook (renew + issuance paths).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mcassaniti · 2026-07-01T08:33:22Z

I'll need some help with this one

Makefile introduces PKG_VERSION '1.5.3', but this version string is missing in the commit subject line

tohojo

A few nits. I think you can ignore the PKG_VERSION comment in this case (it wants you to mention the version bump in the commit header, but in this case it's not an upstream release, so it's kinda incidental)

BKPepe · 2026-07-01T19:08:40Z

A few nits. I think you can ignore the PKG_VERSION comment in this case (it wants you to mention the version bump in the commit header, but in this case it's not an upstream release, so it's kinda incidental)

Honestly, this is a question of how we want to handle it. Some might consider it a false positive, but frankly, I believe that unless proven otherwise, the number of packages using this non-standard approach is so small that I'm unsure what the best way to catch it would be.

Looking at mwan3, @feckert manages to address it there. Of course, one could argue it's a workaround or a bypass of the check, but he implemented it this way long before the OpenWrt GitHub bot existed.

In my view, the workflow in the external repository is that changes accumulate in the master branch, and then a decision is made to cut a new release along with release notes. I think we could introduce this practice here too, just to keep things transparent.

We can definitely open this up for discussion.

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. Version bump to 1.5.3 of acme-common was required. Signed-off-by: Michael A Cassaniti <michael@cassaniti.id.au>

tohojo

One small nit. Also, let's split the package version and revision bumps into its own commit (and mention the version in the commit subject) to appease the formalities bot :)

tohojo · 2026-07-02T09:39:11Z

+		# shellcheck disable=SC2086
+		if ! nft delete rule inet fw4 input $NFT_HANDLE ; then
+			log err "Failed to delete nftables rule $NFT_HANDLE"
+		fi


We should reset NFT_HANDLE to the empty string here to make sure the function is idempotent. I don't think we'll ever call it twice as-is now, but it's still good practice to guard against it.

tohojo requested changes Jun 22, 2026

View reviewed changes

Comment thread net/acme-acmesh/files/hook.sh Outdated

mcassaniti force-pushed the acme-common-nft-fix branch from 2de7fcf to f60b179 Compare June 22, 2026 23:19

mcassaniti force-pushed the acme-common-nft-fix branch from f60b179 to d9c38cb Compare June 23, 2026 00:25

mcassaniti changed the title ~~acme-acmesh: Move nftables rule to hook~~ acme-common: Move nftables rule to hook Jun 23, 2026

mcassaniti force-pushed the acme-common-nft-fix branch from d9c38cb to a23c8f4 Compare June 23, 2026 01:51

mcassaniti force-pushed the acme-common-nft-fix branch from a23c8f4 to af1fa9b Compare June 23, 2026 03:51

tohojo reviewed Jun 23, 2026

View reviewed changes

Comment thread net/acme-acmesh/Makefile Outdated

mcassaniti force-pushed the acme-common-nft-fix branch from af1fa9b to c8b15d8 Compare June 24, 2026 02:07

mcassaniti requested a review from tohojo June 24, 2026 04:47

tohojo requested changes Jun 24, 2026

View reviewed changes

mcassaniti force-pushed the acme-common-nft-fix branch from c8b15d8 to a5ab162 Compare June 24, 2026 11:43

mcassaniti requested a review from tohojo June 24, 2026 13:16

BKPepe requested a review from Copilot July 1, 2026 06:23

BKPepe force-pushed the acme-common-nft-fix branch from a5ab162 to 0f9a9e1 Compare July 1, 2026 06:23

openwrt Bot added the not following guidelines label Jul 1, 2026

Copilot started reviewing on behalf of BKPepe July 1, 2026 06:34 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread net/acme-common/files/functions.sh

Comment thread net/acme-acmesh/files/hook.sh

Comment thread net/uacme/files/hook.sh Outdated

Comment thread net/uacme/files/hook.sh Outdated

mcassaniti force-pushed the acme-common-nft-fix branch from a3e716b to 4092748 Compare July 1, 2026 08:31

tohojo requested changes Jul 1, 2026

View reviewed changes

Comment thread net/acme-acmesh/files/hook.sh Outdated

Comment thread net/uacme/files/hook.sh Outdated

mcassaniti force-pushed the acme-common-nft-fix branch from 4092748 to a7e6963 Compare July 1, 2026 23:14

mcassaniti requested a review from tohojo July 2, 2026 01:50

tohojo requested changes Jul 2, 2026

View reviewed changes

Uh oh!

Conversation

mcassaniti commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Package Details

🧪 Run Testing Details

✅ Formalities

If your PR contains a patch:

Uh oh!

orangepizza commented Jun 22, 2026

Uh oh!

mcassaniti commented Jun 22, 2026

Uh oh!

orangepizza commented Jun 22, 2026

Uh oh!

mcassaniti commented Jun 22, 2026

Uh oh!

Uh oh!

mcassaniti commented Jun 22, 2026

Uh oh!

orangepizza commented Jun 22, 2026

Uh oh!

orangepizza commented Jun 23, 2026

Uh oh!

mcassaniti commented Jun 23, 2026

Uh oh!

orangepizza commented Jun 23, 2026

Uh oh!

orangepizza commented Jun 23, 2026

Uh oh!

orangepizza commented Jun 23, 2026

Uh oh!

Uh oh!

mcassaniti commented Jun 23, 2026

Uh oh!

tohojo commented Jun 23, 2026

Uh oh!

tohojo left a comment

Choose a reason for hiding this comment

Uh oh!

mcassaniti commented Jun 27, 2026

Uh oh!

stangri commented Jun 27, 2026

Uh oh!

orangepizza commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcassaniti commented Jul 1, 2026

Uh oh!

orangepizza commented Jul 1, 2026

Uh oh!

openwrt Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Formality Check: Failed

🛑 CRITICAL ERRORS

⚠️ STYLISTIC WARNINGS & SUGGESTIONS

Uh oh!

BKPepe commented Jul 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcassaniti commented Jul 1, 2026

Uh oh!

tohojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BKPepe commented Jul 1, 2026

Uh oh!

tohojo left a comment

Choose a reason for hiding this comment

Uh oh!

mcassaniti commented Jun 22, 2026 •

edited

Loading

orangepizza commented Jun 28, 2026 •

edited

Loading

openwrt Bot commented Jul 1, 2026 •

edited

Loading