Use SNI for ssl connections #109

ojomio · 2018-10-10T18:37:34Z

No description provided.

…t, `wrap_socket()` does not allow this If explicit `server_hostname` ssl option is specified, it is used as SNI, otherwise the content of the `Host` HTTP header or the host from the requested URL

ojomio · 2018-10-15T07:11:20Z

Does anybody want to get it merged?

ml31415 · 2018-10-18T15:39:02Z

Plese fix all existing test cases, and also add test cases for the things you intend to fix with your pull request. Also, avoiding further external dependencies would be great.

ojomio · 2018-10-18T16:43:56Z

At the moment I created this PR, all tests passed, however, it seems, that google changed
something and event master fails:


py36 runtests: commands[4] | py.test src/geventhttpclient/tests
=========================================================================================== test session starts ============================================================================================
platform linux -- Python 3.6.5, pytest-3.8.2, py-1.6.0, pluggy-0.7.1
rootdir: /home/crystal2/github/geventhttpclient, inifile:
collected 85 items                                                                                                                                                                                         

src/geventhttpclient/tests/test_client.py FF.F....FF.F........                                                                                                                                       [ 23%]
src/geventhttpclient/tests/test_headers.py .............                                                                                                                                             [ 38%]
src/geventhttpclient/tests/test_httplib.py ...                                                                                                                                                       [ 42%]
src/geventhttpclient/tests/test_keep_alive.py .....                                                                                                                                                  [ 48%]
src/geventhttpclient/tests/test_network_failures.py .........                                                                                                                                        [ 58%]
src/geventhttpclient/tests/test_no_module_ssl.py ..                                                                                                                                                  [ 61%]
src/geventhttpclient/tests/test_parser.py .......                                                                                                                                                    [ 69%]
src/geventhttpclient/tests/test_ssl.py ....                                                                                                                                                          [ 74%]
src/geventhttpclient/tests/test_url.py ..............                                                                                                                                                [ 90%]
src/geventhttpclient/tests/test_useragent.py ........                                                                                                                                                [100%]

================================================================================================= FAILURES =================================================================================================
____________________________________________________________________________________________ test_client_simple ____________________________________________________________________________________________

    def test_client_simple():
        client = HTTPClient('www.google.fr')
        assert client.port == 80
        response = client.get('/')
>       assert response.status_code == 200
E       AssertionError: assert 302 == 200
E        +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'http://www.google.com/sorry/index?continue=http://www.google....html; charset=UTF-8', 'server': 'HTTP server (unknown)', 'content-length': '324', 'x-xss-protection': '1; mode=block'}>.status_code

src/geventhttpclient/tests/test_client.py:41: AssertionError
____________________________________________________________________________________ test_client_without_leading_slash _____________________________________________________________________________________

    def test_client_without_leading_slash():
        client = HTTPClient('www.google.fr')
        with client.get("") as response:
>           assert response.status_code == 200
E           AssertionError: assert 302 == 200
E            +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'http://www.google.com/sorry/index?continue=http://www.google....html; charset=UTF-8', 'server': 'HTTP server (unknown)', 'content-length': '324', 'x-xss-protection': '1; mode=block'}>.status_code

src/geventhttpclient/tests/test_client.py:48: AssertionError
________________________________________________________________________________________ test_request_with_headers _________________________________________________________________________________________

    def test_request_with_headers():
        client = HTTPClient('www.google.fr')
        response = client.get('/', headers=test_headers)
>       assert response.status_code == 200
E       AssertionError: assert 302 == 200
E        +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'http://www.google.com/sorry/index?continue=http://www.google....html; charset=UTF-8', 'server': 'HTTP server (unknown)', 'content-length': '324', 'x-xss-protection': '1; mode=block'}>.status_code

src/geventhttpclient/tests/test_client.py:59: AssertionError
______________________________________________________________________________________ test_response_context_manager _______________________________________________________________________________________

    def test_response_context_manager():
        client = HTTPClient.from_url('http://www.google.fr/')
        r = None
        with client.get('/') as response:
>           assert response.status_code == 200
E           AssertionError: assert 302 == 200
E            +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'http://www.google.com/sorry/index?continue=http://www.google....html; charset=UTF-8', 'server': 'HTTP server (unknown)', 'content-length': '324', 'x-xss-protection': '1; mode=block'}>.status_code

src/geventhttpclient/tests/test_client.py:84: AssertionError
_____________________________________________________________________________________________ test_client_ssl ______________________________________________________________________________________________

    @pytest.mark.skipif(
        os.environ.get("TRAVIS") == "true",
        reason="We have issues on travis with the SSL tests"
    )
    def test_client_ssl():
        client = HTTPClient('www.google.fr', ssl=True)
        assert client.port == 443
        response = client.get('/')
>       assert response.status_code == 200
E       assert 302 == 200
E        +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'https://www.google.com/sorry/index?continue=https://www.googl..., 'content-length': '326', 'x-xss-protection': '1; mode=block', 'alt-svc': 'quic=":443"; ma=2592000; v="44,43,39,35"'}>.status_code

src/geventhttpclient/tests/test_client.py:96: AssertionError
_____________________________________________________________________________________ test_multi_queries_greenlet_safe _____________________________________________________________________________________

    def test_multi_queries_greenlet_safe():
        client = HTTPClient('www.google.fr', concurrency=3)
        group = gevent.pool.Group()
        event = gevent.event.Event()
    
        def run(i):
            event.wait()
            response = client.get('/')
            return response, response.read()
    
        count = 0
    
        gevent.spawn_later(0.2, event.set)
        for response, content in group.imap_unordered(run, xrange(5)):
>           assert response.status_code == 200
E           AssertionError: assert 302 == 200
E            +  where 302 = <HTTPSocketPoolResponse status=302 headers={'location': 'http://www.google.com/sorry/index?continue=http://www.google....html; charset=UTF-8', 'server': 'HTTP server (unknown)', 'content-length': '324', 'x-xss-protection': '1; mode=block'}>.status_code

src/geventhttpclient/tests/test_client.py:127: AssertionError
=================================================================================== 6 failed, 79 passed in 6.57 seconds ====================================================================================
ERROR: InvocationError: '/home/crystal2/github/geventhttpclient/.tox/py36/bin/py.test src/geventhttpclient/tests'
_________________________________________________________________________________________________ summary __________________________________________________________________________________________________
ERROR:   py27: commands failed
ERROR:   py35: InterpreterNotFound: python3.5
ERROR:   py36: commands failed

And I 'm bit confused about why you asked

test cases for the things you intend to fix with your pull request

because that is what I added to test_ssl.py. If that is not enough, could you please point what I do not cover with them?

ml31415 · 2018-10-19T06:18:11Z

test cases for the things you intend to fix with your pull request

because that is what I added to test_ssl.py. If that is not enough, could you please point what I do not cover with them?

Sorry, my fault, had missed that when I briefly looked over it. Didn't go through it thoroughly, as the tests were still failing. I suggest to replace google with http://httpbin.org/ if the issues remain, for all occurences of google in the tests.

ojomio · 2018-10-19T06:44:06Z

Ok, here is the update.
I had to use github.com instead of httpbin.org for ssl test, apparently, because httpbin.org requires SNI, and it only works if context factory is in use (bare wrap_socket lacks this functionality), but i didnt want to make the test code more complex than needed

ml31415 · 2018-10-19T07:17:51Z

About dpkt, if I see it correctly, it's merely using the functions tls_multi_factory and parse_variable_array and the unpacking of the data field of TLSHandshake, all together maybe 30 lines of code to replicate. I'd rather prefer to simply copy that functionality into our ssl handling than having an external dependency. I'm aware that there are ups and downs to that approach, but imho it's the more hazzle-free variant in the long run.

ojomio · 2018-10-19T09:16:35Z

hmm, I'll see if I manage to extract it on the weekend. this is dev Dep anyway, so I thought it wouldn't be that bad

ml31415 · 2018-10-19T13:17:33Z

Sure. And thanks already for contributing to our little project here!

ojomio · 2018-10-19T16:20:04Z

It seems there is more than 30 lines, as it makes use of of TLSRecord, which inherits from base Packet class , so it would be cumbersome to try to extract all this machinery from the dpkt.
We may try to parse it manually, but i prefer to use the tools available

gwik

Thanks for this contribution. Supporting SNI is a must to keep this library alive.
I'm confused by the failing CI, has dpkt introduced a breaking change ?
I will gratefully merge this as soon as the CI is green.
For the added dependency don't copy source code. I understand Michael's point of view but I don't think it worth for a test dep.

gwik · 2018-10-21T20:03:59Z

src/geventhttpclient/client.py

+                 insecure=False,
+                 proxy_host=None, proxy_port=None, version=HTTP_11,
+                 headers_type=Headers):
+        if headers is None:


I don't get this change, what's the point ? I'm out of the python world for some time now...

It's about this https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments for headers

ojomio · 2018-10-22T07:24:22Z

Apparently, Travis did not install the dev requirements, I tried to correct it in the config, but someone probably need to actually install it

ojomio · 2018-10-23T13:37:34Z

would anybody please try to install the travis script if I'm right about the cause of the error?

ml31415 · 2018-10-23T16:31:51Z

setup.py

-      install_requires=requirements)
+      install_requires=requirements,
+      extras_require={
+          'dev': get_requirements('requirements-dev.txt'),


Does this handle the "-r requirements.txt" correctly?

ml31415 · 2018-10-23T16:34:25Z

.travis.yml

@@ -6,7 +6,7 @@ python:
  - "3.5"
  - "3.6"
 install:
-  - "pip install ."
+  - "pip install .[dev]"


How about

- pip install -r requirements-dev.txt - pip install .

Can't find a proper documentation for this [] syntax right now. Keeping simple things simple, and that should work in any case

Also

before_install: - pip install -U pip

might be an option, in case the 9.0.1 on Travis is not recent enough for that extras_require syntax.

ojomio · 2018-10-23T21:21:46Z

I have no idea why the travis build hangs, tox on my machine feels good:

=========================================================================================== test session starts ============================================================================================
platform linux -- Python 3.6.5, pytest-3.9.2, py-1.7.0, pluggy-0.8.0
rootdir: /home/crystal2/github/geventhttpclient, inifile:
collected 88 items                                                                                                                                                                                         

src/geventhttpclient/tests/test_client.py ....................                                                                                                                                       [ 22%]
src/geventhttpclient/tests/test_headers.py .............                                                                                                                                             [ 37%]
src/geventhttpclient/tests/test_httplib.py ...                                                                                                                                                       [ 40%]
src/geventhttpclient/tests/test_keep_alive.py .....                                                                                                                                                  [ 46%]
src/geventhttpclient/tests/test_network_failures.py .........                                                                                                                                        [ 56%]
src/geventhttpclient/tests/test_no_module_ssl.py ..                                                                                                                                                  [ 59%]
src/geventhttpclient/tests/test_parser.py .......                                                                                                                                                    [ 67%]
src/geventhttpclient/tests/test_ssl.py .......                                                                                                                                                       [ 75%]
src/geventhttpclient/tests/test_url.py ..............                                                                                                                                                [ 90%]
src/geventhttpclient/tests/test_useragent.py ........                                                                                                                                                [100%]

======================================================================================== 88 passed in 8.35 seconds =========================================================================================
_________________________________________________________________________________________________ summary __________________________________________________________________________________________________
  py27: commands succeeded
ERROR:   py35: InterpreterNotFound: python3.5
  py36: commands succeeded

ml31415 · 2018-10-24T11:09:58Z

Did you try it with the same dependency versions like on travis? Just to exclude issues with them.

certifi-2018.10.15
dpkt-1.9.1
gevent-1.3.7
greenlet-0.4.15

ojomio · 2018-10-24T11:13:31Z

I now think this is somehow related with hostname resolving, I 'll try to mock it, because I currently rely on gethostbyaddr and it may return something that may not resolve back to localhost

ml31415 · 2018-10-24T11:30:19Z

Just for the record, tox also runs fine on my machine. You might try to set -v and -s for pytest, to get some more details into the travis log, and temporarily throw in some debug prints into the test case. It might hang in some other place you didn't expect, some gevent loop story or whatever.

ojomio · 2018-10-25T05:09:42Z

I think that is it

ml31415 · 2018-10-25T14:06:50Z

Looks good to me. @gwik ?

gwik · 2018-10-25T14:47:28Z

LGTM. Thx !

…

On Thu 25 Oct 2018 at 16:07, Michael ***@***.***> wrote: Looks good to me. @gwik <https://github.com/gwik> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#109 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAArmgW8XHTan5gBSJYsiM8W0-Xzrb53ks5uocWLgaJpZM4XWFB9> .

ojomio · 2018-10-25T15:51:36Z

It was tough, thanks)

ml31415 · 2018-10-25T16:00:52Z

Just went through some older bug reports to see if they were related and possibly fixed. Trying

from geventhttpclient.useragent import UserAgent
ua.urlopen('https://bison.streethawk.com/').read()
>>> CertificateError: hostname 'bison.streethawk.com' doesn't match either of '*.pointzi.com', 'pointzi.com'

Chrome and Firefox seem to have no issues with that site and get a correct certificate. Might be worth to have another look I guess.

ojomio · 2018-10-25T18:15:03Z

In [11]: ua = UserAgent( ssl_context_factory=gevent.ssl.create_default_context)

In [12]: ua.urlopen('https://bison.streethawk.com/').read()
Out[12]: '<!doctype html>\r\n<html lang="en">\r\n<head>\r\n ...

I'm not hyper experienced with SSL and don't know why we shouldn't make this context default context , but without context there is no way to use SNI, because gevent/_ssl3.py:

