From 50813f102ab838609789518f53eefacbef99ca4d Mon Sep 17 00:00:00 2001
From: marcoppenheimer <51744472+marcoppenheimer@users.noreply.github.com>
Date: Thu, 5 Sep 2024 00:16:25 +0100
Subject: [PATCH] [DPE-5208] - fix: enforce client auth (#100)

* fix: enforce sasl client auth

* chore: fix failing tls unit-test

* chore: restore ssl.*.clientAuth=none
---
 lib/charms/zookeeper/v0/client.py | 10 +++++-----
 src/managers/config.py            |  3 +++
 src/managers/quorum.py            |  5 +----
 tests/unit/test_config.py         | 13 ++++++++++---
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/lib/charms/zookeeper/v0/client.py b/lib/charms/zookeeper/v0/client.py
index e7f03cc5..84e0ab35 100644
--- a/lib/charms/zookeeper/v0/client.py
+++ b/lib/charms/zookeeper/v0/client.py
@@ -74,7 +74,7 @@ def update_cluster(new_members: List[str], event: EventBase) -> None:
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 6
+LIBPATCH = 7
 
 
 logger = logging.getLogger(__name__)
@@ -369,7 +369,7 @@ def leader_znodes(self, path: str) -> Set[str]:
 
         return all_znode_children
 
-    def create_znode_leader(self, path: str, acls: List[ACL]) -> None:
+    def create_znode_leader(self, path: str, acls: List[ACL] | None = None) -> None:
         """Creates a new zNode on the current quorum leader with given ACLs.
 
         Args:
@@ -388,7 +388,7 @@ def create_znode_leader(self, path: str, acls: List[ACL]) -> None:
         ) as zk:
             zk.create_znode(path=path, acls=acls)
 
-    def set_acls_znode_leader(self, path: str, acls: List[ACL]) -> None:
+    def set_acls_znode_leader(self, path: str, acls: List[ACL] | None = None) -> None:
         """Updates ACLs for an existing zNode on the current quorum leader.
 
         Args:
@@ -577,7 +577,7 @@ def delete_znode(self, path: str) -> None:
             return
         self.client.delete(path, recursive=True)
 
-    def create_znode(self, path: str, acls: List[ACL]) -> None:
+    def create_znode(self, path: str, acls: List[ACL] | None = None) -> None:
         """Create new znode.
 
         Args:
@@ -599,7 +599,7 @@ def get_acls(self, path: str) -> List[ACL]:
 
         return acl_list if acl_list else []
 
-    def set_acls(self, path: str, acls: List[ACL]) -> None:
+    def set_acls(self, path: str, acls: List[ACL] | None = None) -> None:
         """Sets acls for a desired znode path.
 
         Args:
diff --git a/src/managers/config.py b/src/managers/config.py
index f875c9a2..ce615097 100644
--- a/src/managers/config.py
+++ b/src/managers/config.py
@@ -28,6 +28,9 @@
 quorum.auth.learnerRequireSasl=true
 quorum.auth.serverRequireSasl=true
 authProvider.sasl=org.apache.zookeeper.server.auth.SASLAuthenticationProvider
+enforce.auth.enabled=true
+enforce.auth.schemes=sasl
+sessionRequireClientSASLAuth=true
 audit.enable=true
 """
 
diff --git a/src/managers/quorum.py b/src/managers/quorum.py
index a71dbe04..768d1775 100644
--- a/src/managers/quorum.py
+++ b/src/managers/quorum.py
@@ -191,7 +191,6 @@ def update_acls(self, event: RelationEvent | None = None) -> None:
         leader_chroots = self.client.leader_znodes(path="/")
         logger.debug(f"{leader_chroots=}")
 
-        requested_acls = set()
         requested_chroots = set()
 
         for client in self.state.clients:
@@ -209,8 +208,6 @@ def update_acls(self, event: RelationEvent | None = None) -> None:
             )
             logger.info(f"{generated_acl=}")
 
-            requested_acls.add(generated_acl)
-
             # FIXME: data-platform-libs should handle this when it's implemented
             if client.database:
                 if event and client.relation and client.relation.id == event.relation.id:
@@ -224,7 +221,7 @@ def update_acls(self, event: RelationEvent | None = None) -> None:
                 self.client.create_znode_leader(path=client.database, acls=[generated_acl])
 
             # Looks for existing related applications
-            logger.info(f"UPDATE CHROOT - {client.database}")
+            logger.debug(f"UPDATE CHROOT - {client.database}")
             self.client.set_acls_znode_leader(path=client.database, acls=[generated_acl])
 
         # Looks for applications no longer in the relation but still in config
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index d6628825..059c3e7a 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -132,14 +132,21 @@ def test_multiple_jaas_users_are_added(harness):
 def test_tls_enabled(harness):
     with harness.hooks_disabled():
         harness.update_relation_data(
-            harness.charm.state.peer_relation.id, CHARM_KEY, {"tls": "enabled"}
+            harness.charm.state.peer_relation.id,
+            CHARM_KEY,
+            {"tls": "enabled", "quorum": "ssl"},
+        )
+        harness.update_relation_data(
+            harness.charm.state.peer_relation.id,
+            f"{CHARM_KEY}/0",
+            {"certificate": "foo"},
         )
 
-    assert "ssl.clientAuth=none" in harness.charm.config_manager.zookeeper_properties
+    assert "sslQuorum=true" in harness.charm.config_manager.zookeeper_properties
 
 
 def test_tls_disabled(harness):
-    assert "ssl.clientAuth=none" not in harness.charm.config_manager.zookeeper_properties
+    assert "sslQuorum=true" not in harness.charm.config_manager.zookeeper_properties
 
 
 def test_tls_switching_encryption(harness):