| 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
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
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.
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?
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: