]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
sslcrtd_program: Use (more) Squid-wide APIs (#1021)
authorAlex Rousskov <rousskov@measurement-factory.com>
Tue, 26 Apr 2022 15:11:44 +0000 (15:11 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Tue, 26 Apr 2022 16:34:50 +0000 (16:34 +0000)
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.

src/debug/debug.cc
src/security/cert_generators/file/Makefile.am
src/security/cert_generators/file/certificate_db.cc
src/security/cert_generators/file/certificate_db.h
src/security/cert_generators/file/security_file_certgen.cc
src/ssl/crtd_message.cc
src/ssl/crtd_message.h

index bc466a3196ae87852eabdfb7d63ecbb1ae58d416..b30a640d6ffa69d25fb48cb1537285505fabe75f 100644 (file)
@@ -397,6 +397,8 @@ Debug::NameThisHelper(const char * const name)
 
     SettleStderr();
     SettleSyslog();
+
+    debugs(84, 2, "starting " << name << " with PID " << getpid());
 }
 
 void
index aab4174a5edfaffd16197b75a04c208306b1a078..168cf2030c3bbf8b14612cc193e0d8554ef02858 100644 (file)
@@ -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)
index ac8c3b25deb6866435d79b59f55ad065bc56e210..8af1b6bc152ca3fdc2059a59cebb96ffbeded999 100644 (file)
@@ -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 <cerrno>
@@ -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<char *>(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() {
index 2d6ed1db9912731b2498cc535be9fb178c85c5a9..9733fee4e9a7584621bf162bca23e52de11a6383 100644 (file)
@@ -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 <string>
@@ -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.
index a7791505592e72af744eee294875aa12bd885cd5..a2722c42e5155f2b0302e6b97e3547cd3f3bb22a 100644 (file)
@@ -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 <integer><unit> 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<decltype(multiplier * base)>::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<size_t>(m * d);
-    if (static_cast<long>(*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<Ssl::CertificateDb> 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;
index e96e35a0fe2be940e2de5b63d4fc921fb4bd5f0a..df7068e9bb2504f8ea7b0d262d7ca00b1169c634 100644 (file)
@@ -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;
 }
index 6e9bfe2549db983d8a16fb5310b9afbf7be027d0..77284bef65959edb4c6680287bd220590a2e676a 100644 (file)
@@ -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