rfc(feature): Controlling PII and Credentials in SDKs by sl0thentr0py · Pull Request #62 · getsentry/rfcs

sl0thentr0py · 2023-01-17T13:05:56Z

all my homies hate leaky pipelines

resolves getsentry/team-webplatform-meta#8

cleptric · 2023-01-20T16:16:22Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+# ===========================================================================
+# client.py
+
+def capture_event(self, event):


So here we would expect an SDK to process an event like event processors -> event scrubber -> before send?

yep, since we use event processors internally for integrations as well, I would say so.

cleptric · 2023-01-20T16:18:56Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+
+The actual scrubbing logic can be implemented in two ways with trade-offs outlined below.
+
+## A: Walk whole tree


While this might be an expensive option in terms of computation, I still prefer it over option B.

Do we expect users to set password, Password, PASSWORD, or should there be some some convenience built in?

good point, I guess checking lowercased key doesn't hurt so we cover more cases?

Yes, lowercase matching is the way.

cleptric · 2023-01-20T16:21:39Z

Overall I'm 👍 on this! While someone could argue that this is just a default impl of `before_send´, I like the simplicity.

antonpirker · 2023-01-23T09:43:14Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+## (Optional) C: Add separate security denylist
+
+A related proposal is to split the `denylist` into `pii_denylist` and `security_denylist` where stuff in `security_denylist` is **always** scrubbed
+irrespective of `send_default_pii` and stuff in `pii_denylist` is only scrubbed if `send_default_pii` is `False`.


I am for option A) (because it is secure by default and will not rot as fast)

But with two deny lists for PII and security data.

To make it impossible to send session data by setting sendDefaultPII to true. It could be though that some people are depending on having session data for debugging, but if a lot of people complain we can introduce sendDefaultSecurityData

+1

A separation between the two would be great here. It would significantly reduce the risks one has to consider when toggling the sendDefaultPII option.

Some may gravitate to using a sensitive value like a session cookie to track through their apps, but there are more secure ways of doing this with an identifier not inherently tied to auth.

:O thanks for stopping by on my RFC @mdtro! <3

adinauer · 2023-01-24T06:38:41Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+
+# Drawbacks
+
+* Option A: we add one more pass through the event object which adds some non-trivial computational overhead


If code obfuscation is turned on (e.g. ProGuard on Android) Option A could break. Assuming we check typed fields we'd have to use reflection to get the name of the field. This name might have been obfuscated thus not matching the key in the deny list. We'd have to apply the filtering to the serialized event or some step between but that would cause issues with ordering vs beforeSend as that again expects a non serialized event (or transaction for beforeSendTransaction) at the moment. Maybe a filter method could internally iterate fields considering serialization keys.

yes I believe most typed languages would have to go with option B.
The dict without interfaces seems to be mostly a python thing.

adinauer · 2023-01-24T06:42:18Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+        return self.scrub_dict(event)
+```
+
+Here we would walk the whole event dict and whenever we encounter a key from the denylist, we would replace the value with `[Filtered]`.


Could false positives bite use here? I might want to filter a property with the same name in HTTP headers but not in some other data field or similar.

they can, but that's fine. We will filter stuff out, if they have weird payloads like that, they will override the implementation. The default implementation should solve 99% PII needs with an easy to use interface.

adinauer · 2023-01-24T06:44:53Z

text/0062-controlling-pii-and-credentials-in-sd-ks.md

+* `event.exception.values -> value.stacktrace.frames -> frame.vars`
+* `event.spans -> span.data`
+
+## (Optional) C: Add separate security denylist


Is this intended as a second line of defense against leaking security related things? I assume we'd still prefer not to put the data into any of our data structures in the first place.

if we implement the scrubber in this RFC, I personally would like to move away from all those send_default_pii checks scattered around the codebase, but that's up for debate.

I meant more on the security side of things where we add data we never want to send to Sentry only to be removed again by the security denylist. Ah but it can be used by users to filter out regardless of sendDefaultPii so then separation sounds useful.

text/0062-controlling-pii-and-credentials-in-sd-ks.md

sl0thentr0py force-pushed the rfc/controlling-pii-and-credentials-in-sd-ks branch 21 times, most recently from 458eb4f to 82fc0d9 Compare January 20, 2023 13:49

sl0thentr0py marked this pull request as ready for review January 20, 2023 13:50

cleptric reviewed Jan 20, 2023

View reviewed changes

antonpirker reviewed Jan 23, 2023

View reviewed changes

adinauer reviewed Jan 24, 2023

View reviewed changes

sl0thentr0py requested review from a team and removed request for a team January 25, 2023 17:00

sl0thentr0py requested review from a team, AbhiPrasad and Lms24 January 25, 2023 17:01

philipphofmann reviewed Jan 25, 2023

View reviewed changes

text/0062-controlling-pii-and-credentials-in-sd-ks.md Show resolved Hide resolved

rfc(feature): Controlling PII and Credentials in SDKs

a9042ee

sl0thentr0py force-pushed the rfc/controlling-pii-and-credentials-in-sd-ks branch 2 times, most recently from ce2d9c1 to af7d0c6 Compare February 8, 2023 16:43

Update 0062-controlling-pii-and-credentials-in-sd-ks.md

5ec3372

sl0thentr0py force-pushed the rfc/controlling-pii-and-credentials-in-sd-ks branch from af7d0c6 to 5ec3372 Compare February 8, 2023 16:46

cleptric approved these changes Feb 8, 2023

View reviewed changes

sl0thentr0py merged commit 4c8a130 into main Feb 8, 2023

sl0thentr0py deleted the rfc/controlling-pii-and-credentials-in-sd-ks branch February 8, 2023 17:02

sl0thentr0py mentioned this pull request Feb 13, 2023

Implement config.event_scrubber getsentry/sentry-python#1897

Closed

cleptric mentioned this pull request Sep 12, 2023

PHP SDK 4.0.0 getsentry/sentry-php#1586

Closed

1 task

phptek mentioned this pull request May 5, 2024

Data Scrubbing phptek/silverstripe-sentry#96

Open


		The actual scrubbing logic can be implemented in two ways with trade-offs outlined below.

		## A: Walk whole tree


		# Drawbacks

		* Option A: we add one more pass through the event object which adds some non-trivial computational overhead

Uh oh!

Conversation

sl0thentr0py commented Jan 17, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sl0thentr0py Jan 20, 2023 • 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!

cleptric commented Jan 20, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

adinauer Jan 25, 2023 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sl0thentr0py commented Jan 17, 2023 •

edited

Loading

sl0thentr0py Jan 20, 2023 •

edited

Loading

adinauer Jan 25, 2023 •

edited

Loading