From 11e9d3648dd03b22d707b448737e91ec0d474725 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 24 Jan 2025 13:40:32 +0000 Subject: [PATCH 01/10] :bug: Store certificate expiry date and mark for renewal if there are fewer than 30 days of validity remaining --- .../components/dynamic/ssl_certificate.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index b80e6a7dbe..0263b9b0b7 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -2,6 +2,7 @@ import time from contextlib import suppress +from datetime import datetime, timedelta, timezone from typing import Any from acme.errors import ValidationError @@ -118,6 +119,7 @@ def create(self, props: dict[str, Any]) -> CreateResult: certificate_contents=pfx_bytes, key_vault_name=props["key_vault_name"], ) + outs["expiry_date"] = (datetime.now(timezone.utc) + timedelta(days=90)).isoformat() outs["secret_id"] = kvcert.secret_id except Exception as exc: cert_name = f"[green]{props['certificate_secret_name']}[/]" @@ -161,7 +163,15 @@ def diff( """Calculate diff between old and new state""" # Use `id` as a no-op to avoid ARG002 while maintaining function signature id(id_) - return self.partial_diff(old_props, new_props, []) + partial = self.partial_diff(old_props, new_props, []) + expiry_date = datetime.fromisoformat(old_props.get("expiry_date", "0001-01-01T00:00:00+00:00")) + needs_renewal = (datetime.now(timezone.utc) + timedelta(days=30) > expiry_date) + return DiffResult( + changes=partial.changes or needs_renewal, + replaces=partial.replaces, + stables=partial.stables, + delete_before_replace=True, + ) def refresh(self, props: dict[str, Any]) -> dict[str, Any]: try: @@ -183,6 +193,7 @@ def refresh(self, props: dict[str, Any]) -> dict[str, Any]: class SSLCertificate(Resource): _resource_type_name = "dsh:common:SSLCertificate" # set resource type + expiry_date: Output[str] secret_id: Output[str] def __init__( @@ -194,6 +205,6 @@ def __init__( super().__init__( SSLCertificateProvider(), name, - {"secret_id": None, **vars(props)}, + {"expiry_date": None, "secret_id": None, **vars(props)}, opts, ) From 66738c9ec21fd415d68399afbf60f46945c1b577 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 24 Jan 2025 14:48:01 +0000 Subject: [PATCH 02/10] :art: Use certificate expiry date rather than trying to guess --- .../infrastructure/components/dynamic/ssl_certificate.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 0263b9b0b7..36b719ca16 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -119,7 +119,8 @@ def create(self, props: dict[str, Any]) -> CreateResult: certificate_contents=pfx_bytes, key_vault_name=props["key_vault_name"], ) - outs["expiry_date"] = (datetime.now(timezone.utc) + timedelta(days=90)).isoformat() + with suppress(AttributeError): + outs["expiry_date"] = kvcert.properties.expires_on.isoformat() outs["secret_id"] = kvcert.secret_id except Exception as exc: cert_name = f"[green]{props['certificate_secret_name']}[/]" From a01221e999709c1c2b60c6218e313d8bf4a009de Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 24 Jan 2025 14:51:15 +0000 Subject: [PATCH 03/10] :bug: Use non-versioned secret ID for application gateway --- .../infrastructure/components/dynamic/ssl_certificate.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 36b719ca16..9318c5a0ed 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -121,7 +121,8 @@ def create(self, props: dict[str, Any]) -> CreateResult: ) with suppress(AttributeError): outs["expiry_date"] = kvcert.properties.expires_on.isoformat() - outs["secret_id"] = kvcert.secret_id + with suppress(AttributeError): + outs["secret_id"] = "/".join(kvcert.secret_id.split("/")[:-1]) except Exception as exc: cert_name = f"[green]{props['certificate_secret_name']}[/]" domain_name = f"[green]{props['domain_name']}[/]" From 5a05abe772b9135670e7b2e2eccdda3a40660396 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 11:44:28 +0000 Subject: [PATCH 04/10] :bug: Set a maximum number of times to poll for a deleted certificate to catch the case where a non-existent certificate is being purged --- data_safe_haven/external/api/azure_sdk.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/external/api/azure_sdk.py b/data_safe_haven/external/api/azure_sdk.py index 1792988348..3b1f5f7d60 100644 --- a/data_safe_haven/external/api/azure_sdk.py +++ b/data_safe_haven/external/api/azure_sdk.py @@ -827,7 +827,9 @@ def import_keyvault_certificate( ) break except ResourceExistsError: - # Purge any existing deleted certificate with the same name + # Delete any certificate with the same name + self.remove_keyvault_certificate(certificate_name, key_vault_name) + # Purge any existing deleted certificate self.purge_keyvault_certificate(certificate_name, key_vault_name) self.logger.info( f"Imported certificate [green]{certificate_name}[/].", @@ -1094,8 +1096,8 @@ def remove_keyvault_certificate( self.logger.debug( f"Waiting for deletion to complete for certificate [green]{certificate_name}[/]..." ) - while True: - # Keep polling until deleted certificate is available + # Keep polling until deleted certificate is available or 2 minutes have elapsed + for _ in range(12): with suppress(ResourceNotFoundError): if certificate_client.get_deleted_certificate(certificate_name): break From 7732e2b54e17b762935e51ed2fd24a2595bdf462 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 11:45:31 +0000 Subject: [PATCH 05/10] :memo: Minor phrasing update --- docs/source/deployment/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/deployment/index.md b/docs/source/deployment/index.md index 435d32ceb5..cb1bd4dcb3 100644 --- a/docs/source/deployment/index.md +++ b/docs/source/deployment/index.md @@ -46,7 +46,7 @@ See [the instructions here](https://docs.docker.com/security/for-developers/acce $ pipx install data-safe-haven ::: -- Or install a specific version with +- Or install a specific version with (for instance) :::{code} shell $ pipx install data-safe-haven==5.0.0 From d6546aef1f4abcf1651b70b881c5fa4a648edab7 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 11:52:14 +0000 Subject: [PATCH 06/10] :goal_net: Catch specific exceptions rather than general in SSLCertificateProvider::create --- .../dynamic/dsh_resource_provider.py | 3 +++ .../components/dynamic/ssl_certificate.py | 24 ++++++++++++------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py b/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py index 5cc64f0a8e..9e5ce8b5b8 100644 --- a/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py +++ b/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py @@ -84,6 +84,9 @@ def create(self, props: dict[str, Any]) -> CreateResult: Returns: CreateResult: a unique ID for this object plus a set of output properties + + Raises: + An appropriate DataSafeHavenError if the resource could not be created """ @abstractmethod diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 9318c5a0ed..2acf29cd3c 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -3,7 +3,7 @@ import time from contextlib import suppress from datetime import datetime, timedelta, timezone -from typing import Any +from typing import Any, override from acme.errors import ValidationError from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey @@ -16,6 +16,7 @@ from pulumi import Input, Output, ResourceOptions from pulumi.dynamic import CreateResult, DiffResult, Resource from simple_acme_dns import ACMEClient +from simple_acme_dns.errors import InvalidKeyType from data_safe_haven.exceptions import DataSafeHavenAzureError, DataSafeHavenSSLError from data_safe_haven.external import AzureSdk @@ -44,8 +45,8 @@ def __init__( class SSLCertificateProvider(DshResourceProvider): + @override def create(self, props: dict[str, Any]) -> CreateResult: - """Create new SSL certificate.""" outs = dict(**props) try: client = ACMEClient( @@ -61,8 +62,8 @@ def create(self, props: dict[str, Any]) -> CreateResult: private_key_bytes = client.generate_private_key(key_type="rsa2048") client.generate_csr() # Request DNS verification tokens and add them to the DNS record - azure_sdk = AzureSdk(props["subscription_name"], disable_logging=True) verification_tokens = client.request_verification_tokens().items() + azure_sdk = AzureSdk(props["subscription_name"], disable_logging=True) for record_name, record_values in verification_tokens: record_set = azure_sdk.ensure_dns_txt_record( record_name=record_name.replace(f".{props['domain_name']}", ""), @@ -97,7 +98,7 @@ def create(self, props: dict[str, Any]) -> CreateResult: private_key = load_pem_private_key(private_key_bytes, None) if not isinstance(private_key, RSAPrivateKey): msg = f"Private key is of type {type(private_key)} not RSAPrivateKey." - raise TypeError(msg) + raise DataSafeHavenSSLError(msg) all_certs = [ load_pem_x509_certificate(data) for data in certificate_bytes.split(b"\n\n") @@ -119,11 +120,16 @@ def create(self, props: dict[str, Any]) -> CreateResult: certificate_contents=pfx_bytes, key_vault_name=props["key_vault_name"], ) - with suppress(AttributeError): - outs["expiry_date"] = kvcert.properties.expires_on.isoformat() - with suppress(AttributeError): - outs["secret_id"] = "/".join(kvcert.secret_id.split("/")[:-1]) - except Exception as exc: + # Failures here will raise an exception that will be caught below + outs["expiry_date"] = kvcert.properties.expires_on.isoformat() + outs["secret_id"] = "/".join(kvcert.secret_id.split("/")[:-1]) + except ( + AttributeError, + DataSafeHavenAzureError, + IndexError, + InvalidKeyType, + StopIteration, + ) as exc: cert_name = f"[green]{props['certificate_secret_name']}[/]" domain_name = f"[green]{props['domain_name']}[/]" msg = f"Failed to create SSL certificate {cert_name} for {domain_name}." From 57805ac58833562e2b28626d3739156706f8d747 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 11:53:02 +0000 Subject: [PATCH 07/10] :goal_net: Catch specific exceptions rather than general in SSLCertificateProvider::delete --- .../components/dynamic/dsh_resource_provider.py | 3 +++ .../infrastructure/components/dynamic/ssl_certificate.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py b/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py index 9e5ce8b5b8..f461860a6c 100644 --- a/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py +++ b/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py @@ -97,6 +97,9 @@ def delete(self, id_: str, old_props: dict[str, Any]) -> None: Args: id_: the ID of the resource old_props: the outputs from the last create operation + + Raises: + An appropriate DataSafeHavenError if the resource could not be deleted """ @abstractmethod diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 2acf29cd3c..04e7cc436b 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -139,8 +139,8 @@ def create(self, props: dict[str, Any]) -> CreateResult: outs=outs, ) + @override def delete(self, id_: str, props: dict[str, Any]) -> None: - """Delete an SSL certificate.""" # Use `id` as a no-op to avoid ARG002 while maintaining function signature id(id_) try: @@ -156,7 +156,7 @@ def delete(self, id_: str, props: dict[str, Any]) -> None: certificate_name=props["certificate_secret_name"], key_vault_name=props["key_vault_name"], ) - except Exception as exc: + except DataSafeHavenAzureError as exc: cert_name = f"[green]{props['certificate_secret_name']}[/]" domain_name = f"[green]{props['domain_name']}[/]" msg = f"Failed to delete SSL certificate {cert_name} for {domain_name}." From b12dc8cb8647b8dc006ed37393cb527371f106fa Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 11:55:02 +0000 Subject: [PATCH 08/10] :rotating_light: Fix timezone import in SSLCertificateProvider::diff --- .../components/dynamic/ssl_certificate.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 04e7cc436b..97b719c833 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -2,7 +2,7 @@ import time from contextlib import suppress -from datetime import datetime, timedelta, timezone +from datetime import UTC, datetime, timedelta from typing import Any, override from acme.errors import ValidationError @@ -162,18 +162,20 @@ def delete(self, id_: str, props: dict[str, Any]) -> None: msg = f"Failed to delete SSL certificate {cert_name} for {domain_name}." raise DataSafeHavenSSLError(msg) from exc + @override def diff( self, id_: str, old_props: dict[str, Any], new_props: dict[str, Any], ) -> DiffResult: - """Calculate diff between old and new state""" # Use `id` as a no-op to avoid ARG002 while maintaining function signature id(id_) partial = self.partial_diff(old_props, new_props, []) - expiry_date = datetime.fromisoformat(old_props.get("expiry_date", "0001-01-01T00:00:00+00:00")) - needs_renewal = (datetime.now(timezone.utc) + timedelta(days=30) > expiry_date) + expiry_date = datetime.fromisoformat( + old_props.get("expiry_date", "0001-01-01T00:00:00+00:00") + ) + needs_renewal = datetime.now(UTC) + timedelta(days=30) > expiry_date return DiffResult( changes=partial.changes or needs_renewal, replaces=partial.replaces, From a9f8f1180edd9cf4aca75d99708bd88ac233c39f Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 12:07:43 +0000 Subject: [PATCH 09/10] :goal_net: Simplify SSLCertificateProvider::refresh --- data_safe_haven/external/api/azure_sdk.py | 4 +-- .../components/dynamic/ssl_certificate.py | 27 +++++++++---------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/data_safe_haven/external/api/azure_sdk.py b/data_safe_haven/external/api/azure_sdk.py index 3b1f5f7d60..db986fa369 100644 --- a/data_safe_haven/external/api/azure_sdk.py +++ b/data_safe_haven/external/api/azure_sdk.py @@ -641,10 +641,10 @@ def get_keyvault_certificate( """Read a certificate from the KeyVault Returns: - KeyVaultCertificate: The certificate + The KeyVaultCertificate Raises: - DataSafeHavenAzureError if the secret could not be read + DataSafeHavenAzureError if the certificate could not be read """ # Connect to Azure clients certificate_client = CertificateClient( diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index 97b719c833..9928a4093d 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -183,22 +183,19 @@ def diff( delete_before_replace=True, ) + @override def refresh(self, props: dict[str, Any]) -> dict[str, Any]: - try: - outs = dict(**props) - with suppress(DataSafeHavenAzureError, KeyError): - azure_sdk = AzureSdk(outs["subscription_name"], disable_logging=True) - certificate = azure_sdk.get_keyvault_certificate( - outs["certificate_secret_name"], outs["key_vault_name"] - ) - if certificate.secret_id: - outs["secret_id"] = certificate.secret_id - return outs - except Exception as exc: - cert_name = f"[green]{props['certificate_secret_name']}[/]" - domain_name = f"[green]{props['domain_name']}[/]" - msg = f"Failed to refresh SSL certificate {cert_name} for {domain_name}." - raise DataSafeHavenSSLError(msg) from exc + outs = dict(**props) + with suppress(DataSafeHavenAzureError, KeyError): + azure_sdk = AzureSdk(outs["subscription_name"], disable_logging=True) + kvcert = azure_sdk.get_keyvault_certificate( + outs["certificate_secret_name"], outs["key_vault_name"] + ) + if kvcert.secret_id: + outs["secret_id"] = kvcert.secret_id + if kvcert.properties and kvcert.properties.expires_on: + outs["expiry_date"] = kvcert.properties.expires_on.isoformat() + return outs class SSLCertificate(Resource): From 9b4dc9e0f9f17bfe86ab485fb9eeacbd11cbe3cc Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 27 Jan 2025 14:08:13 +0000 Subject: [PATCH 10/10] :memo: Update suggested pipx version Co-authored-by: Jim Madge --- docs/source/deployment/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/deployment/index.md b/docs/source/deployment/index.md index cb1bd4dcb3..94b909fdb9 100644 --- a/docs/source/deployment/index.md +++ b/docs/source/deployment/index.md @@ -49,7 +49,7 @@ $ pipx install data-safe-haven - Or install a specific version with (for instance) :::{code} shell -$ pipx install data-safe-haven==5.0.0 +$ pipx install data-safe-haven==5.3.1 ::: ::::{admonition} [Advanced] install into a virtual environment