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 <pdonnell@ibm.com>
(cherry picked from commit c391dd8a16124879586ec4eebdd7286118ecc1de)

diff --git a/src/auth/Auth.h b/src/auth/Auth.h
index d9cfe598b368a68866ebb5a735352a112d4059ed..236ddbed40d8a10ddc39baca3620f80d66182def 100644 (file)
--- 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 5a32019f9f4de435e89e0dbae8da573b6bb1292c..566cc88c1c768697d287cbfea9a97ccb8fbcac46 100644 (file)
--- 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<int>& CryptoManager::get_secure_key_types()
 {
   static const std::set<int> secure_keys{CEPH_CRYPTO_AES256KRB5};

diff --git a/src/auth/Crypto.h b/src/auth/Crypto.h
index 3abd38ca7b1241fd0a3e1ecd9c3f97c9e15a1173..f4629a1f535b543d3f135f21374fc59d395fc903 100644 (file)
--- 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<int>& 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 ce0836bc682485415ce73b28c9cec34285a16ef1..fbe83c946c8a5b9607877319f462e11852e2bb6a 100644 (file)
--- 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<int> 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<EntityName, EntityAuth>::iterator secrets_begin()
   { return data.secrets_begin(); }
   std::map<EntityName, EntityAuth>::iterator secrets_end()

diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc
index f6c1d0767828ea1221a2aaabbb27056d25ba54b0..8b4b6e531c5141850f0ef22881d2fa9e4fa80fc2 100644 (file)
--- 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<string,list<string>> 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<std::string>("cephx_allowed_ciphers");
+    std::vector<std::string> 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<bool>("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<std::string>("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<std::string,std::list<std::string>> bad_caps_detail;  // entity -> details
+  std::map<EntityName, std::string> bad_key_client_detail;
+  std::map<EntityName, std::string> 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<std::string> 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 7134612615dcd439b59e861ccd5cf6c82c64e2a7..dd9e047a45b68e148fb08d7dbf28ae5233d48f25 100644 (file)
--- 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;