From: Patrick Donnelly Date: Thu, 10 Aug 2023 20:25:34 +0000 (-0400) Subject: mon/MonClient: handle ms_handle_fast_authentication return X-Git-Tag: testing/wip-pdonnell-testing-20240703.143006-debug^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=bfbfbbfed6c02a913d0fcfd6a97071acafb95378;p=ceph-ci.git mon/MonClient: handle ms_handle_fast_authentication return Fixes: https://tracker.ceph.com/issues/62382 Signed-off-by: Patrick Donnelly --- diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 206daf9d5d8..f1a95280121 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -1151,11 +1151,11 @@ bool MDSDaemon::parse_caps(const AuthCapsInfo& info, MDSAuthCaps& caps) } } -int MDSDaemon::ms_handle_fast_authentication(Connection *con) +bool MDSDaemon::ms_handle_fast_authentication(Connection *con) { /* N.B. without mds_lock! */ MDSAuthCaps caps; - return parse_caps(con->get_peer_caps_info(), caps) ? 0 : -1; + return parse_caps(con->get_peer_caps_info(), caps); } void MDSDaemon::ms_handle_accept(Connection *con) diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h index 7f357abdfdd..c1999a32029 100644 --- a/src/mds/MDSDaemon.h +++ b/src/mds/MDSDaemon.h @@ -146,7 +146,7 @@ class MDSDaemon : public Dispatcher { private: bool ms_dispatch2(const ref_t &m) override; - int ms_handle_fast_authentication(Connection *con) override; + bool ms_handle_fast_authentication(Connection *con) override; void ms_handle_accept(Connection *con) override; void ms_handle_connect(Connection *con) override; bool ms_handle_reset(Connection *con) override; diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index f914a3152f9..ca335a744d4 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -257,7 +257,7 @@ entity_addrvec_t DaemonServer::get_myaddrs() const return msgr->get_myaddrs(); } -int DaemonServer::ms_handle_fast_authentication(Connection *con) +bool DaemonServer::ms_handle_fast_authentication(Connection *con) { auto s = ceph::make_ref(cct); con->set_priv(s); @@ -282,17 +282,17 @@ int DaemonServer::ms_handle_fast_authentication(Connection *con) catch (buffer::error& e) { dout(10) << " session " << s << " " << s->entity_name << " failed to decode caps" << dendl; - return -EACCES; + return false; } if (!s->caps.parse(str)) { dout(10) << " session " << s << " " << s->entity_name << " failed to parse caps '" << str << "'" << dendl; - return -EACCES; + return false; } dout(10) << " session " << s << " " << s->entity_name << " has caps " << s->caps << " '" << str << "'" << dendl; } - return 1; + return true; } void DaemonServer::ms_handle_accept(Connection* con) diff --git a/src/mgr/DaemonServer.h b/src/mgr/DaemonServer.h index 998cc7c8385..1193805def5 100644 --- a/src/mgr/DaemonServer.h +++ b/src/mgr/DaemonServer.h @@ -272,7 +272,7 @@ public: ~DaemonServer() override; bool ms_dispatch2(const ceph::ref_t& m) override; - int ms_handle_fast_authentication(Connection *con) override; + bool ms_handle_fast_authentication(Connection *con) override; void ms_handle_accept(Connection *con) override; bool ms_handle_reset(Connection *con) override; void ms_handle_remote_reset(Connection *con) override {} diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc index 0667c078b63..acda4f6d391 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -1609,8 +1609,9 @@ int MonClient::handle_auth_request( // for some channels prior to nautilus (osd heartbeat), we // tolerate the lack of an authorizer. if (!con->get_messenger()->require_authorizer) { - handle_authentication_dispatcher->ms_handle_fast_authentication(con); - return 1; + if (handle_authentication_dispatcher->ms_handle_fast_authentication(con)) { + return 1; + } } return -EACCES; } @@ -1647,8 +1648,10 @@ int MonClient::handle_auth_request( &auth_meta->connection_secret, ac); if (isvalid) { - handle_authentication_dispatcher->ms_handle_fast_authentication(con); - return 1; + if (handle_authentication_dispatcher->ms_handle_fast_authentication(con)) { + return 1; + } + return -EACCES; } if (!more && !was_challenge && auth_meta->authorizer_challenge) { ldout(cct,10) << __func__ << " added challenge on " << con << dendl; diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index a70bfbe33c9..4f632cc1ab7 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -6402,7 +6402,9 @@ int Monitor::handle_auth_request( &auth_meta->connection_secret, &auth_meta->authorizer_challenge); if (isvalid) { - ms_handle_fast_authentication(con); + if (!ms_handle_fast_authentication(con)) { + return -EACCES; + } return 1; } if (!more && !was_challenge && auth_meta->authorizer_challenge) { @@ -6523,7 +6525,9 @@ int Monitor::handle_auth_request( } if (r > 0 && !s->authenticated) { - ms_handle_fast_authentication(con); + if (!ms_handle_fast_authentication(con)) { + return -EACCES; + } } dout(30) << " r " << r << " reply:\n"; @@ -6561,12 +6565,12 @@ void Monitor::ms_handle_accept(Connection *con) } } -int Monitor::ms_handle_fast_authentication(Connection *con) +bool Monitor::ms_handle_fast_authentication(Connection *con) { if (con->get_peer_type() == CEPH_ENTITY_TYPE_MON) { // mon <-> mon connections need no Session, and setting one up // creates an awkward ref cycle between Session and Connection. - return 1; + return true; } auto priv = con->get_priv(); @@ -6576,7 +6580,7 @@ int Monitor::ms_handle_fast_authentication(Connection *con) if (state == STATE_SHUTDOWN) { dout(10) << __func__ << " ignoring new con " << con << " (shutdown)" << dendl; con->mark_down(); - return -EACCES; + return false; } s = session_map.new_session( entity_name_t(con->get_peer_type(), -1), // we don't know yet @@ -6594,11 +6598,10 @@ int Monitor::ms_handle_fast_authentication(Connection *con) << " " << *s << dendl; AuthCapsInfo &caps_info = con->get_peer_caps_info(); - int ret = 0; if (caps_info.allow_all) { s->caps.set_allow_all(); s->authenticated = true; - ret = 1; + return true; } else if (caps_info.caps.length()) { bufferlist::const_iterator p = caps_info.caps.cbegin(); string str; @@ -6607,22 +6610,19 @@ int Monitor::ms_handle_fast_authentication(Connection *con) } catch (const ceph::buffer::error &err) { derr << __func__ << " corrupt cap data for " << con->get_peer_entity_name() << " in auth db" << dendl; - str.clear(); - ret = -EACCES; + return false; } - if (ret >= 0) { - if (s->caps.parse(str, NULL)) { - s->authenticated = true; - ret = 1; - } else { - derr << __func__ << " unparseable caps '" << str << "' for " - << con->get_peer_entity_name() << dendl; - ret = -EACCES; - } + if (s->caps.parse(str, NULL)) { + s->authenticated = true; + return true; + } else { + derr << __func__ << " unparseable caps '" << str << "' for " + << con->get_peer_entity_name() << dendl; + return false; } + } else { + return false; } - - return ret; } void Monitor::set_mon_crush_location(const string& loc) diff --git a/src/mon/Monitor.h b/src/mon/Monitor.h index 13afacafde7..2958388e83a 100644 --- a/src/mon/Monitor.h +++ b/src/mon/Monitor.h @@ -957,7 +957,7 @@ public: MonCap mon_caps; bool get_authorizer(int dest_type, AuthAuthorizer **authorizer); public: // for AuthMonitor msgr1: - int ms_handle_fast_authentication(Connection *con) override; + bool ms_handle_fast_authentication(Connection *con) override; private: void ms_handle_accept(Connection *con) override; bool ms_handle_reset(Connection *con) override; diff --git a/src/msg/Dispatcher.h b/src/msg/Dispatcher.h index 799a4a9de91..8b7a65c795a 100644 --- a/src/msg/Dispatcher.h +++ b/src/msg/Dispatcher.h @@ -215,12 +215,14 @@ public: * * Do not acquire locks in this method! It is considered "fast" delivery. * - * return 1 for success - * return 0 for no action (let another Dispatcher handle it) - * return <0 for failure (failure to parse caps, for instance) + * Note: MonClient is the only caller of this method and it is configured + * to only call a single dispatcher. + * + * return true for success (auth succeeds for this stage of session construction) + * return false for failure (failure to parse caps, for instance) */ - virtual int ms_handle_fast_authentication(Connection *con) { - return 0; + [[nodiscard]] virtual bool ms_handle_fast_authentication(Connection *con) { + return false; } /** diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc index 2b59c7c87e4..b91b147c189 100644 --- a/src/osd/OSD.cc +++ b/src/osd/OSD.cc @@ -7645,9 +7645,8 @@ void OSD::ms_fast_dispatch(Message *m) OID_EVENT_TRACE_WITH_MSG(m, "MS_FAST_DISPATCH_END", false); } -int OSD::ms_handle_fast_authentication(Connection *con) +bool OSD::ms_handle_fast_authentication(Connection *con) { - int ret = 0; auto s = ceph::ref_cast(con->get_priv()); if (!s) { s = ceph::make_ref(cct, con); @@ -7665,6 +7664,7 @@ int OSD::ms_handle_fast_authentication(Connection *con) AuthCapsInfo &caps_info = con->get_peer_caps_info(); if (caps_info.allow_all) { s->caps.set_allow_all(); + return true; } else if (caps_info.caps.length() > 0) { bufferlist::const_iterator p = caps_info.caps.cbegin(); string str; @@ -7674,23 +7674,22 @@ int OSD::ms_handle_fast_authentication(Connection *con) catch (ceph::buffer::error& e) { dout(10) << __func__ << " session " << s << " " << s->entity_name << " failed to decode caps string" << dendl; - ret = -EACCES; - } - if (!ret) { - bool success = s->caps.parse(str); - if (success) { - dout(10) << __func__ << " session " << s - << " " << s->entity_name - << " has caps " << s->caps << " '" << str << "'" << dendl; - ret = 1; - } else { - dout(10) << __func__ << " session " << s << " " << s->entity_name - << " failed to parse caps '" << str << "'" << dendl; - ret = -EACCES; - } + return false; } + bool success = s->caps.parse(str); + if (success) { + dout(10) << __func__ << " session " << s + << " " << s->entity_name + << " has caps " << s->caps << " '" << str << "'" << dendl; + return true; + } else { + dout(10) << __func__ << " session " << s << " " << s->entity_name + << " failed to parse caps '" << str << "'" << dendl; + return false; + } + } else { + return false; } - return ret; } void OSD::_dispatch(Message *m) diff --git a/src/osd/OSD.h b/src/osd/OSD.h index 0ccfdb05d8e..51183bfe388 100644 --- a/src/osd/OSD.h +++ b/src/osd/OSD.h @@ -1507,7 +1507,7 @@ public: bool ms_handle_refused(Connection *con) override { return osd->ms_handle_refused(con); } - int ms_handle_fast_authentication(Connection *con) override { + bool ms_handle_fast_authentication(Connection *con) override { return true; } } heartbeat_dispatcher; @@ -1945,7 +1945,7 @@ private: void ms_handle_connect(Connection *con) override; void ms_handle_fast_connect(Connection *con) override; void ms_handle_fast_accept(Connection *con) override; - int ms_handle_fast_authentication(Connection *con) override; + bool ms_handle_fast_authentication(Connection *con) override; bool ms_handle_reset(Connection *con) override; void ms_handle_remote_reset(Connection *con) override {} bool ms_handle_refused(Connection *con) override; diff --git a/src/test/fio/fio_ceph_messenger.cc b/src/test/fio/fio_ceph_messenger.cc index 81680f102dd..604a92339b6 100644 --- a/src/test/fio/fio_ceph_messenger.cc +++ b/src/test/fio/fio_ceph_messenger.cc @@ -271,8 +271,8 @@ public: bool ms_handle_refused(Connection *con) override { return false; } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } }; diff --git a/src/test/msgr/perf_msgr_client.cc b/src/test/msgr/perf_msgr_client.cc index ffbfc1614fe..003a9ade2f3 100644 --- a/src/test/msgr/perf_msgr_client.cc +++ b/src/test/msgr/perf_msgr_client.cc @@ -57,8 +57,8 @@ class MessengerClient { bool ms_handle_reset(Connection *con) override { return true; } void ms_handle_remote_reset(Connection *con) override {} bool ms_handle_refused(Connection *con) override { return false; } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } }; diff --git a/src/test/msgr/perf_msgr_server.cc b/src/test/msgr/perf_msgr_server.cc index 0c492ab174b..1d2267e1ab8 100644 --- a/src/test/msgr/perf_msgr_server.cc +++ b/src/test/msgr/perf_msgr_server.cc @@ -100,8 +100,8 @@ class ServerDispatcher : public Dispatcher { //cerr << __func__ << " reply message=" << m << std::endl; op_wq.queue(m); } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } }; diff --git a/src/test/msgr/test_msgr.cc b/src/test/msgr/test_msgr.cc index f702cc288ca..07f812589d1 100644 --- a/src/test/msgr/test_msgr.cc +++ b/src/test/msgr/test_msgr.cc @@ -220,8 +220,8 @@ class FakeDispatcher : public Dispatcher { cond.notify_all(); } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } void reply_message(Message *m) { @@ -1709,8 +1709,8 @@ class SyntheticDispatcher : public Dispatcher { } } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } void reply_message(const Message *m, Payload& pl) { @@ -2322,8 +2322,8 @@ class MarkdownDispatcher : public Dispatcher { void ms_fast_dispatch(Message *m) override { ceph_abort(); } - int ms_handle_fast_authentication(Connection *con) override { - return 1; + bool ms_handle_fast_authentication(Connection *con) override { + return true; } };