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

ntlm authentication #3419

Closed
wants to merge 11 commits into from
Closed

ntlm authentication #3419

wants to merge 11 commits into from

Conversation

vmuriart
Copy link

@vmuriart vmuriart commented Jan 24, 2016

Adding ntlm for #1182. Other authentication libraries can be added the same way (ie, kerberos)
The import statement was placed inside the method to only import when needed, and not cause an ImportError if package isn't installed.

Review on Reviewable

Want to reuse the prompting options for ntlm.
@Ivoz
Copy link
Contributor

Ivoz commented Jan 24, 2016

  1. I think this is a good case for an extras_require decency, under say ntlm

  2. import should be at the top of the file, under a try/except (except case should give the intended import name a None value)

  3. attempted use of the option should either succeed or immediately give an error and exit (availability detected with the above None), telling user to e.g install pip[ntlm]. An exception stack trace should never bubble to the user

  4. needs accompanying entry in changelog

@classmethod
def authlib(cls):
# Place holder for Authentication Class
raise NotImplemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotImplementedError , NotImplemented is something else ☺

@vmuriart
Copy link
Author

Thank you @xavfernandez and @Ivoz for the feedback. Implemented the requested changes.

if HttpNtlmAuth is None:
raise InstallationError(
"Dependencies for Ntlm authentication are missing. Install "
"dependencies via the `pip install pip['ntlm']` command."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip[ntlm] should be fine (without the quotes)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! fix and matched it to the upgrade message.

@@ -198,6 +203,31 @@ def parse_credentials(self, netloc):
return userinfo, None
return None, None

@classmethod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since authlib is only called on an instance AFAICT, I'm not sure why it needs to be a class method?

You could make it a @property instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I intended to make it a class variable but had issues with the implementation and changed it a class method.
It can work as a function too; that is, no @property. is the advantage to avoid the extra pair of parenthesis self.authlib()(....) when it s being used?

@Ivoz
Copy link
Contributor

Ivoz commented Jan 25, 2016

We at least want some functional tests.

  1. If pip <blah> --ntlm-auth is called and HttpNtlmAuth is None / not installed we need to check we are indeed returning an error with a message
  2. If it is called, we want to check that requests is making a request with the correct headers

@vmuriart
Copy link
Author

I been thinking about the tests for this; but I'll admit that my knowledge on testing libraries is very limited and basic. My struggle would be on setting up tox to ensure the correct testing environments are being used. Would I need to setup the tests against every enviroment/combination?

That aside, for the second test, I don' think the headers would be added until it receives a 401 error from the host. I was planning on basing the tests of the test for HttpBasicAuth but there wasn't one available that i could find. I'll use requests-ntlm's tests as a template and go from there.

@Ivoz
Copy link
Contributor

Ivoz commented Jan 25, 2016

@vmuriart if you only have one or two Pythons installed, you can just run tox on the ones you have installed with tox --skip-missing-interpreters (or do one particular tox run with e.g tox -e py27). Just need to make sure whatever could you have is py2&3 compatible and passes pep8, and then then Travis CI will run the rest of the pythons for you when you push a commit.

or otherwise you can run just some particular tests using py.test -k <my_test_name>.

My mistake, 2nd test would probably be regarded unit (given functional is mostly testing pip run as a script).

You can pass in username/password with user:[email protected]. I would guess mocking a requests function at a particular point would be a way to go.

@vmuriart
Copy link
Author

Still trying to figure out how to write up the 2nd test. Not sure why i'm getting random fails on this last commit though. I'll take a look again tomorrow morning.

@Ivoz
Copy link
Contributor

Ivoz commented Jan 26, 2016

Look like obnoxious network errors, I restarted those builds for you.

@vmuriart vmuriart force-pushed the pip-ntlm branch 2 times, most recently from 42c843f to 17cf0ea Compare January 26, 2016 14:18
@vmuriart
Copy link
Author

As long as it wasn't me that broke it 😃
Thought about best way to implement the second test, without replicating requests-ntlm's tests. I think the alternate unit test I wrote is more relevant since it test's the functions that were added/changed.

Though I think an Integrated tests for authentications may be needed. There aren't any tests against HTTPBasicAuth. Though I did test it on my local machine manually for 2.7, 3.4, 3.5 to verify that they work.

@vmuriart vmuriart force-pushed the pip-ntlm branch 2 times, most recently from cfe3ca1 to d586349 Compare January 26, 2016 16:16
@rbtcollins
Copy link

I think this should be vendored, precisely as we vendor requests itself. Otherwise the usual version interactions that we vendor requests to avoid, will be able to occur between our vendored version and requests_ntlm. Worse, someone behind an NTLM proxy will have no way to use pip to install the ntlm extra without presupposing the dependency is already installed.

@vmuriart
Copy link
Author

Could that be decided on at a later date? Depending on the usage of it, it could be vendored at a later time.

@pfmoore
Copy link
Member

pfmoore commented Feb 1, 2016

As such I wrote this pr for requests-ntlm. This one uses windows-sspi (through pywin32) to get the username/password from the OS w.o having to prompt the user. It would require vendoring pywin32. On the plus side, it doesn't depend on python-ntlm3.

Vendoring pywin32 is not possible, as it uses a C extension (and it's huge!) Could your PR be written to use ctypes?

Next Steps: need to figure out why your setup isn't working. Not sure what the best way would be to work with you to diagnose this. I have a couple thoughts on what it may be.

I only have limited time to do tests (as I'm having to do this at work and it's not exactly core business activity :-)) so best is probably just put suggestions in the conversation here and I'll try stuff. It's slow, but I'll try to respond promptly (I'm on UK time, no idea how time differences may add to delays).

@vmuriart
Copy link
Author

vmuriart commented Feb 1, 2016

The most basic test I can think at the moment. Using requests and requests-ntlm are you able to access google.com?

@pfmoore
Copy link
Member

pfmoore commented Feb 1, 2016

Apparently not :-(

import requests
from requests_ntlm import HttpNtlmAuth

session = requests.Session()
session.auth = HttpNtlmAuth('DOMAIN\\USER','PASSWORD', session)
session.get('http://www.google.com')

Although this code (which I got from the requests-ntlm page) doesn't include me setting my proxy anywhere, so I guess it's probably not the right code to use :-(

Can you give me some sample code?

Output (for what it's worth - I think all it demonstrates is that without going through the proxy, I can't get to the internet):

Traceback (most recent call last):
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connection.py", line 137, in _new_conn
    (self.host, self.port), self.timeout, **extra_kw)
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\util\connection.py", line 91, in create_connection
    raise err
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\util\connection.py", line 81, in create_connection
    sock.connect(sa)
TimeoutError: [WinError 10060] A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connectionpool.py", line 559, in urlopen
    body=body, headers=headers)
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connectionpool.py", line 353, in _make_request
    conn.request(method, url, **httplib_request_kw)
  File "...\AppData\Local\Programs\Python\Python35\lib\http\client.py", line 1083, in request
    self._send_request(method, url, body, headers)
  File "...\AppData\Local\Programs\Python\Python35\lib\http\client.py", line 1128, in _send_request
    self.endheaders(body)
  File "...\AppData\Local\Programs\Python\Python35\lib\http\client.py", line 1079, in endheaders
    self._send_output(message_body)
  File "...\AppData\Local\Programs\Python\Python35\lib\http\client.py", line 911, in _send_output
    self.send(msg)
  File "...\AppData\Local\Programs\Python\Python35\lib\http\client.py", line 854, in send
    self.connect()
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connection.py", line 162, in connect
    conn = self._new_conn()
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connection.py", line 146, in _new_conn
    self, "Failed to establish a new connection: %s" % e)
requests.packages.urllib3.exceptions.NewConnectionError: <requests.packages.urllib3.connection.HTTPConnection object at 0x0000000003513710>: Failed to establish a new connection: [WinError 10060] A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\adapters.py", line 376, in send
    timeout=timeout
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\connectionpool.py", line 609, in urlopen
    _stacktrace=sys.exc_info()[2])
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\packages\urllib3\util\retry.py", line 273, in increment
    raise MaxRetryError(_pool, url, error or ResponseError(cause))
