Patchwork [2,of,6] mail: remove redundant call to SSL socket validator

login
register
mail settings
Submitter Gregory Szorc
Date March 28, 2016, 6:21 a.m.
Message ID <cf65be71e39936624bf3.1459146091@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/14100/
State Changes Requested
Headers show

Comments

Gregory Szorc - March 28, 2016, 6:21 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1459145731 25200
#      Sun Mar 27 23:15:31 2016 -0700
# Node ID cf65be71e39936624bf39041c93b94e66a45b881
# Parent  78f292d3f2c09f55d1aa62e5926b3888635a2426
mail: remove redundant call to SSL socket validator

Validation is now performed at socket wrapping time, so the
existing call is redundant.

To ensure strict socket validation is performed, we pass the
appropriate argument to the socket wrapping function.

We had to add serverhostname to the ssl arguments because it isn't
passed otherwise. Without it, we can't perform hostname or
certificate validation.
Julien Cristau - March 28, 2016, 6:58 p.m.
On Sun, Mar 27, 2016 at 23:21:31 -0700, Gregory Szorc wrote:

> @@ -129,17 +139,16 @@ def _smtp(ui):
>      s.connect(host=mailhost, port=mailport)
>      if starttls:
>          ui.note(_('(using starttls)\n'))
>          s.ehlo()
>          s.starttls()
>          s.ehlo()
>      if (starttls or smtps) and verifycert:
>          ui.note(_('(verifying remote certificate)\n'))
> -        sslutil.validator(ui, mailhost)(s.sock, verifycert == 'strict')

This ui.note becomes a bit of a lie, since the verification is now done
at connect or starttls time.  Maybe just remove the block?

Cheers,
Julien
Yuya Nishihara - March 29, 2016, 2:52 p.m.
On Sun, 27 Mar 2016 23:21:31 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1459145731 25200
> #      Sun Mar 27 23:15:31 2016 -0700
> # Node ID cf65be71e39936624bf39041c93b94e66a45b881
> # Parent  78f292d3f2c09f55d1aa62e5926b3888635a2426
> mail: remove redundant call to SSL socket validator
> 
> Validation is now performed at socket wrapping time, so the
> existing call is redundant.
> 
> To ensure strict socket validation is performed, we pass the
> appropriate argument to the socket wrapping function.
> 
> We had to add serverhostname to the ssl arguments because it isn't
> passed otherwise. Without it, we can't perform hostname or
> certificate validation.
> 
> diff --git a/mercurial/mail.py b/mercurial/mail.py
> --- a/mercurial/mail.py
> +++ b/mercurial/mail.py
> @@ -96,27 +96,37 @@ def _smtp(ui):
>      # backward compatible: when tls = true, we use starttls.
>      starttls = tls == 'starttls' or util.parsebool(tls)
>      smtps = tls == 'smtps'
>      if (starttls or smtps) and not util.safehasattr(socket, 'ssl'):
>          raise error.Abort(_("can't use TLS: Python SSL support not installed"))
>      mailhost = ui.config('smtp', 'host')
>      if not mailhost:
>          raise error.Abort(_('smtp.host not configured - cannot send mail'))
> +
> +    # There are 3 config values for cert verification: "strict", "loose," and
> +    # False. The first two perform hostname and fingerprint verification.
> +    # "strict" requires that a CA cert be trusted or a fingerprint be defined.
>      verifycert = ui.config('smtp', 'verifycert', 'strict')
>      if verifycert not in ['strict', 'loose']:
>          if util.parsebool(verifycert) is not False:
>              raise error.Abort(_('invalid smtp.verifycert configuration: %s')
>                               % (verifycert))
>          verifycert = False
>      if (starttls or smtps) and verifycert:
>          sslkwargs = sslutil.sslkwargs(ui, mailhost)
> +
> +        sslkwargs['serverhostname'] = mailhost
> +
> +        # Passed to the validator.
> +        if verifycert == 'strict':
> +            sslkwargs['requirefingerprintwhennocacerts'] = True
>      else:
>          # 'ui' is required by sslutil.wrapsocket() and set by sslkwargs()
> -        sslkwargs = {'ui': ui}
> +        sslkwargs = {'ui': ui, 'serverhostname': mailhost}

