Fix: Ensure mod_md `tls-alpn-01` challenge is usable by JonasVerhofste · Pull Request #2643 · puppetlabs/puppetlabs-apache

JonasVerhofste · 2026-08-18T16:59:28Z

The challenge is already configurable in apache::mod::md, but it needs the acme-tls/1 protocol to properly work (see "https: Challenges"): https://httpd.apache.org/docs/2.4/mod/mod_md.html

smortex

LGTM

JonasVerhofste · 2026-08-18T21:28:56Z

That looks like a CI bug, not a failed test. 😅

jst-cyr · 2026-08-19T19:41:01Z

@JonasVerhofste : You are right, the acceptance tests don't run correctly on forks. Initial spec tests look good, I'll run some acceptance tests so we can get platform-specific feedback.

jst-cyr · 2026-08-19T19:50:00Z

Running CI tests for this PR here: #2644

jst-cyr · 2026-08-20T15:59:49Z

Acceptance test results here: https://github.com/puppetlabs/puppetlabs-apache/actions/runs/32295187776?pr=2644

All failures in PR acceptance testing match existing failures in the Nightly build against main:

SLES-12 fails during provisioning (SSH connection refused before the host comes up) — same on nightly.
Debian-13 fails in apache parameters default_confd_files => false doesnt do anything — apache2 fails to start via systemd (mktemp error in the init script) — same on nightly.
SLES-15 fails in apache::custom_config define valid config adds the config — apache2ctl not found — same on nightly.

These are pre-existing issues on main, not caused by this change.

Next step will require an engineering review when the team is available, but tests look good!

jst-cyr · 2026-09-28T17:11:41Z

Rebasing and re-running tests against latest

jst-cyr · 2026-09-28T17:13:10Z

CI test run created: #2652

JonasVerhofste · 2026-09-28T17:26:38Z

Rebasing and re-running tests against latest

@jst-cyr That will break my gpg signing, so hit me up if you want me to rebase again. :)

jst-cyr · 2026-09-28T18:35:25Z

@JonasVerhofste : My apologies for that! I'll ping you the next time before doing that. We do have some new test results though...

Acceptance Test Results: https://github.com/puppetlabs/puppetlabs-apache/actions/runs/36456571257?pr=2652

Mostly green, but some failures on Debian 11 and SLES 12.

Debian 11: failures are expected and match with the failures in the nightly run against the main branch. Nothing to fix here.
SLES 12: This one looks new. It fails a check that makes sure applying the Puppet manifest twice in a row produces no changes the second time (idempotency). The httpd.conf file changed content on the second run, in the SSL vhost test. Since this PR touches mod_md/TLS-ALPN config, it's worth double-checking that change doesn't cause httpd.conf to get regenerated differently between runs on SLES.

JonasVerhofste · 2026-09-28T22:34:06Z

@jst-cyr Going through the codepath I don't see how the SLES12 failure can be caused by this PR for httpd.conf specifically: in the test apache::protocols is not changed from its default [] and nothing turns on acme-tls/1 elsewhere afaict.

Would it be an option to somehow turn on diff output for the litmus tests (show_diff in the puppet.conf), so that we can actually see what changes? The current output tells us that something changed, but not what.

I mostly think this is a flaky test and nothing to do with the change, but with the diff output we could confirm for sure.

jst-cyr · 2026-09-29T14:21:57Z

@JonasVerhofste : It does seem like SLES 12 is quite flaky. Been trying to re-run the tests and it keeps failing provisioning. I think you are right that there is something flaky with the SLES 12 infrastructure. Leave this one with me and I will try to rerun that particular test once provisioning is working again and see if we get the same result.

jst-cyr · 2026-09-30T14:44:03Z

@JonasVerhofste : As you suspected, not a logical problem, but flaky infrastructure. It took a lot of runs (12, specifically) but I eventually got SLES 12 to green. There must be something in the way the infra is provisioned that can cause acceptance tests to fail, but it does look like we are good to go now!

JonasVerhofste · 2026-09-30T14:56:10Z

@jst-cyr I already rebased and force pushed, so from my end: merge at will! :)

jst-cyr · 2026-10-01T16:21:25Z

@JonasVerhofste : It still needs an engineering review, but I've put this on their backlog list so the team can review when they have a moment!

span786

Thanks @JonasVerhofste. Allowing acme-tls/1 in protocols is the right fix for tls-alpn-01, which apache::mod::md already accepts in md_ca_challenges. The init.pp side looks good.

The vhost.pp side has a placement issue (inline comment). Could you also add rspec-puppet cases for both classes? For example: protocols => ['acme-tls/1'] compiles and contains Class['apache::mod::md'], with and without h2. Those would have caught it.

span786 · 2026-10-27T05:38:54Z

      'h2_upgrade'             => $h2_upgrade,
      'h2_window_size'         => $h2_window_size,
    }
+    if 'acme-tls/1' in $protocols {


This include is nested inside the if ('h2' in $protocols or 'h2c' in $protocols or $h2_copy_files != undef or ...) and $ensure == 'present' block. With only acme-tls/1 (e.g. protocols => ['acme-tls/1', 'http/1.1'], the typical non-HTTP/2 setup) that block is skipped, so apache::mod::md is never included. That's exactly the case this PR is meant to fix. It only works today if the vhost also enables h2/h2c or sets an h2_* parameter.

Could it move out of the HTTP/2 block and merge with the existing mdomain check just below? Something like:

if ($mdomain or 'acme-tls/1' in $protocols) and $ensure == 'present' { include apache::mod::md }

That also keeps the $ensure == 'present' guard.

span786

I'm marking my earlier review as changes requested so this doesn't merge as-is.

In manifests/vhost.pp, include apache::mod::md is nested inside the HTTP/2 block. A vhost with only acme-tls/1 (e.g. protocols => ['acme-tls/1', 'http/1.1']) therefore never loads mod_md, which is the case this PR is meant to fix. Please move it out of that block, for example:

  if ($mdomain or 'acme-tls/1' in $protocols) and $ensure == 'present' {
    include apache::mod::md
  }

Please also add rspec-puppet coverage for acme-tls/1 with and without h2, in both apache and apache::vhost. I'll approve once those are in.

The challenge is already configurable in apache::mod::md, but it needs the `acme-tls/1` protocol to properly work (see "https: Challenges"): https://httpd.apache.org/docs/2.4/mod/mod_md.html Signed-off-by: Jonas Verhofsté <jonas@verhofste.tech>

span786 · 2026-10-29T16:08:22Z

@JonasVerhofste, thanks for the push. I don't think it picked up the requested changes, though. The head is back at 91555ab0, which is the original commit from before the rebase, so:

include apache::mod::md in manifests/vhost.pp is still inside the HTTP/2 block. A vhost with protocols => ['acme-tls/1', 'http/1.1'] still never loads mod_md (details).
There are no rspec-puppet cases yet for acme-tls/1 with and without h2, in apache and apache::vhost.
The branch is 7 commits behind main again, so please rebase as well.

Happy to approve once those are in.

JonasVerhofste · 2026-10-29T19:42:19Z

@span786 If you look at the comittor, you'll notice it was you who pushed, not me. :P

Haven't had the time to look at it further, it'll probably be for this weekend.

JonasVerhofste requested review from a team, bastelfreak, ekohl and smortex as code owners August 18, 2026 16:59

smortex added the bugfix label Aug 18, 2026

smortex approved these changes Aug 18, 2026

View reviewed changes

jst-cyr mentioned this pull request Aug 19, 2026

Fix: Ensure mod_md tls-alpn-01 challenge is usable #2644

Closed

jst-cyr force-pushed the fix/acme-tls branch from 7ee051e to d59f879 Compare September 28, 2026 17:11

jst-cyr mentioned this pull request Sep 28, 2026

Fix: Ensure mod_md tls-alpn-01 challenge is usable #2652

Closed

JonasVerhofste force-pushed the fix/acme-tls branch from d59f879 to 36734cf Compare September 28, 2026 22:15

span786 reviewed Oct 27, 2026

View reviewed changes

span786 requested changes Oct 27, 2026

View reviewed changes

span786 self-assigned this Oct 27, 2026

span786 force-pushed the fix/acme-tls branch from 36734cf to 085881e Compare October 27, 2026 05:43

span786 force-pushed the fix/acme-tls branch from 085881e to 91555ab Compare October 27, 2026 15:33

Conversation

JonasVerhofste commented Aug 18, 2026

Uh oh!

smortex left a comment

Choose a reason for hiding this comment

Uh oh!

JonasVerhofste commented Aug 18, 2026

Uh oh!

jst-cyr commented Aug 19, 2026

Uh oh!

jst-cyr commented Aug 19, 2026

Uh oh!

jst-cyr commented Aug 20, 2026

Uh oh!

jst-cyr commented Sep 28, 2026

Uh oh!

jst-cyr commented Sep 28, 2026

Uh oh!

JonasVerhofste commented Sep 28, 2026

Uh oh!

jst-cyr commented Sep 28, 2026

Uh oh!

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

Uh oh!

Uh oh!

jst-cyr commented Sep 29, 2026

Uh oh!

jst-cyr commented Sep 30, 2026

Uh oh!

JonasVerhofste commented Sep 30, 2026

Uh oh!

jst-cyr commented Oct 1, 2026

Uh oh!

span786 left a comment

Choose a reason for hiding this comment

Uh oh!

span786 Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

span786 left a comment

Choose a reason for hiding this comment

Uh oh!

span786 commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonasVerhofste commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JonasVerhofste commented Sep 28, 2026 •

edited

Loading

span786 commented Oct 29, 2026 •

edited

Loading