From 53a45f897567240c5e5c1d55fd93b4d94996ba0f Mon Sep 17 00:00:00 2001 From: Jay W Date: Sun, 11 Oct 2026 21:34:03 +0100 Subject: [PATCH 1/2] Wait for certificate storage private DNS readiness (#5240) (cherry picked from commit 83dd5b95409859491d3e86a933e4b91c5118b6e2) --- .../workflows/build_validation_develop.yml | 1 + CHANGELOG.md | 2 + .../test_certificate_storage_readiness.py | 186 ++++++++++++++++++ templates/shared_services/certs/porter.yaml | 2 +- .../certs/scripts/wait_for_private_storage.py | 132 +++++++++++++ .../certs/terraform/staticweb.tf | 25 ++- 6 files changed, 346 insertions(+), 2 deletions(-) create mode 100644 devops/tests/test_certificate_storage_readiness.py create mode 100644 templates/shared_services/certs/scripts/wait_for_private_storage.py diff --git a/.github/workflows/build_validation_develop.yml b/.github/workflows/build_validation_develop.yml index 76625c9853..c1f2b66d10 100644 --- a/.github/workflows/build_validation_develop.yml +++ b/.github/workflows/build_validation_develop.yml @@ -93,6 +93,7 @@ jobs: - '.github/workflows/pr_comment_bot.yml' - 'devops/tests/**' - 'templates/shared_services/sonatype-nexus-vm/scripts/**' + - 'templates/shared_services/certs/**' - 'templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/**' - '.github/workflows/build_validation_develop.yml' - '.github/workflows/clean_validation_envs.yml' diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cbb2e03d9..8ec16fec67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ See [Legacy Airlock & migration](docs/azure-tre-overview/airlock.md#legacy-airlo * Add weekly and manual builds for Dockerfiles and Porter bundles, with selected pull request builds. Retry a failed target once and report final results. ([#5088](https://github.com/microsoft/AzureTRE/issues/5088)) BUG FIXES: + +* Wait for stable private DNS and verified TLS before configuring certificate storage ([#5240](https://github.com/microsoft/AzureTRE/issues/5240)). * Keep multi-step uninstall resources visible until the full pipeline finishes. Preserve final-step failures and skip completed callbacks during redelivery. (`API` 0.28.4) ([#5233](https://github.com/microsoft/AzureTRE/issues/5233)) * Report Porter uninstall lookup errors as failures so a failed child deletion stops parent cleanup. Add regression coverage for missing error output and the API cascade guard. (`resource_processor` 0.13.13, `API` 0.28.3) ([#5228](https://github.com/microsoft/AzureTRE/issues/5228)) * Run selected bundle tests only after successful smoke tests, including deployment reuse. (`build-scripts` 1.1.3) ([#5226](https://github.com/microsoft/AzureTRE/pull/5226)) diff --git a/devops/tests/test_certificate_storage_readiness.py b/devops/tests/test_certificate_storage_readiness.py new file mode 100644 index 0000000000..e147a9d544 --- /dev/null +++ b/devops/tests/test_certificate_storage_readiness.py @@ -0,0 +1,186 @@ +"""Check private certificate storage readiness without contacting Azure.""" + +import contextlib +import importlib.util +import io +import json +from pathlib import Path +import socket +import ssl +import subprocess +import sys +import unittest +from unittest.mock import MagicMock, patch + + +HELPER = Path(__file__).resolve().parents[2] / "templates/shared_services/certs/scripts/wait_for_private_storage.py" +SPEC = importlib.util.spec_from_file_location("certificate_storage_readiness", HELPER) +READINESS = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(READINESS) +HOSTNAME = "certificate.blob.core.windows.net" +PRIVATE_IP = "10.0.1.27" + + +class CertificateStorageReadinessTests(unittest.TestCase): + def test_private_address_uses_verified_tls_and_storage_hostname(self): + connection = MagicMock() + context = MagicMock() + answers = [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (PRIVATE_IP, 443))] + with ( + patch.object(READINESS.socket, "getaddrinfo", return_value=answers), + patch.object(READINESS.socket, "create_connection", return_value=connection) as connect, + patch.object(READINESS.ssl, "create_default_context", return_value=context), + ): + self.assertTrue(READINESS.probe(HOSTNAME, PRIVATE_IP)[0]) + connect.assert_called_once_with((PRIVATE_IP, 443), timeout=5) + context.wrap_socket.assert_called_once_with(connection.__enter__.return_value, server_hostname=HOSTNAME) + + def test_public_mixed_empty_and_wrong_private_answers_never_connect(self): + for addresses in ([], ["52.239.251.164"], [PRIVATE_IP, "52.239.251.164"], ["10.0.1.28"]): + with self.subTest(addresses=addresses): + answers = [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (address, 443)) for address in addresses] + with ( + patch.object(READINESS.socket, "getaddrinfo", return_value=answers), + patch.object(READINESS.socket, "create_connection") as connect, + ): + self.assertFalse(READINESS.probe(HOSTNAME, PRIVATE_IP)[0]) + connect.assert_not_called() + + def test_lookup_failure_does_not_connect(self): + with ( + patch.object(READINESS.socket, "getaddrinfo", side_effect=socket.gaierror("diagnostic detail")), + patch.object(READINESS.socket, "create_connection") as connect, + ): + ready, reason = READINESS.probe(HOSTNAME, PRIVATE_IP) + self.assertFalse(ready) + self.assertIn("gaierror", reason) + self.assertNotIn("diagnostic detail", reason) + connect.assert_not_called() + + def test_tls_verification_failure_is_not_readiness(self): + answers = [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (PRIVATE_IP, 443))] + with ( + patch.object(READINESS.socket, "getaddrinfo", return_value=answers), + patch.object( + READINESS.socket, "create_connection", side_effect=ssl.SSLCertVerificationError("private diagnostic") + ), + ): + ready, reason = READINESS.probe(HOSTNAME, PRIVATE_IP) + self.assertFalse(ready) + self.assertIn("SSLCertVerificationError", reason) + self.assertNotIn("private diagnostic", reason) + + def wait_with_results(self, results, **kwargs): + elapsed = [0.0] + + def sleep(seconds): + elapsed[0] += seconds + + with ( + patch.object(READINESS.time, "monotonic", side_effect=lambda: elapsed[0]), + patch.object(READINESS.time, "sleep", side_effect=sleep), + patch.object(READINESS, "run_probe", side_effect=results) as probe, + contextlib.redirect_stdout(io.StringIO()), + ): + READINESS.wait_for_private_storage(HOSTNAME, PRIVATE_IP, **kwargs) + return probe.call_count, elapsed[0] + + def test_observed_dns_changes_restart_all_consecutive_checks(self): + # Private, lookup error, private, public, private, public, then stable. + results = [ + (ready, "private" if ready else "lookup or public DNS") + for ready in [True, False, True, False, True, False] + [True] * 7 + ] + count, elapsed = self.wait_with_results(results) + self.assertEqual(count, 13) + self.assertEqual(elapsed, 60) + + def test_sustained_readiness_spans_at_least_thirty_seconds(self): + count, elapsed = self.wait_with_results([(True, "ready")] * 7) + self.assertEqual(count, 7) + self.assertEqual(elapsed, 30) + + def test_tls_failure_also_restarts_stability(self): + results = [(True, "ready")] * 6 + [(False, "TLS check failed")] + [(True, "ready")] * 7 + count, elapsed = self.wait_with_results(results) + self.assertEqual(count, 14) + self.assertEqual(elapsed, 65) + + def test_timeout_prevents_unbounded_wait(self): + with self.assertRaisesRegex(TimeoutError, "within 12 seconds"): + self.wait_with_results([(False, "not ready")] * 3, timeout=12) + + def test_probe_gets_only_remaining_deadline(self): + elapsed = [0.0] + + def consume_budget(hostname, expected_ip, timeout): + self.assertEqual(timeout, 2) + elapsed[0] += timeout + return True, "too late" + + with ( + patch.object(READINESS.time, "monotonic", side_effect=lambda: elapsed[0]), + patch.object(READINESS, "run_probe", side_effect=consume_budget), + patch.object(READINESS.time, "sleep") as sleep, + ): + with self.assertRaises(TimeoutError): + READINESS.wait_for_private_storage(HOSTNAME, PRIVATE_IP, timeout=2, required_successes=1) + sleep.assert_not_called() + + def test_blocked_resolver_child_has_finite_timeout(self): + with patch.object(READINESS.subprocess, "run", side_effect=subprocess.TimeoutExpired("probe", 2)) as run: + ready, reason = READINESS.run_probe(HOSTNAME, PRIVATE_IP, 2) + self.assertFalse(ready) + self.assertIn("timed out", reason) + self.assertEqual(run.call_args.kwargs["timeout"], 2) + self.assertFalse(run.call_args.kwargs.get("shell", False)) + + def test_invalid_child_results_cannot_report_readiness(self): + for value, code in [ + ("not json", 0), + ("[]", 0), + ('{"ready":"yes","reason":"x"}', 0), + ('{"ready":true,"reason":"x"}', 1), + ("", 2), + ]: + with self.subTest(value=value, code=code): + result = subprocess.CompletedProcess([], code, value, "internal diagnostic") + with patch.object(READINESS.subprocess, "run", return_value=result): + ready, reason = READINESS.run_probe(HOSTNAME, PRIVATE_IP, 2) + self.assertFalse(ready) + self.assertNotIn("internal diagnostic", reason) + + def test_cli_rejects_public_and_special_expected_addresses(self): + for address in ("52.239.251.164", "127.0.0.1", "169.254.169.254", "0.0.0.0", "invalid"): + with self.subTest(address=address): + result = subprocess.run( + [sys.executable, str(HELPER), "--hostname", HOSTNAME, "--expected-ip", address], + capture_output=True, + text=True, + timeout=5, + check=False, + ) + self.assertEqual(result.returncode, 2) + self.assertIn("private endpoint IP address", result.stderr) + + def test_cli_failure_returns_nonzero(self): + with ( + patch.object(sys, "argv", [str(HELPER), "--hostname", HOSTNAME, "--expected-ip", PRIVATE_IP]), + patch.object(READINESS, "wait_for_private_storage", side_effect=TimeoutError("not ready")), + contextlib.redirect_stderr(io.StringIO()), + ): + self.assertEqual(READINESS.main(), 1) + + def test_probe_mode_reports_json_for_parent(self): + output = io.StringIO() + with ( + patch.object(sys, "argv", [str(HELPER), "--probe", "--hostname", HOSTNAME, "--expected-ip", PRIVATE_IP]), + patch.object(READINESS, "probe", return_value=(False, "not ready")), + contextlib.redirect_stdout(output), + ): + self.assertEqual(READINESS.main(), 1) + self.assertEqual(json.loads(output.getvalue()), {"ready": False, "reason": "not ready"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/templates/shared_services/certs/porter.yaml b/templates/shared_services/certs/porter.yaml index 2ce29f568c..7cc9cf0896 100755 --- a/templates/shared_services/certs/porter.yaml +++ b/templates/shared_services/certs/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-shared-service-certs -version: 0.7.13 +version: 0.7.14 description: "An Azure TRE shared service to generate certificates for a specified internal domain using Letsencrypt" registry: azuretre dockerfile: Dockerfile.tmpl diff --git a/templates/shared_services/certs/scripts/wait_for_private_storage.py b/templates/shared_services/certs/scripts/wait_for_private_storage.py new file mode 100644 index 0000000000..653b48ced0 --- /dev/null +++ b/templates/shared_services/certs/scripts/wait_for_private_storage.py @@ -0,0 +1,132 @@ +#!/usr/bin/env python3 +"""Wait for consecutive private DNS and verified TLS checks before storage writes.""" + +import argparse +import ipaddress +import json +import os +from pathlib import Path +import socket +import ssl +import subprocess +import sys +import time + + +PROBE_TIMEOUT_SECONDS = 10 +TIMEOUT_SECONDS = 300 +REQUIRED_SUCCESSES = 7 +INTERVAL_SECONDS = 5 + + +def probe(hostname, expected_ip): + """Connect only when every DNS answer matches the private endpoint address.""" + try: + answers = { + ipaddress.ip_address(item[4][0]) for item in socket.getaddrinfo(hostname, 443, type=socket.SOCK_STREAM) + } + if answers != {ipaddress.ip_address(expected_ip)}: + return False, "DNS does not exclusively resolve to the expected private endpoint" + context = ssl.create_default_context() + with socket.create_connection((expected_ip, 443), timeout=5) as connection: + with context.wrap_socket(connection, server_hostname=hostname): + return True, "private DNS and TLS verification passed" + except (OSError, ValueError) as error: + return False, f"DNS or TLS check failed ({type(error).__name__})" + + +def run_probe(hostname, expected_ip, timeout): + # A child process bounds getaddrinfo(), which has no per-call timeout. + try: + result = subprocess.run( + [ + sys.executable, + str(Path(__file__).resolve()), + "--probe", + "--hostname", + hostname, + "--expected-ip", + expected_ip, + ], + capture_output=True, + text=True, + timeout=timeout, + check=False, + ) + except subprocess.TimeoutExpired: + return False, "DNS or TLS check timed out" + if result.returncode not in (0, 1): + return False, "DNS or TLS check process failed" + try: + value = json.loads(result.stdout) + if type(value.get("ready")) is not bool or not isinstance(value.get("reason"), str): + raise ValueError("Invalid probe result") + except (AttributeError, ValueError): + return False, "DNS or TLS check returned an invalid result" + if value["ready"] != (result.returncode == 0): + return False, "DNS or TLS check returned an inconsistent result" + return value["ready"], value["reason"] + + +def wait_for_private_storage( + hostname, expected_ip, timeout=TIMEOUT_SECONDS, required_successes=REQUIRED_SUCCESSES, interval=INTERVAL_SECONDS +): + deadline = time.monotonic() + timeout + successes = 0 + last_reason = "no completed check" + while time.monotonic() < deadline: + remaining = deadline - time.monotonic() + if remaining <= 0: + break + ready, last_reason = run_probe(hostname, expected_ip, min(PROBE_TIMEOUT_SECONDS, remaining)) + if time.monotonic() >= deadline: + break + successes = successes + 1 if ready else 0 + print(f"Storage readiness: {last_reason} ({successes}/{required_successes} consecutive checks)", flush=True) + if successes >= required_successes: + return + time.sleep(min(interval, max(0, deadline - time.monotonic()))) + raise TimeoutError( + f"Private storage endpoint was not ready within {timeout} seconds: {last_reason}. Check private DNS and endpoint connectivity before retrying." + ) + + +def parse_args(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--hostname", default=os.environ.get("STORAGE_BLOB_HOST")) + parser.add_argument("--expected-ip", default=os.environ.get("STORAGE_PRIVATE_IP")) + parser.add_argument("--probe", action="store_true", help=argparse.SUPPRESS) + args = parser.parse_args() + if not args.hostname or any(c.isspace() for c in args.hostname) or any(c in args.hostname for c in "/:\\"): + parser.error("A storage DNS hostname is required") + try: + address = ipaddress.ip_address(args.expected_ip) + except ValueError: + parser.error("A private endpoint IP address is required") + if ( + not address.is_private + or address.is_loopback + or address.is_link_local + or address.is_unspecified + or address.is_multicast + ): + parser.error("The expected address must be a private endpoint IP address") + return args + + +def main(): + args = parse_args() + if args.probe: + ready, reason = probe(args.hostname, args.expected_ip) + print(json.dumps({"ready": ready, "reason": reason})) + return 0 if ready else 1 + try: + wait_for_private_storage(args.hostname, args.expected_ip) + except (OSError, TimeoutError) as error: + print(f"Error: {error}", file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/templates/shared_services/certs/terraform/staticweb.tf b/templates/shared_services/certs/terraform/staticweb.tf index ee8093ed53..1d5b97c3a0 100644 --- a/templates/shared_services/certs/terraform/staticweb.tf +++ b/templates/shared_services/certs/terraform/staticweb.tf @@ -47,7 +47,30 @@ resource "azurerm_storage_account_static_website" "staticweb_site" { index_document = "index.html" error_404_document = "404.html" - depends_on = [azurerm_private_endpoint.blobpe, azurerm_private_endpoint.webpe] + depends_on = [terraform_data.staticweb_private_endpoint_ready] +} + +# Endpoint creation can finish before the DNS answers become stable. +resource "terraform_data" "staticweb_private_endpoint_ready" { + triggers_replace = { + endpoint = azurerm_private_endpoint.blobpe.id + address = azurerm_private_endpoint.blobpe.private_service_connection[0].private_ip_address + helper = filesha256("${path.module}/../scripts/wait_for_private_storage.py") + } + + lifecycle { + replace_triggered_by = [azurerm_private_endpoint.blobpe] + } + + depends_on = [azurerm_private_endpoint.webpe] + + provisioner "local-exec" { + command = "python3 \"${path.module}/../scripts/wait_for_private_storage.py\"" + environment = { + STORAGE_BLOB_HOST = azurerm_storage_account.staticweb.primary_blob_host + STORAGE_PRIVATE_IP = azurerm_private_endpoint.blobpe.private_service_connection[0].private_ip_address + } + } } resource "azurerm_role_assignment" "stgwriter" { From 8cd1682b4ef811b7731ce04e7d479795bf6daaca Mon Sep 17 00:00:00 2001 From: Jay W Date: Sun, 11 Oct 2026 23:23:50 +0100 Subject: [PATCH 2/2] Fix #5240: require TLS 1.2 explicitly in storage readiness checks (cherry picked from commit 1c68171f3a2280f569cac6d75880ab80203a6228) --- .../tests/test_certificate_storage_readiness.py | 15 +++++++++++++++ .../certs/scripts/wait_for_private_storage.py | 1 + 2 files changed, 16 insertions(+) diff --git a/devops/tests/test_certificate_storage_readiness.py b/devops/tests/test_certificate_storage_readiness.py index e147a9d544..06ff461b3b 100644 --- a/devops/tests/test_certificate_storage_readiness.py +++ b/devops/tests/test_certificate_storage_readiness.py @@ -35,6 +35,21 @@ def test_private_address_uses_verified_tls_and_storage_hostname(self): connect.assert_called_once_with((PRIVATE_IP, 443), timeout=5) context.wrap_socket.assert_called_once_with(connection.__enter__.return_value, server_hostname=HOSTNAME) + def test_probe_requires_tls12_and_preserves_certificate_and_hostname_checks(self): + context = ssl.create_default_context() + context.minimum_version = ssl.TLSVersion.MINIMUM_SUPPORTED + answers = [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (PRIVATE_IP, 443))] + with ( + patch.object(READINESS.socket, "getaddrinfo", return_value=answers), + patch.object(READINESS.socket, "create_connection", return_value=MagicMock()), + patch.object(READINESS.ssl, "create_default_context", return_value=context), + patch.object(context, "wrap_socket", return_value=MagicMock()), + ): + self.assertTrue(READINESS.probe(HOSTNAME, PRIVATE_IP)[0]) + self.assertEqual(context.minimum_version, ssl.TLSVersion.TLSv1_2) + self.assertEqual(context.verify_mode, ssl.CERT_REQUIRED) + self.assertTrue(context.check_hostname) + def test_public_mixed_empty_and_wrong_private_answers_never_connect(self): for addresses in ([], ["52.239.251.164"], [PRIVATE_IP, "52.239.251.164"], ["10.0.1.28"]): with self.subTest(addresses=addresses): diff --git a/templates/shared_services/certs/scripts/wait_for_private_storage.py b/templates/shared_services/certs/scripts/wait_for_private_storage.py index 653b48ced0..c357930d99 100644 --- a/templates/shared_services/certs/scripts/wait_for_private_storage.py +++ b/templates/shared_services/certs/scripts/wait_for_private_storage.py @@ -28,6 +28,7 @@ def probe(hostname, expected_ip): if answers != {ipaddress.ip_address(expected_ip)}: return False, "DNS does not exclusively resolve to the expected private endpoint" context = ssl.create_default_context() + context.minimum_version = ssl.TLSVersion.TLSv1_2 with socket.create_connection((expected_ip, 443), timeout=5) as connection: with context.wrap_socket(connection, server_hostname=hostname): return True, "private DNS and TLS verification passed"