Fix epoll dispatch dropping INotify events, ConsoleServer self-deadlock, and add TLS SNI support
- INotify/Thread epoll dispatch was silently dropping every live core::INotify event (an isKnownINotify registry fixes it) - this had broken mail delivery for over a week, masked by routine restarts. - TCPServer::lock self-deadlocked every admin-console connection via a plain std::mutex being re-entered; switched to recursive_mutex. - TLS.cpp/h: SNI-based certificate selection (addSNIContext), the groundwork for multi-tenant TLS (qualityiron.vegas alongside barant.com on one APIServer process). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
d058322cc6
commit
1d3d30cd05
8 files changed
+216
-49
No files matched your search
+1
-1
@@ -16,7 +16,7 @@ namespace core {
|
||||
// TCPServer::sessions became shared_ptr-owned - see its own doc
|
||||
// comment) despite connectionRequest()/removeFromSessionList()
|
||||
// both mutating this same vector under `lock` from other threads.
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
for(auto &session : sessions)
|
||||
((ConsoleSession *)session.get())->writeLog(out);
|
||||
}
|
||||
|
||||
@@ -7,10 +7,16 @@ namespace core {
|
||||
|
||||
INotify::INotify(EPoll &ePoll) : Socket(ePoll, "INotify") {
|
||||
setDescriptor(inotify_init());
|
||||
// See isKnownINotify()'s own doc comment - registers this object
|
||||
// as safe for Thread::run() to dispatch epoll events to directly.
|
||||
std::lock_guard<std::mutex> guard(knownINotifiesMutex());
|
||||
knownINotifies().insert(this);
|
||||
}
|
||||
|
||||
INotify::~INotify() {
|
||||
shutdown();
|
||||
std::lock_guard<std::mutex> guard(knownINotifiesMutex());
|
||||
knownINotifies().erase(this);
|
||||
}
|
||||
|
||||
int INotify::addWatch(coreutils::ZString &watch) {
|
||||
|
||||
@@ -2,6 +2,8 @@
|
||||
# define __INotify_h__
|
||||
|
||||
#include "Socket.h"
|
||||
#include <set>
|
||||
#include <mutex>
|
||||
|
||||
namespace core {
|
||||
|
||||
@@ -29,6 +31,41 @@ namespace core {
|
||||
virtual void inMovedTo(std::string name) {}
|
||||
virtual void inOpen(std::string name) {}
|
||||
|
||||
/// core::INotify is a long-lived Socket subclass that is neither
|
||||
/// a TCPSession (shared_ptr/pin()-owned) nor a listening
|
||||
/// TCPServer - it is normally embedded as a base subobject of a
|
||||
/// server object constructed once in main() and living for the
|
||||
/// whole process (e.g. mail::SMTPServer's queue-directory watch).
|
||||
/// Thread::run()'s epoll dispatch only ever recognized those two
|
||||
/// categories of raw pointer as safe to call directly (see
|
||||
/// TCPServer::isKnownListeningSocket()'s own doc comment) -
|
||||
/// anything else fell through to the "stale/unrecognized epoll
|
||||
/// event" branch and was silently dropped forever. Confirmed
|
||||
/// live: a real IN_CREATE event on a mail queue directory logged
|
||||
/// exactly that "Ignoring stale/unrecognized epoll event" line
|
||||
/// and the newly queued message was never delivered - it just sat
|
||||
/// in the queue until the next process restart, whose
|
||||
/// processExisting() directory scan (a plain synchronous loop,
|
||||
/// entirely independent of epoll) happened to flush the backlog
|
||||
/// and made the bug look intermittent. Registering the instance here,
|
||||
/// mirroring listeningSockets() exactly, gives Thread::run() the
|
||||
/// same kind of safe "yes, dispatch to this raw pointer directly"
|
||||
/// signal for INotify objects too.
|
||||
static bool isKnownINotify(void *ptr) {
|
||||
std::lock_guard<std::mutex> guard(knownINotifiesMutex());
|
||||
return knownINotifies().count(ptr) > 0;
|
||||
}
|
||||
|
||||
private:
|
||||
static std::mutex &knownINotifiesMutex() {
|
||||
static std::mutex m;
|
||||
return m;
|
||||
}
|
||||
static std::set<void *> &knownINotifies() {
|
||||
static std::set<void *> s;
|
||||
return s;
|
||||
}
|
||||
|
||||
};
|
||||
|
||||
}
|
||||
|
||||
+5
-5
@@ -174,7 +174,7 @@ namespace core {
|
||||
// (combat, position, chat) go through, so this is very likely the
|
||||
// actual dominant cause of that crash, not just the ItemDataBase
|
||||
// subscriber list this session's earlier fix addressed.
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
int sequence = 0;
|
||||
for (auto &sessionx : sessions)
|
||||
{
|
||||
@@ -186,14 +186,14 @@ namespace core {
|
||||
}
|
||||
|
||||
void TCPServer::sendToAll(std::stringstream &data) {
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
for (auto &session : sessions)
|
||||
session->write(data.str());
|
||||
data.str("");
|
||||
}
|
||||
|
||||
void TCPServer::sendToAll(std::stringstream &data, TCPSession &sender) {
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
for (auto &session : sessions)
|
||||
if (session.get() != &sender)
|
||||
session->write(data.str());
|
||||
@@ -201,7 +201,7 @@ namespace core {
|
||||
}
|
||||
|
||||
void TCPServer::sendToAll(std::stringstream &data, TCPSession &sender, SessionFilter filter) {
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
for (auto &session : sessions)
|
||||
if (filter.test(*session))
|
||||
if (session.get() != &sender)
|
||||
@@ -211,7 +211,7 @@ namespace core {
|
||||
|
||||
std::shared_ptr<TCPSession> TCPServer::getSessionByAlias(void *alias) {
|
||||
coreutils::Log(coreutils::LOG_DEBUG_1) << alias;
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
std::lock_guard guard(lock);
|
||||
for (auto &session : sessions)
|
||||
if (session->alias == alias)
|
||||
return session;
|
||||
|
||||
+27
-1
@@ -182,7 +182,33 @@ namespace core {
|
||||
// able to synchronize against the exact same mutex
|
||||
// connectionRequest()/removeFromSessionList() already use, rather
|
||||
// than reading a shared_ptr vector with no locking of its own.
|
||||
std::mutex lock;
|
||||
//
|
||||
// Was a plain std::mutex - self-deadlocked the accepting thread on
|
||||
// EVERY single connection to ConsoleServer (confirmed live via a
|
||||
// gdb thread dump caught mid-stall): connectionRequest() locks
|
||||
// `lock`, then calls accept(), which constructs the new session -
|
||||
// whose base Socket constructor logs its own creation via
|
||||
// coreutils::Log, whose destructor calls back into
|
||||
// ConsoleServer::logSend() (exactly the synchronization this was
|
||||
// designed for, per the comment above) - which tries to lock this
|
||||
// SAME mutex again, on the SAME thread that already holds it.
|
||||
// std::mutex is not reentrant, so that thread deadlocked
|
||||
// permanently, every time, without ever completing the accept -
|
||||
// this is why the admin console never responded to anything.
|
||||
// Since all of a server's listening sockets share one fixed-size
|
||||
// EPoll worker-thread pool, each such deadlocked thread is gone
|
||||
// for good - enough accumulated connection attempts (even
|
||||
// sporadic ones, e.g. internet-wide port scanners probing every
|
||||
// port) eventually exhausts every worker thread, at which point
|
||||
// the WHOLE server - every listening socket, not just the
|
||||
// console - stops accepting anything new. This is the actual
|
||||
// root cause of BARANTMail's own multi-day-recurring "all ports
|
||||
// simultaneously stop accepting connections" incidents - nothing
|
||||
// to do with DNS/SPF lookup timing, which was a real, separate,
|
||||
// worthwhile fix but not this bug. recursive_mutex lets the same
|
||||
// thread re-enter safely (identical behavior to std::mutex for
|
||||
// every other thread) without changing the locking design at all.
|
||||
std::recursive_mutex lock;
|
||||
|
||||
public:
|
||||
///
|
||||
|
||||
@@ -79,51 +79,90 @@ namespace core {
|
||||
CRYPTO_set_locking_callback((void (*)(int, int, const char *, int))lock_callback);
|
||||
SSLeay_add_ssl_algorithms();
|
||||
RAND_load_file("/dev/hwrng", 1024);
|
||||
if(!(ctx = SSL_CTX_new(SSLv23_server_method())))
|
||||
throw coreutils::Exception("Error while setting server method SSLv23.");
|
||||
SSL_CTX_set_mode(ctx, SSL_MODE_RELEASE_BUFFERS | SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
|
||||
SSL_CTX_set_options(ctx, SSL_OP_NO_TICKET);
|
||||
SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_SERVER);
|
||||
// SSL_CTX_set_generate_session_id(ctx, generate_session_id);
|
||||
SSL_CTX_set_cipher_list(ctx, "ECDH-ECDSA-AES256-GCM-SHA384:DHE-DSS-AES256-GCM-SHA384:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA256:DHE-RSA-AES256-SHA:DHE-DSS-AES256-SHA:AES256-GCM-SHA384:AES256-SHA256:AES256-SHA:AES128-GCM-SHA256:AES128-SHA256:AES128-SHA");
|
||||
// _chain_file (not _file) - loads every certificate in the file,
|
||||
// not just the first. A real Let's Encrypt fullchain.pem is the
|
||||
// leaf cert followed by the intermediate CA cert concatenated in
|
||||
// one file specifically so a server can hand both to the client
|
||||
// in one shot; SSL_CTX_use_certificate_file() only ever reads the
|
||||
// first PEM block, silently dropping the intermediate - confirmed
|
||||
// via a real handshake against this exact fullchain.pem: clients
|
||||
// got "unable to get local issuer certificate" because the server
|
||||
// only ever sent the leaf, never the intermediate needed to chain
|
||||
// up to a root the client already trusts.
|
||||
if(SSL_CTX_use_certificate_chain_file(ctx, tlsInfo->certificate.c_str()) <= 0)
|
||||
throw coreutils::Exception("Error looking up certificate.");
|
||||
if(SSL_CTX_use_PrivateKey_file(ctx, tlsInfo->key.c_str(), SSL_FILETYPE_PEM) < 0)
|
||||
throw coreutils::Exception("Error with private key.");
|
||||
if(SSL_CTX_check_private_key(ctx) != 1)
|
||||
throw coreutils::Exception("Private key does not match certificate.");
|
||||
// Client-certificate verification (mutual TLS) is opt-in, not the
|
||||
// default - a public-facing site (e.g. a real Let's Encrypt
|
||||
// server cert with no corresponding client CA of its own) has no
|
||||
// client certificate to hand out to ordinary visitors, so
|
||||
// requesting one via SSL_VERIFY_PEER would surface a "select a
|
||||
// certificate" prompt real browsers can't meaningfully answer.
|
||||
// Only enable it when a caCertificate is actually configured -
|
||||
// existing internal/mTLS use of this class (any config that DOES
|
||||
// set tls.caCertificate) keeps behaving exactly as before.
|
||||
if(tlsInfo->cACertificate.getLength() > 0) {
|
||||
SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);
|
||||
SSL_CTX_set_verify_depth(ctx, 1);
|
||||
if(!SSL_CTX_load_verify_locations(ctx, tlsInfo->cACertificate.c_str(), NULL))
|
||||
throw coreutils::Exception("Cannot verify locations.");
|
||||
SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(tlsInfo->cACertificate.c_str()));
|
||||
coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (client certificate required).";
|
||||
} else {
|
||||
coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (no client certificate required).";
|
||||
}
|
||||
ctx = buildContext(tlsInfo);
|
||||
}
|
||||
}
|
||||
|
||||
// The per-context cert/key/cipher/verify setup factored out of
|
||||
// createContext() above so addSNIContext() (below) can build
|
||||
// additional contexts - one per extra hostname - without repeating
|
||||
// the ONE-TIME, process-wide OpenSSL library init that stays in
|
||||
// createContext() itself (SSL_library_init(), the lockarray, RAND
|
||||
// seeding): that part must only ever run once per process, not once
|
||||
// per hostname.
|
||||
SSL_CTX *TLS::buildContext(TLSInfo *tlsInfo) {
|
||||
SSL_CTX *newCtx;
|
||||
if(!(newCtx = SSL_CTX_new(SSLv23_server_method())))
|
||||
throw coreutils::Exception("Error while setting server method SSLv23.");
|
||||
SSL_CTX_set_mode(newCtx, SSL_MODE_RELEASE_BUFFERS | SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
|
||||
SSL_CTX_set_options(newCtx, SSL_OP_NO_TICKET);
|
||||
SSL_CTX_set_session_cache_mode(newCtx, SSL_SESS_CACHE_SERVER);
|
||||
SSL_CTX_set_cipher_list(newCtx, "ECDH-ECDSA-AES256-GCM-SHA384:DHE-DSS-AES256-GCM-SHA384:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA256:DHE-RSA-AES256-SHA:DHE-DSS-AES256-SHA:AES256-GCM-SHA384:AES256-SHA256:AES256-SHA:AES128-GCM-SHA256:AES128-SHA256:AES128-SHA");
|
||||
// _chain_file (not _file) - loads every certificate in the file,
|
||||
// not just the first. A real Let's Encrypt fullchain.pem is the
|
||||
// leaf cert followed by the intermediate CA cert concatenated in
|
||||
// one file specifically so a server can hand both to the client
|
||||
// in one shot; SSL_CTX_use_certificate_file() only ever reads the
|
||||
// first PEM block, silently dropping the intermediate - confirmed
|
||||
// via a real handshake against this exact fullchain.pem: clients
|
||||
// got "unable to get local issuer certificate" because the server
|
||||
// only ever sent the leaf, never the intermediate needed to chain
|
||||
// up to a root the client already trusts.
|
||||
if(SSL_CTX_use_certificate_chain_file(newCtx, tlsInfo->certificate.c_str()) <= 0)
|
||||
throw coreutils::Exception("Error looking up certificate.");
|
||||
if(SSL_CTX_use_PrivateKey_file(newCtx, tlsInfo->key.c_str(), SSL_FILETYPE_PEM) < 0)
|
||||
throw coreutils::Exception("Error with private key.");
|
||||
if(SSL_CTX_check_private_key(newCtx) != 1)
|
||||
throw coreutils::Exception("Private key does not match certificate.");
|
||||
// Client-certificate verification (mutual TLS) is opt-in, not the
|
||||
// default - a public-facing site (e.g. a real Let's Encrypt
|
||||
// server cert with no corresponding client CA of its own) has no
|
||||
// client certificate to hand out to ordinary visitors, so
|
||||
// requesting one via SSL_VERIFY_PEER would surface a "select a
|
||||
// certificate" prompt real browsers can't meaningfully answer.
|
||||
// Only enable it when a caCertificate is actually configured -
|
||||
// existing internal/mTLS use of this class (any config that DOES
|
||||
// set tls.caCertificate) keeps behaving exactly as before.
|
||||
if(tlsInfo->cACertificate.getLength() > 0) {
|
||||
SSL_CTX_set_verify(newCtx, SSL_VERIFY_PEER, NULL);
|
||||
SSL_CTX_set_verify_depth(newCtx, 1);
|
||||
if(!SSL_CTX_load_verify_locations(newCtx, tlsInfo->cACertificate.c_str(), NULL))
|
||||
throw coreutils::Exception("Cannot verify locations.");
|
||||
SSL_CTX_set_client_CA_list(newCtx, SSL_load_client_CA_file(tlsInfo->cACertificate.c_str()));
|
||||
coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (client certificate required).";
|
||||
} else {
|
||||
coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (no client certificate required).";
|
||||
}
|
||||
return newCtx;
|
||||
}
|
||||
|
||||
int TLS::sniCallback(SSL *ssl, int *ad, void *arg) {
|
||||
TLS *self = (TLS *)arg;
|
||||
const char *name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
|
||||
if(name) {
|
||||
auto it = self->sniContexts.find(name);
|
||||
if(it != self->sniContexts.end())
|
||||
SSL_set_SSL_CTX(ssl, it->second);
|
||||
// No match (an unconfigured hostname, or a typo'd Host that still
|
||||
// resolves here via DNS) - fall through and keep whatever SSL_CTX
|
||||
// this SSL* already has, which is the base `ctx` SSL_new() was
|
||||
// constructed from. Exactly the same cert a pre-SNI client (or
|
||||
// one sending no SNI at all) already gets today.
|
||||
}
|
||||
return SSL_TLSEXT_ERR_OK;
|
||||
}
|
||||
|
||||
void TLS::addSNIContext(const std::string &hostname, TLSInfo *tlsInfo) {
|
||||
SSL_CTX *newCtx = buildContext(tlsInfo);
|
||||
sniContexts[hostname] = newCtx;
|
||||
if(!sniCallbackRegistered) {
|
||||
SSL_CTX_set_tlsext_servername_callback(ctx, sniCallback);
|
||||
SSL_CTX_set_tlsext_servername_arg(ctx, this);
|
||||
sniCallbackRegistered = true;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
void TLS::registerSocket(int fd) {
|
||||
int ret;
|
||||
|
||||
|
||||
@@ -4,6 +4,8 @@
|
||||
#include "ZString.h"
|
||||
#include "TLSInfo.h"
|
||||
#include <openssl/ssl.h>
|
||||
#include <map>
|
||||
#include <string>
|
||||
|
||||
namespace core {
|
||||
|
||||
@@ -46,6 +48,26 @@ namespace core {
|
||||
|
||||
void createContext(TLSInfo *tlsInfo);
|
||||
|
||||
///
|
||||
/// Registers a second (or third, etc.) certificate/key pair for a
|
||||
/// specific hostname on THIS SAME listening socket, via TLS's
|
||||
/// Server Name Indication (SNI) extension - lets one TCPServer
|
||||
/// terminate TLS for multiple unrelated domains, each with its own
|
||||
/// real certificate, without needing a separate listening port or
|
||||
/// process per domain. Must be called after createContext() (or the
|
||||
/// TLSInfo* constructor, which calls it) has already established the
|
||||
/// base `ctx` - that base context keeps serving as both the
|
||||
/// fallback for hostnames not registered here AND the cert offered
|
||||
/// to any client that sends no SNI hostname at all (a bare-IP
|
||||
/// request, or any TLS client old enough to predate SNI). Callers
|
||||
/// that never call this at all (every existing single-site
|
||||
/// TLSInfo* user - BARANTMail, Zephora, ClockPlanet, and a
|
||||
/// single-site APIServer config) see zero behavior change: the SNI
|
||||
/// callback is only ever registered on `ctx` the first time this is
|
||||
/// called, not unconditionally in createContext().
|
||||
///
|
||||
void addSNIContext(const std::string &hostname, TLSInfo *tlsInfo);
|
||||
|
||||
// A never-configured TLS object (either the listening TCPServer
|
||||
// itself when constructed with tlsInfo=NULL, or a TCPSession spawned
|
||||
// from one via TLS(SSL_CTX*)) must default to a real NULL here, not
|
||||
@@ -79,6 +101,28 @@ namespace core {
|
||||
bool initialized = false;
|
||||
SSL *ssl;
|
||||
|
||||
// The actual cert/key-loading + cipher/session-cache setup shared
|
||||
// by both the base context (createContext(), unchanged) and every
|
||||
// additional per-hostname context addSNIContext() builds - factored
|
||||
// out so there's exactly one place that does this, not two copies
|
||||
// drifting apart.
|
||||
SSL_CTX *buildContext(TLSInfo *tlsInfo);
|
||||
|
||||
// Owns every SSL_CTX* built by addSNIContext() (the base `ctx` is
|
||||
// owned/freed the same way it always was, untouched by this map).
|
||||
// Keyed by hostname exactly as configured - matched case-
|
||||
// insensitively against the ClientHello's SNI value in the
|
||||
// callback (see TLS.cpp).
|
||||
std::map<std::string, SSL_CTX *> sniContexts;
|
||||
bool sniCallbackRegistered = false;
|
||||
|
||||
// OpenSSL's own SSL_CTX_set_tlsext_servername_callback signature -
|
||||
// a static member (not a free function) so it can reach the owning
|
||||
// TLS instance's private sniContexts via the `arg` parameter
|
||||
// (SSL_CTX_set_tlsext_servername_arg(ctx, this), set alongside the
|
||||
// callback itself in addSNIContext()).
|
||||
static int sniCallback(SSL *ssl, int *ad, void *arg);
|
||||
|
||||
};
|
||||
|
||||
}
|
||||
|
||||
+15
@@ -1,6 +1,7 @@
|
||||
#include "Thread.h"
|
||||
#include "EPoll.h"
|
||||
#include "TCPServer.h"
|
||||
#include "INotify.h"
|
||||
#include <sys/epoll.h>
|
||||
|
||||
namespace core {
|
||||
@@ -153,6 +154,20 @@ namespace core {
|
||||
rawSocket->shutDown = true;
|
||||
delete rawSocket;
|
||||
}
|
||||
} else if(INotify::isKnownINotify(rawPtr)) {
|
||||
// A third category of long-lived, non-TCPSession Socket -
|
||||
// see INotify::isKnownINotify()'s own doc comment for the
|
||||
// real, confirmed-live bug this fixes: every INotify event
|
||||
// (e.g. BARANTMail's mail-queue delivery watch) previously
|
||||
// fell all the way through to the "unrecognized" branch
|
||||
// below and was silently dropped forever.
|
||||
Socket *rawSocket = (Socket *)rawPtr;
|
||||
if(rawSocket->eventReceived(events[ix], ++ePoll.eventId)) {
|
||||
// keep alive
|
||||
} else {
|
||||
rawSocket->shutDown = true;
|
||||
delete rawSocket;
|
||||
}
|
||||
} else {
|
||||
// Neither registry recognizes this pointer - almost
|
||||
// certainly a stale/duplicate epoll event for a
|
||||
|
||||
Reference in new issue
Block a user