This is an automated email from the ASF dual-hosted git repository.
granthenke pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new a026b5f Revert "KUDU-3210 Add lock before verifying signature"
a026b5f is described below
commit a026b5fbbabf067cb65f7228daaa7dabbe87996e
Author: Attila Bukor <[email protected]>
AuthorDate: Mon Nov 16 21:55:21 2020 +0100
Revert "KUDU-3210 Add lock before verifying signature"
This reverts commit f9f3189a6dbe0636d578d80b1d8e60cf7b2e6686.
Change-Id: I02daa5ca95de697714d5e893f8bc813bdc9530d3
Reviewed-on: http://gerrit.cloudera.org:8080/16731
Reviewed-by: Grant Henke <[email protected]>
Reviewed-by: Alexey Serbin <[email protected]>
Tested-by: Kudu Jenkins
---
src/kudu/security/CMakeLists.txt | 2 +-
src/kudu/security/crypto.cc | 13 -------------
src/kudu/security/openssl_util.cc | 33 ---------------------------------
src/kudu/security/openssl_util.h | 33 +++++++++++----------------------
src/kudu/security/tls_context.cc | 10 ----------
src/kudu/security/tls_handshake.cc | 18 +-----------------
6 files changed, 13 insertions(+), 96 deletions(-)
diff --git a/src/kudu/security/CMakeLists.txt b/src/kudu/security/CMakeLists.txt
index bfe7f87..7abaabb 100644
--- a/src/kudu/security/CMakeLists.txt
+++ b/src/kudu/security/CMakeLists.txt
@@ -67,9 +67,9 @@ set(SECURITY_SRCS
ca/cert_management.cc
cert.cc
crypto.cc
+ kerberos_util.cc
gssapi.cc
init.cc
- kerberos_util.cc
openssl_util.cc
${PORTED_X509_CHECK_HOST_CC}
security_flags.cc
diff --git a/src/kudu/security/crypto.cc b/src/kudu/security/crypto.cc
index a6acbc6..2deb348 100644
--- a/src/kudu/security/crypto.cc
+++ b/src/kudu/security/crypto.cc
@@ -27,9 +27,6 @@
#include <functional>
#include <memory>
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-#include <mutex>
-#endif
#include <ostream>
#include <string>
@@ -69,10 +66,6 @@ int DerWritePublicKey(BIO* bio, EVP_PKEY* key) {
return i2d_RSA_PUBKEY_bio(bio, rsa.get());
}
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-OpenSSLMutex mutex;
-#endif
-
} // anonymous namespace
template<> struct SslTypeTraits<BIGNUM> {
@@ -142,9 +135,6 @@ Status PublicKey::VerifySignature(DigestType digest,
const EVP_MD* md = GetMessageDigest(digest);
auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
- std::unique_lock<OpenSSLMutex> l(mutex);
-#endif
OPENSSL_RET_NOT_OK(EVP_DigestVerifyInit(md_ctx.get(), nullptr, md, nullptr,
GetRawData()),
"error initializing verification digest");
OPENSSL_RET_NOT_OK(EVP_DigestVerifyUpdate(md_ctx.get(), data.data(),
data.size()),
@@ -237,9 +227,6 @@ Status PrivateKey::MakeSignature(DigestType digest,
const EVP_MD* md = GetMessageDigest(digest);
auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
- std::unique_lock<OpenSSLMutex> l(mutex);
-#endif
OPENSSL_RET_NOT_OK(EVP_DigestSignInit(md_ctx.get(), nullptr, md, nullptr,
GetRawData()),
"error initializing signing digest");
OPENSSL_RET_NOT_OK(EVP_DigestSignUpdate(md_ctx.get(), data.data(),
data.size()),
diff --git a/src/kudu/security/openssl_util.cc
b/src/kudu/security/openssl_util.cc
index f846048..1f6896c 100644
--- a/src/kudu/security/openssl_util.cc
+++ b/src/kudu/security/openssl_util.cc
@@ -31,10 +31,8 @@
#include <string>
#include <vector>
-#include <gflags/gflags.h>
#include <glog/logging.h>
-#include "kudu/gutil/macros.h"
#include "kudu/gutil/strings/split.h"
#include "kudu/gutil/strings/strip.h"
#include "kudu/gutil/strings/substitute.h"
@@ -42,7 +40,6 @@
#include "kudu/util/debug/leakcheck_disabler.h"
#endif
#include "kudu/util/errno.h"
-#include "kudu/util/flag_tags.h"
#include "kudu/util/flags.h"
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#include "kudu/util/mutex.h"
@@ -54,15 +51,6 @@
#include "kudu/util/thread.h"
#endif
-DEFINE_bool(openssl_defensive_locking,
- false,
- "If enabled, cryptographic methods lock more defensively to work
around thread safety "
- "issues in certain OpenSSL versions. This makes Kudu servers or
clients more stable "
- "when running on an affected OpenSSL version, sacrificing some
performance.");
-TAG_FLAG(openssl_defensive_locking, unsafe);
-TAG_FLAG(openssl_defensive_locking, hidden);
-TAG_FLAG(openssl_defensive_locking, runtime);
-
using std::ostringstream;
using std::string;
using std::vector;
@@ -382,26 +370,5 @@ Status GetPasswordFromShellCommand(const string& cmd,
string* password) {
return Status::OK();
}
-OpenSSLMutex::OpenSSLMutex() :
locking_enabled_(FLAGS_openssl_defensive_locking) {}
-
-void OpenSSLMutex::lock() {
- if (locking_enabled_) {
- mutex_.lock();
- }
-}
-
-bool OpenSSLMutex::try_lock() {
- if (locking_enabled_) {
- return mutex_.try_lock();
- }
- return true;
-}
-
-void OpenSSLMutex::unlock() {
- if (locking_enabled_) {
- mutex_.unlock();
- }
-}
-
} // namespace security
} // namespace kudu
diff --git a/src/kudu/security/openssl_util.h b/src/kudu/security/openssl_util.h
index 113e7a9..e5b522e 100644
--- a/src/kudu/security/openssl_util.h
+++ b/src/kudu/security/openssl_util.h
@@ -24,7 +24,6 @@
#include <functional>
#include <memory>
-#include <mutex>
#include <ostream>
#include <string>
@@ -199,39 +198,29 @@ class RawDataWrapper {
c_unique_ptr<RawDataType> data_;
};
-// Wrapper around std::mutex that only locks if a special
-// 'openssl_defensive_locking' flag is set to true. See the description of the
-// flag for more details.
-class OpenSSLMutex {
- public:
- OpenSSLMutex();
- void lock();
- bool try_lock();
- void unlock();
-
- private:
- std::mutex mutex_;
- const bool locking_enabled_;
-};
namespace internal {
+
// Implementation of SCOPED_OPENSSL_NO_PENDING_ERRORS. Use the macro form
// instead of directly instantiating the implementation class.
struct ScopedCheckNoPendingSSLErrors {
public:
- explicit ScopedCheckNoPendingSSLErrors(const char* func) : func_(func) {
- DCHECK_EQ(ERR_peek_error(), 0) << "Expected no pending OpenSSL errors on "
<< func_
- << " entry, but had: " <<
GetOpenSSLErrors();
+ explicit ScopedCheckNoPendingSSLErrors(const char* func)
+ : func_(func) {
+ DCHECK_EQ(ERR_peek_error(), 0)
+ << "Expected no pending OpenSSL errors on " << func_
+ << " entry, but had: " << GetOpenSSLErrors();
}
~ScopedCheckNoPendingSSLErrors() {
- DCHECK_EQ(ERR_peek_error(), 0) << "Expected no pending OpenSSL errors on "
<< func_
- << " exit, but had: " << GetOpenSSLErrors();
+ DCHECK_EQ(ERR_peek_error(), 0)
+ << "Expected no pending OpenSSL errors on " << func_
+ << " exit, but had: " << GetOpenSSLErrors();
}
private:
const char* const func_;
};
-} // namespace internal
-} // namespace security
+} // namespace internal
+} // namespace security
} // namespace kudu
diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc
index 76c9a52..6f4d19c 100644
--- a/src/kudu/security/tls_context.cc
+++ b/src/kudu/security/tls_context.cc
@@ -94,12 +94,6 @@ DEFINE_int32(openssl_security_level_override, -1,
TAG_FLAG(openssl_security_level_override, hidden);
TAG_FLAG(openssl_security_level_override, unsafe);
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-namespace {
-kudu::security::OpenSSLMutex mutex;
-} // anonymous namespace
-#endif
-
namespace kudu {
namespace security {
@@ -466,10 +460,6 @@ boost::optional<CertSignRequest>
TlsContext::GetCsrIfNecessary() const {
Status TlsContext::AdoptSignedCert(const Cert& cert) {
SCOPED_OPENSSL_NO_PENDING_ERRORS;
-
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
- unique_lock<OpenSSLMutex> lock_global(mutex);
-#endif
unique_lock<RWMutex> lock(lock_);
if (!csr_) {
diff --git a/src/kudu/security/tls_handshake.cc
b/src/kudu/security/tls_handshake.cc
index 3c3b80e..52162c1 100644
--- a/src/kudu/security/tls_handshake.cc
+++ b/src/kudu/security/tls_handshake.cc
@@ -22,15 +22,11 @@
#include <openssl/x509.h>
#include <memory>
-#if OPENSSL_VERSION_NUMBER < 0x10002000L
-#include <mutex>
-#endif
#include <string>
#include "kudu/gutil/strings/strip.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/security/cert.h"
-#include "kudu/security/openssl_util.h"
#include "kudu/security/tls_socket.h"
#include "kudu/util/net/socket.h"
#include "kudu/util/status.h"
@@ -44,12 +40,6 @@ using std::string;
using std::unique_ptr;
using strings::Substitute;
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-namespace {
-kudu::security::OpenSSLMutex mutex;
-} // anonymous namespace
-#endif
-
namespace kudu {
namespace security {
@@ -102,13 +92,7 @@ Status TlsHandshake::Continue(const string& recv, string*
send) {
DCHECK(n == recv.size() || (n == -1 && recv.empty()));
DCHECK_EQ(BIO_ctrl_pending(rbio), recv.size());
- int rc;
- {
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
- std::unique_lock<OpenSSLMutex> lock(mutex);
-#endif
- rc = SSL_do_handshake(ssl_.get());
- }
+ int rc = SSL_do_handshake(ssl_.get());
if (rc != 1) {
int ssl_err = SSL_get_error(ssl_.get(), rc);
// WANT_READ and WANT_WRITE indicate that the handshake is not yet
complete.
