[DPE-3477] Reload stores (#152)

pyproject.toml

@@ -28,6 +28,7 @@ name = "zookeeper-operator"
 version = "1.0"
 description = "zookeeper-operator"
 authors = []
+package-mode = false
 
 [tool.poetry.dependencies]
 python = "^3.10"

src/managers/config.py

@@ -35,11 +35,12 @@
 secureClientPort=2182
 ssl.clientAuth=none
 ssl.quorum.clientAuth=none
-ssl.client.enable=true
 clientCnxnSocket=org.apache.zookeeper.ClientCnxnSocketNetty
 serverCnxnFactory=org.apache.zookeeper.server.NettyServerCnxnFactory
 ssl.trustStore.type=JKS
 ssl.keyStore.type=PKCS12
+sslQuorumReloadCertFiles=true
+client.certReload=true
 """
 
 

src/managers/tls.py

@@ -10,6 +10,7 @@
 
 from core.cluster import SUBSTRATES, ClusterState
 from core.workload import WorkloadBase
+from literals import GROUP, USER
 
 logger = logging.getLogger(__name__)
 
@@ -52,37 +53,93 @@ def set_certificate(self) -> None:
 
     def set_truststore(self) -> None:
         """Creates the unit Java Truststore and adds the unit CA."""
-        keytool_cmd = "charmed-zookeeper.keytool" if self.substrate == "vm" else "keytool"
-
         try:
-            self.workload.exec(
-                command=[
-                    keytool_cmd,
-                    "-import",
-                    "-v",
-                    "-alias",
-                    "ca",
-                    "-file",
-                    self.workload.paths.ca,
-                    "-keystore",
-                    self.workload.paths.truststore,
-                    "-storepass",
-                    self.state.unit_server.truststore_password,
-                    "-noprompt",
-                ],
-            )
+            self._import_ca_in_truststore()
+
             if self.substrate == "vm":
                 self.workload.exec(
-                    command=["chown", "snap_daemon:root", self.workload.paths.truststore],
+                    command=["chown", f"{USER}:{GROUP}", self.workload.paths.truststore],
                 )
-
         except (subprocess.CalledProcessError, ops.pebble.ExecError) as e:
             if "already exists" in str(e.stdout):
+                # Replacement strategy:
+                # - We need to own the file, otherwise keytool throws a permission error upon removing an entry
+                # - We need to make sure that the keystore is not empty at any point, hence the three steps.
+                #  Otherwise, ZK would pick up the file change when it's empty, and crash its internal watcher thread
+                try:
+                    if self.substrate == "vm":
+                        self.workload.exec(
+                            command=["chown", f"{GROUP}:{GROUP}", self.workload.paths.truststore],
+                        )
+                    self._rename_ca_in_truststore()
+                    self._delete_ca_in_truststore()
+                    self._import_ca_in_truststore()
+                    if self.substrate == "vm":
+                        self.workload.exec(
+                            command=["chown", f"{USER}:{GROUP}", self.workload.paths.truststore],
+                        )
+                except (subprocess.CalledProcessError, ops.pebble.ExecError) as e:
+
+                    logger.error(str(e.stdout))
+                    raise e
+
                 return
 
             logger.error(str(e.stdout))
             raise e
 
+    def _import_ca_in_truststore(self, alias: str = "ca") -> None:
+        keytool_cmd = "charmed-zookeeper.keytool" if self.substrate == "vm" else "keytool"
+        self.workload.exec(
+            command=[
+                keytool_cmd,
+                "-import",
+                "-v",
+                "-alias",
+                alias,
+                "-file",
+                self.workload.paths.ca,
+                "-keystore",
+                self.workload.paths.truststore,
+                "-storepass",
+                self.state.unit_server.truststore_password,
+                "-noprompt",
+            ],
+        )
+
+    def _rename_ca_in_truststore(self, from_alias: str = "ca", to_alias: str = "old-ca") -> None:
+        keytool_cmd = "charmed-zookeeper.keytool" if self.substrate == "vm" else "keytool"
+        self.workload.exec(
+            command=[
+                keytool_cmd,
+                "-changealias",
+                "-alias",
+                from_alias,
+                "-destalias",
+                to_alias,
+                "-keystore",
+                self.workload.paths.truststore,
+                "-storepass",
+                self.state.unit_server.truststore_password,
+            ],
+        )
+
+    def _delete_ca_in_truststore(self, alias: str = "old-ca") -> None:
+        keytool_cmd = "charmed-zookeeper.keytool" if self.substrate == "vm" else "keytool"
+        self.workload.exec(
+            command=[
+                keytool_cmd,
+                "-delete",
+                "-v",
+                "-alias",
+                alias,
+                "-keystore",
+                self.workload.paths.truststore,
+                "-storepass",
+                self.state.unit_server.truststore_password,
+            ],
+        )
+
     def set_p12_keystore(self) -> None:
         """Creates the unit Java Keystore and adds unit certificate + private-key."""
         try:
@@ -107,7 +164,7 @@ def set_p12_keystore(self) -> None:
             )
             if self.substrate == "vm":
                 self.workload.exec(
-                    command=["chown", "snap_daemon:root", self.workload.paths.keystore],
+                    command=["chown", f"{USER}:{GROUP}", self.workload.paths.keystore],
                 )
 
         except (subprocess.CalledProcessError, ops.pebble.ExecError) as e:

tests/integration/test_tls.py

@@ -4,11 +4,12 @@
 
 import asyncio
 import logging
+from subprocess import PIPE, check_output
 
 import pytest
 from pytest_operator.plugin import OpsTest
 
-from .helpers import APP_NAME, check_properties, ping_servers
+from .helpers import APP_NAME, check_properties, get_address, ping_servers
 
 logger = logging.getLogger(__name__)
 
@@ -25,6 +26,7 @@ async def test_deploy_ssl_quorum(ops_test: OpsTest, zk_charm):
             channel="edge",
             num_units=1,
             config={"ca-common-name": "zookeeper"},
+            # FIXME (certs): Unpin the revision once the charm is fixed
             revision=163,
         ),
     )
@@ -115,3 +117,28 @@ async def test_client_relate_maintains_quorum(ops_test: OpsTest):
         [APP_NAME, dummy_name], status="active", timeout=1000, idle_period=30
     )
     assert ping_servers(ops_test)
+
+
+@pytest.mark.abort_on_fail
+async def test_renew_cert(ops_test: OpsTest):
+    # invalidate previous certs
+    await ops_test.model.applications[TLS_NAME].set_config({"ca-common-name": "new-name"})
+
+    await ops_test.model.wait_for_idle([APP_NAME], status="active", timeout=1000, idle_period=30)
+    async with ops_test.fast_forward(fast_interval="20s"):
+        await asyncio.sleep(60)
+
+    # check quorum TLS
+    assert ping_servers(ops_test)
+
+    # check client-presented certs
+    host = await get_address(ops_test, unit_num=0)
+
+    response = check_output(
+        f"openssl s_client -showcerts -connect {host}:2182 < /dev/null",
+        stderr=PIPE,
+        shell=True,
+        universal_newlines=True,
+    )
+
+    assert "CN = new-name" in response

tests/unit/test_tls.py

@@ -174,6 +174,54 @@ def test_certificates_available_succeeds(harness: Harness[ZooKeeperCharm]):
     }
 
 
+def test_renew_certificates_auto_reload(harness: Harness[ZooKeeperCharm]):
+    # Setup working relation
+    harness.add_relation(CERTS_REL_NAME, TLS_NAME)
+    harness.add_relation(harness.charm.restart.name, "{CHARM_KEY}/0")
+
+    harness.update_relation_data(
+        harness.charm.state.peer_relation.id, f"{CHARM_KEY}/0", {"csr": "not-missing"}
+    )
+
+    with patch.multiple(
+        "managers.tls.TLSManager",
+        set_private_key=DEFAULT,
+        set_ca=DEFAULT,
+        set_certificate=DEFAULT,
+        set_truststore=DEFAULT,
+        set_p12_keystore=DEFAULT,
+    ):
+        harness.charm.tls_events.certificates.on.certificate_available.emit(
+            certificate_signing_request="not-missing",
+            certificate="cert",
+            ca="ca",
+            chain=["ca", "cert"],
+        )
+
+    with (
+        patch.multiple(
+            "managers.tls.TLSManager",
+            set_private_key=DEFAULT,
+            set_ca=DEFAULT,
+            set_certificate=DEFAULT,
+            set_truststore=DEFAULT,
+            set_p12_keystore=DEFAULT,
+        ),
+        patch(
+            "charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock"
+        ) as patched_restart,
+    ):
+        harness.charm.tls_events.certificates.on.certificate_available.emit(
+            certificate_signing_request="not-missing",
+            certificate="new-cert",
+            ca="ca",
+            chain=["ca", "cert"],
+        )
+
+    assert harness.charm.state.unit_server.certificate == "new-cert"
+    assert not patched_restart.called
+
+
 def test_certificates_available_halfway_through_upgrade_succeeds(harness: Harness[ZooKeeperCharm]):
     harness.add_relation(CERTS_REL_NAME, TLS_NAME)