From 65d8ed80b5a06c87838df1de0d5c5f92019818ba Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Tue, 21 Jul 2026 16:16:47 +0200 Subject: [PATCH] security(files): scrub connector credentials on deletion --- package.json | 2 +- pyproject.toml | 2 +- src/govoplan_files/backend/manifest.py | 72 +++- src/govoplan_files/backend/router.py | 79 ++-- .../storage/connector_credential_deletion.py | 337 ++++++++++++++++++ .../storage/connector_credential_store.py | 18 +- .../backend/storage/connector_deployment.py | 46 ++- .../storage/connector_profile_store.py | 16 +- .../backend/storage/connector_profiles.py | 2 + tests/test_connector_credential_deletion.py | 272 ++++++++++++++ tests/test_connector_deployment.py | 31 ++ webui/package.json | 2 +- 12 files changed, 824 insertions(+), 55 deletions(-) create mode 100644 src/govoplan_files/backend/storage/connector_credential_deletion.py create mode 100644 tests/test_connector_credential_deletion.py diff --git a/package.json b/package.json index f9b396f..8f3a831 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@govoplan/files-webui", - "version": "0.1.8", + "version": "0.1.9", "private": true, "type": "module", "main": "webui/src/index.ts", diff --git a/pyproject.toml b/pyproject.toml index 4b8ee87..ae9d2f3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "govoplan-files" -version = "0.1.8" +version = "0.1.9" description = "GovOPlaN files module with backend and WebUI integration." readme = "README.md" requires-python = ">=3.12" diff --git a/src/govoplan_files/backend/manifest.py b/src/govoplan_files/backend/manifest.py index 7cd2cc9..1117714 100644 --- a/src/govoplan_files/backend/manifest.py +++ b/src/govoplan_files/backend/manifest.py @@ -1,7 +1,10 @@ from __future__ import annotations +from dataclasses import replace from pathlib import Path +from sqlalchemy import inspect + from govoplan_core.core.access import CAPABILITY_AUTH_PERMISSION_EVALUATOR, CAPABILITY_AUTH_PRINCIPAL_RESOLVER from govoplan_core.core.files import CAPABILITY_FILES_ACCESS from govoplan_core.core.module_guards import drop_table_retirement_provider, persistent_table_uninstall_guard @@ -26,6 +29,55 @@ from govoplan_files.backend.db import models as file_models # noqa: F401 - popu register_files_change_tracking() +_files_table_retirement_provider = drop_table_retirement_provider( + file_models.FileBlob, + file_models.FileFolder, + file_models.FileAsset, + file_models.FileVersion, + file_models.FileShare, + file_models.FileConnectorCredential, + file_models.FileConnectorPolicy, + file_models.FileConnectorProfile, + file_models.FileConnectorSpace, + file_models.CampaignAttachmentUse, + label="Files", +) + + +def _files_retirement_provider(session: object | None, module_id: str): + plan = _files_table_retirement_provider(session, module_id) + base_executor = plan.destroy_data_executor + if base_executor is None: + return plan + + def executor(execute_session: object, execute_module_id: str) -> None: + if not hasattr(execute_session, "get_bind") or not hasattr(execute_session, "query"): + raise RuntimeError("No database session is available for Files credential retirement.") + live_inspector = inspect(execute_session.get_bind()) + if any( + live_inspector.has_table(table_name) + for table_name in ( + file_models.FileConnectorCredential.__tablename__, + file_models.FileConnectorProfile.__tablename__, + ) + ): + from govoplan_files.backend.storage.connector_credential_deletion import ( + delete_connector_credentials_for_retirement, + ) + + delete_connector_credentials_for_retirement(execute_session) + base_executor(execute_session, execute_module_id) + + return replace( + plan, + destroy_data_warnings=( + *plan.destroy_data_warnings, + "Files-owned encrypted connector credentials are scrubbed and audited immediately before tables are dropped; legacy non-owned external references are detached without claiming provider-side deletion.", + ), + destroy_data_executor=executor, + ) + + def _permission(scope: str, label: str, description: str) -> PermissionDefinition: module_id, resource, action = scope.split(":", 2) return PermissionDefinition( @@ -115,7 +167,7 @@ def _files_router(context: ModuleContext): manifest = ModuleManifest( id="files", name="Files", - version="0.1.8", + version="0.1.9", required_capabilities=(CAPABILITY_AUTH_PRINCIPAL_RESOLVER, CAPABILITY_AUTH_PERMISSION_EVALUATOR), optional_dependencies=("campaigns",), provides_interfaces=( @@ -149,6 +201,7 @@ manifest = ModuleManifest( body=( "Users select only connector profiles visible in their current scope and import remote content into managed Files storage before another module uses it. " "Administrators define profiles, separate credential references, and ordered system/tenant/owner policies; deny rules win and profile responses never expose secrets. " + "Deleting a database-managed profile or credential immediately scrubs Files-owned encrypted material and records a non-secret audit event in the same transaction; legacy non-owned external references are detached without provider calls. " "Operators control private-network access deployment-wide and must keep every remote connection pinned to a policy-validated DNS/IP answer. " "The built-in HTTP transport pins each connection and refuses redirects. Live S3 and SMB SDK access fails closed until S3 redirects and SMB DFS referrals can be revalidated and pinned. " "Successful imports store the connector id, provider, remote path and identity, source revision, and selected metadata as provenance on the managed file and its audit events." @@ -195,6 +248,9 @@ manifest = ModuleManifest( "Every network peer must be policy-validated and pinned at connection time.", "Redirects and protocol referrals must be rejected or independently revalidated and pinned.", "SDK transports that cannot provide those guarantees fail before client construction.", + "New API-managed external secret references fail closed until Files can prove ownership and provider-side deletion.", + "Deletion and destructive retirement scrub Files-owned encrypted connector material before completion and emit non-secret audit evidence.", + "Legacy non-owned external references are detached and audited, never sent to an arbitrary provider delete operation.", ], "provenance_fields": [ "connector_id", @@ -213,19 +269,7 @@ manifest = ModuleManifest( metadata=Base.metadata, script_location=str(Path(__file__).with_name("migrations") / "versions"), retirement_supported=True, - retirement_provider=drop_table_retirement_provider( - file_models.FileBlob, - file_models.FileFolder, - file_models.FileAsset, - file_models.FileVersion, - file_models.FileShare, - file_models.FileConnectorCredential, - file_models.FileConnectorPolicy, - file_models.FileConnectorProfile, - file_models.FileConnectorSpace, - file_models.CampaignAttachmentUse, - label="Files", - ), + retirement_provider=_files_retirement_provider, retirement_notes="Destructive retirement drops files-owned database tables after the installer captures a database snapshot.", ), uninstall_guard_providers=( diff --git a/src/govoplan_files/backend/router.py b/src/govoplan_files/backend/router.py index 9e35b42..ea6b27a 100644 --- a/src/govoplan_files/backend/router.py +++ b/src/govoplan_files/backend/router.py @@ -108,11 +108,14 @@ from govoplan_files.backend.storage.connector_credential_store import ( ConnectorCredential, connector_credential_from_row, create_connector_credential_row, - deactivate_connector_credential_row, get_connector_credential_row, list_database_connector_credentials, update_connector_credential_row, ) +from govoplan_files.backend.storage.connector_credential_deletion import ( + delete_connector_credential_row, + delete_connector_profile_row, +) from govoplan_files.backend.storage.connector_browse import ( ConnectorBrowseError, ConnectorBrowseUnsupported, @@ -127,7 +130,6 @@ from govoplan_files.backend.storage.connector_deployment import ( from govoplan_files.backend.storage.connector_profile_store import ( connector_profile_from_row, create_connector_profile_row, - deactivate_connector_profile_row, get_connector_profile_row, list_database_connector_profiles, update_connector_profile_row, @@ -2530,6 +2532,7 @@ def discover_connector_endpoint( reject_api_controlled_deployment_references( password_env=payload.credentials.password_env, token_env=payload.credentials.token_env, + secret_ref=payload.credentials.secret_ref, metadata=payload.metadata, ) except ValueError as exc: @@ -2856,23 +2859,49 @@ def deactivate_connector_credential( raise _http_error(exc, not_found=True) from exc _require_connector_credential_write(principal, row.scope_type) try: - deactivate_connector_credential_row(session, row, user_id=principal.user.id) - _record_connector_settings_change( + deletion = delete_connector_credential_row( session, - collection=FILES_CONNECTOR_CREDENTIALS_COLLECTION, - resource_type=FILES_CONNECTOR_CREDENTIAL_RESOURCE, - resource_id=row.id, - operation="deleted", - principal=principal, - tenant_id=row.tenant_id, - payload={"scope_type": row.scope_type, "scope_id": row.scope_id, "provider": row.provider}, + row, + deletion_reason="api_delete", + user_id=principal.user.id, + api_key_id=principal.api_key.id if principal.api_key else None, ) + if deletion.changed: + _record_connector_settings_change( + session, + collection=FILES_CONNECTOR_CREDENTIALS_COLLECTION, + resource_type=FILES_CONNECTOR_CREDENTIAL_RESOURCE, + resource_id=row.id, + operation="deleted", + principal=principal, + tenant_id=row.tenant_id, + payload={"scope_type": row.scope_type, "scope_id": row.scope_id, "provider": row.provider}, + ) + for profile in deletion.affected_profiles: + _record_connector_settings_change( + session, + collection=FILES_CONNECTOR_PROFILES_COLLECTION, + resource_type=FILES_CONNECTOR_PROFILE_RESOURCE, + resource_id=profile.id, + operation="updated", + principal=principal, + tenant_id=profile.tenant_id, + payload={ + "scope_type": profile.scope_type, + "scope_id": profile.scope_id, + "provider": profile.provider, + "reason": "credential_deleted", + }, + ) session.commit() session.refresh(row) return _connector_credential_response(row) except FileStorageError as exc: session.rollback() raise _http_error(exc) from exc + except Exception: + session.rollback() + raise @router.get("/connectors/profiles", response_model=FileConnectorProfilesResponse) @@ -3248,23 +3277,33 @@ def deactivate_connector_profile( raise _http_error(exc, not_found=True) from exc _require_connector_profile_write(principal, row.scope_type) try: - deactivate_connector_profile_row(session, row, user_id=principal.user.id) - _record_connector_settings_change( + changed = delete_connector_profile_row( session, - collection=FILES_CONNECTOR_PROFILES_COLLECTION, - resource_type=FILES_CONNECTOR_PROFILE_RESOURCE, - resource_id=row.id, - operation="deleted", - principal=principal, - tenant_id=row.tenant_id, - payload={"scope_type": row.scope_type, "scope_id": row.scope_id, "provider": row.provider}, + row, + deletion_reason="api_delete", + user_id=principal.user.id, + api_key_id=principal.api_key.id if principal.api_key else None, ) + if changed: + _record_connector_settings_change( + session, + collection=FILES_CONNECTOR_PROFILES_COLLECTION, + resource_type=FILES_CONNECTOR_PROFILE_RESOURCE, + resource_id=row.id, + operation="deleted", + principal=principal, + tenant_id=row.tenant_id, + payload={"scope_type": row.scope_type, "scope_id": row.scope_id, "provider": row.provider}, + ) session.commit() session.refresh(row) return FileConnectorProfileResponse(**connector_profile_from_row(row).to_response()) except FileStorageError as exc: session.rollback() raise _http_error(exc) from exc + except Exception: + session.rollback() + raise @router.get("/{file_id}", response_model=FileAssetResponse) diff --git a/src/govoplan_files/backend/storage/connector_credential_deletion.py b/src/govoplan_files/backend/storage/connector_credential_deletion.py new file mode 100644 index 0000000..bb713df --- /dev/null +++ b/src/govoplan_files/backend/storage/connector_credential_deletion.py @@ -0,0 +1,337 @@ +from __future__ import annotations + +from dataclasses import dataclass + +from sqlalchemy import inspect +from sqlalchemy.orm import Session + +from govoplan_core.audit.logging import audit_event +from govoplan_files.backend.db.models import FileConnectorCredential, FileConnectorProfile + + +@dataclass(frozen=True, slots=True) +class ConnectorCredentialDeletionResult: + changed: bool + affected_profiles: tuple[FileConnectorProfile, ...] = () + + +def delete_connector_credential_row( + session: Session, + row: FileConnectorCredential, + *, + deletion_reason: str, + user_id: str | None = None, + api_key_id: str | None = None, +) -> ConnectorCredentialDeletionResult: + """Scrub a credential tombstone and disable every profile that used it. + + ``secret_ref`` predates a Files-owned secret-provider contract. It may be + shared or deployment-owned, so it is detached locally and explicitly + audited without ever being passed to a provider delete operation. + """ + + dependent_profiles = ( + tuple( + session.query(FileConnectorProfile) + .filter(FileConnectorProfile.credential_profile_id == row.id) + .order_by(FileConnectorProfile.id.asc()) + .all() + ) + if inspect(session.get_bind()).has_table(FileConnectorProfile.__tablename__) + else () + ) + affected_profiles: list[FileConnectorProfile] = [] + for profile in dependent_profiles: + if _delete_connector_profile_row( + session, + profile, + deletion_reason="credential_deleted", + user_id=user_id, + api_key_id=api_key_id, + ): + affected_profiles.append(profile) + + deleted_secret_kinds = _encrypted_secret_kinds(row) + removed_reference_kinds = _credential_reference_kinds(row) + removed_metadata = bool(row.metadata_) + changed = _credential_row_requires_deletion(row) + if changed: + _scrub_credential_row(row, user_id=user_id) + session.add(row) + _audit_credential_deletion( + session, + row, + deletion_reason=deletion_reason, + deleted_secret_kinds=deleted_secret_kinds, + removed_reference_kinds=removed_reference_kinds, + removed_metadata=removed_metadata, + affected_profile_count=len(affected_profiles), + user_id=user_id, + api_key_id=api_key_id, + ) + session.flush() + return ConnectorCredentialDeletionResult( + changed=changed or bool(affected_profiles), + affected_profiles=tuple(affected_profiles), + ) + + +def delete_connector_profile_row( + session: Session, + row: FileConnectorProfile, + *, + deletion_reason: str, + user_id: str | None = None, + api_key_id: str | None = None, +) -> bool: + changed = _delete_connector_profile_row( + session, + row, + deletion_reason=deletion_reason, + user_id=user_id, + api_key_id=api_key_id, + ) + session.flush() + return changed + + +def delete_connector_credentials_for_retirement(session: Session) -> int: + """Scrub and audit stored connector material before destructive retirement. + + Legacy external references are detached and audited as non-owned. Files + never sends those arbitrary references to a provider delete operation. + """ + + inspector = inspect(session.get_bind()) + profiles = ( + session.query(FileConnectorProfile).order_by(FileConnectorProfile.id.asc()).all() + if inspector.has_table(FileConnectorProfile.__tablename__) + else [] + ) + credentials = ( + session.query(FileConnectorCredential).order_by(FileConnectorCredential.id.asc()).all() + if inspector.has_table(FileConnectorCredential.__tablename__) + else [] + ) + deleted = 0 + for profile in profiles: + if not _profile_row_has_credential_material(profile): + continue + if _delete_connector_profile_row( + session, + profile, + deletion_reason="module_data_retired", + ): + deleted += 1 + for credential in credentials: + if not _credential_row_has_material(credential): + continue + result = delete_connector_credential_row( + session, + credential, + deletion_reason="module_data_retired", + ) + if result.changed: + deleted += 1 + session.flush() + return deleted + + +def _delete_connector_profile_row( + session: Session, + row: FileConnectorProfile, + *, + deletion_reason: str, + user_id: str | None = None, + api_key_id: str | None = None, +) -> bool: + deleted_secret_kinds = _encrypted_secret_kinds(row) + removed_reference_kinds = _profile_reference_kinds(row) + removed_metadata = bool(row.metadata_) + changed = _profile_row_requires_deletion(row) + if not changed: + return False + _scrub_profile_row(row, user_id=user_id) + session.add(row) + _audit_profile_deletion( + session, + row, + deletion_reason=deletion_reason, + deleted_secret_kinds=deleted_secret_kinds, + removed_reference_kinds=removed_reference_kinds, + removed_metadata=removed_metadata, + user_id=user_id, + api_key_id=api_key_id, + ) + return True + + +def _scrub_credential_row(row: FileConnectorCredential, *, user_id: str | None) -> None: + row.enabled = False + row.credential_mode = "none" + row.username = None + row.password_encrypted = None + row.token_encrypted = None + row.password_env = None + row.token_env = None + row.secret_ref = None + row.metadata_ = {} + row.updated_by_user_id = user_id + + +def _scrub_profile_row(row: FileConnectorProfile, *, user_id: str | None) -> None: + row.enabled = False + row.credential_profile_id = None + row.credential_mode = "none" + row.username = None + row.password_encrypted = None + row.token_encrypted = None + row.password_env = None + row.token_env = None + row.secret_ref = None + row.metadata_ = {} + row.updated_by_user_id = user_id + + +def _credential_row_requires_deletion(row: FileConnectorCredential) -> bool: + return bool(row.enabled or _credential_row_has_material(row)) + + +def _profile_row_requires_deletion(row: FileConnectorProfile) -> bool: + return bool(row.enabled or _profile_row_has_credential_material(row)) + + +def _credential_row_has_material(row: FileConnectorCredential) -> bool: + return bool( + row.username + or row.password_encrypted + or row.token_encrypted + or row.password_env + or row.token_env + or row.secret_ref + or row.metadata_ + or row.credential_mode not in {"", "none", "anonymous"} + ) + + +def _profile_row_has_credential_material(row: FileConnectorProfile) -> bool: + return bool( + row.credential_profile_id + or row.username + or row.password_encrypted + or row.token_encrypted + or row.password_env + or row.token_env + or row.secret_ref + or row.metadata_ + or row.credential_mode not in {"", "none", "anonymous"} + ) + + +def _encrypted_secret_kinds(row: FileConnectorCredential | FileConnectorProfile) -> list[str]: + kinds: list[str] = [] + if row.password_encrypted: + kinds.append("password") + if row.token_encrypted: + kinds.append("token") + return kinds + + +def _credential_reference_kinds(row: FileConnectorCredential | FileConnectorProfile) -> list[str]: + kinds: list[str] = [] + if row.password_env: + kinds.append("password_env") + if row.token_env: + kinds.append("token_env") + if row.secret_ref: + kinds.append("unowned_external_secret_ref") + return kinds + + +def _profile_reference_kinds(row: FileConnectorProfile) -> list[str]: + kinds = _credential_reference_kinds(row) + if row.credential_profile_id: + kinds.append("credential_profile") + return kinds + + +def _storage_backend(deleted_secret_kinds: list[str], removed_reference_kinds: list[str]) -> str: + if "unowned_external_secret_ref" in removed_reference_kinds: + return "unowned_external_reference_detached" + if deleted_secret_kinds: + return "encrypted_database" + if removed_reference_kinds: + return "reference_only" + return "none" + + +def _audit_credential_deletion( + session: Session, + row: FileConnectorCredential, + *, + deletion_reason: str, + deleted_secret_kinds: list[str], + removed_reference_kinds: list[str], + removed_metadata: bool, + affected_profile_count: int, + user_id: str | None, + api_key_id: str | None, +) -> None: + audit_event( + session, + tenant_id=row.tenant_id, + scope=_audit_scope(row.scope_type), + user_id=user_id, + api_key_id=api_key_id, + action="files.connector_credential_deleted", + object_type="file_connector_credential", + object_id=row.id, + details={ + "scope_type": row.scope_type, + "scope_id": row.scope_id, + "provider": row.provider, + "storage_backend": _storage_backend(deleted_secret_kinds, removed_reference_kinds), + "deleted_secret_kinds": deleted_secret_kinds, + "removed_reference_kinds": removed_reference_kinds, + "removed_metadata": removed_metadata, + "affected_profile_count": affected_profile_count, + "deletion_reason": deletion_reason, + }, + ) + + +def _audit_profile_deletion( + session: Session, + row: FileConnectorProfile, + *, + deletion_reason: str, + deleted_secret_kinds: list[str], + removed_reference_kinds: list[str], + removed_metadata: bool, + user_id: str | None, + api_key_id: str | None, +) -> None: + audit_event( + session, + tenant_id=row.tenant_id, + scope=_audit_scope(row.scope_type), + user_id=user_id, + api_key_id=api_key_id, + action="files.connector_profile_deleted", + object_type="file_connector_profile", + object_id=row.id, + details={ + "scope_type": row.scope_type, + "scope_id": row.scope_id, + "provider": row.provider, + "storage_backend": _storage_backend(deleted_secret_kinds, removed_reference_kinds), + "deleted_secret_kinds": deleted_secret_kinds, + "removed_reference_kinds": removed_reference_kinds, + "removed_metadata": removed_metadata, + "deletion_reason": deletion_reason, + }, + ) + + +def _audit_scope(scope_type: str) -> str: + return "system" if scope_type == "system" else "tenant" diff --git a/src/govoplan_files/backend/storage/connector_credential_store.py b/src/govoplan_files/backend/storage/connector_credential_store.py index db8e1b1..76c2544 100644 --- a/src/govoplan_files/backend/storage/connector_credential_store.py +++ b/src/govoplan_files/backend/storage/connector_credential_store.py @@ -52,6 +52,8 @@ class ConnectorCredential: @property def credentials_configured(self) -> bool: + if not self.enabled: + return False if self.credential_mode.casefold() in {"", "none", "anonymous"}: return True # Environment references are deliberately unavailable to API-managed @@ -188,6 +190,7 @@ def create_connector_credential_row( reject_api_controlled_deployment_references( password_env=password_env, token_env=token_env, + secret_ref=secret_ref, metadata=metadata, ) clean_id = _normalize_id(credential_id) @@ -242,8 +245,15 @@ def update_connector_credential_row( reject_api_controlled_deployment_references( password_env=password_env, token_env=token_env, + secret_ref=secret_ref, metadata=metadata, ) + if secret_ref is not None and _clean(secret_ref) != _clean(row.secret_ref): + if _clean(row.secret_ref): + raise FileStorageError( + "An existing external secret reference cannot be replaced or cleared until Files can prove " + "provider ownership and confirm provider-side deletion" + ) if label is not None: row.label = _normalize_label(label) if provider is not None: @@ -278,14 +288,6 @@ def update_connector_credential_row( return row -def deactivate_connector_credential_row(session: Session, row: FileConnectorCredential, *, user_id: str | None) -> FileConnectorCredential: - row.enabled = False - row.updated_by_user_id = user_id - session.add(row) - session.flush() - return row - - def _normalize_scope(*, tenant_id: str, scope_type: str, scope_id: str | None) -> tuple[str, str | None, str | None]: clean_scope_type = normalize_policy_scope_type(scope_type) clean_scope_id = _clean(scope_id) diff --git a/src/govoplan_files/backend/storage/connector_deployment.py b/src/govoplan_files/backend/storage/connector_deployment.py index b65b00d..8e23a4c 100644 --- a/src/govoplan_files/backend/storage/connector_deployment.py +++ b/src/govoplan_files/backend/storage/connector_deployment.py @@ -17,6 +17,22 @@ _SECRET_ENV_METADATA_KEYS = frozenset( "session_token_env", } ) +_SECRET_VALUE_METADATA_KEYS = frozenset( + { + "password", + "token", + "access_token", + "auth_token", + "bearer_token", + "refresh_token", + "api_key", + "access_key", + "access_key_id", + "secret_key", + "secret_access_key", + "session_token", + } +) _DEVELOPMENT_ENVIRONMENTS = frozenset({"dev", "development", "local", "test", "testing"}) @@ -65,6 +81,7 @@ def reject_api_controlled_deployment_references( *, password_env: str | None = None, token_env: str | None = None, + secret_ref: str | None = None, metadata: Mapping[str, Any] | None = None, ) -> None: """Reject process-secret selectors controlled through tenant-facing APIs.""" @@ -73,14 +90,39 @@ def reject_api_controlled_deployment_references( raise ConnectorDeploymentConfigurationError( "Environment-backed credentials may only be declared in deployment-owned connector configuration" ) - for key, value in (metadata or {}).items(): - if _clean(value) and (str(key).strip().casefold() in _SECRET_ENV_METADATA_KEYS or str(key).strip().casefold().endswith("_env")): + if _clean(secret_ref): + raise ConnectorDeploymentConfigurationError( + "External secret references cannot be managed through the Files API until Files can prove " + "provider ownership and confirm provider-side deletion" + ) + for key, value in _metadata_entries(metadata or {}): + clean_key = str(key).strip().casefold() + if _clean(value) and (clean_key in _SECRET_ENV_METADATA_KEYS or clean_key.endswith("_env")): raise ConnectorDeploymentConfigurationError( "Environment-backed credentials may only be declared in deployment-owned connector configuration" ) + if _clean(value) and ( + clean_key in _SECRET_VALUE_METADATA_KEYS + or clean_key.endswith(("_password", "_secret", "_api_key")) + ): + raise ConnectorDeploymentConfigurationError( + "Connector credential values must use the dedicated encrypted credential fields, not metadata" + ) validate_connector_tls_metadata(metadata) +def _metadata_entries(value: object) -> list[tuple[str, object]]: + entries: list[tuple[str, object]] = [] + if isinstance(value, Mapping): + for key, item in value.items(): + entries.append((str(key), item)) + entries.extend(_metadata_entries(item)) + elif isinstance(value, (list, tuple)): + for item in value: + entries.extend(_metadata_entries(item)) + return entries + + def connector_ca_bundle_path(value: str) -> str: clean_value = _clean(value) if not clean_value: diff --git a/src/govoplan_files/backend/storage/connector_profile_store.py b/src/govoplan_files/backend/storage/connector_profile_store.py index 3d4d1fb..054dbfc 100644 --- a/src/govoplan_files/backend/storage/connector_profile_store.py +++ b/src/govoplan_files/backend/storage/connector_profile_store.py @@ -128,6 +128,7 @@ def create_connector_profile_row( reject_api_controlled_deployment_references( password_env=password_env, token_env=token_env, + secret_ref=secret_ref, metadata=metadata, ) clean_id = _normalize_profile_id(profile_id) @@ -190,8 +191,15 @@ def update_connector_profile_row( reject_api_controlled_deployment_references( password_env=password_env, token_env=token_env, + secret_ref=secret_ref, metadata=metadata, ) + if secret_ref is not None and _clean(secret_ref) != _clean(row.secret_ref): + if _clean(row.secret_ref): + raise FileStorageError( + "An existing external secret reference cannot be replaced or cleared until Files can prove " + "provider ownership and confirm provider-side deletion" + ) if label is not None: row.label = _normalize_label(label) if provider is not None: @@ -234,14 +242,6 @@ def update_connector_profile_row( return row -def deactivate_connector_profile_row(session: Session, row: FileConnectorProfile, *, user_id: str | None) -> FileConnectorProfile: - row.enabled = False - row.updated_by_user_id = user_id - session.add(row) - session.flush() - return row - - def _normalize_scope(*, tenant_id: str, scope_type: str, scope_id: str | None) -> tuple[str, str | None, str | None]: clean_scope_type = normalize_policy_scope_type(scope_type) clean_scope_id = _clean(scope_id) diff --git a/src/govoplan_files/backend/storage/connector_profiles.py b/src/govoplan_files/backend/storage/connector_profiles.py index 440f68d..21ef84f 100644 --- a/src/govoplan_files/backend/storage/connector_profiles.py +++ b/src/govoplan_files/backend/storage/connector_profiles.py @@ -80,6 +80,8 @@ class ConnectorProfile: @property def credentials_configured(self) -> bool: + if not self.enabled: + return False mode = self.credential_mode.casefold() if mode in {"", "none", "anonymous"}: return True diff --git a/tests/test_connector_credential_deletion.py b/tests/test_connector_credential_deletion.py new file mode 100644 index 0000000..9d6edf5 --- /dev/null +++ b/tests/test_connector_credential_deletion.py @@ -0,0 +1,272 @@ +from __future__ import annotations + +import unittest +from types import SimpleNamespace +from unittest.mock import patch + +from sqlalchemy import create_engine, inspect +from sqlalchemy.orm import sessionmaker + +from govoplan_access.backend.db import models as access_models # noqa: F401 - resolve Files user foreign keys +from govoplan_core.core.change_sequence import ChangeSequenceEntry +from govoplan_core.db.base import Base +from govoplan_core.security.secrets import encrypt_secret +from govoplan_files.backend.db.models import FileConnectorCredential, FileConnectorProfile +from govoplan_files.backend.router import deactivate_connector_credential, deactivate_connector_profile + + +class Principal: + tenant_id = "tenant-1" + user = SimpleNamespace(id="user-1") + api_key = None + + def has(self, scope: str) -> bool: + return scope == "files:file:admin" + + +class ConnectorCredentialDeletionTests(unittest.TestCase): + def setUp(self) -> None: + self.engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all( + self.engine, + tables=[ + FileConnectorCredential.__table__, + FileConnectorProfile.__table__, + ChangeSequenceEntry.__table__, + ], + ) + self.session = sessionmaker(bind=self.engine)() + self.principal = Principal() + + def tearDown(self) -> None: + self.session.close() + Base.metadata.drop_all(bind=self.engine) + self.engine.dispose() + + def test_delete_credential_scrubs_material_audits_and_disables_dependents(self) -> None: + credential = self._credential() + profile = self._profile(credential_profile_id=credential.id) + self.session.add_all([credential, profile]) + self.session.commit() + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event" + ) as audit: + response = deactivate_connector_credential( + credential.id, + session=self.session, + principal=self.principal, # type: ignore[arg-type] + ) + + self.assertFalse(response.enabled) + self.assertFalse(response.credentials_configured) + self.assertIsNone(response.credential_secret_source) + self.session.refresh(credential) + self.session.refresh(profile) + self._assert_credential_scrubbed(credential) + self._assert_profile_scrubbed(profile) + + calls = {call.kwargs["action"]: call.kwargs for call in audit.call_args_list} + self.assertEqual( + {"files.connector_profile_deleted", "files.connector_credential_deleted"}, + set(calls), + ) + credential_audit = calls["files.connector_credential_deleted"] + self.assertEqual("encrypted_database", credential_audit["details"]["storage_backend"]) + self.assertEqual(["password", "token"], credential_audit["details"]["deleted_secret_kinds"]) + self.assertEqual(1, credential_audit["details"]["affected_profile_count"]) + self.assertNotIn("do-not-audit", repr(audit.call_args_list)) + + changes = self.session.query(ChangeSequenceEntry).order_by(ChangeSequenceEntry.id.asc()).all() + self.assertEqual([credential.id, profile.id], [change.resource_id for change in changes]) + self.assertEqual(["deleted", "updated"], [change.operation for change in changes]) + + def test_delete_profile_scrubs_inline_credentials_and_audits(self) -> None: + profile = self._profile(credential_profile_id="shared-credential") + self.session.add(profile) + self.session.commit() + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event" + ) as audit: + response = deactivate_connector_profile( + profile.id, + session=self.session, + principal=self.principal, # type: ignore[arg-type] + ) + + self.assertFalse(response.enabled) + self.assertFalse(response.credentials_configured) + self.assertIsNone(response.credential_profile_id) + self.session.refresh(profile) + self._assert_profile_scrubbed(profile) + audit.assert_called_once() + call = audit.call_args.kwargs + self.assertEqual("files.connector_profile_deleted", call["action"]) + self.assertEqual("api_delete", call["details"]["deletion_reason"]) + self.assertIn("credential_profile", call["details"]["removed_reference_kinds"]) + self.assertNotIn("do-not-audit", repr(call)) + + def test_delete_rolls_back_when_audit_fails(self) -> None: + credential = self._credential() + self.session.add(credential) + self.session.commit() + original_password = credential.password_encrypted + original_token = credential.token_encrypted + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event", + side_effect=RuntimeError("audit unavailable"), + ), self.assertRaisesRegex(RuntimeError, "audit unavailable"): + deactivate_connector_credential( + credential.id, + session=self.session, + principal=self.principal, # type: ignore[arg-type] + ) + + persisted = self.session.get(FileConnectorCredential, credential.id) + assert persisted is not None + self.assertTrue(persisted.enabled) + self.assertEqual(original_password, persisted.password_encrypted) + self.assertEqual(original_token, persisted.token_encrypted) + self.assertEqual(0, self.session.query(ChangeSequenceEntry).count()) + + def test_delete_detaches_unowned_legacy_secret_reference_without_claiming_provider_deletion(self) -> None: + credential = self._credential(secret_ref="vault:tenant-1:files:credential") + self.session.add(credential) + self.session.commit() + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event" + ) as audit: + response = deactivate_connector_credential( + credential.id, + session=self.session, + principal=self.principal, # type: ignore[arg-type] + ) + + self.assertFalse(response.enabled) + persisted = self.session.get(FileConnectorCredential, credential.id) + assert persisted is not None + self._assert_credential_scrubbed(persisted) + audit.assert_called_once() + details = audit.call_args.kwargs["details"] + self.assertEqual("unowned_external_reference_detached", details["storage_backend"]) + self.assertIn("unowned_external_secret_ref", details["removed_reference_kinds"]) + self.assertNotIn("vault:tenant-1:files:credential", repr(audit.call_args)) + + def test_retirement_scrubs_and_audits_before_tables_are_dropped(self) -> None: + from govoplan_files.backend.manifest import manifest + + self.session.add_all([self._credential(), self._profile()]) + self.session.commit() + retirement_provider = manifest.migration_spec.retirement_provider + assert retirement_provider is not None + plan = retirement_provider(self.session, "files") + assert plan.destroy_data_executor is not None + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event" + ) as audit: + plan.destroy_data_executor(self.session, "files") + + self.assertEqual(2, audit.call_count) + self.assertEqual( + {"module_data_retired"}, + {call.kwargs["details"]["deletion_reason"] for call in audit.call_args_list}, + ) + self.assertNotIn("do-not-audit", repr(audit.call_args_list)) + self.assertFalse(inspect(self.engine).has_table(FileConnectorCredential.__tablename__)) + self.assertFalse(inspect(self.engine).has_table(FileConnectorProfile.__tablename__)) + + def test_retirement_detaches_unowned_external_reference_before_drop(self) -> None: + from govoplan_files.backend.manifest import manifest + + self.session.add(self._credential(secret_ref="vault:tenant-1:files:credential")) + self.session.commit() + retirement_provider = manifest.migration_spec.retirement_provider + assert retirement_provider is not None + plan = retirement_provider(self.session, "files") + assert plan.destroy_data_executor is not None + + with patch( + "govoplan_files.backend.storage.connector_credential_deletion.audit_event" + ) as audit: + plan.destroy_data_executor(self.session, "files") + + audit.assert_called_once() + details = audit.call_args.kwargs["details"] + self.assertIn("unowned_external_secret_ref", details["removed_reference_kinds"]) + self.assertNotIn("vault:tenant-1:files:credential", repr(audit.call_args)) + self.assertFalse(inspect(self.engine).has_table(FileConnectorCredential.__tablename__)) + self.assertFalse(inspect(self.engine).has_table(FileConnectorProfile.__tablename__)) + + @staticmethod + def _credential(*, secret_ref: str | None = None) -> FileConnectorCredential: + return FileConnectorCredential( + id="credential-1", + tenant_id="tenant-1", + scope_type="tenant", + scope_id="tenant-1", + label="Credential", + provider="webdav", + enabled=True, + credential_mode="basic", + username="connector-user", + password_encrypted=encrypt_secret("do-not-audit-password"), + token_encrypted=encrypt_secret("do-not-audit-token"), + password_env="DEPLOYMENT_PASSWORD", + token_env="DEPLOYMENT_TOKEN", + secret_ref=secret_ref, + policy={}, + metadata_={"private_hint": "do-not-audit-metadata"}, + ) + + @staticmethod + def _profile(*, credential_profile_id: str | None = None) -> FileConnectorProfile: + return FileConnectorProfile( + id="profile-1", + tenant_id="tenant-1", + scope_type="tenant", + scope_id="tenant-1", + label="Profile", + provider="webdav", + endpoint_url="https://dav.example.test", + enabled=True, + credential_profile_id=credential_profile_id, + credential_mode="basic", + username="connector-user", + password_encrypted=encrypt_secret("do-not-audit-profile-password"), + token_encrypted=encrypt_secret("do-not-audit-profile-token"), + password_env="DEPLOYMENT_PROFILE_PASSWORD", + token_env="DEPLOYMENT_PROFILE_TOKEN", + policy={}, + metadata_={"private_hint": "do-not-audit-profile-metadata"}, + ) + + def _assert_credential_scrubbed(self, row: FileConnectorCredential) -> None: + self.assertFalse(row.enabled) + self.assertEqual("none", row.credential_mode) + self.assertIsNone(row.username) + self.assertIsNone(row.password_encrypted) + self.assertIsNone(row.token_encrypted) + self.assertIsNone(row.password_env) + self.assertIsNone(row.token_env) + self.assertIsNone(row.secret_ref) + self.assertEqual({}, row.metadata_) + + def _assert_profile_scrubbed(self, row: FileConnectorProfile) -> None: + self.assertFalse(row.enabled) + self.assertIsNone(row.credential_profile_id) + self.assertEqual("none", row.credential_mode) + self.assertIsNone(row.username) + self.assertIsNone(row.password_encrypted) + self.assertIsNone(row.token_encrypted) + self.assertIsNone(row.password_env) + self.assertIsNone(row.token_env) + self.assertIsNone(row.secret_ref) + self.assertEqual({}, row.metadata_) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_connector_deployment.py b/tests/test_connector_deployment.py index df41159..58483a7 100644 --- a/tests/test_connector_deployment.py +++ b/tests/test_connector_deployment.py @@ -81,6 +81,37 @@ class ConnectorDeploymentBoundaryTests(unittest.TestCase): ) session.get.assert_not_called() + def test_api_profiles_cannot_create_unowned_external_secret_references(self) -> None: + with self.assertRaisesRegex(ConnectorDeploymentConfigurationError, "provider ownership"): + reject_api_controlled_deployment_references(secret_ref="vault:tenant-1:files") + + session = MagicMock() + with self.assertRaisesRegex(ConnectorDeploymentConfigurationError, "provider ownership"): + create_connector_profile_row( + session, + tenant_id="tenant-1", + user_id="user-1", + profile_id="unsafe-secret-ref", + label="Unsafe secret reference", + provider="webdav", + scope_type="tenant", + credential_mode="secret_ref", + secret_ref="vault:tenant-1:files", + ) + session.get.assert_not_called() + + def test_api_profiles_cannot_hide_plaintext_credentials_in_metadata(self) -> None: + for metadata in ( + {"secret_access_key": "plaintext-secret"}, + {"credentials": {"password": "plaintext-secret"}}, + {"provider": {"refresh_token": "plaintext-secret"}}, + ): + with self.subTest(metadata=metadata), self.assertRaisesRegex( + ConnectorDeploymentConfigurationError, + "dedicated encrypted credential fields", + ): + reject_api_controlled_deployment_references(metadata=metadata) + def test_profile_responses_hide_environment_and_local_ca_references(self) -> None: profile = ConnectorProfile( id="deployment-s3", diff --git a/webui/package.json b/webui/package.json index a79bc53..37bd0a8 100644 --- a/webui/package.json +++ b/webui/package.json @@ -1,6 +1,6 @@ { "name": "@govoplan/files-webui", - "version": "0.1.8", + "version": "0.1.9", "private": true, "type": "module", "main": "src/index.ts",