mgr/cephadm: fix objects_by_names initialization + some improvements

This commit include the following changes:

1. Fix objects_by_names initialization as we was using a dict for all
the case including global scoped objects which is not correct. For
those cases an instance of an empty TLSObject must be used.
2. Add sanity checks to the load() method to avoid loading incorrect
and malformed entries.
3. Add some helper functions to avoid code repetition

Fixes: https://tracker.ceph.com/issues/73625

Signed-off-by: Redouane Kachach <[email protected]>

Author: rkachach

src/pybind/mgr/cephadm/tests/test_certmgr.py

@@ -773,13 +773,12 @@ def _fake_prefix_store(prefix):
         assert 'iscsi_ssl_key' in key_store
         assert isinstance(key_store['iscsi_ssl_key'], dict) and len(key_store['iscsi_ssl_key']) == 0
 
-        # Global-scoped key with bad JSON should also not be instantiated (no PrivKey object)
-        # Depending on how you seed known names, it might be absent OR present but falsy.
-        # Accept either: absent OR not a PrivKey instance.
-        if 'mgmt_gateway_ssl_key' in key_store:
-            assert not isinstance(key_store['mgmt_gateway_ssl_key'], PrivKey)
-        else:
-            assert 'mgmt_gateway_ssl_key' not in key_store
+        # Global-scoped key with bad JSON should NOT be hydrated
+        assert 'mgmt_gateway_ssl_key' in key_store
+        obj = key_store.get('mgmt_gateway_ssl_key')
+        # It should be a PrivKey (because globals are pre-seeded) but remain empty/falsy
+        assert isinstance(obj, PrivKey)
+        assert not bool(obj)  # still the tombstone, not parsed content
 
         messages = [r.getMessage() for r in caplog.records
                     if r.levelno >= logging.WARNING and r.name == "cephadm.tlsobject_store"]

src/pybind/mgr/cephadm/tlsobject_store.py

@@ -1,6 +1,5 @@
 from typing import (Any,
                     Dict,
-                    Union,
                     List,
                     Tuple,
                     Optional,
@@ -33,12 +32,22 @@ def __init__(self, mgr: 'CephadmOrchestrator',
         self.mgr: CephadmOrchestrator = mgr
         self.cephadm_signed_object_checker = cephadm_signed_obj_checker
         self.tlsobject_class = tlsobject_class
-        all_known_objects_names = [item for sublist in known_objects_names.values() for item in sublist]
-        self.objects_by_name: Dict[str, Any] = {key: {} for key in all_known_objects_names}
         self.service_scoped_objects = known_objects_names[TLSObjectScope.SERVICE]
         self.host_scoped_objects = known_objects_names[TLSObjectScope.HOST]
         self.global_scoped_objects = known_objects_names[TLSObjectScope.GLOBAL]
         self.store_prefix = f'{TLSOBJECT_STORE_PREFIX}{tlsobject_class.STORAGE_PREFIX}.'
+        # initialize objects by name for the different scopes
+        self.objects_by_name: Dict[str, Any] = {}
+        for n in self.service_scoped_objects + self.host_scoped_objects:
+            self.objects_by_name[n] = {}
+        for n in self.global_scoped_objects:
+            self.objects_by_name[n] = self.tlsobject_class()
+
+    def _kv_key(self, obj_name: str) -> str:
+        return self.store_prefix + obj_name
+
+    def _set_store(self, obj_name: str, payload: Any) -> None:
+        self.mgr.set_store(self._kv_key(obj_name), json.dumps(payload))
 
     def register_object_name(self, obj_name: str, scope: TLSObjectScope) -> None:
         """
@@ -105,18 +114,16 @@ def save_tlsobject(self, obj_name: str,
         self._validate_tlsobject_name(obj_name, service_name, host)
         tlsobject = self.tlsobject_class(tlsobject, user_made, editable)
         scope, target = self.get_tlsobject_scope_and_target(obj_name, service_name, host)
-        j: Union[str, Dict[Any, Any], None] = None
         if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
             self.objects_by_name[obj_name][target] = tlsobject
-            j = {
+            serialized_targets = {
                 key: self.tlsobject_class.to_json(self.objects_by_name[obj_name][key])
                 for key in self.objects_by_name[obj_name]
             }
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            self._set_store(obj_name, serialized_targets)
         elif scope == TLSObjectScope.GLOBAL:
             self.objects_by_name[obj_name] = tlsobject
-            j = self.tlsobject_class.to_json(tlsobject)
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            self._set_store(obj_name, self.tlsobject_class.to_json(tlsobject))
         else:
             logger.error(f'Trying to save TLS object name {obj_name} with a not-supported/unknown TLSObjectScope scope {scope.value}')
 
@@ -155,28 +162,27 @@ def rm_tlsobject(self, obj_name: str, service_name: Optional[str] = None, host:
         """
         self._validate_tlsobject_name(obj_name, service_name, host)
         scope, target = self.get_tlsobject_scope_and_target(obj_name, service_name, host)
-        j: Union[str, Dict[Any, Any], None] = None
         if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
             if obj_name in self.objects_by_name and target in self.objects_by_name[obj_name]:
                 del self.objects_by_name[obj_name][target]
-                j = {
+                serialized_targets = {
                     key: self.tlsobject_class.to_json(self.objects_by_name[obj_name][key])
                     for key in self.objects_by_name[obj_name]
                 }
-                self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+                self._set_store(obj_name, serialized_targets)
                 return True
         elif scope == TLSObjectScope.GLOBAL:
             self.objects_by_name[obj_name] = self.tlsobject_class()
-            j = self.tlsobject_class.to_json(self.objects_by_name[obj_name])
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            serialized_obj = self.tlsobject_class.to_json(self.objects_by_name[obj_name])
+            self._set_store(obj_name, serialized_obj)
             return True
         else:
             raise TLSObjectException(f'Attempted to remove {self.tlsobject_class.__name__.lower()} for unknown obj_name {obj_name}')
         return False
 
     def _validate_tlsobject_name(self, obj_name: str, service_name: Optional[str] = None, host: Optional[str] = None) -> None:
         cred_type = self.tlsobject_class.__name__.lower()
-        if obj_name not in self.objects_by_name.keys():
+        if obj_name not in self.objects_by_name:
             raise TLSObjectException(f'Attempted to access {cred_type} for unknown TLS object name {obj_name}')
         if obj_name in self.host_scoped_objects and not host:
             raise TLSObjectException(f'Need host to access {cred_type} for TLS object {obj_name}')
@@ -205,39 +211,72 @@ def list_tlsobjects(self) -> List[Tuple[str, TLSObjectProtocol, Optional[str]]]:
         return tlsobjects
 
     def load(self) -> None:
+
+        def _kv_preview(key: str, raw: str, n: int = 20) -> Tuple[str, int, str]:
+            # Safe, short, escaped preview for logs
+            return key, (len(raw) if raw else 0), (raw[:n] if raw else "")
+
         for k, v in self.mgr.get_store_prefix(self.store_prefix).items():
             obj_name = k[len(self.store_prefix):]
             is_cephadm_signed_object = self.cephadm_signed_object_checker(obj_name)
             if not is_cephadm_signed_object and obj_name not in self.objects_by_name:
-                logger.warning(f"TLSObjectStore: Discarding unknown obj_name '{obj_name}'")
+                logger.warning("TLSObjectStore: Discarding unknown obj_name %r", obj_name)
                 continue
 
             try:
                 tls_object_targets = json.loads(v)
+                if not isinstance(tls_object_targets, dict):
+                    key_preview, vlen, vstart = _kv_preview(k, v)
+                    logger.error(
+                        "TLSObjectStore: Invalid data structure for object %r. "
+                        "Expected dict but got %s. key=%r, len=%d, startswith=%r",
+                        obj_name, type(tls_object_targets).__name__,
+                        key_preview, vlen, vstart,
+                    )
+                    continue
             except json.JSONDecodeError as e:
+                key_preview, vlen, vstart = _kv_preview(k, v)
                 logger.warning(
-                    f"TLSObjectStore: Cannot parse JSON for '{obj_name}': "
-                    f"key={k}, len={len(v) if v else 0}, startswith={v[:20]!r}, error={e}"
+                    "TLSObjectStore: Cannot parse JSON for %r: key=%r, len=%d, startswith=%r, error=%r",
+                    obj_name, key_preview, vlen, vstart, e,
                 )
                 continue
             except Exception as e:
+                key_preview, vlen, vstart = _kv_preview(k, v)
                 logger.error(
-                    f"TLSObjectStore: Unexpected error happened while trying to parse JSON for '{obj_name}': "
-                    f"key={k}, len={len(v) if v else 0}, startswith={v[:20]!r}, error={e}"
+                    "TLSObjectStore: Unexpected error while parsing %r: key=%r, len=%d, startswith=%r, error=%r",
+                    obj_name, key_preview, vlen, vstart, e,
+                    exc_info=True,  # include traceback for unexpected errors
                 )
                 continue
 
             if is_cephadm_signed_object or (obj_name in self.service_scoped_objects) or (obj_name in self.host_scoped_objects):
                 if is_cephadm_signed_object and obj_name not in self.host_scoped_objects:
                     self.host_scoped_objects.append(obj_name)
                 self.objects_by_name[obj_name] = {}
-                for target in tls_object_targets:
-                    tlsobject = self.tlsobject_class.from_json(tls_object_targets[target])
-                    if tlsobject:
-                        self.objects_by_name[obj_name][target] = tlsobject
+                for target, payload in tls_object_targets.items():
+                    try:
+                        tlsobject = self.tlsobject_class.from_json(payload)
+                        if tlsobject:  # skip tombstones
+                            self.objects_by_name[obj_name][target] = tlsobject
+                        else:
+                            logger.debug("TLSObjectStore: Skipping tombstone for %r (target %r)", obj_name, target)
+                    except Exception as e:
+                        key_preview, vlen, vstart = _kv_preview(k, str(payload))
+                        logger.warning(
+                            "TLSObjectStore: Failed to decode scoped TLS object %r (target %r): %r. "
+                            "key=%r, len=%d, startswith=%r",
+                            obj_name, target, e, key_preview, vlen, vstart
+                        )
             elif obj_name in self.global_scoped_objects:
-                tlsobject = self.tlsobject_class.from_json(tls_object_targets)
-                if tlsobject:
-                    self.objects_by_name[obj_name] = tlsobject
+                try:
+                    self.objects_by_name[obj_name] = self.tlsobject_class.from_json(tls_object_targets)
+                except Exception as e:
+                    key_preview, vlen, vstart = _kv_preview(k, v)
+                    logger.warning(
+                        "TLSObjectStore: Failed to decode global TLS object %r: %r. "
+                        "key=%r, len=%d, startswith=%r",
+                        obj_name, e, key_preview, vlen, vstart
+                    )
             else:
-                logger.error(f"TLSObjectStore: Found a known TLS object name {obj_name} with unknown scope!")
+                logger.error("TLSObjectStore: Found a known TLS object name %r with unknown scope!", obj_name)

src/pybind/mgr/cephadm/tlsobject_types.py

@@ -156,6 +156,6 @@ def from_json(cls, data: Dict[str, str]) -> 'PrivKey':
             user_made = parse_bool(data.get('user_made', False))
             editable = parse_bool(data.get('editable', False))
         except ValueError as e:
-            raise TLSObjectException(f'Expected field in Cert object to be bool but got another type: {e}')
+            raise TLSObjectException(f'Expected field in PrivKey object to be bool but got another type: {e}')
 
         return cls(key=key, user_made=user_made, editable=editable)