From 545e04d7d59ba875dbc14b9fba8f1b99f7804998 Mon Sep 17 00:00:00 2001 From: Donny Peeters <46660228+Donnype@users.noreply.github.com> Date: Wed, 17 Jul 2024 16:20:01 +0200 Subject: [PATCH] Hotfix: boefje config migration should check the SQLAlchemy session (#3227) Signed-off-by: Donny Peeters Co-authored-by: Jeroen Dekkers Co-authored-by: Jan Klopper --- ...de6eb7824b_introduce_boefjeconfig_model.py | 40 +++++++++++++++---- .../boefjes/plugins/kat_nmap_udp/boefje.json | 2 +- boefjes/boefjes/sql/config_storage.py | 3 ++ boefjes/boefjes/sql/session.py | 3 +- ...est_settings_to_boefje_config_migration.py | 20 ++++++++-- 5 files changed, 55 insertions(+), 13 deletions(-) diff --git a/boefjes/boefjes/migrations/versions/f9de6eb7824b_introduce_boefjeconfig_model.py b/boefjes/boefjes/migrations/versions/f9de6eb7824b_introduce_boefjeconfig_model.py index 265cb99c499..ffa35930368 100644 --- a/boefjes/boefjes/migrations/versions/f9de6eb7824b_introduce_boefjeconfig_model.py +++ b/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") diff --git a/boefjes/boefjes/plugins/kat_nmap_udp/boefje.json b/boefjes/boefjes/plugins/kat_nmap_udp/boefje.json index 5a67dc5f570..f9839e53e37 100644 --- a/boefjes/boefjes/plugins/kat_nmap_udp/boefje.json +++ b/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", diff --git a/boefjes/boefjes/sql/config_storage.py b/boefjes/boefjes/sql/config_storage.py index 15afde54e6e..cd2e2b31db8 100644 --- a/boefjes/boefjes/sql/config_storage.py +++ b/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: diff --git a/boefjes/boefjes/sql/session.py b/boefjes/boefjes/sql/session.py index 4a832a12bfa..681a6c963ba 100644 --- a/boefjes/boefjes/sql/session.py +++ b/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 diff --git a/boefjes/tests/integration/test_settings_to_boefje_config_migration.py b/boefjes/tests/integration/test_settings_to_boefje_config_migration.py index a646eae23dd..35e7670f614 100644 --- a/boefjes/tests/integration/test_settings_to_boefje_config_migration.py +++ b/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), }, )