Skip to content

security/acme-client: fix false "validation failed" status on skipped renewal - #5719

Open
daemonhorn wants to merge 1 commit into
opnsense:masterfrom
daemonhorn:fix/acme-client-false-validation-failed-on-skip
Open

daemonhorn wants to merge 1 commit into
opnsense:masterfrom
daemonhorn:fix/acme-client-false-validation-failed-on-skip

Conversation

@daemonhorn

Copy link
Copy Markdown

Important notices

If AI was used, please disclose:

  • Model used: Claude Code (Claude Sonnet 5, Anthropic)
  • Extent of AI involvement: Root cause analysis (tracing the bug through LeUtils::run_shell_command(), LeValidation\Base::run(), LeCertificate::issue(), and the certificates.volt status formatter, and confirming acme.sh's RENEW_SKIP=2 exit code in the upstream acme.sh source), the code patch itself, and this PR description were prepared with AI assistance. All changes were reviewed and verified against the actual acme.sh and opnsense/plugins source, PHP-linted, packaged into a test .pkg, and validated on real OPNsense hardware over several days before this PR was opened.

Describe the problem

On Services > ACME Client > Certificates, the "Last ACME Status" column
shows "validation failed" even when nothing actually failed — the
certificate simply wasn't due for renewal yet and acme.sh correctly
declined to touch it:

[...] Skipping. Next renewal time is: 2026-09-14T13:16:19Z
[...] Add '--force' to force renewal.

This misleads admins into thinking their ACME setup is broken when it's
working exactly as intended.

Root cause: acme.sh's renew() function returns its own $RENEW_SKIP
constant (exit code 2) when it decides a cert isn't due yet. OPNsense's
LeValidation\Base::run() treats any non-zero exit code as a failure:

if ($result) {
    LeUtils::log_error('domain validation failed (' . $this->getMethod() . ')');
    return false;
}

LeCertificate::issue() then calls setStatus(400), and the
certificates.volt status formatter renders statusCode == 400 as the
literal string "validation failed".

Note that LeCertificate::issue() already has its own needsRenewal()
guard (based on the certificate's validFrom_time_t + configured
renewInterval) meant to avoid calling acme.sh at all when renewal isn't
due. In practice it can still diverge from acme.sh's own internal
scheduling — acme.sh's randomized-renewal jitter, and/or a CA-supplied
ACME Renewal Information (ARI) window — which is how the skip path gets
reached at all. That divergence is acme.sh's own logic working as
designed; this PR does not attempt to change it, only to stop
mis-reporting a legitimate skip as a failure.


Describe the proposed solution

Minimal, targeted fix that mirrors the existing needsRenewal() no-op
pattern already in the codebase (log + return, without calling
setStatus(), so the certificate's last real status is left untouched
instead of being overwritten):

  • LeCommon.php: add ACME_RENEW_SKIP = 2, documenting acme.sh's
    RENEW_SKIP exit-code contract.
  • LeValidation\Base::run(): special-case that exit code — log a notice
    instead of an error, set a new public $skipped flag, and return
    false (unchanged failure handling for every other non-zero code).
  • LeCertificate::issue(): when validation reports $skipped, log and
    return without calling setStatus(), just like the existing
    needsRenewal() early-return a few lines above.

Deliberately out of scope: no new status code, no changes to the
certificates.volt formatter or the setStatus() status-code table, and
no changes to remove()/revoke()'s own exit-code handling (different
semantics, no legitimate "skip" outcome there).

Verified: php -l on all changed files; traced the fix end-to-end against
the actual acme.sh source (renew() returning $RENEW_SKIP on the
single-domain --renew path OPNsense uses); confirmed no LeValidation
subclass overrides run(), so every challenge type is covered; built a
test package (real published os-acme-client patched with only these
files) and validated it on real OPNsense hardware over several days,
observing all four relevant status transitions: ok->skip (renewal
attempted before it was due), ok->renew (on-time renewal), ok->validation failed (a forced, genuine failure, confirming real failures still report
correctly), and failed->ok (recovery once the forced failure was
cleared).


Related issue

Fixes #4908 — reported independently by three users with matching logs,
auto-closed by the stale-bot as not_planned despite the reporter's
unanswered request to reopen it. One of the reporters
(stephanjahn-xapio) also tied the same symptom to acme.sh's newer ARI
extension; since both triggers produce the same acme.sh exit code, this
fix covers both without special-casing either.


🤖 Generated with Claude Code

https://claude.ai/code/session_01MVmeD5j4rEfTLjzWAQmbiF

… renewal

acme.sh's renew() returns exit code 2 (RENEW_SKIP) when it decides on
its own that a certificate is not due for renewal yet (e.g. the
configured renewal interval or a CA-provided ACME Renewal Information
window has not been reached). LeValidation\Base::run() treated any
non-zero exit code as a failure, so this legitimate skip was logged as
"domain validation failed" and LeCertificate::issue() set statusCode
400, which the Certificates GUI renders as "validation failed" - even
though nothing actually failed and the existing certificate is still
valid.

Add LeCommon::ACME_RENEW_SKIP and special-case it in
LeValidation\Base::run(): reset the flag at the top of run() (so a
reused validation object can't carry a stale skip into a later real
failure), log a notice instead of an error on a skip, and expose the
outcome via a new public $skipped property. LeCertificate::issue()
now treats a skip like its existing needsRenewal() no-op path - log
and return, without calling setStatus() - so the certificate's last
real status is left untouched instead of being overwritten.

Fixes opnsense#4908.

Validated on real OPNsense hardware over several days, observing all
four status transitions: ok->skip (renewal attempted before it was
due), ok->renew (on-time renewal), ok->validation failed (a forced,
genuine failure, to confirm real failures still report correctly),
and failed->ok (recovery once the forced failure was cleared).

AI tools disclosure: root cause analysis, patch, and this commit
message were prepared with assistance from Claude Code (Claude
Sonnet 5, Anthropic), reviewed, tested, and verified by the submitter.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MVmeD5j4rEfTLjzWAQmbiF
@fraenki fraenki self-assigned this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

security/os-acme-client: Reducing the renewal interval results in auto renewal dns01 validation failed errors

2 participants