classification
Title: SSLSocket extra level of indirection
Type: behavior Stage: resolved
Components: Library (Lib), SSL Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, christian.heimes, dstufft, geertj, giampaolo.rodola, janssen, martin.panter, pitrou
Priority: normal Keywords: patch

Created on 2015-05-30 20:37 by christian.heimes, last changed 2018-03-24 14:59 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
no_sslobject.patch christian.heimes, 2015-05-30 20:37 review
Pull Requests
URL Status Linked Edit
PR 5252 merged christian.heimes, 2018-01-20 18:44
PR 5857 merged miss-islington, 2018-02-24 20:11
PR 6211 merged christian.heimes, 2018-03-24 14:20
PR 6212 merged miss-islington, 2018-03-24 14:37
Messages (9)
msg244494 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2015-05-30 20:37
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.
msg244495 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-05-30 20:38
I'm nosying Geert, who is the original author of the SSLObject code.
msg246168 - (view) Author: Geert Jansen (geertj) * Date: 2015-07-03 11:57
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.
msg246277 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-07-04 20:56
> 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.
msg312753 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-24 20:10
New changeset 141c5e8c2437a9fed95a04c81e400ef725592a17 by Christian Heimes in branch 'master':
bpo-24334: Cleanup SSLSocket (#5252)
https://github.com/python/cpython/commit/141c5e8c2437a9fed95a04c81e400ef725592a17
msg312754 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-24 20:51
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
msg312755 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-24 20:52
Thanks for your review, Antoine!
msg314368 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-03-24 14:36
New changeset e42ae915095ebca789cc36f3a336a3331fe35945 by Christian Heimes in branch 'master':
bpo-24334: Remove inaccurate match_hostname call (#6211)
https://github.com/python/cpython/commit/e42ae915095ebca789cc36f3a336a3331fe35945
msg314371 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-03-24 14:59
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
History
Date User Action Args
2018-03-24 14:59:18christian.heimessetmessages: + msg314371
2018-03-24 14:37:27miss-islingtonsetpull_requests: + pull_request5957
2018-03-24 14:36:52christian.heimessetmessages: + msg314368
2018-03-24 14:20:51christian.heimessetpull_requests: + pull_request5956
2018-02-24 20:52:43christian.heimessetstatus: open -> closed
versions: + Python 3.8, - Python 3.6
messages: + msg312755

resolution: fixed
stage: patch review -> resolved
2018-02-24 20:51:58christian.heimessetmessages: + msg312754
2018-02-24 20:11:08miss-islingtonsetpull_requests: + pull_request5632
2018-02-24 20:10:59christian.heimessetmessages: + msg312753
2018-01-20 18:44:10christian.heimessetstage: needs patch -> patch review
pull_requests: + pull_request5099
2016-09-15 07:54:03christian.heimessetassignee: christian.heimes
components: + SSL
2016-09-08 15:26:57christian.heimessetversions: + Python 3.7, - Python 3.5
2015-07-04 20:56:39pitrousetmessages: + msg246277
2015-07-03 11:57:19geertjsetmessages: + msg246168
2015-05-31 05:29:05martin.pantersetnosy: + martin.panter
2015-05-30 20:38:32pitrousetnosy: + geertj
messages: + msg244495
2015-05-30 20:37:28christian.heimescreate