Redact password from various log messages #5590

kvalev · 2018-07-10T17:42:07Z

Redact all authentication information when logging urls of Git-based requirements.

Current behavior:

Collecting git+https://username:password@gitlab.com/user/project
  Cloning https://username:password@gitlab.com/user/project to /private/var/folders/rp/7lwtlxfs11g5v1k_9vxph_md2rygl_/T/pip-req-build-04B4B0
Requirement already satisfied (use --upgrade to upgrade): project==0.1.0 from git+https://username:password@gitlab.com/user/project in /usr/local/lib/python2.7/site-packages

New behavior:

Collecting git+https://username:***@gitlab.com/user/project
  Cloning https://username:***@gitlab.com/user/project to /private/var/folders/rp/7lwtlxfs11g5v1k_9vxph_md2rygl_/T/pip-req-build-04B4B0
Requirement already satisfied (use --upgrade to upgrade): project==0.1.0 from git+https://username:***@gitlab.com/user/project in /usr/local/lib/python2.7/site-packages

Fixes #4746

pradyunsg · 2018-07-10T18:33:20Z

Thanks @kvalev! This LGTM as is.

I think pip should actually show <redacted> for the authentication information here (and there's one more place where we made a similar change recently). I understand that it might make this a little more work than this PR is currently. Do let me know if you'd be willing to try a hand at that. :)

kvalev · 2018-07-11T14:53:14Z

I will take a stab at it @pradyunsg, it seems simple enough.

cjerdonek · 2018-07-12T05:43:29Z

src/pip/_internal/utils/misc.py

+def redact_auth_from_url(url):
+    # Return a copy of url with 'username:password@' redacted by
+    # substituting the credentials with '<redacted>' in case they
+    # were present in the url.


I think it would be better if only the password is replaced with a substitution string (e.g. <redacted> or ****). It helps to have the username present, e.g. for troubleshooting purposes.

cjerdonek · 2018-07-12T10:14:44Z

tests/unit/test_utils.py

@@ -646,3 +646,26 @@ def test_call_subprocess_closes_stdin():
 def test_remove_auth_from_url(auth_url, expected_url):
    url = remove_auth_from_url(auth_url)
    assert url == expected_url
