Add support of AppEngine SSL library #193

jmuk · 2016-01-25T20:51:31Z

This is a patch to provide the SSL comptibility module working within AppEngine. This change does not enable hyper working on AppEngine yet, but I think this would help approaching to it.

Lukasa · 2016-01-25T20:54:04Z

hyper/ssl_compat_appengine.py

+OP_NO_COMPRESSION = 0
+
+
+class SSLContext:


This is a minor thing, but can we explicitly inherit from object here? It's unnecessary with Python 3, but old-style classes are a nasty surprise when working in Python 2 code and so it'd be good if we could save ourselves the hassle.

Lukasa · 2016-01-25T20:57:01Z

Thanks for this @jmuk!

I've left a bit of inline feedback, but I'm interested in getting an idea of how much time you have to spend here. It'd be really great if we could add some tests to validate that all of this logic works correctly, but as you've pointed out, hyper doesn't directly work with GAE yet even with this patch in place. Are you expecting multiple patches to come down the line shortly to add that function? If you are, I'd be interested in having these PRs land in a parallel branch to the mainline development so that we can get them all in place and then add tests in one fell swoop before merging to mainline.

jmuk · 2016-01-25T21:32:21Z

Thank you for the quick reply.

I'll provide the fixes on your comments, but for the questions; I'm good to be within a separate branch. I actually have another CL for the usage of socket.recv_into (which is not implemented yet), but that is all I've noticed. after that hyper will work on GAE -- I mean, at least it can behave as a http2 client in the way I tried, some may be missing though.

Lukasa · 2016-01-25T21:58:25Z

hyper/http20/connection.py

+            # AppEngine SSLSocket does not have selected_npn_protocol nor
+            # selected_alpn_protocol, so just avoid this assertion.
+            assert is_appengine or (
+                proto in H2_NPN_PROTOCOLS or proto == H2C_PROTOCOL)


Sorry, I think I wasn't clear enough with my previous feedback. When I was asking about H2C_PROTOCOL I wanted to know why you chose that over h2. h2c is meant to be reserved for HTTP/2 via opportunistic encryption, which seemed an odd choice. I'd be happy to use h2.

Okay, pushed a new patch -- now it uses 'h2'

jmuk · 2016-01-26T00:52:21Z

Added test cases, this will make travis green.

Lukasa · 2016-01-26T11:10:37Z

test/test_appengine.py

+        ssl_sock = context.wrap_socket(sock)
+        assert ssl_sock.selected_npn_protocol() == 'h2'
+        ssl_sock.close()
+        self.tear_down()


Presumably these tests should only run if we're actually running on GAE or in a GAE-like system? Otherwise they don't really prove all that much.

I think we need to mark these test classes with pytest.mark.skipif() and use similar logic to what urllib3 uses for its GAE tests, and then add that to the matrix of tests we run on Travis. That might be a little tricky: do you want me to handle doing that?

There's also a timing issue here: the socket_handler really needs to wait until the connection is made and the assertions have been made before any of the tests run. Otherwise, you get problems like the one you can see in the travis run.

The easiest way to do this is to use threading events to synchronise the activity.

GAE doesn't offer a different ssl library actually, it's the same ssl as bundled in future version of Python, but the values in the ssl module is slightly limited. SSLContext doesn't exist, for example.

That means that this ssl_compat_appengine should work with the normal ssl module outside of AppEngine (if it's newer than 2.7.9), and the test case would be still useful to verify the behavior of the module itself.

For the second comment, thank you for catching that. I put sock.do_handshake() on socket_handler so that it verifies to establish the connection, so I think that's good enough.

Ah, ok, if this should work outside GAE then it's probably enough to just add an extra test environment to validate that it also works on GAE. If we're going to support GAE then we should support it properly! =D

Well, I'm afraid it's not possible to verify if it works on GAE here on travisCI. Actually the local emulator (dev_appserver) in AppEngine SDK simply uses the local runtime with local ssl library, so that doesn't mean the actual GAE environment.

Google could put a test case in its internal end-to-end test cases if hyper is listed in the bundled third party libraries (https://cloud.google.com/appengine/docs/python/tools/libraries27?hl=en), but that's anyways out of the scope right now.

Aha, ok, fair enough. In that case, let me do a final code review now. =)

Lukasa · 2016-01-26T13:32:50Z

Your Python 3.2 test failures are also hitting an issue that I just resolved in #194. You may want to rebase on top of development in order to obtain the fix.

AppEngine is a special environment, it's in some older version of Python but it has ssl library, and the library availability is slightly different from standard python runtimes. This change provides a compatibility layer working with AppEngine ssl library.

- SSLContext now inherits from object - stop faking selected_npn_protocol, but fix on the assertion on the protocol so that it won't raise failures even when it does not exist.

- do_handshake_on_connect to False, because appengine test server disconnect immediately on connect.

jmuk · 2016-01-28T04:12:33Z

It seems I've forgotten to reply, but yeah, I've rebased to the top of development, and now it succeeds.

Is there anything I should do?

Lukasa · 2016-01-28T10:03:56Z

hyper/compat.py

@@ -31,6 +38,9 @@ def ignore_missing():
 if is_py2:
    if is_py2_7_9_or_later:
        import ssl
+    elif is_appengine:
+        from . import ssl_compat_appengine
+        ssl = ssl_compat_appengine


Is this order of fallbacks correct? It seems like if GAE identifies itself as Python 2.7.9 or later then we won't use the AppEngine compatibility module. Having not used GAE myself I'm not sure if this order is right...

Fixed, use ssl_compat_appengine when appengine.

When an AppEngine runtime got updated to Python 2.7.9 or later, I was personally thinking that it will use the bundled ssl and this compatibility module becomes unnecessary.
Indeed, however, nothing is sure for now, and anyways this would work with bundled ssl as I commented on another place, using it for GAE environment would be at least harmless even in that.

Lukasa · 2016-01-28T10:04:36Z

M'kay, I have one smaller question about the order of fallbacks.

jmuk · 2016-01-29T10:30:48Z

Sorry, I've come to realize that the lack of npn/alpn protocol could be significant, and adding them can't be done in hyper -- ssl library (in appengine runtime) needs to be updated. That means, unfortunately this current patch is insufficient, and some details in this PR should be changed when ssl library is updated.

Let me close this PR for the time being. Thank you for your reviews.

Lukasa reviewed Jan 25, 2016
View reviewed changes

Lukasa reviewed Jan 26, 2016
View reviewed changes

jmuk added 6 commits January 26, 2016 12:02

Update license.

e1ac334

Fix based on the PR comments.

de99b0e

- SSLContext now inherits from object - stop faking selected_npn_protocol, but fix on the assertion on the protocol so that it won't raise failures even when it does not exist.

A fix: changes the fake selected_npn_protocol but use 'h2'

88a8847

Add test cases for the new ssl_compat_appengine module.

b67dadd

More test fixes.

e6f9255

- do_handshake_on_connect to False, because appengine test server disconnect immediately on connect.

jmuk force-pushed the appengine branch from c996261 to e6f9255 Compare January 26, 2016 20:04

Fix the test a bit -- do_handshake rather than skipping handshake.

701ccc1

Lukasa reviewed Jan 28, 2016
View reviewed changes

Reorder the fallback for ssl module.

5453363

jmuk closed this Jan 29, 2016

		OP_NO_COMPRESSION = 0


		class SSLContext:

Add support of AppEngine SSL library #193

Add support of AppEngine SSL library #193

Uh oh!

Conversation

jmuk commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Jan 25, 2016

Uh oh!

jmuk commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmuk commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Jan 26, 2016

Uh oh!

jmuk commented Jan 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Jan 28, 2016

Uh oh!

jmuk commented Jan 29, 2016

Uh oh!

Uh oh!