URGENT: fix dns_nsupdate when NSUPDATE_OPT is empty

[fraenki]

PR #5224 introduced a new parameter $NSUPDATE_OPT to dns_nsupdate. However, now this DNSAPI is broken, if the new parameter is empty/unset:

[Mon Sep 16 15:48:33 CEST 2024] Adding TXT value: XXX for domain: _acme-challenge.example.com
[Mon Sep 16 15:48:33 CEST 2024] adding _acme-challenge.example.com. 60 in txt "XXX"
could not open '': file not found
[Mon Sep 16 15:48:33 CEST 2024] error updating domain
[Mon Sep 16 15:48:33 CEST 2024] Error adding TXT record to domain: _acme-challenge.example.com
[Mon Sep 16 15:48:33 CEST 2024] Please check log file for more details: /var/log/acme.sh/acme.log

This is a major issue, because it will affect almost ALL users of dns_nsupdate.

The root cause is easily demonstrated by the following code snippet:

# does not work
$ NSUPDATE_OPT="" nsupdate -k /tmp/foo -d "${NSUPDATE_OPT}"
could not open '': file not found

# works as expected
$ NSUPDATE_OPT="" nsupdate -k /tmp/foo -d $NSUPDATE_OPT
Creating key...
...

Note that nsupdate interprets an empty string as a file/input. So when passing an empty parameter to nsupdate, it must not use double quotes.

[github-actions]

Welcome
First thing: don't send PR to the master branch, please send to the dev branch instead.
Please make sure you've read our DNS API Dev Guide and DNS-API-Test.
Then reply on this message, otherwise, your code will not be reviewed or merged.
We look forward to reviewing your Pull request shortly ✨
注意: 必须通过了 DNS-API-Test 才会被 review. 无论是修改, 还是新加的 dns api, 都必须确保通过这个测试.

[fraenki]

Yes, the shellcheck test is complaining...

In dnsapi/dns_nsupdate.sh line 42:
    nsupdate -k "${NSUPDATE_KEY}" $nsdebug $NSUPDATE_OPT <<EOF
                                           ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

But this is desired, it's necessary to fix the bug (and the old $nsdebug parameter fails this test too).

It would be possible to pass the shellcheck test by adding more code. I'll do this if this will be requested by the maintainers.

[fraenki]

@Neilpang Please review. If you accept this bugfix, please also issue a new minor release (otherwise the broken release will get rolled out everywhere). Thank you!

[gmanic]

@fraenki - your fix works fine.

My own full checks worked out fine without double quotes.
Unfortunately, I just added the double quotes due to the shellcheck failure. My bad, sorry for that.

Thanks for your fix.

[Neilpang]

fix the shellcheck errors

[gmanic]

@fraenki
How about creating a variable with the full set of command line parameters once per function [dns_nsupdate_add, dns_nsupdate_rm] before if [ -z "${NSUPDATE_ZONE}" ]; then like this:

nsupdate_parameters="-k ${NSUPDATE_KEY} ${nsdebug} ${NSUPDATE_OPT}"

and using this command instead:

nsupdate "${nsupdate_parameters}" <<EOF

Should fix the shellcheck error - except the last error regarding usage of $? instead of command...

[fraenki]

How about creating a variable with the full set of command line parameters once per function

I've tried this, but unfortunately it does not work: nsupdate requires double quotes around the filename when using the -k option, otherwise it will report an error:

could not read key from  "/etc/acme.sh/configs/profile_nsupdate_example/hook.cnf"  .{private,key}: file not found

Currently testing another fix, but I have to wait an hour because I've hit the Let's Encrypt rate limit. 🤦‍♂️

[gmanic]

I also tried some alternatives. This one seems to run, taking the correct key file and without shellcheck errors:

  nsupdate_cmd="nsupdate -k ${NSUPDATE_KEY} ${nsdebug} ${NSUPDATE_OPT}"
  if [ -z "${NSUPDATE_ZONE}" ]; then
    ${nsupdate_cmd} <<EOF

Unfortunately, I hit auth limits on my nameserver to fully test deployment. 🤦‍♂️

For the error check at the end (another shellcheck error), I assigned
RC=$?
after the (add and rm) run of nsupdate (once per function, after the full if-block if [ -z "${NSUPDATE_ZONE}" ]; then) and extended the error check
if [ "${RC}" -ne 0 ]; then

No shellcheck errors on shellcheck.net 👍

[gmanic]

In my forked repo, shellcheck-test works, but DNS Docker always complains some error I just don't understand...
My Changes to Master

[fraenki]

This one seems to run, taking the correct key file and without shellcheck errors:

Unfortunately, it results in an error, because nsupdate expects the key file to be wrapped in double quotes:

could not read key from /tmp/keyfile.{private,key}: file not found

With nsupdate the rule seems to be: files need to be wrapped in double quotes, all other options must not use double quotes.

[gmanic]

Maybe the filename without .key extension is the cause of nsupdate trying to add {.private,.key}.
My local run (extended with the line "full nsupdate..." - local temporary hack to see the actual commandline) runs without error (domain and txt-record redacted).

[Tue Sep 17 12:26:26 PM CEST 2024] adding _acme-challenge.example.com. 60 in txt "LrgB5T1AMk4ah_6psolKcf10kavqtnkfxxxxxxx"
[Tue Sep 17 12:26:26 PM CEST 2024] full nsupdate -k /root/.acme.sh/_acme-challenge.example.com.key -D -v
[Tue Sep 17 12:26:26 PM CEST 2024] Return code of nsupdate: 0

Edit: maybe this is "good enough" for a push and minor-release to cover at least most dns_nsupdate installations and fix the filename-without-key|private in a second step?

[fraenki]

fix the shellcheck errors

@Neilpang Fixed. Please review/merge.

[fraenki]

@Neilpang Would it be possible to get a new release of acme.sh that includes this fix? Thanks!

[Neilpang]

@fraenki here you go:
https://github.com/acmesh-official/acme.sh/releases/tag/3.0.9