[PLAY-2375] -- Sanitize Storage URIs #54

shroudofthurin · 2020-04-01T05:35:04Z

Add the ability for each backend to sanitize it's URI in such a way that is most appropriate for the way it encodes secret data.

@ustudio/reviewers for your consideration.

…ize the URI in the manner that is most appropriate for the way it encodes secret data. [PLAY-2375]

[PLAY-2375]

spiralman

A couple things. Also, I only happened to remember the download_url_key query parameter in the Swift/CloudFiles backends. There might be other query parameters or URI components that are sensitive in those or other implementations. We should read through the implementations and ensure we have all sensitive data (including optional things) removed in the sanitized URL for each one.

Also, when we finish this, we need to remember to back-port it to the 0.12 branch for legacy Python 2 projects.

spiralman · 2020-04-01T13:50:40Z

storage/url_parser.py

+        parsed_url = parsed_uri._replace(
+            netloc=":".join((parsed_uri.hostname, str(parsed_uri.port))))
+    else:
+        parsed_url = parsed_uri._replace(netloc=parsed_uri.hostname)


I think this might be clearer, and less redundant, if it were rewritten along the lines of:

new_netloc = parsed_uri.hostname if parsed_uri.port is not None: new_netloc = ":".join((new_netloc, str(parsed_uri.port))) # _replace, etc...

tests/test_swift_storage.py

tests/test_cloudfiles_storage.py

spiralman · 2020-04-01T14:14:17Z

storage/storage.py

@@ -129,6 +129,10 @@ def get_download_url(self, seconds: int = 60, key: Optional[str] = None) -> str:
        raise NotImplementedError(
            "{} does not implement 'get_download_url'".format(self._class_name()))

+    def get_sanitized_uri(self) -> str:
+        raise NotImplementedError(


I'm not 100% sure how I feel about this, but, since almost all of the implementations ended up being the same, should we put a default implementation for this in the base class?

One danger with doing that is that it, if a particular backend has sensitive data elsewhere in the URL (besides the netloc) and the implementor forgets to override this implementation, it's possible that the default sanitized URL could return sensitive data.

One solution to that would be to have the default implementation be overly restrictive (say, only returning the scheme, hostname, port and path, no query parameters, etc), and then have implementations override it which want to include extra values in the URL.

Again, I'm on the fence about which of these options is best (including just leaving it as it's currently implemented). Perhaps @tjensen or @joshmarshall has an opinion?

@spiralman @shroudofthurin I think I'm okay with the default implementation being in the base class and even making it overly restrictive.

One thing we could do is have the base class take an optional whitelist of identifiers indicating URI parts that are safe to include in the sanitized URI. Then, subclasses that want to include the query parameters and username can tell the base to include them. e.g.

class DerivedStorage(Storage): def __init__(self): super().__init__(sanitize_whitelist=[USERNAME, QUERY])

...could result in storage URIs that look like: proto://username@netloc/path?query=parameters

@tjensen @spiralman @shroudofthurin While I agree that the current implementations are very similar, this is explicit boilerplate I believe to be a Good Thing[tm], since it's such an important consideration for any project using storage URIs. Thus my preference would be that it should raise, as it does in the code now (although I might prefer renaming the "sanitized_uri" helper to "sanitize_basic_auth" or something more specific.) If we do use a default behavior instead of raising, I recommend that we at least log a warning when a storage class with the "default" sterilization behavior is imported / registered.

