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

Add some framework for SSL tests and refactor makefile a bit #109

Merged
merged 5 commits into from Sep 2, 2018

Conversation

Projects
None yet
3 participants
@webknjaz
Member

webknjaz commented Aug 30, 2018

  • What kind of change does this PR introduce?

    • bug fix
    • feature
    • docs update
    • tests/coverage improvement
    • refactoring
    • other
  • What is the related issue number (starting with #)
    #6 (cheroot.ssl.pyopenssl is now importable from under Python 3, but still doesn't work)
    #95 (origins of testing SSL, added trustme for generation of CA and certs in runtime)

  • What is the current behavior? (You can also link to an open issue here)
    Mess in cheroot.makefile, cheroot.ssl.pyopenssl import fails under Python 3 (trying to inherit class from function 🙈), no SSL tests, no way to generate test certificates.

  • What is the new behavior (if this is a feature change)?
    New test for adapters in the wild, makefile has separately importable StreamWriter and StreamReader,

  • Other information:
    https://gitter.im/cherrypy/cherrypy?at=5b871411c2bd5d117aefa6db

  • Checklist:

    • [≈] I think the code is well written
    • [≈] I wrote good commit messages
    • [≈] I have squashed related commits together after the changes have been approved
    • Unit tests for the changes exist
    • Integration tests for the changes exist (if applicable)
    • I used the same coding conventions as the rest of the project
    • The new code doesn't generate linter offenses
    • Documentation reflects the changes
    • The PR relates to only one subject with a clear title
      and description in grammatically correct, complete sentences

This change is Reviewable

@webknjaz webknjaz self-assigned this Aug 30, 2018

@webknjaz webknjaz requested a review from jaraco Aug 30, 2018

@webknjaz

This comment was marked as resolved.

Member

webknjaz commented Aug 30, 2018

[WinError 10049] The requested address is not valid in its context

This error is happening under AppVeyor for some odd reason.

address = ('0.0.0.0', 1191)

That's probably because connecting to 0.0.0.0 instead of localhost..

@webknjaz

This comment was marked as resolved.

Member

webknjaz commented Aug 30, 2018

def __init__(self, sock, mode='r', bufsize=io.DEFAULT_BUFFER_SIZE):
"""Initialize socket stream reader."""
super().__init__(socket.SocketIO(sock, mode), bufsize)

This comment has been minimized.

@webknjaz

webknjaz Aug 30, 2018

Member

(This is always evaluated under Python 3, so no need to use Python 2 compatible super() syntax)

@codecov

This comment has been minimized.

codecov bot commented Aug 30, 2018

Codecov Report

Merging #109 into master will increase coverage by 3.74%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   70.39%   74.13%   +3.74%     
==========================================
  Files          20       21       +1     
  Lines        3219     3271      +52     
==========================================
+ Hits         2266     2425     +159     
+ Misses        953      846     -107

@webknjaz webknjaz force-pushed the experiment/ssl-tests branch from 93241cf to 3faa039 Aug 30, 2018

@webknjaz

This comment has been minimized.

@jeffvanvoorst

I looked some. It looks reasonable from a high level. However, I am not sufficiently versed in the SSL or stream details of cheroot.

Also, I don't completely understand the implications of using socket.SocketIO versus socket._fileobject

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 31, 2018

@jeffvanvoorst take into account that there's a lot of very old code, which we sometimes have trouble understanding completely. Among that, there's lots of backwards compatibility hacks. AFAIR socket._fileobject (missing in Python 3) was needed for legacy Python 2 versions (a year or two ago you could still find some Python 2.3 compat/backports stuff).
That's what makes it hard to read/support/change. And that's why we want to refactor it and need help in figuring out how to do this harmlessly.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 31, 2018

Alright, it looks like python-future has a backport for that: https://github.com/PythonCharmers/python-future/blob/master/src/future/backports/socket.py#L268-L384

@jaraco I think it'd be a good idea to adopt it. What do you think? I'd simplify our compatibility layers a lot!

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 31, 2018

I've evaluated python-future in the past, and it seems like a pretty robust solution. The fact that it monkey patches the standard library has caused me problems in the past when I would be testing for Python 2/3 compatibility in an environment that had python-future, my results were influenced by the presence of that library, which left me with a slight distaste for it. But in general, I've seen good things come from it.

My main reluctance stems from our existing use of six. I imagine the two can interoperate, though it's not mentioned in the narrative on the topic.

However, given the potential benefits (more single-code base aligned to Python 3), I'm very much in favor, especially if this breathes some extra bit of life into the last Python 2 releases.

@jaraco

Overall, this all seems fine. If python-future can make the changes even more unified, I'd recommend going that route. The only change I disagree with is the copy/paste of SSL_fileobject._safe_call. The rest is fine.

if self._refcount < 0:
self.close()
else:
self._refcount -= 1

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

This refcounting seems wrong to me. I would expect a refcount to be set to 1 when there's one object referencing it, and 0 when there are no objects referencing it. (not 0 when there's one, and -1 when there's none).

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

I stole that from https://github.com/urllib3/urllib3/blob/master/src/urllib3/contrib/securetransport.py#L670-L677
and it still doesn't seem to help. I'd postpone solving this for now.

@@ -200,4 +199,5 @@ def env_dn_dict(self, env_prefix, cert_value):
def makefile(self, sock, mode='r', bufsize=DEFAULT_BUFFER_SIZE):
"""Return socket file object."""
return MakeFile(sock, mode, bufsize)
cls = StreamReader if 'r' in mode else StreamWriter

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

It feels like this logic duplicates logic in makefile.MakeFile (Python 3 version). Ideally, this decision would be made in one place. Though, if one of these methods is deprecated, that's fine.

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

I decided to delay this refactoring. I also don't like the inability to remove MakeFile from cheroot.server module completely...

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

Oh, and I'm also trying hard to decouple those things.

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

Which might mean removing makefile in lots of places.

ssl_timeout = 3
ssl_retry = .01
def _safe_call(self, is_reader, call, *args, **kwargs):

This comment has been minimized.

@jaraco

jaraco Aug 31, 2018

Member

This function seems to add a lot of repetition. I suggest extracting the common aspects of _safe_call into a mix-in for each of these SSLFileobject classes.

This comment has been minimized.

@webknjaz

webknjaz Aug 31, 2018

Member

Yeah, I just did this quick and dirty, I'm also in favor of having a mixin. Just didn't get to actually doing it.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 31, 2018

@jaraco does it run monkey-patching by default? I thought it should be an explicit call for that.
Can we just import tiny things from there skipping the monkey-patching?

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 31, 2018

does [python-future] run monkey-patching by default?

Perhaps not. I guess what I observed was that import builtins only works on Python 2 when python-future is installed, and it was that behavior that confused me. Now that I look at it, I'm not seeing monkey-patching as I'd imagined:

~ $ python2.7 -m rwt future
Collecting future
  Using cached https://files.pythonhosted.org/packages/00/2b/8d082ddfed935f3608cc61140df6dcbf0edea1bc3ab52fb6c29ae3e81e85/future-0.16.0.tar.gz
Building wheels for collected packages: future
  Running setup.py bdist_wheel for future ... done
  Stored in directory: /Users/jaraco/Library/Caches/pip/wheels/bf/c9/a3/c538d90ef17cf7823fa51fc701a7a7a910a80f6a405bf15b1a
Successfully built future
Installing collected packages: future
Successfully installed future-0.16.0
Python 2.7.15 (default, Jun 20 2018, 13:24:23)
[GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import builtins
>>> builtins.str
<class 'future.types.newstr.newstr'>
>>> str
<type 'str'>

So in that case, one does have to explicitly from builtins import str to override the default str on Python 2 (same as we do with six).

So my fears are assuaged (and not justified), so let's try it.

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 31, 2018

I should also say - until this example, I've found no instances where use of python-future would provide functionality that six does not, so I've had little motivation to try it out. Glad to see it adopted here.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 31, 2018

Yeah, the most interesting thing to me is its collection of backports, unfortunately, this is not fully documented. I've found socket.SocketIO by doing googling and nothing mentioning it in docs. Then I went further, looked at the source and realized that there's quite a number of useful backports shipped with this dist.

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 2 alerts when merging c6b37ad into 5edf9cb - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes

Comment posted by LGTM.com

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 3 alerts when merging 655b3dc into 5edf9cb - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 3 alerts when merging a14410c into 5edf9cb - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 1, 2018

This pull request introduces 3 alerts when merging 69dfb97 into 5edf9cb - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

@webknjaz

This comment has been minimized.

Member

webknjaz commented Sep 1, 2018

So while testing future I've faced some bug with error __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0, so I went ahead and copied that backported module and removed __weakref__ from its __slots__. It became importable.
However when I ran tests I've got

  File "/home/wk/src/github/cherrypy/cheroot/cheroot/makefile.py", line 33, in write                                          
    raise TypeError("can't write str to binary stream")                                                                       
TypeError: can't write str to binary stream         

Which probably means that we need to standardize to writing bytes only there.

Which is why I'm going to postpone trying to make use of this backport till a separate PR.

@@ -87,7 +87,7 @@
from . import errors, __version__
from ._compat import bton, ntou
from .workers import threadpool
from .makefile import MakeFile
from .makefile import MakeFile, StreamWriter

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

(It annoys me that I've failed to remove MakeFile at all, but that's all I can do for now)

'sock_shutdown', 'get_peer_certificate', 'want_read',
'want_write', 'set_connect_state', 'set_accept_state',
'connect_ex', 'sendall', 'settimeout', 'gettimeout'):
exec("""def %s(self, *args):

This comment has been minimized.

@webknjaz

webknjaz Sep 1, 2018

Member

I didn't like that it was execing things

Fresh review needed

@webknjaz webknjaz force-pushed the experiment/ssl-tests branch from 69dfb97 to 2eb2abe Sep 2, 2018

@webknjaz webknjaz requested a review from jaraco Sep 2, 2018

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 2, 2018

This pull request introduces 3 alerts when merging 2eb2abe into 9f77c31 - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

@webknjaz webknjaz force-pushed the experiment/ssl-tests branch from 2eb2abe to 7b1de00 Sep 2, 2018

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 2, 2018

This pull request introduces 3 alerts when merging 7b1de00 into c92f476 - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

webknjaz added some commits Jul 15, 2018

Add dummy test for TLS/SSL
Ref #95

In order for it to work now:

* Mark pyopenssl adapter as xfail under Python 3

* Patch builtin SSL adapter to not load cert chain
Split cheroot.makefile.MakeFile into reader/writer
Done this where possible for now

@webknjaz webknjaz force-pushed the experiment/ssl-tests branch from 7b1de00 to 8aca957 Sep 2, 2018

@webknjaz

This comment was marked as outdated.

Member

webknjaz commented Sep 2, 2018

This pull request introduces 3 alerts when merging 8aca957 into c92f476 - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes
  • 1 for Non-callable called

Comment posted by LGTM.com

@jaraco

jaraco approved these changes Sep 2, 2018

Looks good to me. Let's give it a try.

@webknjaz webknjaz merged commit 8aca957 into master Sep 2, 2018

9 of 11 checks passed

ci/circleci: macos-build Your tests failed on CircleCI
Details
codeclimate 4 issues to fix
Details
LGTM analysis: Python 3 new alerts
Details
License Compliance All checks passed.
Details
WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment