refactor: fix typo and add comments to security (#699)

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:

src/runtime/security/negotiation.h

@@ -32,11 +32,12 @@ typedef rpc_holder<negotiation_request, negotiation_response> 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;

src/runtime/security/negotiation_manager.cpp

@@ -36,14 +36,15 @@ 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) ||
            is_http_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") {}
 
@@ -66,7 +67,7 @@ void negotiation_manager::on_negotiation_request(negotiation_rpc rpc)
 
     std::shared_ptr<negotiation> nego = get_negotiation(rpc);
     if (nullptr != nego) {
-        server_negotiation *srv_negotiation = static_cast<server_negotiation *>(nego.get());
+        auto srv_negotiation = static_cast<server_negotiation *>(nego.get());
         srv_negotiation->handle_request(rpc);
     }
 }
@@ -78,7 +79,7 @@ void negotiation_manager::on_negotiation_response(error_code err, negotiation_rp
 
     std::shared_ptr<negotiation> nego = get_negotiation(rpc);
     if (nullptr != nego) {
-        client_negotiation *cli_negotiation = static_cast<client_negotiation *>(nego.get());
+        auto cli_negotiation = static_cast<client_negotiation *>(nego.get());
         cli_negotiation->handle_response(err, std::move(rpc.response()));
     }
 }

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) {

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; // [

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

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;

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;
 };
 

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 {

src/runtime/security/server_negotiation.h

@@ -25,12 +25,15 @@ namespace dsn {
 namespace security {
 extern const std::set<std::string> 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:

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);

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