Migrate to OpenSSL - #2941
Conversation
|
Failing CI is expected here for now. FTL builds inside the |
There was a problem hiding this comment.
Pull request overview
Migrates FTL’s inbound HTTPS/TLS stack (civetweb + FTL X.509 tooling) from mbedTLS to OpenSSL (>= 3.0), aligning with civetweb’s OpenSSL backend and reworking certificate generation/inspection accordingly.
Changes:
- Switch build flags and linking from mbedTLS to OpenSSL (
HAVE_TLS,OPENSSL_API_3_0, linklibssl/libcrypto). - Reimplement
x509.ccertificate/key generation and inspection using OpenSSL EVP/X509 APIs, and update CLI output. - Update Bats tests to compare OpenSSL-style
--read-x509output against recorded expectation files (whitespace-tolerant).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/x509_read.expected | Adds recorded expected --read-x509 output (OpenSSL-style). |
| test/x509_read_key.expected | Adds recorded expected --read-x509-key output (includes private key dump). |
| test/test_suite.bats | Updates X.509 tests to diff -w against expectation files. |
| src/webserver/x509.c | Reimplements certificate generation, reading, domain matching, and validity checks using OpenSSL. |
| src/webserver/webserver.c | Switches TLS-related functionality to OpenSSL (cipher listing, TLS feature gating). |
| src/webserver/civetweb/mod_mbedtls.inl | Removes mbedTLS backend implementation file. |
| src/webserver/civetweb/civetweb.h | Removes mbedTLS debug hook declarations. |
| src/dnsmasq_interface.c | Renames TLS compile-time gating from HAVE_MBEDTLS to HAVE_TLS. |
| src/CMakeLists.txt | Switches TLS detection/linking from mbedTLS libs to OpenSSL libs; renames option to USE_TLS. |
| src/args.c | Updates CLI/version reporting and TLS gating to OpenSSL (HAVE_TLS). |
| patch/civetweb/0001-Add-mbedTLS-debug-logging-hook.patch | Removes now-obsolete civetweb patch for mbedTLS debug hook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I was about to get my branch ready, but yours looks much cleaner. I do think looking at using APIs supported in both 3.5 and 4.0 is worth it. I spun up a quick COPR to get build logs: https://copr.fedorainfracloud.org/coprs/darkexplosiveqwx/pihole-dev/builds/ (ignore the first three, It's your branch starting at the 4th) It found some compile issues: civetweb still uses the old openSSL 1.1 header You may tell your AI to take a look at the openSSL 4.0 compile log, maybe it can find some APIs that are supported in both 3.5 and 4.0: https://download.copr.fedorainfracloud.org/results/darkexplosiveqwx/pihole-dev/fedora-rawhide-x86_64/10699161-pihole-ftl-nightly/builder-live.log.gz |
|
My local compile attempt inside the container with OpenSSL 3.5 (pi-hole/docker-base-images#167) went through fine on this branch |
My COPR doesn't install the legacy |
|
No, the configure command line is with |
From what I found, engine is a separate Engine is discouraged since 3.0 and has been replaced with Providers. Edit: oops I totally missed that it already includes Edit2: The engine header files still seem to be shipped, even with %files devel
%doc CHANGES.md doc/dir-locals.example.el doc/openssl-c-indent.el
%{_prefix}/include/openssl
%exclude %{_prefix}/include/openssl/engine*.h
%{_libdir}/*.so
%{_mandir}/man3/*
%exclude %{_mandir}/man3/ENGINE*
%{_libdir}/pkgconfig/*.pc
%{_libdir}/cmake/OpenSSL/OpenSSLConfig.cmake
%{_libdir}/cmake/OpenSSL/OpenSSLConfigVersion.cmake
%if %{with engine}
%files devel-engine
%{_prefix}/include/openssl/engine*.h
%{_mandir}/man3/ENGINE*
%endifWe should still not depend on an outdated header, that just happens to be available. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Perhaps another stupid question on my part - would the ECH support introduced in OpenSSL v4.0 have any relevance or benefit here? |
Yes, not in this migration but in the context of DoT/DoH (parallel PR #2940); when FTL forwards to an upstream encrypted resolver (Cloudflare, Quad9…), the Does it really make a difference? Likely not. The DoH servers will be dedicated IP addresses serving one purpose so the metadata of the connection alone is likely equally sufficient for possible tracking purposes (thinking of "bad" countries and/or ISPs here). Anyway, the client would need to fetch the upstream's ECH config from a DNS HTTPS record (chicken-and-egg: you need DNS to bootstrap the ECH config) and wire OpenSSL's |
Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Switched this from OpenSSL 3.5 to OpenSSL 4.0. 4.0 ships Depends on pi-hole/docker-base-images#173, which builds the |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. |
9bf5357 to
6ba1105
Compare
Signed-off-by: DL6ER <dl6er@dl6er.de>
975519b to
01ffa4c
Compare
|
Green on all architectures now (amd64, 386, arm/v6, arm/v7, arm64 and riscv64), so this is really ready. For the record, I also put the branch through an intense AI-assisted review - many adversarial passes over the fail-closed forwarding, the connection pool, TLS session resumption and the certificate handling. It came back sound bar one real bug (a slow upstream could stall a TLS handshake past its deadline), which is fixed here; everything else it flagged was cosmetic and would actually make the code much longer to handle edge cases, the current code is already correctly rejecting instead. Clients will retry and then it will work. |
defd564 to
4bfcd6c
Compare
|
Even with OpenSSL 4.0 there still seems to be some legacy engine leftovers. Inside the new ftl-build / # ls -lan /usr/local/include/openssl/engine.h
-rw-r--r-- 1 0 0 50770 Jul 24 10:52 /usr/local/include/openssl/engine.h/ # cat /usr/local/include/openssl/engine.h
/*
* Copyright 2000-2022 The OpenSSL Project Authors. All Rights Reserved.
* Copyright (c) 2002, Oracle and/or its affiliates. All rights reserved
*
* Licensed under the Apache License 2.0 (the "License"). You may not use
* this file except in compliance with the License. You can obtain a copy
* in the file LICENSE in the source distribution or at
* https://www.openssl.org/source/license.html
*/
#ifndef OPENSSL_ENGINE_H
#define OPENSSL_ENGINE_H
#pragma once
#include <openssl/macros.h>
#ifndef OPENSSL_NO_DEPRECATED_3_0
#define HEADER_ENGINE_H
#endif
#include <openssl/opensslconf.h>
/*
* Engine support is gone. Definitions here are provided for the source code
* compatibility only. They are meant to keep compilation working for legacy
* projects that, for whatever reason, cannot remove/disable old legacy code.
*
* We deliberately keep the OPENSSL_NO_ENGINE macro around as it is supplied
* when the build is configured with the `no-engine` option. OpenSSL 4.0 keeps
* the `no-engine` option around.
*
* Note, we have to use a compile-time message to warn only if the API is really
* used. To avoid complex macros, we kind of abuse the existing OSSL_DEPRECATED
* macros.
*/
[...]we can still compile with openSSL 4.0 since civetweb includes the header without using any functions from it, but I still think that it is very ugly nonetheless. In pi-hole/docker-base-images#167 you mentioned:
This is especially interesting, how would this connect to civetweb doing TLS for h1.1, where it still uses the old openssl headers? |
|
I've upgrades my test Pi to the latest FTL and get some error in |
|
I only see: which I think is normal, since it is a Self-Signed Certificate. |
I get this as well.
Mhh.. maybe it is, but can it be suppressed? Users will think, something is wrong. |
|
You are right, this is a side effect of the mbedTLS -> OpenSSL switch, but harmless. mbedTLS silently dropped these received alerts, whereas OpenSSL pushes them onto its error queue and CivetWeb logs the queue verbatim, i.e., once per connection attempt. So nothing actually fails, the user clicks through the browser warning and the connection proceeds as before, we just gained a log line we did not have with mbedTLS. It should be quieted down though, so we will demote these accept-side client alerts to a debug message rather than crying them into |
Since the migration from mbedTLS to OpenSSL (pi-hole#2941), OpenSSL surfaces the TLS alert a browser sends when it rejects our self-signed certificate (`certificate_unknown` / `unknown_ca`) on its error queue. CivetWeb dumps that queue verbatim through `mg_cry_internal`, so `webserver.log` now fills with `sslize error: ...` lines on every fresh connection - something mbedTLS silently swallowed before. Such a handshake is recoverable, i.e., the user clicks through the warning and the connection proceeds, so it is not really an error on our side. When the failing `sslize` runs in a server context and the reason is a received TLS alert (reason >= `SSL_AD_REASON_OFFSET`), log it via `DEBUG_TRACE` instead, which only prints when `debug.webserver` is enabled. Genuine SSL errors and the client path keep crying into the log as before. The in-tree `civetweb.c` change is mirrored in `patch/civetweb/0001-Demote-server-side-TLS-handshake-alerts-to-debug.patch` so it survives the next CivetWeb update. Signed-off-by: DL6ER <dl6er@dl6er.de>
What does this implement/fix?
Migrate FTL's TLS stack from mbedTLS to OpenSSL (>= 3.0). First step toward DoQ/DoH3, which require QUIC.
Why
Webserver / X.509
libssl/libcryptoinstead of the three mbedTLS libraries;OPENSSL_API_3_0for civetweb,HAVE_TLSfor the FTL code (replacingUSE_MBEDTLS/HAVE_MBEDTLS).X509_*certificate creation (SHA-256), self-signed CA and SANs, PEM/X509 reading and validation;--read-x509prints the same output as the openssl CLI.FTL_mbed_debugand the unusedmod_mbedtls.inl.Encrypted-upstream client (
src/dotdoh)Removing mbedTLS everywhere also means porting the DoT/DoH client; that port rebuilds it as a parallel forwarder, using the thread-safe
SSL_CTXsharing and session resumption OpenSSL makes easy.SSL_VERIFY_PEER+ host/IP verification); bare-IP upstreams verify against the certificate's iPAddress SANs.debug.dotdohreports per-upstream stats.nettle (DNSSEC, password hashing, TOTP) is unaffected. TLS stays optional and disabled when libssl/libcrypto are unavailable. Build side: pi-hole/docker-base-images#167 (static OpenSSL 3.5 in
ftl-build, consumed here asv2.20).How to test the change during review
pihole-FTL --gen-x509 ...thenpihole-FTL --read-x509should matchopenssl x509 -text;pihole-FTL -vvreports the OpenSSL version and providers.dns.upstreamsto atls://orhttps://resolver (plusdns.upstreamCAif needed) and confirmdig @127.0.0.1 example.comresolves and the real upstream (not the internal127.47.11.N) is shown in the query log.debug.dotdoh = trueand watch the periodic per-upstream summary (throughput, connection reuse depth, resumed vs full handshakes).test/dotdoh.batscovers DoT and DoH concurrency and keep-alive end to end; the full CI matrix builds and tests all seven arch/compiler targets.Related issue or feature (if applicable): N/A
Pull request in docs with documentation (if applicable): pi-hole/docs#1400
By submitting this pull request, I confirm the following:
git rebase)Checklist:
developmentbranch.