| Submitter | Gregory Szorc |
|---|---|
| Date | June 1, 2016, 2:21 a.m. |
| Message ID | <a17ef4b3ffc9394a49ed.1464747717@ubuntu-vm-main> |
| Download | mbox | patch |
| Permalink | /patch/15307/ |
| State | Superseded |
| Headers | show |
Comments
On Tue, 31 May 2016 19:21:57 -0700, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1464747586 25200 > # Tue May 31 19:19:46 2016 -0700 > # Node ID a17ef4b3ffc9394a49edf4b509fbac04d223b324 > # Parent 48b38b16a8f83ea98ebdf0b370f59fd90dc17935 > mail: unsupport smtp.verifycert (BC) > > smtp.verifycert was accidentally broken by cca59ef27e60. The current > code refuses to talk to a remote server unless the CA is trusted or > the fingerprint is validated. In other words, we lost the ability > for smtp.verifycert to lower/disable security. > > There are special considerations for smtp.verifycert in > sslutil.validatesocket() (the "strict" argument). This violates > the direction sslutil is evolving towards, which has all security > options determined at wrapsocket() time and a unified code path and > configs for determining security options. > > Since smtp.verifycert is broken and since we'll soon have new > security defaults and new mechanisms for controlling host security, > formally deprecate smtp.verifycert. With this patch, the socket > security code in mail.py now effectively mirrors code in url.py and > other places we're doing socket security. (+CC foozy as he should know the detail of smtp.verifycert) I second this change, but "broken by cca59ef27e60" shouldn't be a reason. It isn't even released. > -``verifycert`` > - Optional. Verification for the certificate of mail server, when > - ``tls`` is starttls or smtps. "strict", "loose" or False. For > - "strict" or "loose", the certificate is verified as same as the > - verification for HTTPS connections (see ``[hostfingerprints]`` and > - ``[web] cacerts`` also). For "strict", sending email is also > - aborted, if there is no configuration for mail server in > - ``[hostfingerprints]`` and ``[web] cacerts``. --insecure for > - :hg:`email` overwrites this as "loose". (default: strict) IIRC, this option was introduced to mitigate the risk of breaking change in stable release, where user has web.cacerts but his SMTP server might provide a self-signed certificate, for example. For this purpose, "loose" seems useless. Also, "loose" appears not working from the beginning if cacerts are available. Now we're going to add a config knob to control per-host security features, and we'll provide a hint for it, I think dropping smtp.verifycert is acceptable. The series looks good to me except for a few nits.
At Wed, 1 Jun 2016 22:51:16 +0900, Yuya Nishihara wrote: > > On Tue, 31 May 2016 19:21:57 -0700, Gregory Szorc wrote: > > # HG changeset patch > > # User Gregory Szorc <gregory.szorc@gmail.com> > > # Date 1464747586 25200 > > # Tue May 31 19:19:46 2016 -0700 > > # Node ID a17ef4b3ffc9394a49edf4b509fbac04d223b324 > > # Parent 48b38b16a8f83ea98ebdf0b370f59fd90dc17935 > > mail: unsupport smtp.verifycert (BC) > > > > smtp.verifycert was accidentally broken by cca59ef27e60. The current > > code refuses to talk to a remote server unless the CA is trusted or > > the fingerprint is validated. In other words, we lost the ability > > for smtp.verifycert to lower/disable security. > > > > There are special considerations for smtp.verifycert in > > sslutil.validatesocket() (the "strict" argument). This violates > > the direction sslutil is evolving towards, which has all security > > options determined at wrapsocket() time and a unified code path and > > configs for determining security options. > > > > Since smtp.verifycert is broken and since we'll soon have new > > security defaults and new mechanisms for controlling host security, > > formally deprecate smtp.verifycert. With this patch, the socket > > security code in mail.py now effectively mirrors code in url.py and > > other places we're doing socket security. > > (+CC foozy as he should know the detail of smtp.verifycert) > > I second this change, but "broken by cca59ef27e60" shouldn't be a reason. > It isn't even released. > > > -``verifycert`` > > - Optional. Verification for the certificate of mail server, when > > - ``tls`` is starttls or smtps. "strict", "loose" or False. For > > - "strict" or "loose", the certificate is verified as same as the > > - verification for HTTPS connections (see ``[hostfingerprints]`` and > > - ``[web] cacerts`` also). For "strict", sending email is also > > - aborted, if there is no configuration for mail server in > > - ``[hostfingerprints]`` and ``[web] cacerts``. --insecure for > > - :hg:`email` overwrites this as "loose". (default: strict) > > IIRC, this option was introduced to mitigate the risk of breaking change > in stable release, where user has web.cacerts but his SMTP server might > provide a self-signed certificate, for example. For this purpose, "loose" > seems useless. Also, "loose" appears not working from the beginning if > cacerts are available. FYI As Yuya said, smtp.verifycert was introduced for backward compatibility of existing tool chains. "loose" for smtp.verifycert is corresponded to security level of changeset exchanging via HTTPS in ordinary usecase. - if cacerts is available, validate server certificate strictly before starting communication - otherwise, start communication without validation, but show warning about it > Now we're going to add a config knob to control per-host security features, > and we'll provide a hint for it, I think dropping smtp.verifycert is > acceptable. I agree, too, Default value of smtp.verifycert has been "strict" since Mercurial 2.6 released at 2013-05-01. This seems long enough. > The series looks good to me except for a few nits. > ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
Patch
diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt --- a/mercurial/help/config.txt +++ b/mercurial/help/config.txt @@ -1481,26 +1481,16 @@ Configuration for extensions that need t ``port`` Optional. Port to connect to on mail server. (default: 465 if ``tls`` is smtps; 25 otherwise) ``tls`` Optional. Method to enable TLS when connecting to mail server: starttls, smtps or none. (default: none) -``verifycert`` - Optional. Verification for the certificate of mail server, when - ``tls`` is starttls or smtps. "strict", "loose" or False. For - "strict" or "loose", the certificate is verified as same as the - verification for HTTPS connections (see ``[hostfingerprints]`` and - ``[web] cacerts`` also). For "strict", sending email is also - aborted, if there is no configuration for mail server in - ``[hostfingerprints]`` and ``[web] cacerts``. --insecure for - :hg:`email` overwrites this as "loose". (default: strict) - ``username`` Optional. User name for authenticating with the SMTP server. (default: None) ``password`` Optional. Password for authenticating with the SMTP server. If not specified, interactive sessions will prompt the user for a password; non-interactive sessions will fail. (default: None) diff --git a/mercurial/mail.py b/mercurial/mail.py --- a/mercurial/mail.py +++ b/mercurial/mail.py @@ -101,23 +101,16 @@ 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')) - 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 smtps: ui.note(_('(using smtps)\n')) s = SMTPS(ui, local_hostname=local_hostname, host=mailhost) elif starttls: s = STARTTLS(ui, local_hostname=local_hostname, host=mailhost) else: s = smtplib.SMTP(local_hostname=local_hostname) if smtps: @@ -128,19 +121,19 @@ def _smtp(ui): ui.note(_('sending mail: smtp host %s, port %d\n') % (mailhost, mailport)) 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: + if starttls or smtps: ui.note(_('(verifying remote certificate)\n')) - sslutil.validatesocket(s.sock, verifycert == 'strict') + sslutil.validatesocket(s.sock) 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: