Skip to content
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

Cannot create ssl.SSLSocket without existing socket #71816

Closed
nemunaire mannequin opened this issue Jul 26, 2016 · 9 comments
Closed

Cannot create ssl.SSLSocket without existing socket #71816

nemunaire mannequin opened this issue Jul 26, 2016 · 9 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@nemunaire
Copy link
Mannequin

nemunaire mannequin commented Jul 26, 2016

BPO 27629
Nosy @pitrou, @vstinner, @giampaolo, @tiran, @alex, @dstufft, @Vgr255, @nemunaire
Files
  • fix_sslsocket_init_without_socket_3_3-3_6.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/tiran'
    closed_at = <Date 2017-09-07.19:29:41.684>
    created_at = <Date 2016-07-26.23:21:45.712>
    labels = ['3.7', 'expert-SSL', 'type-bug', 'library']
    title = 'Cannot create ssl.SSLSocket without existing socket'
    updated_at = <Date 2017-09-07.19:29:41.683>
    user = 'https://github.com/nemunaire'

    bugs.python.org fields:

    activity = <Date 2017-09-07.19:29:41.683>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2017-09-07.19:29:41.684>
    closer = 'christian.heimes'
    components = ['Library (Lib)', 'SSL']
    creation = <Date 2016-07-26.23:21:45.712>
    creator = 'nemunaire'
    dependencies = []
    files = ['45778']
    hgrepos = []
    issue_num = 27629
    keywords = ['patch']
    message_count = 9.0
    messages = ['271419', '271420', '271455', '271573', '274800', '275685', '282565', '301613', '301616']
    nosy_count = 9.0
    nosy_names = ['janssen', 'pitrou', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'alex', 'dstufft', 'abarry', 'nemunaire']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27629'
    versions = ['Python 3.7']

    @nemunaire
    Copy link
    Mannequin Author

    nemunaire mannequin commented Jul 26, 2016

    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.

    @nemunaire nemunaire mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 26, 2016
    @nemunaire nemunaire mannequin changed the title Cannot create raw ssl.SSLSocket Cannot create ssl.SSLSocket without existing socket Jul 26, 2016
    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Jul 26, 2016

    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.

    @vstinner
    Copy link
    Member

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

    @nemunaire
    Copy link
    Mannequin Author

    nemunaire mannequin commented Jul 28, 2016

    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'
    < 
    <
    ```======================================================================

    @tiran
    Copy link
    Member

    tiran commented Sep 7, 2016

    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

    @tiran
    Copy link
    Member

    tiran commented Sep 10, 2016

    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.

    @tiran tiran self-assigned this Sep 15, 2016
    @nemunaire
    Copy link
    Mannequin Author

    nemunaire mannequin commented Dec 6, 2016

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

    @tiran
    Copy link
    Member

    tiran commented Sep 7, 2017

    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)

    @tiran tiran added the 3.7 (EOL) end of life label Sep 7, 2017
    @tiran
    Copy link
    Member

    tiran commented Sep 7, 2017

    I have created bpo-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.

    @tiran tiran closed this as completed Sep 7, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants