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 eb67da24d6eb18832e7343866964e929081c0fd0..5c84765e2c6fc524aedd754a7ec524c7747ce24f 100644 (file)
--- 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 2a1184a2ff8e4941fd017921ef139ee490397c7e..922a06d5023dc1bf4196a7fb64b8ad33b665e7f8 100644 (file)
--- 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 e5040d9be2c72b8fce907fc0b3133dc3f4d7fe29..2890bba0adc42eaa812086211728255852ae027c 100644 (file)
--- 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 c39ef2115184420f8d1a15dd53f2bfc4040eeb18..2d2218bac092d3d1a965d4a1b8e64f6caeb13381 100644 (file)
--- 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 d6f1d75636f65677c4d5c5107208522a2633aae9..db51c62c3aabfcc7e138875ededbd518cb517b4c 100644 (file)
--- 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 0b032b412e32e731e2ccf12ba635cde7560386e3..dcec31191d4d826cd0a791a825a94eaee3137949 100644 (file)
--- a/src/security/forward.h
+++ b/src/security/forward.h
@@ -18,6 +18,7 @@
 #include <gnutls/x509.h>
 #endif
 #endif
+#include <list>
 
 /* flags a SSL connection can be configured with */
 #define SSL_FLAG_NO_DEFAULT_CA      (1<<0)
@@ -62,6 +63,18 @@ typedef Security::LockingPointer<struct gnutls_x509_crt_int, gnutls_x509_crt_dei
 typedef void * CertPointer;
 #endif
 
+#if USE_OPENSSL
+CtoCpp1(X509_CRL_free, X509_CRL *)
+typedef LockingPointer<X509_CRL, X509_CRL_free_cpp, CRYPTO_LOCK_X509_CRL> CrlPointer;
+#elif USE_GNUTLS
+CtoCpp1(gnutls_x509_crl_deinit, gnutls_x509_crl_t)
+typedef Security::LockingPointer<struct gnutls_x509_crl_int, gnutls_x509_crl_deinit, -1> CrlPointer;
+#else
+typedef void *CrlPointer;
+#endif
+
+typedef std::list<Security::CrlPointer> CertRevokeList;
+
 } // namespace Security
 
 #endif /* SQUID_SRC_SECURITY_FORWARD_H */

diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h
index 7d9e1d2990bc014abfa2395eeae6e9b7bc66e665..366538ca152bb078b2df7c645bf023a189fa4318 100644 (file)
--- a/src/ssl/gadgets.h
+++ b/src/ssl/gadgets.h
@@ -75,9 +75,6 @@ typedef TidyPointer<SSL, SSL_free_cpp> SSL_Pointer;
 CtoCpp1(DH_free, DH *);
 typedef TidyPointer<DH, DH_free_cpp> DH_Pointer;
 
-sk_free_wrapper(sk_X509_CRL, STACK_OF(X509_CRL) *, X509_CRL_free)
-typedef TidyPointer<STACK_OF(X509_CRL), sk_X509_CRL_free_wrapper> X509_CRL_STACK_Pointer;
-
 sk_free_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free)
 typedef TidyPointer<STACK_OF(X509_NAME), sk_X509_NAME_free_wrapper> X509_NAME_STACK_Pointer;
 

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index 52afe8a0cdf5dee5e1e2ae1a2ac8c2b848e96e2e..2709294bdadbbd32c92bad419b6808982734a0e8 100644 (file)
--- 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 8a9c80feb9d7d32dbbd83470b1df3a516d2b0950..e62a7b947faf25a76441ed99d10a571264e6f91c 100644 (file)
--- a/src/ssl/support.h
+++ b/src/ssl/support.h
@@ -90,7 +90,7 @@ typedef CbDataList<Ssl::CertError> 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 8c3a70d1a340c4c936221b9d02e7bea15c57e6bb..c13b5e33ce5e9e8db0df70bbc53092e470a981bd 100644 (file)
--- 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 34225e06f6586fc72060143282216b4dabd02bd4..3c8fb35353dd3cbb21f81de0c394875256640e1b 100644 (file)
--- 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)