feat(symbol-sources): Redact symbol source secrets from audit logs #28334

relaxolotl · 2021-09-01T00:13:19Z

Does what it says on the tin. Instead of storing the raw contents of a symbol source's secret into an audit log entry, this uses existing logic to store redacted values instead.

Things left to do:

I can't figure out how to get the mock to hook up properly

NATIVE-178

relaxolotl · 2021-09-01T00:15:39Z

tests/sentry/api/endpoints/test_project_details.py

        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")


I'm botching something here I think, and I can't figure out what it is right now

So python is finicky^Wfun. The function is called from sentry.api.base via some indirections. If you look at that module it imports the function directly: from sentry.utils.audit import create_audit_entry. Now at import time the globals of sentry.api.base module get the entry for create_audit_entry.

So sometime later while executing your test the monkeypatch is changing the sentry.utils.audit.__globals__['create_audit_entry'] = patched_version. This will no longer affect sentry.api.base.__globals__['create_audit_entry'] because import time is long over and the actual function you are trying to test keeps using the unpatched version. The solution is to patch the thing which is actually being executed during the test: sentry.api.base.create_audit_entry.

(as an aside, there's some old style-and-or-convention to prefer importing entire modules in python rather than things inside modules, but it doesn't seem to be followed in sentry much. In that case the import in sentry.api.base would have been from sentry.utils import audit and the call audit.create_audit_entry, in that case monkeypatching sentry.utils.audit.create_audit_library would have affected the globals of the audit module as expected and the code does the function lookup in the globals of the module at runtime and would have found the monkeypatched version. oh well)

In any case, I committed a change to fix this test. I also did some renaming to make me less confused. Lastly note that the test had to be changed to not check for the serialised config since it is now put in the audit log without serialising. I think this is ok for behaviour so adapted the test rather than the code.

thanks for fixing this and the in-depth explanation! is there a document that goes into detail how these rules work in python when it comes to imports (e.g. which module is the true owner of an imported or exported method), or is this particular create_audit_entry instance an example of sentry monkeypatching and doing something that's specific to sentry itself?

No this isn't specific to sentry, not sure if there's a simple document. it's kind of a side effect of how the entire python execution model is built and everything is done by an interpreter one thing after the other, how globals and locals are just dicts etc.

Swatinem

hm, I must admit I am completely lost where things are being saved or returned or whatever. not sure if you are now redacting things in places where they are actually persisted as symbol source.

Swatinem · 2021-09-01T08:07:34Z

src/sentry/api/endpoints/project_details.py

+                except Exception:
+                    sources = []
+                redacted_sources = redact_source_secrets(sources)
+                changed_proj_settings["sentry:symbol_sources"] = redacted_sources


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

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

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.

- 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.

flub · 2021-09-01T08:13:49Z

src/sentry/api/endpoints/project_details.py

+                except Exception:
+                    sources = []
+                redacted_sources = redact_source_secrets(sources)
+                changed_proj_settings["sentry:symbol_sources"] = redacted_sources


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

flub · 2021-09-01T08:17:17Z

src/sentry/lang/native/symbolicator.py

-    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."""


Maybe mention that this returns a copy and leaves the original one in place?

flub · 2021-09-01T08:18:58Z

src/sentry/lang/native/symbolicator.py


-    sources = parse_sources(config_sources, False)
-    for source in sources:
+    redacted_sources = deepcopy(config_sources)


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.

flub · 2021-09-01T08:57:38Z

tests/sentry/api/endpoints/test_project_details.py

        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")


So python is finicky^Wfun. The function is called from sentry.api.base via some indirections. If you look at that module it imports the function directly: from sentry.utils.audit import create_audit_entry. Now at import time the globals of sentry.api.base module get the entry for create_audit_entry.

So sometime later while executing your test the monkeypatch is changing the sentry.utils.audit.__globals__['create_audit_entry'] = patched_version. This will no longer affect sentry.api.base.__globals__['create_audit_entry'] because import time is long over and the actual function you are trying to test keeps using the unpatched version. The solution is to patch the thing which is actually being executed during the test: sentry.api.base.create_audit_entry.

(as an aside, there's some old style-and-or-convention to prefer importing entire modules in python rather than things inside modules, but it doesn't seem to be followed in sentry much. In that case the import in sentry.api.base would have been from sentry.utils import audit and the call audit.create_audit_entry, in that case monkeypatching sentry.utils.audit.create_audit_library would have affected the globals of the audit module as expected and the code does the function lookup in the globals of the module at runtime and would have found the monkeypatched version. oh well)

In any case, I committed a change to fix this test. I also did some renaming to make me less confused. Lastly note that the test had to be changed to not check for the serialised config since it is now put in the audit log without serialising. I think this is ok for behaviour so adapted the test rather than the code.

flub · 2021-09-01T08:58:21Z

hm, I must admit I am completely lost where things are being saved or returned or whatever. not sure if you are now redacting things in places where they are actually persisted as symbol source.

I think it is correct 😄

* master: (109 commits) ref(js): Add notice to use useApi when applicable (#28365) fix(tests): Temporarily disable test for migration 0223 (#28363) fix(ui): Use theme border for form footer border (#28359) ref(ts): Authenticators have names (#28362) feat(cdc_search): Implement cdc search for `is:unresolved` query. (#28006) ref(rrweb): Upgrade to latest player (#28333) feat(issue-alerts): Add Sentry Apps Alert Rule Action as an Action (#28338) feat(notifications): Remove NotificationSettings when Slack is uninstalled (#28216) py38(django 2.2): bump django-crispy-forms to 1.8.1 (#28344) types(python): Slack Integration Types (#28236) ref(jira ac): Rm fk constraints from JiraTenant (#28349) chore(js): Upgrade storybook from 6.3.6 -> 6.3.7 (#28323) feat(symbol-sources): Redact symbol source secrets from audit logs (#28334) feat(notifications): Add `actor_id` to Analytics Events (#28347) ref(performance): Separate header from content in transaction events (#28346) Adding in new text styles for the codeowners sync CTA in the issues detail. (#28133) fix(django 2.2): rename view method to avoid clashing (#28312) fix(alerts): Project breadcrumb link to project rule list (#28339) ref(performance): Separate header from content in transaction vitals (#28343) feat(dev_env): Dev env bootstrap support for two Python versions (#28213) ...

…28334) This redacts any secrets stored in symbol sources before they are logged as an audit entry. Co-authored-by: Floris Bruynooghe <flub@sentry.io>

relaxolotl added 2 commits August 31, 2021 20:01

feat(symbol-sources): Log only redacted secrets in the audit log

72acff3

i did my best but i can't seem to make this work

14f2576

relaxolotl requested review from Swatinem and flub September 1, 2021 00:13

relaxolotl commented Sep 1, 2021

View reviewed changes

Swatinem reviewed Sep 1, 2021

View reviewed changes

fix test

888054a

- 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.

vercel bot deployed to Preview – storybook September 1, 2021 08:47 View deployment

vercel bot deployed to Preview – sentry September 1, 2021 08:47 View deployment

flub approved these changes Sep 1, 2021

View reviewed changes

relaxolotl marked this pull request as ready for review September 1, 2021 16:21

relaxolotl requested a review from a team September 1, 2021 16:21

explain that a method doesn't mutate the parameter

4ae11ee

vercel bot deployed to Preview – storybook September 1, 2021 18:28 View deployment

vercel bot deployed to Preview – sentry September 1, 2021 18:28 View deployment

relaxolotl enabled auto-merge (squash) September 1, 2021 18:29

relaxolotl merged commit adb01c0 into master Sep 1, 2021

relaxolotl deleted the feat/redact-audit branch September 1, 2021 20:04

relaxolotl mentioned this pull request Sep 9, 2021

fix(appstore): Redact iTunes session cookies as well #28471

Merged

github-actions bot locked and limited conversation to collaborators Sep 18, 2021

Uh oh!

feat(symbol-sources): Redact symbol source secrets from audit logs #28334

feat(symbol-sources): Redact symbol source secrets from audit logs #28334

Uh oh!

Conversation

relaxolotl commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swatinem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

relaxolotl Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub commented Sep 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

relaxolotl commented Sep 1, 2021 •

edited

Loading

flub Sep 1, 2021 •

edited

Loading

relaxolotl Sep 1, 2021 •

edited

Loading

flub Sep 1, 2021 •

edited

Loading