Looks like the certificates are verified even if smtp.verifycert is off.
Pierre-Yves David - April 1, 2016, 12:35 a.m.
On 03/29/2016 07:52 AM, Yuya Nishihara wrote:
> On Sun, 27 Mar 2016 23:21:31 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1459145731 25200
>> #      Sun Mar 27 23:15:31 2016 -0700
>> # Node ID cf65be71e39936624bf39041c93b94e66a45b881
>> # Parent  78f292d3f2c09f55d1aa62e5926b3888635a2426
>> mail: remove redundant call to SSL socket validator
>>
>> Validation is now performed at socket wrapping time, so the
>> existing call is redundant.
>>
>> To ensure strict socket validation is performed, we pass the
>> appropriate argument to the socket wrapping function.
>>
>> We had to add serverhostname to the ssl arguments because it isn't
>> passed otherwise. Without it, we can't perform hostname or
>> certificate validation.
>>
>> diff --git a/mercurial/mail.py b/mercurial/mail.py
>> --- a/mercurial/mail.py
>> +++ b/mercurial/mail.py
>> @@ -96,27 +96,37 @@ def _smtp(ui):
>>       # backward compatible: when tls = true, we use starttls.
>>       starttls = tls == 'starttls' or util.parsebool(tls)
>>       smtps = tls == 'smtps'
>>       if (starttls or smtps) and not util.safehasattr(socket, 'ssl'):
>>           raise error.Abort(_("can't use TLS: Python SSL support not installed"))
>>       mailhost = ui.config('smtp', 'host')
>>       if not mailhost:
>>           raise error.Abort(_('smtp.host not configured - cannot send mail'))
>> +
>> +    # There are 3 config values for cert verification: "strict", "loose," and
>> +    # False. The first two perform hostname and fingerprint verification.
>> +    # "strict" requires that a CA cert be trusted or a fingerprint be defined.
>>       verifycert = ui.config('smtp', 'verifycert', 'strict')
>>       if verifycert not in ['strict', 'loose']:
>>           if util.parsebool(verifycert) is not False:
>>               raise error.Abort(_('invalid smtp.verifycert configuration: %s')
>>                                % (verifycert))
>>           verifycert = False
>>       if (starttls or smtps) and verifycert:
>>           sslkwargs = sslutil.sslkwargs(ui, mailhost)
>> +
>> +        sslkwargs['serverhostname'] = mailhost
>> +
>> +        # Passed to the validator.
>> +        if verifycert == 'strict':
>> +            sslkwargs['requirefingerprintwhennocacerts'] = True
>>       else:
>>           # 'ui' is required by sslutil.wrapsocket() and set by sslkwargs()
>> -        sslkwargs = {'ui': ui}
>> +        sslkwargs = {'ui': ui, 'serverhostname': mailhost}
>
> Looks like the certificates are verified even if smtp.verifycert is off.

