Fix Plaintext security protocol (#54)

* Refactor and improve plaintext marshallers
* Fix and refactor plaintext security protocol

Signed-off-by: Igor Egorov <[email protected]>

Author: Igor Egorov

include/libp2p/security/plaintext/exchange_message_marshaller.hpp

@@ -16,6 +16,10 @@
 
 namespace libp2p::security::plaintext {
 
+  namespace protobuf {
+    class Exchange;
+  }
+
   /**
    * Performs serializing of a Plaintext exchange message to Protobuf format and
    * deserializes it back
@@ -24,6 +28,22 @@ namespace libp2p::security::plaintext {
    public:
     virtual ~ExchangeMessageMarshaller() = default;
 
+    /**
+     * Converts handy Exchange message to its protobuf counterpart
+     * @param msg handy Exchange message
+     * @return protobuf Exchange message
+     */
+    virtual outcome::result<protobuf::Exchange> handyToProto(
+        const ExchangeMessage &msg) const = 0;
+
+    /**
+     * Converts protobuf Exchange message to its handy counterpart
+     * @param proto_msg protobuf Exchange message
+     * @return handy Exchange message
+     */
+    virtual outcome::result<std::pair<ExchangeMessage, crypto::ProtobufKey>>
+    protoToHandy(const protobuf::Exchange &proto_msg) const = 0;
+
     /**
      * @param msg exchange message to be marshalled to Protobuf
      * @returns a byte array containing a Protobuf representation of an exchange

include/libp2p/security/plaintext/exchange_message_marshaller_impl.hpp

@@ -26,22 +26,26 @@ namespace libp2p::security::plaintext {
     enum class Error {
       PUBLIC_KEY_SERIALIZING_ERROR = 1,
       MESSAGE_SERIALIZING_ERROR,
-      PUBLIC_KEY_DESERIALIZING_ERROR
+      PUBLIC_KEY_DESERIALIZING_ERROR,
+      MESSAGE_DESERIALIZING_ERROR,
     };
 
     explicit ExchangeMessageMarshallerImpl(
         std::shared_ptr<crypto::marshaller::KeyMarshaller> marshaller);
 
+    outcome::result<protobuf::Exchange> handyToProto(
+        const ExchangeMessage &msg) const override;
+
+    outcome::result<std::pair<ExchangeMessage, crypto::ProtobufKey>>
+    protoToHandy(const protobuf::Exchange &proto_msg) const override;
+
     outcome::result<std::vector<uint8_t>> marshal(
         const ExchangeMessage &msg) const override;
 
     outcome::result<std::pair<ExchangeMessage, crypto::ProtobufKey>> unmarshal(
         gsl::span<const uint8_t> msg_bytes) const override;
 
    private:
-    outcome::result<std::unique_ptr<crypto::protobuf::PublicKey>>
-    allocatePubKey(const crypto::PublicKey &pubkey) const;
-
     std::shared_ptr<crypto::marshaller::KeyMarshaller> marshaller_;
   };
 

include/libp2p/security/plaintext/plaintext.hpp

@@ -12,6 +12,10 @@
 #include <libp2p/security/plaintext/exchange_message_marshaller.hpp>
 #include <libp2p/security/security_adaptor.hpp>
 
+namespace libp2p::basic {
+  class ProtobufMessageReadWriter;
+}
+
 namespace libp2p::security {
   /**
    * Implementation of security adaptor, which creates plaintext connection.
@@ -51,11 +55,14 @@ namespace libp2p::security {
    private:
     using MaybePeerId = boost::optional<peer::PeerId>;
 
-    void sendExchangeMsg(const std::shared_ptr<connection::RawConnection> &conn,
-                         SecConnCallbackFunc cb) const;
+    void sendExchangeMsg(
+        const std::shared_ptr<connection::RawConnection> &conn,
+        const std::shared_ptr<basic::ProtobufMessageReadWriter> &rw,
+        SecConnCallbackFunc cb) const;
 
     void receiveExchangeMsg(
         const std::shared_ptr<connection::RawConnection> &conn,
+        const std::shared_ptr<basic::ProtobufMessageReadWriter> &rw,
         const MaybePeerId &p, SecConnCallbackFunc cb) const;
 
     // the callback passed to an async read call in receiveExchangeMsg

include/libp2p/security/secio/exchange_message_marshaller.hpp

@@ -34,7 +34,7 @@ namespace libp2p::security::secio {
         const ExchangeMessage &msg) const = 0;
 
     /**
-     * Converts protubuf Exchange message to its handy counterpart
+     * Converts protobuf Exchange message to its handy counterpart
      * @param proto_msg protobuf Exchange message
      * @return handy Exchange message
      */

src/security/plaintext/CMakeLists.txt

@@ -11,10 +11,11 @@ libp2p_add_library(p2p_plaintext
     )
 target_link_libraries(p2p_plaintext
     Boost::boost
-    p2p_security_error
-    p2p_plaintext_exchange_message_marshaller
     p2p_crypto_error
     p2p_logger
+    p2p_plaintext_exchange_message_marshaller
+    p2p_protobuf_message_read_writer
+    p2p_security_error
     )
 
 libp2p_add_library(p2p_plaintext_exchange_message_marshaller

src/security/plaintext/exchange_message_marshaller_impl.cpp

@@ -17,6 +17,8 @@ OUTCOME_CPP_DEFINE_CATEGORY(libp2p::security::plaintext,
       return "Error while encoding the plaintext exchange message to Protobuf "
              "format";
     case E::PUBLIC_KEY_DESERIALIZING_ERROR:
+      return "Error while decoding the public key from Protobuf format";
+    case E::MESSAGE_DESERIALIZING_ERROR:
       return "Error while decoding the plaintext exchange message from "
              "Protobuf format";
   }
@@ -29,21 +31,51 @@ namespace libp2p::security::plaintext {
       std::shared_ptr<crypto::marshaller::KeyMarshaller> marshaller)
       : marshaller_{std::move(marshaller)} {};
 
-  outcome::result<std::vector<uint8_t>> ExchangeMessageMarshallerImpl::marshal(
+  outcome::result<protobuf::Exchange>
+  ExchangeMessageMarshallerImpl::handyToProto(
       const ExchangeMessage &msg) const {
     plaintext::protobuf::Exchange exchange_msg;
 
-    OUTCOME_TRY(pubkey_msg, allocatePubKey(msg.pubkey));
-    exchange_msg.set_allocated_pubkey(pubkey_msg.get());
+    OUTCOME_TRY(proto_pubkey_bytes, marshaller_->marshal(msg.pubkey));
+    if (!exchange_msg.mutable_pubkey()->ParseFromArray(
+            proto_pubkey_bytes.key.data(), proto_pubkey_bytes.key.size())) {
+      return Error::PUBLIC_KEY_SERIALIZING_ERROR;
+    }
 
     auto id = msg.peer_id.toMultihash().toBuffer();
     exchange_msg.set_id(id.data(), id.size());
 
+    return outcome::success(std::move(exchange_msg));
+  }
+
+  outcome::result<std::pair<ExchangeMessage, crypto::ProtobufKey>>
+  ExchangeMessageMarshallerImpl::protoToHandy(
+      const protobuf::Exchange &proto_msg) const {
+    std::vector<uint8_t> pubkey_message_bytes(
+        proto_msg.pubkey().ByteSizeLong());
+    if (!proto_msg.pubkey().SerializeToArray(pubkey_message_bytes.data(),
+                                             pubkey_message_bytes.size())) {
+      return Error::PUBLIC_KEY_SERIALIZING_ERROR;
+    }
+    crypto::ProtobufKey proto_pubkey{pubkey_message_bytes};
+    OUTCOME_TRY(pubkey, marshaller_->unmarshalPublicKey(proto_pubkey));
+
+    std::vector<uint8_t> peer_id_bytes(proto_msg.id().begin(),
+                                       proto_msg.id().end());
+    OUTCOME_TRY(peer_id, peer::PeerId::fromBytes(peer_id_bytes));
+
+    return {ExchangeMessage{.pubkey = pubkey, .peer_id = peer_id},
+            proto_pubkey};
+  }
+
+  outcome::result<std::vector<uint8_t>> ExchangeMessageMarshallerImpl::marshal(
+      const ExchangeMessage &msg) const {
+    OUTCOME_TRY(exchange_msg, handyToProto(msg));
+
     std::vector<uint8_t> out_msg(exchange_msg.ByteSizeLong());
     if (!exchange_msg.SerializeToArray(out_msg.data(), out_msg.size())) {
       return Error::MESSAGE_SERIALIZING_ERROR;
     }
-    exchange_msg.release_pubkey();
     return out_msg;
   }
 
@@ -55,31 +87,7 @@ namespace libp2p::security::plaintext {
       return Error::PUBLIC_KEY_DESERIALIZING_ERROR;
     }
 
-    std::vector<uint8_t> pubkey_bytes(exchange_msg.pubkey().ByteSizeLong());
-    exchange_msg.pubkey().SerializeToArray(pubkey_bytes.data(),
-                                           pubkey_bytes.size());
-    OUTCOME_TRY(
-        pubkey,
-        marshaller_->unmarshalPublicKey(crypto::ProtobufKey{pubkey_bytes}));
-
-    std::vector<uint8_t> peer_id_bytes(exchange_msg.id().begin(),
-                                       exchange_msg.id().end());
-    OUTCOME_TRY(peer_id, peer::PeerId::fromBytes(peer_id_bytes));
-    return {ExchangeMessage{pubkey, peer_id},
-            crypto::ProtobufKey{std::move(pubkey_bytes)}};
-  }
-
-  outcome::result<std::unique_ptr<crypto::protobuf::PublicKey>>
-  ExchangeMessageMarshallerImpl::allocatePubKey(
-      const crypto::PublicKey &pubkey) const {
-    OUTCOME_TRY(proto_pub_key_bytes, marshaller_->marshal(pubkey));
-    std::string str_pubkey(proto_pub_key_bytes.key.begin(),
-                           proto_pub_key_bytes.key.end());
-    auto pubkey_msg = std::make_unique<crypto::protobuf::PublicKey>();
-    if (!pubkey_msg->ParseFromString(str_pubkey)) {
-      return Error::PUBLIC_KEY_SERIALIZING_ERROR;
-    }
-    return outcome::success(std::move(pubkey_msg));
+    return protoToHandy(exchange_msg);
   }
 
 }  // namespace libp2p::security::plaintext

src/security/plaintext/plaintext.cpp

@@ -7,29 +7,34 @@
 
 #include <functional>
 
+#include <generated/security/plaintext/protobuf/plaintext.pb.h>
+#include <libp2p/basic/protobuf_message_read_writer.hpp>
 #include <libp2p/peer/peer_id.hpp>
 #include <libp2p/security/error.hpp>
 #include <libp2p/security/plaintext/plaintext_connection.hpp>
 
 #if defined(__clang__) || defined(__GNUC__) || defined(__GNUG__)
-#	pragma GCC diagnostic ignored "-Wparentheses"
+#pragma GCC diagnostic ignored "-Wparentheses"
 #endif
 
-#define PLAINTEXT_OUTCOME_TRY(name, res, conn, cb) \
-  auto(name) = (res);                              \
-  if ((name).has_error()) {                        \
-    closeConnection(conn, (name).error());         \
-    cb((name).error());                            \
-    return;                                        \
+#define PLAINTEXT_OUTCOME_VOID_TRY(res, conn, cb) \
+  if ((res).has_error()) {                        \
+    closeConnection(conn, (res).error());         \
+    cb((res).error());                            \
+    return;                                       \
   }
 
+#define PLAINTEXT_OUTCOME_TRY(name, res, conn, cb) \
+  PLAINTEXT_OUTCOME_VOID_TRY((res), (conn), (cb))  \
+  auto(name) = (res).value();
+
 OUTCOME_CPP_DEFINE_CATEGORY(libp2p::security, Plaintext::Error, e) {
   using E = libp2p::security::Plaintext::Error;
   switch (e) {
     case E::EXCHANGE_SEND_ERROR:
       return "Error occured while sending Exchange message to the peer";
     case E::EXCHANGE_RECEIVE_ERROR:
-      return "Error occured while receiving Exchange message to the peer";
+      return "Error occurred while receiving Exchange message to the peer";
     case E::INVALID_PEER_ID:
       return "Received peer id doesn't match actual peer id";
     case E::EMPTY_PEER_ID:
@@ -61,93 +66,74 @@ namespace libp2p::security {
       std::shared_ptr<connection::RawConnection> inbound,
       SecConnCallbackFunc cb) {
     log_->debug("securing inbound connection");
-    sendExchangeMsg(inbound, cb);
-    receiveExchangeMsg(inbound, boost::none, cb);
+    auto rw = std::make_shared<basic::ProtobufMessageReadWriter>(inbound);
+    sendExchangeMsg(inbound, rw, cb);
+    receiveExchangeMsg(inbound, rw, boost::none, cb);
   }
 
   void Plaintext::secureOutbound(
       std::shared_ptr<connection::RawConnection> outbound,
       const peer::PeerId &p, SecConnCallbackFunc cb) {
     log_->debug("securing outbound connection");
-    sendExchangeMsg(outbound, cb);
-    receiveExchangeMsg(outbound, p, cb);
+    auto rw = std::make_shared<basic::ProtobufMessageReadWriter>(outbound);
+    sendExchangeMsg(outbound, rw, cb);
+    receiveExchangeMsg(outbound, rw, p, cb);
   }
 
   void Plaintext::sendExchangeMsg(
       const std::shared_ptr<connection::RawConnection> &conn,
+      const std::shared_ptr<basic::ProtobufMessageReadWriter> &rw,
       SecConnCallbackFunc cb) const {
-    PLAINTEXT_OUTCOME_TRY(out_msg_res,
-                          marshaller_->marshal(plaintext::ExchangeMessage{
-                              .pubkey = idmgr_->getKeyPair().publicKey,
-                              .peer_id = idmgr_->getId()}),
-                          conn, cb)
-
-    auto out_msg = out_msg_res.value();
-    auto len = out_msg.size();
-
-    std::vector<uint8_t> len_bytes = {
-        static_cast<uint8_t>(len >> 24u), static_cast<uint8_t>(len >> 16u),
-        static_cast<uint8_t>(len >> 8u), static_cast<uint8_t>(len)};
-
-    conn->write(len_bytes, 4,
-                [self{shared_from_this()}, out_msg, conn,
-                 cb{std::move(cb)}](auto &&res) mutable {
-                  if (res.has_error()) {
-                    self->closeConnection(conn, Error::EXCHANGE_SEND_ERROR);
-                    return cb(Error::EXCHANGE_SEND_ERROR);
-                  }
-
-                  conn->write(
-                      out_msg, out_msg.size(),
-                      [self{std::move(self)}, cb{cb}, conn](auto &&res) {
-                        if (res.has_error()) {
-                          self->closeConnection(conn,
-                                                Error::EXCHANGE_SEND_ERROR);
-                          return cb(Error::EXCHANGE_SEND_ERROR);
-                        }
-                      });
-                });
+    plaintext::ExchangeMessage exchange_msg{
+        .pubkey = idmgr_->getKeyPair().publicKey, .peer_id = idmgr_->getId()};
+    PLAINTEXT_OUTCOME_TRY(proto_exchange_msg,
+                          marshaller_->handyToProto(exchange_msg), conn, cb)
+
+    rw->write<plaintext::protobuf::Exchange>(
+        proto_exchange_msg,
+        [self{shared_from_this()}, cb{std::move(cb)}, conn](auto &&res) {
+          if (res.has_error()) {
+            self->closeConnection(conn, Error::EXCHANGE_SEND_ERROR);
+            return cb(Error::EXCHANGE_SEND_ERROR);
+          }
+        });
   }
 
   void Plaintext::receiveExchangeMsg(
       const std::shared_ptr<connection::RawConnection> &conn,
+      const std::shared_ptr<basic::ProtobufMessageReadWriter> &rw,
       const MaybePeerId &p, SecConnCallbackFunc cb) const {
-    constexpr size_t kMaxMsgSize = 4;  // we read uint32_t first
-    auto read_bytes = std::make_shared<std::vector<uint8_t>>(kMaxMsgSize);
-
-    conn->read(
-        *read_bytes, kMaxMsgSize,
+    auto remote_peer_exchange_bytes = std::make_shared<std::vector<uint8_t>>();
+    rw->read<plaintext::protobuf::Exchange>(
         [self{shared_from_this()}, conn, p, cb{std::move(cb)},
-         read_bytes](auto &&r) {
-          auto bytes_size = (static_cast<uint32_t>(read_bytes->at(0)) << 24u)
-              + (static_cast<uint32_t>(read_bytes->at(1)) << 16u)
-              + (static_cast<uint32_t>(read_bytes->at(2)) << 8u)
-              + read_bytes->at(3);
-
-          auto received_bytes =
-              std::make_shared<std::vector<uint8_t>>(bytes_size);
-          conn->read(*received_bytes, received_bytes->size(),
-                     [self, conn, p, cb, received_bytes](auto &&r) {
-                       self->readCallback(conn, p, cb, received_bytes, r);
-                     });
-        });
+         remote_peer_exchange_bytes](auto &&res) {
+          if (!res) {
+            return cb(res.error());
+          }
+          self->readCallback(conn, p, cb, remote_peer_exchange_bytes,
+                             remote_peer_exchange_bytes->size());
+        },
+        remote_peer_exchange_bytes);
   }
 
   void Plaintext::readCallback(
       const std::shared_ptr<connection::RawConnection> &conn,
       const MaybePeerId &p, const SecConnCallbackFunc &cb,
       const std::shared_ptr<std::vector<uint8_t>> &read_bytes,
       outcome::result<size_t> read_call_res) const {
-    PLAINTEXT_OUTCOME_TRY(r, read_call_res, conn, cb);
+    /*
+     * The method does redundant unmarshalling of message bytes to proto
+     * exchange message. This could be a subject of further improvement.
+     */
+    PLAINTEXT_OUTCOME_VOID_TRY(read_call_res, conn, cb);
     PLAINTEXT_OUTCOME_TRY(in_exchange_msg, marshaller_->unmarshal(*read_bytes),
                           conn, cb);
-    auto &msg = in_exchange_msg.value().first;
+    auto &msg = in_exchange_msg.first;
     auto received_pid = msg.peer_id;
     auto pkey = msg.pubkey;
 
     // PeerId is derived from the Protobuf-serialized public key, not a raw one
-    auto derived_pid_res =
-        peer::PeerId::fromPublicKey(in_exchange_msg.value().second);
+    auto derived_pid_res = peer::PeerId::fromPublicKey(in_exchange_msg.second);
     if (!derived_pid_res) {
       log_->error("cannot create a PeerId from the received public key: {}",
                   derived_pid_res.error().message());