rfc(feature): Controlling PII and Credentials in SDKs #62
Conversation
sl0thentr0py
force-pushed
the
rfc/controlling-pii-and-credentials-in-sd-ks
branch
21 times, most recently
from
458eb4f to
82fc0d9
Compare
| # =========================================================================== | ||
| # client.py | ||
|
|
||
| def capture_event(self, event): |
There was a problem hiding this comment.
So here we would expect an SDK to process an event like event processors -> event scrubber -> before send?
There was a problem hiding this comment.
yep, since we use event processors internally for integrations as well, I would say so.
|
|
||
| The actual scrubbing logic can be implemented in two ways with trade-offs outlined below. | ||
|
|
||
| ## A: Walk whole tree |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
good point, I guess checking lowercased key doesn't hurt so we cover more cases?
There was a problem hiding this comment.
Yes, lowercase matching is the way.
|
Overall I'm 👍 on this! While someone could argue that this is just a default impl of `before_send´, I like the simplicity. |
| ## (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`. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
:O thanks for stopping by on my RFC @mdtro! <3
|
|
||
| # Drawbacks | ||
|
|
||
| * Option A: we add one more pass through the event object which adds some non-trivial computational overhead |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes I believe most typed languages would have to go with option B.
The dict without interfaces seems to be mostly a python thing.
| 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]`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * `event.exception.values -> value.stacktrace.frames -> frame.vars` | ||
| * `event.spans -> span.data` | ||
|
|
||
| ## (Optional) C: Add separate security denylist |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sl0thentr0py
force-pushed
the
rfc/controlling-pii-and-credentials-in-sd-ks
branch
2 times, most recently
from
ce2d9c1 to
af7d0c6
Compare
sl0thentr0py
force-pushed
the
rfc/controlling-pii-and-credentials-in-sd-ks
branch
from
af7d0c6 to
5ec3372
Compare
all my homies hate leaky pipelines
Rendered RFC
resolves getsentry/team-webplatform-meta#8