From: Patrick Donnelly Date: Sun, 1 Jun 2025 00:54:30 +0000 (-0400) Subject: mon: add health checks for insecure keys X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=6b32cf6d8a245d8192515ddc037b23a2d031c2cb;p=ceph-ci.git mon: add health checks for insecure keys This commit prompted the previous refactor as it was inconvenient to check for health warnings as part of AuthMonitor::tick and then pass those up via PaxosService::encode_health. Signed-off-by: Patrick Donnelly (cherry picked from commit c391dd8a16124879586ec4eebdd7286118ecc1de) --- diff --git a/src/auth/Auth.h b/src/auth/Auth.h index d9cfe598b36..236ddbed40d 100644 --- a/src/auth/Auth.h +++ b/src/auth/Auth.h @@ -360,6 +360,12 @@ struct RotatingSecrets { void wipe() { secrets.clear(); } + auto begin() const { + return secrets.begin(); + } + auto end() const { + return secrets.end(); + } void dump(); void dump(ceph::Formatter *f) const { diff --git a/src/auth/Crypto.cc b/src/auth/Crypto.cc index 5a32019f9f4..566cc88c1c7 100644 --- a/src/auth/Crypto.cc +++ b/src/auth/Crypto.cc @@ -53,6 +53,8 @@ using std::ostringstream; using std::string; +using namespace std::literals::string_view_literals; + using ceph::bufferlist; using ceph::bufferptr; using ceph::Formatter; @@ -1158,6 +1160,20 @@ int CryptoManager::get_key_type(const std::string& s) return -ENOENT; } +std::string_view CryptoManager::get_key_type_name(int type) +{ + switch (type) { + case CEPH_CRYPTO_NONE: + return "none"sv; + case CEPH_CRYPTO_AES256KRB5: + return "aes256k"sv; + case CEPH_CRYPTO_AES: + return "aes"sv; + default: + return "???"sv; + } +} + const std::set& CryptoManager::get_secure_key_types() { static const std::set secure_keys{CEPH_CRYPTO_AES256KRB5}; diff --git a/src/auth/Crypto.h b/src/auth/Crypto.h index 3abd38ca7b1..f4629a1f535 100644 --- a/src/auth/Crypto.h +++ b/src/auth/Crypto.h @@ -269,6 +269,7 @@ public: } static int get_key_type(const std::string& s); + static std::string_view get_key_type_name(int type); static const std::set& get_secure_key_types(); bool crypto_type_supported(int type) const; diff --git a/src/auth/cephx/CephxKeyServer.h b/src/auth/cephx/CephxKeyServer.h index ce0836bc682..fbe83c946c8 100644 --- a/src/auth/cephx/CephxKeyServer.h +++ b/src/auth/cephx/CephxKeyServer.h @@ -249,6 +249,10 @@ public: KeyServer& operator=(const KeyServer&) = delete; bool generate_secret(CryptoKey& secret, std::optional type = std::nullopt); + auto const& get_rotating_secrets() const { + return data.rotating_secrets; + } + bool get_secret(const EntityName& name, CryptoKey& secret) const override; bool get_auth(const EntityName& name, EntityAuth& auth) const; bool get_caps(const EntityName& name, const std::string& type, AuthCapsInfo& caps) const; @@ -355,6 +359,7 @@ public: bool get_service_caps(const EntityName& name, uint32_t service_id, AuthCapsInfo& caps) const; + auto const& get_secrets() const { return data.get_secrets(); } std::map::iterator secrets_begin() { return data.secrets_begin(); } std::map::iterator secrets_end() diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc index f6c1d076782..8b4b6e531c5 100644 --- a/src/mon/AuthMonitor.cc +++ b/src/mon/AuthMonitor.cc @@ -128,7 +128,7 @@ void AuthMonitor::tick() { if (!is_active()) return; - dout(10) << *this << dendl; + dout(10) << __func__ << dendl; // increase global_id? bool propose = false; @@ -171,6 +171,12 @@ void AuthMonitor::tick() if (check_rotate()) { propose = true; + dout(20) << "proposing for rotate" << dendl; + } + + if (check_health()) { + propose = true; + dout(20) << "proposing for health" << dendl; } if (propose) { @@ -466,49 +472,175 @@ void AuthMonitor::encode_pending(MonitorDBStore::TransactionRef t) put_version(t, version, bl); put_last_committed(t, version); - // health + check_health(); +} + +bool AuthMonitor::check_health() +{ auto& next = get_health_checks_pending_writeable(); - map> bad_detail; // entity -> details + next.clear(); /* may be called from ::tick and then ::encode_pending */ + + auto const& secure_key_types = CryptoManager::get_secure_key_types(); + + { + auto allowed_ciphers = cct->_conf.get_val("cephx_allowed_ciphers"); + std::vector details; + for (auto& c : allowed_ciphers) { + if (!secure_key_types.contains(c)) { + ostringstream ss; + auto name = CryptoManager::get_key_type_name(c); + ss << "insecure cipher " << name << " allowed for auth"; + details.push_back(ss.str()); + } + } + if (!details.empty()) { + auto& check = next.add("AUTH_INSECURE_KEYS_ALLOWED", HEALTH_WARN, "Monitors are configured to allow auth using insecure key types", details.size()); + for (auto& detail : details) { + check.detail.push_back(detail); + } + /* So that existing clusters continue to allow issuing older key types. */ + cct->_conf.set_val_default("mon_auth_allow_insecure_key", "true"); + } else { + cct->_conf.set_val_default("mon_auth_allow_insecure_key", "false"); + } + } + + if (cct->_conf.get_val("mon_auth_allow_insecure_key")) { + next.add("AUTH_INSECURE_KEYS_CREATABLE", HEALTH_WARN, "Monitors are configured to allow creation of insecure key types", 1); + } + + { + auto service_key_type_name = cct->_conf.get_val("auth_service_cipher"); + auto service_key_type = CryptoManager::get_key_type(service_key_type_name); + if (!secure_key_types.contains(service_key_type)) { + next.add("AUTH_INSECURE_SERVICE_TICKETS", HEALTH_WARN, "Monitors are configured to issue insecure service key types", 1); + } + } + + std::map> bad_caps_detail; // entity -> details + std::map bad_key_client_detail; + std::map bad_key_service_detail; for (auto const& [entity, auth] : mon.key_server.get_secrets()) { for (auto& p : auth.caps) { ostringstream ss; if (!valid_caps(p.first, p.second, &ss)) { ostringstream ss2; ss2 << entity << " " << ss.str(); - bad_detail[entity.to_str()].push_back(ss2.str()); + bad_caps_detail[entity.to_str()].push_back(ss2.str()); + } + } + if (!secure_key_types.contains(auth.key.get_type())) { + auto name = CryptoManager::get_key_type_name(auth.key.get_type()); + std::ostringstream ss; + ss << "entity " << entity << " using insecure key type: " << name; + if (entity.is_client()) { + bad_key_client_detail[entity] = ss.str(); + } else { + bad_key_service_detail[entity] = ss.str(); } } } + for (auto& inc : pending_auth) { if (inc.inc_type == AUTH_DATA) { KeyServerData::Incremental auth_inc; auto iter = inc.auth_data.cbegin(); decode(auth_inc, iter); + auto& auth = auth_inc.auth; + auto& entity = auth_inc.name; if (auth_inc.op == KeyServerData::AUTH_INC_DEL) { - bad_detail.erase(auth_inc.name.to_str()); + bad_caps_detail.erase(entity.to_str()); + bad_key_client_detail.erase(entity); + bad_key_service_detail.erase(entity); } else if (auth_inc.op == KeyServerData::AUTH_INC_ADD) { - for (auto& p : auth_inc.auth.caps) { + for (auto& p : auth.caps) { ostringstream ss; if (!valid_caps(p.first, p.second, &ss)) { ostringstream ss2; - ss2 << auth_inc.name << " " << ss.str(); - bad_detail[auth_inc.name.to_str()].push_back(ss2.str()); + ss2 << entity << " " << ss.str(); + bad_caps_detail[entity.to_str()].push_back(ss2.str()); } } + if (!secure_key_types.contains(auth.key.get_type())) { + auto name = CryptoManager::get_key_type_name(auth.key.get_type()); + std::ostringstream ss; + ss << "entity " << entity << " using insecure key type: " << name; + if (entity.is_client()) { + bad_key_client_detail[entity] = ss.str(); + } else { + bad_key_service_detail[entity] = ss.str(); + } + } } } } - if (bad_detail.size()) { + if (bad_caps_detail.size()) { ostringstream ss; - ss << bad_detail.size() << " auth entities have invalid capabilities"; + ss << bad_caps_detail.size() << " auth entities have invalid capabilities"; health_check_t *check = &next.add("AUTH_BAD_CAPS", HEALTH_ERR, ss.str(), - bad_detail.size()); - for (auto& i : bad_detail) { + bad_caps_detail.size()); + for (auto& i : bad_caps_detail) { for (auto& j : i.second) { check->detail.push_back(j); } } } + if (!bad_key_client_detail.empty()) { + std::ostringstream summary; + summary << bad_key_client_detail.size() << " auth client entities with insecure key types"; + auto& check = next.add("AUTH_INSECURE_CLIENT_KEY_TYPE", HEALTH_WARN, summary.str(), bad_key_client_detail.size()); + for (auto& [name, detail] : bad_key_client_detail) { + check.detail.push_back(detail); + } + } + if (!bad_key_service_detail.empty()) { + std::ostringstream summary; + summary << bad_key_service_detail.size() << " auth service entities with insecure key types"; + auto& check = next.add("AUTH_INSECURE_SERVICE_KEY_TYPE", HEALTH_ERR, summary.str(), bad_key_service_detail.size()); + for (auto& [name, detail] : bad_key_service_detail) { + check.detail.push_back(detail); + } + } + + std::vector bad_rotating_service_keys; + for (auto const& [entity_type, secrets] : mon.key_server.get_rotating_secrets()) { + auto entity_name = EntityName::ceph_entity_type_to_str(entity_type); + dout(20) << __func__ << ": examining " << entity_name << " for insecure rotating keys" << dendl; + if (entity_type == CEPH_ENTITY_TYPE_AUTH) { + /* Do not report insecure key types for auth because: + * - We do not wipe that key type via `ceph auth + * wipe-rotating-service-keys` as it would prevent global id reclaim when + * obtaining new tickets. + * - The auth keys have limited value: a malicious actor can only create + * a ticket but not the service tickets (which require the rotating + * service keys). That ticket's only value would be the global id for + * reclaim. + * - Eventually these insecure key types will rotate out as long as the + * operator has addressed the others. + */ + continue; + } + for (auto const& [id, secret] : secrets) { + if (!secure_key_types.contains(secret.key.get_type())) { + auto name = CryptoManager::get_key_type_name(secret.key.get_type()); + std::ostringstream ss; + ss << "rotating service keys for " << entity_name << " using insecure key type: " << name; + bad_rotating_service_keys.push_back(ss.str()); + break; + } + } + } + + if (!bad_rotating_service_keys.empty()) { + std::ostringstream summary; + summary << bad_rotating_service_keys.size() << " rotating auth service keys using insecure key types"; + auto& check = next.add("AUTH_INSECURE_ROTATING_SERVICE_KEY_TYPE", HEALTH_ERR, summary.str(), bad_rotating_service_keys.size()); + for (auto& detail : bad_rotating_service_keys) { + check.detail.push_back(detail); + } + } + + return next != get_health_checks(); /* should propose */ } void AuthMonitor::encode_full(MonitorDBStore::TransactionRef t) diff --git a/src/mon/AuthMonitor.h b/src/mon/AuthMonitor.h index 7134612615d..dd9e047a45b 100644 --- a/src/mon/AuthMonitor.h +++ b/src/mon/AuthMonitor.h @@ -164,6 +164,8 @@ public: private: bool prepare_used_pending_keys(MonOpRequestRef op); + bool check_health(); + // propose pending update to peers void encode_pending(MonitorDBStore::TransactionRef t) override; void encode_full(MonitorDBStore::TransactionRef t) override;