From 5248e7de4a64820a1d6d24b82fb43c1b3cce7711 Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Tue, 21 Jul 2026 15:43:56 +0200 Subject: [PATCH] Fail closed for SMB referral transports --- README.md | 8 +- dev/connectors/README.md | 13 +-- docs/CONNECTOR_BOUNDARY.md | 7 +- .../backend/storage/connector_browse.py | 4 +- .../backend/storage/connector_imports.py | 4 +- .../backend/storage/connector_providers.py | 2 +- tests/test_connector_providers.py | 79 +++++++++---------- 7 files changed, 58 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index f5560fc..f588e99 100644 --- a/README.md +++ b/README.md @@ -108,10 +108,10 @@ download-link API; Nextcloud and generic WebDAV profiles use authenticated `GET` requests against the configured WebDAV endpoint. SMB profiles use `smb://server[:port]/share[/path]` endpoints and deployment-owned or encrypted stored credentials through `smbprotocol`. Because that SDK cannot accept a -preconnected socket, private-network deployments currently require an explicit -IP address in SMB endpoints; DNS names fail closed until connection-time DNS -pinning is supported. Public-only deployments resolve and substitute a validated -public IP before invoking the SDK. +preconnected socket and may follow DFS referrals to additional hosts, live SMB +access fails closed in both public-only and private-network deployments. It will +remain disabled until every initial connection and referral target can be +policy-validated and pinned. The S3 browse/import implementation and S3 managed-storage backend currently fail closed before creating a `boto3` client. Botocore does not yet use the diff --git a/dev/connectors/README.md b/dev/connectors/README.md index 832b02e..c5c27b6 100644 --- a/dev/connectors/README.md +++ b/dev/connectors/README.md @@ -138,12 +138,13 @@ cd /mnt/DATA/git/govoplan-files/dev/connectors /mnt/DATA/git/govoplan-core/.venv/bin/python smoke.py ``` -The script reads `.env` from this directory when present, seeds tiny WebDAV, -Nextcloud, SMB, and MinIO fixtures, then browses and imports them through the -connector helper layer. Pass `--require-smb` after recreating the SMB container -to fail on SMB access errors instead of reporting them as an optional skip. Pass -`--require-s3` after starting MinIO and installing `govoplan-files[s3]` to fail -on S3 access errors instead of reporting them as an optional skip. +The script reads `.env` from this directory when present and seeds tiny WebDAV, +Nextcloud, SMB, and MinIO fixtures. WebDAV and Nextcloud are browsed and imported +through the pinned connector transport. SMB and S3 product access deliberately +fails closed until their SDK transports support peer pinning, so their fixtures +are retained for transport development and reported as expected optional skips. +The `--require-smb` and `--require-s3` switches are useful only while developing +that transport support and currently make the smoke check fail by design. SMB smoke checks need the optional Python dependency in the environment running the script: diff --git a/docs/CONNECTOR_BOUNDARY.md b/docs/CONNECTOR_BOUNDARY.md index 2b5bafe..929da7e 100644 --- a/docs/CONNECTOR_BOUNDARY.md +++ b/docs/CONNECTOR_BOUNDARY.md @@ -60,9 +60,10 @@ Every provider must: - use a transport that pins every connection to a policy-validated DNS/IP answer and revalidates redirects; SDK transports without that guarantee fail closed -SMB DNS endpoints therefore fail closed in private-network mode until the SDK -supports connection-time pinning; explicit IP endpoints remain available. Live -S3 access is disabled in all modes until `boto3`/botocore can be bound to the +Live SMB access is disabled in all modes until `smbprotocol` initial connections +and DFS referral targets can be pinned and policy-validated. An explicit IP is +not sufficient because the server may still issue a referral to another peer. +Live S3 access is likewise disabled until `boto3`/botocore can be bound to the pinned transport, including SDK-managed redirects and endpoint discovery. ## Non-Goals For Files diff --git a/src/govoplan_files/backend/storage/connector_browse.py b/src/govoplan_files/backend/storage/connector_browse.py index b0a3f3a..3aa0ed8 100644 --- a/src/govoplan_files/backend/storage/connector_browse.py +++ b/src/govoplan_files/backend/storage/connector_browse.py @@ -15,7 +15,7 @@ from defusedxml import ElementTree as SafeElementTree from govoplan_core.security.outbound_http import ( OutboundHttpError, - pinned_outbound_hostname, + validate_unpinned_sdk_host, validate_unpinned_sdk_http_url, ) @@ -783,7 +783,7 @@ def _smb_location(profile: ConnectorProfile) -> _SmbLocation: raise ConnectorBrowseError("SMB connector endpoint_url must include a server") port = parsed.port or _int(profile.metadata.get("port")) or 445 try: - server = pinned_outbound_hostname(server, port=port, label="SMB connector endpoint") + validate_unpinned_sdk_host(server, port=port, label="SMB connector endpoint") except OutboundHttpError as exc: raise ConnectorBrowseError(str(exc)) from exc path_parts = [part for part in unquote(parsed.path or "").strip("/").split("/") if part] diff --git a/src/govoplan_files/backend/storage/connector_imports.py b/src/govoplan_files/backend/storage/connector_imports.py index 85e193c..09edd2d 100644 --- a/src/govoplan_files/backend/storage/connector_imports.py +++ b/src/govoplan_files/backend/storage/connector_imports.py @@ -194,12 +194,12 @@ def _read_webdav_file(profile: ConnectorProfile, *, path: str, max_bytes: int) - def _read_smb_file(profile: ConnectorProfile, *, path: str, max_bytes: int) -> ConnectorDownloadedFile: - location = _smb_location(profile) file_path = normalize_connector_browse_path(path) if not file_path: raise ConnectorImportError("SMB import requires a file path") - unc_path = _smb_unc_path(location, file_path) try: + location = _smb_location(profile) + unc_path = _smb_unc_path(location, file_path) smbclient = _smbclient_module() kwargs = _smb_client_kwargs(profile, location) stat_result = smbclient.stat(unc_path, **kwargs) diff --git a/src/govoplan_files/backend/storage/connector_providers.py b/src/govoplan_files/backend/storage/connector_providers.py index 03318f3..d625235 100644 --- a/src/govoplan_files/backend/storage/connector_providers.py +++ b/src/govoplan_files/backend/storage/connector_providers.py @@ -110,7 +110,7 @@ def connector_provider_descriptors() -> tuple[ConnectorProviderDescriptor, ...]: conflict_strategy="Managed import/sync uses existing files conflict_strategy handling after download.", preview_strategy="Previews are generated from the frozen managed file after import or sync, not directly from the share.", audit_events=("files.connector.imported", "files.connector.synced", "files.connector.accessed"), - notes="Requires the optional smbprotocol dependency and an smb://server[:port]/share[/path] profile endpoint. Private-network mode requires an explicit IP endpoint until the transport supports connection-time DNS pinning.", + notes="Browse/import logic is implemented, but live smbprotocol access fails closed until the SDK supports connection-time DNS/IP pinning for initial connections and DFS referrals.", ), ConnectorProviderDescriptor( provider="s3", diff --git a/tests/test_connector_providers.py b/tests/test_connector_providers.py index 90f3e10..4c74433 100644 --- a/tests/test_connector_providers.py +++ b/tests/test_connector_providers.py @@ -5,7 +5,7 @@ from datetime import UTC, datetime from unittest.mock import patch from govoplan_files.backend.storage.connector_browse import ConnectorBrowseError, _smb_location, browse_connector_profile -from govoplan_files.backend.storage.connector_imports import read_connector_file +from govoplan_files.backend.storage.connector_imports import ConnectorImportError, read_connector_file from govoplan_files.backend.storage.connector_profiles import ConnectorProfile, connector_profiles_from_payload from govoplan_files.backend.storage.connector_providers import connector_provider_descriptors @@ -75,58 +75,55 @@ def s3_profile(**overrides: object) -> ConnectorProfile: class ConnectorProviderTests(unittest.TestCase): - def test_smb_uses_validated_numeric_target_when_private_networks_are_disabled(self) -> None: + def test_smb_sdk_transport_fails_closed_before_client_creation_in_all_modes(self) -> None: profile = ConnectorProfile( - id="public-smb", - label="Public SMB", + id="smb", + label="SMB", provider="smb", endpoint_url="smb://files.example.test/share", ) - with patch.dict( - "os.environ", - {"APP_ENV": "production", "GOVOPLAN_CONNECTOR_ALLOW_PRIVATE_NETWORKS": "false"}, - ), patch( - "govoplan_core.security.outbound_http.socket.getaddrinfo", - return_value=[(2, 1, 6, "", ("93.184.216.34", 445))], - ): - location = _smb_location(profile) + for allow_private, address in ((False, "93.184.216.34"), (True, "10.0.0.5")): + with self.subTest(allow_private=allow_private), patch.dict( + "os.environ", + { + "APP_ENV": "production", + "GOVOPLAN_CONNECTOR_ALLOW_PRIVATE_NETWORKS": str(allow_private).lower(), + }, + ), patch( + "govoplan_core.security.outbound_http.socket.getaddrinfo", + return_value=[(2, 1, 6, "", (address, 445))], + ), patch("govoplan_files.backend.storage.connector_browse._smbclient_module") as sdk, self.assertRaisesRegex( + ConnectorBrowseError, + "redirects/referrals.*DNS/IP pinning", + ): + browse_connector_profile(profile, path="") + sdk.assert_not_called() - self.assertEqual("93.184.216.34", location.server) - - def test_smb_dns_endpoint_fails_closed_when_private_networks_are_enabled(self) -> None: - profile = ConnectorProfile( - id="private-smb", - label="Private SMB", - provider="smb", - endpoint_url="smb://files.internal.example/share", - ) - with patch.dict( - "os.environ", - {"APP_ENV": "production", "GOVOPLAN_CONNECTOR_ALLOW_PRIVATE_NETWORKS": "true"}, - ), patch("govoplan_core.security.outbound_http.socket.getaddrinfo") as resolver, self.assertRaisesRegex( - ConnectorBrowseError, - "cannot pin DNS resolution", - ): - _smb_location(profile) - resolver.assert_not_called() - - def test_smb_explicit_private_ip_is_a_pinned_target(self) -> None: - profile = ConnectorProfile( - id="private-smb", - label="Private SMB", - provider="smb", - endpoint_url="smb://10.0.0.5/share", - ) + def test_smb_explicit_ip_still_fails_closed_because_the_sdk_may_follow_referrals(self) -> None: + profile = ConnectorProfile(id="smb", label="SMB", provider="smb", endpoint_url="smb://10.0.0.5/share") with patch.dict( "os.environ", {"APP_ENV": "production", "GOVOPLAN_CONNECTOR_ALLOW_PRIVATE_NETWORKS": "true"}, ), patch( "govoplan_core.security.outbound_http.socket.getaddrinfo", return_value=[(2, 1, 6, "", ("10.0.0.5", 445))], - ): - location = _smb_location(profile) + ), self.assertRaisesRegex(ConnectorBrowseError, "redirects/referrals.*DNS/IP pinning"): + _smb_location(profile) - self.assertEqual("10.0.0.5", location.server) + def test_smb_import_surfaces_fail_closed_policy_as_an_import_error(self) -> None: + profile = ConnectorProfile(id="smb", label="SMB", provider="smb", endpoint_url="smb://10.0.0.5/share") + with patch.dict( + "os.environ", + {"APP_ENV": "production", "GOVOPLAN_CONNECTOR_ALLOW_PRIVATE_NETWORKS": "true"}, + ), patch( + "govoplan_core.security.outbound_http.socket.getaddrinfo", + return_value=[(2, 1, 6, "", ("10.0.0.5", 445))], + ), patch("govoplan_files.backend.storage.connector_imports._smbclient_module") as sdk, self.assertRaisesRegex( + ConnectorImportError, + "redirects/referrals.*DNS/IP pinning", + ): + read_connector_file(profile, library_id="", path="notice.txt", max_bytes=1024) + sdk.assert_not_called() def test_provider_descriptors_include_s3_and_reserved_microsoft_providers(self) -> None: descriptors = {descriptor.provider: descriptor for descriptor in connector_provider_descriptors()}