Repopulate ~/.netrc auth. #1892

Merged
merged 3 commits into from Jan 31, 2014

Conversation

Projects
None yet
5 participants
@Lukasa
Member

Lukasa commented Jan 29, 2014

This should be a fix for #1885.

@sigmavirus24, can you give me some code review? =)

requests/sessions.py
+ original_parsed = urlparse(resp.request.url)
+ redirect_parsed = urlparse(url)
+
+ if original_parsed.hostname != redirect_parsed.hostname:

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 30, 2014

Member

What about:

if (original_parsed.hostname != redirect_parsed.hostname and 
        'Authorization' in headers):
    del headers['Authorization']
@sigmavirus24

sigmavirus24 Jan 30, 2014

Member

What about:

if (original_parsed.hostname != redirect_parsed.hostname and 
        'Authorization' in headers):
    del headers['Authorization']
@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 30, 2014

Member

One comment otherwise LGTM.

You don't need to write a test for this, but I have an idea that I'd like to test it with.

Member

sigmavirus24 commented Jan 30, 2014

One comment otherwise LGTM.

You don't need to write a test for this, but I have an idea that I'd like to test it with.

@Lukasa

This comment has been minimized.

Show comment Hide comment
@Lukasa

Lukasa Jan 30, 2014

Member

Done and done.

Member

Lukasa commented Jan 30, 2014

Done and done.

@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 30, 2014

Member

:shipit:

Member

sigmavirus24 commented Jan 30, 2014

:shipit:

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jan 30, 2014

Member

I need to think about this.

Member

kennethreitz commented Jan 30, 2014

I need to think about this.

@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 30, 2014

Member

@kennethreitz we absolutely cannot continue reusing authorizations on redirects to sites that are not the same host. With that in mind we almost certainly need to issue a CVE. I'll happily work on that though.

We've been leaking credentials and we need to at least address that. Whether we repopulate the auth after stopping the leak or not is more of a feature decision. I'm sure one of the security experts, like @dstufft would back up @Lukasa and I on that.

Member

sigmavirus24 commented Jan 30, 2014

@kennethreitz we absolutely cannot continue reusing authorizations on redirects to sites that are not the same host. With that in mind we almost certainly need to issue a CVE. I'll happily work on that though.

We've been leaking credentials and we need to at least address that. Whether we repopulate the auth after stopping the leak or not is more of a feature decision. I'm sure one of the security experts, like @dstufft would back up @Lukasa and I on that.

@defaltd

This comment has been minimized.

Show comment Hide comment
@defaltd

defaltd Jan 31, 2014

@kennethreitz , call me the outsider but @sigmavirus24 got a point. As the new guy 'round these parts. You have 1.1million downloads. Even if half that still use, we are talking about a major security failure in the code base. 🙅

defaltd commented Jan 31, 2014

@kennethreitz , call me the outsider but @sigmavirus24 got a point. As the new guy 'round these parts. You have 1.1million downloads. Even if half that still use, we are talking about a major security failure in the code base. 🙅

@Lukasa

This comment has been minimized.

Show comment Hide comment
@Lukasa

Lukasa Jan 31, 2014

Member

I'm happy to remove the repopulation of ~/.netrc, but the only reason this shouldn't get merged I just fixed (I wasn't respecting Session.trust_env).

Member

Lukasa commented Jan 31, 2014

I'm happy to remove the repopulation of ~/.netrc, but the only reason this shouldn't get merged I just fixed (I wasn't respecting Session.trust_env).

@defaltd

This comment has been minimized.

Show comment Hide comment
@defaltd

defaltd Jan 31, 2014

@Lukasa So in that sense, are you suggesting that the session itself may be invalidated and then a restart is required?

defaltd commented Jan 31, 2014

@Lukasa So in that sense, are you suggesting that the session itself may be invalidated and then a restart is required?

@Lukasa

This comment has been minimized.

Show comment Hide comment
@Lukasa

Lukasa Jan 31, 2014

Member

No. =)

All I was saying is that the previous version of this fix didn't respect the flag that controls whether we should look at ~/.netrc. Now it does.

The barest minimum we have to do is remove Authorization headers on redirects to new hosts. That's pretty much mandatory. Everything else is gravy.

Member

Lukasa commented Jan 31, 2014

No. =)

All I was saying is that the previous version of this fix didn't respect the flag that controls whether we should look at ~/.netrc. Now it does.

The barest minimum we have to do is remove Authorization headers on redirects to new hosts. That's pretty much mandatory. Everything else is gravy.

@defaltd

This comment has been minimized.

Show comment Hide comment
@defaltd

defaltd Jan 31, 2014

Oh well alrighty then! :) 👍

defaltd commented Jan 31, 2014

Oh well alrighty then! :) 👍

@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 31, 2014

Member

@Sirenity it isn't a big deal but requests should have far more than 8 million downloads last I checked (we're probably upwards of 9 million) and that's only from PyPI. We have no way of measuring how many downloads installations came from distribution repositories (i.e., people installing aptitude, yum, or pacman).

Member

sigmavirus24 commented Jan 31, 2014

@Sirenity it isn't a big deal but requests should have far more than 8 million downloads last I checked (we're probably upwards of 9 million) and that's only from PyPI. We have no way of measuring how many downloads installations came from distribution repositories (i.e., people installing aptitude, yum, or pacman).

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jan 31, 2014

Member

@sigmavirus24 this was an explicit design decision, and it has been stated as such many times before. I've considered implementing a patch much like many times before, and this was a minor concern in the back of my mind when I did "the big refactor". As I said, I need to think about it.

Further comments about are neither helpful nor welcome. :)

Member

kennethreitz commented Jan 31, 2014

@sigmavirus24 this was an explicit design decision, and it has been stated as such many times before. I've considered implementing a patch much like many times before, and this was a minor concern in the back of my mind when I did "the big refactor". As I said, I need to think about it.

Further comments about are neither helpful nor welcome. :)

@dstufft

This comment has been minimized.

Show comment Hide comment
@dstufft

dstufft Jan 31, 2014

Contributor

Oof, I just noticed this. This really should change FWIW, carrying authentication data across access boundaries is a bad idea generally. It'd be up to Mitre but a CVE is probably appropriate as well. If you do get a CVE and you do change this, include the CVE number in the changelog please :]

Contributor

dstufft commented Jan 31, 2014

Oof, I just noticed this. This really should change FWIW, carrying authentication data across access boundaries is a bad idea generally. It'd be up to Mitre but a CVE is probably appropriate as well. If you do get a CVE and you do change this, include the CVE number in the changelog please :]

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jan 31, 2014

Member

This simple patch is not a technical change, but a fairly large philosophical one. Traditionally, I have recommended that all security-concious users disable redirection support, and follow them on their own. This patch would make security the something that Requests does for you (something that I already did for the Python world when I verified SSL by default). This would take it to the next level.

I'm not judging it as either good or bad. I just need need to think about it.

Again, no further input needed. Will be back to you shortly.

Member

kennethreitz commented Jan 31, 2014

This simple patch is not a technical change, but a fairly large philosophical one. Traditionally, I have recommended that all security-concious users disable redirection support, and follow them on their own. This patch would make security the something that Requests does for you (something that I already did for the Python world when I verified SSL by default). This would take it to the next level.

I'm not judging it as either good or bad. I just need need to think about it.

Again, no further input needed. Will be back to you shortly.

kennethreitz added a commit that referenced this pull request Jan 31, 2014

@kennethreitz kennethreitz merged commit f1893c8 into requests:master Jan 31, 2014

1 check passed

default Merged build finished.
Details

@Lukasa Lukasa deleted the Lukasa:netrcauth branch Jan 31, 2014

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jan 31, 2014

Member

Also, if we could refactor this, it would be great. You ruined my beautiful code. :)

Member

kennethreitz commented Jan 31, 2014

Also, if we could refactor this, it would be great. You ruined my beautiful code. :)

@dstufft

This comment has been minimized.

Show comment Hide comment
@dstufft

dstufft Jan 31, 2014

Contributor

\o/

On Jan 31, 2014, at 12:21 PM, Kenneth Reitz notifications@github.com wrote:

Also, if we could refactor this, it would be great. You ruined my beautiful code. :)


Reply to this email directly or view it on GitHub.

Contributor

dstufft commented Jan 31, 2014

\o/

On Jan 31, 2014, at 12:21 PM, Kenneth Reitz notifications@github.com wrote:

Also, if we could refactor this, it would be great. You ruined my beautiful code. :)


Reply to this email directly or view it on GitHub.

@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 31, 2014

Member

I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug.

Member

sigmavirus24 commented Jan 31, 2014

I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug.

@dstufft

This comment has been minimized.

Show comment Hide comment
@dstufft

dstufft Jan 31, 2014

Contributor

What is the CVE? Useful for discussion even prior to release.

On Jan 31, 2014, at 12:40 PM, Ian Cordasco notifications@github.com wrote:

I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug.


Reply to this email directly or view it on GitHub.

Contributor

dstufft commented Jan 31, 2014

What is the CVE? Useful for discussion even prior to release.

On Jan 31, 2014, at 12:40 PM, Ian Cordasco notifications@github.com wrote:

I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug.


Reply to this email directly or view it on GitHub.

@sigmavirus24

This comment has been minimized.

Show comment Hide comment
@sigmavirus24

sigmavirus24 Jan 31, 2014

Member

For this one: CVE-2014-1829

Edit Just got it from MITRE this morning so it may not show up in searches yet.

Member

sigmavirus24 commented Jan 31, 2014

For this one: CVE-2014-1829

Edit Just got it from MITRE this morning so it may not show up in searches yet.

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jan 31, 2014

Member

I cleaned up the ugly code. :P

Member

kennethreitz commented Jan 31, 2014

I cleaned up the ugly code. :P

@Lukasa

This comment has been minimized.

Show comment Hide comment
@Lukasa

Lukasa Jan 31, 2014

Member

We've learned a valuable lesson today: the easiest way to get Kenneth to write more code is to put really ugly code in front of him and let his natural instincts take over. ;)

Member

Lukasa commented Jan 31, 2014

We've learned a valuable lesson today: the easiest way to get Kenneth to write more code is to put really ugly code in front of him and let his natural instincts take over. ;)

@Lukasa Lukasa referenced this pull request in requests/requests-oauthlib Apr 22, 2014

Merged

Re-authorize requests upon redirect. #118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment