Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dnsdist: Always store the OpenSSLTLSIOCtx in the connection #14671

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Sep 13, 2024

Short description

Instead of storing the frontend context for server-side connection and the SSL_CTX* for client-side.

The first commit in this PR fixes a race condition where reloading the certificate and key via reloadAllCertificates(), for example, could cause the ALPN-related data to be destroyed while it was still used by an existing TLS connection. The time window to trigger the issue is quite narrow because ALPN data is only accessed during the initial handshake, but we have got one report of it happening in the wild, and TSAN confirms the race. This is not a security issue since it cannot be triggered without administrative access to dnsdist.
The remaining commits are making this code more clean and robust, and should probably not be backported.
I would recommend reviewing this PR commit per commit!

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne rgacogne added this to the dnsdist-2.0.0 milestone Sep 13, 2024
@coveralls
Copy link

coveralls commented Sep 13, 2024

Pull Request Test Coverage Report for Build 10882923159

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 62 of 66 (93.94%) changed or added relevant lines in 3 files are covered.
  • 55 unchanged lines in 17 files lost coverage.
  • Overall coverage increased (+0.002%) to 64.681%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/tcpiohandler.cc 49 53 92.45%
Files with Coverage Reduction New Missed Lines %
pdns/libssl.cc 1 59.01%
ext/lmdb-safe/lmdb-typed.hh 1 73.6%
pdns/packethandler.cc 1 72.51%
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
modules/lmdbbackend/lmdbbackend.cc 2 73.54%
pdns/ws-auth.cc 2 80.8%
pdns/recursordist/test-syncres_cc2.cc 3 88.91%
pdns/misc.hh 3 87.62%
pdns/tsigverifier.cc 3 77.22%
pdns/fstrm_logger.cc 3 44.08%
Totals Coverage Status
Change from base Build 10881391740: 0.002%
Covered Lines: 124775
Relevant Lines: 162216

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants