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

Provide knobs to disable session ticket generation on TLS 1.3 #81301

Closed
njsmith opened this issue Jun 1, 2019 · 8 comments
Closed

Provide knobs to disable session ticket generation on TLS 1.3 #81301

njsmith opened this issue Jun 1, 2019 · 8 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes topic-SSL type-feature A feature request or enhancement

Comments

@njsmith
Copy link
Contributor

njsmith commented Jun 1, 2019

BPO 37120
Nosy @vstinner, @tiran, @alex, @njsmith, @dstufft, @miss-islington
PRs
  • bpo-37120: Add SSLContext.num_tickets (GH-13719) #13719
  • bpo-37120: Fix _ssl get_num_tickets() #14668
  • [3.8] bpo-37120: Fix _ssl get_num_tickets() (GH-14668) #14671
  • 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-19.19:59:46.185>
    created_at = <Date 2019-06-01.05:05:29.206>
    labels = ['expert-SSL', 'type-feature', '3.8', '3.9']
    title = 'Provide knobs to disable session ticket generation on TLS 1.3'
    updated_at = <Date 2021-04-19.19:59:46.185>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2021-04-19.19:59:46.185>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-19.19:59:46.185>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2019-06-01.05:05:29.206>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37120
    keywords = ['patch']
    message_count = 8.0
    messages = ['344148', '344156', '344463', '344466', '344677', '345914', '347548', '347557']
    nosy_count = 7.0
    nosy_names = ['janssen', 'vstinner', 'christian.heimes', 'alex', 'njs', 'dstufft', 'miss-islington']
    pr_nums = ['13719', '14668', '14671']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue37120'
    versions = ['Python 3.8', 'Python 3.9']

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Jun 1, 2019

    Maybe we should expose the SSL_CTX_set_num_tickets function:

    https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_num_tickets.html

    This is a new function added in OpenSSL 1.1.1, that lets you control the number of tickets issued by a TLS 1.3 connection.

    It turns out that by default, openssl 1.1.1 issues 2 tickets at the end of the server handshake. In principle this can cause deadlocks and data corruption:

    openssl/openssl#7967
    openssl/openssl#7948

    And my problem specifically is that this pretty much kills all of Trio's fancy protocol testing helpers, because any protocol that's built on top of TLS is automatically broken as far as the helpers are concerned. And they're right. Right now I have to disable TLS 1.3 entirely to get Trio's test suite to avoid deadlocking.

    Hopefully the openssl devs will fix this, but so far their latest comment is that they consider this a feature and so they think it has to stay broken for at least RHEL 8's lifetime.

    Currently the only workaround is to either disable TLS 1.3, or disable tickets. But the 'ssl' module doesn't expose any way to control tickets. This issue proposes to add that.

    Fortunately, exposing SSL_CTX_set_num_tickets should be pretty trivial, at least in theory.

    Questions:

    Do we have a preferred convention for how to expose these kinds of settings at the Python level? Attribute on SSLContext?

    There's both an SSL* and a SSL_CTX* – I guess the CTX version is sufficient, or is there another convention?

    As a bonus complication, openssl sometimes ignores the configured number of tickets, and uses a completely different mechanism:

    The default number of tickets is 2; the default number of tickets sent following a resumption handshake is 1 but this cannot be changed using these functions. The number of tickets following a resumption handshake can be reduced to 0 using custom session ticket callbacks (see https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_ticket_cb.html)

    Do we want to open the can-of-worms involved in wrapping this too? I think if we only wrapped SSL_CTX_set_num_tickets then that would be enough to let me kluge my tests into passing and pretend that things are just fine, so long as we don't test session resumption...

    @njsmith njsmith added 3.8 only security fixes 3.9 only security fixes labels Jun 1, 2019
    @njsmith njsmith added topic-SSL type-feature A feature request or enhancement labels Jun 1, 2019
    @tiran
    Copy link
    Member

    tiran commented Jun 1, 2019

    +1 for the idea

    Yes, for simple flags and settings, an attribute on the SSLContext is prefer. The SSLContext object is the configuration space for its connections. I would prefer to keep the setting only on the context and not clutter SSLSocket and SSLObject with additional attributes. PR 13719 goes one step further and restricts the setter to a PROTOCOL_TLS_SERVER, too.

    Let's look in the callback another time. I won't be able to come up with a wrapper for that in the next three days.

    @tiran
    Copy link
    Member

    tiran commented Jun 3, 2019

    New changeset 78c7d52 by Christian Heimes in branch 'master':
    bpo-37120: Add SSLContext.num_tickets (GH-13719)
    78c7d52

    @tiran
    Copy link
    Member

    tiran commented Jun 3, 2019

    I have merged the PR to land the feature in time for the feature freeze.

    Regarding your comment on client_context.num_ticket getter: IMHO it's not a good idea to raise an exception on attribute access. It may break introspection.

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Jun 5, 2019

    Regarding your comment on client_context.num_ticket getter: IMHO it's not a good idea to raise an exception on attribute access. It may break introspection.

    Hmm, I see what you mean.

    Basically my intuition is: it's a bit weird to make the attribute's existence "sort of" depend on whether it's a client or server context. It would make sense to me to have it entirely disappear on client contexts (from __dir__, read-access, and write-access), and it would make sense to me to have it always be present, just a no-op. But having it present, and almost the same as on server contexts, except that assigning to it fails... that feels a little weird.

    @vstinner
    Copy link
    Member

    New changeset 78c7d52 by Christian Heimes in branch 'master':
    bpo-37120: Add SSLContext.num_tickets (GH-13719)
    78c7d52

    This change introduced this warning on Windows:

    c:\vstinner\python\master\modules\_ssl.c(3624): warning C4267: 'function': conv
    ersion from 'size_t' to 'long', possible loss of data [C:\vstinner\python\maste
    r\PCbuild\_ssl.vcxproj]

    SSL_CTX_get_num_tickets() return type is size_t. I suggest this change:

    diff --git a/Modules/_ssl.c b/Modules/_ssl.c
    index 2331c58ad7..3ffb6380d3 100644
    --- a/Modules/_ssl.c
    +++ b/Modules/_ssl.c
    @@ -3621,7 +3621,7 @@ set_maximum_version(PySSLContext *self, PyObject *arg, void *c)
     static PyObject *
     get_num_tickets(PySSLContext *self, void *c)
     {
    -    return PyLong_FromLong(SSL_CTX_get_num_tickets(self->ctx));
    +    return PyLong_FromSize_t(SSL_CTX_get_num_tickets(self->ctx));
     }
     
     static int

    @miss-islington
    Copy link
    Contributor

    New changeset 76611c7 by Miss Islington (bot) (Victor Stinner) in branch 'master':
    bpo-37120: Fix _ssl get_num_tickets() (GH-14668)
    76611c7

    @miss-islington
    Copy link
    Contributor

    New changeset bbad695 by Miss Islington (bot) in branch '3.8':
    bpo-37120: Fix _ssl get_num_tickets() (GH-14668)
    bbad695

    @tiran tiran closed this as completed Apr 19, 2021
    @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.8 only security fixes 3.9 only security fixes topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants