Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 9 additions & 1 deletion src/sentry/api/endpoints/project_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but isn’t this the main place where project details are saved?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's done on line 562: project.update_option(). This variable is only used for the audit log.

Copy link
Contributor Author

@relaxolotl relaxolotl Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, to expand on what floris has said project option updating in this method has the following pattern:

  • if the request includes an option that needs to be updated
  • try to update the option
  • if the update succeeded (returns true) then record the new value in changed_proj_settings

so in this case what this block tries to do is upon a successful update of the symbol source option, we redact the secrets in the source and stuff them into changed_proj_settings. changed_proj_settings is used later in this method as the payload for the audit log entry.

if "defaultEnvironment" in result:
if result["defaultEnvironment"] is None:
project.delete_option("sentry:default_environment")
Expand Down
12 changes: 8 additions & 4 deletions src/sentry/api/serializers/models/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 = {
Expand Down Expand Up @@ -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,
}
)

Expand Down
14 changes: 8 additions & 6 deletions src/sentry/lang/native/symbolicator.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import sys
import time
import uuid
from copy import deepcopy
from urllib.parse import urljoin

import jsonschema
Expand Down Expand Up @@ -389,20 +390,21 @@ def parse_backfill_sources(sources_json, original_sources):
return sources


def redact_source_secrets(config_sources):
def redact_source_secrets(config_sources: json.JSONData) -> json.JSONData:
"""
Returns a json string with all of the secrets redacted from every source.
Returns a JSONData with all of the secrets redacted from every source.

May raise InvalidSourcesError if the provided sources are invalid.
The original value is not mutated in the process; A clone is created
and returned by this function.
"""

sources = parse_sources(config_sources, False)
for source in sources:
redacted_sources = deepcopy(config_sources)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a copy is enough (there's config_sources.copy() for that these days instead of having to import the module) but this sure makes things easier to reason about and i doubt this is performance-sensitive so this is probably better.

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):
Expand Down
18 changes: 13 additions & 5 deletions tests/sentry/api/endpoints/test_project_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -752,11 +752,12 @@ 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.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": {
Expand All @@ -769,12 +770,19 @@ def test_redacted_symbol_source_secrets(self):
"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": [redacted_source]})

self.get_valid_response(
self.org_slug, self.proj_slug, symbolSources=json.dumps([redacted_source])
)
Expand Down