From d5f52b2de7cea4ba3b6e6937a55423641fcc2b3f 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 octopus ] --- 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 6b225e4aca30d..0b4d7c8627d50 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 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 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 a165ccf74d227..8c3ac7bd4b79b 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -720,6 +720,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(std::make_pair(peer, std::move(mc))); ldout(cct, 10) << "picked mon." << monmap.get_name(rank) << " con " << conn @@ -1799,9 +1802,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); @@ -1922,12 +1922,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) { @@ -1946,9 +1940,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