From 18f300b03f5abac1bf42f983af187dc07c9b9bc7 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Tue, 22 Dec 2020 19:44:30 +0800 Subject: [PATCH] refactor: fix typo and add comments to security --- src/runtime/security/client_negotiation.h | 5 +++-- src/runtime/security/negotiation.h | 5 +++-- src/runtime/security/negotiation_manager.cpp | 9 +++++---- src/runtime/security/negotiation_utils.h | 2 ++ src/runtime/security/replica_access_controller.h | 6 +++--- src/runtime/security/sasl_client_wrapper.h | 5 ++++- src/runtime/security/sasl_wrapper.cpp | 9 ++++----- src/runtime/security/sasl_wrapper.h | 7 +++++-- src/runtime/security/server_negotiation.cpp | 2 +- src/runtime/security/server_negotiation.h | 7 +++++-- src/runtime/test/server_negotiation_test.cpp | 12 ++++++------ thirdparty/CMakeLists.txt | 7 ++++++- 12 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/runtime/security/client_negotiation.h b/src/runtime/security/client_negotiation.h index 609fe1551d..16e7a1c878 100644 --- a/src/runtime/security/client_negotiation.h +++ b/src/runtime/security/client_negotiation.h @@ -22,12 +22,13 @@ namespace dsn { namespace security { +// client_negotiation negotiates a session on client side. class client_negotiation : public negotiation { public: - client_negotiation(rpc_session_ptr session); + explicit client_negotiation(rpc_session_ptr session); - void start(); + void start() override; void handle_response(error_code err, const negotiation_response &&response); private: diff --git a/src/runtime/security/negotiation.h b/src/runtime/security/negotiation.h index c947979852..adac0121ba 100644 --- a/src/runtime/security/negotiation.h +++ b/src/runtime/security/negotiation.h @@ -32,11 +32,12 @@ typedef rpc_holder negotiation_rpc; class negotiation { public: - negotiation(rpc_session_ptr session) - : _session(session), _status(negotiation_status::type::INVALID) + explicit negotiation(rpc_session_ptr session) + : _session(std::move(session)), _status(negotiation_status::type::INVALID) { _sasl = create_sasl_wrapper(_session->is_client()); } + virtual ~negotiation() = 0; virtual void start() = 0; diff --git a/src/runtime/security/negotiation_manager.cpp b/src/runtime/security/negotiation_manager.cpp index 520bb8a022..d6977e7c70 100644 --- a/src/runtime/security/negotiation_manager.cpp +++ b/src/runtime/security/negotiation_manager.cpp @@ -35,13 +35,14 @@ inline bool is_negotiation_message(dsn::task_code code) return code == RPC_NEGOTIATION || code == RPC_NEGOTIATION_ACK; } +// in_white_list returns if the rpc code can be allowed to bypass negotiation. inline bool in_white_list(task_code code) { return is_negotiation_message(code) || fd::is_failure_detector_message(code); } -negotiation_map negotiation_manager::_negotiations; -utils::rw_lock_nr negotiation_manager::_lock; +/*static*/ negotiation_map negotiation_manager::_negotiations; +/*static*/ utils::rw_lock_nr negotiation_manager::_lock; negotiation_manager::negotiation_manager() : serverlet("negotiation_manager") {} @@ -64,7 +65,7 @@ void negotiation_manager::on_negotiation_request(negotiation_rpc rpc) std::shared_ptr nego = get_negotiation(rpc); if (nullptr != nego) { - server_negotiation *srv_negotiation = static_cast(nego.get()); + auto srv_negotiation = static_cast(nego.get()); srv_negotiation->handle_request(rpc); } } @@ -76,7 +77,7 @@ void negotiation_manager::on_negotiation_response(error_code err, negotiation_rp std::shared_ptr nego = get_negotiation(rpc); if (nullptr != nego) { - client_negotiation *cli_negotiation = static_cast(nego.get()); + auto cli_negotiation = static_cast(nego.get()); cli_negotiation->handle_response(err, std::move(rpc.response())); } } diff --git a/src/runtime/security/negotiation_utils.h b/src/runtime/security/negotiation_utils.h index 27b8e59bf3..d9138749e0 100644 --- a/src/runtime/security/negotiation_utils.h +++ b/src/runtime/security/negotiation_utils.h @@ -21,6 +21,8 @@ namespace dsn { namespace security { + +// TODO(wutao): rename to negotiation_status_to_string inline const char *enum_to_string(negotiation_status::type s) { switch (s) { diff --git a/src/runtime/security/replica_access_controller.h b/src/runtime/security/replica_access_controller.h index 51395e9bf4..427b279d0f 100644 --- a/src/runtime/security/replica_access_controller.h +++ b/src/runtime/security/replica_access_controller.h @@ -25,9 +25,9 @@ namespace security { class replica_access_controller : public access_controller { public: - replica_access_controller(const std::string &name); - bool allowed(message_ex *msg); - void update(const std::string &users); + explicit replica_access_controller(const std::string &name); + bool allowed(message_ex *msg) override; + void update(const std::string &users) override; private: utils::rw_lock_nr _lock; // [ diff --git a/src/runtime/security/sasl_client_wrapper.h b/src/runtime/security/sasl_client_wrapper.h index 6acaf6309d..72feb8e2d9 100644 --- a/src/runtime/security/sasl_client_wrapper.h +++ b/src/runtime/security/sasl_client_wrapper.h @@ -21,15 +21,18 @@ namespace dsn { namespace security { + +// sasl_client_wrapper is a simple wrapper over cyrus-sasl's sasl_client_xxx API. class sasl_client_wrapper : public sasl_wrapper { public: sasl_client_wrapper() = default; - ~sasl_client_wrapper() = default; + ~sasl_client_wrapper() override = default; error_s init(); error_s start(const std::string &mechanism, const blob &input, blob &output); error_s step(const blob &input, blob &output); }; + } // namespace security } // namespace dsn diff --git a/src/runtime/security/sasl_wrapper.cpp b/src/runtime/security/sasl_wrapper.cpp index e8f855cf7e..a2fdf9f724 100644 --- a/src/runtime/security/sasl_wrapper.cpp +++ b/src/runtime/security/sasl_wrapper.cpp @@ -28,9 +28,8 @@ const char *sasl_err_desc(int status, sasl_conn_t *conn) { if (conn != nullptr) { return sasl_errdetail(conn); - } else { - return sasl_errstring(status, nullptr, nullptr); } + return sasl_errstring(status, nullptr, nullptr); } sasl_wrapper::~sasl_wrapper() @@ -40,14 +39,14 @@ sasl_wrapper::~sasl_wrapper() } } -error_s sasl_wrapper::retrive_username(std::string &output) +error_s sasl_wrapper::retrieve_username(std::string &output) { - FAIL_POINT_INJECT_F("sasl_wrapper_retrive_username", [](dsn::string_view str) { + FAIL_POINT_INJECT_F("sasl_wrapper_retrieve_username", [](dsn::string_view str) { error_code err = error_code::try_get(str.data(), ERR_UNKNOWN); return error_s::make(err); }); - // retrive username from _conn. + // retrieve username from _conn. // If this is a sasl server, it gets the name of the corresponding sasl client. // But if this is a sasl client, it gets the name of itself char *username = nullptr; diff --git a/src/runtime/security/sasl_wrapper.h b/src/runtime/security/sasl_wrapper.h index 57bc7b231d..e22320e023 100644 --- a/src/runtime/security/sasl_wrapper.h +++ b/src/runtime/security/sasl_wrapper.h @@ -32,15 +32,18 @@ class sasl_wrapper virtual error_s start(const std::string &mechanism, const blob &input, blob &output) = 0; virtual error_s step(const blob &input, blob &output) = 0; /** - * retrive username from sasl connection. + * retrieve username from sasl connection. * If this is a sasl server, it gets the name of the corresponding sasl client. * But if this is a sasl client, it gets the name of itself **/ - error_s retrive_username(/*out*/ std::string &output); + error_s retrieve_username(/*out*/ std::string &output); protected: sasl_wrapper() = default; + + // wrap_error wraps a sasl error with full description. error_s wrap_error(int sasl_err); + sasl_conn_t *_conn = nullptr; }; diff --git a/src/runtime/security/server_negotiation.cpp b/src/runtime/security/server_negotiation.cpp index a8fae1b4d9..62dc929a2b 100644 --- a/src/runtime/security/server_negotiation.cpp +++ b/src/runtime/security/server_negotiation.cpp @@ -141,7 +141,7 @@ void server_negotiation::do_challenge(negotiation_rpc rpc, error_s err_s, const if (err_s.is_ok()) { std::string user_name; - auto retrive_err = _sasl->retrive_username(user_name); + auto retrive_err = _sasl->retrieve_username(user_name); if (retrive_err.is_ok()) { succ_negotiation(rpc, user_name); } else { diff --git a/src/runtime/security/server_negotiation.h b/src/runtime/security/server_negotiation.h index 5f25f12d65..ada7aad99f 100644 --- a/src/runtime/security/server_negotiation.h +++ b/src/runtime/security/server_negotiation.h @@ -25,12 +25,15 @@ namespace dsn { namespace security { extern const std::set supported_mechanisms; +// server_negotiation negotiates a session on server side. class server_negotiation : public negotiation { public: - server_negotiation(rpc_session_ptr session); + explicit server_negotiation(rpc_session_ptr session); - void start(); + void start() override; + + // handle_request handles negotiate_request from the session. void handle_request(negotiation_rpc rpc); private: diff --git a/src/runtime/test/server_negotiation_test.cpp b/src/runtime/test/server_negotiation_test.cpp index 394a605f07..175e99eaf6 100644 --- a/src/runtime/test/server_negotiation_test.cpp +++ b/src/runtime/test/server_negotiation_test.cpp @@ -143,7 +143,7 @@ TEST_F(server_negotiation_test, on_initiate) struct { std::string sasl_start_result; - std::string sasl_retrive_username_result; + std::string sasl_retrieve_username_result; negotiation_status::type req_status; negotiation_status::type resp_status; negotiation_status::type nego_status; @@ -180,8 +180,8 @@ TEST_F(server_negotiation_test, on_initiate) for (const auto &test : tests) { fail::setup(); fail::cfg("sasl_server_wrapper_start", "return(" + test.sasl_start_result + ")"); - fail::cfg("sasl_wrapper_retrive_username", - "return(" + test.sasl_retrive_username_result + ")"); + fail::cfg("sasl_wrapper_retrieve_username", + "return(" + test.sasl_retrieve_username_result + ")"); auto rpc = create_negotiation_rpc(test.req_status, ""); on_initiate(rpc); @@ -198,7 +198,7 @@ TEST_F(server_negotiation_test, on_challenge_resp) struct { std::string sasl_step_result; - std::string sasl_retrive_username_result; + std::string sasl_retrieve_username_result; negotiation_status::type req_status; negotiation_status::type resp_status; negotiation_status::type nego_status; @@ -233,8 +233,8 @@ TEST_F(server_negotiation_test, on_challenge_resp) for (const auto &test : tests) { fail::setup(); fail::cfg("sasl_server_wrapper_step", "return(" + test.sasl_step_result + ")"); - fail::cfg("sasl_wrapper_retrive_username", - "return(" + test.sasl_retrive_username_result + ")"); + fail::cfg("sasl_wrapper_retrieve_username", + "return(" + test.sasl_retrieve_username_result + ")"); auto rpc = create_negotiation_rpc(test.req_status, ""); on_challenge_resp(rpc); diff --git a/thirdparty/CMakeLists.txt b/thirdparty/CMakeLists.txt index 8ab17584e4..3c44aed6b6 100644 --- a/thirdparty/CMakeLists.txt +++ b/thirdparty/CMakeLists.txt @@ -9,6 +9,7 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") endif () include(ExternalProject) +include(CheckCXXCompilerFlag) set(TP_DIR ${PROJECT_SOURCE_DIR}) set(TP_OUTPUT ${PROJECT_SOURCE_DIR}/output) @@ -128,11 +129,15 @@ ExternalProject_Add(thrift DEPENDS boost ) +check_cxx_compiler_flag(-Wformat-overflow COMPILER_SUPPORTS_FORMAT_OVERFLOW) +if (COMPILER_SUPPORTS_FORMAT_OVERFLOW) + set(ZOOKEEPER_CFLAGS -Wno-error=format-overflow) +endif () ExternalProject_Add(zookeeper URL ${OSS_URL_PREFIX}/zookeeper-3.4.10.tar.gz http://ftp.jaist.ac.jp/pub/apache/zookeeper/zookeeper-3.4.10/zookeeper-3.4.10.tar.gz URL_MD5 e4cf1b1593ca870bf1c7a75188f09678 - CONFIGURE_COMMAND cd src/c && CFLAGS=-Wno-error=format ./configure --enable-static=yes --enable-shared=no --prefix=${TP_OUTPUT} --with-pic=yes + CONFIGURE_COMMAND cd src/c && CFLAGS=${ZOOKEEPER_CFLAGS} ./configure --enable-static=yes --enable-shared=no --prefix=${TP_OUTPUT} --with-pic=yes BUILD_COMMAND cd src/c && make INSTALL_COMMAND cd src/c && make install BUILD_IN_SOURCE 1