SSL Certificate Hot-Reload for Mumble Server #6974
Conversation
Implements periodic certificate expiry monitoring and automatic reloading from disk when certificates are near expiration. This eliminates the need for server restarts when SSL certificates are renewed (e.g., Let's Encrypt auto-renewal). The server checks certificate expiry every hour via QTimer. When a certificate expires within 7 days or has already expired, it attempts to reload from the configured sslCert/sslKey file paths in murmur.ini. Currently only supports file-based certificates (not database-stored). All reload operations are logged. Implementation: - Added qtCertCheck timer and initCertMonitoring() to Server class - Added checkCertExpiry() slot for hourly expiry checks - Added reloadCertFromDisk() method that calls Meta::loadSSLSettings() - Only reloads when bUsingMetaCert is true (file-based certificates) - Validates new certificate before accepting - Safe fallback keeps current certificate if reload fails New connections automatically use refreshed certificates after successful reload. Existing connections remain unaffected. Testing was performed with self-signed certificates expiring in 1 day. Certificate was replaced on disk with new 90-day cert. Next hourly check detected change and reloaded successfully without restart or disconnects. Build config: cmake -Dserver=ON -Dclient=OFF -Denable-mysql=OFF -Denable-postgresql=OFF -Dzeroconf=OFF -Dice=OFF
WalkthroughThese changes add certificate monitoring capabilities to the Server class. A Qt timer triggers hourly checks via the checkCertExpiry() method, which verifies if the qscCert will expire within 7 days or has already expired. If expiry is detected, reloadCertFromDisk() is called to reload the certificate from the file system. The reload method includes safety mechanisms: backing up current certificates before replacement and restoring them if the reload fails. The initCertMonitoring() method sets up the timer and is called during Server initialization in the constructor. Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/murmur/Cert.cpp(1 hunks)src/murmur/Server.cpp(6 hunks)src/murmur/Server.h(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/murmur/Server.cpp (1)
src/murmur/Cert.cpp (2)
initCertMonitoring(218-222)initCertMonitoring(218-218)
src/murmur/Server.h (1)
src/murmur/Cert.cpp (6)
initCertMonitoring(218-222)initCertMonitoring(218-218)checkCertExpiry(224-255)checkCertExpiry(224-224)reloadCertFromDisk(257-297)reloadCertFromDisk(257-257)
src/murmur/Cert.cpp (1)
src/murmur/Server.cpp (4)
log(1367-1370)log(1367-1367)log(1372-1380)log(1372-1372)
🔇 Additional comments (3)
src/murmur/Cert.cpp (1)
218-222: LGTM - Timer setup is straightforward.The hourly certificate check interval is appropriate for the stated goal of detecting expiring certificates within a 7-day window. The PR objectives mention making the interval configurable as a future enhancement, which is reasonable.
src/murmur/Server.h (1)
213-215: LGTM - Declarations are correct.The new certificate monitoring API is well-structured:
qtCertChecktimer is appropriately declared as a value membercheckCertExpiry()is correctly declared as a slot for Qt signal/slot connectionreloadCertFromDisk()has clear boolean return semantics for success/failureThe public visibility of these methods allows external control over certificate monitoring if needed.
Also applies to: 230-230, 241-241
src/murmur/Server.cpp (1)
257-257: ****The
qtCertChecktimer is an embedded member variable in theServerclass, not a heap-allocated pointer. Member variables are automatically destroyed via C++ RAII when the containing object is destroyed—the QTimer destructor will stop the timer synchronously. No explicitstop()call is needed or recommended in the destructor, and no race conditions exist with this pattern. The code is correct as-is.Likely an incorrect or invalid review comment.
| void Server::checkCertExpiry() { | ||
| if (qscCert.isNull()) { | ||
| return; | ||
| } | ||
|
|
||
| QDateTime now = QDateTime::currentDateTime(); | ||
| QDateTime expiryDate = qscCert.expiryDate(); | ||
| qint64 daysUntilExpiry = now.daysTo(expiryDate); | ||
|
|
||
| // Check if certificate expires within 7 days or has already expired | ||
| if (daysUntilExpiry <= 7 && daysUntilExpiry >= 0) { | ||
| log(QString("Certificate expires in %1 days, attempting to reload from disk").arg(daysUntilExpiry)); | ||
|
|
||
| if (reloadCertFromDisk()) { | ||
| log("Successfully reloaded certificate from disk"); | ||
| // Update expiry date after reload | ||
| expiryDate = qscCert.expiryDate(); | ||
| daysUntilExpiry = now.daysTo(expiryDate); | ||
| log(QString("New certificate expires in %1 days").arg(daysUntilExpiry)); | ||
| } else { | ||
| log("Failed to reload certificate from disk, keeping current certificate"); | ||
| } | ||
| } else if (daysUntilExpiry < 0) { | ||
| log(QString("Certificate has expired %1 days ago, attempting to reload from disk").arg(-daysUntilExpiry)); | ||
|
|
||
| if (reloadCertFromDisk()) { | ||
| log("Successfully reloaded certificate from disk"); | ||
| } else { | ||
| log("Failed to reload certificate from disk, keeping current expired certificate"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Add thread synchronization for certificate access.
This function reads and updates qscCert, qskKey, and qlIntermediates (via reloadCertFromDisk()) without holding any locks. Given that Server inherits from QThread and has qrwlVoiceThread for synchronizing access to shared data, certificate members should also be protected.
Consider:
- New client connections (in
newClient()around line 1435-1436) readqscCertandqskKeyto configure SSL - The voice thread may indirectly access certificate information
- Concurrent calls to
checkCertExpiry()could cause data races
Wrap certificate access with appropriate locking:
void Server::checkCertExpiry() {
+ QReadLocker lock(&qrwlVoiceThread);
+
if (qscCert.isNull()) {
return;
}
QDateTime now = QDateTime::currentDateTime();
QDateTime expiryDate = qscCert.expiryDate();
qint64 daysUntilExpiry = now.daysTo(expiryDate);
+ lock.unlock();
+
// Check if certificate expires within 7 days or has already expired
if (daysUntilExpiry <= 7 && daysUntilExpiry >= 0) {And update reloadCertFromDisk() to acquire a write lock when modifying certificates.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/murmur/Cert.cpp around lines 224 to 255, checkCertExpiry() accesses and
updates shared certificate state (qscCert, qskKey, qlIntermediates via
reloadCertFromDisk()) without locking; wrap all reads of qscCert/expiryDate and
the logic that may call reloadCertFromDisk() in a read or upgradeable lock using
the existing qrwlVoiceThread (acquire a read lock for expiry checks, upgrade to
a write lock before calling reloadCertFromDisk or modifying certificate
variables), and change reloadCertFromDisk() to acquire the write lock
(qrwlVoiceThread.lockForWrite()/unlock) while it replaces
qscCert/qskKey/qlIntermediates to prevent races with newClient() and other
threads that read these members.
| bool Server::reloadCertFromDisk() { | ||
| // Only reload if we're using Meta certificate (which is loaded from file) | ||
| if (!bUsingMetaCert) { | ||
| // This server has a database-stored certificate, cannot reload from disk | ||
| return false; | ||
| } | ||
|
|
||
| // Store current certificate as backup | ||
| QSslCertificate oldCert = qscCert; | ||
| QSslKey oldKey = qskKey; | ||
| QList< QSslCertificate > oldInter = qlIntermediates; | ||
|
|
||
| // Force Meta to reload its SSL settings from disk | ||
| if (!Meta::mp->loadSSLSettings()) { | ||
| log("Failed to reload SSL settings from murmur.ini"); | ||
| return false; | ||
| } | ||
|
|
||
| // Try to reload from Meta's certificate | ||
| qskKey = Meta::mp->qskKey; | ||
| qscCert = Meta::mp->qscCert; | ||
| qlIntermediates = Meta::mp->qlIntermediates; | ||
|
|
||
| // Verify that the reloaded certificate is valid and different | ||
| if (qscCert.isNull() || qskKey.isNull()) { | ||
| // Reload failed, restore old certificate | ||
| qscCert = oldCert; | ||
| qskKey = oldKey; | ||
| qlIntermediates = oldInter; | ||
| return false; | ||
| } | ||
|
|
||
| // Check if certificate actually changed | ||
| if (qscCert == oldCert) { | ||
| // Certificate hasn't changed on disk | ||
| return false; | ||
| } | ||
|
|
||
| // Successfully reloaded new certificate | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Critical: Add validation and null pointer checks.
Multiple safety issues in the certificate reload logic:
-
Null pointer dereference risk:
Meta::mpis accessed without null check (lines 270, 276-278). If Meta is not initialized, this will crash. -
Missing certificate validation: After reloading, the code only checks if the certificate and key are null (line 281) but doesn't verify:
- The key actually matches the certificate (should use
isKeyForCert()from line 24) - The new certificate isn't already expired
- The certificate is properly formed
- The key actually matches the certificate (should use
-
Thread safety: Same issue as
checkCertExpiry()- certificate members are modified without locking.
Apply these fixes:
bool Server::reloadCertFromDisk() {
// Only reload if we're using Meta certificate (which is loaded from file)
if (!bUsingMetaCert) {
// This server has a database-stored certificate, cannot reload from disk
return false;
}
+ if (!Meta::mp) {
+ log("Meta not initialized, cannot reload certificate");
+ return false;
+ }
+
// Store current certificate as backup
QSslCertificate oldCert = qscCert;
QSslKey oldKey = qskKey;
QList< QSslCertificate > oldInter = qlIntermediates;
// Force Meta to reload its SSL settings from disk
if (!Meta::mp->loadSSLSettings()) {
log("Failed to reload SSL settings from murmur.ini");
return false;
}
+ QWriteLocker lock(&qrwlVoiceThread);
+
// Try to reload from Meta's certificate
qskKey = Meta::mp->qskKey;
qscCert = Meta::mp->qscCert;
qlIntermediates = Meta::mp->qlIntermediates;
// Verify that the reloaded certificate is valid and different
if (qscCert.isNull() || qskKey.isNull()) {
// Reload failed, restore old certificate
qscCert = oldCert;
qskKey = oldKey;
qlIntermediates = oldInter;
return false;
}
+ // Verify the key matches the certificate
+ if (!isKeyForCert(qskKey, qscCert)) {
+ log("Reloaded certificate and key do not match");
+ qscCert = oldCert;
+ qskKey = oldKey;
+ qlIntermediates = oldInter;
+ return false;
+ }
+
+ // Check if new certificate is already expired
+ if (qscCert.expiryDate() < QDateTime::currentDateTime()) {
+ log("Reloaded certificate is already expired");
+ qscCert = oldCert;
+ qskKey = oldKey;
+ qlIntermediates = oldInter;
+ return false;
+ }
+
// Check if certificate actually changed
if (qscCert == oldCert) {
// Certificate hasn't changed on disk
return false;
}
// Successfully reloaded new certificate
return true;
}🤖 Prompt for AI Agents
In src/murmur/Cert.cpp around lines 257-297, add null-pointer, validation and
locking: first check Meta::mp is not null before calling loadSSLSettings or
accessing its members and return false with a log if it is null; wrap all
reads/writes to qscCert, qskKey and qlIntermediates with the same mutex used
elsewhere for certificate state (use the appropriate lock/guard to ensure
thread-safety); after copying Meta::mp->qscCert/qskKey/qlIntermediates validate
the new certificate by ensuring it is not null, the key matches the certificate
(call isKeyForCert or the existing helper), the certificate parses correctly and
is not expired (check its effective/expiry dates), and if any validation fails
restore oldCert/oldKey/oldInter, unlock and return false; only return true after
successful validation and replacing the state while holding the lock.
The checkCertExpiry() function was accessing shared certificate state (qscCert, qskKey, qlIntermediates) without any locking, creating a race condition with newClient() and other threads that read these values. Additionally, the initial implementation created a new mutex (qmCert) instead of using the existing qrwlVoiceThread read-write lock that protects certificate state throughout the codebase. Using two different locks to protect the same data provides no protection. Changes: - checkCertExpiry() now uses QReadLocker(&qrwlVoiceThread) when reading certificate state to check expiry dates - reloadCertFromDisk() uses QReadLocker to safely copy old state for comparison, then lockForWrite()/unlock() for atomic certificate updates - Removed qmCert mutex entirely in favor of existing qrwlVoiceThread lock This ensures all certificate access uses a single, consistent locking mechanism and prevents data races during hot-reload operations.
|
Wow that coderabbit review demon machine slaps hahahaha. |
|
The server only needs the SIGUSR1 signal to reload certs. You do not need to restart and interrupt users. And if you have systemd you can run reload as that's what it does. I just run systemctl reload in my lets-encrypt deploy script. https://github.com/search?q=repo%3Amumble-voip%2Fmumble+SIGUSr1&type=code |
|
Thanks I'm clearly an idiot.
…On October 29, 2025 11:43:32 AM PDT, HaMiLToN5 ***@***.***> wrote:
hamilton5 left a comment (mumble-voip/mumble#6974)
The server only needs the SIGUSR1 signal to reload certs. You do not need to restart and intruppt users. And if you have systemd you can run reload as that's what it does. I just run systemctl reload in my lets-encrypt deploy script.
https://github.com/search?q=repo%3Amumble-voip%2Fmumble+SIGUSr1&type=code
--
Reply to this email directly or view it on GitHub:
#6974 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
2 participants
Currently, when SSL certificates expire or are renewed (like with Let's Encrypt auto-renewal that expire every few months), the Mumble server requires a full restart to pick up the new certificates. This causes service interruption and disconnects all connected users. A friend of mine runs a HUGE (small) mumble server and was annoyed by this. I've worked on a QT project before (GQRX) so somewhat familiar with how this should go.
The server will check every hour for a new certificate with QTimer, and will try to reload from the configured ssl file paths in murmur.ini if we are within a week of expiry.
This just works for file based certificates for now, so not database stored certs but that would be an obvious fleshing out of this. The cert checking activity is logged, idk if this is too aggressive for you guys.
I tested this with some self signed test certs, and had some test code to shorten the expiry checking (not in this commit) - started with a certificate expiring in a day, triggering our check for a replacement. I replaced the cert on the disk with a new 90-day expiry one, and the next check detected the change and reloaded the certs successfully. So no restart or connection drops needed, new connects will use the new cert.
Build configuration used for testing (I didn't want to install all the dependancies):
cmake -Dserver=ON -Dclient=OFF -Denable-mysql=OFF -Denable-postgresql=OFF -Dzeroconf=OFF -Dice=OFFUser-Visible Changes
Future Enhancements
Checks
Just a note here: I did use Claude Code for a lot of this, making sure I complied with the commit guidelines and the development guide, so if the formatting or structure seems unusually polished for a first-time contributor, that's why. I can write CPP just fine on my own (cope 😭 ), I'm just 10x faster with this tool, and 100x faster with getting familiar with a codebase. Happy to adjust anything that doesn't fit the project's style or conventions, but I did use clang-format and supervised the demon machine throughout the entire thing. Tested compilation on master before making any edits, made my edits, tested compilation, added some test code and swapped out certs (as described above) compiled and tested again, removed the test code to force it to trigger without waiting a few hours, and compiled and tested again, removed the test code, compiled and well, here we are.
Speaking of, changes to spacing of server.cpp is just from clang-format.
Thanks!