From: Amos Jeffries Date: Thu, 24 Sep 2015 21:08:23 +0000 (-0700) Subject: Crypto-NG: cleanup and optimize CRL handling X-Git-Tag: SQUID_4_0_1~44 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6b19d1f9da9516dfb1ec49a7847ff282b88e6b3f;p=thirdparty%2Fsquid.git Crypto-NG: cleanup and optimize CRL handling Certificate Revokation Lists have gone through several iterations of logic redesign leading to duplicated code and non-optimal I/O. Client contexts were loading CRL directly from disk into the context on every new context creation. Whereas the server contexts were loading into an OpenSSL STACK_OF structure and adding from memory instead of disk. This later design is more performant. * Move the pre-loaded CRL set to Security::PeerOptions and store in a std::list structure as LockingPointer which will deallocate as needed on shutdwown and reconfigure. This depends on trunk rev.14304 * Replace the client context disk I/O with the pre-loaded CRL list * Add GnuTLS CRL list types. Though at this point GnuTLS does not pre-load the CRL files. --- diff --git a/src/Makefile.am b/src/Makefile.am index eb67da24d6..5c84765e2c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2490,6 +2490,7 @@ tests_testHttp1Parser_LDADD= \ ip/libip.la \ $(top_builddir)/lib/libmiscutil.la \ $(SQUID_CPPUNIT_LIBS) \ + $(SSLLIB) \ $(COMPAT_LIB) \ $(XTRA_LIBS) tests_testHttp1Parser_LDFLAGS = $(LIBADD_DL) diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc index 2a1184a2ff..922a06d502 100644 --- a/src/anyp/PortCfg.cc +++ b/src/anyp/PortCfg.cc @@ -57,7 +57,6 @@ AnyP::PortCfg::PortCfg() : certsToChain(), untrustedSigningCert(), untrustedSignPkey(), - clientVerifyCrls(), clientCA(), dhParams(), eecdhCurve(NULL) @@ -150,9 +149,6 @@ AnyP::PortCfg::configureSslServerContext() fatalf("Unable to generate signing SSL certificate for untrusted sites for %s_port %s", AnyP::ProtocolType_str[transport.protocol], s.toUrl(buf, sizeof(buf))); } - if (!secure.crlFile.isEmpty()) - clientVerifyCrls.reset(Ssl::loadCrl(secure.crlFile.c_str(), secure.parsedFlags)); - if (clientca) { clientCA.reset(SSL_load_client_CA_file(clientca)); if (clientCA.get() == NULL) { diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h index e5040d9be2..2890bba0ad 100644 --- a/src/anyp/PortCfg.h +++ b/src/anyp/PortCfg.h @@ -87,7 +87,6 @@ public: Security::CertPointer untrustedSigningCert; ///< x509 certificate for signing untrusted generated certificates Ssl::EVP_PKEY_Pointer untrustedSignPkey; ///< private key for signing untrusted generated certificates - Ssl::X509_CRL_STACK_Pointer clientVerifyCrls; ///< additional CRL lists to use when verifying the client certificate Ssl::X509_NAME_STACK_Pointer clientCA; ///< CA certificates to use when verifying client certificates Ssl::DH_Pointer dhParams; ///< DH parameters for temporary/ephemeral DH key exchanges char *eecdhCurve; ///< Elliptic curve for ephemeral EC-based DH key exchanges diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc index c39ef21151..2d2218bac0 100644 --- a/src/security/PeerOptions.cc +++ b/src/security/PeerOptions.cc @@ -34,6 +34,7 @@ Security::PeerOptions::PeerOptions(const Security::PeerOptions &p) : sslDomain(p.sslDomain), parsedOptions(p.parsedOptions), parsedFlags(p.parsedFlags), + parsedCrl(p.parsedCrl), sslVersion(p.sslVersion), encryptTransport(p.encryptTransport) { @@ -79,6 +80,7 @@ Security::PeerOptions::parse(const char *token) caDir = SBuf(token + 7); } else if (strncmp(token, "crlfile=", 8) == 0) { crlFile = SBuf(token + 8); + loadCrlFile(); } else if (strncmp(token, "flags=", 6) == 0) { if (parsedFlags != 0) { debugs(3, DBG_PARSE_NOTE(1), "WARNING: Overwriting flags=" << sslFlags << " with " << SBuf(token + 6)); @@ -196,9 +198,11 @@ Security::PeerOptions::createClientContext(bool setOptions) // XXX: temporary performance regression. c_str() data copies and prevents this being a const method t = sslCreateClientContext(certFile.c_str(), privateKeyFile.c_str(), sslCipher.c_str(), (setOptions ? parsedOptions : 0), parsedFlags, - caFile.c_str(), caDir.c_str(), crlFile.c_str()); + caFile.c_str(), caDir.c_str()); #endif + updateContextCrl(t); + return t; } @@ -448,6 +452,54 @@ Security::PeerOptions::parseFlags() return fl; } +/// Load a CRLs list stored in the file whose /path/name is in crlFile +/// replaces any CRL loaded previously +void +Security::PeerOptions::loadCrlFile() +{ + parsedCrl.clear(); + if (crlFile.isEmpty()) + return; + +#if USE_OPENSSL + BIO *in = BIO_new_file(crlFile.c_str(), "r"); + if (!in) { + debugs(83, 2, "WARNING: Failed to open CRL file " << crlFile); + return; + } + + while (X509_CRL *crl = PEM_read_bio_X509_CRL(in,NULL,NULL,NULL)) { + parsedCrl.emplace_back(Security::CrlPointer(crl)); + } + BIO_free(in); +#endif +} + +void +Security::PeerOptions::updateContextCrl(Security::ContextPointer &ctx) +{ +#if USE_OPENSSL + bool verifyCrl = false; + X509_STORE *st = SSL_CTX_get_cert_store(ctx); + if (parsedCrl.size()) { + for (auto &i : parsedCrl) { + if (!X509_STORE_add_crl(st, i.get())) + debugs(83, 2, "WARNING: Failed to add CRL"); + else + verifyCrl = true; + } + } + +#if X509_V_FLAG_CRL_CHECK + if ((parsedFlags & SSL_FLAG_VERIFY_CRL_ALL)) + X509_STORE_set_flags(st, X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL); + else if (verifyCrl || (parsedFlags & SSL_FLAG_VERIFY_CRL)) + X509_STORE_set_flags(st, X509_V_FLAG_CRL_CHECK); +#endif + +#endif /* USE_OPENSSL */ +} + void parse_securePeerOptions(Security::PeerOptions *opt) { diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h index d6f1d75636..db51c62c3a 100644 --- a/src/security/PeerOptions.h +++ b/src/security/PeerOptions.h @@ -37,12 +37,16 @@ public: /// sync the context options with tls-min-version=N configuration void updateTlsVersionLimits(); + /// setup the CRL details for the given context + void updateContextCrl(Security::ContextPointer &); + /// output squid.conf syntax with 'pfx' prefix on parameters for the stored settings void dumpCfg(Packable *, const char *pfx) const; private: long parseOptions(); long parseFlags(); + void loadCrlFile(); public: SBuf certFile; ///< path of file containing PEM format X509 certificate @@ -61,6 +65,8 @@ public: long parsedOptions; ///< parsed value of sslOptions long parsedFlags; ///< parsed value of sslFlags + Security::CertRevokeList parsedCrl; ///< CRL to use when verifying the remote end certificate + private: int sslVersion; diff --git a/src/security/forward.h b/src/security/forward.h index 0b032b412e..dcec31191d 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -18,6 +18,7 @@ #include #endif #endif +#include /* flags a SSL connection can be configured with */ #define SSL_FLAG_NO_DEFAULT_CA (1<<0) @@ -62,6 +63,18 @@ typedef Security::LockingPointer CrlPointer; +#elif USE_GNUTLS +CtoCpp1(gnutls_x509_crl_deinit, gnutls_x509_crl_t) +typedef Security::LockingPointer CrlPointer; +#else +typedef void *CrlPointer; +#endif + +typedef std::list CertRevokeList; + } // namespace Security #endif /* SQUID_SRC_SECURITY_FORWARD_H */ diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 7d9e1d2990..366538ca15 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -75,9 +75,6 @@ typedef TidyPointer SSL_Pointer; CtoCpp1(DH_free, DH *); typedef TidyPointer DH_Pointer; -sk_free_wrapper(sk_X509_CRL, STACK_OF(X509_CRL) *, X509_CRL_free) -typedef TidyPointer X509_CRL_STACK_Pointer; - sk_free_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free) typedef TidyPointer X509_NAME_STACK_Pointer; diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 52afe8a0cd..2709294bda 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -471,64 +471,6 @@ ssl_initialize(void) ssl_ex_index_ssl_validation_counter = SSL_get_ex_new_index(0, (void *) "ssl_validation_counter", NULL, NULL, &ssl_free_int); } -/// \ingroup ServerProtocolSSLInternal -static int -ssl_load_crl(SSL_CTX *sslContext, const char *CRLfile) -{ - X509_STORE *st = SSL_CTX_get_cert_store(sslContext); - X509_CRL *crl; - BIO *in = BIO_new_file(CRLfile, "r"); - int count = 0; - - if (!in) { - debugs(83, 2, "WARNING: Failed to open CRL file '" << CRLfile << "'"); - return 0; - } - - while ((crl = PEM_read_bio_X509_CRL(in,NULL,NULL,NULL))) { - if (!X509_STORE_add_crl(st, crl)) - debugs(83, 2, "WARNING: Failed to add CRL from file '" << CRLfile << "'"); - else - ++count; - - X509_CRL_free(crl); - } - - BIO_free(in); - return count; -} - -STACK_OF(X509_CRL) * -Ssl::loadCrl(const char *CRLFile, long &flags) -{ - X509_CRL *crl; - BIO *in = BIO_new_file(CRLFile, "r"); - if (!in) { - debugs(83, 2, "WARNING: Failed to open CRL file '" << CRLFile << "'"); - return NULL; - } - - STACK_OF(X509_CRL) *CRLs = sk_X509_CRL_new_null(); - if (!CRLs) { - debugs(83, 2, "WARNING: Failed to allocate X509_CRL stack to load file '" << CRLFile << "'"); - return NULL; - } - - int count = 0; - while ((crl = PEM_read_bio_X509_CRL(in,NULL,NULL,NULL))) { - if (!sk_X509_CRL_push(CRLs, crl)) - debugs(83, 2, "WARNING: Failed to add CRL from file '" << CRLFile << "'"); - else - ++count; - } - BIO_free(in); - - if (count) - flags |= SSL_FLAG_VERIFY_CRL; - - return CRLs; -} - DH * Ssl::readDHParams(const char *dhfile) { @@ -666,21 +608,7 @@ configureSslContext(SSL_CTX *sslContext, AnyP::PortCfg &port) SSL_CTX_set_verify(sslContext, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, ssl_verify_cb); } - if (port.clientVerifyCrls.get()) { - X509_STORE *st = SSL_CTX_get_cert_store(sslContext); - for (int i = 0; i < sk_X509_CRL_num(port.clientVerifyCrls.get()); ++i) { - X509_CRL *crl = sk_X509_CRL_value(port.clientVerifyCrls.get(), i); - if (!X509_STORE_add_crl(st, crl)) - debugs(83, 2, "WARNING: Failed to add CRL"); - } - } - -#if X509_V_FLAG_CRL_CHECK - if (port.secure.parsedFlags & SSL_FLAG_VERIFY_CRL_ALL) - X509_STORE_set_flags(SSL_CTX_get_cert_store(sslContext), X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL); - else if (port.secure.parsedFlags & SSL_FLAG_VERIFY_CRL) - X509_STORE_set_flags(SSL_CTX_get_cert_store(sslContext), X509_V_FLAG_CRL_CHECK); -#endif + port.secure.updateContextCrl(sslContext); } else { debugs(83, 9, "Not requiring any client certificates"); @@ -784,7 +712,7 @@ ssl_next_proto_cb(SSL *s, unsigned char **out, unsigned char *outlen, const unsi #endif SSL_CTX * -sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, long fl, const char *CAfile, const char *CApath, const char *CRLfile) +sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, long fl, const char *CAfile, const char *CApath) { ssl_initialize(); @@ -861,19 +789,6 @@ sslCreateClientContext(const char *certfile, const char *keyfile, const char *ci debugs(83, DBG_IMPORTANT, "WARNING: Ignoring error setting CA certificate locations: " << ERR_error_string(ssl_error, NULL)); } - if (*CRLfile) { - ssl_load_crl(sslContext, CRLfile); - fl |= SSL_FLAG_VERIFY_CRL; - } - -#if X509_V_FLAG_CRL_CHECK - if (fl & SSL_FLAG_VERIFY_CRL_ALL) - X509_STORE_set_flags(SSL_CTX_get_cert_store(sslContext), X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL); - else if (fl & SSL_FLAG_VERIFY_CRL) - X509_STORE_set_flags(SSL_CTX_get_cert_store(sslContext), X509_V_FLAG_CRL_CHECK); - -#endif - if (!(fl & SSL_FLAG_NO_DEFAULT_CA) && !SSL_CTX_set_default_verify_paths(sslContext)) { const int ssl_error = ERR_get_error(); diff --git a/src/ssl/support.h b/src/ssl/support.h index 8a9c80feb9..e62a7b947f 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -90,7 +90,7 @@ typedef CbDataList CertErrors; SSL_CTX *sslCreateServerContext(AnyP::PortCfg &port); /// \ingroup ServerProtocolSSLAPI -SSL_CTX *sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, long flags, const char *CAfile, const char *CApath, const char *CRLfile); +SSL_CTX *sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, long flags, const char *CAfile, const char *CApath); /// \ingroup ServerProtocolSSLAPI int ssl_read_method(int, char *, int); @@ -155,12 +155,6 @@ inline const char *bumpMode(int bm) return (0 <= bm && bm < Ssl::bumpEnd) ? Ssl::BumpModeStr[bm] : NULL; } -/** - \ingroup ServerProtocolSSLAPI - * Load a CRLs list stored in a file - */ -STACK_OF(X509_CRL) *loadCrl(const char *CRLFile, long &flags); - /** \ingroup ServerProtocolSSLAPI * Load DH params from file diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 8c3a70d1a3..c13b5e33ce 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -21,6 +21,7 @@ Security::PeerOptions Security::ProxyOutgoingConfig; void Security::PeerOptions::parse(char const*) STUB Security::ContextPointer Security::PeerOptions::createClientContext(bool) STUB_RETVAL(NULL) void Security::PeerOptions::updateTlsVersionLimits() STUB +void Security::PeerOptions::updateContextCrl(Security::ContextPointer &) STUB void Security::PeerOptions::dumpCfg(Packable*, char const*) const STUB long Security::PeerOptions::parseOptions() STUB_RETVAL(0) long Security::PeerOptions::parseFlags() STUB_RETVAL(0) diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 34225e06f6..3c8fb35353 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -57,7 +57,7 @@ bool CertError::operator == (const CertError &ce) const STUB_RETVAL(false) bool CertError::operator != (const CertError &ce) const STUB_RETVAL(false) } // namespace Ssl SSL_CTX *sslCreateServerContext(AnyP::PortCfg &port) STUB_RETVAL(NULL) -SSL_CTX *sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, const char *flags, const char *CAfile, const char *CApath, const char *CRLfile) STUB_RETVAL(NULL) +SSL_CTX *sslCreateClientContext(const char *certfile, const char *keyfile, const char *cipher, long options, const char *flags, const char *CAfile, const char *CApath) STUB_RETVAL(NULL) int ssl_read_method(int, char *, int) STUB_RETVAL(0) int ssl_write_method(int, const char *, int) STUB_RETVAL(0) void ssl_shutdown_method(SSL *ssl) STUB @@ -72,7 +72,6 @@ namespace Ssl //GETX509ATTRIBUTE GetX509CAAttribute; //GETX509ATTRIBUTE GetX509Fingerprint; const char *BumpModeStr[] = {""}; -STACK_OF(X509_CRL) *loadCrl(const char *CRLFile, long &flags) STUB_RETVAL(NULL) DH *readDHParams(const char *dhfile) STUB_RETVAL(NULL) bool generateUntrustedCert(Security::CertPointer & untrustedCert, EVP_PKEY_Pointer & untrustedPkey, Security::CertPointer const & cert, EVP_PKEY_Pointer const & pkey) STUB_RETVAL(false) SSL_CTX * generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port) STUB_RETVAL(NULL)