I definitely agree with renaming the sanitize_uri helper to something more explicit (basic_auth feels a little confusing, because we're not actually using it for HTTP Basic Auth). The spec calls it "user information", so maybe remove_user_info?

Anyway, I'm 50/50 on whether there should be a default implementation or it should raise, so it seems like we have a hung jury. :-) I guess I'd recommend @shroudofthurin make the deciding vote, since it's her PR.

@spiralman I think I will go with your original solution:

One solution to that would be to have the default implementation be overly restrictive (say, only returning the scheme, hostname, port and path, no query parameters, etc), and then have implementations override it which want to include extra values in the URL.

[PLAY-2375]

…sent, as per reviewer's request [PLAY-2375]

…equest [PLAY-2375]

[PLAY-2375]

…, port and path, no query parameters. [PLAY-2375]

…ctive, and then have implementations override it which want to include extra values in the URL, as per reviewer's request. [PLAY-2375]

[PLAY-2375]

shroudofthurin · 2020-04-06T05:44:52Z

@spiralman I think I've addressed all of your comments. Would you mind taking another look at this PR when you get the chance? Thanks 🍡

[PLAY-2375]

spiralman

A couple things.

spiralman · 2020-04-06T14:28:39Z

storage/url_parser.py

+
+    new_query = dict(parse_qsl(parsed_uri.query))
+
+    if "download_url_key" in new_query:


This code is specific to Swift/CloudFiles, and should be in those implementations, not here.

spiralman · 2020-04-06T14:29:49Z

tests/test_local_storage.py

@@ -267,3 +267,20 @@ def test_local_storage_get_download_url_returns_none_on_empty_base(self) -> None

    def test_local_storage_rejects_multiple_query_values_for_download_url_key_setting(self) -> None:
        self.assert_rejects_multiple_query_values("/foo/bar/object.mp4", "download_url_base")
+
+    def test_local_storage_get_sanitized_uri(self) -> None:


The test name should indicate what the expected behavior is. In this case, there's nothing "secret" in a local file:// URI, so there is nothing to remove.

spiralman · 2020-04-06T14:32:22Z

storage/url_parser.py

+    return new_uri.geturl()
+
+
+def sanitize_filepath(parsed_uri: ParseResult) -> str:


I'm confused about the behvior of this function. It seems like it removes the username/password and all query parameters from the URL. However, that doesn't really match what the name says (I would expect it to remove sensitive information from the path part of the URI, based on the name).

…viewer's request. [PLAY-2375]

…ns, as per reviewer's request. [PLAY-2375]

[PLAY-2375]

shroudofthurin · 2020-04-07T07:25:35Z

@spiralman back to you

[PLAY-2375]

spiralman

LGTM

spiralman · 2020-04-08T18:42:57Z

@shroudofthurin I realized that this is missing a version number bump. Normally, we bump the minor version, for a new feature, since this is still pre-1.0. That'd make the next version 0.14.0, but if you and @joshmarshall prefer to bump the bugfix version, that's fine with me (SemVer is a little goofy before 1.0).

I'm not making this a blocking comment so that you can get it merged during the day in your timezone.

tjensen · 2020-04-08T21:19:32Z

@spiralman If we bump the master version from 0.13.3 to 0.14, what should we bump the Python 2-only v0.12 branch version to?

spiralman · 2020-04-08T22:04:04Z

@tjensen Yeah, that's where it's kinda goofy. My feeling is only bump the bugfix number in the 0.12 branch, but bump the minor version in master: since Python 2 is no longer officially supported, I think we'll need to maintain the 0.12 branch on an "as needed" basis, which means SemVer won't work out very well. Given that usage of it should drop off, hopefully that's OK?

I'm fine with other ideas for bumping the version. Since we haven't ever moved to 1.0, SemVer is somewhat hazy, so I suspect it mostly just matters that we bump it to a bigger number.

[PLAY-2375]

joshustudio

@shroudofthurin Just a few remaining documentation questions!

joshustudio · 2020-04-09T05:33:43Z

README.md

+#### `get_sanitized_uri()` ####
+
+Removes the username/password, as well as all query paramters, form the URL.
+


"parameters"

Do we want to add documentation for remove_user_info() too, since other users can create and register their own storage providers?

README.md

[PLAY-2375]

…ble documentation, included the latest Python 2.7 release as well, as per reviewer's request. [PLAY-2375]

shroudofthurin · 2020-04-09T05:47:49Z

@joshustudio I've made the requested changes 🍡

…s request. [PLAY-2375]

shroudofthurin · 2020-04-09T06:52:02Z

@joshustudio Added documentation for the url_parser module. Back to you.

joshustudio

LGTM!

shroudofthurin added 8 commits March 30, 2020 15:17

Start adding get_sanitized_uri methods, so that the backend can sanit…

c68bdbc

…ize the URI in the manner that is most appropriate for the way it encodes secret data. [PLAY-2375]

Added get_sanitized_uri method to ftp storage.

d8b75a4

[PLAY-2375]

Create url_parser lib that can be used to sanitize storage uris.

f64a7c6

[PLAY-2375]

Update storage classes to use new url parser library

9caea27

[PLAY-2375]

Update google cloud storage to have get_sanitized_uri method

2cdda81

[PLAY-2375]

Update local storage to have get_sanitized_uri method

9669dce

[PLAY-2375]

Update swift storage to have get_sanitized_uri method

7ad1bb3

[PLAY-2375]

Fix mypy errors

b2ddf1b

[PLAY-2375]

spiralman requested changes Apr 1, 2020

View reviewed changes

shroudofthurin added 7 commits April 2, 2020 14:08

Remove some redundancies, as per reviewer's request.

a3ad578

[PLAY-2375]

Ensure that download_url_key is removed from the sanitized URL if pre…

8d62fff

…sent, as per reviewer's request [PLAY-2375]

Rename library helper to something more explicit, as per reviewer's r…

8695207

…equest [PLAY-2375]

Rename library helper tests to reflect the changes made to file.

d80a51e

[PLAY-2375]

Add new function to url_parser that only returns the scheme, hostname…

a598144

…, port and path, no query parameters. [PLAY-2375]

Make the default implementation of get_sanitized_uri be overly restri…

7df52df

…ctive, and then have implementations override it which want to include extra values in the URL, as per reviewer's request. [PLAY-2375]

Add get_santized_uri implementation to s3.

6e4ab70

[PLAY-2375]

Fix mypy errors

4a8e9d9

[PLAY-2375]

spiralman requested changes Apr 6, 2020

View reviewed changes

shroudofthurin added 4 commits April 6, 2020 23:47

Update test name to indicate what the expected behavior is, as per re…

e8ad3fc

…viewer's request. [PLAY-2375]

Move Swift/CloudFiles specific code into their specific implementatio…

e0906fe

…ns, as per reviewer's request. [PLAY-2375]

Rename url_parser function, as per reviewer's request.

d5b3d43

[PLAY-2375]

Fix mypy errors.

03a3bc2

[PLAY-2375]

shroudofthurin added 2 commits April 7, 2020 16:39

Rename url_parser function, as per reviewer's request.

aee6715

[PLAY-2375]

Fix mypy errors.

96efff4

[PLAY-2375]

spiralman previously approved these changes Apr 7, 2020

View reviewed changes

Bumping version to v0.14.0.

77aaa07

[PLAY-2375]

shroudofthurin dismissed spiralman’s stale review via 77aaa07 April 9, 2020 00:47

Document the newly added function, get_sanitized_uri().

fe50ae6

[PLAY-2375]

joshustudio requested changes Apr 9, 2020

View reviewed changes

shroudofthurin added 2 commits April 9, 2020 14:41

FIx spelling mistake

9221f65

[PLAY-2375]

Since the README on master for the GitHub project is the most accessi…

35e928a

…ble documentation, included the latest Python 2.7 release as well, as per reviewer's request. [PLAY-2375]

Document the url parser library in the READ.me docs, as per reviewer'…

92042b8

…s request. [PLAY-2375]

joshustudio approved these changes Apr 9, 2020

View reviewed changes

shroudofthurin merged commit 28349fb into master Apr 9, 2020

shroudofthurin deleted the play-2375-sanitize-storage-uris branch April 9, 2020 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLAY-2375] -- Sanitize Storage URIs #54

[PLAY-2375] -- Sanitize Storage URIs #54

shroudofthurin commented Apr 1, 2020

spiralman left a comment

spiralman Apr 1, 2020

spiralman Apr 1, 2020

tjensen Apr 1, 2020

joshmarshall Apr 2, 2020

spiralman Apr 2, 2020

shroudofthurin Apr 3, 2020

shroudofthurin commented Apr 6, 2020

spiralman left a comment

spiralman Apr 6, 2020

spiralman Apr 6, 2020

spiralman Apr 6, 2020

shroudofthurin commented Apr 7, 2020

spiralman left a comment

spiralman commented Apr 8, 2020

tjensen commented Apr 8, 2020

spiralman commented Apr 8, 2020

joshustudio left a comment

joshustudio Apr 9, 2020

joshustudio Apr 9, 2020

shroudofthurin commented Apr 9, 2020

shroudofthurin commented Apr 9, 2020

joshustudio left a comment


		new_query = dict(parse_qsl(parsed_uri.query))

		if "download_url_key" in new_query:

		return new_uri.geturl()


		def sanitize_filepath(parsed_uri: ParseResult) -> str:

		#### `get_sanitized_uri()` ####

		Removes the username/password, as well as all query paramters, form the URL.

[PLAY-2375] -- Sanitize Storage URIs #54

[PLAY-2375] -- Sanitize Storage URIs #54

Conversation

shroudofthurin commented Apr 1, 2020

spiralman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shroudofthurin commented Apr 6, 2020

spiralman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shroudofthurin commented Apr 7, 2020

spiralman left a comment

Choose a reason for hiding this comment

spiralman commented Apr 8, 2020

tjensen commented Apr 8, 2020

spiralman commented Apr 8, 2020

joshustudio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shroudofthurin commented Apr 9, 2020

shroudofthurin commented Apr 9, 2020

joshustudio left a comment

Choose a reason for hiding this comment