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

Jump to bottom

SSLSocket extra level of indirection #68522

Closed
tiran opened this issue May 30, 2015 · 9 comments
Closed

SSLSocket extra level of indirection #68522

tiran opened this issue May 30, 2015 · 9 comments
Assignees
@tiran
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@tiran
Copy link
Member

tiran commented May 30, 2015

BPO 24334
Nosy @pitrou, @giampaolo, @tiran, @alex, @vadmium, @dstufft
PRs
  • bpo-24334: Cleanup SSLSocket #5252
  • [3.7] bpo-24334: Cleanup SSLSocket (GH-5252) #5857
  • bpo-24334: Remove inaccurate match_hostname call #6211
  • [3.7] bpo-24334: Remove inaccurate match_hostname call (GH-6211) #6212
    		•
    Files
  • no_sslobject.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 2018-02-24.20:52:43.134>
created_at = <Date 2015-05-30.20:37:28.364>
labels = ['3.7', 'expert-SSL', '3.8', 'type-bug', 'library']
title = 'SSLSocket extra level of indirection'
updated_at = <Date 2018-03-24.14:59:18.615>
user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2018-03-24.14:59:18.615>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2018-02-24.20:52:43.134>
closer = 'christian.heimes'
components = ['Library (Lib)', 'SSL']
creation = <Date 2015-05-30.20:37:28.364>
creator = 'christian.heimes'
dependencies = []
files = ['39570']
hgrepos = []
issue_num = 24334
keywords = ['patch']
message_count = 9.0
messages = ['244494', '244495', '246168', '246277', '312753', '312754', '312755', '314368', '314371']
nosy_count = 8.0
nosy_names = ['geertj', 'janssen', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'alex', 'martin.panter', 'dstufft']
pr_nums = ['5252', '5857', '6211', '6212']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24334'
versions = ['Python 3.7', 'Python 3.8']
    @tiran
    Copy link
    Member Author

    tiran commented May 30, 2015

    I just noticed that bpo-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.

    @tiran tiran added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 30, 2015
    @pitrou
    Copy link
    Member

    pitrou commented May 30, 2015

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

    @geertj
    Copy link
    Mannequin

    geertj mannequin commented Jul 3, 2015

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 4, 2015

    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.

    @tiran tiran added 3.7 (EOL) end of life topic-SSL labels Sep 8, 2016
    @tiran tiran self-assigned this Sep 15, 2016
    @tiran
    Copy link
    Member Author

    tiran commented Feb 24, 2018

    New changeset 141c5e8 by Christian Heimes in branch 'master':
    bpo-24334: Cleanup SSLSocket (bpo-5252)
    141c5e8

    @tiran
    Copy link
    Member Author

    tiran commented Feb 24, 2018

    New changeset 8fa8478 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
    [3.7] bpo-24334: Cleanup SSLSocket (GH-5252) (bpo-5857)
    8fa8478

    @tiran
    Copy link
    Member Author

    tiran commented Feb 24, 2018

    Thanks for your review, Antoine!

    @tiran tiran added the 3.8 only security fixes label Feb 24, 2018
    @tiran tiran closed this as completed Feb 24, 2018
    @tiran
    Copy link
    Member Author

    tiran commented Mar 24, 2018

    New changeset e42ae91 by Christian Heimes in branch 'master':
    bpo-24334: Remove inaccurate match_hostname call (bpo-6211)
    e42ae91

    @tiran
    Copy link
    Member Author

    tiran commented Mar 24, 2018

    New changeset 1a0bb62 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
    [3.7] bpo-24334: Remove inaccurate match_hostname call (GH-6211) (bpo-6212)
    1a0bb62

    @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
    Assignees

    @tiran tiran

    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @tiran @pitrou