From: Kefu Chai <kchai@redhat.com>
Date: Fri, 26 Apr 2019 08:09:01 +0000 (+0800)
Subject: crimson/mon: hold rotating_keyring using a pointer
X-Git-Tag: v15.1.0~2803^2~2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=870937b97924dc61241edaf87f56d8210949c4e5;p=ceph.git

crimson/mon: hold rotating_keyring using a pointer

we could have kept rotating_keyring as a plain member variable, but
`AuthClientHandler` keeps a weak reference to it, we are not able to
update it to point it to the new rotating_keyring in the newly created
active_con created from the connected pending connection -- the move
ctor of ceph::mon::Connection does create a new RotatingKeyring. so this
has two consequences:

- the raw pointer held by AuthClientHandler is not valid anymore, after
  the pending connection is destroyed when it is promoted to active_con.
  so we are writing to freed memory when renewing the rotating keyring.
- we won't have access to the updated keyring.

if we use a std::vector<unique_ptr<Connection>> for pending_conns, this
change won't be needed. we could make this change in a future change
though.

Signed-off-by: Kefu Chai <kchai@redhat.com>
---

diff --git a/src/crimson/mon/MonClient.cc b/src/crimson/mon/MonClient.cc
index cbc2cf7696c6..c5174f249255 100644
--- a/src/crimson/mon/MonClient.cc
+++ b/src/crimson/mon/MonClient.cc
@@ -110,7 +110,7 @@ private:
   const AuthRegistry& auth_registry;
   ceph::net::ConnectionRef conn;
   std::unique_ptr<AuthClientHandler> auth;
-  RotatingKeyRing rotating_keyring;
+  std::unique_ptr<RotatingKeyRing> rotating_keyring;
   uint64_t global_id;
   clock_t::time_point last_rotating_renew_sent;
 };
@@ -120,7 +120,10 @@ Connection::Connection(const AuthRegistry& auth_registry,
                        KeyRing* keyring)
   : auth_registry{auth_registry},
     conn{conn},
-    rotating_keyring{nullptr, CEPH_ENTITY_TYPE_OSD, keyring}
+    rotating_keyring{
+      std::make_unique<RotatingKeyRing>(nullptr,
+                                        CEPH_ENTITY_TYPE_OSD,
+                                        keyring)}
 {}
 
 seastar::future<> Connection::handle_auth_reply(Ref<MAuthReply> m)
@@ -148,7 +151,7 @@ seastar::future<> Connection::renew_rotating_keyring()
   auto ttl = std::chrono::seconds{
     static_cast<long>(ceph::common::local_conf()->auth_service_ticket_ttl)};
   auto cutoff = now - ttl / 4;
-  if (!rotating_keyring.need_new_secrets(utime_t(cutoff))) {
+  if (!rotating_keyring->need_new_secrets(utime_t(cutoff))) {
     return seastar::now();
   }
   if (now - last_rotating_renew_sent < std::chrono::seconds{1}) {
@@ -174,7 +177,7 @@ AuthAuthorizer* Connection::get_authorizer(peer_type_t peer) const
 }
 
 KeyStore& Connection::get_keys() {
-  return rotating_keyring;
+  return *rotating_keyring;
 }
 
 std::unique_ptr<AuthClientHandler>
@@ -187,7 +190,7 @@ Connection::create_auth(ceph::auth::method_t protocol,
   std::unique_ptr<AuthClientHandler> auth;
   auth.reset(AuthClientHandler::create(&cct,
                                        protocol,
-                                       &rotating_keyring));
+                                       rotating_keyring.get()));
   if (!auth) {
     logger().error("no handler for protocol {}", protocol);
     throw std::system_error(make_error_code(