[SVCS-334]Adding custom sentry sanitizer by AddisonSchiller · Pull Request #297 · CenterForOpenScience/waterbutler

AddisonSchiller · 2017-11-08T19:01:39Z

Ticket

https://openscience.atlassian.net/browse/SVCS-334

Purpose

Sanitize and censor tokens we get back from Dataverse that show up in local variables

Changes

Added a sanitizer based that inherits from the default raven one. It has increased functionality to parse through dictionaries, look for dataverse keys, and a larger scope of variable names to sanitize (key, token)

NOTE: sanitize.py and test_sanitize.py's location in waterbutler are pretty variable. I put them in server just because that was the easiest place at the time. if they should live somewhere else, that is an easy change, and something that might need to be considered.

Side effects

Some sentry things that we don't want to sanitize may end up being sanitized.

QA Notes

To test, you need to trigger an error that will log a Dataverse API token in a local variable or request some how.
An easy way to do this is if locally testing, upgrade your furl to 1.0.1 in Waterbutler with Dataverse attached to your project. You will also need to add a personal sentry account to your Waterbutler (through your raven.config file, or by manually putting it in your settings).

One thing to note: If an API key shows up in the actual error message, this is currently not censored (not sure best way to go about this, but the only time I ever encountered this was with the error :Bad API token.. so the token is not valid anymore anyway.

To trigger the above "Bad API token" functionality, attach a dataverse account. Then on dataverse, go refresh your token and refresh the page. This error will trigger until the OSF has to revalidate (Or something like that. This only happened to me once or twice).

There are other manual ways to test locally, such as raising errors in odd places with variables named things like 'key' or having a variable value that looks like a dataverse token etc.

Deployment Notes

coveralls · 2017-11-08T19:09:36Z

Coverage increased (+0.04%) to 89.141% when pulling 853afcc on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 473191c on CenterForOpenScience:develop.

cslzchen

As discussed, need recursion for dict type and a question on UTF-8.

cslzchen · 2017-11-15T19:32:07Z

waterbutler/server/sanitize.py

+    MASK = '*' * 8
+
+    # Token and key added from original. Key is used by Dataverse
+    FIELDS = frozenset([


For phase 2 consideration: double check the list.

@cslzchen Can you please clarify this comment?

@TomBaxter My suggestion is check if there are other possible keys that may store sensitive or secret information. Here is the default keys from SanitizePasswordProcessor: https://github.com/htquach/raven-python/blob/master/raven/processors.py#L72.

FIELDS = frozenset([ 'password', 'secret', 'passwd', 'authorization', 'api_key', 'apikey', 'pw', 'cred' ])

We don't have 'pw', 'cred' and we added 'access_token', 'token', 'key' and 'sentry_dsn'. Other possible ones are 'refresh_token', 'code', etc. We need to understand what we need and what we don't.

Understood. I plan on making better use of inheritance so that we don't miss any defaults in the parent. I'll also check the sentry data for Dataverse, for other possible things to sanitize. I'll definitely add refresh_token.

cslzchen · 2017-11-15T19:33:28Z

waterbutler/server/sanitize.py

+        if isinstance(value, str) and self.VALUES_RE.search(value):
+            return self.MASK
+
+        if isinstance(value, dict):


Need recursion for dict + a test fot it.

Complete, for lists as well

cslzchen · 2017-11-15T19:42:23Z

waterbutler/server/sanitize.py

+        # Just in case we have bytes here, we want to turn them into text
+        # properly without failing so we can perform our check.
+        if isinstance(key, bytes):
+            key = key.decode('utf-8', 'replace')


Is it guaranteed to be correctly UTF-8 encoded?

Unsure. This line is directly from raven, so I assume so.

I checked the code base and it turns out we rarely try en/decoding. Let's leave it this way. In the future we may want to have a cleanup ticket that add try...except wrapper on this.

ill add a comment

coveralls · 2017-11-16T21:50:09Z

Coverage increased (+0.03%) to 89.973% when pulling fa72098 on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 26bf209 on CenterForOpenScience:develop.

coveralls · 2017-11-17T15:58:17Z

Coverage increased (+0.05%) to 89.994% when pulling 12f163e on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 26bf209 on CenterForOpenScience:develop.

coveralls · 2017-11-17T17:08:33Z

Coverage increased (+0.05%) to 89.994% when pulling b776bc8 on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 26bf209 on CenterForOpenScience:develop.

Johnetordoff

Just an import order problem and some debugging stuff to get rid of.

Johnetordoff · 2017-11-28T14:31:56Z

tests/server/test_sanitize.py

@@ -0,0 +1,196 @@
+import pytest
+from unittest import mock


unittest is a built-in library, import order.

Johnetordoff · 2017-11-28T14:36:09Z

tests/server/test_sanitize.py

+        result = sanitizer.sanitize('sanitize_dict', sanitize_dict)
+
+        # Sanity check
+        assert result != {


Not necessary given the next the statement.

coveralls · 2017-11-28T15:44:29Z

Coverage increased (+0.05%) to 89.994% when pulling 1df226a on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 26bf209 on CenterForOpenScience:develop.

coveralls · 2017-11-28T15:44:29Z

Coverage increased (+0.05%) to 89.994% when pulling 1df226a on AddisonSchiller:feature/SVCS-334-davaverse-tokens into 26bf209 on CenterForOpenScience:develop.

cslzchen · 2017-11-29T20:43:12Z

waterbutler/server/app.py

        debug=debug,
    )
-    app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=waterbutler.__version__)
+    app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=waterbutler.__version__,


~~Need to update this after bumping setuptools to 37.0.0.~~ Update: ignore this comment

@cslzchen Can you please clarify this comment?

@TomBaxter Thanks for catching this. I probably was thinking about MFR. No need to change.

TomBaxter · 2017-12-26T14:46:02Z

waterbutler/server/sanitize.py

+    MASK = '*' * 8
+
+    # Token and key added from original. Key is used by Dataverse
+    FIELDS = frozenset([


@cslzchen Can you please clarify this comment?

TomBaxter · 2017-12-26T14:49:50Z

waterbutler/server/app.py

        debug=debug,
    )
-    app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=waterbutler.__version__)
+    app.sentry_client = AsyncSentryClient(settings.SENTRY_DSN, release=waterbutler.__version__,


@cslzchen Can you please clarify this comment?

TomBaxter · 2017-12-26T14:50:50Z

waterbutler/server/sanitize.py

+
+        key = key.lower()
+        for field in self.FIELDS:
+            if field in key:


Check key directly from self.FIELDS

TomBaxter · 2017-12-26T14:55:22Z

waterbutler/server/sanitize.py

+
+
+class WBSanitizer(SanitizePasswordsProcessor):
+    """Asterisk out things that look like passwords, keys, etc."""


Make better use of parent class.

cslzchen · 2018-01-03T20:54:07Z

As mentioned, this PR is closed and continues in #313.

cslzchen self-requested a review November 10, 2017 19:45

cslzchen added the Code Review label Nov 10, 2017

cslzchen requested changes Nov 15, 2017

View reviewed changes

Adding custom sentry sanitizer

58ef57f

AddisonSchiller force-pushed the feature/SVCS-334-davaverse-tokens branch from 0aae5df to fb273ea Compare November 17, 2017 16:09

Add sanitize nested lists and dictionaries

b776bc8

AddisonSchiller force-pushed the feature/SVCS-334-davaverse-tokens branch from 56d7c6b to b776bc8 Compare November 17, 2017 16:13

Johnetordoff suggested changes Nov 28, 2017

View reviewed changes

change test and import order

1df226a

cslzchen reviewed Nov 29, 2017

View reviewed changes

cslzchen added To Be Replaced and removed Code Review labels Dec 15, 2017

TomBaxter reviewed Dec 26, 2017

View reviewed changes

TomBaxter mentioned this pull request Dec 27, 2017

[SVCS-334] Sentry captures dataverse API tokens on certain error types #313

Closed

1 task

cslzchen closed this Jan 3, 2018

cslzchen removed To Be Replaced labels Oct 1, 2018



		class WBSanitizer(SanitizePasswordsProcessor):
		"""Asterisk out things that look like passwords, keys, etc."""

Conversation

AddisonSchiller commented Nov 8, 2017

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

Uh oh!

coveralls commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cslzchen 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!

cslzchen Dec 26, 2017 • 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!

TomBaxter Dec 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

cslzchen Nov 17, 2017 • 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!

coveralls commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Johnetordoff 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!

coveralls commented Nov 28, 2017

Uh oh!

coveralls commented Nov 28, 2017

Uh oh!

cslzchen Nov 29, 2017 • 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!

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!

cslzchen commented Jan 3, 2018

Uh oh!

Reviewers

coveralls commented Nov 8, 2017 •

edited

Loading

cslzchen Dec 26, 2017 •

edited

Loading

TomBaxter Dec 26, 2017 •

edited

Loading

cslzchen Nov 17, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

cslzchen Nov 29, 2017 •

edited

Loading