Skip to content

caddytls: Fix client auth (fix #7724) - #7727

Merged
mholt merged 1 commit into
masterfrom
fix-7724
May 14, 2026
Merged

caddytls: Fix client auth (fix #7724)#7727
mholt merged 1 commit into
masterfrom
fix-7724

Conversation

@mholt

@mholt mholt commented May 14, 2026

Copy link
Copy Markdown
Member

The peer certificates should be loaded even if existingVerifyPeerCert is nil.

Patched with the assistance of Copilot, as an experiment.

Do you think we need to return an error in the default case (i.e. neither branch gets entered)?

@mholt
caddytls: Fix client auth (fix #7724)
63e3ae4 
The peer certificates should be loaded even if existingVerifyPeerCert is nil.

Patched with the assistance of Copilot, as an experiment.
Copilot AI reviewed May 14, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes caddytls client-auth verification so client certificate verifiers receive the peer certificate raw DER bytes even when no pre-existing VerifyPeerCertificate callback is configured (addressing #7724).

Changes:

  • Build rawCerts from tls.ConnectionState.PeerCertificates unconditionally in verifyConnection.
  • Pass rawCerts through to ClientCertificateVerifier implementations instead of nil.
  • Add unit tests covering raw cert propagation and verifier error returns.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
modules/caddytls/connpolicy.go Ensures verifyConnection always prepares and forwards peer rawCerts to verifiers (and existing verification callback when present).
modules/caddytls/connpolicy_verifyconnection_test.go Adds tests to confirm raw cert forwarding and error propagation from verifiers.

ðŸ’Ą Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread modules/caddytls/connpolicy.go
@mholt mholt mentioned this pull request May 14, 2026
Closed
steadytao
steadytao approved these changes May 14, 2026
View reviewed changes

@steadytao steadytao left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved. I verified this with a direct red/green regression test against the LeafCertClientAuth path; current master fails with no client certificate provided while this PR passes the same test because it restores passing raw peer cert bytes from tls.ConnectionState.PeerCertificates into the configured ClientCertificateVerifier.

issue was behaviour drift introduced by 1fbb287

@steadytao steadytao added the bug 🐞 Something isn't working label May 14, 2026
@steadytao steadytao added this to the v2.11.4 milestone May 14, 2026
@steadytao

steadytao commented May 14, 2026

Copy link
Copy Markdown
Member

Do you think we need to return an error in the default case (i.e. neither branch gets entered)?

If there is no existing callback and no configured Caddy verifier, there is no extra verification work to do so returning nil would be the right behaviour

@steadytao steadytao linked an issue May 14, 2026 that may be closed by this pull request
Client TLS not working on v2.11.3 #7724
Closed
@steadytao steadytao removed this from the v2.11.4 milestone May 14, 2026
fuomag9 added a commit to ingres-si/ingressi that referenced this pull request May 14, 2026
@fuomag9 @claude
fix: pin Caddy to v2.11.2 to avoid v2.11.3 client TLS regression
f53c4d4 
xcaddy defaults to building from master, so yesterday's rebuild pulled
v2.11.3 (released 2026-05-12), which broke mTLS client authentication —
the server requests the cert and then rejects it with a "bad certificate"
TLS alert, surfacing as ERR_BAD_SSL_CLIENT_AUTH_CERT in browsers and
causing the functional mTLS test setup to fail.

Pin to v2.11.2 (last good release) until the upstream fix lands.

Refs: caddyserver/caddy#7724, caddyserver/caddy#7727

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mholt
mholt merged commit 6c675e2 into master May 14, 2026
40 checks passed
@mholt
mholt deleted the fix-7724 branch May 14, 2026 16:05
@mholt

mholt commented May 14, 2026

Copy link
Copy Markdown
Member Author

Thanks @steadytao !

This was referenced Jun 2, 2026
Closed
Merged
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
HarmonybrewBot HarmonybrewBot
!10336 merge bump-caddy-2.11.4 into main
4bfdca5 
caddy 2.11.4

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.

- [ ] `resource` blocks have been checked for updates.
<details>
  <summary>release notes</summary>
  <pre>This release patches more security, security-adjacent, and normal bugs. The FrankenPHP project has collaborated on PHP-adjacent patches, which we are grateful for.

The recent surge of patches is mostly attributed to token predictors. We have had to reject more than 75% of "security" reports because they were AI slop spam (or just lazy/incorrect). Please use LLMs and agents wisely to avoid wasting precious maintainer resources. We have started blocking offending accounts that spam slop reports. Thank you to all who submit responsible reports following our security policy to make the project better. We appreciate that the community deems the Caddy project worthy of contribution to improve the broader ecosystem!

Security-related patches:
- caddyhttp: Normalize Windows backslashes in path matcher (thanks @Vincent550102)
- rewrite: Prevent placeholder re-expansion in injected query (thanks @WhiskerEnt)
- templates: Improved `stripHTML` action to more reliably remove malformed HTML (thanks to @jmrcsnchz)
- caddyhttp: Ignore header fields with underscores to prevent collisions (thanks @Vincent550102 for the report and @dunglas for the patch)

There are also several other various fixes and enhancements by many other contributors. Thank you everyone who participated!

## What's Changed
* reverseproxy: further prevent body closes from dial errors by @jameshartig in caddyserver/caddy#7715
* caddytls: Fix client auth (fix #7724) by @mholt in caddyserver/caddy#7727
* chore: deps upgrade by @mohammed90 in caddyserver/caddy#7751
* caddyhttp: omit Last-Modified for unusable mod times by @bb4242 in caddyserver/caddy#7740
* caddytls: fix TLS state races and ECH rotation retry by @broady in caddyserver/caddy#7756
* chore: clean up wording and typo fixes by @steadytao in caddyserver/caddy#7745
* reverseproxy: Add regression test for DialInfo network override by @eyupcanakman in caddyserver/caddy#7758
* caddyauth: add candidate placeholders for rejected identities by @steadytao in caddyserver/caddy#7698
* cmd: support caddy start on IPv6-only hosts by @steadytao in caddyserver/caddy#7744
* caddyfile: preserve implicit TLS issuer semantics by @steadytao in caddyserver/caddy#7743
* reverseproxy: wraps request body to prevent closing if not read by @WeidiDeng in caddyserver/caddy#7719
* caddytls: match IDN SNI in connection policies by @steadytao in caddyserver/caddy#7742
* build(deps): bump the all-updates group across 1 directory with 9 updates by @dependabot[bot] in caddyserver/caddy#7752
* caddyhttp: normalize Windows backslashes in path matcher by @Vincent550102 in caddyserver/caddy#7763
* go.mod: update x/net by @steadytao in caddyserver/caddy#7767
* rewrite: prevent placeholder re-expansion in injected query by @WhiskerEnt in caddyserver/caddy#7761
* perf(replacer): optimize memory allocation for file placeholders by @Jualhosting in caddyserver/caddy#7773
* caddytls: skip idna.ToASCII for pure ASCII SNI values by @sleet0922 in caddyserver/caddy#7770
* encode: prioritize zstd and br over gzip in content negotiation by @Jualhosting in caddyserver/caddy#7772
* httpcaddyfile: fix incorrect error message on duplicate matchers by @Brunotlps in caddyserver/caddy#7780
* Patch for GHSA-vcc4-2c75-vc9v by @jmrcsnchz in caddyserver/caddy#7785

## New Contributors
* @jameshartig made their first contribution in caddyserver/caddy#7715
* @bb4242 made their first contribution in caddyserver/caddy#7740
* @broady made their first contribution in caddyserver/caddy#7756
* @eyupcanakman made their first contribution in caddyserver/caddy#7758
* @Vincent550102 made their first contribution in caddyserver/caddy#7763
* @WhiskerEnt made their first contribution in caddyserver/caddy#7761
* @Jualhosting made their first contribution in caddyserver/caddy#7773
* @sleet0922 made their first contribution in caddyserver/caddy#7770
* @Brunotlps made their first contribution in caddyserver/caddy#7780
* @jmrcsnchz made their first contribution in caddyserver/caddy#7785

**Full Changelog**: https://github.com/caddyserver/caddy/compare/v2.11.3...v2.11.4</pre>
  <p>View the full release notes at <a href="https://github.com/caddyserver/caddy/releases/tag/v2.11.4">https://github.com/caddyserver/caddy/releases/tag/v2.11.4</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!10336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@steadytao steadytao steadytao approved these changes
@francislavoie francislavoie Awaiting requested review from francislavoie
@mohammed90 mohammed90 Awaiting requested review from mohammed90

Assignees

No one assigned

Labels

bug 🐞 Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Client TLS not working on v2.11.3

3 participants

@mholt @steadytao