From 45d060da3ddc966d91c83e3e2aa68370863b757d 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) Conflicts: src/mon/MonClient.cc [ commit 1e9b18008c5e ("mon: set MonClient::_add_conn return type to void") not in nautilus ] --- 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 e0f3a7de952a6..8ae882bc1f667 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 cd1196a689ce2..b99691af7f0c7 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(bufferlist& bl) const override; diff --git a/src/auth/krb/KrbClientHandler.hpp b/src/auth/krb/KrbClientHandler.hpp index a8960a8898cbd..f1546dbb338c8 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 eb3ef8f552171..4cc4c8a0c933d 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 96bd816f7dceb..d57075f5797a4 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -681,6 +681,9 @@ MonConnection& 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()); + } auto inserted = pending_cons.insert(make_pair(peer, move(mc))); ldout(cct, 10) << "picked mon." << monmap.get_name(rank) << " con " << conn @@ -1552,9 +1555,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); @@ -1671,12 +1671,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) { @@ -1695,9 +1689,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