Skip to content
This repository was archived by the owner on Jan 13, 2021. It is now read-only.

Add support of AppEngine SSL library #193

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion LICENSE
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2014 Cory Benfield
Copyright (c) 2014 Cory Benfield, Google Inc.

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
Expand Down
12 changes: 11 additions & 1 deletion hyper/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,19 @@
# TODO log?
ssl_compat = None

try:
import google.appengine
is_appengine = True
except ImportError:
is_appengine = False

_ver = sys.version_info
is_py2 = _ver[0] == 2
is_py2_7_9_or_later = _ver[0] >= 2 and _ver[1] >= 7 and _ver[2] >= 9
is_py3 = _ver[0] == 3
is_py3_3 = is_py3 and _ver[1] == 3


@contextmanager
def ignore_missing():
try:
Expand All @@ -29,7 +36,10 @@ def ignore_missing():
pass

if is_py2:
if is_py2_7_9_or_later:
if is_appengine:
from . import ssl_compat_appengine
ssl = ssl_compat_appengine
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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.

elif is_py2_7_9_or_later:
import ssl
else:
ssl = ssl_compat
Expand Down
49 changes: 49 additions & 0 deletions hyper/ssl_compat_appengine.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# -*- coding: utf-8 -*-
"""
hyper/ssl_compat_appengine
~~~~~~~~~

Provides the ssl module interface which hyper assumes, based on AppEngine ssl.

This module complements some constants and classes which don't exist on
AppEngine's SSL module, to be used by hyper smoothly.
See: https://cloud.google.com/appengine/docs/python/sockets/ssl_support
"""

import ssl

from ssl import PROTOCOL_TLSv1, PROTOCOL_SSLv23, PROTOCOL_SSLv3
from ssl import CERT_NONE, CERT_OPTIONAL, CERT_REQUIRED
from ssl import match_hostname
Copy link
Member

Choose a reason for hiding this comment

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

Does AppEngine promise to provide match_hostname?

Copy link
Author

Choose a reason for hiding this comment

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

well, I'm afraid not, but it's available actually as far as I tried, and I think there's not a big reason to drop it now.
I can put try: with catching ImportError, but in that case I'm not sure how the fake match_hostname should be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

This was mostly a question about whether GAE provides this function in general. If it does, I'm not too worried about having it as an unconditional import.


OP_NO_COMPRESSION = 0


class SSLContext(object):
"""A SSL context which implements the methods used by hyper."""

def __init__(self, proto):
self.protocol_version = proto
self.verify_mode = CERT_REQUIRED
self.check_hostname = True
self.options = 0
self.custom_ca_cert = None

def set_default_verify_paths(self):
# Intentionally do nothing.
pass

def load_verify_locations(self, cafile):
self.custom_ca_cert = cafile

def wrap_socket(self, sock, server_side=False, do_handshake_on_connect=True,
suppress_ragged_eofs=True, server_hostname=None):
sock = ssl.wrap_socket(
sock, server_side=server_side, cert_reqs=self.verify_mode,
ssl_version=self.protocol_version, ca_certs=self.custom_ca_cert,
suppress_ragged_eofs=suppress_ragged_eofs,
do_handshake_on_connect=do_handshake_on_connect)
# AppEngine SSLSocket does not have selected_npn_protocol, a hacky
# solution to return a dummy data.
sock.selected_npn_protocol = lambda: 'h2'
return sock
61 changes: 61 additions & 0 deletions test/test_appengine.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# -*- coding: utf-8 -*-
"""
Tests the ssl compatibility module for appengine.
"""
import hyper
from hyper import ssl_compat_appengine
from server import SocketLevelTest
import socket
import pytest


class TestAppengine(object):
"""
Test cases for ssl_compat_appengine module.
"""

def test_field_existences(self):
assert ssl_compat_appengine.PROTOCOL_TLSv1 is not None
assert ssl_compat_appengine.PROTOCOL_SSLv23 is not None
assert ssl_compat_appengine.PROTOCOL_SSLv3 is not None
assert ssl_compat_appengine.CERT_NONE is not None
assert ssl_compat_appengine.CERT_OPTIONAL is not None
assert ssl_compat_appengine.CERT_REQUIRED is not None
assert ssl_compat_appengine.OP_NO_COMPRESSION is not None
assert ssl_compat_appengine.match_hostname is not None
assert ssl_compat_appengine.SSLContext is not None

def test_SSLContext(self):
context = ssl_compat_appengine.SSLContext(
ssl_compat_appengine.PROTOCOL_SSLv23)
context.set_default_verify_paths()
assert context.protocol_version == ssl_compat_appengine.PROTOCOL_SSLv23


class TestAppengineSocket(SocketLevelTest):
"""
Test case for wrap_socket.
"""
h2 = False

def socket_handler(self, listener):
sock = listener.accept()[0]
sock.do_handshake()
sock.close()

def test_wrap_socket(self):
self.set_up()
self._start_server(self.socket_handler)
context = ssl_compat_appengine.SSLContext(
ssl_compat_appengine.PROTOCOL_SSLv23)
context.set_default_verify_paths()
context.verify_mode = ssl_compat_appengine.CERT_NONE
context.verify_hostname = False
# This invocation makes sure it does not fail.
context.load_verify_locations('test/certs/server.crt')
sock = socket.create_connection(
(self.server_thread.host, self.server_thread.port))
ssl_sock = context.wrap_socket(sock)
assert ssl_sock.selected_npn_protocol() == 'h2'
ssl_sock.close()
self.tear_down()
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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