def wrap_socket(sock, keyfile=None, certfile=None,
                server_side=False, cert_reqs=CERT_NONE,
                ssl_version=PROTOCOL_SSLv23, ca_certs=None,
                do_handshake_on_connect=True,
                suppress_ragged_eofs=True,
                ciphers=None):

    return SSLSocket(sock=sock, keyfile=keyfile, certfile=certfile,
                     server_side=server_side, cert_reqs=cert_reqs,
                     ssl_version=ssl_version, ca_certs=ca_certs,
                     do_handshake_on_connect=do_handshake_on_connect,
                     suppress_ragged_eofs=suppress_ragged_eofs,
                     ciphers=ciphers)

ml31415 · 2018-10-25T19:34:34Z

Me neither, but if that context is necessary for SNI to work, it should be set. Or is there any harmful side effect I'd be missing?

ojomio and others added 4 commits October 10, 2018 22:27

SNI extension is sent whenever SSLContext factory is set. If it is no…

5e5da16

…t, `wrap_socket()` does not allow this If explicit `server_hostname` ssl option is specified, it is used as SNI, otherwise the content of the `Host` HTTP header or the host from the requested URL

ssl options in HTTPClient not mutated(as well as headers)

d47ad55

try/except in test_ssl

06f948d

compatibility fixes

e626b90

Fix client tests

b61a18f

precise SSL error

5bf1dac

gwik reviewed Oct 21, 2018

View reviewed changes

Use install .[dev] for tests

7e2de78

ojomio force-pushed the SNI branch from 54b235e to 7e2de78 Compare October 22, 2018 07:11

ml31415 reviewed Oct 23, 2018

View reviewed changes

Travis config review

291484c

ojomio force-pushed the SNI branch from 45c0fa7 to 291484c Compare October 23, 2018 21:13

Try gevent.socket gethostbyaddr version for test_ssl

cb626c3

mock gevent.socket.getaddrinfo to return 127.0.0.1 for any host

009001e

ml31415 merged commit 3fefedd into geventhttpclient:master Oct 25, 2018

ml31415 mentioned this pull request Jan 5, 2019

Fixes SNI hostnames #89

Closed

gileri mentioned this pull request Apr 30, 2020

FastHttpUser doesn't use the SNI TLS extension locustio/locust#1360

Closed

miedzinski mentioned this pull request Oct 14, 2022

Prefer using create_default_ssl_context #170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SNI for ssl connections #109

Use SNI for ssl connections #109

ojomio commented Oct 10, 2018

ojomio commented Oct 15, 2018

ml31415 commented Oct 18, 2018 •

edited

Loading

ojomio commented Oct 18, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

gwik left a comment

gwik Oct 21, 2018

ojomio Oct 22, 2018

ojomio commented Oct 22, 2018

ojomio commented Oct 23, 2018

ml31415 Oct 23, 2018

ml31415 Oct 23, 2018 •

edited

Loading

ml31415 Oct 23, 2018

ojomio commented Oct 23, 2018

ml31415 commented Oct 24, 2018

ojomio commented Oct 24, 2018 •

edited

Loading

ml31415 commented Oct 24, 2018 •

edited

Loading

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

gwik commented Oct 25, 2018 via email

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

Use SNI for ssl connections #109

Use SNI for ssl connections #109

Conversation

ojomio commented Oct 10, 2018

ojomio commented Oct 15, 2018

ml31415 commented Oct 18, 2018 • edited Loading

ojomio commented Oct 18, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

ml31415 commented Oct 19, 2018

ojomio commented Oct 19, 2018

gwik left a comment

Choose a reason for hiding this comment

gwik Oct 21, 2018

Choose a reason for hiding this comment

ojomio Oct 22, 2018

Choose a reason for hiding this comment

ojomio commented Oct 22, 2018

ojomio commented Oct 23, 2018

ml31415 Oct 23, 2018

Choose a reason for hiding this comment

ml31415 Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

ml31415 Oct 23, 2018

Choose a reason for hiding this comment

ojomio commented Oct 23, 2018

ml31415 commented Oct 24, 2018

ojomio commented Oct 24, 2018 • edited Loading

ml31415 commented Oct 24, 2018 • edited Loading

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

gwik commented Oct 25, 2018 via email

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

ojomio commented Oct 25, 2018

ml31415 commented Oct 25, 2018

ml31415 commented Oct 18, 2018 •

edited

Loading

ml31415 Oct 23, 2018 •

edited

Loading

ojomio commented Oct 24, 2018 •

edited

Loading

ml31415 commented Oct 24, 2018 •

edited

Loading