Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't override `Authorization` header when contents are bearer token (or any other token) #3929

Open
tomvlk opened this issue Mar 19, 2017 · 14 comments

Comments

@tomvlk
Copy link

commented Mar 19, 2017

I found out that the requests lib is overriding the authorization header when a netrc file is in place, which is awesome. But in some cases you won't want this at all, and is a design flaw imo.

For example you use a bearer token, it gets replaced by the user/password from netrc.

Also see the issue here: python-social-auth/social-core#43

@Lukasa

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

This behaviour can be overridden by trust_env, which allows you to instruct Requests to ignore the .netrc file. Is that sufficient for your purposes?

@tomvlk

This comment has been minimized.

Copy link
Author

commented Mar 19, 2017

Well, if you look at the issue of the social auth library, you may see that it's really nice to override the user/pass.
Short explanation about my situation related to social library:
I'm using a webservice that requires you to use the username/password HTTP authentication, for this I require the use of .netrc which is perfect. But on the same domain/path there is a oAuth2 endpoint that uses the Authorization header with the oAuth tokens.

If the .netrc feature/standard is only supporting username/password, why not replacing header when not containing a bearer or any other token. This could be done with checking the contents if the header is manually provided.

@Lukasa

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

So, I am open to that, but nervous about it. There are failure modes at either side, and trying to be "clever" may cause us to just be difficult to understand. The virtue of the model we have today is that it is very simple and consistent. So my question stands: do the functions currently available suffice for your use case?

@tomvlk

This comment has been minimized.

Copy link
Author

commented Mar 19, 2017

Well I agree on the fact that it's not really nice to change such behavior right now as it's always breaking something. And indeed the usage of the trust_env is a good option, but in this case the author of the library should give us an option to enable/disable it as a backend developer. Also the trust_env will disable more environmental settings or behavior and not only the netrc function, like proxy settings if I understood right.

Another option would be turning it on/off per request that overrides the session trust_env, or have another way to not override one specific header.

Anyway, the module is already trying to be "clever" by replacing the whole header 😃 . Which is great when you have full control over the Session.

@Lukasa

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

So it is quite possible that the library wrapping requests should be setting trust_env to False if it is handling headers itself.

@tomvlk

This comment has been minimized.

Copy link
Author

commented Mar 19, 2017

Yep, and that should be the case. But it remains that if you have mixed requests, like I have, it's kinda hard to manage. You need to have two sessions.

@Lukasa

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

Well, you don't really. You can just flip the setting around as-and-when you need it.

However, here's a framework I'd consider for handling auth in the 3.0 branch. I'd welcome a PR to make this the case.

  1. If the user sets an Authorization header themselves, either via the request or on the Session, we don't bother to look at the netrc file.
  2. If they didn't, we look at the netrc file for basic auth.
  3. If we get redirected, we fall back to only looking at the netrc file (which we already currently do).

Does that sound like a reasonable approach to your case?

@tomvlk

This comment has been minimized.

Copy link
Author

commented Mar 20, 2017

That sounds like a pretty clear way to solve this case. Only downside is that it can cause breaking code. But it's better to not force override when user manually given the details in my opinion.

I'm not sure if I'm capable of doing the PR myself due to time limits.

@fjarlq

This comment has been minimized.

Copy link

commented Apr 5, 2017

I encountered this problem when trying to figure out why the python-digitalocean module, which uses Requests, was failing due to an unexpected authentication error.

The root cause turned out to be this default directive in my $HOME/.netrc:

default login anonymous password anonymous@

which I used, many years ago, to automate my anonymous FTP logins.

I'm surprised that this directive in my .netrc would cause Requests to automatically override the authentication information that is being specified explicitly by python-digitalocean, especially since the directive is merely setting default login information that is used when accessing a host that lacks an explicit machine entry in .netrc.

So I like @Lukasa's idea above: when the caller specifies an Authorization header, I think the .netrc directives (whether default or machine) should be ignored. Thanks!

@lmazuel

This comment has been minimized.

Copy link

commented May 2, 2018

Got bitten by this one as well :(
To answer @Lukasa question:

do the functions currently available suffice for your use case?

I would say no, because trust_env is not only netrc, it's also REQUESTS_CA_BUNDLE for instance. Right now it's a little too much "take it or leave it" for the entire set of possible env stuff I could want to use :(

@tkdchen

This comment has been minimized.

Copy link

commented Aug 23, 2018

It would be nice to allow to disable reading from .netrc explicitly. Currently, it looks requests also handles an existing .netrc even if for requests.get which is expected to be a anonymous request in most cases.

@tkdchen

This comment has been minimized.

Copy link

commented Aug 23, 2018

How about provide a auth class something like NetrcAuth instead?

@danrue

This comment has been minimized.

Copy link

commented Nov 1, 2018

Coming here after spending several hours debugging an issue which ended up being the presence of a ~/.netrc file. This behavior violates POLA and should be explicitly enabled rather than enabled by default.

@bors-ltd

This comment has been minimized.

Copy link

commented Apr 23, 2019

I lost half a day because I could not log to production any more, and I couldn't find the issue in our infrastructure. Found out it was because I stored my password in ~/.netrc and requests read it and added an Authorization header when I was using a Bearer instead, and got rejected from the server.

It should only happen with an explicit BasicAuth().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.