mds: move session setup to ms_handle_accept

Session setup in ms_handle_authentication is (historically) racy where multiple
connections from the same client can come in before one is finally accepted.  A
session should only be created after ms_handle_accept. The MDS did some
backflips before this commit to ensure this.

Moreover, with the msgr2 changes, it is even more necessary since the address
nonce is not set until before ms_handle_accept is called.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

diff --git a/src/mds/MDSAuthCaps.h b/src/mds/MDSAuthCaps.h
index c21df082137ed6b666327f37a11ee8fe3b0bfca9..cc1006cdf669e5382deefda9edaca07071f5c638 100644 (file)
--- a/src/mds/MDSAuthCaps.h
+++ b/src/mds/MDSAuthCaps.h
@@ -52,7 +52,8 @@ struct MDSCapSpec {
   static const unsigned RWS            = (READ|WRITE|SNAPSHOT);
   static const unsigned RWPS           = (READ|WRITE|SET_VXATTR|SNAPSHOT);
 
-  MDSCapSpec(unsigned _caps=0) : caps(_caps) {
+  MDSCapSpec() = default;
+  MDSCapSpec(unsigned _caps) : caps(_caps) {
     if (caps & ALL)
       caps |= RWPS;
   }
@@ -84,7 +85,7 @@ struct MDSCapSpec {
     return (caps & SET_VXATTR);
   }
 private:
-  unsigned caps;
+  unsigned caps = 0;
 };
 
 // conditions before we are allowed to do it
@@ -153,16 +154,19 @@ struct MDSCapGrant {
 
 class MDSAuthCaps
 {
-  CephContext *cct;
+  CephContext *cct = nullptr;
   std::vector<MDSCapGrant> grants;
 
 public:
-  explicit MDSAuthCaps(CephContext *cct_=NULL)
-    : cct(cct_) { }
+  MDSAuthCaps() = default;
+  explicit MDSAuthCaps(CephContext *cct_) : cct(cct_) {}
 
   // this ctor is used by spirit/phoenix; doesn't need cct.
-  explicit MDSAuthCaps(const std::vector<MDSCapGrant> &grants_)
-    : cct(NULL), grants(grants_) { }
+  explicit MDSAuthCaps(const std::vector<MDSCapGrant>& grants_) : grants(grants_) {}
+
+  void clear() {
+    grants.clear();
+  }
 
   void set_allow_all();
   bool parse(CephContext *cct, std::string_view str, std::ostream *err);

diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc
index 0c77f0d70bcb73ed003118639d30d4cdf76dc61b..2ce3784e6c5882604193f7ad5508abc2cfdae065 100644 (file)
--- a/src/mds/MDSDaemon.cc
+++ b/src/mds/MDSDaemon.cc
@@ -1230,7 +1230,7 @@ bool MDSDaemon::ms_handle_reset(Connection *con)
   if (stopping) {
     return false;
   }
-  dout(5) << "ms_handle_reset on " << con->get_peer_addr() << dendl;
+  dout(5) << "ms_handle_reset on " << con->get_peer_socket_addr() << dendl;
   if (beacon.get_want_state() == CEPH_MDS_STATE_DNE)
     return false;
 
@@ -1258,7 +1258,7 @@ void MDSDaemon::ms_handle_remote_reset(Connection *con)
     return;
   }
 
-  dout(5) << "ms_handle_remote_reset on " << con->get_peer_addr() << dendl;
+  dout(5) << "ms_handle_remote_reset on " << con->get_peer_socket_addr() << dendl;
   if (beacon.get_want_state() == CEPH_MDS_STATE_DNE)
     return;
 
@@ -1283,11 +1283,47 @@ KeyStore *MDSDaemon::ms_get_auth1_authorizer_keystore()
   return monc->rotating_secrets.get();
 }
 
+bool MDSDaemon::parse_caps(const AuthCapsInfo& info, MDSAuthCaps& caps)
+{
+  caps.clear();
+  if (info.allow_all) {
+    caps.set_allow_all();
+    return true;
+  } else {
+    auto it = info.caps.begin();
+    string auth_cap_str;
+    try {
+      decode(auth_cap_str, it);
+    } catch (const buffer::error& e) {
+      dout(1) << __func__ << ": cannot decode auth caps buffer of length " << info.caps.length() << dendl;
+      return false;
+    }
+
+    dout(10) << __func__ << ": parsing auth_cap_str='" << auth_cap_str << "'" << dendl;
+    CachedStackStringStream cs;
+    if (caps.parse(g_ceph_context, auth_cap_str, cs.get())) {
+      return true;
+    } else {
+      dout(1) << __func__ << ": auth cap parse error: " << cs->strv() << " parsing '" << auth_cap_str << "'" << dendl;
+      return false;
+    }
+  }
+}
+
 int MDSDaemon::ms_handle_authentication(Connection *con)
 {
-  std::lock_guard l(mds_lock);
-  int ret = 0;
+  /* N.B. without mds_lock! */
+  MDSAuthCaps caps;
+  return parse_caps(con->get_peer_caps_info(), caps) ? 0 : -1;
+}
+
+void MDSDaemon::ms_handle_accept(Connection *con)
+{
   entity_name_t n(con->get_peer_type(), con->get_peer_global_id());
+  std::lock_guard l(mds_lock);
+  if (stopping) {
+    return;
+  }
 
   // We allow connections and assign Session instances to connections
   // even if we have not been assigned a rank, because clients with
@@ -1306,7 +1342,7 @@ int MDSDaemon::ms_handle_authentication(Connection *con)
   if (!s) {
     s = new Session(con);
     s->info.auth_name = con->get_peer_entity_name();
-    s->info.inst.addr = con->get_peer_addr();
+    s->info.inst.addr = con->get_peer_socket_addr();
     s->info.inst.name = n;
     dout(10) << " new session " << s << " for " << s->info.inst
             << " con " << con << dendl;
@@ -1319,64 +1355,11 @@ int MDSDaemon::ms_handle_authentication(Connection *con)
             << " existing con " << s->get_connection()
             << ", new/authorizing con " << con << dendl;
     con->set_priv(RefCountedPtr{s});
-
-    // Wait until we fully accept the connection before setting
-    // s->connection.  In particular, if there are multiple incoming
-    // connection attempts, they will all get their authorizer
-    // validated, but some of them may "lose the race" and get
-    // dropped.  We only want to consider the winner(s).  See
-    // ms_handle_accept().  This is important for Sessions we replay
-    // from the journal on recovery that don't have established
-    // messenger state; we want the con from only the winning
-    // connect attempt(s).  (Normal reconnects that don't follow MDS
-    // recovery are reconnected to the existing con by the
-    // messenger.)
-  }
-
-  AuthCapsInfo &caps_info = con->get_peer_caps_info();
-  if (caps_info.allow_all) {
-    // Flag for auth providers that don't provide cap strings
-    s->auth_caps.set_allow_all();
-  } else {
-    auto p = caps_info.caps.cbegin();
-    string auth_cap_str;
-    try {
-      decode(auth_cap_str, p);
-
-      dout(10) << __func__ << ": parsing auth_cap_str='" << auth_cap_str << "'"
-              << dendl;
-      std::ostringstream errstr;
-      if (!s->auth_caps.parse(g_ceph_context, auth_cap_str, &errstr)) {
-       dout(1) << __func__ << ": auth cap parse error: " << errstr.str()
-               << " parsing '" << auth_cap_str << "'" << dendl;
-       clog->warn() << name << " mds cap '" << auth_cap_str
-                    << "' does not parse: " << errstr.str();
-       ret = -EPERM;
-      } else {
-       ret = 1;
-      }
-    } catch (buffer::error& e) {
-      // Assume legacy auth, defaults to:
-      //  * permit all filesystem ops
-      //  * permit no `tell` ops
-      dout(1) << __func__ << ": cannot decode auth caps bl of length "
-             << caps_info.caps.length() << dendl;
-      ret = -EPERM;
-    }
   }
-  return ret;
-}
 
-void MDSDaemon::ms_handle_accept(Connection *con)
-{
-  std::lock_guard l(mds_lock);
-  if (stopping) {
-    return;
-  }
+  parse_caps(con->get_peer_caps_info(), s->auth_caps);
 
-  auto priv = con->get_priv();
-  auto s = static_cast<Session *>(priv.get());
-  dout(10) << "ms_handle_accept " << con->get_peer_addr() << " con " << con << " session " << s << dendl;
+  dout(10) << "ms_handle_accept " << con->get_peer_socket_addr() << " con " << con << " session " << s << dendl;
   if (s) {
     if (s->get_connection() != con) {
       dout(10) << " session connection " << s->get_connection()

diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h
index 130f1044ccf15496aef1b9f973639df61bb62f26..5cdcd5fa514c71fc9d9abb8c9a28b30c86b65c8a 100644 (file)
--- a/src/mds/MDSDaemon.h
+++ b/src/mds/MDSDaemon.h
@@ -178,6 +178,8 @@ private:
 
   static const std::vector<MDSCommand>& get_commands();
 
+  bool parse_caps(const AuthCapsInfo&, MDSAuthCaps&);
+
   mono_time starttime = mono_clock::zero();
 };
 

diff --git a/src/msg/Messenger.cc b/src/msg/Messenger.cc
index 7569acbb34155e22c5dc9a2527f789e38dc2d709..51c1c76b0ec33772b7b72191043c479a0b3dcad8 100644 (file)
--- a/src/msg/Messenger.cc
+++ b/src/msg/Messenger.cc
@@ -207,7 +207,7 @@ bool Messenger::ms_deliver_verify_authorizer(
        connection_secret,
        challenge);
       if (isvalid) {
-       dis->ms_handle_authentication(con);
+       return dis->ms_handle_authentication(con)>=0;
       }
       return true;
     }