From 6c8d6994bbf43d644814468ed5ef5c092a4d360c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Mart=C3=ADn?= Date: Fri, 18 Sep 2026 10:16:13 +0200 Subject: [PATCH] fix(api): sign report URLs against public storage host (#12552) --- .env | 7 + ...-download-public-storage-endpoint.fixed.md | 1 + .../s3-client-default-region.fixed.md | 1 + api/src/backend/api/tests/test_views.py | 48 ++++++- api/src/backend/api/v1/views.py | 5 +- api/src/backend/config/django/base.py | 5 + api/src/backend/tasks/jobs/export.py | 43 ++++++- api/src/backend/tasks/tests/test_export.py | 120 ++++++++++++++++++ 8 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 api/changelog.d/report-download-public-storage-endpoint.fixed.md create mode 100644 api/changelog.d/s3-client-default-region.fixed.md diff --git a/.env b/.env index 721d25f4bf..136531ec2d 100644 --- a/.env +++ b/.env @@ -115,6 +115,13 @@ DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION="" # The name of the S3 bucket where scan output should be stored DJANGO_OUTPUT_S3_AWS_OUTPUT_BUCKET="" +# The storage address the browser can reach, used only to sign report download URLs +# (e.g. "https://storage.example.com"). Leave empty on AWS S3. Set it when storage is +# only reachable inside the container network, such as MinIO on "http://minio:9000". +# The reverse proxy in front of it must forward the Host header unchanged: SigV4 signs +# Host, so rewriting it to the internal name invalidates the signature. +DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="" + # Django settings DJANGO_ALLOWED_HOSTS=localhost,127.0.0.1,prowler-api DJANGO_BIND_ADDRESS=0.0.0.0 diff --git a/api/changelog.d/report-download-public-storage-endpoint.fixed.md b/api/changelog.d/report-download-public-storage-endpoint.fixed.md new file mode 100644 index 0000000000..87004d8a56 --- /dev/null +++ b/api/changelog.d/report-download-public-storage-endpoint.fixed.md @@ -0,0 +1 @@ +Report download URLs can be signed against a browser-reachable storage host via `DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL`, so downloads complete on deployments where storage is only reachable inside the container network diff --git a/api/changelog.d/s3-client-default-region.fixed.md b/api/changelog.d/s3-client-default-region.fixed.md new file mode 100644 index 0000000000..62efc579f2 --- /dev/null +++ b/api/changelog.d/s3-client-default-region.fixed.md @@ -0,0 +1 @@ +A scan report download no longer fails with a server error when `DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION` is unset, which is common on storage with no meaningful region diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 37b4a87729..43a13e4d4c 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -82,7 +82,7 @@ from django.db import close_old_connections, connection, connections from django.db.models import Count from django.db.models.signals import pre_delete from django.http import JsonResponse -from django.test import RequestFactory +from django.test import RequestFactory, override_settings from django.test.utils import CaptureQueriesContext from django.urls import reverse from django_celery_results.models import TaskResult @@ -4540,6 +4540,52 @@ class TestScanViewSet: assert response.status_code == status.HTTP_302_FOUND assert response["Location"] == presigned_url + @override_settings( + DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL="https://storage.example.com", + 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 test_report_s3_redirects_to_the_public_storage_host( + self, authenticated_client, scans_fixture, monkeypatch + ): + """The object is looked up internally but the redirect the browser follows is public.""" + scan = scans_fixture[0] + bucket = "test-bucket" + key = "report.zip" + scan.output_location = f"s3://{bucket}/{key}" + scan.state = StateChoices.COMPLETED + scan.save() + + monkeypatch.setattr( + "api.v1.views.env", + type("env", (), {"str": lambda self, *_args, **_kwargs: bucket})(), + ) + + head_calls = [] + + class InternalS3Client: + def head_object(self, Bucket, Key): + head_calls.append((Bucket, Key)) + return {} + + def generate_presigned_url(self, *_args, **_kwargs): + raise AssertionError("the internal client must not sign the redirect") + + monkeypatch.setattr("api.v1.views.get_s3_client", lambda: InternalS3Client()) + + url = reverse("scan-report", kwargs={"pk": scan.id}) + response = authenticated_client.get(url) + + assert response.status_code == status.HTTP_302_FOUND + assert head_calls == [(bucket, key)] + + location = urlparse(response["Location"]) + assert location.netloc == "storage.example.com" + assert location.path == f"/{bucket}/{key}" + assert "X-Amz-Signature" in parse_qs(location.query) + def test_report_s3_success_no_local_files( self, authenticated_client, scans_fixture, monkeypatch ): diff --git a/api/src/backend/api/v1/views.py b/api/src/backend/api/v1/views.py index d0294f4abd..1bb40f4fd3 100644 --- a/api/src/backend/api/v1/views.py +++ b/api/src/backend/api/v1/views.py @@ -326,7 +326,7 @@ from rest_framework_simplejwt.token_blacklist.models import ( ) from tasks.beat import schedule_provider_scan from tasks.jobs.attack_paths import db_utils as attack_paths_db_utils -from tasks.jobs.export import get_s3_client +from tasks.jobs.export import get_s3_client, get_s3_presign_client from tasks.tasks import ( QUEUED_SCAN_TASK_STATE, backfill_compliance_summaries_task, @@ -2407,7 +2407,8 @@ class ScanViewSet(ProviderVisibilityMixin, BaseRLSViewSet): } if content_type: params["ResponseContentType"] = content_type - url = client.generate_presigned_url( + # The browser follows this URL, so it is signed against the public host. + url = (get_s3_presign_client() or client).generate_presigned_url( "get_object", Params=params, ExpiresIn=300, diff --git a/api/src/backend/config/django/base.py b/api/src/backend/config/django/base.py index b967110c10..a208dda915 100644 --- a/api/src/backend/config/django/base.py +++ b/api/src/backend/config/django/base.py @@ -295,6 +295,11 @@ DJANGO_OUTPUT_S3_AWS_SECRET_ACCESS_KEY = env.str( ) DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN = env.str("DJANGO_OUTPUT_S3_AWS_SESSION_TOKEN", "") DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION = env.str("DJANGO_OUTPUT_S3_AWS_DEFAULT_REGION", "") +# Browser-reachable storage host used to sign download URLs. Empty means sign against the +# same endpoint the API talks to, which is what Prowler Cloud on S3 does. +DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL = env.str( + "DJANGO_OUTPUT_S3_AWS_PUBLIC_ENDPOINT_URL", "" +) # HTTP Security Headers SECURE_CONTENT_TYPE_NOSNIFF = True diff --git a/api/src/backend/tasks/jobs/export.py b/api/src/backend/tasks/jobs/export.py index e658d6018c..aacb22578a 100644 --- a/api/src/backend/tasks/jobs/export.py +++ b/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: diff --git a/api/src/backend/tasks/tests/test_export.py b/api/src/backend/tasks/tests/test_export.py index 416361d95e..bbec0b742e 100644 --- a/api/src/backend/tasks/tests/test_export.py +++ b/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/")