From c5696736e6582b8e93794f6fe15f356ac620efa0 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Fri, 27 Sep 2024 09:44:17 +0200 Subject: [PATCH] [DPE-5306][SECURITY] No write before protocol init (#194) --- .../data_platform_libs/v0/data_interfaces.py | 21 +- tests/integration/conftest.py | 2 +- tests/unit/test_data_interfaces.py | 208 +++++++++++++++--- 3 files changed, 204 insertions(+), 27 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index aaed2e52..3bc2dd85 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 39 +LIBPATCH = 40 PYDEPS = ["ops>=2.0.0"] @@ -391,6 +391,10 @@ class IllegalOperationError(DataInterfacesError): """To be used when an operation is not allowed to be performed.""" +class PrematureDataAccessError(DataInterfacesError): + """To be raised when the Relation Data may be accessed (written) before protocol init complete.""" + + ############################################################################## # Global helpers / utilities ############################################################################## @@ -1453,6 +1457,8 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: class ProviderData(Data): """Base provides-side of the data products relation.""" + RESOURCE_FIELD = "database" + def __init__( self, model: Model, @@ -1618,6 +1624,15 @@ def _fetch_my_specific_relation_data( def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: """Set values for fields not caring whether it's a secret or not.""" req_secret_fields = [] + + keys = set(data.keys()) + if self.fetch_relation_field(relation.id, self.RESOURCE_FIELD) is None and ( + keys - {"endpoints", "read-only-endpoints", "replset"} + ): + raise PrematureDataAccessError( + "Premature access to relation data, update is forbidden before the connection is initialized." + ) + if relation.app: req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) @@ -3290,6 +3305,8 @@ class KafkaRequiresEvents(CharmEvents): class KafkaProviderData(ProviderData): """Provider-side of the Kafka relation.""" + RESOURCE_FIELD = "topic" + def __init__(self, model: Model, relation_name: str) -> None: super().__init__(model, relation_name) @@ -3539,6 +3556,8 @@ class OpenSearchRequiresEvents(CharmEvents): class OpenSearchProvidesData(ProviderData): """Provider-side of the OpenSearch relation.""" + RESOURCE_FIELD = "index" + def __init__(self, model: Model, relation_name: str) -> None: super().__init__(model, relation_name) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 730486f6..35da09bd 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -189,7 +189,7 @@ def fetch_old_versions(): for commit in last_commits: check_call(f"git checkout {commit}", shell=True) version = check_output( - "grep LIBPATCH lib/charms/data_platform_libs/v0/data_interfaces.py | cut -d ' ' -f 3", + "grep ^LIBPATCH lib/charms/data_platform_libs/v0/data_interfaces.py | cut -d ' ' -f 3", shell=True, universal_newlines=True, ) diff --git a/tests/unit/test_data_interfaces.py b/tests/unit/test_data_interfaces.py index f74da7d5..b99b8b63 100644 --- a/tests/unit/test_data_interfaces.py +++ b/tests/unit/test_data_interfaces.py @@ -38,6 +38,7 @@ KafkaRequires, OpenSearchProvides, OpenSearchRequires, + PrematureDataAccessError, TopicRequestedEvent, ) from charms.harness_extensions.v0.capture_events import capture, capture_events @@ -347,6 +348,7 @@ def _on_index_requested(self, _) -> None: class DataProvidesBaseTests(ABC): SECRET_FIELDS = ["username", "password", "tls", "tls-ca", "uris"] + DATABASE_FIELD = "database" @pytest.fixture def use_caplog(self, caplog): @@ -402,24 +404,60 @@ def test_diff(self): def test_relation_interface(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_functions(interface, self.rel_id) def test_relation_interface_dict(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_dict(interface, self.rel_id) verify_relation_interface_dict_external_relation(interface, self.rel_id) + def test_relation_interface_premature_update(self): + """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + interface = self.harness.charm.provider + + with pytest.raises(PrematureDataAccessError): + interface.update_relation_data(self.rel_id, {"blah": "blabbla"}) + + # Interface function: fetch_relation_field() + interface.update_relation_data(self.rel_id, {"endpoints": "host1:port1"}) + interface.update_relation_data(self.rel_id, {"read-only-endpoints": "host2:port2"}) + interface.update_relation_data(self.rel_id, {"replset": "mongo-replset"}) + + assert interface.fetch_my_relation_field(self.rel_id, "endpoints") == "host1:port1" + assert ( + interface.fetch_my_relation_field(self.rel_id, "read-only-endpoints") == "host2:port2" + ) + assert interface.fetch_my_relation_field(self.rel_id, "replset") == "mongo-replset" + @pytest.mark.usefixtures("only_without_juju_secrets") def test_set_credentials(self): """Asserts that the database name is in the relation databag when it's requested.""" + # Set some data in the relation. + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the credentials in the relation using the provides charm library. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") # Check that the credentials are present in the relation. assert self.harness.get_relation_data(self.rel_id, self.app_name) == { - "data": "{}", # Data is the diff stored between multiple relation changed events. + "data": json.dumps( + {self.DATABASE_FIELD: DATABASE} + ), # Data is the diff stored between multiple relation changed events. "username": "test-username", "password": "test-password", } @@ -427,13 +465,19 @@ def test_set_credentials(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_set_credentials_secrets(self): """Asserts that credentials are set up as secrets if possible.""" + # Set some data in the relation. + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the credentials in the relation using the provides charm library. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") # Check that the credentials are present in the relation. assert json.loads(self.harness.get_relation_data(self.rel_id, self.app_name)["data"]) == ( { - REQ_SECRET_FIELDS: json.dumps(self.SECRET_FIELDS) + REQ_SECRET_FIELDS: json.dumps(self.SECRET_FIELDS), + self.DATABASE_FIELD: DATABASE, } # Data is the diff stored between multiple relation changed events. # noqa ) secret_id = self.harness.get_relation_data(self.rel_id, self.app_name)[ @@ -447,13 +491,20 @@ def test_set_credentials_secrets(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_set_credentials_secrets_provides_juju3_requires_juju2(self): """Asserts that the databag is used if one side of the relation is on Juju2.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + self.harness.update_relation_data(self.rel_id, "application", {REQ_SECRET_FIELDS: ""}) # Set the credentials in the relation using the provides charm library. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") # Check that the credentials are present in the relation. assert self.harness.get_relation_data(self.rel_id, self.app_name) == { - "data": "{}", # Data is the diff stored between multiple relation changed events. + "data": json.dumps( + {self.DATABASE_FIELD: DATABASE} + ), # Data is the diff stored between multiple relation changed events. "username": "test-username", "password": "test-password", } @@ -728,29 +779,45 @@ def test_peer_relation_other_unit_dict(self): @pytest.mark.usefixtures("only_without_juju_secrets") def test_provider_interface_functions(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_functions(interface, self.rel_id) rel_data = interface.fetch_relation_data() - assert rel_data == {0: {}} + assert rel_data == {0: {self.DATABASE_FIELD: DATABASE}} rel_data = interface.fetch_my_relation_data() - assert rel_data == {0: {"data": json.dumps({})}} + assert rel_data == {0: {"data": json.dumps({self.DATABASE_FIELD: DATABASE})}} @pytest.mark.usefixtures("only_with_juju_secrets") def test_provider_interface_functions_secrets(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_functions(interface, self.rel_id) rel_data = interface.fetch_relation_data() assert rel_data == { - 0: {"requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]'} + 0: { + "requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]', + self.DATABASE_FIELD: DATABASE, + } } rel_data = interface.fetch_my_relation_data() assert rel_data == { 0: { "data": json.dumps( - {"requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]'} + { + "requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]', + self.DATABASE_FIELD: DATABASE, + } ) } } @@ -758,24 +825,41 @@ def test_provider_interface_functions_secrets(self): @pytest.mark.usefixtures("only_without_juju_secrets") def test_provider_interface_dict(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_dict(interface, self.rel_id) - assert interface.as_dict(self.rel_id) == {"data": json.dumps({})} + assert interface.as_dict(self.rel_id) == { + self.DATABASE_FIELD: DATABASE, + "data": json.dumps({self.DATABASE_FIELD: DATABASE}), + } @pytest.mark.usefixtures("use_caplog") @pytest.mark.usefixtures("only_with_juju_secrets") def test_provider_interface_dict_secrets(self): """Check the functionality of each public interface function.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + interface = self.harness.charm.provider verify_relation_interface_dict(interface, self.rel_id) datadict = interface.as_dict(self.rel_id) assert datadict == { "data": json.dumps( - {"requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]'} + { + "requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]', + self.DATABASE_FIELD: DATABASE, + } ), "requested-secrets": '["username", "password", "tls", "tls-ca", "uris"]', + self.DATABASE_FIELD: DATABASE, } # Non-leader can fetch the data @@ -797,7 +881,7 @@ def test_on_database_requested(self, _on_database_requested): self.harness.update_relation_data( self.rel_id, "application", - {"database": DATABASE, "extra-user-roles": EXTRA_USER_ROLES}, + {self.DATABASE_FIELD: DATABASE, "extra-user-roles": EXTRA_USER_ROLES}, ) # Assert the correct hook is called. @@ -814,13 +898,18 @@ def test_on_database_requested(self, _on_database_requested): self.harness.update_relation_data( self.rel_id, "application", - {"database": DATABASE, "external-node-connectivity": "true"}, + {self.DATABASE_FIELD: DATABASE, "external-node-connectivity": "true"}, ) event = _on_database_requested.call_args[0][0] assert event.external_node_connectivity is True def test_set_endpoints(self): """Asserts that the endpoints are in the relation databag when they change.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the endpoints in the relation using the provides charm library. self.harness.charm.provider.set_endpoints(self.rel_id, "host1:port,host2:port") @@ -832,6 +921,11 @@ def test_set_endpoints(self): def test_set_read_only_endpoints(self): """Asserts that the read only endpoints are in the relation databag when they change.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the endpoints in the relation using the provides charm library. self.harness.charm.provider.set_read_only_endpoints(self.rel_id, "host1:port,host2:port") @@ -844,6 +938,11 @@ def test_set_read_only_endpoints(self): @pytest.mark.usefixtures("only_without_juju_secrets") def test_set_additional_fields(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_replset(self.rel_id, "rs0") self.harness.charm.provider.set_tls(self.rel_id, "True") @@ -853,7 +952,9 @@ def test_set_additional_fields(self): # Check that the additional fields are present in the relation. assert self.harness.get_relation_data(self.rel_id, "database") == { - "data": "{}", # Data is the diff stored between multiple relation changed events. + "data": json.dumps( + {self.DATABASE_FIELD: DATABASE} + ), # Data is the diff stored between multiple relation changed events. "replset": "rs0", "tls": "True", "tls-ca": "Canonical", @@ -864,6 +965,11 @@ def test_set_additional_fields(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_set_additional_fields_secrets(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_replset(self.rel_id, "rs0") self.harness.charm.provider.set_tls(self.rel_id, "True") @@ -877,9 +983,9 @@ def test_set_additional_fields_secrets(self): assert secret_id relation_data == { - "data": f'{REQ_SECRET_FIELDS}: "' - + f"{json.dumps(self.SECRET_FIELDS)}" - + '"}', # Data is the diff stored between multiple relation changed events. # noqa + "data": json.dumps( + {REQ_SECRET_FIELDS: self.SECRET_FIELDS, self.DATABASE_FIELD: DATABASE} + ), # Data is the diff stored between multiple relation changed events. # noqa "replset": "rs0", "version": "1.0", "uris": "host1:port,host2:port", @@ -991,6 +1097,11 @@ def test_fetch_relation_data_and_field_secrets(self): @pytest.mark.usefixtures("use_caplog") @pytest.mark.usefixtures("only_without_juju_secrets") def test_fetch_my_relation_data_and_field(self): + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set some data in the relation. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") self.harness.update_relation_data(self.rel_id, "database", {"somedata": "somevalue"}) @@ -1003,7 +1114,7 @@ def test_fetch_my_relation_data_and_field(self): "username": "test-username", "password": "test-password", "somedata": "somevalue", - "data": json.dumps({}), + "data": json.dumps({"database": DATABASE}), } } @@ -1042,6 +1153,11 @@ def test_fetch_my_relation_data_and_field(self): @pytest.mark.usefixtures("use_caplog") @pytest.mark.usefixtures("only_with_juju_secrets") def test_fetch_my_relation_data_and_field_secrets(self): + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set some data in the relation. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") self.harness.update_relation_data(self.rel_id, "database", {"somedata": "somevalue"}) @@ -1054,7 +1170,12 @@ def test_fetch_my_relation_data_and_field_secrets(self): "username": "test-username", "password": "test-password", "somedata": "somevalue", - "data": json.dumps({REQ_SECRET_FIELDS: json.dumps(self.SECRET_FIELDS)}), + "data": json.dumps( + { + REQ_SECRET_FIELDS: json.dumps(self.SECRET_FIELDS), + self.DATABASE_FIELD: DATABASE, + } + ), } } @@ -1092,6 +1213,11 @@ def test_fetch_my_relation_data_and_field_secrets(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_delete_relation_data_and_field_secrets(self): + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Set some data in the relation. self.harness.charm.provider.set_credentials(self.rel_id, "test-username", "test-password") secret_id = self.harness.get_relation_data(self.rel_id, self.app_name)["secret-user"] @@ -1252,6 +1378,8 @@ class TestKafkaProvides(DataProvidesBaseTests, unittest.TestCase): app_name = "kafka" charm = KafkaCharm + DATABASE_FIELD = "topic" + def get_harness(self) -> Tuple[Harness, int]: harness = Harness(self.charm, meta=self.metadata) # Set up the initial relation and hooks. @@ -1294,6 +1422,9 @@ def test_on_topic_requested(self, _on_topic_requested): def test_set_bootstrap_server(self): """Asserts that the bootstrap-server are in the relation databag when they change.""" + # We pretend that the connection is initialized + self.harness.update_relation_data(self.rel_id, "application", {self.DATABASE_FIELD: TOPIC}) + # Set the bootstrap-server in the relation using the provides charm library. self.harness.charm.provider.set_bootstrap_server(self.rel_id, "host1:port,host2:port") @@ -1306,6 +1437,9 @@ def test_set_bootstrap_server(self): @pytest.mark.usefixtures("only_without_juju_secrets") def test_set_additional_fields(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data(self.rel_id, "application", {self.DATABASE_FIELD: TOPIC}) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_tls(self.rel_id, "True") self.harness.charm.provider.set_tls_ca(self.rel_id, "Canonical") @@ -1314,7 +1448,9 @@ def test_set_additional_fields(self): # Check that the additional fields are present in the relation. assert self.harness.get_relation_data(self.rel_id, self.app_name) == { - "data": "{}", # Data is the diff stored between multiple relation changed events. + "data": json.dumps( + {self.DATABASE_FIELD: TOPIC} + ), # Data is the diff stored between multiple relation changed events. "tls": "True", "tls-ca": "Canonical", "zookeeper-uris": "host1:port,host2:port", @@ -1324,6 +1460,9 @@ def test_set_additional_fields(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_set_additional_fields_secrets(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data(self.rel_id, "application", {self.DATABASE_FIELD: TOPIC}) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_tls(self.rel_id, "True") self.harness.charm.provider.set_tls_ca(self.rel_id, "Canonical") @@ -1336,9 +1475,9 @@ def test_set_additional_fields_secrets(self): assert secret_id relation_data == { - "data": '{REQ_SECRET_FIELDS: "' - + f"{json.dumps(self.SECRET_FIELDS)}" - + '"}', # Data is the diff stored between multiple relation changed events. # noqa + "data": json.dumps( + {REQ_SECRET_FIELDS: self.SECRET_FIELDS} + ), # Data is the diff stored between multiple relation changed events. # noqa "zookeeper-uris": "host1:port,host2:port", "consumer-group-prefix": "pr1,pr2", } @@ -1394,6 +1533,8 @@ class TestOpenSearchProvides(DataProvidesBaseTests, unittest.TestCase): app_name = "opensearch" charm = OpenSearchCharm + DATABASE_FIELD = "index" + def get_harness(self) -> Tuple[Harness, int]: harness = Harness(self.charm, meta=self.metadata) # Set up the initial relation and hooks. @@ -1437,18 +1578,26 @@ def test_on_index_requested(self, _on_index_requested): @pytest.mark.usefixtures("only_without_juju_secrets") def test_set_additional_fields(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data(self.rel_id, "application", {self.DATABASE_FIELD: INDEX}) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_tls_ca(self.rel_id, "Canonical") # Check that the additional fields are present in the relation. assert self.harness.get_relation_data(self.rel_id, self.app_name) == { - "data": "{}", # Data is the diff stored between multiple relation changed events. + "data": json.dumps( + {self.DATABASE_FIELD: INDEX} + ), # Data is the diff stored between multiple relation changed events. "tls-ca": "Canonical", } @pytest.mark.usefixtures("only_with_juju_secrets") def test_set_additional_fields_secrets(self): """Asserts that the additional fields are in the relation databag when they are set.""" + # We pretend that the connection is initialized + self.harness.update_relation_data(self.rel_id, "application", {self.DATABASE_FIELD: INDEX}) + # Set the additional fields in the relation using the provides charm library. self.harness.charm.provider.set_tls_ca(self.rel_id, "Canonical") @@ -1458,9 +1607,9 @@ def test_set_additional_fields_secrets(self): assert secret_id relation_data == { - "data": '{REQ_SECRET_FIELDS: "' - + f"{json.dumps(self.SECRET_FIELDS)}" - + '"}', # Data is the diff stored between multiple relation changed events. # noqa + "data": json.dumps( + {REQ_SECRET_FIELDS: self.SECRET_FIELDS} + ), # Data is the diff stored between multiple relation changed events. # noqa } secret = self.harness.charm.model.get_secret(id=secret_id) @@ -1776,6 +1925,8 @@ class TestDatabaseRequires(DataRequirerBaseTests, unittest.TestCase): app_name = "application" provider = "database" + DATABASE_FIELD = "database" + def setUp(self): self.harness = self.get_harness() self.rel_id = self.add_relation(self.harness, self.provider) @@ -2128,6 +2279,11 @@ def test_fetch_my_relation_data_and_fields_secrets(self): @pytest.mark.usefixtures("only_with_juju_secrets") def test_on_database_created_requires_juju3_provides_juju2(self, _on_database_created): """Asserts that the databag is used if one side of the relation is on Juju2.""" + # We pretend that the connection is initialized + self.harness.update_relation_data( + self.rel_id, "application", {self.DATABASE_FIELD: DATABASE} + ) + # Simulate sharing the credentials of a new created database. assert not self.harness.charm.requirer.is_resource_created() @@ -2729,6 +2885,8 @@ class TestOpenSearchRequires(DataRequirerBaseTests, unittest.TestCase): relation_name = OPENSEARCH_RELATION_NAME charm = ApplicationCharmOpenSearch + DATABASE_FIELD = "index" + app_name = "application" provider = "opensearch"