Skip to content

Expand %x and %D after bumped SQUID_X509_V_ERR_DOMAIN_MISMATCH#2373

Closed
somecookie wants to merge 4 commits intosquid-cache:masterfrom
measurement-factory:OSD-2-bumped-domain-mismatch-details
Closed

Expand %x and %D after bumped SQUID_X509_V_ERR_DOMAIN_MISMATCH#2373
somecookie wants to merge 4 commits intosquid-cache:masterfrom
measurement-factory:OSD-2-bumped-domain-mismatch-details

Conversation

@somecookie
Copy link
Contributor

Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various
processing stages, including when receiving an HTTP request on a
successfully bumped TLS connection. If that request targets a domain not
covered by the server certificate, and sslproxy_cert_error prohibits a
mismatch (it does by default), then Squid terminates the transaction
with an ERR_SECURE_CONNECT_FAIL response. That generated error response
body lacked %x and %D error details:

  The system returned:

- [No Error] (TLS code: [Unknown Error Code])
+ [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH)

- [No Error Detail]
+ Certificate does not match domainname: /L=.../O=.../CN=example.com

The first [No Error] expansion of %E remains unchanged because this
particular error does not set errno.

ConnStateData::serveDelayedError() changes fix the above problem but %x
expansion in error pages and %err_detail in access log get a misleading
+broken_cert detail. To address that flaw, we changed the default for
broken certificate in Security::ErrorDetail constructor API from peer
certificate to nil. When broken certificate is nil, ErrorDetail now uses
valid certificate to expand %ssl_cn and similar certificate-inspecting
error page %codes.

All Security::ErrorDetail creators were checked and adjusted if needed:

  • ConnStateData::serveDelayedError(): No caller changes. Using the new
    ErrorDetail creation API fixes this code by supplying nil broken
    certificate (because the certificate is valid in this context).

  • ssl_verify_cb(): No caller changes. We already use peer certificate as
    the default broken certificate because doing so is "reasonable" here.

  • Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep
    the original "if there was no error_cert_ID, then use peerCert"
    behavior while using new Security::ErrorDetail creation API.

Thus, the last two contexts are not affected by this error reporting API
change. The exceptional serveDelayedError() caller is affected, but
Squid did not report any certificate detail in that case until this
branch fixes, so this branch does not change one "reporting certificate"
to another; it only starts reporting (important) information when none
was available before.

This is a Measurement Factory project.

rousskov and others added 4 commits February 9, 2026 17:09
Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors on various
processing stages, including when receiving an HTTP request on a
successfully bumped TLS connection. If that request targets a domain not
covered by the server certificate, and sslproxy_cert_error prohibits a
mismatch (it does by default), then Squid terminates the transaction
with an ERR_SECURE_CONNECT_FAIL response. That generated error response
body lacked %x and %D error details:

```diff
  The system returned:

- [No Error] (TLS code: [Unknown Error Code])
+ [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH)

- [No Error Detail]
+ Certificate does not match domainname: /L=.../O=.../CN=a.test
```

The first `[No Error]` expansion of %E remains unchanged because this
particular error does not set `errno`.

ConnStateData::serveDelayedError() changes fix the above problem but %x
expansion in error pages and %err_detail in access log get a misleading
`+broken_cert` detail. To address that flaw, we changed the default for
broken certificate in Security::ErrorDetail constructor API from peer
certificate to nil. When broken certificate is nil, ErrorDetail now uses
valid certificate to expand %ssl_cn and similar certificate-inspecting
error page %codes.

All Security::ErrorDetail creators were checked and adjusted if needed:

* ConnStateData::serveDelayedError(): No caller changes. Using the new
  ErrorDetail creation API fixes this code by supplying nil broken
  certificate (because the certificate is _valid_ in this context).

* ssl_verify_cb(): No caller changes. We already use peer certificate as
  the default broken certificate because doing so is "reasonable" here.

* Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep
  the original "if there was no error_cert_ID, then use peerCert"
  behavior while using new Security::ErrorDetail creation API.

Thus, the last two contexts are not affected by this error reporting API
change. The exceptional serveDelayedError() caller is affected, but
Squid did not report any certificate detail in that case until this
branch fixes, so this branch does not change one "reporting certificate"
to another; it only starts reporting (important) information when none
was available before.

----

Squash-merged (fast-forward eligible)
SQUID-1088-bumped-domain-mismatch-details-bag64a as of commit 7a3c87b.
@somecookie somecookie marked this pull request as ready for review February 10, 2026 16:40
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

The earlier variation of these changes was tested in production. This master-based variation was lab-tested.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Feb 10, 2026
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

LGTM

squid-anubis pushed a commit that referenced this pull request Feb 12, 2026
Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various
processing stages, including when receiving an HTTP request on a
successfully bumped TLS connection. If that request targets a domain not
covered by the server certificate, and sslproxy_cert_error prohibits a
mismatch (it does by default), then Squid terminates the transaction
with an ERR_SECURE_CONNECT_FAIL response. That generated error response
body lacked %x and %D error details:

```diff
  The system returned:

- [No Error] (TLS code: [Unknown Error Code])
+ [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH)

- [No Error Detail]
+ Certificate does not match domainname: /L=.../O=.../CN=example.com
```

The first `[No Error]` expansion of %E remains unchanged because this
particular error does not set `errno`.

ConnStateData::serveDelayedError() changes fix the above problem but %x
expansion in error pages and %err_detail in access log get a misleading
`+broken_cert` detail. To address that flaw, we changed the default for
broken certificate in Security::ErrorDetail constructor API from peer
certificate to nil. When broken certificate is nil, ErrorDetail now uses
valid certificate to expand %ssl_cn and similar certificate-inspecting
error page %codes.

All Security::ErrorDetail creators were checked and adjusted if needed:

* ConnStateData::serveDelayedError(): No caller changes. Using the new
  ErrorDetail creation API fixes this code by supplying nil broken
  certificate (because the certificate is _valid_ in this context).

* ssl_verify_cb(): No caller changes. We already use peer certificate as
  the default broken certificate because doing so is "reasonable" here.

* Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep
  the original "if there was no error_cert_ID, then use peerCert"
  behavior while using new Security::ErrorDetail creation API.

Thus, the last two contexts are not affected by this error reporting API
change. The exceptional serveDelayedError() caller is affected, but
Squid did not report any certificate detail in that case until this
branch fixes, so this branch does not change one "reporting certificate"
to another; it only starts reporting (important) information when none
was available before.

This is a Measurement Factory project.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Feb 12, 2026
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 12, 2026
@rousskov rousskov removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Feb 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-merged https://github.com/measurement-factory/anubis#pull-request-labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants