From 4086e3ba3fee02c139919c15b2ca07ca86c68efe Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Tue, 19 Sep 2023 19:22:33 +0300 Subject: [PATCH] TLS fixes (#245) * Drop incorrect rootCAs section for e2e TLS * Push "recv ca certs" on upgrade --- src/charm.py | 44 ++++++++++++++++++++++------- tests/unit/test_charm.py | 2 ++ tests/unit/test_route.py | 1 + tests/unit/test_tls_certificates.py | 1 + 4 files changed, 38 insertions(+), 10 deletions(-) diff --git a/src/charm.py b/src/charm.py index b829d991..36bf1fbc 100755 --- a/src/charm.py +++ b/src/charm.py @@ -11,6 +11,7 @@ import re import socket import typing +from string import Template from typing import Any, Dict, List, Optional, Union from urllib.parse import urlparse @@ -85,7 +86,7 @@ _SERVER_KEY_PATH = f"{_DYNAMIC_CONFIG_DIR}/server.key" _CA_CERTS_PATH = "/usr/local/share/ca-certificates" _CA_CERT_PATH = f"{_CA_CERTS_PATH}/traefik-ca.crt" - +_RECV_CA_TEMPLATE = Template(f"{_CA_CERTS_PATH}/receive-ca-cert-$rel_id-ca.crt") BIN_PATH = "/usr/bin/traefik" @@ -251,13 +252,35 @@ def __init__(self, *args): def _on_recv_ca_cert_available(self, event: CertificateTransferAvailableEvent): # Assuming only one cert per relation (this is in line with the original lib design). - target = f"{_CA_CERTS_PATH}/receive-ca-cert-{event.relation_id}-ca.crt" - self.container.push(target, event.ca, make_dirs=True) + self._update_received_ca_certs(event) + + def _update_received_ca_certs(self, event: Optional[CertificateTransferAvailableEvent] = None): + """Push the cert attached to the event, if it is given; otherwise push all certs. + + This function is needed because relation events are not emitted on upgrade, and because we + do not have (nor do we want) persistent storage for certs. + Calling this function from upgrade-charm might be too early though. Pebble-ready is + preferred. + """ + if event: + self.container.push( + _RECV_CA_TEMPLATE.substitute(rel_id=event.relation_id), event.ca, make_dirs=True + ) + else: + for relation in self.model.relations.get(self.recv_ca_cert.relationship_name, []): + # For some reason, relation.units includes our unit and app. Need to exclude them. + for unit in set(relation.units).difference([self.app, self.unit]): + # Note: this nested loop handles the case of multi-unit CA, each unit providing + # a different ca cert, but that is not currently supported by the lib itself. + cert_path = _RECV_CA_TEMPLATE.substitute(rel_id=relation.id) + if cert := relation.data[unit].get("ca"): + self.container.push(cert_path, cert, make_dirs=True) + self._update_system_certs() def _on_recv_ca_cert_removed(self, event: CertificateTransferRemovedEvent): # Assuming only one cert per relation (this is in line with the original lib design). - target = f"{_CA_CERTS_PATH}/receive-ca-cert-{event.relation_id}-ca.crt" + target = _RECV_CA_TEMPLATE.substitute(rel_id=event.relation_id) self.container.remove_path(target, recursive=True) self._update_system_certs() @@ -514,6 +537,7 @@ def _on_traefik_pebble_ready(self, _: PebbleReadyEvent): self._clear_all_configs_and_restart_traefik() # push the (fresh new) configs. self._process_status_and_configurations() + self._update_received_ca_certs() self._set_workload_version() def _clear_all_configs_and_restart_traefik(self): @@ -993,14 +1017,14 @@ def _generate_config_block( transports = {} elif is_end_to_end: + # We cannot assume traefik's CA is the same CA that signed the proxied apps. + # Since we use the update_ca_certificates machinery, we don't need to specify the + # "rootCAs" entry. + # Keeping the serverTransports section anyway because it is informative ("endToEndTLS" + # vs "reverseTerminationTransport") when inspecting the config file in production. transport_name = "endToEndTLS" lb_def["serversTransport"] = transport_name - transports = { - transport_name: { - # Note: assuming traefik's CA is the same CA that signed the proxied apps. - "rootCAs": [_CA_CERT_PATH], - } - } + transports = {transport_name: {"insecureSkipVerify": False}} else: transports = {} diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 6825edcb..b37ef3a4 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -152,6 +152,7 @@ def setUp(self): self.harness: Harness[TraefikIngressCharm] = Harness(TraefikIngressCharm) self.harness.set_model_name("test-model") self.addCleanup(self.harness.cleanup) + self.harness.handle_exec("traefik", ["update-ca-certificates", "--fresh"], result=0) patcher = patch.object(TraefikIngressCharm, "version", property(lambda *_: "0.0.0")) self.mock_version = patcher.start() @@ -416,6 +417,7 @@ def setUp(self): self.harness: Harness[TraefikIngressCharm] = Harness(TraefikIngressCharm) self.harness.set_model_name("test-model") self.addCleanup(self.harness.cleanup) + self.harness.handle_exec("traefik", ["update-ca-certificates", "--fresh"], result=0) patcher = patch.object(TraefikIngressCharm, "version", property(lambda *_: "0.0.0")) self.mock_version = patcher.start() diff --git a/tests/unit/test_route.py b/tests/unit/test_route.py index 54583cc1..7ec77bc6 100644 --- a/tests/unit/test_route.py +++ b/tests/unit/test_route.py @@ -65,6 +65,7 @@ def harness() -> Harness[TraefikIngressCharm]: harness = Harness(TraefikIngressCharm) harness.set_model_name(MODEL_NAME) + harness.handle_exec("traefik", ["update-ca-certificates", "--fresh"], result=0) patcher = patch.object(TraefikIngressCharm, "version", property(lambda *_: "0.0.0")) patcher.start() diff --git a/tests/unit/test_tls_certificates.py b/tests/unit/test_tls_certificates.py index e25758c9..49bf6f3d 100644 --- a/tests/unit/test_tls_certificates.py +++ b/tests/unit/test_tls_certificates.py @@ -18,6 +18,7 @@ def setUp(self): self.harness: Harness[TraefikIngressCharm] = Harness(TraefikIngressCharm) self.harness.set_model_name("test-model") self.addCleanup(self.harness.cleanup) + self.harness.handle_exec("traefik", ["update-ca-certificates", "--fresh"], result=0) patcher = patch.object(TraefikIngressCharm, "version", property(lambda *_: "0.0.0")) self.mock_version = patcher.start()