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.

Sign up for GitHub

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

SSL: check_hostname should imply CERT_REQUIRED #75612

Closed
tiran opened this issue Sep 12, 2017 · 2 comments
Closed

SSL: check_hostname should imply CERT_REQUIRED #75612

tiran opened this issue Sep 12, 2017 · 2 comments
Assignees
@tiran
Labels
3.7 (EOL) end of life topic-SSL type-feature A feature request or enhancement

Comments

@tiran
Copy link
Member

tiran commented Sep 12, 2017

BPO 31431
Nosy @tiran, @alex, @dstufft
PRs
  • bpo-31431: SSLContext.check_hostname auto-sets CERT_REQUIRED #3531
    		•

    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-15.18:30:20.902>
created_at = <Date 2017-09-12.15:58:47.913>
labels = ['expert-SSL', 'type-feature', '3.7']
title = 'SSL: check_hostname should imply CERT_REQUIRED'
updated_at = <Date 2017-09-15.18:30:20.901>
user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2017-09-15.18:30:20.901>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2017-09-15.18:30:20.902>
closer = 'christian.heimes'
components = ['SSL']
creation = <Date 2017-09-12.15:58:47.913>
creator = 'christian.heimes'
dependencies = []
files = []
hgrepos = []
issue_num = 31431
keywords = ['patch']
message_count = 2.0
messages = ['301967', '302288']
nosy_count = 4.0
nosy_names = ['janssen', 'christian.heimes', 'alex', 'dstufft']
pr_nums = ['3531']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue31431'
versions = ['Python 3.7']
    @tiran
    Copy link
    Member Author

    tiran commented Sep 12, 2017

    Hostname verification makes not much sense without verifying the certificate chain first. At the moment one has to set verify_mode to CERT_REQUIRED first:

    >>> import ssl                                                                                                                                                                                                                                                                                                    
>>> ctx = ssl.SSLContext(ssl.PROTOCOL_TLS)
>>> ctx.check_hostname, ctx.verify_mode                                                                                                                                                                                                                                                                           
(False, <VerifyMode.CERT_NONE: 0>)

>>> ctx.check_hostname = True
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED
>>> ctx.verify_mode = ssl.CERT_REQUIRED
>>> ctx.check_hostname = True

    On the other hand verify mode cannot be set to CERT_NONE without disabling check_hostname first. One has to remember to set the values in opposite order!

    >>> ctx.verify_mode = ssl.CERT_NONE
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/heimes/dev/python/cpython/Lib/ssl.py", line 485, in verify_mode
    super(SSLContext, SSLContext).verify_mode.__set__(self, value)                                                                                                                                                                                                                                                
ValueError: Cannot set verify_mode to CERT_NONE when check_hostname is enabled.
>>> ctx.check_hostname = False
>>> ctx.verify_mode = ssl.CERT_NONE

    I find this confusing. In order to support PROTOCOL_TLS_CLIENT with _create_unverified_context(), I had to modify the code to this abomination:

        if not check_hostname:
        context.check_hostname = False
    if cert_reqs is not None:
        context.verify_mode = cert_reqs
    if check_hostname:
        context.check_hostname = True

    Rather than making our users to jump through additional hoops, check_hostname = True should just set CERT_REQUIRED. This is a sane and safe default. On the other hand, ssl.CERT_NONE shall *not* disable check_hostname and still fail with a ValueError if check_hostname is enabled.

    By the way we should not suggest CERT_OPTIONAL here, too. For TLS client connections, CERT_OPTIONAL is not really optional. CERT_OPTIONAL: SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, verify_cb), CERT_REQUIRED: SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, verify_cb). According to https://www.openssl.org/docs/man1.0.2/ssl/SSL_CTX_set_verify.html SSL_VERIFY_FAIL_IF_NO_PEER_CERT is ignored in client mode. I'll open a new bug report.

    @tiran tiran added the 3.7 (EOL) end of life label Sep 12, 2017
    @tiran tiran self-assigned this Sep 12, 2017
    @tiran tiran added topic-SSL type-feature A feature request or enhancement labels Sep 12, 2017
    @tiran
    Copy link
    Member Author

    tiran commented Sep 15, 2017

    New changeset e82c034 by Christian Heimes in branch 'master':
    bpo-31431: SSLContext.check_hostname auto-sets CERT_REQUIRED (bpo-3531)
    e82c034

    @tiran tiran closed this as completed Sep 15, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @temoto temoto mentioned this issue Nov 17, 2022
    Merged
    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 topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @tiran