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

SSLContext's check_hostname needlessly intertwined with SNI #67110

Closed
dstufft opened this issue Nov 23, 2014 · 11 comments
Closed

SSLContext's check_hostname needlessly intertwined with SNI #67110

dstufft opened this issue Nov 23, 2014 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@dstufft
Copy link
Member

dstufft commented Nov 23, 2014

BPO 22921
Nosy @pitrou, @tiran, @benjaminp, @alex, @dstufft
Files
  • check-hostname-no-sni.patch
  • check-hostname-no-sni-with-docs.patch
  • check-hostname-no-sni-with-docs-2.patch
  • check-hostname-no-sni-with-docs-3.patch
  • check-hostname-no-sni-with-docs-py27.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 = None
    closed_at = <Date 2014-11-24.02:13:49.731>
    created_at = <Date 2014-11-23.05:27:45.388>
    labels = ['type-bug', 'library']
    title = "SSLContext's check_hostname needlessly intertwined with SNI"
    updated_at = <Date 2014-11-24.02:17:25.024>
    user = 'https://github.com/dstufft'

    bugs.python.org fields:

    activity = <Date 2014-11-24.02:17:25.024>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-11-24.02:13:49.731>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2014-11-23.05:27:45.388>
    creator = 'dstufft'
    dependencies = []
    files = ['37256', '37258', '37259', '37260', '37262']
    hgrepos = []
    issue_num = 22921
    keywords = ['patch']
    message_count = 11.0
    messages = ['231544', '231557', '231575', '231576', '231578', '231580', '231581', '231582', '231585', '231586', '231587']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'christian.heimes', 'benjamin.peterson', 'alex', 'python-dev', 'dstufft']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22921'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 23, 2014

    The SSLContext().wrap_socket() method allows you to pass in a server_hostname option which will be used for two purposes, it will be used as the server name for SNI and it will be used to verify the server name of the certificate. However currently if the OpenSSL you're using does not have SNI then sending the server_hostname option to wrap_socket() will raise a ValueError.

    I think that instead server_hostname should always be accepted by SSLContext().wrap_socket() regardless of if SNI is available or if check_hostname is available. It's just going to be stored and used later so we can conditonally use it for SNI or for checking the hostname depending on if SNI is available or checking if a hostname is available. The way it works right now is that unless you're happy not working when SNI is not available you have to check the hostname yourself.

    If we can fix this, I think it would be smart to do it ASAP and get it into Python 2.7.9 and backported to the various Python 3.x's so that in the near future it works with all recent versions of the various Pythons (though older micro releases it may not).

    This shouldn't break any code since it's changing what used to be an error into a saner working case.

    @dstufft dstufft added the type-feature A feature request or enhancement label Nov 23, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Nov 23, 2014

    This sounds ok to me, but are there still SNI-less OpenSSLs around?

    @pitrou pitrou added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Nov 23, 2014
    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 23, 2014

    I tested this patch on Python 3.5 compiled on CentOS 5.11 which does not have SNI enabled. The end result is that you can use server_hostname even when SNI isn't there to enable the SSL certificate checks. Of course the check will fail if the host your connecting to requires SNI to serve the expected certificate, but that's no different than it is today.

    The docs still need updated, I can do that a little bit later today, but figured I'd let people review this since it's done and working other than the docs.

    The basic gist of the patch is that we stash the hostname and use it for the validation checks, but we don't send it deeper into the stack if SNI is not available.

    @tiran
    Copy link
    Member

    tiran commented Nov 23, 2014

    Thanks a lot, Donald!

    Back then I didn't pursue the point because I wasn't sure about possible security implications.

    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 23, 2014

    Added docs.

    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 23, 2014

    A new patch that achieves the same thing in a simpler way at benjamin's suggestion.

    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 23, 2014

    Uploaded a third patch, this is the same technique as in the -2 patch, except it fixes a missed spot in Lib/ssl.py where I needed a better error message.

    Additionally this goes through and unskips all of the tests that were marked as depending on HAS_SNI when what they really depended on was the ability to set SSLContext().check_hostname = True.

    This also fixes a number of tests that are currently failing whenever HAS_SNI = False that started to fail as fallout of PEP-476.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 23, 2014

    New changeset f2d4beb90a5b by Benjamin Peterson in branch '3.4':
    don't require OpenSSL SNI to pass hostname to ssl functions (bpo-22921)
    https://hg.python.org/cpython/rev/f2d4beb90a5b

    New changeset 24dfe7310cc1 by Benjamin Peterson in branch 'default':
    merge 3.4 (bpo-22921)
    https://hg.python.org/cpython/rev/24dfe7310cc1

    @dstufft
    Copy link
    Member Author

    dstufft commented Nov 24, 2014

    Added a patch for Python 2.7

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 24, 2014

    New changeset ce4073afd992 by Benjamin Peterson in branch '2.7':
    allow hostname to be passed to SSLContext even if OpenSSL doesn't support SNI (closes bpo-22921)
    https://hg.python.org/cpython/rev/ce4073afd992

    @python-dev python-dev mannequin closed this as completed Nov 24, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 24, 2014

    New changeset 40f9e91f3626 by Benjamin Peterson in branch '2.7':
    add NEWS note for bpo-22921
    https://hg.python.org/cpython/rev/40f9e91f3626

    New changeset 060fd5d09063 by Benjamin Peterson in branch '3.4':
    add NEWS note for bpo-22921
    https://hg.python.org/cpython/rev/060fd5d09063

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants