Hotfix: boefje config migration should check the SQLAlchemy session (#…

…3227)

Signed-off-by: Donny Peeters <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
Co-authored-by: Jan Klopper <[email protected]>

boefjes/boefjes/migrations/versions/f9de6eb7824b_introduce_boefjeconfig_model.py

@@ -6,12 +6,15 @@
 
 """
 
+import logging
+
 import sqlalchemy as sa
 from alembic import op
 from sqlalchemy.orm import sessionmaker
 
 from boefjes.local_repository import get_local_repository
 from boefjes.sql.plugin_storage import create_plugin_storage
+from boefjes.storage.interfaces import PluginNotFound
 
 # revision identifiers, used by Alembic.
 revision = "f9de6eb7824b"
@@ -20,6 +23,9 @@
 depends_on = None
 
 
+logger = logging.getLogger(__name__)
+
+
 def upgrade() -> None:
     # ### commands auto generated by Alembic - please adjust! ###
     op.create_table(
@@ -70,6 +76,12 @@ def upgrade() -> None:
             if local_plugins[plugin_id].type != "boefje":
                 raise ValueError(f"Settings for normalizer or bit found: {plugin_id}. Remove these entries first.")
 
+            try:
+                storage.boefje_by_id(plugin_id)
+                continue  # The Boefje already exists
+            except PluginNotFound:
+                pass  # The raw query bypasses the session "cache", so this just checks for duplicates
+
             storage.create_boefje(local_plugins[plugin_id])  # type: ignore
 
         query = """
@@ -80,7 +92,14 @@ def upgrade() -> None:
         for plugin_id_output in op.get_bind().execute(query).fetchall():
             plugin_id = plugin_id_output[0]
             if plugin_id not in local_plugins:
-                raise ValueError(f"Invalid plugin id found: {plugin_id}")
+                logger.warning("Unknown plugin id found: %s. You might have to re-enable the plugin!", plugin_id)
+                continue
+
+            try:
+                storage.boefje_by_id(plugin_id)
+                continue  # The Boefje already exists
+            except PluginNotFound:
+                pass  # The raw query bypasses the session "cache", so this just checks for duplicates
 
             if local_plugins[plugin_id].type == "boefje":
                 storage.create_boefje(local_plugins[plugin_id])  # type: ignore
@@ -93,7 +112,14 @@ def upgrade() -> None:
         for plugin_id_output in op.get_bind().execute(query).fetchall():
             plugin_id = plugin_id_output[0]
             if plugin_id not in local_plugins:
-                raise ValueError(f"Invalid plugin id found: {plugin_id}")
+                logger.warning("Unknown plugin id found: %s. You might have to re-enable the plugin!", plugin_id)
+                continue
+
+            try:
+                storage.normalizer_by_id(plugin_id)
+                continue  # The Normalizer already exists
+            except PluginNotFound:
+                pass  # The raw query bypasses the session "cache", so this just checks for duplicates
 
             if local_plugins[plugin_id].type == "normalizer":
                 storage.create_normalizer(local_plugins[plugin_id])  # type: ignore
@@ -103,25 +129,25 @@ def upgrade() -> None:
             INSERT INTO boefje_config (settings, boefje_id, organisation_pk)
             SELECT s.values, b.id, s.organisation_pk from settings s
             join boefje b on s.plugin_id = b.plugin_id
-        """)
+        """)  # Add boefjes and set the settings for boefjes
 
     with connection.begin():
         connection.execute("""
-            INSERT INTO boefje_config (settings, boefje_id, organisation_pk)
+            INSERT INTO boefje_config (enabled, boefje_id, organisation_pk)
             SELECT p.enabled, b.id, p.organisation_pk FROM plugin_state p
             JOIN boefje b ON p.plugin_id = b.plugin_id
             LEFT JOIN boefje_config bc ON bc.boefje_id = b.id WHERE bc.boefje_id IS NULL
-        """)
+        """)  # Add boefjes and set the enabled field for boefjes that to not exist yet
         connection.execute("""
             UPDATE boefje_config bc SET enabled = p.enabled from plugin_state p
             JOIN boefje b ON p.plugin_id = b.plugin_id
             where b.id = bc.boefje_id and p.organisation_pk = bc.organisation_pk
-        """)
+        """)  # Set the enabled field for boefjes
         connection.execute("""
             UPDATE normalizer_config nc SET enabled = p.enabled from plugin_state p
             JOIN normalizer n ON p.plugin_id = n.plugin_id
             where n.id = nc.normalizer_id and p.organisation_pk = nc.organisation_pk
-        """)
+        """)  # Set the enabled field for normalizers
 
     op.drop_table("settings")
     op.drop_table("plugin_state")

boefjes/boefjes/plugins/kat_nmap_udp/boefje.json

@@ -10,7 +10,7 @@
     "TOP_PORTS_UDP"
   ],
   "scan_level": 2,
-  "oci_image": "openkat/nmap",
+  "oci_image": "ghcr.io/minvws/openkat/nmap:latest",
   "oci_arguments": [
     "--open",
     "-T4",

boefjes/boefjes/sql/config_storage.py

@@ -71,6 +71,9 @@ def get_all_settings(self, organisation_id: str, plugin_id: str) -> dict:
         except ConfigNotFound:
             return {}
 
+        if not instance.settings or instance.settings == "{}":  # Handle empty settings and the server default of "{}"
+            return {}
+
         return json.loads(self.encryption.decode(instance.settings))
 
     def delete(self, organisation_id: str, plugin_id: str) -> None:

boefjes/boefjes/sql/session.py

@@ -1,6 +1,7 @@
 import structlog
 from sqlalchemy.exc import DatabaseError
 from sqlalchemy.orm import Session
+from typing_extensions import Self
 
 from boefjes.storage.interfaces import StorageError
 
@@ -23,7 +24,7 @@ class SessionMixin:
     def __init__(self, session: Session):
         self.session: Session = session
 
-    def __enter__(self) -> "SessionMixin":
+    def __enter__(self) -> Self:
         return self
 
     def __exit__(self, exc_type: type[Exception], exc_value: str, exc_traceback: str) -> None:  # noqa: F841

boefjes/tests/integration/test_settings_to_boefje_config_migration.py

@@ -37,6 +37,13 @@ def setUp(self) -> None:
         ]
         query = f"INSERT INTO settings (id, values, plugin_id, organisation_pk) values {','.join(map(str, entries))}"  # noqa: S608
         self.engine.execute(text(query))
+
+        entries = [(1, "dns-records", True, 1), (2, "nmap-udp", True, 1)]
+        query = (
+            f"INSERT INTO plugin_state (id, plugin_id, enabled, organisation_pk) values {','.join(map(str, entries))}"  # noqa: S608
+        )
+        self.engine.execute(text(query))
+
         session.close()
 
     def test_fail_on_wrong_plugin_ids(self):
@@ -68,9 +75,13 @@ def test_fail_on_wrong_plugin_ids(self):
 
         assert SQLPluginStorage(session, settings).boefje_by_id("dns-records").id == "dns-records"
 
-        settings_storage = SQLConfigStorage(session, encrypter)
-        assert settings_storage.get_all_settings("dev1", "dns-records") == {"key1": "val1"}
-        assert settings_storage.get_all_settings("dev2", "dns-records") == {"key1": "val1", "key2": "val2"}
+        config_storage = SQLConfigStorage(session, encrypter)
+        assert config_storage.get_all_settings("dev1", "dns-records") == {"key1": "val1"}
+        assert config_storage.get_all_settings("dev2", "dns-records") == {"key1": "val1", "key2": "val2"}
+        assert config_storage.get_all_settings("dev1", "nmap-udp") == {}
+
+        assert config_storage.is_enabled_by_id("dns-records", "dev1")
+        assert config_storage.is_enabled_by_id("nmap-udp", "dev1")
 
         session.close()
 
@@ -82,11 +93,12 @@ def test_downgrade(self):
         encrypter = create_encrypter()
         all_settings = list(self.engine.execute(text("select * from settings")).fetchall())
         self.assertSetEqual(
-            {(encrypter.decode(x[1]), x[2], x[3]) for x in all_settings},
+            {(encrypter.decode(x[1]) if x[1] != "{}" else "{}", x[2], x[3]) for x in all_settings},
             {
                 ('{"key1": "val1"}', "dns-records", 1),
                 ('{"key1": "val1", "key2": "val2"}', "dns-records", 2),
                 ('{"key2": "val2", "key3": "val3"}', "nmap", 1),
+                ("{}", "nmap-udp", 1),
             },
         )