From 621f4299cbf34e3daf591d314cf1c14496d5f0d1 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 11 Jul 2017 03:11:52 +1200 Subject: [PATCH] TLS: move X509_NAME_STACK_Pointer to Security::ServerOptions This Pointer is only used by the Server port options. No GnuTLS support added in this patch, just a straight shuffle of the OpenSSL code. --- src/anyp/PortCfg.cc | 14 +----- src/anyp/PortCfg.h | 3 -- src/cache_cf.cc | 10 +---- src/security/ServerOptions.cc | 84 +++++++++++++++++++++++++++++++++++ src/security/ServerOptions.h | 26 +++++++++-- src/ssl/gadgets.h | 3 -- src/ssl/support.cc | 36 ++++----------- src/ssl/support.h | 3 ++ src/tests/stub_libsecurity.cc | 3 ++ 9 files changed, 125 insertions(+), 57 deletions(-) diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc index 798d81bcb4..34659c49ab 100644 --- a/src/anyp/PortCfg.cc +++ b/src/anyp/PortCfg.cc @@ -42,7 +42,6 @@ AnyP::PortCfg::PortCfg() : listenConn() #if USE_OPENSSL , - clientca(NULL), sslContextSessionId(NULL), generateHostCertificates(true), dynamicCertMemCacheSize(4*1024*1024), // 4 MB @@ -50,8 +49,7 @@ AnyP::PortCfg::PortCfg() : signPkey(), certsToChain(), untrustedSigningCert(), - untrustedSignPkey(), - clientCA() + untrustedSignPkey() #endif { memset(&tcp_keepalive, 0, sizeof(tcp_keepalive)); @@ -68,7 +66,6 @@ AnyP::PortCfg::~PortCfg() safe_free(defaultsite); #if USE_OPENSSL - safe_free(clientca); safe_free(sslContextSessionId); #endif } @@ -95,8 +92,6 @@ AnyP::PortCfg::clone() const b->secure = secure; #if USE_OPENSSL - if (clientca) - b->clientca = xstrdup(clientca); if (sslContextSessionId) b->sslContextSessionId = xstrdup(sslContextSessionId); @@ -136,13 +131,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 (clientca) { - clientCA.reset(SSL_load_client_CA_file(clientca)); - if (clientCA.get() == NULL) { - fatalf("Unable to read client CAs! from %s", clientca); - } - } - if (!secure.createStaticServerContext(*this)) { char buf[128]; fatalf("%s_port %s initialization error", AnyP::ProtocolType_str[transport.protocol], s.toUrl(buf, sizeof(buf))); diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h index 17b4cfe0ac..43938f1a3c 100644 --- a/src/anyp/PortCfg.h +++ b/src/anyp/PortCfg.h @@ -73,7 +73,6 @@ public: Security::ServerOptions secure; #if USE_OPENSSL - char *clientca; char *sslContextSessionId; ///< "session id context" for secure.staticSslContext bool generateHostCertificates; ///< dynamically make host cert for sslBump size_t dynamicCertMemCacheSize; ///< max size of generated certificates memory cache @@ -83,8 +82,6 @@ public: Ssl::X509_STACK_Pointer certsToChain; ///< x509 certificates to send with the generated cert Security::CertPointer untrustedSigningCert; ///< x509 certificate for signing untrusted generated certificates Ssl::EVP_PKEY_Pointer untrustedSignPkey; ///< private key for signing untrusted generated certificates - - Ssl::X509_NAME_STACK_Pointer clientCA; ///< CA certificates to use when verifying client certificates #endif }; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 1ff67e067a..fc5547c47b 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -3718,8 +3718,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token) } else if (strncmp(token, "cipher=", 7) == 0) { s->secure.parse(token); } else if (strncmp(token, "clientca=", 9) == 0) { - safe_free(s->clientca); - s->clientca = xstrdup(token + 9); + s->secure.parse(token); } else if (strncmp(token, "cafile=", 7) == 0) { debugs(3, DBG_PARSE_NOTE(1), "UPGRADE WARNING: '" << token << "' is deprecated " << "in " << cfg_directive << ". Use 'tls-cafile=' instead."); @@ -3801,12 +3800,7 @@ parsePortCfg(AnyP::PortCfgPointer *head, const char *optionName) parse_port_option(s, token); } -#if USE_OPENSSL - // if clientca has been defined but not cafile, then use it to verify - // but if cafile has been defined, only use that to verify - if (s->clientca && !s->secure.caFiles.size()) - s->secure.caFiles.emplace_back(SBuf(s->clientca)); -#endif + s->secure.syncCaFiles(); if (s->transport.protocol == AnyP::PROTO_HTTPS) { s->secure.encryptTransport = true; diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index c6030f68b1..7f0f8d56b1 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -21,6 +21,25 @@ #include #endif +Security::ServerOptions & +Security::ServerOptions::operator =(const Security::ServerOptions &old) { + if (this != &old) { + Security::PeerOptions::operator =(old); + clientCaFile = old.clientCaFile; + dh = old.dh; + dhParamsFile = old.dhParamsFile; + eecdhCurve = old.eecdhCurve; + parsedDhParams = old.parsedDhParams; +#if USE_OPENSSL + if (auto *stk = SSL_dup_CA_list(old.clientCaStack.get())) + clientCaStack = Security::ServerOptions::X509_NAME_STACK_Pointer(stk); +#else + clientCaStack = nullptr; +#endif + } + return *this; +} + void Security::ServerOptions::parse(const char *token) { @@ -130,12 +149,46 @@ Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &port) if (!Ssl::InitServerContext(t, port)) return false; #endif + if (!loadClientCaFile()) + return false; } staticContext = std::move(t); return bool(staticContext); } +void +Security::ServerOptions::syncCaFiles() +{ + // if caFiles is set, just use that + if (caFiles.size()) + return; + + // otherwise fall back to clientca if it is defined + if (!clientCaFile.isEmpty()) + caFiles.emplace_back(clientCaFile); +} + +/// load clientca= file (if any) into memory. +/// \retval true clientca is not set, or loaded successfully +/// \retval false unable to load the file, or not using OpenSSL +bool +Security::ServerOptions::loadClientCaFile() +{ + if (clientCaFile.isEmpty()) + return true; + +#if USE_OPENSSL + auto *stk = SSL_load_client_CA_file(clientCaFile.c_str()); + clientCaStack = Security::ServerOptions::X509_NAME_STACK_Pointer(stk); +#endif + if (!clientCaStack) { + debugs(83, DBG_CRITICAL, "FATAL: Unable to read client CAs from file: " << clientCaFile); + } + + return bool(clientCaStack); +} + void Security::ServerOptions::loadDhParams() { @@ -167,6 +220,37 @@ Security::ServerOptions::loadDhParams() #endif } +void +Security::ServerOptions::updateContextClientCa(Security::ContextPointer &ctx) +{ +#if USE_OPENSSL + if (clientCaStack) { + ERR_clear_error(); + if (STACK_OF(X509_NAME) *clientca = SSL_dup_CA_list(clientCaStack.get())) { + SSL_CTX_set_client_CA_list(ctx.get(), clientca); + } else { + auto ssl_error = ERR_get_error(); + debugs(83, DBG_CRITICAL, "ERROR: Failed to dupe the client CA list: " << Security::ErrorString(ssl_error)); + return; + } + + if (parsedFlags & SSL_FLAG_DELAYED_AUTH) { + debugs(83, 9, "Not requesting client certificates until acl processing requires one"); + SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_NONE, nullptr); + } else { + debugs(83, 9, "Requiring client certificates."); + Ssl::SetupVerifyCallback(ctx); + } + + updateContextCrl(ctx); + + } else { + debugs(83, 9, "Not requiring any client certificates"); + SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_NONE, NULL); + } +#endif +} + void Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) { diff --git a/src/security/ServerOptions.h b/src/security/ServerOptions.h index 6072d97399..94b9069a58 100644 --- a/src/security/ServerOptions.h +++ b/src/security/ServerOptions.h @@ -19,15 +19,20 @@ namespace Security class ServerOptions : public PeerOptions { public: +#if USE_OPENSSL + sk_dtor_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free); + typedef std::unique_ptr X509_NAME_STACK_Pointer; +#endif + ServerOptions() : PeerOptions() { // Bug 4005: dynamic contexts use a lot of memory and it // is more secure to have only a small set of trusted CA. flags.tlsDefaultCa.defaultTo(false); } ServerOptions(const ServerOptions &) = default; - ServerOptions &operator =(const ServerOptions &) = default; - ServerOptions(ServerOptions &&) = default; - ServerOptions &operator =(ServerOptions &&) = default; + ServerOptions &operator =(const ServerOptions &); + ServerOptions(ServerOptions &&o) { this->operator =(o); } + ServerOptions &operator =(ServerOptions &&o) { this->operator =(o); return *this; } virtual ~ServerOptions() = default; /* Security::PeerOptions API */ @@ -44,14 +49,29 @@ public: /// update the context with DH, EDH, EECDH settings void updateContextEecdh(Security::ContextPointer &); + /// update the context with CA details used to verify client certificates + void updateContextClientCa(Security::ContextPointer &); + + /// sync the various sources of CA files to be loaded + void syncCaFiles(); + public: /// TLS context to use for HTTPS accelerator or static SSL-Bump Security::ContextPointer staticContext; private: + bool loadClientCaFile(); void loadDhParams(); private: + SBuf clientCaFile; ///< name of file to load client CAs from +#if USE_OPENSSL + /// CA certificate(s) to use when verifying client certificates + X509_NAME_STACK_Pointer clientCaStack; +#else + void *clientCaStack = nullptr; +#endif + SBuf dh; ///< Diffi-Helman cipher config SBuf dhParamsFile; ///< Diffi-Helman ciphers parameter file SBuf eecdhCurve; ///< Elliptic curve for ephemeral EC-based DH key exchanges diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 0aa8a97cff..93fc8123dd 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -67,9 +67,6 @@ typedef std::unique_ptr> RSA_Pointer; typedef std::unique_ptr> X509_REQ_Pointer; -sk_dtor_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free); -typedef std::unique_ptr X509_NAME_STACK_Pointer; - typedef std::unique_ptr> AUTHORITY_KEYID_Pointer; sk_dtor_wrapper(sk_GENERAL_NAME, STACK_OF(GENERAL_NAME) *, GENERAL_NAME_free); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index f90812715b..f85170700c 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -376,6 +376,12 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) return ok; } +void +Ssl::SetupVerifyCallback(Security::ContextPointer &ctx) +{ + SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, ssl_verify_cb); +} + // "dup" function for SSL_get_ex_new_index("cert_err_check") #if SQUID_USE_CONST_CRYPTO_EX_DATA_DUP static int @@ -533,31 +539,7 @@ configureSslContext(Security::ContextPointer &ctx, AnyP::PortCfg &port) port.secure.updateContextEecdh(ctx); port.secure.updateContextCa(ctx); - - if (port.clientCA.get()) { - ERR_clear_error(); - if (STACK_OF(X509_NAME) *clientca = SSL_dup_CA_list(port.clientCA.get())) { - SSL_CTX_set_client_CA_list(ctx.get(), clientca); - } else { - ssl_error = ERR_get_error(); - debugs(83, DBG_CRITICAL, "ERROR: Failed to dupe the client CA list: " << Security::ErrorString(ssl_error)); - return false; - } - - if (port.secure.parsedFlags & SSL_FLAG_DELAYED_AUTH) { - debugs(83, 9, "Not requesting client certificates until acl processing requires one"); - SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_NONE, NULL); - } else { - debugs(83, 9, "Requiring client certificates."); - SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, ssl_verify_cb); - } - - port.secure.updateContextCrl(ctx); - - } else { - debugs(83, 9, "Not requiring any client certificates"); - SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_NONE, NULL); - } + port.secure.updateContextClientCa(ctx); if (port.secure.parsedFlags & SSL_FLAG_DONT_VERIFY_DOMAIN) SSL_CTX_set_ex_data(ctx.get(), ssl_ctx_ex_index_dont_verify_domain, (void *) -1); @@ -678,11 +660,11 @@ Ssl::InitClientContext(Security::ContextPointer &ctx, Security::PeerOptions &pee maybeSetupRsaCallback(ctx); if (fl & SSL_FLAG_DONT_VERIFY_PEER) { - debugs(83, 2, "NOTICE: Peer certificates are not verified for validity!"); + debugs(83, 2, "SECURITY WARNING: Peer certificates are not verified for validity!"); SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_NONE, NULL); } else { debugs(83, 9, "Setting certificate verification callback."); - SSL_CTX_set_verify(ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, ssl_verify_cb); + Ssl::SetupVerifyCallback(ctx); } return true; diff --git a/src/ssl/support.h b/src/ssl/support.h index 0537a2789f..67d8c2b64b 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -79,6 +79,9 @@ bool InitServerContext(Security::ContextPointer &, AnyP::PortCfg &); /// initialize a TLS client context with OpenSSL specific settings bool InitClientContext(Security::ContextPointer &, Security::PeerOptions &, long flags); +/// set the certificate verify callback for a context +void SetupVerifyCallback(Security::ContextPointer &); + } //namespace Ssl /// \ingroup ServerProtocolSSLAPI diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 055dcec7e3..4214e3a1bb 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -87,11 +87,14 @@ void parse_securePeerOptions(Security::PeerOptions *) STUB #include "security/ServerOptions.h" //Security::ServerOptions::ServerOptions(const Security::ServerOptions &) STUB +Security::ServerOptions &Security::ServerOptions::operator=(Security::ServerOptions const&) STUB_RETVAL(*this); void Security::ServerOptions::parse(const char *) STUB void Security::ServerOptions::dumpCfg(Packable *, const char *) const STUB Security::ContextPointer Security::ServerOptions::createBlankContext() const STUB_RETVAL(Security::ContextPointer()) bool Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &) STUB_RETVAL(false) void Security::ServerOptions::updateContextEecdh(Security::ContextPointer &) STUB +void Security::ServerOptions::updateContextClientCa(Security::ContextPointer &) STUB +void Security::ServerOptions::syncCaFiles() STUB #include "security/Session.h" namespace Security { -- 2.39.5