From ad4bfbfae6b6266ecdb5b3ab4d3dbea0f6be6cff Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 12:45:15 +0100 Subject: [PATCH 1/7] :loud_sound: Minor logging wording change --- data_safe_haven/external/api/graph_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 34c1403555..f4aebd2486 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -797,14 +797,14 @@ def http_get(self, url: str, **kwargs: Any) -> requests.Response: **kwargs, ) except requests.exceptions.RequestException as exc: - msg = f"Could not execute GET request from '{url}'." + msg = f"Could not execute GET request to '{url}'." raise DataSafeHavenMicrosoftGraphError(msg) from exc # We do not use response.ok as this allows 3xx codes if requests.codes.OK <= response.status_code < requests.codes.MULTIPLE_CHOICES: return response else: - msg = f"Could not execute GET request from '{url}'. Response content received: '{response.content.decode()}'." + msg = f"Could not execute GET request to '{url}'. Response content received: '{response.content.decode()}'. Token {self.token}" raise DataSafeHavenMicrosoftGraphError(msg) def http_patch(self, url: str, **kwargs: Any) -> requests.Response: From be8c261138585c9aa2644aed5345e3847b92d6bf Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 14:36:28 +0100 Subject: [PATCH 2/7] :recycle: Better name for updating encrypted key --- data_safe_haven/infrastructure/project_manager.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/data_safe_haven/infrastructure/project_manager.py b/data_safe_haven/infrastructure/project_manager.py index 33c206e5c1..7ff11ba1d1 100644 --- a/data_safe_haven/infrastructure/project_manager.py +++ b/data_safe_haven/infrastructure/project_manager.py @@ -135,7 +135,7 @@ def stack(self) -> automation.Stack: ) self.logger.info(f"Loaded stack [green]{self.stack_name}[/].") # Ensure encrypted key is stored in the Pulumi configuration - self.update_dsh_pulumi_config() + self.update_dsh_pulumi_encrypted_key() except automation.CommandError as exc: msg = f"Could not load Pulumi stack {self.stack_name}." raise DataSafeHavenPulumiError(msg) from exc @@ -377,13 +377,13 @@ def update_dsh_pulumi_project(self) -> None: } self.pulumi_project.stack_config = all_config_dict - def update_dsh_pulumi_config(self) -> None: - """Update persistent data in the DSHPulumiProject object""" + def update_dsh_pulumi_encrypted_key(self) -> None: + """Update encrypted key in the DSHPulumiProject object""" stack_key = self.stack.workspace.stack_settings( stack_name=self.stack_name ).encrypted_key - if self.pulumi_config.encrypted_key is None: + if not self.pulumi_config.encrypted_key: self.pulumi_config.encrypted_key = stack_key elif self.pulumi_config.encrypted_key != stack_key: msg = "Stack encrypted key does not match project encrypted key" From e7ab2f1bf6ce2a84c19ea29b5f0b77a220e76a7a Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 14:40:53 +0100 Subject: [PATCH 3/7] :bug: Do not update the Pulumi project after we've removed the stack --- data_safe_haven/infrastructure/project_manager.py | 1 - 1 file changed, 1 deletion(-) diff --git a/data_safe_haven/infrastructure/project_manager.py b/data_safe_haven/infrastructure/project_manager.py index 7ff11ba1d1..2596d5e56a 100644 --- a/data_safe_haven/infrastructure/project_manager.py +++ b/data_safe_haven/infrastructure/project_manager.py @@ -352,7 +352,6 @@ def teardown(self) -> None: try: self.refresh() self.destroy() - self.update_dsh_pulumi_project() except Exception as exc: msg = "Tearing down Pulumi infrastructure failed.." raise DataSafeHavenPulumiError(msg) from exc From 3a73371f288c4ab75676396f727e956c67796877 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 12:03:21 +0100 Subject: [PATCH 4/7] :loud_sound: Log Pulumi exceptions --- data_safe_haven/infrastructure/project_manager.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/data_safe_haven/infrastructure/project_manager.py b/data_safe_haven/infrastructure/project_manager.py index 2596d5e56a..714db38ad8 100644 --- a/data_safe_haven/infrastructure/project_manager.py +++ b/data_safe_haven/infrastructure/project_manager.py @@ -137,6 +137,7 @@ def stack(self) -> automation.Stack: # Ensure encrypted key is stored in the Pulumi configuration self.update_dsh_pulumi_encrypted_key() except automation.CommandError as exc: + self.log_exception(exc) msg = f"Could not load Pulumi stack {self.stack_name}." raise DataSafeHavenPulumiError(msg) from exc return self._stack @@ -228,6 +229,7 @@ def destroy(self) -> None: self._stack.workspace.remove_stack(self.stack_name) self.logger.info(f"Removed Pulumi stack [green]{self.stack_name}[/].") except automation.CommandError as exc: + self.log_exception(exc) if "no stack named" not in str(exc): msg = "Pulumi stack could not be removed." raise DataSafeHavenPulumiError(msg) from exc @@ -289,6 +291,11 @@ def install_plugins(self) -> None: msg = "Installing Pulumi plugins failed.." raise DataSafeHavenPulumiError(msg) from exc + def log_exception(self, exc: automation.CommandError) -> None: + with suppress(KeyError): + stderr = str(exc).split("\n")[3].replace(" stderr: ", "") + self.log_message(f"Pulumi output: {stderr}") + def log_message(self, message: str) -> None: return from_ansi(self.logger, message) @@ -322,6 +329,7 @@ def refresh(self) -> None: # Note that we disable parallelisation which can cause deadlock self.stack.refresh(parallel=1, **self.pulumi_extra_args) except automation.CommandError as exc: + self.log_exception(exc) msg = "Pulumi refresh failed." raise DataSafeHavenPulumiError(msg) from exc @@ -331,6 +339,7 @@ def run_pulumi_command(self, command: str) -> str: result = self.stack._run_pulumi_cmd_sync(command.split()) return str(result.stdout) except automation.CommandError as exc: + self.log_exception(exc) msg = "Failed to run command." raise DataSafeHavenPulumiError(msg) from exc @@ -339,6 +348,7 @@ def secret(self, name: str) -> str: try: return str(self.stack.get_config(name).value) except automation.CommandError as exc: + self.log_exception(exc) msg = f"Secret '{name}' was not found." raise DataSafeHavenPulumiError(msg) from exc @@ -353,6 +363,7 @@ def teardown(self) -> None: self.refresh() self.destroy() except Exception as exc: + self.log_exception(exc) msg = "Tearing down Pulumi infrastructure failed.." raise DataSafeHavenPulumiError(msg) from exc @@ -366,6 +377,7 @@ def update(self) -> None: self.evaluate(result.summary.result) self.update_dsh_pulumi_project() except automation.CommandError as exc: + self.log_exception(exc) msg = "Pulumi update failed." raise DataSafeHavenPulumiError(msg) from exc From 514d7e5c0e7b4dc1796d1cbb7b91d3b23cc8b771 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 12:07:59 +0100 Subject: [PATCH 5/7] :truck: Change refresh from a staticmethod to an instance method --- .../infrastructure/components/dynamic/dsh_resource_provider.py | 3 +-- .../infrastructure/components/dynamic/entra_application.py | 3 +-- .../infrastructure/components/dynamic/file_share_file.py | 3 +-- .../infrastructure/components/dynamic/ssl_certificate.py | 3 +-- 4 files changed, 4 insertions(+), 8 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 fe24c13d6c..7cc5a7b439 100644 --- a/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py +++ b/data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py @@ -43,8 +43,7 @@ def partial_diff( delete_before_replace=True, # delete the existing resource before replacing ) - @staticmethod - def refresh(props: dict[str, Any]) -> dict[str, Any]: + def refresh(self, props: dict[str, Any]) -> dict[str, Any]: return dict(**props) def check( diff --git a/data_safe_haven/infrastructure/components/dynamic/entra_application.py b/data_safe_haven/infrastructure/components/dynamic/entra_application.py index 0e13dfc7e8..5a2e3936c2 100644 --- a/data_safe_haven/infrastructure/components/dynamic/entra_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/entra_application.py @@ -35,8 +35,7 @@ def __init__( class EntraApplicationProvider(DshResourceProvider): - @staticmethod - def refresh(props: dict[str, Any]) -> dict[str, Any]: + def refresh(self, props: dict[str, Any]) -> dict[str, Any]: try: outs = dict(**props) with suppress(DataSafeHavenMicrosoftGraphError): diff --git a/data_safe_haven/infrastructure/components/dynamic/file_share_file.py b/data_safe_haven/infrastructure/components/dynamic/file_share_file.py index 6e7f146efd..5a40c5e4aa 100644 --- a/data_safe_haven/infrastructure/components/dynamic/file_share_file.py +++ b/data_safe_haven/infrastructure/components/dynamic/file_share_file.py @@ -66,8 +66,7 @@ def get_file_client( credential=storage_account_key, ) - @staticmethod - def refresh(props: dict[str, Any]) -> dict[str, Any]: + def refresh(self, props: dict[str, Any]) -> dict[str, Any]: with suppress(Exception): file_client = FileShareFileProvider.get_file_client( props["storage_account_name"], diff --git a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py index ad07ea5f14..723b0555d0 100644 --- a/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py +++ b/data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py @@ -43,8 +43,7 @@ def __init__( class SSLCertificateProvider(DshResourceProvider): - @staticmethod - def refresh(props: dict[str, Any]) -> dict[str, Any]: + def refresh(self, props: dict[str, Any]) -> dict[str, Any]: try: outs = dict(**props) with suppress(DataSafeHavenAzureError): From 1dc6f8744ed47d4ab4ea04e3b11ebe80944f6c0d Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 12:41:16 +0100 Subject: [PATCH 6/7] :bug: Make auth_token an attribute of the EntraApplicationProvider rather than a prop. This means it will not be cached in the resource and consequently go stale. --- .../components/dynamic/entra_application.py | 19 ++++++++++--------- .../infrastructure/programs/sre/identity.py | 4 ++-- .../programs/sre/remote_desktop.py | 4 ++-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/data_safe_haven/infrastructure/components/dynamic/entra_application.py b/data_safe_haven/infrastructure/components/dynamic/entra_application.py index 5a2e3936c2..475a5e95cf 100644 --- a/data_safe_haven/infrastructure/components/dynamic/entra_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/entra_application.py @@ -18,7 +18,6 @@ class EntraApplicationProps: def __init__( self, application_name: Input[str], - auth_token: Input[str], application_role_assignments: Input[list[str]] | None = None, application_secret_name: Input[str] | None = None, delegated_role_assignments: Input[list[str]] | None = None, @@ -28,18 +27,21 @@ def __init__( self.application_name = application_name self.application_role_assignments = application_role_assignments self.application_secret_name = application_secret_name - self.auth_token = Output.secret(auth_token) self.delegated_role_assignments = delegated_role_assignments self.public_client_redirect_uri = public_client_redirect_uri self.web_redirect_url = web_redirect_url class EntraApplicationProvider(DshResourceProvider): + def __init__(self, auth_token: str): + self.auth_token = auth_token + super().__init__() + def refresh(self, props: dict[str, Any]) -> dict[str, Any]: try: outs = dict(**props) with suppress(DataSafeHavenMicrosoftGraphError): - graph_api = GraphApi(auth_token=outs["auth_token"]) + graph_api = GraphApi(auth_token=self.auth_token) if json_response := graph_api.get_application_by_name( outs["application_name"] ): @@ -65,7 +67,7 @@ def create(self, props: dict[str, Any]) -> CreateResult: """Create new Entra application.""" outs = dict(**props) try: - graph_api = GraphApi(auth_token=props["auth_token"]) + graph_api = GraphApi(auth_token=self.auth_token) request_json = { "displayName": props["application_name"], "signInAudience": "AzureADMyOrg", @@ -121,7 +123,7 @@ def delete(self, id_: str, props: dict[str, Any]) -> None: # Use `id` as a no-op to avoid ARG002 while maintaining function signature id(id_) try: - graph_api = GraphApi(auth_token=props["auth_token"]) + graph_api = GraphApi(auth_token=self.auth_token) graph_api.delete_application(props["application_name"]) except Exception as exc: msg = f"Failed to delete application '{props['application_name']}' from Entra ID." @@ -136,8 +138,7 @@ def diff( """Calculate diff between old and new state""" # Use `id` as a no-op to avoid ARG002 while maintaining function signature id(id_) - # Exclude "auth_token" which should not trigger a diff - return self.partial_diff(old_props, new_props, ["auth_token"]) + return self.partial_diff(old_props, new_props) def update( self, @@ -149,7 +150,6 @@ def update( try: # Delete the old application, using the auth token from new_props old_props_ = {**old_props} - old_props_["auth_token"] = new_props["auth_token"] self.delete(id_, old_props_) # Create a new application updated = self.create(new_props) @@ -169,10 +169,11 @@ def __init__( self, name: str, props: EntraApplicationProps, + auth_token: str, opts: ResourceOptions | None = None, ): super().__init__( - EntraApplicationProvider(), + EntraApplicationProvider(auth_token), name, { "application_id": None, diff --git a/data_safe_haven/infrastructure/programs/sre/identity.py b/data_safe_haven/infrastructure/programs/sre/identity.py index c5ebac6951..88b719f302 100644 --- a/data_safe_haven/infrastructure/programs/sre/identity.py +++ b/data_safe_haven/infrastructure/programs/sre/identity.py @@ -25,7 +25,7 @@ def __init__( dns_resource_group_name: Input[str], dns_server_ip: Input[str], entra_application_name: Input[str], - entra_auth_token: Input[str], + entra_auth_token: str, entra_tenant_id: Input[str], location: Input[str], networking_resource_group_name: Input[str], @@ -99,10 +99,10 @@ def __init__( application_name=props.entra_application_name, application_role_assignments=["User.Read.All", "GroupMember.Read.All"], application_secret_name="Apricot Authentication Secret", - auth_token=props.entra_auth_token, delegated_role_assignments=["User.Read.All"], public_client_redirect_uri="urn:ietf:wg:oauth:2.0:oob", ), + auth_token=props.entra_auth_token, opts=child_opts, ) diff --git a/data_safe_haven/infrastructure/programs/sre/remote_desktop.py b/data_safe_haven/infrastructure/programs/sre/remote_desktop.py index 8231399920..509bb60863 100644 --- a/data_safe_haven/infrastructure/programs/sre/remote_desktop.py +++ b/data_safe_haven/infrastructure/programs/sre/remote_desktop.py @@ -37,7 +37,7 @@ def __init__( dns_server_ip: Input[str], entra_application_fqdn: Input[str], entra_application_name: Input[str], - entra_auth_token: Input[str], + entra_auth_token: str, entra_tenant_id: Input[str], ldap_group_filter: Input[str], ldap_group_search_base: Input[str], @@ -135,9 +135,9 @@ def __init__( f"{self._name}_entra_application", EntraApplicationProps( application_name=props.entra_application_name, - auth_token=props.entra_auth_token, web_redirect_url=props.entra_application_url, ), + auth_token=props.entra_auth_token, opts=child_opts, ) From 607c3576f86b49527c3c0f4dd7868c40a763030d Mon Sep 17 00:00:00 2001 From: James Robinson Date: Fri, 28 Jun 2024 14:08:16 +0100 Subject: [PATCH 7/7] :truck: Remove an existing secret if it exists --- data_safe_haven/external/api/graph_api.py | 27 +++++++++++++---------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index f4aebd2486..363c90a2b5 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -20,7 +20,6 @@ from data_safe_haven import console from data_safe_haven.exceptions import ( DataSafeHavenMicrosoftGraphError, - DataSafeHavenValueError, ) from data_safe_haven.functions import alphanumeric from data_safe_haven.logging import get_logger @@ -266,27 +265,31 @@ def create_application( def create_application_secret( self, application_name: str, application_secret_name: str ) -> str: - """Add a secret to an existing Entra application + """Add a secret to an existing Entra application, overwriting any existing secret. Returns: str: Contents of newly-created secret Raises: - DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists + DataSafeHavenMicrosoftGraphError if the secret could not be created """ try: application_json = self.get_application_by_name(application_name) if not application_json: msg = f"Could not retrieve application '{application_name}'" raise DataSafeHavenMicrosoftGraphError(msg) - # If the secret already exists then raise an exception - if "passwordCredentials" in application_json and any( - cred["displayName"] == application_secret_name - for cred in application_json["passwordCredentials"] - ): - msg = f"Secret '{application_secret_name}' already exists in application '{application_name}'." - raise DataSafeHavenValueError(msg) - # Create the application secret if it does not exist + # If the secret already exists then remove it + if "passwordCredentials" in application_json: + for secret in application_json["passwordCredentials"]: + if secret["displayName"] == application_secret_name: + self.logger.debug( + f"Removing pre-existing secret '{secret['displayName']}' from application '{application_name}'." + ) + self.http_post( + f"{self.base_endpoint}/applications/{application_json['id']}/removePassword", + json={"keyId": secret["keyId"]}, + ) + # Create the application secret self.logger.debug( f"Creating application secret '[green]{application_secret_name}[/]'...", ) @@ -303,7 +306,7 @@ def create_application_secret( f"{self.base_endpoint}/applications/{application_json['id']}/addPassword", json=request_json, ).json() - self.logger.info( + self.logger.debug( f"Created application secret '[green]{application_secret_name}[/]'.", ) return str(json_response["secretText"])