From 003289d4b9106d78a81d313c2200c6f9df8e3a95 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Tue, 7 Jun 2016 09:38:00 +0800 Subject: [PATCH 1/8] auth/cephx: fix race conditon for some public methods of KeyServer These methods are called only by AuthMonitor and are accessed without protection of internal lock, which is not safe. Signed-off-by: xie xingguo --- src/auth/cephx/CephxKeyServer.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/auth/cephx/CephxKeyServer.h b/src/auth/cephx/CephxKeyServer.h index 76b1c13630578..b742f052f527c 100644 --- a/src/auth/cephx/CephxKeyServer.h +++ b/src/auth/cephx/CephxKeyServer.h @@ -241,10 +241,12 @@ class KeyServer : public KeyStore { } void clear_secrets() { + Mutex::Locker l(lock); data.clear_secrets(); } void apply_data_incremental(KeyServerData::Incremental& inc) { + Mutex::Locker l(lock); data.apply_incremental(inc); } void set_ver(version_t ver) { @@ -267,6 +269,7 @@ class KeyServer : public KeyStore { return (b != data.secrets_end()); } int get_num_secrets() { + Mutex::Locker l(lock); return data.secrets.size(); } @@ -280,6 +283,7 @@ class KeyServer : public KeyStore { dst = data; } void export_keyring(KeyRing& keyring) { + Mutex::Locker l(lock); for (map::iterator p = data.secrets.begin(); p != data.secrets.end(); ++p) { From d55f43f1b378a544864743ed3531dcd7ad566e0f Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Mon, 6 Jun 2016 15:51:18 +0800 Subject: [PATCH 2/8] auth: return error if we are unable to parse keyring file Signed-off-by: xie xingguo --- src/auth/KeyRing.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/auth/KeyRing.cc b/src/auth/KeyRing.cc index 527076df025f9..b946a298b48bb 100644 --- a/src/auth/KeyRing.cc +++ b/src/auth/KeyRing.cc @@ -233,6 +233,7 @@ int KeyRing::load(CephContext *cct, const std::string &filename) } catch (const buffer::error& err) { lderr(cct) << "error parsing file " << filename << dendl; + return -EIO; } ldout(cct, 2) << "KeyRing::load: loaded key file " << filename << dendl; From 5acb47fdb25f147e8f099c1a8b45a5c3888f6264 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Mon, 6 Jun 2016 19:24:01 +0800 Subject: [PATCH 3/8] mon/AuthMonitor: fix wrongly error handling logic We shall set err correctly instead of rs here... Signed-off-by: xie xingguo --- src/mon/AuthMonitor.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc index 9bfc81434f804..0358fa06c778d 100644 --- a/src/mon/AuthMonitor.cc +++ b/src/mon/AuthMonitor.cc @@ -721,7 +721,7 @@ bool AuthMonitor::prepare_command(MonOpRequestRef op) ::decode(keyring, iter); } catch (const buffer::error &ex) { ss << "error decoding keyring" << " " << ex.what(); - rs = err; + err = -EINVAL; goto done; } import_keyring(keyring); From 9eab1d99dae48ce31482e030c3a3c2f0ca9ec3ae Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Mon, 6 Jun 2016 19:39:54 +0800 Subject: [PATCH 4/8] auth/cephx: process formatter dump more tenderly E.g., if there is no secrets, we don't open a session and leave it haning after returning. Signed-off-by: xie xingguo --- src/auth/cephx/CephxKeyServer.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/auth/cephx/CephxKeyServer.cc b/src/auth/cephx/CephxKeyServer.cc index 28b1437539586..8aafe07ce625e 100644 --- a/src/auth/cephx/CephxKeyServer.cc +++ b/src/auth/cephx/CephxKeyServer.cc @@ -296,15 +296,14 @@ bool KeyServer::contains(const EntityName& name) const int KeyServer::encode_secrets(Formatter *f, stringstream *ds) const { Mutex::Locker l(lock); - - if (f) - f->open_array_section("auth_dump"); - map::const_iterator mapiter = data.secrets_begin(); if (mapiter == data.secrets_end()) return -ENOENT; + if (f) + f->open_array_section("auth_dump"); + while (mapiter != data.secrets_end()) { const EntityName& name = mapiter->first; if (ds) { From 5ed15148e508830e4a723f358f4f0cb86c7857c7 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Mon, 6 Jun 2016 19:55:22 +0800 Subject: [PATCH 5/8] auth/cephx: fix race condition for build_session_auth_info() Signed-off-by: xie xingguo --- src/auth/cephx/CephxKeyServer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/auth/cephx/CephxKeyServer.cc b/src/auth/cephx/CephxKeyServer.cc index 8aafe07ce625e..cda1554375d99 100644 --- a/src/auth/cephx/CephxKeyServer.cc +++ b/src/auth/cephx/CephxKeyServer.cc @@ -457,6 +457,7 @@ int KeyServer::build_session_auth_info(uint32_t service_id, CephXServiceTicketIn info.service_secret = service_secret; info.secret_id = secret_id; + Mutex::Locker l(lock); return _build_session_auth_info(service_id, auth_ticket_info, info); } From 3655b0012737bb659a22d595b8db5b48e53faa7f Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Tue, 7 Jun 2016 10:17:53 +0800 Subject: [PATCH 6/8] auth/cephx: return error if we are unable to decode rotate-key Signed-off-by: xie xingguo --- src/auth/cephx/CephxClientHandler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/auth/cephx/CephxClientHandler.cc b/src/auth/cephx/CephxClientHandler.cc index a314608cc2336..fe1f87a8510ce 100644 --- a/src/auth/cephx/CephxClientHandler.cc +++ b/src/auth/cephx/CephxClientHandler.cc @@ -167,7 +167,7 @@ int CephxClientHandler::handle_response(int ret, bufferlist::iterator& indata) if (decode_decrypt(cct, secrets, secret_key, indata, error)) { ldout(cct, 0) << "could not set rotating key: decode_decrypt failed. error:" << error << dendl; - error.clear(); + return -EINVAL; } else { rotating_secrets->set_secrets(secrets); } From b8af5f75a44923ac25fdbf968e66c8c0c5e199d5 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Tue, 7 Jun 2016 09:28:50 +0800 Subject: [PATCH 7/8] auth/cephx: kill dead code Signed-off-by: xie xingguo --- src/auth/AuthClientHandler.h | 5 ----- src/auth/cephx/CephxKeyServer.h | 5 ----- src/mon/MonClient.h | 6 ------ 3 files changed, 16 deletions(-) diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h index 0bebb74c447f8..69fbd6aeadeea 100644 --- a/src/auth/AuthClientHandler.h +++ b/src/auth/AuthClientHandler.h @@ -47,11 +47,6 @@ class AuthClientHandler { want = keys | CEPH_ENTITY_TYPE_AUTH; validate_tickets(); } - void add_want_keys(__u32 keys) { - RWLock::WLocker l(lock); - want |= keys; - validate_tickets(); - } virtual int get_protocol() const = 0; diff --git a/src/auth/cephx/CephxKeyServer.h b/src/auth/cephx/CephxKeyServer.h index b742f052f527c..8e1bd183dcd59 100644 --- a/src/auth/cephx/CephxKeyServer.h +++ b/src/auth/cephx/CephxKeyServer.h @@ -273,11 +273,6 @@ class KeyServer : public KeyStore { return data.secrets.size(); } - /*void add_rotating_secret(uint32_t service_id, ExpiringCryptoKey& key) { - Mutex::Locker l(lock); - data.add_rotating_secret(service_id, key); - } - */ void clone_to(KeyServerData& dst) const { Mutex::Locker l(lock); dst = data; diff --git a/src/mon/MonClient.h b/src/mon/MonClient.h index 4e08ceb6d6c0a..2c7051bdb183f 100644 --- a/src/mon/MonClient.h +++ b/src/mon/MonClient.h @@ -382,12 +382,6 @@ class MonClient : public Dispatcher { auth->set_want_keys(want | CEPH_ENTITY_TYPE_MON); } - void add_want_keys(uint32_t want) { - want_keys |= want; - if (auth) - auth->add_want_keys(want); - } - // admin commands private: uint64_t last_mon_command_tid; From 39f26f864192a6de9ce55b65841e924a92ff0ea0 Mon Sep 17 00:00:00 2001 From: xie xingguo Date: Sun, 12 Jun 2016 16:27:07 +0800 Subject: [PATCH 8/8] mon/AuthMonitor: fix assert of version Version shall always be greater than keys_ver below here, otherwise the prior code logic shall prevent us from going this far. Signed-off-by: xie xingguo --- src/mon/AuthMonitor.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc index 0358fa06c778d..f5622a2207c0b 100644 --- a/src/mon/AuthMonitor.cc +++ b/src/mon/AuthMonitor.cc @@ -121,7 +121,7 @@ void AuthMonitor::update_from_paxos(bool *need_bootstrap) version_t keys_ver = mon->key_server.get_ver(); if (version == keys_ver) return; - assert(version >= keys_ver); + assert(version > keys_ver); version_t latest_full = get_version_latest_full();