IMPALA-5221: Avoid re-use of stale SASL contexts. The TSaslTransport is written as a thrift extension that is a wrapper around the Cyrus-SASL APIs. This transport is then used by Impala's RPC layer.
On RHEL7 systems that use newer versions of the Cyrus-SASL library, we noticed that we sometimes crash inside the Cyrus-SASL thirdparty while trying to lock an internal mutex. During my investigation, I found that we needed to fix the order of negotiation that happens in an edge case. The steps to use the Cyrus-SASL APIs for SASL negitiation are the following (Replace '_client_' with '_server_' for server calls): sasl_client_new() sasl_client_start() sasl_client_step() sasl_dispose() < --- When we're done with the connection. sasl_client_new() was being called in the constructor TSaslClient() which is invoked from SaslAuthProvider::WrapClientTransport(). sasl_client_start() and sasl_client_step() were being called under TSaslTransport::open(). If for some reason we hit an error during SASL negotiation, the TSaslTransport::open() call would fail. When we fail, the ThriftClientImpl::OpenWithRetry() does a retry, which directly retries the negotiation from sasl_client_start(). This caused the use of already freed resources from the first negotiation failure, hence causing the crash. To fix this, we make sure that on a negotiation failure, we dispose of all the resources properly by calling sasl_dispose() and retry the negotiation from the start by calling sasl_client_new() first, and then the remaining steps. This is done by moving the sasl_client_new() and sasl_server_new() calls out of the TSaslClient/TSaslServer constructors and into a new call called TSasl*::setupSaslContext(), which is called under TSaslTransport::open(). The patch is fairly large for the above mentioned change, however, most of it is just plumbing. Testing: Tested on systems with older SASL versions to make sure we don't regress. Also tested on systems with newer SASL versions where we previously saw the crash and verified that we don't see them anymore. Also, tested with GSSAPI and LDAP mechanisms. Change-Id: I7d509e14dfe46fa28d0626cf84daf6de82955d80 Reviewed-on: http://gerrit.cloudera.org:8080/7116 Reviewed-by: Sailesh Mukil <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/1d8ad293 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/1d8ad293 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/1d8ad293 Branch: refs/heads/master Commit: 1d8ad29387019b86e91092ecedc4c2f01d017da8 Parents: 6dd50f6 Author: Sailesh Mukil <[email protected]> Authored: Mon Jun 5 17:15:45 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Sat Jun 10 00:19:52 2017 +0000 ---------------------------------------------------------------------- be/src/transport/TSasl.cpp | 63 ++++++++---- be/src/transport/TSasl.h | 53 +++++++++- be/src/transport/TSaslClientTransport.cpp | 16 +++ be/src/transport/TSaslClientTransport.h | 8 ++ be/src/transport/TSaslServerTransport.cpp | 12 +++ be/src/transport/TSaslServerTransport.h | 14 +++ be/src/transport/TSaslTransport.cpp | 130 ++++++++++++++----------- be/src/transport/TSaslTransport.h | 16 +++ 8 files changed, 232 insertions(+), 80 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSasl.cpp ---------------------------------------------------------------------- diff --git a/be/src/transport/TSasl.cpp b/be/src/transport/TSasl.cpp index 3108dff..7b4d94d 100644 --- a/be/src/transport/TSasl.cpp +++ b/be/src/transport/TSasl.cpp @@ -41,6 +41,13 @@ using boost::algorithm::to_lower; namespace sasl { +TSasl::TSasl(const string& service, const string& serverFQDN, sasl_callback_t* callbacks) + : service(service), + serverFQDN(serverFQDN), + authComplete(false), + callbacks(callbacks), + conn(nullptr) { } + uint8_t* TSasl::unwrap(const uint8_t* incoming, const int offset, const uint32_t len, uint32_t* outLen) { uint32_t outputlen; @@ -101,14 +108,28 @@ string TSasl::getUsername() { } TSaslClient::TSaslClient(const string& mechanisms, const string& authenticationId, - const string& protocol, const string& serverName, const map<string,string>& props, - sasl_callback_t* callbacks) { - conn = NULL; + const string& service, const string& serverFQDN, const map<string,string>& props, + sasl_callback_t* callbacks) + : TSasl(service, serverFQDN, callbacks), + clientStarted(false), + mechList(mechanisms) { if (!props.empty()) { throw SaslServerImplException("Properties not yet supported"); } - int result = sasl_client_new(protocol.c_str(), serverName.c_str(), - NULL, NULL, callbacks, 0, &conn); + /* + if (!authenticationId.empty()) { + // TODO: setup security property + sasl_security_properties_t secprops; + // populate secprops + result = sasl_setprop(conn, SASL_AUTH_EXTERNAL, authenticationId.c_str()); + } + */ +} + +void TSaslClient::setupSaslContext() { + DCHECK(conn == nullptr); + int result = sasl_client_new(service.c_str(), serverFQDN.c_str(), NULL, NULL, callbacks, + 0, &conn); if (result != SASL_OK) { if (conn) { throw SaslServerImplException(sasl_errdetail(conn)); @@ -116,19 +137,12 @@ TSaslClient::TSaslClient(const string& mechanisms, const string& authenticationI throw SaslServerImplException(sasl_errstring(result, NULL, NULL)); } } +} - if (!authenticationId.empty()) { - /* TODO: setup security property */ - /* - sasl_security_properties_t secprops; - // populate secprops - result = sasl_setprop(conn, SASL_AUTH_EXTERNAL, authenticationId.c_str()); - */ - } - - chosenMech = mechList = mechanisms; - authComplete = false; +void TSaslClient::resetSaslContext() { clientStarted = false; + authComplete = false; + disposeSaslContext(); } /* Evaluates the challenge data and generates a response. */ @@ -190,12 +204,16 @@ bool TSaslClient::hasInitialResponse() { } TSaslServer::TSaslServer(const string& service, const string& serverFQDN, - const string& userRealm, - unsigned flags, sasl_callback_t* callbacks) { - conn = NULL; + const string& userRealm, unsigned flags, sasl_callback_t* callbacks) + : TSasl(service, serverFQDN, callbacks), + userRealm(userRealm), + flags(flags), + serverStarted(false) { } + +void TSaslServer::setupSaslContext() { int result = sasl_server_new(service.c_str(), serverFQDN.size() == 0 ? NULL : serverFQDN.c_str(), - userRealm.size() == 0 ? NULL :userRealm.c_str(), + userRealm.size() == 0 ? NULL : userRealm.c_str(), NULL, NULL, callbacks, flags, &conn); if (result != SASL_OK) { if (conn) { @@ -204,9 +222,12 @@ TSaslServer::TSaslServer(const string& service, const string& serverFQDN, throw SaslServerImplException(sasl_errstring(result, NULL, NULL)); } } +} - authComplete = false; +void TSaslServer::resetSaslContext() { serverStarted = false; + authComplete = false; + disposeSaslContext(); } uint8_t* TSaslServer::evaluateChallengeOrResponse(const uint8_t* response, http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSasl.h ---------------------------------------------------------------------- diff --git a/be/src/transport/TSasl.h b/be/src/transport/TSasl.h index ca13f10..06e3755 100644 --- a/be/src/transport/TSasl.h +++ b/be/src/transport/TSasl.h @@ -56,8 +56,14 @@ class SaslException : public TTransportException { * They are mostly wrappers for the cyrus-sasl library routines. */ class TSasl { - public: - virtual ~TSasl() { sasl_dispose(&conn); } + public: + /* Setup the SASL negotiation state. */ + virtual void setupSaslContext() = 0; + + /* Reset the SASL negotiation state. */ + virtual void resetSaslContext() = 0; + + virtual ~TSasl() { disposeSaslContext(); } /* * Called once per application to free resources.` @@ -107,10 +113,35 @@ class TSasl { std::string getUsername(); protected: + /* Name of service */ + std::string service; + + /* FQDN of server in use or the server to connect to */ + std::string serverFQDN; + /* Authorization is complete. */ bool authComplete; + + /* + * Callbacks to provide to the Cyrus-SASL library. Not owned. The user of the class + * must ensure that the callbacks live as long as the TSasl instance in use. + */ + sasl_callback_t* callbacks; + /* Sasl Connection. */ sasl_conn_t* conn; + + TSasl(const std::string& service, const std::string& serverFQDN, + sasl_callback_t* callbacks); + + /* Dispose of the SASL state. It is called once per connection as a part of teardown. */ + void disposeSaslContext() { + if (conn != nullptr) { + sasl_dispose(&conn); + conn = NULL; + } + } + }; class SaslClientImplException : public SaslException { @@ -144,6 +175,12 @@ class TSaslClient : public sasl::TSasl { /* Retrieves the negotiated property */ std::string getNegotiatedProperty(const std::string& propName); + /* Setup the SASL client negotiation state. */ + virtual void setupSaslContext(); + + /* Reset the SASL client negotiation state. */ + virtual void resetSaslContext(); + /* Determines whether this mechanism has an optional initial response. */ virtual bool hasInitialResponse(); @@ -181,10 +218,22 @@ class TSaslServer : public sasl::TSasl { } } + /* Setup the SASL server negotiation state. */ + virtual void setupSaslContext(); + + /* Reset the SASL server negotiation state. */ + virtual void resetSaslContext(); + /* Evaluates the response data and generates a challenge. */ virtual uint8_t* evaluateChallengeOrResponse(const uint8_t* challenge, const uint32_t len, uint32_t* resLen); private: + /* The domain of the user agent */ + std::string userRealm; + + /* Flags to pass down to the SASL library */ + unsigned flags; + /* true if sasl_server_start has been called. */ bool serverStarted; }; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslClientTransport.cpp ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslClientTransport.cpp b/be/src/transport/TSaslClientTransport.cpp index d8ae1ba..279a966 100644 --- a/be/src/transport/TSaslClientTransport.cpp +++ b/be/src/transport/TSaslClientTransport.cpp @@ -37,6 +37,22 @@ TSaslClientTransport::TSaslClientTransport(boost::shared_ptr<sasl::TSasl> saslCl : TSaslTransport(saslClient, transport) { } +void TSaslClientTransport::setupSaslNegotiationState() { + if (!sasl_) { + throw SaslClientImplException( + "Invalid state: setupSaslNegotiationState() failed. TSaslClient not created"); + } + sasl_->setupSaslContext(); +} + +void TSaslClientTransport::resetSaslNegotiationState() { + if (!sasl_) { + throw SaslClientImplException( + "Invalid state: resetSaslNegotiationState() failed. TSaslClient not created"); + } + sasl_->resetSaslContext(); +} + void TSaslClientTransport::handleSaslStartMessage() { uint32_t resLength = 0; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslClientTransport.h ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslClientTransport.h b/be/src/transport/TSaslClientTransport.h index da708ae..f23a3c0 100644 --- a/be/src/transport/TSaslClientTransport.h +++ b/be/src/transport/TSaslClientTransport.h @@ -48,6 +48,14 @@ class TSaslClientTransport : public TSaslTransport { boost::shared_ptr<TTransport> transport); protected: + /* Set up the Sasl server state for a connection. */ + virtual void setupSaslNegotiationState(); + + /* Reset the Sasl client state. The negotiation will have to start from scratch + * after this is called. + */ + virtual void resetSaslNegotiationState(); + /// Handle any startup messages. virtual void handleSaslStartMessage(); }; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslServerTransport.cpp ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslServerTransport.cpp b/be/src/transport/TSaslServerTransport.cpp index b867c31..a8000b1 100644 --- a/be/src/transport/TSaslServerTransport.cpp +++ b/be/src/transport/TSaslServerTransport.cpp @@ -74,6 +74,17 @@ void TSaslServerTransport::setSaslServer(sasl::TSasl* saslServer) { sasl_.reset(saslServer); } +void TSaslServerTransport::setupSaslNegotiationState() { + // Do nothing, as explained in header comment. +} + +void TSaslServerTransport::resetSaslNegotiationState() { + // Sometimes we may fail negotiation before creating the TSaslServer negotitation + // state if the client's first message is invalid. So we don't assume that TSaslServer + // will have been created. + if (sasl_) sasl_->resetSaslContext(); +} + void TSaslServerTransport::handleSaslStartMessage() { uint32_t resLength; NegotiationStatus status; @@ -106,6 +117,7 @@ void TSaslServerTransport::handleSaslStartMessage() { serverDefinition->realm_, serverDefinition->flags_, &serverDefinition->callbacks_[0])); + sasl_->setupSaslContext(); // First argument is interpreted as C-string sasl_->evaluateChallengeOrResponse( reinterpret_cast<const uint8_t*>(message_str.c_str()), resLength, &resLength); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslServerTransport.h ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslServerTransport.h b/be/src/transport/TSaslServerTransport.h index c662771..b01fcf3 100644 --- a/be/src/transport/TSaslServerTransport.h +++ b/be/src/transport/TSaslServerTransport.h @@ -41,6 +41,20 @@ namespace apache { namespace thrift { namespace transport { */ class TSaslServerTransport : public TSaslTransport { private: + /* Set up the Sasl server state for a connection. + * The Server negotiation state is setup in handleSaslStartMessage() after + * agreeing upon the client's chosen mechanism. So we do nothing here. + * The client always sends the first message in the protocol, so there isn't + * a case where the TSaslServer state needs to be setup before receiving the + * first message (see TSaslClient::hasInitialResponse()). + */ + virtual void setupSaslNegotiationState(); + + /* Reset the Sasl server state. The negotiation will have to start from scratch + * after this is called. + */ + virtual void resetSaslNegotiationState(); + /* Handle the initial message from the client. */ virtual void handleSaslStartMessage(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslTransport.cpp ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslTransport.cpp b/be/src/transport/TSaslTransport.cpp index a64861b..55e57c8 100644 --- a/be/src/transport/TSaslTransport.cpp +++ b/be/src/transport/TSaslTransport.cpp @@ -39,6 +39,7 @@ namespace apache { namespace thrift { namespace transport { TSaslTransport::TSaslTransport(boost::shared_ptr<TTransport> transport) : transport_(transport), memBuf_(new TMemoryBuffer(DEFAULT_MEM_BUF_SIZE)), + sasl_(NULL), shouldWrap_(false), isClient_(false) { } @@ -68,73 +69,74 @@ namespace apache { namespace thrift { namespace transport { return sasl_->getUsername(); } - void TSaslTransport::sendSaslMessage(const NegotiationStatus status, - const uint8_t* payload, const uint32_t length, bool flush) { - uint8_t messageHeader[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; - uint8_t dummy = 0; - if (payload == NULL) { - payload = &dummy; - } - messageHeader[0] = (uint8_t)status; - encodeInt(length, messageHeader, STATUS_BYTES); - transport_->write(messageHeader, HEADER_LENGTH); - transport_->write(payload, length); - if (flush) transport_->flush(); - } - - void TSaslTransport::open() { + void TSaslTransport::doSaslNegotiation() { NegotiationStatus status = TSASL_INVALID; uint32_t resLength; - // Only client should open the underlying transport. - if (isClient_ && !transport_->isOpen()) { - transport_->open(); - } - - // initiate SASL message - handleSaslStartMessage(); - - // SASL connection handshake - while (!sasl_->isComplete()) { - uint8_t* message = receiveSaslMessage(&status, &resLength); - if (status == TSASL_COMPLETE) { - if (isClient_) { - if (!sasl_->isComplete()) { - // Server sent COMPLETE out of order. - throw TTransportException("Received COMPLETE but no handshake occurred"); + try { + // Setup Sasl context. + setupSaslNegotiationState(); + + // Initiate SASL message. + handleSaslStartMessage(); + + // SASL connection handshake + while (!sasl_->isComplete()) { + uint8_t* message = receiveSaslMessage(&status, &resLength); + if (status == TSASL_COMPLETE) { + if (isClient_) { + if (!sasl_->isComplete()) { + // Server sent COMPLETE out of order. + throw TTransportException("Received COMPLETE but no handshake occurred"); + } + break; // handshake complete } - break; // handshake complete + } else if (status != TSASL_OK) { + stringstream ss; + ss << "Expected COMPLETE or OK, got " << status; + throw TTransportException(ss.str()); } - } else if (status != TSASL_OK) { - stringstream ss; - ss << "Expected COMPLETE or OK, got " << status; - throw TTransportException(ss.str()); + uint32_t challengeLength; + uint8_t* challenge = sasl_->evaluateChallengeOrResponse( + message, resLength, &challengeLength); + sendSaslMessage(sasl_->isComplete() ? TSASL_COMPLETE : TSASL_OK, + challenge, challengeLength); } - uint32_t challengeLength; - uint8_t* challenge = sasl_->evaluateChallengeOrResponse( - message, resLength, &challengeLength); - sendSaslMessage(sasl_->isComplete() ? TSASL_COMPLETE : TSASL_OK, - challenge, challengeLength); - } - // If the server isn't complete yet, we need to wait for its response. - // This will occur with ANONYMOUS auth, for example, where we send an - // initial response and are immediately complete. - if (isClient_ && (status == TSASL_INVALID || status == TSASL_OK)) { - receiveSaslMessage(&status, &resLength); - if (status != TSASL_COMPLETE) { - stringstream ss; - ss << "Expected COMPLETE or OK, got " << status; - throw TTransportException(ss.str()); + // If the server isn't complete yet, we need to wait for its response. + // This will occur with ANONYMOUS auth, for example, where we send an + // initial response and are immediately complete. + if (isClient_ && (status == TSASL_INVALID || status == TSASL_OK)) { + receiveSaslMessage(&status, &resLength); + if (status != TSASL_COMPLETE) { + stringstream ss; + ss << "Expected COMPLETE or OK, got " << status; + throw TTransportException(ss.str()); + } } + // TODO : need to set the shouldWrap_ based on QOP + /* + String qop = (String) sasl.getNegotiatedProperty(Sasl.QOP); + if (qop != null && !qop.equalsIgnoreCase("auth")) + shouldWrap_ = true; + */ + } catch (const TException& e) { + // If we hit an exception, that means the Sasl negotiation failed. We explicitly + // reset the negotiation state here since the caller may retry an open() which would + // start a new connection negotiation. + resetSaslNegotiationState(); + throw e; } + } - // TODO : need to set the shouldWrap_ based on QOP - /* - String qop = (String) sasl.getNegotiatedProperty(Sasl.QOP); - if (qop != null && !qop.equalsIgnoreCase("auth")) - shouldWrap_ = true; - */ + void TSaslTransport::open() { + // Only client should open the underlying transport. + if (isClient_ && !transport_->isOpen()) { + transport_->open(); + } + + // Start the SASL negotiation protocol. + doSaslNegotiation(); } void TSaslTransport::close() { @@ -235,6 +237,20 @@ namespace apache { namespace thrift { namespace transport { transport_->flush(); } + void TSaslTransport::sendSaslMessage(const NegotiationStatus status, + const uint8_t* payload, const uint32_t length, bool flush) { + uint8_t messageHeader[STATUS_BYTES + PAYLOAD_LENGTH_BYTES]; + uint8_t dummy = 0; + if (payload == NULL) { + payload = &dummy; + } + messageHeader[0] = (uint8_t)status; + encodeInt(length, messageHeader, STATUS_BYTES); + transport_->write(messageHeader, HEADER_LENGTH); + transport_->write(payload, length); + if (flush) transport_->flush(); + } + uint8_t* TSaslTransport::receiveSaslMessage(NegotiationStatus* status, uint32_t* length) { uint8_t messageHeader[HEADER_LENGTH]; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/1d8ad293/be/src/transport/TSaslTransport.h ---------------------------------------------------------------------- diff --git a/be/src/transport/TSaslTransport.h b/be/src/transport/TSaslTransport.h index 602f089..368f680 100644 --- a/be/src/transport/TSaslTransport.h +++ b/be/src/transport/TSaslTransport.h @@ -175,6 +175,21 @@ class TSaslTransport : public TVirtualTransport<TSaslTransport> { } /** + * Performs the SASL negotiation. + */ + void doSaslNegotiation(); + + /** + * Create the Sasl context for a server/client connection. + */ + virtual void setupSaslNegotiationState() = 0; + + /** + * Reset the negotiation state. + */ + virtual void resetSaslNegotiationState() = 0; + + /** * Read a complete Thrift SASL message. * * @return The SASL status and payload from this message. @@ -216,6 +231,7 @@ class TSaslTransport : public TVirtualTransport<TSaslTransport> { /// If memBuf_ is filled with bytes that are already read, and has crossed a size /// threshold (see implementation for exact value), resize the buffer to a default value. void shrinkBuffer(); + }; }}} // apache::thrift::transport