+
+
+@pytest.mark.parametrize('auth_url, expected_url', [


Maybe add test cases of:

containing : but no @ (e.g. domain.tld:8080)

password containing a :

Also, I would order the test cases "simplest" to least simple, e.g. start with the test cases having no user-pass, and put all cases with password redaction at the end. The current ordering makes it harder to tell what cases are covered and left out.

Does a password containing ":" need to be urlencoded?

Maybe.

>>> urlparse('https://user:pass:word@domain.tld/svn/project/trunk') ParseResult(scheme='https', netloc='user:pass:word@domain.tld', path='/svn/project/trunk', params='', query='', fragment='')

It couldn't hurt to add test cases for both -- : both urlencoded and not urlencoded. Since this is user input, it seems like we can't guarantee it will be encoded correctly, etc.

To expand the previous example:

>>> parsed = urlparse('https://user:pass:word@domain.tld/svn/project/trunk') >>> parsed.password 'pass:word'

(But yes, it looks like it's supposed to be encoded.)

this is user input, it seems like we can't guarantee it will be encoded correctly, etc.

Yeah. This makes sense. @kvalev Could you add a case here: 'https://user:pass:word@domain.tld/svn/project/trunk'?

Already did, see the latest commit.

Indeed. I was viewing an outdated diff. Sorry for the noise. :)

cjerdonek · 2018-07-12T10:16:13Z

src/pip/_internal/utils/misc.py

@@ -852,6 +853,30 @@ def enum(*sequential, **named):
    return type('Enum', (), enums)


+def redact_password_from_url(url):
+    # Return a copy of url by redacting the password with '****' in case it
+    # was present in the url.


Make this a docstring not code comment.

cjerdonek · 2018-07-12T10:16:56Z

src/pip/_internal/utils/misc.py

+    # Return a copy of url by redacting the password with '****' in case it
+    # was present in the url.
+
+    # parsed url


This comment seems unnecessary.

cjerdonek · 2018-07-12T10:17:08Z

src/pip/_internal/utils/misc.py

+    # parsed url
+    purl = urllib_parse.urlsplit(url)
+
+    # redact the password


This comment seems unnecessary.

cjerdonek · 2018-07-12T10:17:18Z

src/pip/_internal/utils/misc.py

+
+        redacted_netloc = userpass + '@' + netloc
+
+    # redacted url


This comment seems unnecessary.

cjerdonek · 2018-07-12T10:19:15Z

news/4746.bugfix

@@ -0,0 +1 @@
+Redact username/password from log messages when installing packages from Git


Update to say you're only redacting the password. Also, isn't redact_password_from_url() being called in non-Git cases, too, in some cases? I would just say "Redact password from the URL in some log messages."

cjerdonek · 2018-07-12T18:29:29Z

src/pip/_internal/utils/misc.py

+
+    redacted_netloc = purl.netloc
+    if purl.password:
+        auth, netloc = redacted_netloc.split('@')


FYI, this will cause an exception if the user included a password with an unencoded "@", like
"https://user:pass@word@domain.tld/svn/project/trunk"

cjerdonek · 2018-07-12T18:34:17Z

tests/unit/test_utils.py

+
+@pytest.mark.parametrize('auth_url, expected_url', [
+    ('http://user@domain.tld:8080/',
+     'http://user@domain.tld:8080/',),


I don't see the test case I suggested of a netloc containing a : with no @ for the netloc, like "domain.tld:8080", to make sure the characters following the colon aren't stripped.

cjerdonek · 2018-07-12T18:42:56Z

src/pip/_internal/utils/misc.py

+    purl = urllib_parse.urlsplit(url)
+
+    redacted_netloc = purl.netloc
+    if purl.password:


It looks like this new version will leak the password if it's the empty string:

>>> a = urllib.parse.urlsplit("https://user:@domain.tld/svn") >>> a.password ''

If you want to use the password attribute, you should probably be checking for non-None.

cjerdonek · 2018-07-15T08:52:50Z

I'm a big fan of sharing code and not duplicating logic. One thing I notice is that remove_auth_from_url() and redact_password_from_url() are almost identical, except for what they do to the netloc.

I think it would be good to share code between remove_auth_from_url() and redact_password_from_url(), e.g. by defining a function with signature transform_url(url, transform_netloc) that accepts the part that is different between the two functions (a function that transforms the netloc).

This could even reduce the combinatorial explosion of cases to test because you could independently test the two different transform_netloc() functions, instead of having every test be a test of the whole thing.

pradyunsg · 2018-07-15T09:40:44Z

+1 to what Chris says, that would simplify both the tests and the code. :)

cjerdonek · 2018-07-22T19:26:26Z

In the interests of sharing code and not duplicating logic, I think it would help a lot to add a new function to misc.py whose implementation matches this newly added method (it already has tests):

pip/src/pip/_internal/vcs/subversion.py

Lines 103 to 125 in a296897

    
               def get_netloc_and_auth(self, netloc): 
        
                   """ 
        
                   Parse out and remove from the netloc the auth information. 
        
                   This allows the auth information to be provided via the --username 
        
                   and --password options instead of via the URL. 
        
                   """ 
        
                   if '@' not in netloc: 
        
                       return netloc, (None, None) 
        
                   # Split from the right because that's how urllib.parse.urlsplit() 
        
                   # behaves if more than one @ is present (by checking the password 
        
                   # attribute of urlsplit()'s return value). 
        
                   auth, netloc = netloc.rsplit('@', 1) 
        
                   if ':' in auth: 
        
                       # Split from the left because that's how urllib.parse.urlsplit() 
        
                       # behaves if more than one : is present (again by checking the 
        
                       # password attribute of the return value) 
        
                       user_pass = tuple(auth.split(':', 1)) 
        
                   else: 
        
                       user_pass = auth, None 
        
                   return netloc, user_pass

And then change Subversion.get_netloc_and_auth() to call this new function.

You can then also use the new function inside the new functions you'll be creating in misc.py.

BrownTruck · 2018-07-24T20:30:02Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

orf · 2018-09-10T12:16:38Z

This PR seems abandoned, I created #5773 to follow on the work.

lock · 2019-06-01T18:45:26Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

kvalev added 3 commits July 10, 2018 19:19

Remove username/password from log message

2052942

Added news fragment

6f5cae2

Lint issue

27a82d1

pradyunsg added the type: enhancement Improvements to functionality label Jul 10, 2018

Redact username/password

58a8adf

kvalev changed the title ~~Remove username/password from log messages when installing from Git~~ Redact username/password from log messages when installing from Git Jul 12, 2018

cjerdonek reviewed Jul 12, 2018

View reviewed changes

kvalev added 2 commits July 12, 2018 11:39

Redact only the password

51b6075

Lint issue

72b539d

cjerdonek reviewed Jul 12, 2018

View reviewed changes

Minor code changes

44a0cfb

kvalev changed the title ~~Redact username/password from log messages when installing from Git~~ Redact password from various log messages Jul 12, 2018

cjerdonek reviewed Jul 12, 2018

View reviewed changes

Handling of corner cases

9dc8f9d

pradyunsg closed this Jul 22, 2018

pradyunsg reopened this Jul 22, 2018

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 24, 2018

cjerdonek mentioned this pull request Jul 25, 2018

Add split_auth_from_netloc() to misc.py #5657

Merged

orf mentioned this pull request Sep 10, 2018

Redact basic authentication passwords from log messages #5773

Merged

kvalev closed this Sep 10, 2018

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redact password from various log messages #5590

Redact password from various log messages #5590

kvalev commented Jul 10, 2018 •

edited

Loading

pradyunsg commented Jul 10, 2018

kvalev commented Jul 11, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

pradyunsg Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

pradyunsg Jul 12, 2018

kvalev Jul 12, 2018

pradyunsg Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek Jul 12, 2018

cjerdonek commented Jul 15, 2018

pradyunsg commented Jul 15, 2018

cjerdonek commented Jul 22, 2018

BrownTruck commented Jul 24, 2018

orf commented Sep 10, 2018

lock bot commented Jun 1, 2019

		@@ -0,0 +1 @@
		Redact username/password from log messages when installing packages from Git

Redact password from various log messages #5590

Redact password from various log messages #5590

Conversation

kvalev commented Jul 10, 2018 • edited Loading

pradyunsg commented Jul 10, 2018

kvalev commented Jul 11, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjerdonek commented Jul 15, 2018

pradyunsg commented Jul 15, 2018

cjerdonek commented Jul 22, 2018

BrownTruck commented Jul 24, 2018

orf commented Sep 10, 2018

lock bot commented Jun 1, 2019

kvalev commented Jul 10, 2018 •

edited

Loading