TLS-negotiation [2/n]: remove SASL ANONYMOUS mechanism ANONYMOUS has not been enabled on any released server version going back all the way to 1.0. Adding it back an enabling it would be a backwards compatible change, so there is no harm in removing the currently unused code.
Change-Id: I5badb63bd8e416fbcbdc7bb6f8fa1e3ec9e6e036 Reviewed-on: http://gerrit.cloudera.org:8080/5756 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/73dcce64 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/73dcce64 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/73dcce64 Branch: refs/heads/master Commit: 73dcce642e85518d619ec3a4e9efcbef87a54c5c Parents: 13014ca Author: Dan Burkert <[email protected]> Authored: Wed Dec 14 20:11:45 2016 -0800 Committer: Dan Burkert <[email protected]> Committed: Mon Jan 23 18:17:38 2017 +0000 ---------------------------------------------------------------------- src/kudu/rpc/connection.cc | 2 -- src/kudu/rpc/sasl_client.cc | 26 +++++++++----------------- src/kudu/rpc/sasl_client.h | 4 ---- src/kudu/rpc/sasl_common.cc | 5 ----- src/kudu/rpc/sasl_common.h | 2 -- src/kudu/rpc/sasl_helper.cc | 11 ----------- src/kudu/rpc/sasl_helper.h | 7 ------- src/kudu/rpc/sasl_rpc-test.cc | 33 ++++++--------------------------- src/kudu/rpc/sasl_server.cc | 5 ----- src/kudu/rpc/sasl_server.h | 4 ---- 10 files changed, 15 insertions(+), 84 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/connection.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/connection.cc b/src/kudu/rpc/connection.cc index 8506b99..c7a87e9 100644 --- a/src/kudu/rpc/connection.cc +++ b/src/kudu/rpc/connection.cc @@ -656,8 +656,6 @@ Status Connection::InitSaslClient() { << "servers requiring Kerberos authentication."; } } - // TODO(todd): we dont seem to ever use ANONYMOUS. Should we remove it? - RETURN_NOT_OK(sasl_client().EnableAnonymous()); RETURN_NOT_OK(sasl_client().EnablePlain(user_credentials().real_user(), "")); RETURN_NOT_OK(sasl_client().Init(kSaslProtoName)); return Status::OK(); http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_client.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_client.cc b/src/kudu/rpc/sasl_client.cc index 83b1f97..379aec8 100644 --- a/src/kudu/rpc/sasl_client.cc +++ b/src/kudu/rpc/sasl_client.cc @@ -101,11 +101,6 @@ SaslClient::SaslClient(string app_name, Socket* socket) callbacks_.push_back(SaslBuildCallback(SASL_CB_LIST_END, nullptr, nullptr)); } -Status SaslClient::EnableAnonymous() { - DCHECK_EQ(client_state_, SaslNegotiationState::NEW); - return helper_.EnableAnonymous(); -} - Status SaslClient::EnablePlain(const string& user, const string& pass) { DCHECK_EQ(client_state_, SaslNegotiationState::NEW); RETURN_NOT_OK(helper_.EnablePlain()); @@ -207,7 +202,6 @@ Status SaslClient::Negotiate() { // We set nego_ok_ = true when the SASL library returns SASL_OK to us. // We set nego_response_expected_ = true each time we send a request to the server. - // When using ANONYMOUS, we get SASL_OK back immediately but still send INITIATE to the server. while (!nego_ok_ || nego_response_expected_) { ResponseHeader header; Slice param_buf; @@ -457,9 +451,13 @@ int SaslClient::GetOptionCb(const char* plugin_name, const char* option, return helper_.GetOptionCb(plugin_name, option, result, len); } -// Used for PLAIN and ANONYMOUS. +// Used for PLAIN. // SASL callback for SASL_CB_USER, SASL_CB_AUTHNAME, SASL_CB_LANGUAGE int SaslClient::SimpleCb(int id, const char** result, unsigned* len) { + if (PREDICT_FALSE(!helper_.IsPlainEnabled())) { + LOG(DFATAL) << "SASL Client: Simple callback called, but PLAIN auth is not enabled"; + return SASL_FAIL; + } if (PREDICT_FALSE(result == nullptr)) { LOG(DFATAL) << "SASL Client: result outparam is NULL"; return SASL_BADPARAM; @@ -469,19 +467,13 @@ int SaslClient::SimpleCb(int id, const char** result, unsigned* len) { // For impersonation, USER is the impersonated user, AUTHNAME is the "sudoer". case SASL_CB_USER: TRACE("SASL Client: callback for SASL_CB_USER"); - if (helper_.IsPlainEnabled()) { - *result = plain_auth_user_.c_str(); - if (len != nullptr) *len = plain_auth_user_.length(); - } else if (helper_.IsAnonymousEnabled()) { - *result = nullptr; - } + *result = plain_auth_user_.c_str(); + if (len != nullptr) *len = plain_auth_user_.length(); break; case SASL_CB_AUTHNAME: TRACE("SASL Client: callback for SASL_CB_AUTHNAME"); - if (helper_.IsPlainEnabled()) { - *result = plain_auth_user_.c_str(); - if (len != nullptr) *len = plain_auth_user_.length(); - } + *result = plain_auth_user_.c_str(); + if (len != nullptr) *len = plain_auth_user_.length(); break; case SASL_CB_LANGUAGE: LOG(DFATAL) << "SASL Client: Unable to handle SASL callback type SASL_CB_LANGUAGE" http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_client.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_client.h b/src/kudu/rpc/sasl_client.h index f8478c3..e35ff2a 100644 --- a/src/kudu/rpc/sasl_client.h +++ b/src/kudu/rpc/sasl_client.h @@ -48,10 +48,6 @@ class SaslClient { // Does not take ownership of the socket indicated by the fd. SaslClient(string app_name, Socket* socket); - // Enable ANONYMOUS authentication. - // Must be called after Init(). - Status EnableAnonymous(); - // Enable PLAIN authentication. // Must be called after Init(). Status EnablePlain(const string& user, const string& pass); http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_common.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_common.cc b/src/kudu/rpc/sasl_common.cc index 4e9aed5..359dc80 100644 --- a/src/kudu/rpc/sasl_common.cc +++ b/src/kudu/rpc/sasl_common.cc @@ -39,7 +39,6 @@ using std::set; namespace kudu { namespace rpc { -const char* const kSaslMechAnonymous = "ANONYMOUS"; const char* const kSaslMechPlain = "PLAIN"; const char* const kSaslMechGSSAPI = "GSSAPI"; @@ -380,9 +379,6 @@ sasl_callback_t SaslBuildCallback(int id, int (*proc)(void), void* context) { } SaslMechanism::Type SaslMechanism::value_of(const string& mech) { - if (boost::iequals(mech, "ANONYMOUS")) { - return ANONYMOUS; - } if (boost::iequals(mech, "PLAIN")) { return PLAIN; } @@ -394,7 +390,6 @@ SaslMechanism::Type SaslMechanism::value_of(const string& mech) { const char* SaslMechanism::name_of(SaslMechanism::Type val) { switch (val) { - case ANONYMOUS: return "ANONYMOUS"; case PLAIN: return "PLAIN"; case GSSAPI: return "GSSAPI"; default: http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_common.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_common.h b/src/kudu/rpc/sasl_common.h index cf1ee27..419fc5f 100644 --- a/src/kudu/rpc/sasl_common.h +++ b/src/kudu/rpc/sasl_common.h @@ -36,7 +36,6 @@ namespace rpc { using std::string; // Constants -extern const char* const kSaslMechAnonymous; extern const char* const kSaslMechPlain; extern const char* const kSaslMechGSSAPI; @@ -102,7 +101,6 @@ struct SaslNegotiationState { struct SaslMechanism { enum Type { INVALID, - ANONYMOUS, PLAIN, GSSAPI }; http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_helper.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_helper.cc b/src/kudu/rpc/sasl_helper.cc index 06fc82c..60be7a6 100644 --- a/src/kudu/rpc/sasl_helper.cc +++ b/src/kudu/rpc/sasl_helper.cc @@ -46,7 +46,6 @@ using google::protobuf::MessageLite; SaslHelper::SaslHelper(PeerType peer_type) : peer_type_(peer_type), conn_header_exchanged_(false), - anonymous_enabled_(false), plain_enabled_(false), gssapi_enabled_(false) { tag_ = (peer_type_ == SERVER) ? "Sasl Server" : "Sasl Client"; @@ -124,16 +123,6 @@ int SaslHelper::GetOptionCb(const char* plugin_name, const char* option, return SASL_FAIL; } -Status SaslHelper::EnableAnonymous() { - RETURN_NOT_OK(EnableMechanism(kSaslMechAnonymous)); - anonymous_enabled_ = true; - return Status::OK(); -} - -bool SaslHelper::IsAnonymousEnabled() const { - return anonymous_enabled_; -} - Status SaslHelper::EnablePlain() { RETURN_NOT_OK(EnableMechanism(kSaslMechPlain)); plain_enabled_ = true; http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_helper.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_helper.h b/src/kudu/rpc/sasl_helper.h index 408b488..45adbe3 100644 --- a/src/kudu/rpc/sasl_helper.h +++ b/src/kudu/rpc/sasl_helper.h @@ -84,12 +84,6 @@ class SaslHelper { // Implements the client_mech_list / mech_list callbacks. int GetOptionCb(const char* plugin_name, const char* option, const char** result, unsigned* len); - // Enable the ANONYMOUS SASL mechanism. - Status EnableAnonymous(); - - // Check for the ANONYMOUS SASL mechanism. - bool IsAnonymousEnabled() const; - // Enable the PLAIN SASL mechanism. Status EnablePlain(); @@ -125,7 +119,6 @@ class SaslHelper { std::set<string> mechs_; // Active mechanisms. mutable string mech_list_; // Mechanism list string returned by callbacks. - bool anonymous_enabled_; bool plain_enabled_; bool gssapi_enabled_; http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_rpc-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_rpc-test.cc b/src/kudu/rpc/sasl_rpc-test.cc index 7aecbb7..6220e72 100644 --- a/src/kudu/rpc/sasl_rpc-test.cc +++ b/src/kudu/rpc/sasl_rpc-test.cc @@ -73,10 +73,10 @@ class TestSaslRpc : public RpcTestBase { // Test basic initialization of the objects. TEST_F(TestSaslRpc, TestBasicInit) { SaslServer server(kSaslAppName, nullptr); - server.EnableAnonymous(); + server.EnablePlain(); ASSERT_OK(server.Init(kSaslAppName)); SaslClient client(kSaslAppName, nullptr); - client.EnableAnonymous(); + client.EnablePlain("test", "test"); ASSERT_OK(client.Init(kSaslAppName)); } @@ -123,27 +123,6 @@ static void RunNegotiationTest(const SocketCallable& server_runner, //////////////////////////////////////////////////////////////////////////////// -static void RunAnonNegotiationServer(Socket* conn) { - SaslServer sasl_server(kSaslAppName, conn); - CHECK_OK(sasl_server.EnableAnonymous()); - CHECK_OK(sasl_server.Init(kSaslAppName)); - CHECK_OK(sasl_server.Negotiate()); -} - -static void RunAnonNegotiationClient(Socket* conn) { - SaslClient sasl_client(kSaslAppName, conn); - CHECK_OK(sasl_client.EnableAnonymous()); - CHECK_OK(sasl_client.Init(kSaslAppName)); - CHECK_OK(sasl_client.Negotiate()); -} - -// Test SASL negotiation using the ANONYMOUS mechanism over a socket. -TEST_F(TestSaslRpc, TestAnonNegotiation) { - RunNegotiationTest(RunAnonNegotiationServer, RunAnonNegotiationClient); -} - -//////////////////////////////////////////////////////////////////////////////// - static void RunPlainNegotiationServer(Socket* conn) { SaslServer sasl_server(kSaslAppName, conn); CHECK_OK(sasl_server.EnablePlain()); @@ -430,7 +409,7 @@ TEST_F(TestSaslRpc, TestPreflight) { static void RunTimeoutExpectingServer(Socket* conn) { SaslServer sasl_server(kSaslAppName, conn); - CHECK_OK(sasl_server.EnableAnonymous()); + CHECK_OK(sasl_server.EnablePlain()); CHECK_OK(sasl_server.Init(kSaslAppName)); Status s = sasl_server.Negotiate(); ASSERT_TRUE(s.IsNetworkError()) << "Expected client to time out and close the connection. Got: " @@ -439,7 +418,7 @@ static void RunTimeoutExpectingServer(Socket* conn) { static void RunTimeoutNegotiationClient(Socket* sock) { SaslClient sasl_client(kSaslAppName, sock); - CHECK_OK(sasl_client.EnableAnonymous()); + CHECK_OK(sasl_client.EnablePlain("test", "test")); CHECK_OK(sasl_client.Init(kSaslAppName)); MonoTime deadline = MonoTime::Now() - MonoDelta::FromMilliseconds(100L); sasl_client.set_deadline(deadline); @@ -457,7 +436,7 @@ TEST_F(TestSaslRpc, TestClientTimeout) { static void RunTimeoutNegotiationServer(Socket* sock) { SaslServer sasl_server(kSaslAppName, sock); - CHECK_OK(sasl_server.EnableAnonymous()); + CHECK_OK(sasl_server.EnablePlain()); CHECK_OK(sasl_server.Init(kSaslAppName)); MonoTime deadline = MonoTime::Now() - MonoDelta::FromMilliseconds(100L); sasl_server.set_deadline(deadline); @@ -468,7 +447,7 @@ static void RunTimeoutNegotiationServer(Socket* sock) { static void RunTimeoutExpectingClient(Socket* conn) { SaslClient sasl_client(kSaslAppName, conn); - CHECK_OK(sasl_client.EnableAnonymous()); + CHECK_OK(sasl_client.EnablePlain("test", "test")); CHECK_OK(sasl_client.Init(kSaslAppName)); Status s = sasl_client.Negotiate(); ASSERT_TRUE(s.IsNetworkError()) << "Expected server to time out and close the connection. Got: " http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_server.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.cc b/src/kudu/rpc/sasl_server.cc index 6b3560d..6dae445 100644 --- a/src/kudu/rpc/sasl_server.cc +++ b/src/kudu/rpc/sasl_server.cc @@ -65,11 +65,6 @@ SaslServer::SaslServer(string app_name, Socket* socket) callbacks_.push_back(SaslBuildCallback(SASL_CB_LIST_END, nullptr, nullptr)); } -Status SaslServer::EnableAnonymous() { - DCHECK_EQ(server_state_, SaslNegotiationState::NEW); - return helper_.EnableAnonymous(); -} - Status SaslServer::EnablePlain() { DCHECK_EQ(server_state_, SaslNegotiationState::NEW); RETURN_NOT_OK(helper_.EnablePlain()); http://git-wip-us.apache.org/repos/asf/kudu/blob/73dcce64/src/kudu/rpc/sasl_server.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.h b/src/kudu/rpc/sasl_server.h index f26f229..7cbf6fd 100644 --- a/src/kudu/rpc/sasl_server.h +++ b/src/kudu/rpc/sasl_server.h @@ -46,10 +46,6 @@ class SaslServer { // Does not take ownership of 'socket'. SaslServer(string app_name, Socket* socket); - // Enable ANONYMOUS authentication. - // Must be called after Init(). - Status EnableAnonymous(); - // Enable PLAIN authentication. // Despite PLAIN authentication taking a username and password, we disregard // the password and use this as a "unauthenticated" mode.