I'm not sure why you say that (neither why this would be True  or 
False). Can you elaborate?
Yuya Nishihara - April 1, 2016, 12:47 p.m.
On Thu, 31 Mar 2016 17:35:19 -0700, Pierre-Yves David wrote:
> On 03/29/2016 07:52 AM, Yuya Nishihara wrote:
> > On Sun, 27 Mar 2016 23:21:31 -0700, Gregory Szorc wrote:  
> >> +    # False. The first two perform hostname and fingerprint verification.
> >> +    # "strict" requires that a CA cert be trusted or a fingerprint be defined.
> >>       verifycert = ui.config('smtp', 'verifycert', 'strict')
> >>       if verifycert not in ['strict', 'loose']:
> >>           if util.parsebool(verifycert) is not False:
> >>               raise error.Abort(_('invalid smtp.verifycert configuration: %s')
> >>                                % (verifycert))
> >>           verifycert = False
> >>       if (starttls or smtps) and verifycert:
> >>           sslkwargs = sslutil.sslkwargs(ui, mailhost)
> >> +
> >> +        sslkwargs['serverhostname'] = mailhost
> >> +
> >> +        # Passed to the validator.
> >> +        if verifycert == 'strict':
> >> +            sslkwargs['requirefingerprintwhennocacerts'] = True
> >>       else:
> >>           # 'ui' is required by sslutil.wrapsocket() and set by sslkwargs()
> >> -        sslkwargs = {'ui': ui}
> >> +        sslkwargs = {'ui': ui, 'serverhostname': mailhost}  
> >
> > Looks like the certificates are verified even if smtp.verifycert is off.  
> 
> I'm not sure why you say that (neither why this would be True  or
> False). Can you elaborate?

Before, sslutil.validator() was called only if (starttls or smtps) and verifycert.
But this patch adds 'serverhostname' to the "else" case, which is the flag to
trigger sslutil.validator() in sslutil.wrapsocket(). That's why I guessed it
would break --config smtp.verifycert=off (False, or 0).

I've never tested, so I might be wrong. But we should be careful because
smtp.verifycert isn't covered by our test suite.

Patch

diff --git a/mercurial/mail.py b/mercurial/mail.py
--- a/mercurial/mail.py
+++ b/mercurial/mail.py
@@ -96,27 +96,37 @@  def _smtp(ui):
     # backward compatible: when tls = true, we use starttls.
     starttls = tls == 'starttls' or util.parsebool(tls)
     smtps = tls == 'smtps'
     if (starttls or smtps) and not util.safehasattr(socket, 'ssl'):
         raise error.Abort(_("can't use TLS: Python SSL support not installed"))
     mailhost = ui.config('smtp', 'host')
     if not mailhost:
         raise error.Abort(_('smtp.host not configured - cannot send mail'))
+
+    # There are 3 config values for cert verification: "strict", "loose," and
+    # False. The first two perform hostname and fingerprint verification.
+    # "strict" requires that a CA cert be trusted or a fingerprint be defined.
     verifycert = ui.config('smtp', 'verifycert', 'strict')
     if verifycert not in ['strict', 'loose']:
         if util.parsebool(verifycert) is not False:
             raise error.Abort(_('invalid smtp.verifycert configuration: %s')
                              % (verifycert))
         verifycert = False
     if (starttls or smtps) and verifycert:
         sslkwargs = sslutil.sslkwargs(ui, mailhost)
+
+        sslkwargs['serverhostname'] = mailhost
+
+        # Passed to the validator.
+        if verifycert == 'strict':
+            sslkwargs['requirefingerprintwhennocacerts'] = True
     else:
         # 'ui' is required by sslutil.wrapsocket() and set by sslkwargs()
-        sslkwargs = {'ui': ui}
+        sslkwargs = {'ui': ui, 'serverhostname': mailhost}
     if smtps:
         ui.note(_('(using smtps)\n'))
         s = SMTPS(sslkwargs, local_hostname=local_hostname)
     elif starttls:
         s = STARTTLS(sslkwargs, local_hostname=local_hostname)
     else:
         s = smtplib.SMTP(local_hostname=local_hostname)
     if smtps:
@@ -129,17 +139,16 @@  def _smtp(ui):
     s.connect(host=mailhost, port=mailport)
     if starttls:
         ui.note(_('(using starttls)\n'))
         s.ehlo()
         s.starttls()
         s.ehlo()
     if (starttls or smtps) and verifycert:
         ui.note(_('(verifying remote certificate)\n'))
-        sslutil.validator(ui, mailhost)(s.sock, verifycert == 'strict')
     username = ui.config('smtp', 'username')
     password = ui.config('smtp', 'password')
     if username and not password:
         password = ui.getpass()
     if username and password:
         ui.note(_('(authenticating to mail server as %s)\n') %
                   (username))
         try: