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

Unable to copy ssl.SSLContext #77204

Closed
vitaly-krugl mannequin opened this issue Mar 8, 2018 · 24 comments
Closed

Unable to copy ssl.SSLContext #77204

vitaly-krugl mannequin opened this issue Mar 8, 2018 · 24 comments
Assignees
Labels
3.8 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@vitaly-krugl
Copy link
Mannequin

vitaly-krugl mannequin commented Mar 8, 2018

BPO 33023
Nosy @pitrou, @tiran, @ned-deily, @serhiy-storchaka, @miss-islington, @vitaly-krugl
PRs
  • bpo-33023: SSL pickle error message #6099
  • bpo-33023: Fix NotImplemented to NotImplementedError. #10934
  • [3.7] bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934) #11000
  • [3.6] bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934). #11001
  • [2.7] bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934). (GH-11001) #11008
  • 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 2021-04-17.18:24:35.232>
    created_at = <Date 2018-03-08.06:09:21.578>
    labels = ['expert-SSL', 'type-bug', '3.8']
    title = 'Unable to copy ssl.SSLContext'
    updated_at = <Date 2021-04-17.18:24:35.232>
    user = 'https://github.com/vitaly-krugl'

    bugs.python.org fields:

    activity = <Date 2021-04-17.18:24:35.232>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-17.18:24:35.232>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2018-03-08.06:09:21.578>
    creator = 'vitaly.krug'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33023
    keywords = ['patch']
    message_count = 24.0
    messages = ['313422', '313437', '313458', '313459', '313621', '313703', '313704', '313731', '314322', '314347', '314367', '314375', '314382', '314417', '314419', '318894', '318899', '318903', '328980', '331151', '331247', '331248', '331249', '331270']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'christian.heimes', 'ned.deily', 'serhiy.storchaka', 'miss-islington', 'vitaly.krug']
    pr_nums = ['6099', '10934', '11000', '11001', '11008']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33023'
    versions = ['Python 3.8']

    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 8, 2018

    import copy
    import ssl
    
    copy.copy(ssl.create_default_context())
    

    results in

    TypeError: can't pickle SSLContext objects

    This prevents me from being able to copy.deepcopy() an object that references ssl.SSLContext.

    The apparent root cause is apparently that ssl.SSLContext passes an extra arg to its __new__ method, but doesn't implement the method __getnewargs__ that would let copy extract the extra arg.

    @vitaly-krugl vitaly-krugl mannequin added type-crash A hard crash of the interpreter, possibly with a core dump topic-SSL labels Mar 8, 2018
    @vitaly-krugl vitaly-krugl mannequin assigned tiran Mar 8, 2018
    @tiran
    Copy link
    Member

    tiran commented Mar 8, 2018

    This is rather a feature than a bug. It is not possible to make a copy of a SSLContext object because OpenSSL doesn't support the operation. A context contains elements that can't be cloned easily, e.g. session resumption tickets.

    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 8, 2018

    Hi Christian, thank you for following up. Here is my use case, and perhaps you can suggest something else that will work:

    I am refactoring the transport layer in the Pika AMQP client library. The user provides an ssl.SSLContext instance for connecting to an AMQP broker (e.g., RabbitMQ). Pika will resolve the hostname via getaddrinfo and make attempts to establish TCP and AMQP connection to the candidate IP addresses until one succeeds in establishing an AMQP connection over SSL. Each connection attempt will require a fresh unadulterated clone of the ssl.SSLContext instance provided by user to avoid any side-effects from prior connection attempts.

    How can I obtain this pristine clone cleanly for each new connection attempt?

    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 8, 2018

    Also, updating ssl.SSLContext documentation about intentional inability to copy generically and suggestion how to go about it if you need to obtain a clone or similar would be terrific and save developers time so they won't run into this gotcha when designing and implementing solutions. Many thanks!

    @pitrou
    Copy link
    Member

    pitrou commented Mar 11, 2018

    Each connection attempt will require a fresh unadulterated clone of the ssl.SSLContext instance provided by user to avoid any side-effects from prior connection attempts.

    What would those side-effects be?

    @pitrou pitrou added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 11, 2018
    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 12, 2018

    What would those side-effects be?

    Christian Heimes suggested that

    A context contains elements that can't be cloned easily, e.g. session resumption tickets.

    My concern then would be potential side-effects from such session resumption tickets and anything else that one connection attempt might save/change within an SSL Context that might have an undesirable side-effect on the follow-on connection attempts.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 12, 2018

    You won't have any session resumption tickets until a connection succeeds. And even then, I don't think it would be a problem. By design, SSL contexts are meant to be re-used accross multiple connections.

    @tiran
    Copy link
    Member

    tiran commented Mar 13, 2018

    Antoine is correct. Session resumption is fully transparent for the application layer. The context object is designed to be reused by multiple connections to same or different servers.

    @serhiy-storchaka
    Copy link
    Member

    PR 6099 changes error message "can't pickle SSLContext objects" to "cannot serialize SSLContext object", right? Wouldn't be better to change the standard error message instead?

    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 24, 2018

    It would be very helpful to make a statement in SSLContext's documentation to the effect that it's not copyable. This is frankly the first time I run into a non-copyable object.I spend quite a bit of time researching this after implementing a copying strategy that failed. It would have saved me (and others...) so much time is there was a warning in SSLContext documentation about not being able to serialize/copy/deepcopy by design!

    Also, making that exception message more generic (ha, I wasn't pickling anything?!) as Serhiy Storchaka suggested would be a welcome addition, but not replacement for documentation.

    @tiran
    Copy link
    Member

    tiran commented Mar 24, 2018

    Serhiy,
    I don't understand what you are trying to tell me. "cannot serialize '%s' object" is used all over the interpreter, e.g. io, pickle, etree, and more. I feel it's the standard message.

    Vitaly,
    A lot of objects can't be copied. It's the general case for all kinds of objects that hold operating system resources (files, sockets) or wrap external C libraries (bz2, lzma, sqlite, ssl). We generally don't document that an object cannot be pickled, serialized, or copied. If documentation doesn't state that an object is copy-able or doesn't provide a dedicated copy method, than it can't be copied.

    @vitaly-krugl
    Copy link
    Mannequin Author

    vitaly-krugl mannequin commented Mar 24, 2018

    Thank you, I'll consider myself having been warned :)

    On Sat, Mar 24, 2018, 7:28 AM Christian Heimes <report@bugs.python.org>
    wrote:

    Christian Heimes <lists@cheimes.de> added the comment:

    Serhiy,
    I don't understand what you are trying to tell me. "cannot serialize '%s'
    object" is used all over the interpreter, e.g. io, pickle, etree, and more.
    I feel it's the standard message.

    Vitaly,
    A lot of objects can't be copied. It's the general case for all kinds of
    objects that hold operating system resources (files, sockets) or wrap
    external C libraries (bz2, lzma, sqlite, ssl). We generally don't document
    that an object cannot be pickled, serialized, or copied. If documentation
    doesn't state that an object is copy-able or doesn't provide a dedicated
    copy method, than it can't be copied.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue33023\>


    @serhiy-storchaka
    Copy link
    Member

    Christian, I thought the reason of adding __getstate__ methods which raise an exception is that the existing error message doesn't satisfy your. What is the other reason?

    @tiran
    Copy link
    Member

    tiran commented Mar 25, 2018

    That's the only reason for PR 6099. The change makes it more obvious that SSL objects can't be serialized or copied.

    @serhiy-storchaka
    Copy link
    Member

    See bpo-33138.

    @ned-deily
    Copy link
    Member

    We have a languishing PR here. What should be done with it? If I understand correctly, Serhiy is proposing a more general cleanup in bpo-33138 PR 6239. If that is merged, can PR 6099 here be simplified? What branches should it apply to? (retargeting for 3.8 pending comments)

    @ned-deily ned-deily added the 3.8 only security fixes label Jun 7, 2018
    @serhiy-storchaka
    Copy link
    Member

    The change to the dup() method looks as a bug fix to me. It needs to be backported to all maintained versions. New tests can be backported to all versions except perhaps 2.7 (not tested). Changes to __getstate__() will be not needed if merge bpo-33138 PR 6239 (they just change the error message). But it may be needed to apply them to 2.7 only.

    @serhiy-storchaka
    Copy link
    Member

    If changes to __getstate__() be merged in 2.7, I suggest to unify them with bpo-33138.

    @serhiy-storchaka
    Copy link
    Member

    After discussing on Python-Dev all error messages were changed to "cannot pickle 'XXX' object".

    https://mail.python.org/pipermail/python-dev/2018-October/155599.html

    I suggest to not add __getstate__() methods with different error messages. But the dup() method should be fixed.

    @serhiy-storchaka
    Copy link
    Member

    PR 10934 fixes just a typo in the name of NotImplementedError. It fixes also a similar typo in IDLE. It adds new tests and fixes existing tests for NotImplementedError in SSLSocket.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 42b1d61 by Serhiy Storchaka in branch 'master':
    bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934)
    42b1d61

    @miss-islington
    Copy link
    Contributor

    New changeset 6485aa6 by Miss Islington (bot) in branch '3.7':
    bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934)
    6485aa6

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7a2cf1e by Serhiy Storchaka in branch '3.6':
    bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934). (GH-11001)
    7a2cf1e

    @serhiy-storchaka
    Copy link
    Member

    New changeset 324e179 by Serhiy Storchaka in branch '2.7':
    [2.7] bpo-33023: Fix NotImplemented to NotImplementedError. (GH-10934). (GH-11001) (GH-11008)
    324e179

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants