Skip to content

Fix host name verification for PerfdataWriterConnection#10799

Merged
jschmidt-icinga merged 3 commits into
masterfrom
fix-pdwc-tls-host-check
Apr 22, 2026
Merged

Fix host name verification for PerfdataWriterConnection#10799
jschmidt-icinga merged 3 commits into
masterfrom
fix-pdwc-tls-host-check

Conversation

@jschmidt-icinga
Copy link
Copy Markdown
Contributor

@jschmidt-icinga jschmidt-icinga commented Apr 17, 2026

Fixes the host name verification for PerfdataWriterConnection by correctly appending the optional host parameter when constructing the AsioTlsStream. This was omitted from the original PR #10668 by accident, despite the writers correctly passing the parameter before.

Testing

Given that the writers worked with this check in place before #10668, and PerfdataWriterConnection otherwise was verified in that PR, I'll not retest all TLS-capable backends this time.

Also, since the PerfdataWriterConnection unit-tests failed after this change and the fixtures used needed two small adjustments to work again, that verifies both that the check does fail if the host names don't match up and succeeds when they are made to match up again.

I've also added an additional test-case that ensures the verification fails when the host name doesn't match. The test case will be moved to a separate PR linking to this one at a later point.

Fixes #10797.

This was omitted by accident from the original PR, despite
being done in the original perfdata writer connection code.

Without setting this parameter, host name verification will be
disabled, which poses a security risk.
@cla-bot cla-bot Bot added the cla/signed label Apr 17, 2026
@jschmidt-icinga jschmidt-icinga added this to the 2.16.0 milestone Apr 17, 2026
@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch 3 times, most recently from 8691b83 to f4127d2 Compare April 17, 2026 11:40
@jschmidt-icinga jschmidt-icinga marked this pull request as ready for review April 17, 2026 11:44
@jschmidt-icinga jschmidt-icinga marked this pull request as draft April 17, 2026 12:03
@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch from c06009c to 6172edb Compare April 20, 2026 06:44
@jschmidt-icinga jschmidt-icinga marked this pull request as ready for review April 20, 2026 06:50
@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch from 6172edb to aa3a0c5 Compare April 20, 2026 10:23
@yhabteab yhabteab added bug Something isn't working area/metrics General metrics handling labels Apr 20, 2026
yhabteab
yhabteab previously approved these changes Apr 20, 2026
Copy link
Copy Markdown
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

I don't know what's going on with LibreSSL either, so no idea what else to do.

@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch from c5c1123 to aab0b64 Compare April 21, 2026 14:12
julianbrost
julianbrost previously approved these changes Apr 21, 2026
Copy link
Copy Markdown
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

The implementation change itself is fine.

Regarding the tests, correct me if I'm wrong: there was a new test case previously added in this PR which triggered some race condition (in other test cases) with high probability the ARM-emulated Docker builds (to an extent that retry didn't help). Thus, include the fix in the 2.16.0 (so that we don't have a security regression), but delay the addition of the test case itself past that release.

@yhabteab
Copy link
Copy Markdown
Member

I don't really know what's going on with this PR today but what makes you think the tests run fine now? The docker arm build still hangs.

@jschmidt-icinga
Copy link
Copy Markdown
Contributor Author

jschmidt-icinga commented Apr 22, 2026

I've cherry-picked a commit from #10780 because I noticed that it's only the perfdata_connection/connection_refused test that fails here, which is the one I had already figured out for a while in #10780 (unlike stuck_in_handshake which continues to baffle me). 🤞

Edit: Ignore the back and forth on the Listen(). I got confused why I split up the function and never added it to connection_refused, but that was the entire point that fixes the test case. 🤦 Much too early in the day to efficiently panic. 🥱

@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch 2 times, most recently from 8a1fcc0 to 88dc49d Compare April 22, 2026 06:33
@jschmidt-icinga jschmidt-icinga force-pushed the fix-pdwc-tls-host-check branch from 88dc49d to 7ee3386 Compare April 22, 2026 06:36
Copy link
Copy Markdown
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

I hope this is it now 🤞

@jschmidt-icinga jschmidt-icinga merged commit 03d3558 into master Apr 22, 2026
28 checks passed
@jschmidt-icinga jschmidt-icinga deleted the fix-pdwc-tls-host-check branch April 22, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/metrics General metrics handling bug Something isn't working cla/signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken TLS hostname checks in perfdata writers after refactoring (in master branch only)

3 participants