requests.packages.urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='www.google.com', port=80): Max retries exceeded with url: / (Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x0000000003513710>: Failed to establish a new connection: [WinError 10060] A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond',))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".\req.py", line 6, in <module>
    session.get('http://www.google.com')
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\sessions.py", line 480, in get
    return self.request('GET', url, **kwargs)
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\sessions.py", line 468, in request
    resp = self.send(prep, **send_kwargs)
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\sessions.py", line 576, in send
    r = adapter.send(request, **kwargs)
  File "C:\Work\Scratch\pip-pip-ntlm\vv\lib\site-packages\requests\adapters.py", line 437, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPConnectionPool(host='www.google.com', port=80): Max retries exceeded with url: / (Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x0000000003513710>: Failed to establish a new connection: [WinError 10060] A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond',))

@pfmoore
Copy link
Member

pfmoore commented Feb 1, 2016

Never mind, I worked it out. Add the proxy data as per http://docs.python-requests.org/en/latest/user/advanced/#proxies and I get a successful response back from Google.

@vmuriart
Copy link
Author

vmuriart commented Feb 1, 2016

progress! did it require it to use the requests-ntlm or was the proxy enough to go through? My case is the latter, so just want to verify your copy requires ntlm to get through.
If that was the case, the pip version might be missing passing the proxy parameters as well.

@Ivoz
Copy link
Contributor

Ivoz commented Feb 1, 2016

@pfmoore unfortunately bundling ntlmlib would require bringing in pycrypto as well...

I'm wondering if telling people to use something like Cntlm proxy which acts as a... proxy-proxy! :D is just easier. It's windows software they can just install.

@vmuriart
Copy link
Author

vmuriart commented Feb 1, 2016

@Ivoz Looks like Cntlm hasnt been updated in a while; though probably still works. I know atleast in my case, it might raise some flags with IT and would discourage some less tech savvy users from using python within our network.

  • In order to bundle it, looks like an implementation of ntlm needs to be rewritten and supported (ie, lots of work)
  • Merging the code as is fixes ntlm authentication for some users, in particular those that wouldn't be developers, but rather end users and it would be a one time install. Drawback would be some users expecting ntlm authentication working out of the box
  • Rewrite pr to instead implement a plug-n-play type thingy file. The user can pass an "authentication file" that would essentially be a requests Session or request-auth object that would then be combined with the pipSession.

I'm starting to incline towards the last option just because it keeps the authentication methods/code separate from pip and back to their respective libraries. Otherwise there might be issues being opened for bugs on the ntlm code that would be otherwise unrelated to pip.

@vmuriart
Copy link
Author

vmuriart commented Feb 2, 2016

Would an MIT License work ? I found this project that implemented their own ntlm module.

@vmuriart
Copy link
Author

vmuriart commented Feb 2, 2016

Scratch that. The sspi module is ok, the ntlm module was copied from python-ntlm and kept the gnu license

@pfmoore
Copy link
Member

pfmoore commented Feb 2, 2016

I'm wondering if telling people to use something like Cntlm proxy which acts as a... proxy-proxy! :D is just easier. It's windows software they can just install.

As @vmuriart this would likely raise flags with the security people. It basically means you have a proxy running on your PC that allows web access under your username. It runs as a service, so it's always running rather than just working for pip. Personally, I find that cntlm is a case of a cure being worse than the disease, unfortunately. (OTOH, how easy would it be to write a small proxy in pure Python with requests and requests_ntlm? If the code for that is simple enough, and it could be written to proxy only requests from pip, say by looking at the user agent string, then maybe that would be worthwhile workaround for "advanced" users, as it's totally in their control)

BTW, @vmuriart have you checked whether the python-ntlm3 author would be willing to relicense? If she would, I still think that's the best option.

@vmuriart
Copy link
Author

vmuriart commented Feb 2, 2016

I haven't checked yet. Though it was my understanding that w this license
it cascades down, so it would need to be changed on its parent projects as
well.

On Tuesday, February 2, 2016, Paul Moore [email protected] wrote:

I'm wondering if telling people to use something like Cntlm proxy which
acts as a... proxy-proxy! :D is just easier. It's windows software they can
just install.

As @vmuriart https://github.com/vmuriart this would likely raise flags
with the security people. It basically means you have a proxy running on
your PC that allows web access under your username. It runs as a service,
so it's always running rather than just working for pip. Personally, I find
that cntlm is a case of a cure being worse than the disease, unfortunately.
(OTOH, how easy would it be to write a small proxy in pure Python with
requests and requests_ntlm? If the code for that is simple enough, and it
could be written to proxy only requests from pip, say by looking at the
user agent string, then maybe that would be worthwhile workaround for
"advanced" users, as it's totally in their control)

BTW, @vmuriart https://github.com/vmuriart have you checked whether the
python-ntlm3 author would be willing to relicense? If she would, I still
think that's the best option.


Reply to this email directly or view it on GitHub
#3419 (comment).

@Ivoz
Copy link
Contributor

Ivoz commented Feb 2, 2016

@pfmoore python-ntlm3 is a fork, and as such not only would the current maintainer need to agree to relicensing, so would the original author and contributors, of which there are a few.

It basically means you have a proxy running on your PC that allows web access under your username.

True, but only accessible by localhost. Not suddenly as a proxy broadcasting to the rest of the world. ntlmaps is another proxy with the same idea as Cntml, but written in python. OTOH, I have no idea why you think changing the language the intermediating proxy is in suddenly makes things ok.

@pfmoore
Copy link
Member

pfmoore commented Feb 2, 2016

True, but only accessible by localhost. Not suddenly as a proxy broadcasting to the rest of the world.

I wasn't aware of that fact, but I'm not 100% sure our security people would consider that a sufficient restriction.

ntlmaps is another proxy with the same idea as Cntml, but written in python. OTOH, I have no idea why you think changing the language the intermediating proxy is in suddenly makes things ok.

There were two points:

  1. cntlm is a service, and hence "always on" (modulo the hassle of stopping and starting the service). A user-level process is more likely to be something that's only run when needed. That's not about language, just about the design of cntlm not being ideal for my use case.
  2. I asked if the code would be "simple enough" - the idea being that if it's simple enough (as a result of deferring the hard bits to requests/requests_ntlm) then tailoring it to (say) only proxy PyPI, or only act as a proxy for pip, would be practical. Not for "end users", but for developers like me willing to use it as a workaround.

But any locally-run proxy is likely to be a non-starter in my environment. For a long time I've been running the Forefront TMG Client, which works much like cntlm, but changes behind the scenes (that I'm not clear on, and have no way of finding out about) have resulted in that no longer working. Hence my interest in "native" NTLM support.

Having said that, curl --proxy ... --proxy-ntlm --proxy-user ... https://pypi.python.org/pypi -I is currently failing for me with 502 Proxy Error ( Forefront TMG denied the specified Uniform Resource Locator (URL). ), which makes me think that PyPI is now being actively blocked by the proxy, so the question has suddenly become moot for me :-(

I think it's fair to say that my access to PyPI is no longer a good test case for this change - and the change is unlikely to help me anyway. Beyond that, my woes with corporate security rules are probably of little interest to anyone here :-)

To summarise my position for the record:

  • I'm generally in favour of handling NTLM (and any other authentication methods people might need)
  • I think that anything we add should be vendored in, just like any other dependency pip uses.
  • Clearly there are a variety of "corporate proxy" scenarios - mine (proxy manages internet access) is very different from yours (open internet, proxy needed for internal resources).
  • Ideally, we need a better picture of what scenarios exist, and how many people are affected. Otherwise it's pretty much impossible to judge the cost/benefit of a PR like this. Getting those figures is unlikely to be practical, though.

@vmuriart
Copy link
Author

vmuriart commented Feb 3, 2016

  • I think that anything we add should be vendored in, just like any other dependency pip uses.
  • Clearly there are a variety of "corporate proxy" scenarios - mine (proxy manages internet access) is very different from yours (open internet, proxy needed for internal resources).
  • Ideally, we need a better picture of what scenarios exist, and how many people are affected. Otherwise it's pretty much impossible to judge the cost/benefit of a PR like this. Getting those figures is unlikely to be practical, though.

I think all three points speak to why it needs to be vendored. #1182 describes the scenarios that most users that need ntlm have faced. While I focused on solving the 401 error, 407 should work as well with pip install foo_bar2 --auth_ntlm --proxy something.foo.com:8080

I'll try to find another ntlm library, but for now sounds like we are at standstill.

@Ivoz
Copy link
Contributor

Ivoz commented Feb 3, 2016

Cntlm can be run as a standard exe; it is not limited to only running as an always-on service.

@pfmoore
Copy link
Member

pfmoore commented Feb 3, 2016

Can it? I didn't notice that option when I last tried it (it installed a service immediately from what I recall). I'll keep that in mind if I need to try it out as a possible workaround in future.

@miketrim
Copy link

@pfmoore I find that with my corporate firewall, curl can't get through if I specify the username explicitly, buy it works if I use the single colon option as per the manpage, ie curl --proxy ... --proxy-ntlm --proxy-user : url. This apparently uses SSPI to pick up the username and password from the environment.

@dstufft dstufft closed this May 18, 2016
@dstufft
Copy link
Member

dstufft commented May 18, 2016

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck
Copy link
Contributor

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master.

If you do nothing, this Pull Request will be automatically migrated by @BrownTruck for you.

If you would like to retain control over this pull request then you should resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to migrate this Pull Request yourself, here is an example message that you can copy and paste:

Adding ntlm for #1182. Other authentication libraries can be added the same way (ie, kerberos)
The import statement was placed inside the method to only import when needed, and not cause an ImportError if package isn't installed.

---

*This was migrated from pypa/pip#3419 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

If this pull request is no longer needed, please feel free to close it.

@BrownTruck
Copy link
Contributor

This Pull Request has been automatically migrated to #3731 to reparent it to the master branch. Please see the new pull request for any new discussion.

@BrownTruck BrownTruck closed this May 26, 2016
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants