From 72acff3d5f839ed85b9ab9e201a1cc9b1d102500 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 31 Aug 2021 17:28:46 -0400 Subject: [PATCH 1/4] feat(symbol-sources): Log only redacted secrets in the audit log --- .../project_app_store_connect_credentials.py | 9 ++++++--- src/sentry/api/endpoints/project_details.py | 10 +++++++++- src/sentry/api/serializers/models/project.py | 12 ++++++++---- src/sentry/lang/native/symbolicator.py | 15 ++++++--------- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/sentry/api/endpoints/project_app_store_connect_credentials.py b/src/sentry/api/endpoints/project_app_store_connect_credentials.py index 8e8ea94a53bc53..1f439a08949523 100644 --- a/src/sentry/api/endpoints/project_app_store_connect_credentials.py +++ b/src/sentry/api/endpoints/project_app_store_connect_credentials.py @@ -70,7 +70,7 @@ ) from sentry.api.fields.secret import SecretField, validate_secret from sentry.lang.native import appconnect -from sentry.lang.native.symbolicator import secret_fields +from sentry.lang.native.symbolicator import redact_source_secrets, secret_fields from sentry.models import AppConnectBuild, AuditLogEntryEvent, LatestAppConnectBuildsCheck, Project from sentry.tasks.app_store_connect import dsym_download from sentry.utils import json @@ -287,12 +287,14 @@ def post(self, request: Request, project: Project) -> Response: new_sources = validated_config.update_project_symbol_source(project, allow_multiple) except ValueError: raise AppConnectMultipleSourcesError + + redacted_sources = redact_source_secrets(new_sources) self.create_audit_entry( request=request, organization=project.organization, target_object=project.id, event=AuditLogEntryEvent.PROJECT_EDIT, - data={appconnect.SYMBOL_SOURCES_PROP_NAME: new_sources}, + data={appconnect.SYMBOL_SOURCES_PROP_NAME: redacted_sources}, ) dsym_download.apply_async( @@ -403,12 +405,13 @@ def post(self, request: Request, project: Project, credentials_id: str) -> Respo project, allow_multiple=True ) + redacted_sources = redact_source_secrets(new_sources) self.create_audit_entry( request=request, organization=project.organization, target_object=project.id, event=AuditLogEntryEvent.PROJECT_EDIT, - data={appconnect.SYMBOL_SOURCES_PROP_NAME: new_sources}, + data={appconnect.SYMBOL_SOURCES_PROP_NAME: redacted_sources}, ) dsym_download.apply_async( diff --git a/src/sentry/api/endpoints/project_details.py b/src/sentry/api/endpoints/project_details.py index 6fc260eca92b03..39510f84246d75 100644 --- a/src/sentry/api/endpoints/project_details.py +++ b/src/sentry/api/endpoints/project_details.py @@ -26,6 +26,7 @@ InvalidSourcesError, parse_backfill_sources, parse_sources, + redact_source_secrets, ) from sentry.lang.native.utils import convert_crashreport_count from sentry.models import ( @@ -559,7 +560,14 @@ def put(self, request, project): ] if result.get("symbolSources") is not None: if project.update_option("sentry:symbol_sources", result["symbolSources"]): - changed_proj_settings["sentry:symbol_sources"] = result["symbolSources"] or None + # Redact secrets so they don't get logged directly to the Audit Log + sources_json = result["symbolSources"] or None + try: + sources = parse_sources(sources_json) + except Exception: + sources = [] + redacted_sources = redact_source_secrets(sources) + changed_proj_settings["sentry:symbol_sources"] = redacted_sources if "defaultEnvironment" in result: if result["defaultEnvironment"] is None: project.delete_option("sentry:default_environment") diff --git a/src/sentry/api/serializers/models/project.py b/src/sentry/api/serializers/models/project.py index 913bdf8dec35d0..acbade39b6eed4 100644 --- a/src/sentry/api/serializers/models/project.py +++ b/src/sentry/api/serializers/models/project.py @@ -18,7 +18,7 @@ from sentry.eventstore.models import DEFAULT_SUBJECT_TEMPLATE from sentry.features.base import ProjectFeature from sentry.ingest.inbound_filters import FilterTypes -from sentry.lang.native.symbolicator import redact_source_secrets +from sentry.lang.native.symbolicator import parse_sources, redact_source_secrets from sentry.lang.native.utils import convert_crashreport_count from sentry.models import ( EnvironmentProject, @@ -41,6 +41,7 @@ from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes from sentry.snuba import discover from sentry.snuba.sessions import check_has_health_data, get_current_and_previous_crash_free_rates +from sentry.utils import json from sentry.utils.compat import zip STATUS_LABELS = { @@ -797,16 +798,19 @@ def get_value_with_default(key): ) custom_symbol_sources_json = attrs["options"].get("sentry:symbol_sources") try: - symbol_sources = redact_source_secrets(custom_symbol_sources_json) + sources = parse_sources(custom_symbol_sources_json, False) except Exception: # In theory sources stored on the project should be valid. If they are invalid, we don't # want to abort serialization just for sources, so just return an empty list instead of # returning sources with their secrets included. - symbol_sources = "[]" + serialized_sources = "[]" + else: + redacted_sources = redact_source_secrets(sources) + serialized_sources = json.dumps(redacted_sources) data.update( { - "symbolSources": symbol_sources, + "symbolSources": serialized_sources, } ) diff --git a/src/sentry/lang/native/symbolicator.py b/src/sentry/lang/native/symbolicator.py index 4ca48b04606bec..344dafdb5124df 100644 --- a/src/sentry/lang/native/symbolicator.py +++ b/src/sentry/lang/native/symbolicator.py @@ -4,6 +4,7 @@ import sys import time import uuid +from copy import deepcopy from urllib.parse import urljoin import jsonschema @@ -389,20 +390,16 @@ def parse_backfill_sources(sources_json, original_sources): return sources -def redact_source_secrets(config_sources): - """ - Returns a json string with all of the secrets redacted from every source. - - May raise InvalidSourcesError if the provided sources are invalid. - """ +def redact_source_secrets(config_sources: json.JSONData) -> json.JSONData: + """Returns a JSONData with all of the secrets redacted from every source.""" - sources = parse_sources(config_sources, False) - for source in sources: + redacted_sources = deepcopy(config_sources) + for source in redacted_sources: for secret in secret_fields(source["type"]): if secret in source: source[secret] = {"hidden-secret": True} - return json.dumps(sources) + return redacted_sources def get_options_for_project(project): From 14f25762b41972f51eac91e95f7c143fdd48dff7 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 31 Aug 2021 20:04:05 -0400 Subject: [PATCH 2/4] i did my best but i can't seem to make this work --- tests/sentry/api/endpoints/test_project_details.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index 32a138d71feeee..70b5e882b210ac 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -27,7 +27,7 @@ ScheduledDeletion, ) from sentry.testutils import APITestCase -from sentry.testutils.helpers import Feature +from sentry.testutils.helpers import Feature, faux from sentry.types.integrations import ExternalProviders from sentry.utils import json from sentry.utils.compat import mock, zip @@ -752,7 +752,8 @@ def test_cap_secondary_grouping_expiry(self): expiry = response.data["secondaryGroupingExpiry"] assert (now + 3600 * 24 * 90) < expiry < (now + 3600 * 24 * 92) - def test_redacted_symbol_source_secrets(self): + @mock.patch("sentry.utils.audit.create_audit_entry") + def test_redacted_symbol_source_secrets(self, create_audit_entry): with Feature( {"organizations:symbol-sources": True, "organizations:custom-symbol-sources": True} ): @@ -775,6 +776,14 @@ def test_redacted_symbol_source_secrets(self): # redact password redacted_source["password"] = {"hidden-secret": True} + + # check that audit entry was created with redacted password + assert create_audit_entry.called + call = faux.faux(create_audit_entry) + assert call.kwarg_equals( + "data", {"sentry:symbol_sources": json.dumps([redacted_source])} + ) + self.get_valid_response( self.org_slug, self.proj_slug, symbolSources=json.dumps([redacted_source]) ) From 888054a6ca77b860efb3fd80b6ea3889b6d119c9 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Wed, 1 Sep 2021 10:46:06 +0200 Subject: [PATCH 3/4] fix test - Need to patch the imported function and not where it was imported from - The audit log no longer logs the serialised JSON for symbolSources, but rather logs the JSON directly. --- tests/sentry/api/endpoints/test_project_details.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index 70b5e882b210ac..27620bd8de12dc 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -752,12 +752,12 @@ def test_cap_secondary_grouping_expiry(self): expiry = response.data["secondaryGroupingExpiry"] assert (now + 3600 * 24 * 90) < expiry < (now + 3600 * 24 * 92) - @mock.patch("sentry.utils.audit.create_audit_entry") + @mock.patch("sentry.api.base.create_audit_entry") def test_redacted_symbol_source_secrets(self, create_audit_entry): with Feature( {"organizations:symbol-sources": True, "organizations:custom-symbol-sources": True} ): - redacted_source = { + config = { "id": "honk", "name": "honk source", "layout": { @@ -770,19 +770,18 @@ def test_redacted_symbol_source_secrets(self, create_audit_entry): "password": "beepbeep", } self.get_valid_response( - self.org_slug, self.proj_slug, symbolSources=json.dumps([redacted_source]) + self.org_slug, self.proj_slug, symbolSources=json.dumps([config]) ) - assert self.project.get_option("sentry:symbol_sources") == json.dumps([redacted_source]) + assert self.project.get_option("sentry:symbol_sources") == json.dumps([config]) # redact password + redacted_source = config.copy() redacted_source["password"] = {"hidden-secret": True} # check that audit entry was created with redacted password assert create_audit_entry.called call = faux.faux(create_audit_entry) - assert call.kwarg_equals( - "data", {"sentry:symbol_sources": json.dumps([redacted_source])} - ) + assert call.kwarg_equals("data", {"sentry:symbol_sources": [redacted_source]}) self.get_valid_response( self.org_slug, self.proj_slug, symbolSources=json.dumps([redacted_source]) From 4ae11eef8e7d471a52d4ce7f415a517d42f37313 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 1 Sep 2021 14:27:30 -0400 Subject: [PATCH 4/4] explain that a method doesn't mutate the parameter --- src/sentry/lang/native/symbolicator.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/sentry/lang/native/symbolicator.py b/src/sentry/lang/native/symbolicator.py index 344dafdb5124df..31ed16a7e7cf33 100644 --- a/src/sentry/lang/native/symbolicator.py +++ b/src/sentry/lang/native/symbolicator.py @@ -391,7 +391,12 @@ def parse_backfill_sources(sources_json, original_sources): def redact_source_secrets(config_sources: json.JSONData) -> json.JSONData: - """Returns a JSONData with all of the secrets redacted from every source.""" + """ + Returns a JSONData with all of the secrets redacted from every source. + + The original value is not mutated in the process; A clone is created + and returned by this function. + """ redacted_sources = deepcopy(config_sources) for source in redacted_sources: