[DPE-5962] - Implement Basic TLS workflow #13

skourta · 2024-12-17T11:21:26Z

This PR implements basic TLS workflow for enabling/disabling TLS on startup and while the cluster is running.

TLS Support Enhancements:

metadata.yaml: Added new interfaces for peer-certificates and client-certificates to support TLS certificates.
src/charm.py: Introduced TLSManager, TLSEvents, and RollingOpsManager to manage TLS transitions and rolling restarts. [1] [2]
src/core/models.py: Added TLSState enum and properties to handle TLS state and certificate readiness in EtcdServer. [1] [2]

Dependency Updates:

pyproject.toml: Updated dependencies for cryptography and pydantic to specific versions to ensure compatibility with TLS certificate handling.

`EtcdClient` Enhancements:

src/common/client.py: Added methods for member list retrieval and health checks, and updated _run_etcdctl to support TLS arguments. [1] [2]

Code Documentation:

src/common/secrets.py: Added detailed docstrings to improve code documentation and clarify the purpose and usage of methods. [1] [2]

reneradoi

Hey Smail, thanks for the implementation! It looks quite good already, I have added some questions where we need to clarify things, and some remarks for things that should be changed.

One general question: Could you please add some more unit test coverage for adding a certificate/ca, e.g. via certificate availabe?

If you need clarification, we can also schedule a call to go through the topics.

Thank you!

src/common/secrets.py

src/managers/tls.py

src/charm.py

src/common/client.py

reneradoi · 2025-01-09T11:56:43Z

src/common/client.py

+                capture_output=True,
+                input=use_input,
+                timeout=10,
+            ).stdout


Why did you remove the strip()?

I believe it is unnecessary and should not be stripped at this point. We use the function to return multiple formats including the json format which returns a parseable string directly.

In an earlier PR, Mehdi explicitly asked for it: #5 (comment)

I'll add it back. Thanks, René.

reneradoi · 2025-01-09T12:01:48Z

src/core/models.py

+        """The common name for the server certificate."""
+        cn = self.relation_data.get("common-name", "")
+        if not cn:
+            cn = f"{self.unit_name}-{uuid.uuid4()!s}"


Why do we add a unique identifier to the common name?

This came up in a discussion with Mykola. The unit_name alone might be unique in some cases like cross-model relations.

Ok, I understand that. But I thought that's what the argument add_unique_id_to_subject_name in generate_csr of the tls-lib is for?

When inspecting one of the certs written to disk, I see this:
Subject: CN = etcd/7-2403feb0-985f-4ac9-9ab3-233fe636f831, x500UniqueIdentifier = 5df60c61-6e07-4e05-8ae6-704a199f780a, O = peer

Looks like we have to uuid's now.

After discussing this in a meeting, we agreed to switch the common name back to the unit_name and leave a TODO comment to handle this in case we have cross-model relations.

We can still address it in this PR - please replace the uuid by the model UUID - it can be retrieved from: charm.model.uuid

src/events/etcd.py

reneradoi · 2025-01-09T12:07:20Z

src/events/tls.py

+        """
+        self.charm.tls_manager.set_tls_state(state=TLSState.TO_TLS)
+
+    def _on_relation_joined(self, event: RelationJoinedEvent):


I believe the logic in the handler can be removed for now, it is not really necessary to defer this event.

I wanted to block the user from adding only one relation and not the other. We need both client and peer relations to be present together. This blocks the charm until they are both created.

Edit: This comment was wrongly placed here previously.

I understand your idea and we can keep it.

Can you explain this choice? Since we ended up using the 2 relations approach, why is it not possible for the user to choose to enable TLS for only one of the 2?

reneradoi · 2025-01-09T12:09:33Z

src/events/tls.py

+        if (
+            not self.charm.state.unit_server.peer_cert_ready
+            and not self.charm.state.unit_server.client_cert_ready
+        ):
+            self.charm.rolling_restart()


That means removing only one of the relations (e.g. switching the client certificate provider) would not restart the units. Is this a good idea? We would then leave the cluster in Error state forever, no?

My line of thinking was to enable/disable both the client and peer together we do not want a state where only one of the relations is there and the other is not.

We should further discuss this scenario.

Agree. We either need to find a way to automate replacing only one of the cert providers, or document it in the user docs how to handle this use case.

After discussing this in a meeting, I will perform manual tests to ensure the code handles this case. The rolling restart should be initiated when the new relation is created and the new certs arrive.

I've tested it and it fails:

juju integrate etcd:peer-certificates self-signed-certificates:certificates

juju integrate etcd:client-certificates self-signed-certificates:certificates

juju remove-relation etcd:client-certificates self-signed-certificates:certificates

juju integrate etcd:client-certificates self-signed-certificates:certificates

results in:

etcd/9* blocked idle 12 10.105.253.156 failed to transition to/from tls state etcd/10 blocked idle 10 10.105.253.228 failed to transition to/from tls state etcd/11 blocked idle 11 10.105.253.70 failed to transition to/from tls state

debug-log:

unit-etcd-11: 08:43:07 DEBUG unit.etcd/11.juju-log restart:12: Member list: None unit-etcd-11: 08:43:07 ERROR unit.etcd/11.juju-log restart:12: Enabling TLS failed: member list command failed unit-etcd-11: 08:43:07 ERROR unit.etcd/11.juju-log restart:12: failed to transition to/from tls state

We need to make a decision here, are we supporting 1 type of certificates or it's either 2 or 0.

src/managers/cluster.py

reneradoi · 2025-01-09T16:15:43Z

src/managers/tls.py

+            if key_path.exists():
+                key_path.unlink()
+
+            if ca_path.exists():
+                ca_path.unlink()


I've found a small issue by testing: when the client-certificates relation is removed, the key and ca-cert remain on disk:

root@juju-e1b312-7:/var/snap/charmed-etcd/common/tls# ls client.key client_ca.pem

I believe this is because the key_path and ca_path variables are still set to the peer paths.

Yes, that is true. I missed adding ca_path = Path(self.workload.paths.tls.client_ca) under the CertType.CLIENT condition.

reneradoi · 2025-01-10T14:35:19Z

tests/integration/helpers.py

 ) -> str:
    """Write data to etcd using `etcdctl` via `juju ssh`."""
    etcd_command = f"{SNAP_NAME}.etcdctl put {key} {value} --endpoints={endpoints}"
    if user:
        etcd_command = f"{etcd_command} --user={user}"
    if password:
        etcd_command = f"{etcd_command} --password={password}"
+    if tls_enabled:
+        etcd_command = f"{etcd_command} --cacert /var/snap/charmed-etcd/common/tls/client_ca.pem --cert /var/snap/charmed-etcd/common/tls/client.pem --key /var/snap/charmed-etcd/common/tls/client.key"


I'm sorry Smail for bringing up another one after you already worked on all my previous comments. Could you please make this a multiline string?

etcd_command = f"""{etcd_command} \ --cacert ... \ --cert ... \ ...

