classification
Title: Cannot create ssl.SSLSocket without existing socket
Type: behavior Stage: resolved
Components: Library (Lib), SSL Versions: Python 3.7
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, christian.heimes, dstufft, ebarry, giampaolo.rodola, haypo, janssen, nemunaire, pitrou
Priority: normal Keywords: patch

Created on 2016-07-26 23:21 by nemunaire, last changed 2017-09-07 19:29 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
fix_sslsocket_init_without_socket_3_3-3_6.patch nemunaire, 2016-12-06 18:36 review
Messages (9)
msg271419 - (view) Author: (nemunaire) * Date: 2016-07-26 23:21
I got this stacktrace:
  File "test_ssl.py", line 3, in <module>
    sock = ssl.SSLSocket(server_hostname="docs.python.org")
  File "/usr/lib/python3.4/ssl.py", line 536, in __init__
    if sock.getsockopt(SOL_SOCKET, SO_TYPE) != SOCK_STREAM:
AttributeError: 'NoneType' object has no attribute 'getsockopt'

with this minimal code:
import ssl

sock = ssl.SSLSocket(server_hostname="docs.python.org")
sock.connect(("docs.python.org", 443))
sock.sendall(b"GET /3/library/ssl.html HTTP/1.0\r\nHost: docs.python.org\r\n\r\n")
print(sock.recv(4096).decode())

Whereas the None socket is correctly handled a few lines later: https://hg.python.org/cpython/file/tip/Lib/ssl.py#l715

All Python >= 3.3 are affected (since https://hg.python.org/cpython/rev/a00842b783cf) and can be patched with the same file, attached to this issue.
msg271420 - (view) Author: Emanuel Barry (ebarry) * Date: 2016-07-26 23:48
Thank you for the report and the patch! :) This will need a test in Lib/test/test_ssl.py to check for this particular case.

I've removed 3.3 and 3.4 from the Versions field, since these versions no longer get regular bugfixes (only security bugfixes may go in these). As a result, only 3.5 and 3.6 will get the fix.
msg271455 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-07-27 14:25
The change introducing the != SOCK_STREAM check (change a00842b783cf) was made in 2013. It looks like the case was not tested since at least 3 years...

> This will need a test in Lib/test/test_ssl.py to check for this particular case.

Yes, this new test is mandatory to make sure that the feature is not broken again (check for regression).

The workaround is to pass an existing socket...
msg271573 - (view) Author: (nemunaire) * Date: 2016-07-28 17:34
Here is a new patch with tests on constructor.

The patch on the feature is quite different: instead of testing for None socket, I choose to delay the != SOCK_STREAM check in the later condition, when we know we treat a socket.

Tests include different constructor forms: with a given socket, with a fileno (didn't work either, before this patch) and without socket nor file descriptor (as in my original test).

I don't have sufficient background to judge if tests will work on all platform (eg. fileno and windows, ...).

Here is the interesting diff of the tests on SSL (before/after applying the patch on the feature):

32c32
< test_constructor (__main__.BasicSocketTests) ... ERROR
---
> test_constructor (__main__.BasicSocketTests) ... ok
519,528d518
< ERROR: test_constructor (__main__.BasicSocketTests)
< ----------------------------------------------------------------------
< Traceback (most recent call last):
<   File "test_ssl.py", line 149, in test_constructor
<     ss = ssl.SSLSocket()
<   File "/usr/lib/python3.4/ssl.py", line 545, in __init__
<     if sock.getsockopt(SOL_SOCKET, SO_TYPE) != SOCK_STREAM:
< AttributeError: 'NoneType' object has no attribute 'getsockopt'
< 
< ======================================================================
msg274800 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-07 11:43
The patch is incomplete. Please also check that type == SOCK_STREAM. The code can be simplified with a bitmask test:

if sock is not None:
    type = sock.type
if type & socket.SOCK_STREAM != socket.SOCK_STREAM:
    raise NotImplementedError
msg275685 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-10 19:45
I'm considering to not fix this bug and rather remove the dead code. This feature was never documented and has been broken since 3.3, maybe earlier. It's also hard to use it correctly because you need to pass the correct socket family and type.

For 3.6 I'm planning to deprecate SSLSocket.__init__() in favor of SSLContext.wrap_socket() anyway. Please use https://docs.python.org/3/library/socket.html#socket.socket with fileno argument. It auto-detects the correct socket type and family.
msg282565 - (view) Author: (nemunaire) * Date: 2016-12-06 18:36
The documentation already recommends to use SSLContext.wrap_socket(), but it didn't cover all use cases. Eg. I use multiple inheritance to handle both socket and SSLSocket inside a factory pattern, in order to override some methods defined in both classes. If I use SSLContext.wrap_socket(), it erases my overrides.

As the patch add some tests on this feature, this is no more dead code; however I let you decide the future of this issue: I've updated my patch with your remarks if you want to include it (it applies from Python 3.3 to current Python 3.6).
msg301613 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-07 18:51
How about I make the actual SSLSocket and SSLObject class customizable so you can override what is returned by wrap_socket() and wrap_bio()?

class MySSLSocket(ssl.SSLSocket):
    pass

ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
ctx.sslsocket_class = MySSLSocket
sock = ctx.wrap_socket(socket.socket(), server_side=True)
assert isinstance(sock, MySSLSocket)
msg301616 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-07 19:29
I have created #27629 to allow customization of SSLObject and SSLSocket. I'm closing this as "won't fix" because I rather want people to move away from ssl.wrap_socket() and manual instantiation of SSLSocket.
History
Date User Action Args
2017-09-07 19:29:41christian.heimessetstatus: open -> closed
resolution: wont fix
messages: + msg301616

stage: patch review -> resolved
2017-09-07 19:15:14christian.heimeslinkissue31386 superseder
2017-09-07 18:51:07christian.heimessetmessages: + msg301613
versions: + Python 3.7, - Python 3.5, Python 3.6
2016-12-06 18:37:25nemunairesetfiles: - fix_sslsocket_init_without_socket_3_3-3_6.patch
2016-12-06 18:36:44nemunairesetfiles: + fix_sslsocket_init_without_socket_3_3-3_6.patch

messages: + msg282565
2016-09-15 07:49:10christian.heimessetassignee: christian.heimes
components: + SSL
2016-09-10 19:45:19christian.heimessetmessages: + msg275685
2016-09-07 11:43:20christian.heimessetmessages: + msg274800
2016-09-07 11:34:22berker.peksaglinkissue27996 superseder
2016-07-28 17:34:33nemunairesetfiles: - fix_sslsocket_init_without_socket_3.3-3_6.patch
2016-07-28 17:34:23nemunairesetfiles: + fix_sslsocket_init_without_socket_3_3-3_6.patch

messages: + msg271573
2016-07-27 14:25:17hayposetnosy: + haypo
messages: + msg271455
2016-07-26 23:48:06ebarrysetversions: - Python 3.3, Python 3.4
nosy: + ebarry, alex, janssen, pitrou, dstufft, christian.heimes, giampaolo.rodola

messages: + msg271420

stage: patch review
2016-07-26 23:33:56nemunairesettitle: Cannot create raw ssl.SSLSocket -> Cannot create ssl.SSLSocket without existing socket
2016-07-26 23:21:45nemunairecreate