This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 4657f062f401a2da5e5f4aa3a3e3244f45dba8c2
Author: wzhou-code <wz...@cloudera.com>
AuthorDate: Mon Nov 16 15:53:30 2020 -0800
IMPALA-10305 (part 2): Sync Kudu's FIPS compliant changes
kudu-3210 added FIPS compliant changes for Kudu.
In previous patch, we ported the following patches for kudu-3210 into
Impala source tree:
http://gerrit.cloudera.org:8080/16631/
http://gerrit.cloudera.org:8080/16657/
http://gerrit.cloudera.org:8080/16658/
http://gerrit.cloudera.org:8080/16659/
The last patch http://gerrit.cloudera.org:8080/16659/ fixed an OpenSSL
race condition with work around by adding lock before verifying
signature. But this issue could be fixed by redefining the thread ID
callback without any additional locking.
Kudu reverted commit f9f3189a6dbe0636d578d80b1d8e60cf7b2e6686
and added a new patch to redefine the thread ID callback.
https://gerrit.cloudera.org/#/c/16730/
https://gerrit.cloudera.org/#/c/16731/
This patch syncs Kudu's code changes of above two patches.
Testing:
- Passed exhausive tests.
Change-Id: I04b9d46b5d7228289565617b8d3cfbef9f3b5ba3
Reviewed-on: http://gerrit.cloudera.org:8080/16736
Reviewed-by: Tim Armstrong <tarmstr...@cloudera.com>
Reviewed-by: Attila Bukor <abu...@apache.org>
Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
---
be/src/kudu/security/CMakeLists.txt | 2 +-
be/src/kudu/security/crypto.cc | 13 -----------
be/src/kudu/security/openssl_util.cc | 42 ++++++++---------------------------
be/src/kudu/security/openssl_util.h | 33 +++++++++------------------
be/src/kudu/security/tls_context.cc | 10 ---------
be/src/kudu/security/tls_handshake.cc | 18 +--------------
6 files changed, 22 insertions(+), 96 deletions(-)
diff --git a/be/src/kudu/security/CMakeLists.txt
b/be/src/kudu/security/CMakeLists.txt
index 9475e7d..22e7442 100644
--- a/be/src/kudu/security/CMakeLists.txt
+++ b/be/src/kudu/security/CMakeLists.txt
@@ -73,9 +73,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/be/src/kudu/security/crypto.cc b/be/src/kudu/security/crypto.cc
index 5e6fd16..234d193 100644
--- a/be/src/kudu/security/crypto.cc
+++ b/be/src/kudu/security/crypto.cc
@@ -18,9 +18,6 @@
#include "kudu/security/crypto.h"
#include <memory>
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-#include <mutex>
-#endif
#include <ostream>
#include <string>
@@ -70,10 +67,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> {
@@ -143,9 +136,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()),
@@ -238,9 +228,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/be/src/kudu/security/openssl_util.cc
b/be/src/kudu/security/openssl_util.cc
index 7198db3..3d9544c 100644
--- a/be/src/kudu/security/openssl_util.cc
+++ b/be/src/kudu/security/openssl_util.cc
@@ -28,10 +28,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"
@@ -39,7 +37,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"
@@ -47,15 +44,9 @@
#include "kudu/util/scoped_cleanup.h"
#include "kudu/util/status.h"
#include "kudu/util/subprocess.h"
-
-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);
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+#include "kudu/util/thread.h"
+#endif
using std::ostringstream;
using std::string;
@@ -97,6 +88,10 @@ void LockingCB(int mode, int type, const char* /*file*/, int
/*line*/) {
m->unlock();
}
}
+
+void ThreadIdCB(CRYPTO_THREADID* tid) {
+ CRYPTO_THREADID_set_numeric(tid, Thread::UniqueThreadId());
+}
#endif
void CheckFIPSMode() {
@@ -209,6 +204,8 @@ void DoInitializeOpenSSL() {
// Callbacks used by OpenSSL required in a multi-threaded setting.
CRYPTO_set_locking_callback(LockingCB);
+
+ CRYPTO_THREADID_set_callback(ThreadIdCB);
}
#endif
CheckFIPSMode();
@@ -370,26 +367,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/be/src/kudu/security/openssl_util.h
b/be/src/kudu/security/openssl_util.h
index 356ecb0..00c4ebf 100644
--- a/be/src/kudu/security/openssl_util.h
+++ b/be/src/kudu/security/openssl_util.h
@@ -19,7 +19,6 @@
#include <functional>
#include <memory>
-#include <mutex>
#include <ostream>
#include <string>
@@ -190,39 +189,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/be/src/kudu/security/tls_context.cc
b/be/src/kudu/security/tls_context.cc
index 622165f..9409319 100644
--- a/be/src/kudu/security/tls_context.cc
+++ b/be/src/kudu/security/tls_context.cc
@@ -82,12 +82,6 @@ DEFINE_int32(ipki_server_key_size, 2048,
"is used for TLS connections to and from clients and other
servers.");
TAG_FLAG(ipki_server_key_size, experimental);
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-namespace {
-kudu::security::OpenSSLMutex mutex;
-} // anonymous namespace
-#endif
-
namespace kudu {
namespace security {
@@ -433,10 +427,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/be/src/kudu/security/tls_handshake.cc
b/be/src/kudu/security/tls_handshake.cc
index d61da6d..8e17e6c 100644
--- a/be/src/kudu/security/tls_handshake.cc
+++ b/be/src/kudu/security/tls_handshake.cc
@@ -22,15 +22,11 @@
#include <openssl/x509v3.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.