That would make it easier to read.

Mehdi-Bendriss

Thanks Smail, great work! I left some comments and some questions

Mehdi-Bendriss · 2025-01-11T15:57:31Z

src/charm.py

+        if self.state.unit_server.tls_state == TLSState.TO_TLS:
+            try:
+                logger.debug("Enabling TLS through rolling restart")
+                logger.debug("Broadcasting new peer url")


To not pollute the charm.py with too many log lines, this and similar debug logs should be moved to the cluster_manager.broadcast_peer_url and other manager methods.

Mehdi-Bendriss · 2025-01-11T15:59:15Z

src/charm.py

+                logger.debug("Writing configuration")
+                self.config_manager.set_config_properties()
+
+                self.tls_manager.set_tls_state(state=TLSState.TLS)


I don't think that this should be the job of the manager which only acts as a proxy to self.charm.state. Consider doing it here directly

Mehdi-Bendriss · 2025-01-11T16:00:18Z

src/charm.py

+                    raise Exception("Failed to check health of the member after restart")
+
+            except Exception as e:
+                logger.error(f"Enabling TLS failed: {e}")
+                self.set_status(Status.TLS_TRANSITION_FAILED)


no need to raise an exception here, log and directly set the status instead.

Mehdi-Bendriss · 2025-01-11T16:04:30Z

src/charm.py

+                logger.debug("Enabling TLS through rolling restart")
+                logger.debug("Broadcasting new peer url")
+                self.cluster_manager.broadcast_peer_url(
+                    self.state.unit_server.peer_url.replace("http://", "https://")


The state.unit_server.peer_url is no longer up-to-date if we do this.
We should rather:

introduce a new state: scheme which will hold / return either values: http, https

update the state here

broadcast_peer_url can take no argument, and will fetch the right peer_url and peer_scheme and form the full url.

I think the events/tls module is better suited for changing the state - the restart should rather be dumber imo

Mehdi-Bendriss · 2025-01-11T16:24:00Z

src/charm.py

@@ -44,6 +52,57 @@ def set_status(self, key: Status) -> None:
        getattr(logger, log_level.lower())(status.message)
        self.unit.status = status

+    def _restart(self, _) -> None:


In general, this method carries quite a bit of duplicated logic, incorporating the feedback points, we could simplify it like:

def _restart(self, _) -> None: """Restart callback for the rolling ips lib.""" status_on_failure = FAILED_TO_RESTART if self.state.unit_server.tls_state == TLSState.TO_TLS: logger.debug("Enabling TLS through rolling restart") self.state.unit_server.update({"peer-scheme": "https", "tls-state": TLSState.TLS}) status_on_failure = Status.TLS_TRANSITION_FAILED elif self.state.unit_server.tls_state == TLSState.TO_NO_TLS: logger.debug("Disabling TLS through rolling restart") self.tls_manager.delete_certificates() self.state.unit_server.update({"peer-scheme": "http", "tls-state": TLSState.NO_TLS}) status_on_failure = Status.TLS_TRANSITION_FAILED self.cluster_manager.broadcast_peer_url() self.config_manager.set_config_properties() if not self.cluster_manager.restart_member(): self.set_status(status_on_failure) logger.error("Failed to check health of the member after restart")

Mehdi-Bendriss · 2025-01-11T21:38:58Z

tests/integration/test_tls.py

+            key=TEST_KEY,
+        )
+        == TEST_VALUE
+    )


