Skip to content

httpcaddyfile: Inherit global ACME issuer settings in tls shortcuts#7617

Merged
steadytao merged 1 commit intocaddyserver:masterfrom
steadytao:fix/global-cert-issuer-acme-shortcuts
Apr 26, 2026
Merged

httpcaddyfile: Inherit global ACME issuer settings in tls shortcuts#7617
steadytao merged 1 commit intocaddyserver:masterfrom
steadytao:fix/global-cert-issuer-acme-shortcuts

Conversation

@steadytao
Copy link
Copy Markdown
Member

Fixes implicit ACME issuer construction for tls shortcut directives such as ca, ca_root, and dns when a global cert_issuer acme { ... } is also configured.

Before this change, the tls shortcuts built a fresh site-local ACME issuer and replaced the base/global issuers for that automation policy. That meant ACME-specific settings configured globally through cert_issuer acme such as disable_tlsalpn_challenge were not carried over to the subject-specific issuer actually used for issuance or renewal.

This change makes implicit ACME issuers inherit from any globally-configured ACME issuer templates then applies the local shortcut settings as overrides. This preserves global ACME issuer behavior while still allowing per-site shortcut customization such as overriding the CA directory or trusted roots.

Notably, this fixes the #7612 case where:

  • a global cert_issuer acme { disable_tlsalpn_challenge } was configured
  • the site used tls { ca ... ca_root ... }
  • the adapted config ended up with a subject-specific ACME issuer that no longer had TLS-ALPN disabled

This fix is not specific to TLS-ALPN. It preserves nested ACME challenge settings generically for implicit issuer construction.

Tests added:

  • regression coverage for the exact global cert_issuer + local ca/ca_root composition shape from #7612
  • direct merge coverage to ensure nested ACME challenge settings are preserved rather than overwritten shallowly

Fixes #7612.

Assistance Disclosure

No AI was used.

@francislavoie
Copy link
Copy Markdown
Member

Hmmm. I think this does make sense, but I worry that some users may be relying on this behaviour so it could be a breaking change. Also because we design config as one-way like disable_*, there's no enable_* to re-override it, so keeping certain settings like that sticky through a merge can be hard to manage. It means having to throw away the global if there's a single case where that option is undesirable for it to be applied everywhere. I'm not sure we want to add the enable_* counterparts, I don't think I want to encourage flip-floppy config like that and people setting stuff as enabled when the default is already enabled and so on.

@steadytao
Copy link
Copy Markdown
Member Author

This helped me think about the broader posture here and I think you’re right to be cautious.

I was originally looking at it mainly from the reporter’s expectation that global cert_issuer acme settings would carry into the implicit ACME issuer created by tls { ca ... ca_root ... } but the “sticky disable” problem makes that a much less obvious rule to bake in. Since disable_* options are one-way and there’s no clean per-site way to undo them, inheriting them implicitly could easily turn into a surprising config-wide behavior change.

That also lines up with the reporter’s follow-up concern, even if the PR made the original Caddyfile path behave the way they expected it would not affect already-adapted JSON/autosave so it doesn’t really settle the broader composition semantics.

At this point I’m leaning away from the current broad merge approach. The safer direction is probably to avoid implicitly propagating module-specific disable_* ACME settings through tls shortcut issuers and instead either document that boundary more clearly or warn when users combine global cert_issuer acme with site-local ACME shortcuts in a way that looks like it should compose but currently doesn’t.

@steadytao steadytao marked this pull request as draft April 2, 2026 11:48
@francislavoie
Copy link
Copy Markdown
Member

even if the PR made the original Caddyfile path behave the way they expected it would not affect already-adapted JSON/autosave so it doesn’t really settle the broader composition semantics.

That's not a valid concern, no users adapt to JSON then upgrade Caddy then load JSON config. Users use caddy run -c Caddyfile which adapts their Caddyfile using the current version. It's out of scope to consider old JSON configs adapted from Caddyfile, it's safe to assume users will adapt with the newest version.

@steadytao
Copy link
Copy Markdown
Member Author

That’s a fair point and I think I framed that part too loosely.

I don’t think the already-adapted JSON/autosave angle is really the right compatibility concern here. The more relevant concern is that users running a Caddyfile will have it adapted by the new version so this would silently change the meaning of existing configs that combine global cert_issuer acme with site-local tls shortcuts. Since the settings in question are one-way disable_* flags there also isn’t a clean local way to undo that inheritance if it ends up being undesirable.

Does that sound like the right way to think about the compatibility risk here? If so, I’m wondering whether the better direction is to keep the current semantics and instead either document the boundary more clearly or warn on that combination rather than trying to make the adapter implicitly compose them?

@mholt
Copy link
Copy Markdown
Member

mholt commented Apr 2, 2026

@francislavoie Tbh I do feel like I would expect this to disable the challenge for ALL sites. Usually if it's disabled, it's because there's not a route for TLS to get from the "outside" to the current process or machine. I don't know if I ever see it disabled on a per-site basis.

@steadytao
Copy link
Copy Markdown
Member Author

Was looking through issues and noticed this one has been sitting stale for a small while. Just nudging this in your direction @francislavoie

@francislavoie francislavoie added the bug 🐞 Something isn't working label Apr 25, 2026
@francislavoie francislavoie added this to the v2.11.3 milestone Apr 25, 2026
Copy link
Copy Markdown
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm good with this. Does deserve a noted shout in the release notes that the semantics change a bit for existing configs, but it's better in the long run.

@steadytao steadytao marked this pull request as ready for review April 25, 2026 14:54
@steadytao
Copy link
Copy Markdown
Member Author

Does deserve a noted shout in the release notes that the semantics change a bit for existing configs.

Most definitely agree.

@steadytao
Copy link
Copy Markdown
Member Author

Release note. I have it noted to include in Behaviour changes to call out for 2.11.3

  • Caddyfile tls ACME shortcut issuers now inherit global cert_issuer acme settings before applying local shortcut overrides. This fixes configs that expected global ACME settings such as disable_tlsalpn_challenge to apply to shortcut-created issuers but may change behaviour for existing configs that combine global cert_issuer acme with per-site tls shortcuts.

    If this inheritance is not desired for a site, avoid using the tls ACME shortcut directives for that site and configure the site’s issuer explicitly instead or move the differing ACME settings out of the global issuer and into only the sites that should receive them.

@steadytao steadytao merged commit c1918ff into caddyserver:master Apr 26, 2026
27 checks passed
@steadytao steadytao deleted the fix/global-cert-issuer-acme-shortcuts branch April 26, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐞 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TLS_ALPN challenge doesn't get disabled by Caddyfile directive

3 participants