From 8a97e1a6a1f38e552d9ab4593c15b27a5e275f20 Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Wed, 1 May 2024 15:47:05 +0200
Subject: [PATCH 01/19] vault

---
 .../observability_libs/v1/cert_handler.py     | 141 ++++++++++--------
 1 file changed, 81 insertions(+), 60 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index b1fd140..75c6c3f 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -34,7 +34,7 @@
 import ipaddress
 import socket
 from itertools import filterfalse
-from typing import List, Optional, Union
+from typing import List, Optional, Union, Dict
 
 try:
     from charms.tls_certificates_interface.v3.tls_certificates import (  # type: ignore
@@ -58,11 +58,10 @@
 from ops.charm import CharmBase, RelationBrokenEvent
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.jujuversion import JujuVersion
-from ops.model import SecretNotFoundError
+from ops.model import SecretNotFoundError, Secret
 
 logger = logging.getLogger(__name__)
 
-
 LIBID = "b5cd5cd580f3428fa5f59a8876dcbe6a"
 LIBAPI = 1
 LIBPATCH = 6
@@ -87,23 +86,73 @@ class CertHandlerEvents(ObjectEvents):
     cert_changed = EventSource(CertChanged)
 
 
+class Vault:
+    """Simple application secret wrapper for local usage."""
+    _uninitialized_key = "__uninitialized_secret_key__"
+
+    def __init__(self, charm: CharmBase, label: str, __id: str = None):
+        self.charm = charm
+        self.label = label  # needs to be charm-unique.
+        self._id = __id
+
+    @property
+    def _secret(self) -> Secret:
+        if self._id:
+            # we are observers. This vault has been created with import_
+            # if this secret is not found, there's something wrong.
+            try:
+                return self.charm.model.get_secret(id=self._id, label=self.label)
+            except SecretNotFoundError:
+                logger.exception(f"Unable to load vault from secret id {self._id}: "
+                                 f"has the remote created it?")
+                raise
+
+        # we are owners.
+        try:
+            return self.charm.model.get_secret(label=self.label)
+        except SecretNotFoundError:
+            # we need to set SOME contents when we're creating the secret, so we do it.
+            return self.charm.app.add_secret({self._uninitialized_key: "42"}, label=self.label)
+
+    def store(self, contents: Dict[str, str], clear: bool=False):
+        """Create a new revision by updating the previous one with ``contents``."""
+        secret = self._secret
+        current = secret.get_content(refresh=True)
+
+        if clear:
+            current.clear()
+        elif current.get(self._uninitialized_key):
+            # is this the first revision? clean up the dummy contents we created instants ago.
+            del current[self._uninitialized_key]
+
+        current.update(contents)
+        secret.set_content(current)
+
+    def get_value(self, key):
+        """Like retrieve, but single-value."""
+        return self._secret.get_content(refresh=True).get(key)
+
+    def retrieve(self):
+        """Return the full vault content."""
+        return self._secret.get_content(refresh=True)
+
+    def nuke(self):
+        self._secret.remove_all_revisions()
+
+
 class CertHandler(Object):
     """A wrapper for the requirer side of the TLS Certificates charm library."""
 
     on = CertHandlerEvents()  # pyright: ignore
 
-    _ca_cert_chain_secret_label = "ca-certificate-chain"
-    _csr_secret_id = "csr-secret-id"
-    _privkey_secret_id = "private-key-secret-id"
-
     def __init__(
-        self,
-        charm: CharmBase,
-        *,
-        key: str,
-        certificates_relation_name: str = "certificates",
-        cert_subject: Optional[str] = None,
-        sans: Optional[List[str]] = None,
+            self,
+            charm: CharmBase,
+            *,
+            key: str,
+            certificates_relation_name: str = "certificates",
+            cert_subject: Optional[str] = None,
+            sans: Optional[List[str]] = None,
     ):
         """CertHandler is used to wrap TLS Certificates management operations for charms.
 
@@ -130,6 +179,8 @@ def __init__(
         self.sans_ip = list(filter(is_ip_address, sans))
         self.sans_dns = list(filterfalse(is_ip_address, sans))
 
+        self.vault = Vault(charm, label="cert-handler-private-vault")
+
         self.certificates_relation_name = certificates_relation_name
         self.certificates = TLSCertificatesRequiresV3(self.charm, self.certificates_relation_name)
 
@@ -172,50 +223,40 @@ def enabled(self) -> bool:
             return False
 
         if not self.charm.model.get_relation(
-            self.certificates_relation_name
+                self.certificates_relation_name
         ).units:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-            self.certificates_relation_name
+                self.certificates_relation_name
         ).app:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-            self.certificates_relation_name
+                self.certificates_relation_name
         ).data:  # pyright: ignore
             return False
 
         return True
 
     def _on_certificates_relation_joined(self, _) -> None:
-        self._generate_privkey()
         self._generate_csr()
 
-    def _generate_privkey(self):
-        # Generate priv key unless done already
-        # TODO figure out how to go about key rotation.
-
-        if not (relation := self.charm.model.get_relation(self.certificates_relation_name)):
-            return
-
-        if not self.private_key:
-            private_key = generate_private_key()
-            secret = self.charm.unit.add_secret({"private-key": private_key.decode()})
-            secret.grant(relation)
-            relation.data[self.charm.unit][self._privkey_secret_id] = secret.id  # pyright: ignore
-
     def _on_config_changed(self, _):
         relation = self.charm.model.get_relation(self.certificates_relation_name)
 
         if not relation:
             return
 
-        self._generate_privkey()
         self._generate_csr(renew=True)
 
+    @property
+    def relation(self):
+        """The certificates relation."""
+        return self.charm.model.get_relation(self.certificates_relation_name)
+
     def _generate_csr(
-        self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
+            self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
     ):
         """Request a CSR "creation" if renew is False, otherwise request a renewal.
 
@@ -226,7 +267,7 @@ def _generate_csr(
         This method intentionally does not emit any events, leave it for caller's responsibility.
         """
         # if we are in a relation-broken hook, we might not have a relation to publish the csr to.
-        if not self.charm.model.get_relation(self.certificates_relation_name):
+        if not self.relation:
             logger.warning(
                 f"No {self.certificates_relation_name!r} relation found. " f"Cannot generate csr."
             )
@@ -262,11 +303,6 @@ def _generate_csr(
                 )
                 self.certificates.request_certificate_creation(certificate_signing_request=csr)
 
-            # Note: CSR is being replaced with a new one, so until we get the new cert, we'd have
-            # a mismatch between the CSR and the cert.
-            # For some reason the csr contains a trailing '\n'. TODO figure out why
-            self._csr = csr.decode().strip()
-
         if clear_cert:
             try:
                 secret = self.model.get_secret(label=self._ca_cert_chain_secret_label)
@@ -330,31 +366,16 @@ def _retrieve_from_secret(self, value: str, secret_id_name: str) -> Optional[str
     @property
     def private_key(self) -> Optional[str]:
         """Private key."""
-        return self._retrieve_from_secret("private-key", self._privkey_secret_id)
-
-    @property
-    def private_key_secret_id(self) -> Optional[str]:
-        """ID of the Juju Secret for the Private key."""
-        return self._retrieve_secret_id(self._privkey_secret_id)
+        private_key = self.vault.get_value("private-key")
+        if private_key is None:
+            private_key = generate_private_key()
+            self.vault.store({"private-key": private_key.decode()})
+        return private_key
 
     @property
     def _csr(self) -> Optional[str]:
         return self._retrieve_from_secret("csr", self._csr_secret_id)
 
-    @_csr.setter
-    def _csr(self, value: str):
-        if not (relation := self.charm.model.get_relation(self.certificates_relation_name)):
-            return
-
-        if not (secret_id := relation.data[self.charm.unit].get(self._csr_secret_id, None)):
-            secret = self.charm.unit.add_secret({"csr": value})
-            secret.grant(relation)
-            relation.data[self.charm.unit][self._csr_secret_id] = secret.id  # pyright: ignore
-            return
-
-        secret = self.model.get_secret(id=secret_id)
-        secret.set_content({"csr": value})
-
     @property
     def ca_cert(self) -> Optional[str]:
         """CA Certificate."""
@@ -380,7 +401,7 @@ def chain(self) -> Optional[str]:
         return self._chain
 
     def _on_certificate_expiring(
-        self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
+            self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
     ) -> None:
         """Generate a new CSR and request certificate renewal."""
         if event.certificate == self.server_cert:

From 8b41b08a25a8d3757469a7c0cb60d3e2c3546385 Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Wed, 1 May 2024 17:29:04 +0200
Subject: [PATCH 02/19] vault implementation draft

---
 .../observability_libs/v1/cert_handler.py     | 294 +++++++++++-------
 1 file changed, 190 insertions(+), 104 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 75c6c3f..98070f3 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -31,10 +31,12 @@
 
 Since this library uses [Juju Secrets](https://juju.is/docs/juju/secret) it requires Juju >= 3.0.3.
 """
+import abc
 import ipaddress
+import json
 import socket
 from itertools import filterfalse
-from typing import List, Optional, Union, Dict
+from typing import List, Optional, Union, Dict, Any
 
 try:
     from charms.tls_certificates_interface.v3.tls_certificates import (  # type: ignore
@@ -44,7 +46,7 @@
         CertificateInvalidatedEvent,
         TLSCertificatesRequiresV3,
         generate_csr,
-        generate_private_key,
+        generate_private_key, ProviderCertificate,
     )
 except ImportError as e:
     raise ImportError(
@@ -58,7 +60,7 @@
 from ops.charm import CharmBase, RelationBrokenEvent
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.jujuversion import JujuVersion
-from ops.model import SecretNotFoundError, Secret
+from ops.model import SecretNotFoundError, Secret, Relation
 
 logger = logging.getLogger(__name__)
 
@@ -86,35 +88,108 @@ class CertHandlerEvents(ObjectEvents):
     cert_changed = EventSource(CertChanged)
 
 
-class Vault:
-    """Simple application secret wrapper for local usage."""
-    _uninitialized_key = "__uninitialized_secret_key__"
+class _VaultBackend(abc.ABC):
+    def store(self, contents: Dict[str, str], clear: bool = False): ...
+
+    def get_value(self, key: str) -> str: ...
+
+    def retrieve(self) -> Dict[str, str]: ...
+
+    def nuke(self): ...
+
 
-    def __init__(self, charm: CharmBase, label: str, __id: str = None):
+class _RelationVaultBackend(_VaultBackend):
+    """Relation backend for Vault.
+
+    Use it to store data in a relation databag.
+    Assumes that a single relation exists and its data is readable.
+    If not, it will raise RuntimeErrors as soon as you try to read/write.
+    It will store the data, in plaintext (json-dumped) nested under a configurable
+    key in the **unit databag** of this relation.
+    """
+
+    def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secret-contents"):
+        self.charm = charm
+        self.relation_name = relation_name
+        self.nest_under = nest_under  # needs to be charm-unique.
+
+    def _check_ready(self):
+        try:
+            self.charm.model.get_relation(self.relation_name).data[self.charm.unit]
+        except Exception as e:
+            # if something goes wrong here, the peer-backed vault is not ready to operate
+            # it can be because you are trying to use it too soon, i.e. before the peer
+            # relation has been created (or has joined).
+            raise RuntimeError(" backend not ready.") from e
+
+    @property
+    def _relation(self) -> Optional[Relation]:
+        self._check_ready()
+        return self.charm.model.get_relation(self.relation_name)
+
+    @property
+    def _databag(self):
+        self._check_ready()
+        return self._relation.data[self.charm.unit]
+
+    def _read(self) -> Dict[str, str]:
+        value = self._databag.get(self.nest_under)
+        if value:
+            return json.loads(value)
+        return {}
+
+    def _write(self, value: Dict[str, str]):
+        if not all(isinstance(x, str) for x in value.values()):
+            # the caller has to take care of encoding
+            raise TypeError(f"You can only store strings in Vault.")
+
+        self._databag[self.nest_under] = json.dumps(value)
+
+    def store(self, contents: Dict[str, str], clear: bool = False):
+        """Create a new revision by updating the previous one with ``contents``."""
+        current = self._read()
+
+        if clear:
+            current.clear()
+
+        current.update(contents)
+        self._write(current)
+
+    def get_value(self, key: str):
+        """Like retrieve, but single-value."""
+        return self._read().get(key)
+
+    def retrieve(self):
+        """Return the full vault content."""
+        return self._read()
+
+    def nuke(self):
+        del self._databag[self.nest_under]
+
+
+class _SecretVaultBackend(_VaultBackend):
+    """Relation backend for Vault.
+
+    Use it to store data in a Juju secret.
+    Assumes that Juju supports secrets.
+    If not, it will raise some exception as soon as you try to read/write.
+    """
+    _uninitialized_key = "uninitialized-secret-key"
+
+    def __init__(self, charm: CharmBase, label: str):
         self.charm = charm
         self.label = label  # needs to be charm-unique.
-        self._id = __id
 
     @property
     def _secret(self) -> Secret:
-        if self._id:
-            # we are observers. This vault has been created with import_
-            # if this secret is not found, there's something wrong.
-            try:
-                return self.charm.model.get_secret(id=self._id, label=self.label)
-            except SecretNotFoundError:
-                logger.exception(f"Unable to load vault from secret id {self._id}: "
-                                 f"has the remote created it?")
-                raise
-
-        # we are owners.
         try:
+            # we are owners, so we don't need to grant it to ourselves
             return self.charm.model.get_secret(label=self.label)
         except SecretNotFoundError:
             # we need to set SOME contents when we're creating the secret, so we do it.
             return self.charm.app.add_secret({self._uninitialized_key: "42"}, label=self.label)
 
-    def store(self, contents: Dict[str, str], clear: bool=False):
+    def store(self, contents: Dict[str, str], clear: bool = False):
         """Create a new revision by updating the previous one with ``contents``."""
         secret = self._secret
         current = secret.get_content(refresh=True)
@@ -140,6 +215,29 @@ def nuke(self):
         self._secret.remove_all_revisions()
 
 
+class Vault:
+    """Simple application secret wrapper for local usage."""
+
+    def __init__(self, backend: _VaultBackend):
+        self._backend = backend
+
+    def store(self, contents: Dict[str, str], clear: bool = False):
+        """Store these contents in the vault overriding whatever is there."""
+        self._backend.store(contents, clear=clear)
+
+    def get_value(self, key: str):
+        """Like retrieve, but single-value."""
+        return self._backend.get_value(key)
+
+    def retrieve(self) -> Dict[str, str]:
+        """Return the full vault content."""
+        return self._backend.retrieve()
+
+    def nuke(self):
+        """Clear the vault."""
+        self._backend.nuke()
+
+
 class CertHandler(Object):
     """A wrapper for the requirer side of the TLS Certificates charm library."""
 
@@ -167,9 +265,8 @@ def __init__(
             sans: DNS names. If none are given, use FQDN.
         """
         super().__init__(charm, key)
-        self._check_juju_supports_secrets()
-
         self.charm = charm
+
         # We need to sanitize the unit name, otherwise route53 complains:
         # "urn:ietf:params:acme:error:malformed" :: Domain name contains an invalid character
         self.cert_subject = charm.unit.name.replace("/", "-") if not cert_subject else cert_subject
@@ -179,7 +276,16 @@ def __init__(
         self.sans_ip = list(filter(is_ip_address, sans))
         self.sans_dns = list(filterfalse(is_ip_address, sans))
 
-        self.vault = Vault(charm, label="cert-handler-private-vault")
+        if self._check_juju_supports_secrets():
+            vault_backend = _SecretVaultBackend(charm, label="cert-handler-private-vault")
+
+            # TODO: gracefully handle situations where the
+            #  secret is gone because the admin has removed it manually
+            # self.framework.observe(self.charm.on.secret_remove, self._rotate_csr)
+
+        else:
+            vault_backend = _RelationVaultBackend(charm, relation_name="peers")
+        self.vault = Vault(vault_backend)
 
         self.certificates_relation_name = certificates_relation_name
         self.certificates = TLSCertificatesRequiresV3(self.charm, self.certificates_relation_name)
@@ -212,6 +318,34 @@ def __init__(
             self.charm.on[self.certificates_relation_name].relation_broken,  # pyright: ignore
             self._on_certificates_relation_broken,
         )
+        self.framework.observe(
+            self.charm.on.upgrade_charm,  # pyright: ignore
+            self._on_upgrade_charm,
+        )
+
+    def _on_upgrade_charm(self, _):
+        self._migrate_vault()
+
+    def _migrate_vault(self):
+        peer_backend = _RelationVaultBackend(self.charm, relation_name="peers")
+
+        if self._check_juju_supports_secrets():
+            # we are on recent juju
+            if self.vault.retrieve():
+                # we already were on recent juju: nothing to migrate
+                return
+
+            # we used to be on old juju: our secret stuff is in peer data
+            if peer_backend.retrieve():
+                # move over to secret-backed storage
+                self.vault.store(peer_backend.retrieve())
+
+                # clear the peer storage
+                peer_backend.nuke()
+                return
+
+        # if we are downgrading, i.e. from juju with secrets to juju without,
+        # we have lost all that was in the secrets backend.
 
     @property
     def enabled(self) -> bool:
@@ -240,7 +374,8 @@ def enabled(self) -> bool:
         return True
 
     def _on_certificates_relation_joined(self, _) -> None:
-        self._generate_csr()
+        if not self._csr:
+            self._generate_csr()
 
     def _on_config_changed(self, _):
         relation = self.charm.model.get_relation(self.certificates_relation_name)
@@ -252,7 +387,7 @@ def _on_config_changed(self, _):
 
     @property
     def relation(self):
-        """The certificates relation."""
+        """The "certificates" relation."""
         return self.charm.model.get_relation(self.certificates_relation_name)
 
     def _generate_csr(
@@ -304,101 +439,55 @@ def _generate_csr(
                 self.certificates.request_certificate_creation(certificate_signing_request=csr)
 
         if clear_cert:
-            try:
-                secret = self.model.get_secret(label=self._ca_cert_chain_secret_label)
-                secret.remove_all_revisions()
-            except SecretNotFoundError:
-                logger.debug("Secret with label: 'ca-certificate-chain' not found")
+            self.vault.nuke()
 
     def _on_certificate_available(self, event: CertificateAvailableEvent) -> None:
-        """Get the certificate from the event and store it in a peer relation.
-
-        Note: assuming "limit: 1" in metadata
-        """
-        event_csr = (
-            event.certificate_signing_request.strip()
-            if event.certificate_signing_request
-            else None
-        )
-        if event_csr == self._csr:
-            content = {
-                "ca-cert": event.ca,
-                "server-cert": event.certificate,
-                "chain": event.chain_as_pem(),
-                "csr": event_csr,
-            }
-            if not (relation := self.charm.model.get_relation(self.certificates_relation_name)):
-                logger.error("Relation %s not found", self.certificates_relation_name)
-                return
-
-            # if we have a secret from a previous certificates relation already, keep it and reuse it.
-            try:
-                secret = self.model.get_secret(label=self._ca_cert_chain_secret_label)
-                secret.set_content(content)
-            except SecretNotFoundError:
-                secret = self.charm.unit.add_secret(
-                    content, label=self._ca_cert_chain_secret_label
-                )
-
-            secret.grant(relation)
-            relation.data[self.charm.unit]["secret-id"] = secret.id  # pyright: ignore
-            self.on.cert_changed.emit()  # pyright: ignore
-
-    def _retrieve_secret_id(self, secret_id_name: str) -> Optional[str]:
-        if not (relation := self.charm.model.get_relation(self.certificates_relation_name)):
-            return None
-
-        if not (secret_id := relation.data[self.charm.unit].get(secret_id_name)):
-            return None
-
-        return secret_id
-
-    def _retrieve_from_secret(self, value: str, secret_id_name: str) -> Optional[str]:
-        if not (secret_id := self._retrieve_secret_id(secret_id_name)):
-            return None
-
-        if not (secret := self.model.get_secret(id=secret_id)):
-            return None
-
-        content = secret.get_content()
-        return content.get(value)
+        """Emit cert-changed."""
+        self.on.cert_changed.emit()  # pyright: ignore
 
     @property
     def private_key(self) -> Optional[str]:
-        """Private key."""
+        """Private key.
+
+        BEWARE: if the vault misbehaves, the backing secret is removed, the peer relation dies
+        or whatever, we might be calling generate_private_key() again and cause a desync
+        with the CSR because it's going to be signed with an outdated key we have no way of retrieving.
+        The caller needs to ensure that if the vault backend gets reset, then so does the csr.
+
+        TODO: we could consider adding a way to verify if the csr was signed by our privkey,
+            and do that on collect_unit_status as a sanity check
+        """
         private_key = self.vault.get_value("private-key")
         if private_key is None:
-            private_key = generate_private_key()
-            self.vault.store({"private-key": private_key.decode()})
+            private_key = generate_private_key().decode()
+            self.vault.store({"private-key": private_key})
         return private_key
 
     @property
     def _csr(self) -> Optional[str]:
-        return self._retrieve_from_secret("csr", self._csr_secret_id)
+        csrs = self.certificates.get_requirer_csrs()
+        if not csrs:
+            return None
+        return csrs[-1].csr
+
+    def get_cert(self) -> ProviderCertificate:
+        all_certs = self.certificates.get_provider_certificates()
+        return [c for c in all_certs if c.csr == self._csr][0]
 
     @property
     def ca_cert(self) -> Optional[str]:
         """CA Certificate."""
-        return self._retrieve_from_secret("ca-cert", "secret-id")
-
-    @property
-    def ca_server_cert_secret_id(self) -> Optional[str]:
-        """CA server cert secret id."""
-        return self._retrieve_secret_id("secret-id")
+        return self.get_cert().ca
 
     @property
     def server_cert(self) -> Optional[str]:
         """Server Certificate."""
-        return self._retrieve_from_secret("server-cert", "secret-id")
-
-    @property
-    def _chain(self) -> Optional[str]:
-        return self._retrieve_from_secret("chain", "secret-id")
+        return self.get_cert().certificate
 
     @property
     def chain(self) -> Optional[str]:
         """Return the ca chain."""
-        return self._chain
+        return self.get_cert().chain_as_pem()
 
     def _on_certificate_expiring(
             self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
@@ -432,17 +521,14 @@ def _on_all_certificates_invalidated(self, _: AllCertificatesInvalidatedEvent) -
 
     def _on_certificates_relation_broken(self, _: RelationBrokenEvent) -> None:
         """Clear all secrets data when removing the relation."""
-        try:
-            secret = self.model.get_secret(label=self._ca_cert_chain_secret_label)
-            secret.remove_all_revisions()
-        except SecretNotFoundError:
-            logger.debug(f"Secret {self._ca_cert_chain_secret_label!r}' not found")
+        self.vault.nuke()
         self.on.cert_changed.emit()  # pyright: ignore
 
-    def _check_juju_supports_secrets(self) -> None:
+    def _check_juju_supports_secrets(self) -> bool:
         version = JujuVersion.from_environ()
 
         if not JujuVersion(version=str(version)).has_secrets:
             msg = f"Juju version {version} does not supports Secrets. Juju >= 3.0.3 is needed"
             logger.error(msg)
-            raise RuntimeError(msg)
+            return False
+        return True

From 37705d04506b4eacc8957fda3046a7a27aff4c22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:37:26 -0300
Subject: [PATCH 03/19] linting

---
 lib/charms/observability_libs/v1/cert_handler.py | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 98070f3..b7f5843 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -36,7 +36,7 @@
 import json
 import socket
 from itertools import filterfalse
-from typing import List, Optional, Union, Dict, Any
+from typing import Dict, List, Optional, Union
 
 try:
     from charms.tls_certificates_interface.v3.tls_certificates import (  # type: ignore
@@ -44,9 +44,10 @@
         CertificateAvailableEvent,
         CertificateExpiringEvent,
         CertificateInvalidatedEvent,
+        ProviderCertificate,
         TLSCertificatesRequiresV3,
         generate_csr,
-        generate_private_key, ProviderCertificate,
+        generate_private_key,
     )
 except ImportError as e:
     raise ImportError(
@@ -60,7 +61,7 @@
 from ops.charm import CharmBase, RelationBrokenEvent
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.jujuversion import JujuVersion
-from ops.model import SecretNotFoundError, Secret, Relation
+from ops.model import Relation, Secret, SecretNotFoundError
 
 logger = logging.getLogger(__name__)
 
@@ -141,7 +142,7 @@ def _read(self) -> Dict[str, str]:
     def _write(self, value: Dict[str, str]):
         if not all(isinstance(x, str) for x in value.values()):
             # the caller has to take care of encoding
-            raise TypeError(f"You can only store strings in Vault.")
+            raise TypeError("You can only store strings in Vault.")
 
         self._databag[self.nest_under] = json.dumps(value)
 

From 81f8f2c72f99333fcd08d5239a97ee7acf29ddc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:39:47 -0300
Subject: [PATCH 04/19] add doctring

---
 lib/charms/observability_libs/v1/cert_handler.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index b7f5843..f1bdedc 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -472,6 +472,7 @@ def _csr(self) -> Optional[str]:
         return csrs[-1].csr
 
     def get_cert(self) -> ProviderCertificate:
+        """Get cert."""
         all_certs = self.certificates.get_provider_certificates()
         return [c for c in all_certs if c.csr == self._csr][0]
 

From b083c0cfb37711f0577080d727462c7b60e40d11 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:40:03 -0300
Subject: [PATCH 05/19] more linting here and there

---
 .../observability_libs/v1/cert_handler.py     | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index f1bdedc..cfed2d1 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -175,6 +175,7 @@ class _SecretVaultBackend(_VaultBackend):
     Assumes that Juju supports secrets.
     If not, it will raise some exception as soon as you try to read/write.
     """
+
     _uninitialized_key = "uninitialized-secret-key"
 
     def __init__(self, charm: CharmBase, label: str):
@@ -245,13 +246,13 @@ class CertHandler(Object):
     on = CertHandlerEvents()  # pyright: ignore
 
     def __init__(
-            self,
-            charm: CharmBase,
-            *,
-            key: str,
-            certificates_relation_name: str = "certificates",
-            cert_subject: Optional[str] = None,
-            sans: Optional[List[str]] = None,
+        self,
+        charm: CharmBase,
+        *,
+        key: str,
+        certificates_relation_name: str = "certificates",
+        cert_subject: Optional[str] = None,
+        sans: Optional[List[str]] = None,
     ):
         """CertHandler is used to wrap TLS Certificates management operations for charms.
 
@@ -358,17 +359,17 @@ def enabled(self) -> bool:
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).units:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).app:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).data:  # pyright: ignore
             return False
 
@@ -392,7 +393,7 @@ def relation(self):
         return self.charm.model.get_relation(self.certificates_relation_name)
 
     def _generate_csr(
-            self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
+        self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
     ):
         """Request a CSR "creation" if renew is False, otherwise request a renewal.
 
@@ -492,7 +493,7 @@ def chain(self) -> Optional[str]:
         return self.get_cert().chain_as_pem()
 
     def _on_certificate_expiring(
-            self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
+        self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
     ) -> None:
         """Generate a new CSR and request certificate renewal."""
         if event.certificate == self.server_cert:

From b363e75d43d7be8eb0baf13cf24e2e7f74f7a010 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:50:06 -0300
Subject: [PATCH 06/19] fix static-lib check

---
 lib/charms/observability_libs/v1/cert_handler.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index cfed2d1..90deb77 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -92,7 +92,7 @@ class CertHandlerEvents(ObjectEvents):
 class _VaultBackend(abc.ABC):
     def store(self, contents: Dict[str, str], clear: bool = False): ...
 
-    def get_value(self, key: str) -> str: ...
+    def get_value(self, key: str) -> Optional[str]: ...
 
     def retrieve(self) -> Dict[str, str]: ...
 
@@ -116,7 +116,7 @@ def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secr
 
     def _check_ready(self):
         try:
-            self.charm.model.get_relation(self.relation_name).data[self.charm.unit]
+            self.charm.model.get_relation(self.relation_name).data[self.charm.unit]   # pyright: ignore
         except Exception as e:
             # if something goes wrong here, the peer-backed vault is not ready to operate
             # it can be because you are trying to use it too soon, i.e. before the peer
@@ -131,7 +131,7 @@ def _relation(self) -> Optional[Relation]:
     @property
     def _databag(self):
         self._check_ready()
-        return self._relation.data[self.charm.unit]
+        return self._relation.data[self.charm.unit]  # pyright: ignore
 
     def _read(self) -> Dict[str, str]:
         value = self._databag.get(self.nest_under)

From c71a652f0d32d90b789d9a80c06e46afd7aaee4c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:50:21 -0300
Subject: [PATCH 07/19] increase LIBPATCH

---
 lib/charms/observability_libs/v1/cert_handler.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 90deb77..91c9671 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -67,7 +67,7 @@
 
 LIBID = "b5cd5cd580f3428fa5f59a8876dcbe6a"
 LIBAPI = 1
-LIBPATCH = 6
+LIBPATCH = 7
 
 
 def is_ip_address(value: str) -> bool:

From 2d93422d28e82cb12707bde128b42ca7b453dc8f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Wed, 1 May 2024 13:51:55 -0300
Subject: [PATCH 08/19] linting here, linting there, linting everywhere

---
 lib/charms/observability_libs/v1/cert_handler.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 91c9671..64ec070 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -116,7 +116,9 @@ def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secr
 
     def _check_ready(self):
         try:
-            self.charm.model.get_relation(self.relation_name).data[self.charm.unit]   # pyright: ignore
+            self.charm.model.get_relation(self.relation_name).data[  # pyright: ignore
+                self.charm.unit
+            ]
         except Exception as e:
             # if something goes wrong here, the peer-backed vault is not ready to operate
             # it can be because you are trying to use it too soon, i.e. before the peer

From cd22203de6bbeaab97f8cece9826bd62a7d1fcff Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 10:28:52 +0200
Subject: [PATCH 09/19] lint and static fixes

---
 .../observability_libs/v1/cert_handler.py     | 112 ++++++++++--------
 1 file changed, 60 insertions(+), 52 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 98070f3..4fd86a5 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -36,7 +36,7 @@
 import json
 import socket
 from itertools import filterfalse
-from typing import List, Optional, Union, Dict, Any
+from typing import Dict, List, Optional, Union
 
 try:
     from charms.tls_certificates_interface.v3.tls_certificates import (  # type: ignore
@@ -44,9 +44,10 @@
         CertificateAvailableEvent,
         CertificateExpiringEvent,
         CertificateInvalidatedEvent,
+        ProviderCertificate,
         TLSCertificatesRequiresV3,
         generate_csr,
-        generate_private_key, ProviderCertificate,
+        generate_private_key,
     )
 except ImportError as e:
     raise ImportError(
@@ -60,13 +61,13 @@
 from ops.charm import CharmBase, RelationBrokenEvent
 from ops.framework import EventBase, EventSource, Object, ObjectEvents
 from ops.jujuversion import JujuVersion
-from ops.model import SecretNotFoundError, Secret, Relation
+from ops.model import Relation, Secret, SecretNotFoundError
 
 logger = logging.getLogger(__name__)
 
 LIBID = "b5cd5cd580f3428fa5f59a8876dcbe6a"
 LIBAPI = 1
-LIBPATCH = 6
+LIBPATCH = 7
 
 
 def is_ip_address(value: str) -> bool:
@@ -89,13 +90,17 @@ class CertHandlerEvents(ObjectEvents):
 
 
 class _VaultBackend(abc.ABC):
-    def store(self, contents: Dict[str, str], clear: bool = False): ...
+    def store(self, contents: Dict[str, str], clear: bool = False):
+        ...
 
-    def get_value(self, key: str) -> str: ...
+    def get_value(self, key: str) -> Optional[str]:
+        ...
 
-    def retrieve(self) -> Dict[str, str]: ...
+    def retrieve(self) -> Dict[str, str]:
+        ...
 
-    def nuke(self): ...
+    def nuke(self):
+        ...
 
 
 class _RelationVaultBackend(_VaultBackend):
@@ -106,6 +111,8 @@ class _RelationVaultBackend(_VaultBackend):
     If not, it will raise RuntimeErrors as soon as you try to read/write.
     It will store the data, in plaintext (json-dumped) nested under a configurable
     key in the **unit databag** of this relation.
+
+    Typically, you'll use this with peer relations.
     """
 
     def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secret-contents"):
@@ -114,13 +121,12 @@ def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secr
         self.nest_under = nest_under  # needs to be charm-unique.
 
     def _check_ready(self):
-        try:
-            self.charm.model.get_relation(self.relation_name).data[self.charm.unit]
-        except Exception as e:
+        relation = self.charm.model.get_relation(self.relation_name)
+        if not relation or not relation.data:
             # if something goes wrong here, the peer-backed vault is not ready to operate
-            # it can be because you are trying to use it too soon, i.e. before the peer
+            # it can be because you are trying to use it too soon, i.e. before the (peer)
             # relation has been created (or has joined).
-            raise RuntimeError(" backend not ready.") from e
+            raise RuntimeError("Relation backend not ready.")
 
     @property
     def _relation(self) -> Optional[Relation]:
@@ -130,7 +136,8 @@ def _relation(self) -> Optional[Relation]:
     @property
     def _databag(self):
         self._check_ready()
-        return self._relation.data[self.charm.unit]
+        # _check_ready verifies that there is a relation
+        return self._relation.data[self.charm.unit]  # type: ignore
 
     def _read(self) -> Dict[str, str]:
         value = self._databag.get(self.nest_under)
@@ -141,7 +148,7 @@ def _read(self) -> Dict[str, str]:
     def _write(self, value: Dict[str, str]):
         if not all(isinstance(x, str) for x in value.values()):
             # the caller has to take care of encoding
-            raise TypeError(f"You can only store strings in Vault.")
+            raise TypeError("You can only store strings in Vault.")
 
         self._databag[self.nest_under] = json.dumps(value)
 
@@ -155,7 +162,7 @@ def store(self, contents: Dict[str, str], clear: bool = False):
         current.update(contents)
         self._write(current)
 
-    def get_value(self, key: str):
+    def get_value(self, key: str) -> Optional[str]:
         """Like retrieve, but single-value."""
         return self._read().get(key)
 
@@ -174,6 +181,7 @@ class _SecretVaultBackend(_VaultBackend):
     Assumes that Juju supports secrets.
     If not, it will raise some exception as soon as you try to read/write.
     """
+
     _uninitialized_key = "uninitialized-secret-key"
 
     def __init__(self, charm: CharmBase, label: str):
@@ -203,7 +211,7 @@ def store(self, contents: Dict[str, str], clear: bool = False):
         current.update(contents)
         secret.set_content(current)
 
-    def get_value(self, key):
+    def get_value(self, key: str) -> Optional[str]:
         """Like retrieve, but single-value."""
         return self._secret.get_content(refresh=True).get(key)
 
@@ -244,13 +252,13 @@ class CertHandler(Object):
     on = CertHandlerEvents()  # pyright: ignore
 
     def __init__(
-            self,
-            charm: CharmBase,
-            *,
-            key: str,
-            certificates_relation_name: str = "certificates",
-            cert_subject: Optional[str] = None,
-            sans: Optional[List[str]] = None,
+        self,
+        charm: CharmBase,
+        *,
+        key: str,
+        certificates_relation_name: str = "certificates",
+        cert_subject: Optional[str] = None,
+        sans: Optional[List[str]] = None,
     ):
         """CertHandler is used to wrap TLS Certificates management operations for charms.
 
@@ -357,33 +365,29 @@ def enabled(self) -> bool:
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).units:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).app:  # pyright: ignore
             return False
 
         if not self.charm.model.get_relation(
-                self.certificates_relation_name
+            self.certificates_relation_name
         ).data:  # pyright: ignore
             return False
 
         return True
 
     def _on_certificates_relation_joined(self, _) -> None:
-        if not self._csr:
-            self._generate_csr()
+        # this will only generate a csr if we don't have one already
+        self._generate_csr()
 
     def _on_config_changed(self, _):
-        relation = self.charm.model.get_relation(self.certificates_relation_name)
-
-        if not relation:
-            return
-
-        self._generate_csr(renew=True)
+        # this will only generate a csr if we don't have one already
+        self._generate_csr()
 
     @property
     def relation(self):
@@ -391,7 +395,7 @@ def relation(self):
         return self.charm.model.get_relation(self.certificates_relation_name)
 
     def _generate_csr(
-            self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
+        self, overwrite: bool = False, renew: bool = False, clear_cert: bool = False
     ):
         """Request a CSR "creation" if renew is False, otherwise request a renewal.
 
@@ -470,31 +474,38 @@ def _csr(self) -> Optional[str]:
             return None
         return csrs[-1].csr
 
-    def get_cert(self) -> ProviderCertificate:
+    def get_cert(self) -> Optional[ProviderCertificate]:
+        """Get the certificate from relation data."""
         all_certs = self.certificates.get_provider_certificates()
-        return [c for c in all_certs if c.csr == self._csr][0]
+        # search for the cert matching our csr.
+        matching_cert = [c for c in all_certs if c.csr == self._csr]
+        return matching_cert[0] if matching_cert else None
 
     @property
     def ca_cert(self) -> Optional[str]:
         """CA Certificate."""
-        return self.get_cert().ca
+        cert = self.get_cert()
+        return cert.ca if cert else None
 
     @property
     def server_cert(self) -> Optional[str]:
         """Server Certificate."""
-        return self.get_cert().certificate
+        cert = self.get_cert()
+        return cert.certificate if cert else None
 
     @property
     def chain(self) -> Optional[str]:
-        """Return the ca chain."""
-        return self.get_cert().chain_as_pem()
+        """Return the ca chain bundled as a single PEM string."""
+        cert = self.get_cert()
+        return cert.chain_as_pem() if cert else None
 
     def _on_certificate_expiring(
-            self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
+        self, event: Union[CertificateExpiringEvent, CertificateInvalidatedEvent]
     ) -> None:
         """Generate a new CSR and request certificate renewal."""
         if event.certificate == self.server_cert:
             self._generate_csr(renew=True)
+            # FIXME why are we not emitting cert_changed here?
 
     def _certificate_revoked(self, event) -> None:
         """Remove the certificate and generate a new CSR."""
@@ -505,13 +516,11 @@ def _certificate_revoked(self, event) -> None:
 
     def _on_certificate_invalidated(self, event: CertificateInvalidatedEvent) -> None:
         """Deal with certificate revocation and expiration."""
-        if event.certificate != self.server_cert:
-            return
-
-        # if event.reason in ("revoked", "expired"):
-        # Currently, the reason does not matter to us because the action is the same.
-        self._generate_csr(overwrite=True, clear_cert=True)
-        self.on.cert_changed.emit()  # pyright: ignore
+        if event.certificate == self.server_cert:
+            # if event.reason in ("revoked", "expired"):
+            # Currently, the reason does not matter to us because the action is the same.
+            self._generate_csr(overwrite=True, clear_cert=True)
+            self.on.cert_changed.emit()  # pyright: ignore
 
     def _on_all_certificates_invalidated(self, _: AllCertificatesInvalidatedEvent) -> None:
         # Do what you want with this information, probably remove all certificates
@@ -526,9 +535,8 @@ def _on_certificates_relation_broken(self, _: RelationBrokenEvent) -> None:
 
     def _check_juju_supports_secrets(self) -> bool:
         version = JujuVersion.from_environ()
-
         if not JujuVersion(version=str(version)).has_secrets:
             msg = f"Juju version {version} does not supports Secrets. Juju >= 3.0.3 is needed"
-            logger.error(msg)
+            logger.debug(msg)
             return False
         return True

From cdfadcf450169f96b5812c81d87b5acb5ac3961e Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 10:44:22 +0200
Subject: [PATCH 10/19] fmt

---
 lib/charms/observability_libs/v1/cert_handler.py | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 4fd86a5..7e7d2a5 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -90,17 +90,13 @@ class CertHandlerEvents(ObjectEvents):
 
 
 class _VaultBackend(abc.ABC):
-    def store(self, contents: Dict[str, str], clear: bool = False):
-        ...
+    def store(self, contents: Dict[str, str], clear: bool = False): ...
 
-    def get_value(self, key: str) -> Optional[str]:
-        ...
+    def get_value(self, key: str) -> Optional[str]: ...
 
-    def retrieve(self) -> Dict[str, str]:
-        ...
+    def retrieve(self) -> Dict[str, str]: ...
 
-    def nuke(self):
-        ...
+    def nuke(self): ...
 
 
 class _RelationVaultBackend(_VaultBackend):

From 7d3f27f21fe68aa54fc7dd83789b78de8facd644 Mon Sep 17 00:00:00 2001
From: PietroPasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 13:43:13 +0200
Subject: [PATCH 11/19] Apply suggestions from code review

Co-authored-by: Leon <82407168+sed-i@users.noreply.github.com>
---
 lib/charms/observability_libs/v1/cert_handler.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 7e7d2a5..90471fb 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -90,6 +90,12 @@ class CertHandlerEvents(ObjectEvents):
 
 
 class _VaultBackend(abc.ABC):
+    """Base class for a single secret manager.
+    
+    Assumptions:
+    - A single secret (label) is managed by a single instance.
+    - Secret is per-unit (not per-app, i.e. may differ from unit to unit).
+    """
     def store(self, contents: Dict[str, str], clear: bool = False): ...
 
     def get_value(self, key: str) -> Optional[str]: ...
@@ -109,6 +115,9 @@ class _RelationVaultBackend(_VaultBackend):
     key in the **unit databag** of this relation.
 
     Typically, you'll use this with peer relations.
+    
+    Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
+      Modifying relation data yourself would go unnoticed and disrupt consistency.
     """
 
     def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secret-contents"):
@@ -176,6 +185,9 @@ class _SecretVaultBackend(_VaultBackend):
     Use it to store data in a Juju secret.
     Assumes that Juju supports secrets.
     If not, it will raise some exception as soon as you try to read/write.
+    
+    Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
+      Modifying secret's data yourself would go unnoticed and disrupt consistency.
     """
 
     _uninitialized_key = "uninitialized-secret-key"

From c54d9ec204e82cf8e4716a585c91dc4b78a9a253 Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 13:43:23 +0200
Subject: [PATCH 12/19] local changes

---
 .../observability_libs/v1/cert_handler.py     | 29 ++++++++++++-------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 7e7d2a5..1b44fbc 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -96,7 +96,7 @@ def get_value(self, key: str) -> Optional[str]: ...
 
     def retrieve(self) -> Dict[str, str]: ...
 
-    def nuke(self): ...
+    def clear(self): ...
 
 
 class _RelationVaultBackend(_VaultBackend):
@@ -166,7 +166,7 @@ def retrieve(self):
         """Return the full vault content."""
         return self._read()
 
-    def nuke(self):
+    def clear(self):
         del self._databag[self.nest_under]
 
 
@@ -201,7 +201,7 @@ def store(self, contents: Dict[str, str], clear: bool = False):
         if clear:
             current.clear()
         elif current.get(self._uninitialized_key):
-            # is this the first revision? clean up the dummy contents we created instants ago.
+            # is this the first revision? clean up the mock contents we created instants ago.
             del current[self._uninitialized_key]
 
         current.update(contents)
@@ -215,7 +215,7 @@ def retrieve(self):
         """Return the full vault content."""
         return self._secret.get_content(refresh=True)
 
-    def nuke(self):
+    def clear(self):
         self._secret.remove_all_revisions()
 
 
@@ -237,9 +237,9 @@ def retrieve(self) -> Dict[str, str]:
         """Return the full vault content."""
         return self._backend.retrieve()
 
-    def nuke(self):
+    def clear(self):
         """Clear the vault."""
-        self._backend.nuke()
+        self._backend.clear()
 
 
 class CertHandler(Object):
@@ -345,7 +345,7 @@ def _migrate_vault(self):
                 self.vault.store(peer_backend.retrieve())
 
                 # clear the peer storage
-                peer_backend.nuke()
+                peer_backend.clear()
                 return
 
         # if we are downgrading, i.e. from juju with secrets to juju without,
@@ -439,7 +439,7 @@ def _generate_csr(
                 self.certificates.request_certificate_creation(certificate_signing_request=csr)
 
         if clear_cert:
-            self.vault.nuke()
+            self.vault.clear()
 
     def _on_certificate_available(self, event: CertificateAvailableEvent) -> None:
         """Emit cert-changed."""
@@ -455,7 +455,7 @@ def private_key(self) -> Optional[str]:
         The caller needs to ensure that if the vault backend gets reset, then so does the csr.
 
         TODO: we could consider adding a way to verify if the csr was signed by our privkey,
-            and do that on collect_unit_status as a sanity check
+            and do that on collect_unit_status as a consistency check
         """
         private_key = self.vault.get_value("private-key")
         if private_key is None:
@@ -468,6 +468,15 @@ def _csr(self) -> Optional[str]:
         csrs = self.certificates.get_requirer_csrs()
         if not csrs:
             return None
+
+        # in principle we only ever need one cert.
+        # we might want to complicate this a bit once we get into cert rotations: during the rotation, we may need to
+        # keep the old one around for a little while. If there's multiple certs, at the moment we're
+        # ignoring all but the last one.
+        if len(csrs) > 1:
+            logger.warning("Multiple CSRs found in `certificates` relation. "
+                           "cert_handler is not ready to expect it.")
+
         return csrs[-1].csr
 
     def get_cert(self) -> Optional[ProviderCertificate]:
@@ -526,7 +535,7 @@ def _on_all_certificates_invalidated(self, _: AllCertificatesInvalidatedEvent) -
 
     def _on_certificates_relation_broken(self, _: RelationBrokenEvent) -> None:
         """Clear all secrets data when removing the relation."""
-        self.vault.nuke()
+        self.vault.clear()
         self.on.cert_changed.emit()  # pyright: ignore
 
     def _check_juju_supports_secrets(self) -> bool:

From 5a36ca7b225a569839266709efc1d243ca1c13fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Thu, 2 May 2024 08:56:07 -0300
Subject: [PATCH 13/19] remove raise RuntimeError since the generation is in
 `private_key` property

---
 lib/charms/observability_libs/v1/cert_handler.py | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 90471fb..5119276 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -91,11 +91,12 @@ class CertHandlerEvents(ObjectEvents):
 
 class _VaultBackend(abc.ABC):
     """Base class for a single secret manager.
-    
+
     Assumptions:
     - A single secret (label) is managed by a single instance.
     - Secret is per-unit (not per-app, i.e. may differ from unit to unit).
     """
+
     def store(self, contents: Dict[str, str], clear: bool = False): ...
 
     def get_value(self, key: str) -> Optional[str]: ...
@@ -115,7 +116,7 @@ class _RelationVaultBackend(_VaultBackend):
     key in the **unit databag** of this relation.
 
     Typically, you'll use this with peer relations.
-    
+
     Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
       Modifying relation data yourself would go unnoticed and disrupt consistency.
     """
@@ -185,7 +186,7 @@ class _SecretVaultBackend(_VaultBackend):
     Use it to store data in a Juju secret.
     Assumes that Juju supports secrets.
     If not, it will raise some exception as soon as you try to read/write.
-    
+
     Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
       Modifying secret's data yourself would go unnoticed and disrupt consistency.
     """
@@ -423,12 +424,6 @@ def _generate_csr(
         # In case we already have a csr, do not overwrite it by default.
         if overwrite or renew or not self._csr:
             private_key = self.private_key
-            if private_key is None:
-                # FIXME: raise this in a less nested scope by
-                #  generating privkey and csr in the same method.
-                raise RuntimeError(
-                    "private key unset. call _generate_privkey() before you call this method."
-                )
             csr = generate_csr(
                 private_key=private_key.encode(),
                 subject=self.cert_subject,
@@ -458,7 +453,7 @@ def _on_certificate_available(self, event: CertificateAvailableEvent) -> None:
         self.on.cert_changed.emit()  # pyright: ignore
 
     @property
-    def private_key(self) -> Optional[str]:
+    def private_key(self) -> str:
         """Private key.
 
         BEWARE: if the vault misbehaves, the backing secret is removed, the peer relation dies

From f6f8f61edd3c6b42239e946669674fd3fb360405 Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 13:58:24 +0200
Subject: [PATCH 14/19] pr comments

---
 .../observability_libs/v1/cert_handler.py     | 41 ++++++++++++-------
 .../test_cert_handler/test_cert_handler_v1.py |  7 +++-
 tox.ini                                       |  2 +-
 3 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index 1b42f6b..9891e30 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -69,6 +69,8 @@
 LIBAPI = 1
 LIBPATCH = 7
 
+VAULT_SECRET_LABEL = "cert-handler-private-vault"
+
 
 def is_ip_address(value: str) -> bool:
     """Return True if the input value is a valid IPv4 address; False otherwise."""
@@ -91,11 +93,12 @@ class CertHandlerEvents(ObjectEvents):
 
 class _VaultBackend(abc.ABC):
     """Base class for a single secret manager.
-    
+
     Assumptions:
     - A single secret (label) is managed by a single instance.
     - Secret is per-unit (not per-app, i.e. may differ from unit to unit).
     """
+
     def store(self, contents: Dict[str, str], clear: bool = False): ...
 
     def get_value(self, key: str) -> Optional[str]: ...
@@ -115,15 +118,19 @@ class _RelationVaultBackend(_VaultBackend):
     key in the **unit databag** of this relation.
 
     Typically, you'll use this with peer relations.
-    
+
     Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
       Modifying relation data yourself would go unnoticed and disrupt consistency.
     """
 
-    def __init__(self, charm: CharmBase, relation_name: str, nest_under: str = "secret-contents"):
+    _NEST_UNDER = "lib.charms.observability_libs.v1.cert_handler::vault"
+    # This key needs to be relation-unique. If someone ever creates multiple Vault(_RelationVaultBackend)
+    # instances backed by the same (peer) relation, they'll need to set different _NEST_UNDERs
+    # for each _RelationVaultBackend instance or they'll be fighting over it.
+
+    def __init__(self, charm: CharmBase, relation_name: str):
         self.charm = charm
         self.relation_name = relation_name
-        self.nest_under = nest_under  # needs to be charm-unique.
 
     def _check_ready(self):
         relation = self.charm.model.get_relation(self.relation_name)
@@ -145,7 +152,7 @@ def _databag(self):
         return self._relation.data[self.charm.unit]  # type: ignore
 
     def _read(self) -> Dict[str, str]:
-        value = self._databag.get(self.nest_under)
+        value = self._databag.get(self._NEST_UNDER)
         if value:
             return json.loads(value)
         return {}
@@ -155,7 +162,7 @@ def _write(self, value: Dict[str, str]):
             # the caller has to take care of encoding
             raise TypeError("You can only store strings in Vault.")
 
-        self._databag[self.nest_under] = json.dumps(value)
+        self._databag[self._NEST_UNDER] = json.dumps(value)
 
     def store(self, contents: Dict[str, str], clear: bool = False):
         """Create a new revision by updating the previous one with ``contents``."""
@@ -176,7 +183,7 @@ def retrieve(self):
         return self._read()
 
     def clear(self):
-        del self._databag[self.nest_under]
+        del self._databag[self._NEST_UNDER]
 
 
 class _SecretVaultBackend(_VaultBackend):
@@ -185,25 +192,27 @@ class _SecretVaultBackend(_VaultBackend):
     Use it to store data in a Juju secret.
     Assumes that Juju supports secrets.
     If not, it will raise some exception as soon as you try to read/write.
-    
+
     Note: it is assumed that this object has exclusive access to the data, even though in practice it does not.
       Modifying secret's data yourself would go unnoticed and disrupt consistency.
     """
 
     _uninitialized_key = "uninitialized-secret-key"
 
-    def __init__(self, charm: CharmBase, label: str):
+    def __init__(self, charm: CharmBase, secret_label: str):
         self.charm = charm
-        self.label = label  # needs to be charm-unique.
+        self.secret_label = secret_label  # needs to be charm-unique.
 
     @property
     def _secret(self) -> Secret:
         try:
             # we are owners, so we don't need to grant it to ourselves
-            return self.charm.model.get_secret(label=self.label)
+            return self.charm.model.get_secret(label=self.secret_label)
         except SecretNotFoundError:
             # we need to set SOME contents when we're creating the secret, so we do it.
-            return self.charm.app.add_secret({self._uninitialized_key: "42"}, label=self.label)
+            return self.charm.unit.add_secret(
+                {self._uninitialized_key: "42"}, label=self.secret_label
+            )
 
     def store(self, contents: Dict[str, str], clear: bool = False):
         """Create a new revision by updating the previous one with ``contents``."""
@@ -293,7 +302,7 @@ def __init__(
         self.sans_dns = list(filterfalse(is_ip_address, sans))
 
         if self._check_juju_supports_secrets():
-            vault_backend = _SecretVaultBackend(charm, label="cert-handler-private-vault")
+            vault_backend = _SecretVaultBackend(charm, secret_label=VAULT_SECRET_LABEL)
 
             # TODO: gracefully handle situations where the
             #  secret is gone because the admin has removed it manually
@@ -486,8 +495,10 @@ def _csr(self) -> Optional[str]:
         # keep the old one around for a little while. If there's multiple certs, at the moment we're
         # ignoring all but the last one.
         if len(csrs) > 1:
-            logger.warning("Multiple CSRs found in `certificates` relation. "
-                           "cert_handler is not ready to expect it.")
+            logger.warning(
+                "Multiple CSRs found in `certificates` relation. "
+                "cert_handler is not ready to expect it."
+            )
 
         return csrs[-1].csr
 
diff --git a/tests/scenario/test_cert_handler/test_cert_handler_v1.py b/tests/scenario/test_cert_handler/test_cert_handler_v1.py
index 5fd2680..9e2ed94 100644
--- a/tests/scenario/test_cert_handler/test_cert_handler_v1.py
+++ b/tests/scenario/test_cert_handler/test_cert_handler_v1.py
@@ -9,7 +9,10 @@
 libs = str(Path(__file__).parent.parent.parent.parent / "lib")
 sys.path.append(libs)
 
-from lib.charms.observability_libs.v1.cert_handler import CertHandler  # noqa E402
+from lib.charms.observability_libs.v1.cert_handler import (
+    CertHandler,
+    VAULT_SECRET_LABEL,
+)  # noqa E402
 
 
 class MyCharm(CharmBase):
@@ -37,7 +40,7 @@ def certificates():
 @pytest.mark.parametrize("leader", (True, False))
 def test_cert_joins(ctx, certificates, leader):
     with ctx.manager(
-        certificates.joined_event, State(leader=leader, relations=[certificates])
+        certificates.joined_event, State(leader=leader, relations=[certificates], secrets=[])
     ) as mgr:
         mgr.run()
         assert mgr.charm.ch.private_key
diff --git a/tox.ini b/tox.ini
index cec7b7b..7d6511f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -90,7 +90,7 @@ description = Scenario tests (CI satisfaction placeholder)
 description = Scenario tests (manual only: GH runner does not like charmcraft fetch-lib)
 deps =
     pytest
-    ops-scenario >= 5.1
+    ops-scenario
     cryptography
     jsonschema
     -r{toxinidir}/requirements.txt

From 8372347b5d941b6c495c68d66fe3ffe2e0e9c047 Mon Sep 17 00:00:00 2001
From: Pietro Pasotti <starfire.daemon@gmail.com>
Date: Thu, 2 May 2024 13:59:45 +0200
Subject: [PATCH 15/19] fmt

---
 tests/scenario/test_cert_handler/test_cert_handler_v1.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/scenario/test_cert_handler/test_cert_handler_v1.py b/tests/scenario/test_cert_handler/test_cert_handler_v1.py
index 9e2ed94..b235493 100644
--- a/tests/scenario/test_cert_handler/test_cert_handler_v1.py
+++ b/tests/scenario/test_cert_handler/test_cert_handler_v1.py
@@ -6,13 +6,12 @@
 from ops import CharmBase
 from scenario import Context, Relation, State
 
-libs = str(Path(__file__).parent.parent.parent.parent / "lib")
-sys.path.append(libs)
-
 from lib.charms.observability_libs.v1.cert_handler import (
     CertHandler,
-    VAULT_SECRET_LABEL,
-)  # noqa E402
+)
+
+libs = str(Path(__file__).parent.parent.parent.parent / "lib")
+sys.path.append(libs)
 
 
 class MyCharm(CharmBase):

From c3399f3b5ab1b1734c2a99dd84f8ccb79df77b10 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Thu, 2 May 2024 09:01:08 -0300
Subject: [PATCH 16/19] fix ImportError message

---
 lib/charms/observability_libs/v1/cert_handler.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/charms/observability_libs/v1/cert_handler.py b/lib/charms/observability_libs/v1/cert_handler.py
index a154505..ab36935 100644
--- a/lib/charms/observability_libs/v1/cert_handler.py
+++ b/lib/charms/observability_libs/v1/cert_handler.py
@@ -51,7 +51,7 @@
     )
 except ImportError as e:
     raise ImportError(
-        "failed to import charms.tls_certificates_interface.v2.tls_certificates; "
+        "failed to import charms.tls_certificates_interface.v3.tls_certificates; "
         "Either the library itself is missing (please get it through charmcraft fetch-lib) "
         "or one of its dependencies is unmet."
     ) from e

From da5e5b817672f84547cbe0c26a9582f4f12ff0d0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Thu, 2 May 2024 09:49:51 -0300
Subject: [PATCH 17/19] new itests added

---
 tests/integration/test_cert_handler_v1.py | 34 +++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/integration/test_cert_handler_v1.py b/tests/integration/test_cert_handler_v1.py
index e106c1b..3d3cf61 100644
--- a/tests/integration/test_cert_handler_v1.py
+++ b/tests/integration/test_cert_handler_v1.py
@@ -87,3 +87,37 @@ async def test_secrets_does_not_change_after_refresh(ops_test: OpsTest, tester_c
 
     for path in paths:
         assert secrets[path] == get_secret(ops_test, APP_NAME, path)
+
+
+@pytest.mark.abort_on_fail
+async def test_change_ssc_and_tester_still_have_certs(ops_test: OpsTest):
+    await ops_test.model.remove_application("ca", block_until_done=True)
+    await asyncio.gather(
+        ops_test.model.deploy(
+            "self-signed-certificates",
+            application_name="ca2",
+            channel="beta",
+            trust=True,
+        ),
+    )
+    # wait for all charms to be active
+    await ops_test.model.wait_for_idle(apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1)
+    logger.info("All services active")
+
+    await ops_test.model.add_relation(APP_NAME, "ca2")
+    logger.info("Relations issued")
+    await ops_test.model.wait_for_idle(apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1)
+    # Check the certs files are in the filesystem
+    for path in [KEY_PATH, CERT_PATH, CA_CERT_PATH]:
+        assert 0 == subprocess.check_call(
+            [
+                "juju",
+                "ssh",
+                "--model",
+                ops_test.model_full_name,
+                "--container",
+                "httpbin",
+                f"{APP_NAME}/0",
+                f"ls {path}",
+            ]
+        )

From d0bd3ecad62b0397ab6cd0c23e45af5957410ccf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Thu, 2 May 2024 09:51:40 -0300
Subject: [PATCH 18/19] fmt...

---
 tests/integration/test_cert_handler_v1.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/integration/test_cert_handler_v1.py b/tests/integration/test_cert_handler_v1.py
index 3d3cf61..4590da3 100644
--- a/tests/integration/test_cert_handler_v1.py
+++ b/tests/integration/test_cert_handler_v1.py
@@ -101,12 +101,16 @@ async def test_change_ssc_and_tester_still_have_certs(ops_test: OpsTest):
         ),
     )
     # wait for all charms to be active
-    await ops_test.model.wait_for_idle(apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1)
+    await ops_test.model.wait_for_idle(
+        apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1
+    )
     logger.info("All services active")
 
     await ops_test.model.add_relation(APP_NAME, "ca2")
     logger.info("Relations issued")
-    await ops_test.model.wait_for_idle(apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1)
+    await ops_test.model.wait_for_idle(
+        apps=["ca2", APP_NAME], status="active", wait_for_exact_units=1
+    )
     # Check the certs files are in the filesystem
     for path in [KEY_PATH, CERT_PATH, CA_CERT_PATH]:
         assert 0 == subprocess.check_call(

From 1bbf27e7b938e60d4330590b6bb06322fee7c2f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jose=20C=2E=20Mass=C3=B3n?= <jose.masson@canonical.com>
Date: Thu, 2 May 2024 10:10:46 -0300
Subject: [PATCH 19/19] add TODO comments

---
 tests/integration/test_cert_handler_v1.py   | 3 +++
 tests/integration/tester-charm/src/charm.py | 1 +
 2 files changed, 4 insertions(+)

diff --git a/tests/integration/test_cert_handler_v1.py b/tests/integration/test_cert_handler_v1.py
index 4590da3..1b1f42d 100644
--- a/tests/integration/test_cert_handler_v1.py
+++ b/tests/integration/test_cert_handler_v1.py
@@ -125,3 +125,6 @@ async def test_change_ssc_and_tester_still_have_certs(ops_test: OpsTest):
                 f"ls {path}",
             ]
         )
+
+    # TODO: Fix tester charm to use listen HTTPS when the relation to ssc is established
+    # and then curl HTTPS url to check if everything is working.
diff --git a/tests/integration/tester-charm/src/charm.py b/tests/integration/tester-charm/src/charm.py
index e7c698d..a801eb0 100755
--- a/tests/integration/tester-charm/src/charm.py
+++ b/tests/integration/tester-charm/src/charm.py
@@ -40,6 +40,7 @@ def _on_upgrade_charm(self, _):
         self._update_cert()
 
     def _on_server_cert_changed(self, _):
+        # TODO: When the relation is established, configure gunicorn to listen HTTPS.
         self._update_cert()
 
     def _on_httpbin_pebble_ready(self, event: ops.PebbleReadyEvent):