From 505f9427c55a365b464f1b204880df1d3ccccb7f Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Mon, 8 Jan 2018 03:31:16 +1300 Subject: [PATCH] Bug 4631: security_file_certgen helper without disk cache (#95) * disable the certificate DB disk cache if -s and -M command line options are omitted. E.g. with this you can change squid.conf from: sslcrtd_program security_file_certgen -s /var/lib/ssl_db -M 32MB ...to... sslcrtd_program security_file_certgen ...and it will operate without the disk cache, generating certs fresh every time. * Remove Ssl::CertificateDb::IsEnabledDiskStore() Make the CertificateDb temporary objects dynamically allocated instead. * Do command line checks in main() not the CertificateDb object. This avoids a risky constructor exception and simplifies validity testing of parameters. * Update man(8) documentation The helper version is now 1.1. A minor version bump since it is being kept compatible with installations using 1.0 properly but new feature available. Also simplify the command line SYNOPSIS and incomplete mention of sslcrtd_* squid.conf directives. --- .../cert_generators/file/certificate_db.cc | 13 +--- .../cert_generators/file/certificate_db.h | 4 - .../file/security_file_certgen.8.in | 71 ++++++++++++------ .../file/security_file_certgen.cc | 75 ++++++++++++------- 4 files changed, 99 insertions(+), 64 deletions(-) diff --git a/src/security/cert_generators/file/certificate_db.cc b/src/security/cert_generators/file/certificate_db.cc index 84abc6aad8..a3ed84dab2 100644 --- a/src/security/cert_generators/file/certificate_db.cc +++ b/src/security/cert_generators/file/certificate_db.cc @@ -256,13 +256,8 @@ Ssl::CertificateDb::CertificateDb(std::string const & aDb_path, size_t aMax_db_s size_full(aDb_path + "/" + size_file), max_db_size(aMax_db_size), fs_block_size((aFs_block_size ? aFs_block_size : 2048)), - dbLock(db_full), - enabled_disk_store(true) { - if (db_path.empty() && !max_db_size) - enabled_disk_store = false; - else if ((db_path.empty() && max_db_size) || (!db_path.empty() && !max_db_size)) - throw std::runtime_error("security_file_certgen is missing the required parameter. There should be -s and -M parameters together."); -} + dbLock(db_full) +{} bool Ssl::CertificateDb::find(std::string const &key, const Security::CertPointer &expectedOrig, Security::CertPointer &cert, Security::PrivateKeyPointer &pkey) @@ -632,10 +627,6 @@ bool Ssl::CertificateDb::hasRows() const return true; } -bool Ssl::CertificateDb::IsEnabledDiskStore() const { - return enabled_disk_store; -} - bool Ssl::CertificateDb::WriteEntry(const std::string &filename, const Security::CertPointer &cert, const Security::PrivateKeyPointer &pkey, const Security::CertPointer &orig) { diff --git a/src/security/cert_generators/file/certificate_db.h b/src/security/cert_generators/file/certificate_db.h index 1989b77236..f58181485e 100644 --- a/src/security/cert_generators/file/certificate_db.h +++ b/src/security/cert_generators/file/certificate_db.h @@ -103,8 +103,6 @@ public: /// Save certificate to disk. bool addCertAndPrivateKey(std::string const & useKey, const Security::CertPointer & cert, const Security::PrivateKeyPointer & pkey, const Security::CertPointer &orig); - bool IsEnabledDiskStore() const; ///< Check enabled of dist store. - /// Create and initialize a database under the db_path static void Create(std::string const & db_path); /// Check the database stored under the db_path. @@ -187,8 +185,6 @@ private: const size_t max_db_size; ///< Max size of db. const size_t fs_block_size; ///< File system block size. mutable Lock dbLock; ///< protects the database file - - bool enabled_disk_store; ///< The storage on the disk is enabled. }; } // namespace Ssl diff --git a/src/security/cert_generators/file/security_file_certgen.8.in b/src/security/cert_generators/file/security_file_certgen.8.in index 913492f1b0..3799c4c1f8 100644 --- a/src/security/cert_generators/file/security_file_certgen.8.in +++ b/src/security/cert_generators/file/security_file_certgen.8.in @@ -3,48 +3,49 @@ .SH NAME security_file_certgen \- SSL certificate generator for Squid. .PP -Version 1.0 +Version 1.1 . .SH SYNOPSIS .if !'po4a'hide' .B security_file_certgen -.if !'po4a'hide' .B [\-dhv] -.br -.if !'po4a'hide' .B security_file_certgen -.if !'po4a'hide' .B "[\-d] \-s " +.if !'po4a'hide' .B "[\-cdhv] [\-s " directory -.if !'po4a'hide' .B "[\-M " +.if !'po4a'hide' .B "\-M " size .if !'po4a'hide' .B "] [\-b " fs_block_size .if !'po4a'hide' .B ] -.br -.if !'po4a'hide' .B security_file_certgen -.if !'po4a'hide' .B "[\-d] \-c \-s " -directory . .SH DESCRIPTION .B security_file_certgen is an installed binary. .PP Because the generation and signing of SSL certificates takes time -Squid must use external process to handle the work. +Squid can use this helper as an external process to handle the work. . -This process generates new SSL certificates and uses a disk cache of certificates -to improve response times on repeated requests. Communication occurs via TCP sockets bound to the loopback interface. . +This helper can use a disk cache of certificates to improve response +times on repeated requests. It can also operate without a cache, +generating new certificates on every request. +. .SH OPTIONS .if !'po4a'hide' .TP 12 .if !'po4a'hide' .B \-b fs_block_size File system block size in bytes. Needed for processing natural size of certificate on disk. -Default value is 2048 bytes. The following suffixes are accepted: B, KB, MB, GB. -When no suffix is set, B is assumed. +Default value is 2048 bytes. +The following suffixes are accepted: +.B B, KB, MB, GB. +When no suffix is set, +.B B +is assumed. . .if !'po4a'hide' .TP .if !'po4a'hide' .B \-c Initialize the SSL storage database and exit. Requires the .B \-s -option to determine the storage location being created. +and +.B \-M +options to determine the storage location and size being created. . .if !'po4a'hide' .TP .if !'po4a'hide' .B \-d @@ -56,7 +57,9 @@ Display the binary help and command line syntax info using stderr. . .if !'po4a'hide' .TP .if !'po4a'hide' .B \-s directory -Directory path of SSL storage database. +Directory path of SSL storage database. Requires the +.B \-M +option. . .if !'po4a'hide' .TP .if !'po4a'hide' .B \-M size @@ -74,13 +77,13 @@ Display the binary version details using stderr. . .PP Certificates are stored in this database in signed form. -After any change to the signing CA in squid.conf be sure to erase and re-initialize the certificate database. +After any change to the signing CA in squid.conf be sure to erase and reinitialize the certificate database. . .PP .B Certificate chaining . .PP -The version 1.0 of this helper will not add chained intermediate CA certificates. +The versions 1.0 to 1.1 of this helper will not add chained intermediate CA certificates. The client must have a full chain of trust from the root CA all the way down to the end certificate generated by this program. . @@ -89,7 +92,7 @@ root and the intermediate public CA on the clients. . .SH CONFIGURATION .PP -Before this helper can be used the storage area for new certificates must be initialized manually. +Before this helper can be used with disk storage, the storage area for new certificates must be initialized manually. This is done from the command line using the .B \-c parameter. @@ -97,7 +100,7 @@ parameter. .PP For example: .if !'po4a'hide' .RS -.if !'po4a'hide' .B @DEFAULT_SSL_CRTD@ \-c \-s @DEFAULT_SSL_DB_DIR@ +.if !'po4a'hide' .B @DEFAULT_SSL_CRTD@ \-c \-s @DEFAULT_SSL_DB_DIR@ \-M 4MB .if !'po4a'hide' .RE . .PP @@ -107,15 +110,22 @@ After any change to the signing CA in squid.conf be sure to erase and re-initial .PP For simple configuration the helper defaults can be used. Only HTTP listening port options are required to enable generation and set the signing CA certificate. -For Example: +. +.PP +For example: .if !'po4a'hide' .RS .if !'po4a'hide' .B http_port 3128 ssl-bump generate-host-certificates=on dynamic_cert_mem_cache_size=4MB cert=@SYSCONFDIR@/ssl_cert/example.com.pem .if !'po4a'hide' .RE . .PP -For more customized configuration the helper certificate storage directory location and size can be altered with the -.B sslcrtd_program +For more customized configuration, the helper certificate storage directory location and size can be altered with the +.B sslcrtd_program +configuration directive. The number of helper processes running can be configured with the +and +.B ssl_crtd_children configuration directive. +. +.PP For example: .if !'po4a'hide' .RS .if !'po4a'hide' .B sslcrtd_program @DEFAULT_SSL_CRTD@ \-s @DEFAULT_SSL_DB_DIR@ \-M 4MB @@ -123,6 +133,19 @@ For example: .if !'po4a'hide' .B sslcrtd_children 5 .if !'po4a'hide' .RE . +.PP +To operate without disk storage, the helper should be configured explicitly without the +.B \-s +and +.B \-M +parameters. +. +.PP +For example: +.if !'po4a'hide' .RS +.if !'po4a'hide' .B sslcrtd_program @DEFAULT_SSL_CRTD@ +.if !'po4a'hide' .RE +. .SH AUTHOR This program was written by .if !'po4a'hide' .I Christos Tsantilas diff --git a/src/security/cert_generators/file/security_file_certgen.cc b/src/security/cert_generators/file/security_file_certgen.cc index c1f3d0b808..6a14013007 100644 --- a/src/security/cert_generators/file/security_file_certgen.cc +++ b/src/security/cert_generators/file/security_file_certgen.cc @@ -184,7 +184,10 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co if (!request_message.parseRequest(certProperties, error)) throw std::runtime_error("Error while parsing the crtd request: " + error); - Ssl::CertificateDb db(db_path, max_db_size, fs_block_size); + // TODO: create a DB object only once, instead re-allocating here on every call. + std::unique_ptr db; + if (!db_path.empty()) + db.reset(new Ssl::CertificateDb(db_path, max_db_size, fs_block_size)); Security::CertPointer cert; Security::PrivateKeyPointer pkey; @@ -193,7 +196,9 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co bool dbFailed = false; try { - db.find(certKey, certProperties.mimicCert, cert, pkey); + if (db) + db->find(certKey, certProperties.mimicCert, cert, pkey); + } catch (std::runtime_error &err) { dbFailed = true; error = err.what(); @@ -203,16 +208,23 @@ static bool processNewRequest(Ssl::CrtdMessage & request_message, std::string co if (!Ssl::generateSslCertificate(cert, pkey, certProperties)) throw std::runtime_error("Cannot create ssl certificate or private key."); - if (!dbFailed && db.IsEnabledDiskStore()) { - try { - if (!db.addCertAndPrivateKey(certKey, cert, pkey, certProperties.mimicCert)) { - dbFailed = true; - error = "Cannot add certificate to db."; - } - } catch (const std::runtime_error &err) { - dbFailed = true; - error = err.what(); - } + try { + /* XXX: this !dbFailed condition prevents the helper fixing DB issues + by adding cleanly generated certs. Which is not consistent with other + data caches used by Squid - they purge broken entries and allow clean + entries to later try and fix the issue. + We leave it in place now only to avoid breaking existing installations + behaviour with version 1.x of the helper. + + TODO: remove the !dbFailed condition when fixing the CertificateDb + 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."); + + } catch (const std::runtime_error &err) { + dbFailed = true; + error = err.what(); } } @@ -257,6 +269,10 @@ int main(int argc, char *argv[]) db_path = optarg; break; 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"); } @@ -276,29 +292,38 @@ 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"); + 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."); + std::cout << "Initialization SSL db..." << std::endl; Ssl::CertificateDb::Create(db_path); std::cout << "Done" << std::endl; exit(EXIT_SUCCESS); } - if (fs_block_size == 0) { - struct statvfs sfs; - - if (xstatvfs(db_path.c_str(), &sfs)) { - fs_block_size = 2048; - } else { - fs_block_size = sfs.f_frsize; - // Sanity check; make sure we have a meaningful value. - if (fs_block_size < 512) - fs_block_size = 2048; + // only do filesystem checks when a path (-s) is given + if (!db_path.empty()) { + if (fs_block_size == 0) { + struct statvfs sfs; + + if (xstatvfs(db_path.c_str(), &sfs)) { + fs_block_size = 2048; + } else { + fs_block_size = sfs.f_frsize; + // Sanity check; make sure we have a meaningful value. + if (fs_block_size < 512) + fs_block_size = 2048; + } } - } - - { Ssl::CertificateDb::Check(db_path, max_db_size, fs_block_size); } + // Initialize SSL subsystem SSL_load_error_strings(); SSLeay_add_ssl_algorithms(); -- 2.47.2