From 419cf3fcb69461ffe5f1352fe83fd68dff4f38a8 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Thu, 10 Aug 2023 16:25:34 -0400 Subject: [PATCH] mon/MonClient: handle ms_handle_fast_authentication return Fixes: https://tracker.ceph.com/issues/62382 Signed-off-by: Patrick Donnelly (cherry picked from commit bfbfbbfed6c02a913d0fcfd6a97071acafb95378) Conflicts: src/mon/Monitor.cc: missing state shutdown check --- src/mds/MDSDaemon.cc | 4 ++-- src/mds/MDSDaemon.h | 2 +- src/mgr/DaemonServer.cc | 8 +++---- src/mgr/DaemonServer.h | 2 +- src/mon/MonClient.cc | 11 +++++---- src/mon/Monitor.cc | 38 +++++++++++++++--------------- src/mon/Monitor.h | 2 +- src/msg/Dispatcher.h | 12 ++++++---- src/osd/OSD.cc | 33 +++++++++++++------------- src/osd/OSD.h | 4 ++-- src/test/fio/fio_ceph_messenger.cc | 4 ++-- src/test/msgr/perf_msgr_client.cc | 4 ++-- src/test/msgr/perf_msgr_server.cc | 4 ++-- src/test/msgr/test_msgr.cc | 12 +++++----- 14 files changed, 72 insertions(+), 68 deletions(-) diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 126a9a668e168..7542befdc556c 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -1083,11 +1083,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 e7929d2c37dec..d97b0ebf7e0a4 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 525f732fbe6db..15bd98a923d55 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -180,7 +180,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); @@ -205,17 +205,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 a7b6456100439..615057d7af1db 100644 --- a/src/mgr/DaemonServer.h +++ b/src/mgr/DaemonServer.h @@ -269,7 +269,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 fa74acb16192c..21c0a82ef2356 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -1601,8 +1601,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; } @@ -1639,8 +1640,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 0f6edb6070191..1c0423f5e571a 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -6361,7 +6361,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) { @@ -6482,7 +6484,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"; @@ -6520,12 +6524,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(); @@ -6548,11 +6552,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; @@ -6561,22 +6564,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 030e094784fbe..e49525a6ca427 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 885f1843b31c4..21c92f193c058 100644 --- a/src/msg/Dispatcher.h +++ b/src/msg/Dispatcher.h @@ -209,12 +209,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 8c937ea1d87f7..480df63f3d133 100644 --- a/src/osd/OSD.cc +++ b/src/osd/OSD.cc @@ -7594,9 +7594,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); @@ -7614,6 +7613,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; @@ -7623,23 +7623,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::do_waiters() diff --git a/src/osd/OSD.h b/src/osd/OSD.h index 52880bf4acea2..cceb1a47e8225 100644 --- a/src/osd/OSD.h +++ b/src/osd/OSD.h @@ -1530,7 +1530,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; @@ -1989,7 +1989,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 84c55176bd7ee..ab8b53a714ee2 100644 --- a/src/test/fio/fio_ceph_messenger.cc +++ b/src/test/fio/fio_ceph_messenger.cc @@ -270,8 +270,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 ffbfc1614fe73..003a9ade2f331 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 0c492ab174b7b..1d2267e1ab8ca 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 8766ac30e7b66..5ebe33a725587 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) { @@ -2267,8 +2267,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; } }; -- 2.39.5