From ca3c587d1e6516643d8f09cb43e7ef5b016cde97 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 19 Sep 2024 10:34:11 -0300 Subject: [PATCH] [DPE-5248] Add pgAudit (#612) * Add pgAudit Signed-off-by: Marcelo Henrique Neppel * Update snap revisions Signed-off-by: Marcelo Henrique Neppel * Add pgAudit and integration test Signed-off-by: Marcelo Henrique Neppel * Fix integration test Signed-off-by: Marcelo Henrique Neppel * Remove txt file Signed-off-by: Marcelo Henrique Neppel * Add unit test Signed-off-by: Marcelo Henrique Neppel * Enable pgAudit by default Signed-off-by: Marcelo Henrique Neppel * Enable/disable the plugin at the unit test Signed-off-by: Marcelo Henrique Neppel * Fix test_no_password_exposed_on_logs Signed-off-by: Marcelo Henrique Neppel --------- Signed-off-by: Marcelo Henrique Neppel --- config.yaml | 4 + lib/charms/postgresql_k8s/v0/postgresql.py | 22 ++++- src/charm.py | 23 ++++- src/config.py | 1 + src/constants.py | 5 +- src/relations/db.py | 6 +- src/relations/postgresql_provider.py | 6 +- templates/patroni.yml.j2 | 2 +- tests/integration/test_audit.py | 103 ++++++++++++++++++++ tests/integration/test_password_rotation.py | 7 +- tests/unit/test_charm.py | 34 +++++++ tests/unit/test_db.py | 6 +- tests/unit/test_postgresql_provider.py | 2 +- 13 files changed, 198 insertions(+), 23 deletions(-) create mode 100644 tests/integration/test_audit.py diff --git a/config.yaml b/config.yaml index e9e4d01669..40aaf2ead9 100644 --- a/config.yaml +++ b/config.yaml @@ -303,6 +303,10 @@ options: default: false type: boolean description: Enable timescaledb extension + plugin_audit_enable: + default: true + type: boolean + description: Enable pgAudit extension profile: description: | Profile representing the scope of deployment, and used to tune resource allocation. diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index c3412d36df..0a13a765de 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -36,7 +36,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 34 +LIBPATCH = 35 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" @@ -114,6 +114,25 @@ def __init__( self.database = database self.system_users = system_users + def _configure_pgaudit(self, enable: bool) -> None: + connection = None + try: + connection = self._connect_to_database() + connection.autocommit = True + with connection.cursor() as cursor: + if enable: + cursor.execute("ALTER SYSTEM SET pgaudit.log = 'ROLE,DDL,MISC,MISC_SET';") + cursor.execute("ALTER SYSTEM SET pgaudit.log_client TO off;") + cursor.execute("ALTER SYSTEM SET pgaudit.log_parameter TO off") + else: + cursor.execute("ALTER SYSTEM RESET pgaudit.log;") + cursor.execute("ALTER SYSTEM RESET pgaudit.log_client;") + cursor.execute("ALTER SYSTEM RESET pgaudit.log_parameter;") + cursor.execute("SELECT pg_reload_conf();") + finally: + if connection is not None: + connection.close() + def _connect_to_database( self, database: str = None, database_host: str = None ) -> psycopg2.extensions.connection: @@ -325,6 +344,7 @@ def enable_disable_extensions(self, extensions: Dict[str, bool], database: str = if enable else f"DROP EXTENSION IF EXISTS {extension};" ) + self._configure_pgaudit(ordered_extensions.get("pgaudit", False)) except psycopg2.errors.UniqueViolation: pass except psycopg2.errors.DependentObjectsStillExist: diff --git a/src/charm.py b/src/charm.py index f0a119d5db..19916df374 100755 --- a/src/charm.py +++ b/src/charm.py @@ -76,6 +76,7 @@ PATRONI_CONF_PATH, PATRONI_PASSWORD_KEY, PEER, + PLUGIN_OVERRIDES, POSTGRESQL_SNAP_NAME, RAFT_PASSWORD_KEY, REPLICATION_PASSWORD_KEY, @@ -84,6 +85,7 @@ SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, SNAP_PACKAGES, + SPI_MODULE, SYSTEM_USERS, TLS_CA_FILE, TLS_CERT_FILE, @@ -996,8 +998,6 @@ def enable_disable_extensions(self, database: str = None) -> None: if self._patroni.get_primary() is None: logger.debug("Early exit enable_disable_extensions: standby cluster") return - spi_module = ["refint", "autoinc", "insert_username", "moddatetime"] - plugins_exception = {"uuid_ossp": '"uuid-ossp"'} original_status = self.unit.status extensions = {} # collect extensions @@ -1007,10 +1007,10 @@ def enable_disable_extensions(self, database: str = None) -> None: # Enable or disable the plugin/extension. extension = "_".join(plugin.split("_")[1:-1]) if extension == "spi": - for ext in spi_module: + for ext in SPI_MODULE: extensions[ext] = enable continue - extension = plugins_exception.get(extension, extension) + extension = PLUGIN_OVERRIDES.get(extension, extension) if self._check_extension_dependencies(extension, enable): self.unit.status = BlockedStatus(EXTENSIONS_DEPENDENCY_MESSAGE) return @@ -1514,7 +1514,6 @@ def _install_snap_packages(self, packages: List[str], refresh: bool = False) -> snap_package.hold() else: snap_package.ensure(snap.SnapState.Latest, channel=snap_version["channel"]) - except (snap.SnapError, snap.SnapNotFoundError) as e: logger.error( "An exception occurred when installing %s. Reason: %s", snap_name, str(e) @@ -1861,6 +1860,20 @@ def log_pitr_last_transaction_time(self) -> None: else: logger.error("Can't tell last completed transaction time") + def get_plugins(self) -> List[str]: + """Return a list of installed plugins.""" + plugins = [ + "_".join(plugin.split("_")[1:-1]) + for plugin in self.config.plugin_keys() + if self.config[plugin] + ] + plugins = [PLUGIN_OVERRIDES.get(plugin, plugin) for plugin in plugins] + if "spi" in plugins: + plugins.remove("spi") + for ext in SPI_MODULE: + plugins.append(ext) + return plugins + if __name__ == "__main__": main(PostgresqlOperatorCharm) diff --git a/src/config.py b/src/config.py index 5552a06b19..a08cb9518c 100644 --- a/src/config.py +++ b/src/config.py @@ -36,6 +36,7 @@ class CharmConfig(BaseConfigModel): optimizer_join_collapse_limit: Optional[int] profile: str profile_limit_memory: Optional[int] + plugin_audit_enable: bool plugin_citext_enable: bool plugin_debversion_enable: bool plugin_hstore_enable: bool diff --git a/src/constants.py b/src/constants.py index 36391cc55b..bc76ad2cdb 100644 --- a/src/constants.py +++ b/src/constants.py @@ -41,7 +41,7 @@ ( POSTGRESQL_SNAP_NAME, { - "revision": {"aarch64": "121", "x86_64": "120"}, + "revision": {"aarch64": "125", "x86_64": "124"}, "channel": "14/stable", }, ) @@ -84,3 +84,6 @@ TRACING_PROTOCOL = "otlp_http" BACKUP_TYPE_OVERRIDES = {"full": "full", "differential": "diff", "incremental": "incr"} +PLUGIN_OVERRIDES = {"audit": "pgaudit", "uuid_ossp": '"uuid-ossp"'} + +SPI_MODULE = ["refint", "autoinc", "insert_username", "moddatetime"] diff --git a/src/relations/db.py b/src/relations/db.py index 778c52cfd3..dd4ece54c3 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -201,11 +201,7 @@ def set_up_relation(self, relation: Relation) -> bool: self.charm.set_secret(APP_SCOPE, f"{user}-database", database) self.charm.postgresql.create_user(user, password, self.admin) - plugins = [ - "_".join(plugin.split("_")[1:-1]) - for plugin in self.charm.config.plugin_keys() - if self.charm.config[plugin] - ] + plugins = self.charm.get_plugins() self.charm.postgresql.create_database( database, user, plugins=plugins, client_relations=self.charm.client_relations diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 3f17dbc62a..63fba9b28e 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -91,11 +91,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: user = f"relation-{event.relation.id}" password = new_password() self.charm.postgresql.create_user(user, password, extra_user_roles=extra_user_roles) - plugins = [ - "_".join(plugin.split("_")[1:-1]) - for plugin in self.charm.config.plugin_keys() - if self.charm.config[plugin] - ] + plugins = self.charm.get_plugins() self.charm.postgresql.create_database( database, user, plugins=plugins, client_relations=self.charm.client_relations diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index af3773a068..426b226e72 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -97,7 +97,7 @@ bootstrap: log_truncate_on_rotation: 'on' logging_collector: 'on' wal_level: logical - shared_preload_libraries: 'timescaledb' + shared_preload_libraries: 'timescaledb,pgaudit' {%- if pg_parameters %} {%- for key, value in pg_parameters.items() %} {{key}}: {{value}} diff --git a/tests/integration/test_audit.py b/tests/integration/test_audit.py new file mode 100644 index 0000000000..257ef9cf70 --- /dev/null +++ b/tests/integration/test_audit.py @@ -0,0 +1,103 @@ +#!/usr/bin/env python3 +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +import asyncio +import logging + +import psycopg2 as psycopg2 +import pytest as pytest +from pytest_operator.plugin import OpsTest +from tenacity import Retrying, stop_after_delay, wait_fixed + +from .helpers import ( + APPLICATION_NAME, + DATABASE_APP_NAME, + run_command_on_unit, +) +from .new_relations.helpers import build_connection_string + +logger = logging.getLogger(__name__) + +RELATION_ENDPOINT = "database" + + +@pytest.mark.group(1) +@pytest.mark.abort_on_fail +async def test_audit_plugin(ops_test: OpsTest, charm) -> None: + """Test the audit plugin.""" + await asyncio.gather( + ops_test.model.deploy(charm, config={"profile": "testing"}), + ops_test.model.deploy(APPLICATION_NAME), + ) + await ops_test.model.relate(f"{APPLICATION_NAME}:{RELATION_ENDPOINT}", DATABASE_APP_NAME) + async with ops_test.fast_forward(): + await ops_test.model.wait_for_idle( + apps=[APPLICATION_NAME, DATABASE_APP_NAME], status="active" + ) + + logger.info("Checking that the audit plugin is enabled") + connection_string = await build_connection_string( + ops_test, APPLICATION_NAME, RELATION_ENDPOINT + ) + connection = None + try: + connection = psycopg2.connect(connection_string) + with connection.cursor() as cursor: + cursor.execute("CREATE TABLE test2(value TEXT);") + cursor.execute("GRANT SELECT ON test2 TO PUBLIC;") + cursor.execute("SET TIME ZONE 'Europe/Rome';") + finally: + if connection is not None: + connection.close() + unit_name = f"{DATABASE_APP_NAME}/0" + for attempt in Retrying(stop=stop_after_delay(90), wait=wait_fixed(10), reraise=True): + with attempt: + try: + logs = await run_command_on_unit( + ops_test, + unit_name, + "sudo grep AUDIT /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log", + ) + assert "MISC,BEGIN,,,BEGIN" in logs + assert ( + "DDL,CREATE TABLE,TABLE,public.test2,CREATE TABLE test2(value TEXT);" in logs + ) + assert "ROLE,GRANT,TABLE,,GRANT SELECT ON test2 TO PUBLIC;" in logs + assert "MISC,SET,,,SET TIME ZONE 'Europe/Rome';" in logs + except Exception: + assert False, "Audit logs were not found when the plugin is enabled." + + logger.info("Disabling the audit plugin") + await ops_test.model.applications[DATABASE_APP_NAME].set_config({ + "plugin_audit_enable": "False" + }) + await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") + + logger.info("Removing the previous logs") + await run_command_on_unit( + ops_test, + unit_name, + "rm /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log", + ) + + logger.info("Checking that the audit plugin is disabled") + try: + connection = psycopg2.connect(connection_string) + with connection.cursor() as cursor: + cursor.execute("CREATE TABLE test1(value TEXT);") + cursor.execute("GRANT SELECT ON test1 TO PUBLIC;") + cursor.execute("SET TIME ZONE 'Europe/Rome';") + finally: + if connection is not None: + connection.close() + try: + logs = await run_command_on_unit( + ops_test, + unit_name, + "sudo grep AUDIT /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log", + ) + except Exception: + pass + else: + logger.info(f"Logs: {logs}") + assert False, "Audit logs were found when the plugin is disabled." diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index c456585134..d3331b1ee9 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -2,6 +2,7 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. import json +import re import time import psycopg2 @@ -194,4 +195,8 @@ async def test_no_password_exposed_on_logs(ops_test: OpsTest) -> None: ) except Exception: continue - assert len(logs) == 0, f"Sensitive information detected on {unit.name} logs" + regex = re.compile("(PASSWORD )(?!)") + logs_without_false_positives = regex.findall(logs) + assert ( + len(logs_without_false_positives) == 0 + ), f"Sensitive information detected on {unit.name} logs" diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 80ec9f1975..5cb0709891 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -541,6 +541,9 @@ def test_enable_disable_extensions(harness, caplog): plugin_timescaledb_enable: default: false type: boolean + plugin_audit_enable: + default: true + type: boolean profile: default: production type: string""" @@ -2552,3 +2555,34 @@ def test_restart_services_after_reboot(harness): harness.charm._restart_services_after_reboot() _start_patroni.assert_called_once() _start_stop_pgbackrest_service.assert_called_once() + + +def test_get_plugins(harness): + with patch("charm.PostgresqlOperatorCharm._on_config_changed"): + # Test when the charm has no plugins enabled. + assert harness.charm.get_plugins() == ["pgaudit"] + + # Test when the charm has some plugins enabled. + harness.update_config({ + "plugin_audit_enable": True, + "plugin_citext_enable": True, + "plugin_spi_enable": True, + }) + assert harness.charm.get_plugins() == [ + "pgaudit", + "citext", + "refint", + "autoinc", + "insert_username", + "moddatetime", + ] + + # Test when the charm has the pgAudit plugin disabled. + harness.update_config({"plugin_audit_enable": False}) + assert harness.charm.get_plugins() == [ + "citext", + "refint", + "autoinc", + "insert_username", + "moddatetime", + ] diff --git a/tests/unit/test_db.py b/tests/unit/test_db.py index 28f5e946c8..aa0cbae1f0 100644 --- a/tests/unit/test_db.py +++ b/tests/unit/test_db.py @@ -228,7 +228,7 @@ def test_set_up_relation(harness): user = f"relation-{rel_id}" postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( - DATABASE, user, plugins=[], client_relations=[relation] + DATABASE, user, plugins=["pgaudit"], client_relations=[relation] ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() @@ -255,7 +255,7 @@ def test_set_up_relation(harness): assert harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( - DATABASE, user, plugins=[], client_relations=[relation] + DATABASE, user, plugins=["pgaudit"], client_relations=[relation] ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() @@ -276,7 +276,7 @@ def test_set_up_relation(harness): assert harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( - "application", user, plugins=[], client_relations=[relation] + "application", user, plugins=["pgaudit"], client_relations=[relation] ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index 2ccb8a0d30..35d5383f7f 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -130,7 +130,7 @@ def test_on_database_requested(harness): database_relation = harness.model.get_relation(RELATION_NAME) client_relations = [database_relation] postgresql_mock.create_database.assert_called_once_with( - DATABASE, user, plugins=[], client_relations=client_relations + DATABASE, user, plugins=["pgaudit"], client_relations=client_relations ) postgresql_mock.get_postgresql_version.assert_called_once() _update_endpoints.assert_called_once()