From: Alex Rousskov Date: Tue, 26 Apr 2022 15:11:44 +0000 (+0000) Subject: sslcrtd_program: Use (more) Squid-wide APIs (#1021) X-Git-Tag: SQUID_6_0_1~204 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=0efecd3284dbeb24436e22f8273b698a985bdc38;p=thirdparty%2Fsquid.git sslcrtd_program: Use (more) Squid-wide APIs (#1021) This switch allows us to use Squid-native debugging and error handling code in ssl/libsslutil.la. Such use is necessary to fix and enhance certificate handling code currently located in that library. For now, we only did straightforward adjustments like fixing parseBytesOptionValue() and Ssl::CrtdMessage::parseRequest() error detection/handling. More serious changes deserve dedicated PRs. This change also converts the remaining bare std::runtime_error uses in runtime Squid code and almost all std::cerr uses in sslcrtd_program: * std::runtime_error: TextException provides the source code location of the thrower and will be enhanced along with Squid improvements. * std::cerr: debugs() provides much better runtime logging, on many levels. The usage() dump still uses std::cerr because debugs() decorations are not useful in that special case and because, IMO, that case should be using std::cout instead (it is not reporting an error). Removing STL APIs (that have Squid-native alternatives) from the old helper code also reduces the temptation to use wrong APIs in new code, especially when authors are not familiar with Squid conventions/plans. Also replaced sslcrtd_program's `Here` hack with SourceLocation. Also report all started helpers (at debug level 2) because a successful helper start is a significant event worth reporting (when level-2 debugging is enabled) across all helpers. --- diff --git a/src/debug/debug.cc b/src/debug/debug.cc index bc466a3196..b30a640d6f 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -397,6 +397,8 @@ Debug::NameThisHelper(const char * const name) SettleStderr(); SettleSyslog(); + + debugs(84, 2, "starting " << name << " with PID " << getpid()); } void diff --git a/src/security/cert_generators/file/Makefile.am b/src/security/cert_generators/file/Makefile.am index aab4174a5e..168cf2030c 100644 --- a/src/security/cert_generators/file/Makefile.am +++ b/src/security/cert_generators/file/Makefile.am @@ -25,6 +25,11 @@ security_file_certgen_SOURCES = \ security_file_certgen_LDADD = \ $(top_builddir)/src/ssl/libsslutil.la \ + $(top_builddir)/src/sbuf/libsbuf.la \ + $(top_builddir)/src/debug/libdebug.la \ + $(top_builddir)/src/comm/libminimal.la \ + $(top_builddir)/src/mem/libminimal.la \ + $(top_builddir)/src/base/libbase.la \ $(top_builddir)/src/time/libtime.la \ $(SSLLIB) \ $(COMPAT_LIB) diff --git a/src/security/cert_generators/file/certificate_db.cc b/src/security/cert_generators/file/certificate_db.cc index ac8c3b25de..8af1b6bc15 100644 --- a/src/security/cert_generators/file/certificate_db.cc +++ b/src/security/cert_generators/file/certificate_db.cc @@ -8,6 +8,8 @@ #include "squid.h" #include "base/HardFun.h" +#include "base/TextException.h" +#include "sbuf/Stream.h" #include "security/cert_generators/file/certificate_db.h" #include @@ -53,7 +55,7 @@ void Ssl::Lock::lock() fd = open(filename.c_str(), O_RDWR); if (fd == -1) #endif - throw std::runtime_error("Failed to open file " + filename); + throw TextException(ToSBuf("Failed to open file ", filename), Here()); #if _SQUID_WINDOWS_ if (!LockFile(hFile, 0, 0, 1, 0)) @@ -62,7 +64,7 @@ void Ssl::Lock::lock() #else if (flock(fd, LOCK_EX) != 0) #endif - throw std::runtime_error("Failed to get a lock of " + filename); + throw TextException(ToSBuf("Failed to get a lock of ", filename), Here()); } void Ssl::Lock::unlock() @@ -85,7 +87,7 @@ void Ssl::Lock::unlock() } #endif else - throw std::runtime_error("Lock is already unlocked for " + filename); + throw TextException(ToSBuf("Lock is already unlocked for ", filename), Here()); } Ssl::Lock::~Lock() @@ -94,8 +96,10 @@ Ssl::Lock::~Lock() unlock(); } -Ssl::Locker::Locker(Lock &aLock, const char *aFileName, int aLineNo): - weLocked(false), lock(aLock), fileName(aFileName), lineNo(aLineNo) +Ssl::Locker::Locker(Lock &aLock, const SourceLocation &aCaller): + weLocked(false), + lock(aLock), + caller(aCaller) { if (!lock.locked()) { lock.lock(); @@ -105,7 +109,6 @@ Ssl::Locker::Locker(Lock &aLock, const char *aFileName, int aLineNo): Ssl::Locker::~Locker() { - (void)lineNo; // lineNo is unused but some find it helpful in debugging if (weLocked) lock.unlock(); } @@ -260,13 +263,13 @@ Ssl::CertificateDb::CertificateDb(std::string const & aDb_path, size_t aMax_db_s bool Ssl::CertificateDb::find(std::string const &key, const Security::CertPointer &expectedOrig, Security::CertPointer &cert, Security::PrivateKeyPointer &pkey) { - const Locker locker(dbLock, Here); + const Locker locker(dbLock, Here()); load(); return pure_find(key, expectedOrig, cert, pkey); } bool Ssl::CertificateDb::purgeCert(std::string const & key) { - const Locker locker(dbLock, Here); + const Locker locker(dbLock, Here()); load(); if (!db) return false; @@ -281,7 +284,7 @@ bool Ssl::CertificateDb::purgeCert(std::string const & key) { bool Ssl::CertificateDb::addCertAndPrivateKey(std::string const &useKey, const Security::CertPointer &cert, const Security::PrivateKeyPointer &pkey, const Security::CertPointer &orig) { - const Locker locker(dbLock, Here); + const Locker locker(dbLock, Here()); load(); if (!db || !cert || !pkey) return false; @@ -365,25 +368,25 @@ Ssl::CertificateDb::addCertAndPrivateKey(std::string const &useKey, const Securi void Ssl::CertificateDb::Create(std::string const & db_path) { if (db_path == "") - throw std::runtime_error("Path to db is empty"); + throw TextException("Path to db is empty", Here()); std::string db_full(db_path + "/" + db_file); std::string cert_full(db_path + "/" + cert_dir); std::string size_full(db_path + "/" + size_file); if (mkdir(db_path.c_str(), 0777)) - throw std::runtime_error("Cannot create " + db_path); + throw TextException(ToSBuf("Cannot create ", db_path), Here()); if (mkdir(cert_full.c_str(), 0777)) - throw std::runtime_error("Cannot create " + cert_full); + throw TextException(ToSBuf("Cannot create ", cert_full), Here()); std::ofstream size(size_full.c_str()); if (size) size << 0; else - throw std::runtime_error("Cannot open " + size_full + " to open"); + throw TextException(ToSBuf("Cannot open ", size_full, " to open"), Here()); std::ofstream db(db_full.c_str()); if (!db) - throw std::runtime_error("Cannot open " + db_full + " to open"); + throw TextException(ToSBuf("Cannot open ", db_full, " to open"), Here()); } void @@ -475,7 +478,7 @@ size_t Ssl::CertificateDb::readSize() { void Ssl::CertificateDb::writeSize(size_t db_size) { std::ofstream ofstr(size_full.c_str()); if (!ofstr) - throw std::runtime_error("cannot write \"" + size_full + "\" file"); + throw TextException(ToSBuf("cannot write \"", size_full, "\" file"), Here()); ofstr << db_size; } @@ -494,7 +497,7 @@ void Ssl::CertificateDb::load() { // Load db from file. Ssl::BIO_Pointer in(BIO_new(BIO_s_file())); if (!in || BIO_read_filename(in.get(), db_full.c_str()) <= 0) - throw std::runtime_error("Uninitialized SSL certificate database directory: " + db_path + ". To initialize, run \"security_file_certgen -c -s " + db_path + "\"."); + throw TextException(ToSBuf("Uninitialized SSL certificate database directory: ", db_path, ". To initialize, run \"security_file_certgen -c -s ", db_path, "\"."), Here()); bool corrupt = false; Ssl::TXT_DB_Pointer temp_db(TXT_DB_read(in.get(), cnlNumber)); @@ -509,22 +512,22 @@ void Ssl::CertificateDb::load() { corrupt = true; if (corrupt) - throw std::runtime_error("The SSL certificate database " + db_path + " is corrupted. Please rebuild"); + throw TextException(ToSBuf("The SSL certificate database ", db_path, " is corrupted. Please rebuild"), Here()); db.reset(temp_db.release()); } void Ssl::CertificateDb::save() { if (!db) - throw std::runtime_error("The certificates database is not loaded");; + throw TextException("The certificates database is not loaded", Here()); // To save the db to file, create a new BIO with BIO file methods. Ssl::BIO_Pointer out(BIO_new(BIO_s_file())); if (!out || !BIO_write_filename(out.get(), const_cast(db_full.c_str()))) - throw std::runtime_error("Failed to initialize " + db_full + " file for writing");; + throw TextException(ToSBuf("Failed to initialize ", db_full, " file for writing"), Here()); if (TXT_DB_write(out.get(), db.get()) < 0) - throw std::runtime_error("Failed to write " + db_full + " file"); + throw TextException(ToSBuf("Failed to write ", db_full, " file"), Here()); } // Normally defined in defines.h file @@ -535,7 +538,7 @@ void Ssl::CertificateDb::deleteRow(const char **row, int rowIndex) { subSize(filename); int ret = remove(filename.c_str()); if (ret < 0 && errno != ENOENT) - throw std::runtime_error("Failed to remove certificate file " + filename + " from db"); + throw TextException(ToSBuf("Failed to remove certificate file ", filename, " from db"), Here()); } bool Ssl::CertificateDb::deleteInvalidCertificate() { diff --git a/src/security/cert_generators/file/certificate_db.h b/src/security/cert_generators/file/certificate_db.h index 2d6ed1db99..9733fee4e9 100644 --- a/src/security/cert_generators/file/certificate_db.h +++ b/src/security/cert_generators/file/certificate_db.h @@ -9,6 +9,7 @@ #ifndef SQUID_SSL_CERTIFICATE_DB_H #define SQUID_SSL_CERTIFICATE_DB_H +#include "base/Here.h" #include "ssl/gadgets.h" #include @@ -39,18 +40,16 @@ class Locker { public: /// locks the lock if the lock was unlocked - Locker(Lock &lock, const char *aFileName, int lineNo); + Locker(Lock &, const SourceLocation &); /// unlocks the lock if it was locked by us ~Locker(); private: bool weLocked; ///< whether we locked the lock Lock &lock; ///< the lock we are operating on - const std::string fileName; ///< where the lock was needed - const int lineNo; ///< where the lock was needed -}; -/// convenience macro to pass source code location to Locker and others -#define Here __FILE__, __LINE__ + /// where the lock was needed (currently not reported anywhere) + const SourceLocation caller; +}; /** * Database class for storing SSL certificates and their private keys. diff --git a/src/security/cert_generators/file/security_file_certgen.cc b/src/security/cert_generators/file/security_file_certgen.cc index a779150559..a2722c42e5 100644 --- a/src/security/cert_generators/file/security_file_certgen.cc +++ b/src/security/cert_generators/file/security_file_certgen.cc @@ -7,7 +7,10 @@ */ #include "squid.h" +#include "base/TextException.h" +#include "debug/Stream.h" #include "helper/protocol_defines.h" +#include "sbuf/Stream.h" #include "security/cert_generators/file/certificate_db.h" #include "ssl/crtd_message.h" #include "time/gadgets.h" @@ -95,13 +98,13 @@ static size_t parseBytesUnits(const char * unit) if (!strncasecmp(unit, B_GBYTES_STR, strlen(B_GBYTES_STR))) return 1 << 30; - std::cerr << "WARNING: Unknown bytes unit '" << unit << "'" << std::endl; - - return 0; + throw TextException(ToSBuf("Unknown bytes unit: ", unit), Here()); } -/// Parse uninterrapted string of bytes value. It looks like "4MB". -static bool parseBytesOptionValue(size_t * bptr, char const * value) +/// Parse the number of bytes given as value (e.g., 4MB). +/// \param name the name of the option being parsed +static size_t +parseBytesOptionValue(const char * const name, const char * const value) { // Find number from string beginning. char const * number_begin = value; @@ -111,22 +114,22 @@ static bool parseBytesOptionValue(size_t * bptr, char const * value) ++number_end; } + if (number_end <= number_begin) + throw TextException(ToSBuf("expecting a decimal number at the beginning of ", name, " value but got: ", value), Here()); + std::string number(number_begin, number_end - number_begin); std::istringstream in(number); - int d = 0; - if (!(in >> d)) - return false; + size_t base = 0; + if (!(in >> base) || !in.eof()) + throw TextException(ToSBuf("unsupported integer part of ", name, " value: ", number), Here()); - int m; - if ((m = parseBytesUnits(number_end)) == 0) { - return false; - } + const auto multiplier = parseBytesUnits(number_end); + static_assert(std::is_unsigned::value, "no signed overflows"); + const auto product = multiplier * base; + if (base && multiplier != product / base) + throw TextException(ToSBuf(name, " size too large: ", value), Here()); - *bptr = static_cast(m * d); - if (static_cast(*bptr * 2) != m * d * 2) - return false; - - return true; + return product; } /// Print help using response code. @@ -169,9 +172,7 @@ static void usage() static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string const & db_path, size_t max_db_size, size_t fs_block_size) { Ssl::CertificateProperties certProperties; - std::string error; - if (!request_message.parseRequest(certProperties, error)) - throw std::runtime_error("Error while parsing the crtd request: " + error); + request_message.parseRequest(certProperties); // TODO: create a DB object only once, instead re-allocating here on every call. std::unique_ptr db; @@ -188,14 +189,15 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co if (db) db->find(certKey, certProperties.mimicCert, cert, pkey); - } catch (std::runtime_error &err) { + } catch (...) { dbFailed = true; - error = err.what(); + debugs(83, DBG_IMPORTANT, "ERROR: Database search failure: " << CurrentException << + Debug::Extra << "database location: " << db_path); } if (!cert || !pkey) { if (!Ssl::generateSslCertificate(cert, pkey, certProperties)) - throw std::runtime_error("Cannot create ssl certificate or private key."); + throw TextException("Cannot create ssl certificate or private key.", Here()); try { /* XXX: this !dbFailed condition prevents the helper fixing DB issues @@ -209,20 +211,18 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co object lifecycle and formally altering the helper behaviour. */ if (!dbFailed && db && !db->addCertAndPrivateKey(certKey, cert, pkey, certProperties.mimicCert)) - throw std::runtime_error("Cannot add certificate to db."); + throw TextException("Cannot add certificate to db.", Here()); - } catch (const std::runtime_error &err) { + } catch (...) { dbFailed = true; - error = err.what(); + debugs(83, DBG_IMPORTANT, "ERROR: Database update failure: " << CurrentException << + Debug::Extra << "database location: " << db_path); } } - if (dbFailed) - std::cerr << "security_file_certgen helper database '" << db_path << "' failed: " << error << std::endl; - std::string bufferToWrite; if (!Ssl::writeCertAndPrivateKeyToMemory(cert, pkey, bufferToWrite)) - throw std::runtime_error("Cannot write ssl certificate or/and private key to memory."); + throw TextException("Cannot write ssl certificate or/and private key to memory.", Here()); Ssl::CrtdMessage response_message(Ssl::CrtdMessage::REPLY); response_message.setCode("OK"); @@ -238,6 +238,8 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co int main(int argc, char *argv[]) { try { + Debug::NameThisHelper("sslcrtd_program"); + size_t max_db_size = 0; size_t fs_block_size = 0; int8_t c; @@ -250,9 +252,7 @@ int main(int argc, char *argv[]) debug_enabled = 1; break; case 'b': - if (!parseBytesOptionValue(&fs_block_size, optarg)) { - throw std::runtime_error("Error when parsing -b options value"); - } + fs_block_size = parseBytesOptionValue("-b", optarg); break; case 's': db_path = optarg; @@ -260,11 +260,9 @@ int main(int argc, char *argv[]) case 'M': // use of -M without -s is probably an admin mistake, so make it an error if (db_path.empty()) { - throw std::runtime_error("Error -M option requires an -s parameter be set first."); - } - if (!parseBytesOptionValue(&max_db_size, optarg)) { - throw std::runtime_error("Error when parsing -M options value"); + throw TextException("Error -M option requires an -s parameter be set first.", Here()); } + max_db_size = parseBytesOptionValue("-M", optarg); break; case 'v': std::cout << "security_file_certgen version " << VERSION << std::endl; @@ -283,12 +281,12 @@ int main(int argc, char *argv[]) // when -s is used, -M is required if (!db_path.empty() && max_db_size == 0) - throw std::runtime_error("security_file_certgen -s requires an -M parameter"); + throw TextException("security_file_certgen -s requires an -M parameter", Here()); if (create_new_db) { // when -c is used, -s is required (implying also -M, which is checked above) if (db_path.empty()) - throw std::runtime_error("security_file_certgen is missing the required parameter. There should be -s and -M parameters when -c is used."); + throw TextException("security_file_certgen is missing the required parameter. There should be -s and -M parameters when -c is used.", Here()); std::cout << "Initialization SSL db..." << std::endl; Ssl::CertificateDb::Create(db_path); @@ -329,16 +327,16 @@ int main(int argc, char *argv[]) } if (parse_result == Ssl::CrtdMessage::ERROR) { - throw std::runtime_error("Cannot parse request message."); + throw TextException("Cannot parse request message.", Here()); } else if (request_message.getCode() == Ssl::CrtdMessage::code_new_certificate) { processNewRequest(request_message, db_path, max_db_size, fs_block_size); } else { - throw std::runtime_error("Unknown request code: \"" + request_message.getCode() + "\"."); + throw TextException(ToSBuf("Unknown request code: \"", request_message.getCode(), "\"."), Here()); } std::cout.flush(); } - } catch (std::runtime_error & error) { - std::cerr << argv[0] << ": " << error.what() << std::endl; + } catch (...) { + debugs(83, DBG_CRITICAL, "FATAL: Cannot generate certificates: " << CurrentException); return EXIT_FAILURE; } return EXIT_SUCCESS; diff --git a/src/ssl/crtd_message.cc b/src/ssl/crtd_message.cc index e96e35a0fe..df7068e9bb 100644 --- a/src/ssl/crtd_message.cc +++ b/src/ssl/crtd_message.cc @@ -7,6 +7,8 @@ */ #include "squid.h" +#include "base/TextException.h" +#include "sbuf/Stream.h" #include "ssl/crtd_message.h" #include "ssl/gadgets.h" @@ -175,15 +177,15 @@ void Ssl::CrtdMessage::composeBody(CrtdMessage::BodyParams const & map, std::str body += '\n' + other_part; } -bool Ssl::CrtdMessage::parseRequest(Ssl::CertificateProperties &certProperties, std::string &error) +void +Ssl::CrtdMessage::parseRequest(CertificateProperties &certProperties) { Ssl::CrtdMessage::BodyParams map; std::string certs_part; parseBody(map, certs_part); Ssl::CrtdMessage::BodyParams::iterator i = map.find(Ssl::CrtdMessage::param_host); if (i == map.end()) { - error = "Cannot find \"host\" parameter in request message"; - return false; + throw TextException("Cannot find \"host\" parameter in request message", Here()); } certProperties.commonName = i->second; @@ -206,9 +208,7 @@ bool Ssl::CrtdMessage::parseRequest(Ssl::CertificateProperties &certProperties, i = map.find(Ssl::CrtdMessage::param_Sign); if (i != map.end()) { if ((certProperties.signAlgorithm = Ssl::certSignAlgorithmId(i->second.c_str())) == Ssl::algSignEnd) { - error = "Wrong signing algorithm: "; - error += i->second; - return false; + throw TextException(ToSBuf("Wrong signing algorithm: ", i->second), Here()); } } else certProperties.signAlgorithm = Ssl::algSignTrusted; @@ -216,14 +216,11 @@ bool Ssl::CrtdMessage::parseRequest(Ssl::CertificateProperties &certProperties, i = map.find(Ssl::CrtdMessage::param_SignHash); const char *signHashName = i != map.end() ? i->second.c_str() : SQUID_SSL_SIGN_HASH_IF_NONE; if (!(certProperties.signHash = EVP_get_digestbyname(signHashName))) { - error = "Wrong signing hash: "; - error += signHashName; - return false; + throw TextException(ToSBuf("Wrong signing hash: ", signHashName), Here()); } if (!Ssl::readCertAndPrivateKeyFromMemory(certProperties.signWithX509, certProperties.signWithPkey, certs_part.c_str())) { - error = "Broken signing certificate!"; - return false; + throw TextException("Broken signing certificate!", Here()); } static const std::string CERT_BEGIN_STR("-----BEGIN CERTIFICATE"); @@ -233,7 +230,6 @@ bool Ssl::CrtdMessage::parseRequest(Ssl::CertificateProperties &certProperties, if ((pos= certs_part.find(CERT_BEGIN_STR, pos)) != std::string::npos) Ssl::readCertFromMemory(certProperties.mimicCert, certs_part.c_str() + pos); } - return true; } void Ssl::CrtdMessage::composeRequest(Ssl::CertificateProperties const &certProperties) @@ -253,10 +249,10 @@ void Ssl::CrtdMessage::composeRequest(Ssl::CertificateProperties const &certProp std::string certsPart; if (!Ssl::writeCertAndPrivateKeyToMemory(certProperties.signWithX509, certProperties.signWithPkey, certsPart)) - throw std::runtime_error("Ssl::writeCertAndPrivateKeyToMemory()"); + throw TextException("Ssl::writeCertAndPrivateKeyToMemory()", Here()); if (certProperties.mimicCert.get()) { if (!Ssl::appendCertToMemory(certProperties.mimicCert, certsPart)) - throw std::runtime_error("Ssl::appendCertToMemory()"); + throw TextException("Ssl::appendCertToMemory()", Here()); } body += "\n" + certsPart; } diff --git a/src/ssl/crtd_message.h b/src/ssl/crtd_message.h index 6e9bfe2549..77284bef65 100644 --- a/src/ssl/crtd_message.h +++ b/src/ssl/crtd_message.h @@ -69,7 +69,7 @@ public: void composeBody(BodyParams const & map, std::string const & other_part); /// orchestrates entire request parsing - bool parseRequest(Ssl::CertificateProperties &, std::string &error); + void parseRequest(CertificateProperties &); void composeRequest(Ssl::CertificateProperties const &); // throws /// String code for "new_certificate" messages