Issue 24334: SSLSocket extra level of indirection

I just noticed that #21965 has added an extra level of indirection to the SSLSocket object. In Python 3.4 and earlier the ssl.SSLSocket object has one level of indirection:

>>> import ssl
>>> ctx = ssl.create_default_context()
>>> sock = ssl.create_connection(('www.python.org', 443))
>>> ssock = ctx.wrap_socket(sock, server_hostname='www.python.org')
>>> ssock
<ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketType.SOCK_STREAM, proto=6, laddr=('192.168.7.122', 39657), raddr=('185.31.17.223', 443)>
>>> ssock._sslobj
<_ssl._SSLSocket object at 0x7efcb9fd8c00>


In 3.5 an additional level comes into play:

>>> import ssl
>>> ctx = ssl.create_default_context()
>>> sock = ssl.create_connection(('www.python.org', 443))
>>> ssock = ctx.wrap_socket(sock, server_hostname='www.python.org')
>>> ssock
<ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.7.122', 39664), raddr=('185.31.17.223', 443)>
>>> ssock._sslobj
<ssl.SSLObject object at 0x7fa55a96bb00>
>>> ssock._sslobj._sslobj
<_ssl._SSLSocket object at 0x7fa5506918a0>


Method calls like SSLSocket.read() now call SSLObject.read(), which call _SSLSocket.read(). That seems a bit excessive. Isn't there a better way with less indirections?

I have created a proof-of-concept patch that removes the extra layer with some code duplication. Maybe the common code can be moved into a commmon base class for SSLObject and SSLSocket? After all both classes provide a similar interface.

I'm nosying Geert, who is the original author of the SSLObject code.

Apologies for the late reply.

I made SSLSocket go through SSLObject so that the test suite that is primarily testing SSLSocket will test both.

Also, this layering allows us to define some non-networked operations (such as SSL certificate checking and channel binding types) in a single location rather than duplicating them between SSLSocket and SSLObject.

> I made SSLSocket go through SSLObject so that the test suite that is primarily testing SSLSocket will test both.

Indeed, I like the fact it makes test coverage broader.

Of course, if there's another way to get such coverage without duplicating lots of code, then why not.

New changeset 141c5e8c2437a9fed95a04c81e400ef725592a17 by Christian Heimes in branch 'master':
bpo-24334: Cleanup SSLSocket (#5252)
https://github.com/python/cpython/commit/141c5e8c2437a9fed95a04c81e400ef725592a17

New changeset 8fa8478ddeba0870da1152f0a2985c8a7eeb9fd1 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-24334: Cleanup SSLSocket (GH-5252) (#5857)
https://github.com/python/cpython/commit/8fa8478ddeba0870da1152f0a2985c8a7eeb9fd1

Thanks for your review, Antoine!

New changeset e42ae915095ebca789cc36f3a336a3331fe35945 by Christian Heimes in branch 'master':
bpo-24334: Remove inaccurate match_hostname call (#6211)
https://github.com/python/cpython/commit/e42ae915095ebca789cc36f3a336a3331fe35945

New changeset 1a0bb626f4cfd95f7ec406ea7d3f9433def559fc by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-24334: Remove inaccurate match_hostname call (GH-6211) (#6212)
https://github.com/python/cpython/commit/1a0bb626f4cfd95f7ec406ea7d3f9433def559fc

resolution: fixed
stage: patch review -> resolved