From 0c9aec9cd37b92f57d1736be945340a94ef01d09 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Fri, 27 Mar 2020 15:12:13 +0800 Subject: [PATCH] crimson/net: audit peer_name(type, id) during handshake Allow connect to specific peer with entity_name_t, with required internal validation during handshake in v2. Signed-off-by: Yingxin Cheng --- src/crimson/net/Connection.h | 33 ++++++++++++++--- src/crimson/net/Messenger.h | 8 ++++- src/crimson/net/Protocol.h | 2 +- src/crimson/net/ProtocolV1.cc | 13 +++++-- src/crimson/net/ProtocolV1.h | 2 +- src/crimson/net/ProtocolV2.cc | 55 +++++++++++++++++++++++++---- src/crimson/net/ProtocolV2.h | 3 +- src/crimson/net/SocketConnection.cc | 4 +-- src/crimson/net/SocketConnection.h | 2 +- src/crimson/net/SocketMessenger.cc | 4 +-- src/crimson/net/SocketMessenger.h | 2 +- 11 files changed, 105 insertions(+), 23 deletions(-) diff --git a/src/crimson/net/Connection.h b/src/crimson/net/Connection.h index 04c57cb120f..25b3f5af562 100644 --- a/src/crimson/net/Connection.h +++ b/src/crimson/net/Connection.h @@ -29,7 +29,7 @@ class Interceptor; using seq_num_t = uint64_t; class Connection : public seastar::enable_shared_from_this { - entity_name_t peer_name = {0, -1}; + entity_name_t peer_name = {0, entity_name_t::NEW}; protected: entity_addr_t peer_addr; @@ -42,9 +42,34 @@ class Connection : public seastar::enable_shared_from_this { clock_t::time_point last_keepalive; clock_t::time_point last_keepalive_ack; - void set_peer_type(entity_type_t peer_type) { peer_name._type = peer_type; } - void set_peer_id(int64_t peer_id) { peer_name._num = peer_id; } - void set_peer_name(entity_name_t name) { peer_name = name; } + void set_peer_type(entity_type_t peer_type) { + // it is not allowed to assign an unknown value when the current + // value is known + assert(!(peer_type == 0 && + peer_name.type() != 0)); + // it is not allowed to assign a different known value when the + // current value is also known. + assert(!(peer_type != 0 && + peer_name.type() != 0 && + peer_type != peer_name.type())); + peer_name._type = peer_type; + } + void set_peer_id(int64_t peer_id) { + // it is not allowed to assign an unknown value when the current + // value is known + assert(!(peer_id == entity_name_t::NEW && + peer_name.num() != entity_name_t::NEW)); + // it is not allowed to assign a different known value when the + // current value is also known. + assert(!(peer_id != entity_name_t::NEW && + peer_name.num() != entity_name_t::NEW && + peer_id != peer_name.num())); + peer_name._num = peer_id; + } + void set_peer_name(entity_name_t name) { + set_peer_type(name.type()); + set_peer_id(name.num()); + } public: uint64_t peer_global_id = 0; diff --git a/src/crimson/net/Messenger.h b/src/crimson/net/Messenger.h index 0ad93ea35c8..6423f89d967 100644 --- a/src/crimson/net/Messenger.h +++ b/src/crimson/net/Messenger.h @@ -78,7 +78,13 @@ public: /// or a new pending connection virtual ConnectionRef connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type) = 0; + const entity_name_t& peer_name) = 0; + + ConnectionRef + connect(const entity_addr_t& peer_addr, + const entity_type_t& peer_type) { + return connect(peer_addr, entity_name_t(peer_type, -1)); + } // wait for messenger shutdown virtual seastar::future<> wait() = 0; diff --git a/src/crimson/net/Protocol.h b/src/crimson/net/Protocol.h index b765b23539d..a6c922c33f1 100644 --- a/src/crimson/net/Protocol.h +++ b/src/crimson/net/Protocol.h @@ -41,7 +41,7 @@ class Protocol { } virtual void start_connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type) = 0; + const entity_name_t& peer_name) = 0; virtual void start_accept(SocketRef&& socket, const entity_addr_t& peer_addr) = 0; diff --git a/src/crimson/net/ProtocolV1.cc b/src/crimson/net/ProtocolV1.cc index b22ce30653a..a5a813a81a6 100644 --- a/src/crimson/net/ProtocolV1.cc +++ b/src/crimson/net/ProtocolV1.cc @@ -302,7 +302,7 @@ ProtocolV1::repeat_connect() } void ProtocolV1::start_connect(const entity_addr_t& _peer_addr, - const entity_type_t& _peer_type) + const entity_name_t& _peer_name) { ceph_assert(state == state_t::none); logger().trace("{} trigger connecting, was {}", conn, static_cast(state)); @@ -312,8 +312,8 @@ void ProtocolV1::start_connect(const entity_addr_t& _peer_addr, ceph_assert(!socket); conn.peer_addr = _peer_addr; conn.target_addr = _peer_addr; - conn.set_peer_type(_peer_type); - conn.policy = messenger.get_policy(_peer_type); + conn.set_peer_name(_peer_name); + conn.policy = messenger.get_policy(_peer_name.type()); messenger.register_conn( seastar::static_pointer_cast(conn.shared_from_this())); gated_dispatch("start_connect", [this] { @@ -534,6 +534,13 @@ seastar::future ProtocolV1::repeat_handle_connect() .then([this](bufferlist bl) { auto p = bl.cbegin(); ::decode(h.connect, p); + if (conn.get_peer_type() != 0 && + conn.get_peer_type() != h.connect.host_type) { + logger().error("{} repeat_handle_connect(): my peer type does not match" + " what peer advertises {} != {}", + conn, conn.get_peer_type(), h.connect.host_type); + throw std::system_error(make_error_code(error::protocol_aborted)); + } conn.set_peer_type(h.connect.host_type); conn.policy = messenger.get_policy(h.connect.host_type); if (!conn.policy.lossy && !conn.policy.server && conn.target_addr.get_port() <= 0) { diff --git a/src/crimson/net/ProtocolV1.h b/src/crimson/net/ProtocolV1.h index 2ebfcc9ca92..31a6ddc2eae 100644 --- a/src/crimson/net/ProtocolV1.h +++ b/src/crimson/net/ProtocolV1.h @@ -19,7 +19,7 @@ class ProtocolV1 final : public Protocol { private: void start_connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type) override; + const entity_name_t& peer_name) override; void start_accept(SocketRef&& socket, const entity_addr_t& peer_addr) override; diff --git a/src/crimson/net/ProtocolV2.cc b/src/crimson/net/ProtocolV2.cc index b6d35514d59..7f5ff573b82 100644 --- a/src/crimson/net/ProtocolV2.cc +++ b/src/crimson/net/ProtocolV2.cc @@ -154,18 +154,18 @@ ProtocolV2::ProtocolV2(Dispatcher& dispatcher, ProtocolV2::~ProtocolV2() {} void ProtocolV2::start_connect(const entity_addr_t& _peer_addr, - const entity_type_t& _peer_type) + const entity_name_t& _peer_name) { ceph_assert(state == state_t::NONE); ceph_assert(!socket); conn.peer_addr = _peer_addr; conn.target_addr = _peer_addr; - conn.set_peer_type(_peer_type); - conn.policy = messenger.get_policy(_peer_type); + conn.set_peer_name(_peer_name); + conn.policy = messenger.get_policy(_peer_name.type()); client_cookie = generate_client_cookie(); - logger().info("{} ProtocolV2::start_connect(): peer_addr={}, peer_type={}, cc={}" + logger().info("{} ProtocolV2::start_connect(): peer_addr={}, peer_name={}, cc={}" " policy(lossy={}, server={}, standby={}, resetcheck={})", - conn, _peer_addr, ceph_entity_type_name(_peer_type), client_cookie, + conn, _peer_addr, _peer_name, client_cookie, conn.policy.lossy, conn.policy.server, conn.policy.standby, conn.policy.resetcheck); messenger.register_conn( @@ -744,6 +744,13 @@ ProtocolV2::client_connect() throw std::system_error( make_error_code(crimson::net::error::bad_peer_address)); } + if (conn.get_peer_id() != entity_name_t::NEW && + conn.get_peer_id() != server_ident.gid()) { + logger().error("{} connection peer id ({}) does not match " + "what it should be ({}) during connecting, close", + conn, server_ident.gid(), conn.get_peer_id()); + abort_in_close(*this, true); + } conn.set_peer_id(server_ident.gid()); conn.set_features(server_ident.supported_features() & conn.policy.features_supported); @@ -1075,6 +1082,20 @@ seastar::future<> ProtocolV2::server_auth() }); } +bool ProtocolV2::validate_peer_name(const entity_name_t& peer_name) const +{ + auto my_peer_name = conn.get_peer_name(); + if (my_peer_name.type() != peer_name.type()) { + return false; + } + if (my_peer_name.num() != entity_name_t::NEW && + peer_name.num() != entity_name_t::NEW && + my_peer_name.num() != peer_name.num()) { + return false; + } + return true; +} + seastar::future ProtocolV2::send_wait() { @@ -1131,6 +1152,12 @@ ProtocolV2::handle_existing_connection(SocketConnectionRef existing_conn) existing_proto->client_cookie, existing_proto->server_cookie); + if (!validate_peer_name(existing_conn->get_peer_name())) { + logger().error("{} server_connect: my peer_name doesn't match" + " the existing connection {}, abort", conn, existing_conn); + abort_in_fault(); + } + if (existing_proto->state == state_t::REPLACING) { logger().warn("{} server_connect: racing replace happened while" " replacing existing connection {}, send wait.", @@ -1252,6 +1279,13 @@ ProtocolV2::server_connect() make_error_code(crimson::net::error::bad_peer_address)); } + if (conn.get_peer_id() != entity_name_t::NEW && + conn.get_peer_id() != client_ident.gid()) { + logger().error("{} client_ident peer_id ({}) does not match" + " what it should be ({}) during accepting, abort", + conn, client_ident.gid(), conn.get_peer_id()); + abort_in_fault(); + } conn.set_peer_id(client_ident.gid()); client_cookie = client_ident.cookie(); @@ -1409,6 +1443,12 @@ ProtocolV2::server_reconnect() existing_proto->client_cookie, existing_proto->server_cookie); + if (!validate_peer_name(existing_conn->get_peer_name())) { + logger().error("{} server_reconnect: my peer_name doesn't match" + " the existing connection {}, abort", conn, existing_conn); + abort_in_fault(); + } + if (existing_proto->state == state_t::REPLACING) { logger().warn("{} server_reconnect: racing replace happened while " " replacing existing connection {}, retry global.", @@ -1763,7 +1803,10 @@ void ProtocolV2::trigger_replacing(bool reconnect, return write_frame(reconnect_ok); } else { client_cookie = new_client_cookie; - conn.set_peer_name(new_peer_name); + assert(conn.get_peer_type() == new_peer_name.type()); + if (conn.get_peer_id() == entity_name_t::NEW) { + conn.set_peer_id(new_peer_name.num()); + } connection_features = new_conn_features; return send_server_ident(); } diff --git a/src/crimson/net/ProtocolV2.h b/src/crimson/net/ProtocolV2.h index f98bf3d4366..5d3a6742181 100644 --- a/src/crimson/net/ProtocolV2.h +++ b/src/crimson/net/ProtocolV2.h @@ -20,7 +20,7 @@ class ProtocolV2 final : public Protocol { private: void start_connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type) override; + const entity_name_t& peer_name) override; void start_accept(SocketRef&& socket, const entity_addr_t& peer_addr) override; @@ -159,6 +159,7 @@ class ProtocolV2 final : public Protocol { seastar::future<> _handle_auth_request(bufferlist& auth_payload, bool more); seastar::future<> server_auth(); + bool validate_peer_name(const entity_name_t& peer_name) const; seastar::future send_wait(); seastar::future reuse_connection(ProtocolV2* existing_proto, bool do_reset=false, diff --git a/src/crimson/net/SocketConnection.cc b/src/crimson/net/SocketConnection.cc index 6d84acf1469..4e80b8104d1 100644 --- a/src/crimson/net/SocketConnection.cc +++ b/src/crimson/net/SocketConnection.cc @@ -114,9 +114,9 @@ bool SocketConnection::update_rx_seq(seq_num_t seq) void SocketConnection::start_connect(const entity_addr_t& _peer_addr, - const entity_type_t& _peer_type) + const entity_name_t& _peer_name) { - protocol->start_connect(_peer_addr, _peer_type); + protocol->start_connect(_peer_addr, _peer_name); } void diff --git a/src/crimson/net/SocketConnection.h b/src/crimson/net/SocketConnection.h index 358e8e00807..90b84181995 100644 --- a/src/crimson/net/SocketConnection.h +++ b/src/crimson/net/SocketConnection.h @@ -83,7 +83,7 @@ class SocketConnection : public Connection { /// start a handshake from the client's perspective, /// only call when SocketConnection first construct void start_connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type); + const entity_name_t& peer_name); /// start a handshake from the server's perspective, /// only call when SocketConnection first construct void start_accept(SocketRef&& socket, diff --git a/src/crimson/net/SocketMessenger.cc b/src/crimson/net/SocketMessenger.cc index c11e2b32753..d6cab609734 100644 --- a/src/crimson/net/SocketMessenger.cc +++ b/src/crimson/net/SocketMessenger.cc @@ -132,7 +132,7 @@ seastar::future<> SocketMessenger::start(Dispatcher *disp) { } crimson::net::ConnectionRef -SocketMessenger::connect(const entity_addr_t& peer_addr, const entity_type_t& peer_type) +SocketMessenger::connect(const entity_addr_t& peer_addr, const entity_name_t& peer_name) { assert(seastar::engine().cpu_id() == master_sid); @@ -146,7 +146,7 @@ SocketMessenger::connect(const entity_addr_t& peer_addr, const entity_type_t& pe } SocketConnectionRef conn = seastar::make_shared( *this, *dispatcher, peer_addr.is_msgr2()); - conn->start_connect(peer_addr, peer_type); + conn->start_connect(peer_addr, peer_name); return conn->shared_from_this(); } diff --git a/src/crimson/net/SocketMessenger.h b/src/crimson/net/SocketMessenger.h index d1b86e16a2a..9419e0a15c3 100644 --- a/src/crimson/net/SocketMessenger.h +++ b/src/crimson/net/SocketMessenger.h @@ -66,7 +66,7 @@ class SocketMessenger final : public Messenger { seastar::future<> start(Dispatcher *dispatcher) override; ConnectionRef connect(const entity_addr_t& peer_addr, - const entity_type_t& peer_type) override; + const entity_name_t& peer_name) override; // can only wait once seastar::future<> wait() override { assert(seastar::engine().cpu_id() == master_sid); -- 2.39.5