add a new .put (different key/value and assert on it)

Mehdi-Bendriss · 2025-01-11T21:39:24Z

tests/integration/test_tls.py

+@pytest.mark.runner(["self-hosted", "linux", "X64", "jammy", "large"])
+@pytest.mark.group(1)
+@pytest.mark.abort_on_fail
+async def test_turning_on_tls(ops_test: OpsTest) -> None:


Suggested change

async def test_turning_on_tls(ops_test: OpsTest) -> None:

async def test_enable_tls(ops_test: OpsTest) -> None:

Mehdi-Bendriss · 2025-01-11T21:39:38Z

tests/integration/test_tls.py

+    assert ops_test.model
+    model = ops_test.model_full_name
+    assert model is not None


Similar comment, is this needed?

Mehdi-Bendriss · 2025-01-11T21:39:52Z

tests/integration/test_tls.py

+
+    # enable TLS and check if the cluster is still accessible
+    await ops_test.model.integrate(f"{APP_NAME}:peer-certificates", TLS_NAME)
+    await ops_test.model.integrate(f"{APP_NAME}:client-certificates", TLS_NAME)


similar comment on status

Mehdi-Bendriss · 2025-01-11T21:40:06Z

tests/integration/test_tls.py

+            tls_enabled=True,
+        )
+        == TEST_VALUE
+    )


needs a .put here

skourta added 13 commits January 8, 2025 15:34

fetched lib and added interface in metadata

21a339c

added dependencies for the tls lib

9ed03f5

added etcdpaths dataclass

8d75058

feat: turn on TLS

ea586d1

feat: turn off TLS

14d012b

add some unit tests and fix failing unit test for the charm

705a9ac

fix typo

d6475d7

remove uneeded deps in charmcraft.yaml and update poetry.lock

949302a

add integration tests

ac355e6

added rolling ops to handle activating and deactivating tls

7b76812

enhanced docstrings

1ee73df

fixed enabling tls on cluster creation

7c81778

fixed unit tests and tweaked integration tests

ea43909

skourta force-pushed the DPE-5962-tls-implement-basic-flow branch from 0a3ece6 to ea43909 Compare January 8, 2025 15:35

use 3 units instead of 2 in tls integration tests

d07de5d

skourta marked this pull request as ready for review January 8, 2025 15:52

skourta requested review from Mehdi-Bendriss and reneradoi January 8, 2025 15:52

reneradoi requested changes Jan 9, 2025

View reviewed changes

fixed based on feedback

d3203e5

reneradoi reviewed Jan 9, 2025

View reviewed changes

skourta added 2 commits January 10, 2025 10:10

couple of fixed based on feedback

cd3c4e8

added more unit tests

dd67d30

reneradoi reviewed Jan 10, 2025

View reviewed changes

Mehdi-Bendriss requested changes Jan 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-5962] - Implement Basic TLS workflow #13

[DPE-5962] - Implement Basic TLS workflow #13

skourta commented Dec 17, 2024 •

edited

Loading

reneradoi left a comment

reneradoi Jan 9, 2025

skourta Jan 9, 2025

reneradoi Jan 9, 2025

skourta Jan 10, 2025

reneradoi Jan 9, 2025

skourta Jan 9, 2025

reneradoi Jan 9, 2025

skourta Jan 10, 2025

Mehdi-Bendriss Jan 11, 2025

reneradoi Jan 9, 2025

skourta Jan 9, 2025

reneradoi Jan 9, 2025 •

edited

Loading

Mehdi-Bendriss Jan 11, 2025

reneradoi Jan 9, 2025

skourta Jan 9, 2025 •

edited

Loading

reneradoi Jan 9, 2025

skourta Jan 10, 2025

reneradoi Jan 10, 2025

Mehdi-Bendriss Jan 11, 2025

reneradoi Jan 9, 2025 •

edited

Loading

skourta Jan 10, 2025

reneradoi Jan 10, 2025

Mehdi-Bendriss left a comment

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025 •

edited

Loading

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

Mehdi-Bendriss Jan 11, 2025

	async def test_turning_on_tls(ops_test: OpsTest) -> None:
	async def test_enable_tls(ops_test: OpsTest) -> None:

[DPE-5962] - Implement Basic TLS workflow #13

Are you sure you want to change the base?

[DPE-5962] - Implement Basic TLS workflow #13

Conversation

skourta commented Dec 17, 2024 • edited Loading

TLS Support Enhancements:

Dependency Updates:

EtcdClient Enhancements:

Code Documentation:

reneradoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reneradoi Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skourta Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reneradoi Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mehdi-Bendriss Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skourta commented Dec 17, 2024 •

edited

Loading

`EtcdClient` Enhancements:

reneradoi Jan 9, 2025 •

edited

Loading

skourta Jan 9, 2025 •

edited

Loading

reneradoi Jan 9, 2025 •

edited

Loading

Mehdi-Bendriss Jan 11, 2025 •

edited

Loading