From a4369b611d362ccdb9eb5ef319267d916a9e2ecb Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Mon, 8 Mar 2021 15:37:02 +0100 Subject: [PATCH] mon/MonClient: preserve auth state on reconnects Commit a2eb6ae3fb57 ("mon/monclient: hunt for multiple monitor in parallel") introduced a regression where auth state (global_id and AuthClientHandler) was no longer preserved on reconnects. The ensuing breakage was quickly noticed and prompted a follow-on fix 8bb6193c8f53 ("mon/MonClient: persist global_id across re-connecting"). However, as evident from the subject, the follow-on fix only took care of the global_id part. AuthClientHandler is still destroyed and all cephx tickets are discarded. A new from-scratch instance is created for each MonConnection and CEPHX_GET_AUTH_SESSION_KEY requests end up with CephXAuthenticate::old_ticket not populated. The bug is in MonClient, so both msgr1 and msgr2 are affected. This should have resulted in a similar sort of breakage but didn't because of a much larger bug. The monitor should have denied the attempt to reclaim global_id with no valid ticket proving previous possession of that global_id presented. Alas, it appears that this aspect of the cephx protocol has never been enforced. This is dealt with in the next patch. To fix the issue at hand, clone AuthClientHandler into each MonConnection so that each respective CEPHX_GET_AUTH_SESSION_KEY request gets a copy of the current auth ticket. Signed-off-by: Ilya Dryomov (cherry picked from commit 236b536b28482ec9d8b872de03da7d702ce4787b) --- src/auth/AuthClientHandler.h | 2 ++ src/auth/cephx/CephxClientHandler.h | 4 ++++ src/auth/krb/KrbClientHandler.hpp | 6 +++++- src/auth/none/AuthNoneClientHandler.h | 4 ++++ src/mon/MonClient.cc | 24 ++++++++++++------------ 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h index 3e2f73db049b4..c8989b1a0d9e6 100644 --- a/src/auth/AuthClientHandler.h +++ b/src/auth/AuthClientHandler.h @@ -37,6 +37,8 @@ public: {} virtual ~AuthClientHandler() {} + virtual AuthClientHandler* clone() const = 0; + void init(const EntityName& n) { name = n; } void set_want_keys(__u32 keys) { diff --git a/src/auth/cephx/CephxClientHandler.h b/src/auth/cephx/CephxClientHandler.h index 95a7e566428ea..601a5c69f4ba3 100644 --- a/src/auth/cephx/CephxClientHandler.h +++ b/src/auth/cephx/CephxClientHandler.h @@ -48,6 +48,10 @@ public: reset(); } + CephxClientHandler* clone() const override { + return new CephxClientHandler(*this); + } + void reset() override; void prepare_build_request() override; int build_request(ceph::buffer::list& bl) const override; diff --git a/src/auth/krb/KrbClientHandler.hpp b/src/auth/krb/KrbClientHandler.hpp index 9ab26a6905d8e..58e5311167a48 100644 --- a/src/auth/krb/KrbClientHandler.hpp +++ b/src/auth/krb/KrbClientHandler.hpp @@ -39,7 +39,11 @@ class KrbClientHandler : public AuthClientHandler { reset(); } ~KrbClientHandler() override; - + + KrbClientHandler* clone() const override { + return new KrbClientHandler(*this); + } + int get_protocol() const override { return CEPH_AUTH_GSS; } void reset() override { m_gss_client_name = GSS_C_NO_NAME; diff --git a/src/auth/none/AuthNoneClientHandler.h b/src/auth/none/AuthNoneClientHandler.h index 740ef0fc20d3c..66b4d59fc5dde 100644 --- a/src/auth/none/AuthNoneClientHandler.h +++ b/src/auth/none/AuthNoneClientHandler.h @@ -26,6 +26,10 @@ public: AuthNoneClientHandler(CephContext *cct_) : AuthClientHandler(cct_) {} + AuthNoneClientHandler* clone() const override { + return new AuthNoneClientHandler(*this); + } + void reset() override { } void prepare_build_request() override {} diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc index 7e60ce973b5c3..b807d378e49bb 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -750,6 +750,9 @@ void MonClient::_add_conn(unsigned rank, uint64_t global_id) auto peer = monmap.get_addrs(rank); auto conn = messenger->connect_to_mon(peer); MonConnection mc(cct, conn, global_id, &auth_registry); + if (auth) { + mc.get_auth().reset(auth->clone()); + } pending_cons.insert(std::make_pair(peer, std::move(mc))); ldout(cct, 10) << "picked mon." << monmap.get_name(rank) << " con " << conn @@ -1714,9 +1717,6 @@ int MonConnection::get_auth_request( return -EACCES; } - if (auth) { - auth.reset(); - } int r = _init_auth(*method, entity_name, want_keys, keyring, true); ceph_assert(r == 0); @@ -1837,12 +1837,6 @@ int MonConnection::_negotiate(MAuthReply *m, uint32_t want_keys, RotatingKeyRing* keyring) { - if (auth && (int)m->protocol == auth->get_protocol()) { - // good, negotiation completed - auth->reset(); - return 0; - } - int r = _init_auth(m->protocol, entity_name, want_keys, keyring, false); if (r == -ENOTSUP) { if (m->result == -ENOTSUP) { @@ -1861,9 +1855,15 @@ int MonConnection::_init_auth( RotatingKeyRing* keyring, bool msgr2) { - ldout(cct,10) << __func__ << " method " << method << dendl; - auth.reset( - AuthClientHandler::create(cct, method, keyring)); + ldout(cct, 10) << __func__ << " method " << method << dendl; + if (auth && auth->get_protocol() == (int)method) { + ldout(cct, 10) << __func__ << " already have auth, reseting" << dendl; + auth->reset(); + return 0; + } + + ldout(cct, 10) << __func__ << " creating new auth" << dendl; + auth.reset(AuthClientHandler::create(cct, method, keyring)); if (!auth) { ldout(cct, 10) << " no handler for protocol " << method << dendl; return -ENOTSUP; -- 2.39.5