Patchwork [7,of,8,v5] sslutil: propagate return value ssl.PROTOCOL_SSLv23 from protocolsettings()

login
register
mail settings
Submitter Manuel Jacob
Date June 1, 2020, 3:28 a.m.
Message ID <64807e560eedc6c2571d.1590982098@tmp>
Download mbox | patch
Permalink /patch/46444/
State New
Headers show

Comments

Manuel Jacob - June 1, 2020, 3:28 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1590970587 -7200
#      Mon Jun 01 02:16:27 2020 +0200
# Node ID 64807e560eedc6c2571d34ffb7bd2f7e356dd606
# Parent  7576507bfe5ea28ab6d496d532bb9b453998ca35
# EXP-Topic require_modern_ssl
sslutil: propagate return value ssl.PROTOCOL_SSLv23 from protocolsettings()

Also, protocolsettings() was renamed to ssloptions() to reflect that only the
options are returned.

Also, the now unused 'protocol' entry of the config dict was removed.
Yuya Nishihara - June 1, 2020, 11:33 a.m.
On Mon, 01 Jun 2020 05:28:18 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1590970587 -7200
> #      Mon Jun 01 02:16:27 2020 +0200
> # Node ID 64807e560eedc6c2571d34ffb7bd2f7e356dd606
> # Parent  7576507bfe5ea28ab6d496d532bb9b453998ca35
> # EXP-Topic require_modern_ssl
> sslutil: propagate return value ssl.PROTOCOL_SSLv23 from protocolsettings()

> -def protocolsettings(minimumprotocol):
> +def ssloptions(minimumprotocol):
>      """Resolve the protocol for a config value.
>  
>      Returns a tuple of (protocol, options) which are values used by SSLContext.
> @@ -268,7 +265,7 @@ def protocolsettings(minimumprotocol):
>      # There is no guarantee this attribute is defined on the module.
>      options |= getattr(ssl, 'OP_NO_COMPRESSION', 0)
>  
> -    return ssl.PROTOCOL_SSLv23, options
> +    return options

Need to move/rephrase the following comment:

    # Despite its name, PROTOCOL_SSLv23 selects the highest protocol
    # that both ends support, including TLS protocols.
    #
    # The PROTOCOL_TLSv* constants select a specific TLS version
    # only (as opposed to multiple versions). So the method for
    # supporting multiple TLS versions is to use PROTOCOL_SSLv23 and
    # disable protocols via SSLContext.options and OP_NO_* constants.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -79,8 +79,6 @@  def _hostsettings(ui, hostname):
         b'disablecertverification': False,
         # Whether the legacy [hostfingerprints] section has data for this host.
         b'legacyfingerprint': False,
-        # PROTOCOL_* constant to use for SSLContext.__init__.
-        b'protocol': None,
         # String representation of minimum protocol to be used for UI
         # presentation.
         b'minimumprotocolui': None,
@@ -126,7 +124,7 @@  def _hostsettings(ui, hostname):
         minimumprotocol = b'tls1.0'
 
     s[b'minimumprotocolui'] = minimumprotocol
-    s[b'protocol'], s[b'ctxoptions'] = protocolsettings(minimumprotocol)
+    s[b'ctxoptions'] = ssloptions(minimumprotocol)
 
     ciphers = ui.config(b'hostsecurity', b'ciphers')
     ciphers = ui.config(b'hostsecurity', b'%s:ciphers' % bhostname, ciphers)
@@ -228,14 +226,13 @@  def _hostsettings(ui, hostname):
             # user).
             s[b'verifymode'] = ssl.CERT_NONE
 
-    assert s[b'protocol'] is not None
     assert s[b'ctxoptions'] is not None
     assert s[b'verifymode'] is not None
 
     return s
 
 
-def protocolsettings(minimumprotocol):
+def ssloptions(minimumprotocol):
     """Resolve the protocol for a config value.
 
     Returns a tuple of (protocol, options) which are values used by SSLContext.
@@ -268,7 +265,7 @@  def protocolsettings(minimumprotocol):
     # There is no guarantee this attribute is defined on the module.
     options |= getattr(ssl, 'OP_NO_COMPRESSION', 0)
 
-    return ssl.PROTOCOL_SSLv23, options
+    return options
 
 
 def wrapsocket(sock, keyfile, certfile, ui, serverhostname=None):
@@ -323,7 +320,7 @@  def wrapsocket(sock, keyfile, certfile, 
     # bundle with a specific CA cert removed. If the system/default CA bundle
     # is loaded and contains that removed CA, you've just undone the user's
     # choice.
-    sslcontext = ssl.SSLContext(settings[b'protocol'])
+    sslcontext = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
     sslcontext.options |= settings[b'ctxoptions']
     sslcontext.verify_mode = settings[b'verifymode']
 
@@ -520,7 +517,8 @@  def wrapserversocket(
                 _(b'referenced certificate file (%s) does not exist') % f
             )
 
-    protocol, options = protocolsettings(b'tls1.0')
+    protocol = ssl.PROTOCOL_SSLv23
+    options = ssloptions(b'tls1.0')
 
     # This config option is intended for use in tests only. It is a giant
     # footgun to kill security. Don't define it.