fix(api): sign report URLs against public storage host (#12552)

api/src/backend/tasks/jobs/export.py

@@ -6,6 +6,7 @@ import boto3
 import config.django.base as base
 from api.db_utils import rls_transaction
 from api.models import Scan
+from botocore.config import Config
 from botocore.exceptions import ClientError, NoCredentialsError, ParamValidationError
 from celery.utils.log import get_task_logger
 from django.conf import settings
@@ -222,7 +223,9 @@ def get_s3_client():
             aws_access_key_id=settings.DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID,
             aws_secret_access_key=settings.DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY,
             aws_session_token=settings.DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN,
-            region_name=settings.DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION,
+            # Storage that has no meaningful region, MinIO among it, is usually configured
+            # without one, and botocore rejects an empty region before any request is made.
+            region_name=settings.DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION or "us-east-1",
         )
         s3_client.list_buckets()
     except (ClientError, NoCredentialsError, ParamValidationError, ValueError):
@@ -232,6 +235,44 @@ def get_s3_client():
     return s3_client
 
 
+def get_s3_presign_client():
+    """Return a client that signs URLs against the public storage host.
+
+    None means no public host is configured and the caller should presign with its own
+    client, which leaves deployments on real S3 with the URL they get today.
+    """
+    public_endpoint = settings.DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL
+    if not public_endpoint:
+        return None
+
+    # Blank keys are signed as-is (empty credential scope) instead of deferring to the
+    # provider chain, so static credentials are only passed when they are set.
+    credentials = {}
+    if (
+        settings.DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID
+        and settings.DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY
+    ):
+        credentials = {
+            "aws_access_key_id": settings.DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID,
+            "aws_secret_access_key": settings.DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY,
+            # An empty string is a token as far as botocore is concerned: it appends an
+            # empty X-Amz-Security-Token that storage counts when it recomputes the signature.
+            "aws_session_token": settings.DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN or None,
+        }
+
+    return boto3.client(
+        "s3",
+        **credentials,
+        # SigV4 puts the region in the credential scope, and MinIO answers to us-east-1
+        # unless it was told otherwise, so an empty region would sign an unusable URL.
+        region_name=settings.DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION or "us-east-1",
+        endpoint_url=public_endpoint,
+        # The signature covers the host, so the addressing style has to be pinned rather
+        # than guessed from the endpoint: MinIO serves path-style.
+        config=Config(signature_version="s3v4", s3={"addressing_style": "path"}),
+    )
+
+
 def _upload_to_s3(
     tenant_id: str, scan_id: str, local_path: str, relative_key: str
 ) -> str | None:

api/src/backend/tasks/tests/test_export.py

@@ -4,15 +4,18 @@ import zipfile
 from datetime import datetime
 from pathlib import Path
 from unittest.mock import MagicMock, patch
+from urllib.parse import parse_qs, urlparse
 
 import pytest
 from botocore.exceptions import ClientError
+from django.test import override_settings
 from tasks.jobs.export import (
     _compress_output_files,
     _generate_compliance_output_directory,
     _generate_output_directory,
     _upload_to_s3,
     get_s3_client,
+    get_s3_presign_client,
 )
 
 
@@ -47,6 +50,19 @@ class TestOutputs:
         assert client is not None
         client_mock.list_buckets.assert_called()
 
+    @patch("tasks.jobs.export.boto3.client")
+    @override_settings(
+        DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID="access-key",
+        DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY="secret-key",
+        DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN="",
+        DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION="",
+    )
+    def test_get_s3_client_without_a_region_uses_a_default(self, mock_boto_client):
+        """botocore rejects an empty region up front, and the download views do not catch it."""
+        get_s3_client()
+
+        assert mock_boto_client.call_args.kwargs["region_name"] == "us-east-1"
+
     @patch("tasks.jobs.export.boto3.client")
     @patch("tasks.jobs.export.settings")
     def test_get_s3_client_fallback(self, mock_settings, mock_boto_client):
@@ -243,3 +259,107 @@ class TestOutputs:
         assert os.path.isdir(os.path.dirname(ens))
         assert threatscore.endswith(f"aws-test-check-{expected_timestamp}")
         assert ens.endswith(f"aws-test-check-{expected_timestamp}")
+
+
+PRESIGN_SETTINGS = {
+    "DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID": "access-key",
+    "DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY": "secret-key",
+    "DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN": "",
+    "DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION": "eu-west-1",
+}
+
+
+def _presign(client):
+    return client.generate_presigned_url(
+        "get_object",
+        Params={"Bucket": "output-bucket", "Key": "tenant/scan/report.zip"},
+        ExpiresIn=300,
+    )
+
+
+class TestS3PresignClient:
+    @override_settings(**PRESIGN_SETTINGS, DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="")
+    def test_no_public_endpoint_returns_none(self):
+        assert get_s3_presign_client() is None
+
+    @override_settings(
+        **PRESIGN_SETTINGS,
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_url_targets_the_public_host_in_path_style(self):
+        url = urlparse(_presign(get_s3_presign_client()))
+
+        assert url.netloc == "storage.example.com"
+        assert url.path == "/output-bucket/tenant/scan/report.zip"
+
+    @override_settings(
+        **PRESIGN_SETTINGS,
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_signature_covers_the_public_host(self):
+        query = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        assert query["X-Amz-SignedHeaders"] == ["host"]
+        assert "/eu-west-1/s3/aws4_request" in query["X-Amz-Credential"][0]
+
+    @override_settings(
+        **PRESIGN_SETTINGS,
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_signature_is_bound_to_the_host_it_was_signed_against(self):
+        """Rewriting the host afterwards cannot work, which is why the endpoint is a setting."""
+        public = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        with override_settings(
+            DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="http://minio:9000"
+        ):
+            internal = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        assert public["X-Amz-Signature"] != internal["X-Amz-Signature"]
+
+    @override_settings(
+        **{**PRESIGN_SETTINGS, "DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION": ""},
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_region_falls_back_to_the_minio_default(self):
+        query = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        assert "/us-east-1/s3/aws4_request" in query["X-Amz-Credential"][0]
+
+    @override_settings(
+        **PRESIGN_SETTINGS,
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_unset_session_token_is_left_out_of_the_url(self):
+        """An empty token still reaches the URL as a blank param that storage signs over."""
+        url = _presign(get_s3_presign_client())
+        query = parse_qs(urlparse(url).query, keep_blank_values=True)
+
+        assert "X-Amz-Security-Token" not in query
+
+    @override_settings(
+        **{**PRESIGN_SETTINGS, "DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN": "session-token"},
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_session_token_is_forwarded_when_set(self):
+        query = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        assert query["X-Amz-Security-Token"] == ["session-token"]
+
+    @override_settings(
+        **{
+            **PRESIGN_SETTINGS,
+            "DJANGO_OUTPUT_S3_AWS_ACCESS_KEY_ID": "",
+            "DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY": "",
+        },
+        DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com",
+    )
+    def test_blank_static_credentials_defer_to_the_provider_chain(self, monkeypatch):
+        """Empty keys would otherwise be signed as-is, yielding a blank credential scope."""
+        monkeypatch.setenv("AWS_ACCESS_KEY_ID", "chain-key")
+        monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "chain-secret")
+        monkeypatch.delenv("AWS_SESSION_TOKEN", raising=False)
+
+        query = parse_qs(urlparse(_presign(get_s3_presign_client())).query)
+
+        assert query["X-Amz-Credential"][0].startswith("chain-key/")