Patchwork [4,of,5] sslutil: require a server hostname when wrapping sockets (API)

login
register
mail settings
Submitter Gregory Szorc
Date April 10, 2016, 6:04 p.m.
Message ID <3d9c3f8bfed01b5ab163.1460311476@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/14490/
State Accepted
Headers show

Comments

Gregory Szorc - April 10, 2016, 6:04 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1460311241 25200
#      Sun Apr 10 11:00:41 2016 -0700
# Node ID 3d9c3f8bfed01b5ab163bc95180576a79e6016ef
# Parent  a0c629f58c3ed8acfa008a12415c172f315c70d3
sslutil: require a server hostname when wrapping sockets (API)

All callers appear to be passing the hostname. So this shouldn't
break anything. By specifying the hostname, more validation options
from the ssl module are available to us. Although this patch stops
short of using them.
Yuya Nishihara - April 11, 2016, 11:22 a.m.
On Sun, 10 Apr 2016 11:04:36 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1460311241 25200
> #      Sun Apr 10 11:00:41 2016 -0700
> # Node ID 3d9c3f8bfed01b5ab163bc95180576a79e6016ef
> # Parent  a0c629f58c3ed8acfa008a12415c172f315c70d3
> sslutil: require a server hostname when wrapping sockets (API)
> 
> All callers appear to be passing the hostname. So this shouldn't
> break anything. By specifying the hostname, more validation options
> from the ssl module are available to us. Although this patch stops
> short of using them.
> 
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -115,16 +115,19 @@ def wrapsocket(sock, keyfile, certfile, 
>  
>      In addition to the arguments supported by ``ssl.wrap_socket``, we allow
>      the following additional arguments:
>  
>      * serverhostname - The expected hostname of the remote server. If the
>        server (and client) support SNI, this tells the server which certificate
>        to use.
>      """
> +    if not serverhostname:
> +        raise error.Abort('serverhostname argument required')

% hg --config smtp.verifycert=false email tip --trace
Traceback (most recent call last):
  File "mercurial/dispatch.py", line 204, in _runcatch
    return _dispatch(req)
  File "mercurial/dispatch.py", line 887, in _dispatch
    cmdpats, cmdoptions)
  File "mercurial/dispatch.py", line 632, in runcommand
    ret = _runcommand(ui, options, cmd, d)
  File "mercurial/extensions.py", line 204, in closure
    return func(*(args + a), **kw)
  File "hgext/pager.py", line 160, in pagecmd
    return orig(ui, options, cmd, cmdfunc)
  File "mercurial/extensions.py", line 204, in closure
    return func(*(args + a), **kw)
  File "hgext/color.py", line 494, in colorcmd
    return orig(ui_, opts, cmd, cmdfunc)
  File "mercurial/dispatch.py", line 1018, in _runcommand
    return checkargs()
  File "mercurial/dispatch.py", line 978, in checkargs
    return cmdfunc()
  File "mercurial/dispatch.py", line 884, in <lambda>
    d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
  File "mercurial/util.py", line 1000, in check
    return func(*args, **kwargs)
  File "mercurial/extensions.py", line 204, in closure
    return func(*(args + a), **kw)
  File "mercurial/util.py", line 1000, in check
    return func(*args, **kwargs)
  File "hgext/mq.py", line 3519, in mqcommand
    return orig(ui, repo, *args, **kwargs)
  File "mercurial/util.py", line 1000, in check
    return func(*args, **kwargs)
  File "hgext/patchbomb.py", line 715, in email
    sendmail = mail.connect(ui, mbox=mbox)
  File "mercurial/mail.py", line 193, in connect
    return _smtp(ui)
  File "mercurial/mail.py", line 133, in _smtp
    s.starttls()
  File "mercurial/mail.py", line 62, in starttls
    **self._sslkwargs)
  File "mercurial/sslutil.py", line 124, in wrapsocket
    raise error.Abort('serverhostname argument required')
Abort: serverhostname argument required
abort: serverhostname argument required

Cheers,
Gregory Szorc - April 16, 2016, 4:10 p.m.
On Mon, Apr 11, 2016 at 4:22 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 10 Apr 2016 11:04:36 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1460311241 25200
> > #      Sun Apr 10 11:00:41 2016 -0700
> > # Node ID 3d9c3f8bfed01b5ab163bc95180576a79e6016ef
> > # Parent  a0c629f58c3ed8acfa008a12415c172f315c70d3
> > sslutil: require a server hostname when wrapping sockets (API)
> >
> > All callers appear to be passing the hostname. So this shouldn't
> > break anything. By specifying the hostname, more validation options
> > from the ssl module are available to us. Although this patch stops
> > short of using them.
> >
> > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> > --- a/mercurial/sslutil.py
> > +++ b/mercurial/sslutil.py
> > @@ -115,16 +115,19 @@ def wrapsocket(sock, keyfile, certfile,
> >
> >      In addition to the arguments supported by ``ssl.wrap_socket``, we
> allow
> >      the following additional arguments:
> >
> >      * serverhostname - The expected hostname of the remote server. If
> the
> >        server (and client) support SNI, this tells the server which
> certificate
> >        to use.
> >      """
> > +    if not serverhostname:
> > +        raise error.Abort('serverhostname argument required')
>
> % hg --config smtp.verifycert=false email tip --trace
> Traceback (most recent call last):
>   File "mercurial/dispatch.py", line 204, in _runcatch
>     return _dispatch(req)
>   File "mercurial/dispatch.py", line 887, in _dispatch
>     cmdpats, cmdoptions)
>   File "mercurial/dispatch.py", line 632, in runcommand
>     ret = _runcommand(ui, options, cmd, d)
>   File "mercurial/extensions.py", line 204, in closure
>     return func(*(args + a), **kw)
>   File "hgext/pager.py", line 160, in pagecmd
>     return orig(ui, options, cmd, cmdfunc)
>   File "mercurial/extensions.py", line 204, in closure
>     return func(*(args + a), **kw)
>   File "hgext/color.py", line 494, in colorcmd
>     return orig(ui_, opts, cmd, cmdfunc)
>   File "mercurial/dispatch.py", line 1018, in _runcommand
>     return checkargs()
>   File "mercurial/dispatch.py", line 978, in checkargs
>     return cmdfunc()
>   File "mercurial/dispatch.py", line 884, in <lambda>
>     d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
>   File "mercurial/util.py", line 1000, in check
>     return func(*args, **kwargs)
>   File "mercurial/extensions.py", line 204, in closure
>     return func(*(args + a), **kw)
>   File "mercurial/util.py", line 1000, in check
>     return func(*args, **kwargs)
>   File "hgext/mq.py", line 3519, in mqcommand
>     return orig(ui, repo, *args, **kwargs)
>   File "mercurial/util.py", line 1000, in check
>     return func(*args, **kwargs)
>   File "hgext/patchbomb.py", line 715, in email
>     sendmail = mail.connect(ui, mbox=mbox)
>   File "mercurial/mail.py", line 193, in connect
>     return _smtp(ui)
>   File "mercurial/mail.py", line 133, in _smtp
>     s.starttls()
>   File "mercurial/mail.py", line 62, in starttls
>     **self._sslkwargs)
>   File "mercurial/sslutil.py", line 124, in wrapsocket
>     raise error.Abort('serverhostname argument required')
> Abort: serverhostname argument required
> abort: serverhostname argument required
>

This got filed as 5203 and timeless submitted a patch the other day before
I had a chance to fix it.

Thank you, timeless!

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -115,16 +115,19 @@  def wrapsocket(sock, keyfile, certfile, 
 
     In addition to the arguments supported by ``ssl.wrap_socket``, we allow
     the following additional arguments:
 
     * serverhostname - The expected hostname of the remote server. If the
       server (and client) support SNI, this tells the server which certificate
       to use.
     """
+    if not serverhostname:
+        raise error.Abort('serverhostname argument required')
+
     # Despite its name, PROTOCOL_SSLv23 selects the highest protocol
     # that both ends support, including TLS protocols. On legacy stacks,
     # the highest it likely goes in TLS 1.0. On modern stacks, it can
     # support TLS 1.2.
     #
     # 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