Patchwork [STABLE] commands: print security protocol support in debuginstall

login
register
mail settings
Submitter Gregory Szorc
Date Oct. 19, 2016, 10:16 p.m.
Message ID <37eaf6c2b4ac3c101596.1476915375@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17178/
State Superseded
Headers show

Comments

Gregory Szorc - Oct. 19, 2016, 10:16 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1476914831 25200
#      Wed Oct 19 15:07:11 2016 -0700
# Branch stable
# Node ID 37eaf6c2b4ac3c1015965676db89e435a79b45ee
# Parent  e478f11e418288b8308457303d3ddf6a23f874f8
commands: print security protocol support in debuginstall

Over the past ~48 hours I've had to instruct multiple people to run
Python code to query the ssl module to see what TLS protocol support
is present. I think it would be useful for `hg debuginstall` to print
this info to make it easier to access and debug why Mercurial is
complaining about using an insecure TLS 1.0 protocol.

Ideally we'd also print the path to the CA cert bundle. But the APIs
for querying that in sslutil can emit warnings, making it slightly
more difficult to integrate into `hg debuginstall`. That work will
have to wait for another day.

Yes, I realize it is feature freeze. But I think this is useful to
have in the release and it only changes a debug* command, so it
shouldn't be that risky.
Augie Fackler - Oct. 20, 2016, 2:26 p.m.
On Wed, Oct 19, 2016 at 03:16:15PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1476914831 25200
> #      Wed Oct 19 15:07:11 2016 -0700
> # Branch stable
> # Node ID 37eaf6c2b4ac3c1015965676db89e435a79b45ee
> # Parent  e478f11e418288b8308457303d3ddf6a23f874f8
> commands: print security protocol support in debuginstall

I'm going to give this an enthusiastic +1. It's probably a little
sketchy to take it during the freeze, but I agree we should just do
it. If I don't hear objections before the end of my workday on Friday,
I'll take this.

>
> Over the past ~48 hours I've had to instruct multiple people to run
> Python code to query the ssl module to see what TLS protocol support
> is present. I think it would be useful for `hg debuginstall` to print
> this info to make it easier to access and debug why Mercurial is
> complaining about using an insecure TLS 1.0 protocol.
>
> Ideally we'd also print the path to the CA cert bundle. But the APIs
> for querying that in sslutil can emit warnings, making it slightly
> more difficult to integrate into `hg debuginstall`. That work will
> have to wait for another day.
>
> Yes, I realize it is feature freeze. But I think this is useful to
> have in the release and it only changes a debug* command, so it
> shouldn't be that risky.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -63,16 +63,17 @@ from . import (
>      pvec,
>      repair,
>      revlog,
>      revset,
>      scmutil,
>      setdiscovery,
>      simplemerge,
>      sshserver,
> +    sslutil,
>      streamclone,
>      templatekw,
>      templater,
>      treediscovery,
>      ui as uimod,
>      util,
>  )
>
> @@ -2698,16 +2699,34 @@ def debuginstall(ui, **opts):
>      # Python
>      fm.write('pythonexe', _("checking Python executable (%s)\n"),
>               sys.executable)
>      fm.write('pythonver', _("checking Python version (%s)\n"),
>               ("%s.%s.%s" % sys.version_info[:3]))
>      fm.write('pythonlib', _("checking Python lib (%s)...\n"),
>               os.path.dirname(os.__file__))
>
> +    security = set(sslutil.supportedprotocols)
> +    if sslutil.hassni:
> +        security.add('sni')
> +
> +    fm.write('pythonsecurity', _("checking Python security support (%s)\n"),
> +             ', '.join(sorted(security)))
> +
> +    # These are warnings, not errors. So don't increment problem count. This
> +    # may change in the future.
> +    fm.condwrite('tls1.2' not in security, 'tlswarning', '  %s\n',
> +                 _('TLS 1.2 not supported by Python install; '
> +                   'network connections lack modern security'))
> +    fm.condwrite('sni' not in security, 'sniwarning', '  %s\n',
> +                 _('SNI not supported by Python install; may have '
> +                   'connectivity issues with some servers'))
> +
> +    # TODO print CA cert info
> +
>      # hg version
>      hgver = util.version()
>      fm.write('hgver', _("checking Mercurial version (%s)\n"),
>               hgver.split('+')[0])
>      fm.write('hgverextra', _("checking Mercurial custom build (%s)\n"),
>               '+'.join(hgver.split('+')[1:]))
>
>      # compiled modules
> diff --git a/tests/test-install.t b/tests/test-install.t
> --- a/tests/test-install.t
> +++ b/tests/test-install.t
> @@ -1,14 +1,17 @@
>  hg debuginstall
>    $ hg debuginstall
>    checking encoding (ascii)...
>    checking Python executable (*) (glob)
>    checking Python version (2.*) (glob)
>    checking Python lib (*lib*)... (glob)
> +  checking Python security support (*) (glob)
> +    TLS 1.2 not supported by Python install; network connections lack modern security (?)
> +    SNI not supported by Python install; may have connectivity issues with some servers (?)
>    checking Mercurial version (*) (glob)
>    checking Mercurial custom build (*) (glob)
>    checking module policy (*) (glob)
>    checking installed modules (*mercurial)... (glob)
>    checking templates (*mercurial?templates)... (glob)
>    checking default template (*mercurial?templates?map-cmdline.default) (glob)
>    checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
>    checking username (test)
> @@ -28,30 +31,36 @@ hg debuginstall JSON
>      "extensionserror": null,
>      "hgmodulepolicy": "*", (glob)
>      "hgmodules": "*mercurial", (glob)
>      "hgver": "*", (glob)
>      "hgverextra": "*", (glob)
>      "problems": 0,
>      "pythonexe": "*", (glob)
>      "pythonlib": "*", (glob)
> +    "pythonsecurity": "*", (glob)
>      "pythonver": "*.*.*", (glob)
> +    "sniwarning": "SNI not supported by Python install; may have connectivity issues with some servers",
>      "templatedirs": "*mercurial?templates", (glob)
> +    "tlswarning": "TLS 1.2 not supported by Python install; network connections lack modern security",
>      "username": "test",
>      "usernameerror": null,
>      "vinotfound": false
>     }
>    ]
>
>  hg debuginstall with no username
>    $ HGUSER= hg debuginstall
>    checking encoding (ascii)...
>    checking Python executable (*) (glob)
>    checking Python version (2.*) (glob)
>    checking Python lib (*lib*)... (glob)
> +  checking Python security support (*) (glob)
> +    TLS 1.2 not supported by Python install; network connections lack modern security (?)
> +    SNI not supported by Python install; may have connectivity issues with some servers (?)
>    checking Mercurial version (*) (glob)
>    checking Mercurial custom build (*) (glob)
>    checking module policy (*) (glob)
>    checking installed modules (*mercurial)... (glob)
>    checking templates (*mercurial?templates)... (glob)
>    checking default template (*mercurial?templates?map-cmdline.default) (glob)
>    checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
>    checking username...
> @@ -66,16 +75,19 @@ path variables are expanded (~ is the sa
>  #if execbit
>    $ chmod 755 tools/testeditor.exe
>  #endif
>    $ hg debuginstall --config ui.editor=~/tools/testeditor.exe
>    checking encoding (ascii)...
>    checking Python executable (*) (glob)
>    checking Python version (*) (glob)
>    checking Python lib (*lib*)... (glob)
> +  checking Python security support (*) (glob)
> +    TLS 1.2 not supported by Python install; network connections lack modern security (?)
> +    SNI not supported by Python install; may have connectivity issues with some servers (?)
>    checking Mercurial version (*) (glob)
>    checking Mercurial custom build (*) (glob)
>    checking module policy (*) (glob)
>    checking installed modules (*mercurial)... (glob)
>    checking templates (*mercurial?templates)... (glob)
>    checking default template (*mercurial?templates?map-cmdline.default) (glob)
>    checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
>    checking username (test)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 20, 2016, 3:49 p.m.
On Thu, 20 Oct 2016 10:26:20 -0400, Augie Fackler wrote:
> On Wed, Oct 19, 2016 at 03:16:15PM -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1476914831 25200
> > #      Wed Oct 19 15:07:11 2016 -0700
> > # Branch stable
> > # Node ID 37eaf6c2b4ac3c1015965676db89e435a79b45ee
> > # Parent  e478f11e418288b8308457303d3ddf6a23f874f8
> > commands: print security protocol support in debuginstall
> 
> I'm going to give this an enthusiastic +1. It's probably a little
> sketchy to take it during the freeze, but I agree we should just do
> it. If I don't hear objections before the end of my workday on Friday,
> I'll take this.

+1

A few nits from a templater expert. ;-)

> > +    security = set(sslutil.supportedprotocols)
> > +    if sslutil.hassni:
> > +        security.add('sni')
> > +
> > +    fm.write('pythonsecurity', _("checking Python security support (%s)\n"),
> > +             ', '.join(sorted(security)))

Please use fm.formatlist().

> > +    # These are warnings, not errors. So don't increment problem count. This
> > +    # may change in the future.
> > +    fm.condwrite('tls1.2' not in security, 'tlswarning', '  %s\n',
> > +                 _('TLS 1.2 not supported by Python install; '
> > +                   'network connections lack modern security'))
> > +    fm.condwrite('sni' not in security, 'sniwarning', '  %s\n',
> > +                 _('SNI not supported by Python install; may have '
> > +                   'connectivity issues with some servers'))

Maybe they should use fm.plain() because they seem a kind of explanation
about 'pythonsecurity'. Otherwise, "hg debuginstall -Tjson" would print
these even if tls1.2 and sni are supported.
Pierre-Yves David - Oct. 21, 2016, 3:06 p.m.
On 10/20/2016 05:49 PM, Yuya Nishihara wrote:
> On Thu, 20 Oct 2016 10:26:20 -0400, Augie Fackler wrote:
>> On Wed, Oct 19, 2016 at 03:16:15PM -0700, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1476914831 25200
>>> #      Wed Oct 19 15:07:11 2016 -0700
>>> # Branch stable
>>> # Node ID 37eaf6c2b4ac3c1015965676db89e435a79b45ee
>>> # Parent  e478f11e418288b8308457303d3ddf6a23f874f8
>>> commands: print security protocol support in debuginstall
>>
>> I'm going to give this an enthusiastic +1. It's probably a little
>> sketchy to take it during the freeze, but I agree we should just do
>> it. If I don't hear objections before the end of my workday on Friday,
>> I'll take this.
>
> +1

+1 for the feature too. The change we made in 3.9 on our security policy 
had a significant impact on users. It was the right thing to do, but 
small change helping users to adapt are worth taking.

> A few nits from a templater expert. ;-)

Does the expert wants a V2 ?

Cheers
Gregory Szorc - Oct. 21, 2016, 11:26 p.m.
On Thu, Oct 20, 2016 at 8:49 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 20 Oct 2016 10:26:20 -0400, Augie Fackler wrote:
> > On Wed, Oct 19, 2016 at 03:16:15PM -0700, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1476914831 25200
> > > #      Wed Oct 19 15:07:11 2016 -0700
> > > # Branch stable
> > > # Node ID 37eaf6c2b4ac3c1015965676db89e435a79b45ee
> > > # Parent  e478f11e418288b8308457303d3ddf6a23f874f8
> > > commands: print security protocol support in debuginstall
> >
> > I'm going to give this an enthusiastic +1. It's probably a little
> > sketchy to take it during the freeze, but I agree we should just do
> > it. If I don't hear objections before the end of my workday on Friday,
> > I'll take this.
>
> +1
>
> A few nits from a templater expert. ;-)
>
> > > +    security = set(sslutil.supportedprotocols)
> > > +    if sslutil.hassni:
> > > +        security.add('sni')
> > > +
> > > +    fm.write('pythonsecurity', _("checking Python security support
> (%s)\n"),
> > > +             ', '.join(sorted(security)))
>
> Please use fm.formatlist().
>
> > > +    # These are warnings, not errors. So don't increment problem
> count. This
> > > +    # may change in the future.
> > > +    fm.condwrite('tls1.2' not in security, 'tlswarning', '  %s\n',
> > > +                 _('TLS 1.2 not supported by Python install; '
> > > +                   'network connections lack modern security'))
> > > +    fm.condwrite('sni' not in security, 'sniwarning', '  %s\n',
> > > +                 _('SNI not supported by Python install; may have '
> > > +                   'connectivity issues with some servers'))
>
> Maybe they should use fm.plain() because they seem a kind of explanation
> about 'pythonsecurity'. Otherwise, "hg debuginstall -Tjson" would print
> these even if tls1.2 and sni are supported.
>

I incorporated the formatter changes. Thank you for the suggestions.

On the subject of the formatter, I've found the expected usage a bit
confusing. Specifically, I'm not sure when it is appropriate to use the
various APIs. If you or anyone else could find time to write docstrings in
formatter.py, it would really be appreciated.
Yuya Nishihara - Oct. 22, 2016, 7:54 a.m.
On Fri, 21 Oct 2016 16:26:08 -0700, Gregory Szorc wrote:
> On Thu, Oct 20, 2016 at 8:49 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On the subject of the formatter, I've found the expected usage a bit
> confusing. Specifically, I'm not sure when it is appropriate to use the
> various APIs. If you or anyone else could find time to write docstrings in
> formatter.py, it would really be appreciated.

Ok, I'll add a module doc and some doctests to show usage.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -63,16 +63,17 @@  from . import (
     pvec,
     repair,
     revlog,
     revset,
     scmutil,
     setdiscovery,
     simplemerge,
     sshserver,
+    sslutil,
     streamclone,
     templatekw,
     templater,
     treediscovery,
     ui as uimod,
     util,
 )
 
@@ -2698,16 +2699,34 @@  def debuginstall(ui, **opts):
     # Python
     fm.write('pythonexe', _("checking Python executable (%s)\n"),
              sys.executable)
     fm.write('pythonver', _("checking Python version (%s)\n"),
              ("%s.%s.%s" % sys.version_info[:3]))
     fm.write('pythonlib', _("checking Python lib (%s)...\n"),
              os.path.dirname(os.__file__))
 
+    security = set(sslutil.supportedprotocols)
+    if sslutil.hassni:
+        security.add('sni')
+
+    fm.write('pythonsecurity', _("checking Python security support (%s)\n"),
+             ', '.join(sorted(security)))
+
+    # These are warnings, not errors. So don't increment problem count. This
+    # may change in the future.
+    fm.condwrite('tls1.2' not in security, 'tlswarning', '  %s\n',
+                 _('TLS 1.2 not supported by Python install; '
+                   'network connections lack modern security'))
+    fm.condwrite('sni' not in security, 'sniwarning', '  %s\n',
+                 _('SNI not supported by Python install; may have '
+                   'connectivity issues with some servers'))
+
+    # TODO print CA cert info
+
     # hg version
     hgver = util.version()
     fm.write('hgver', _("checking Mercurial version (%s)\n"),
              hgver.split('+')[0])
     fm.write('hgverextra', _("checking Mercurial custom build (%s)\n"),
              '+'.join(hgver.split('+')[1:]))
 
     # compiled modules
diff --git a/tests/test-install.t b/tests/test-install.t
--- a/tests/test-install.t
+++ b/tests/test-install.t
@@ -1,14 +1,17 @@ 
 hg debuginstall
   $ hg debuginstall
   checking encoding (ascii)...
   checking Python executable (*) (glob)
   checking Python version (2.*) (glob)
   checking Python lib (*lib*)... (glob)
+  checking Python security support (*) (glob)
+    TLS 1.2 not supported by Python install; network connections lack modern security (?)
+    SNI not supported by Python install; may have connectivity issues with some servers (?)
   checking Mercurial version (*) (glob)
   checking Mercurial custom build (*) (glob)
   checking module policy (*) (glob)
   checking installed modules (*mercurial)... (glob)
   checking templates (*mercurial?templates)... (glob)
   checking default template (*mercurial?templates?map-cmdline.default) (glob)
   checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
   checking username (test)
@@ -28,30 +31,36 @@  hg debuginstall JSON
     "extensionserror": null,
     "hgmodulepolicy": "*", (glob)
     "hgmodules": "*mercurial", (glob)
     "hgver": "*", (glob)
     "hgverextra": "*", (glob)
     "problems": 0,
     "pythonexe": "*", (glob)
     "pythonlib": "*", (glob)
+    "pythonsecurity": "*", (glob)
     "pythonver": "*.*.*", (glob)
+    "sniwarning": "SNI not supported by Python install; may have connectivity issues with some servers",
     "templatedirs": "*mercurial?templates", (glob)
+    "tlswarning": "TLS 1.2 not supported by Python install; network connections lack modern security",
     "username": "test",
     "usernameerror": null,
     "vinotfound": false
    }
   ]
 
 hg debuginstall with no username
   $ HGUSER= hg debuginstall
   checking encoding (ascii)...
   checking Python executable (*) (glob)
   checking Python version (2.*) (glob)
   checking Python lib (*lib*)... (glob)
+  checking Python security support (*) (glob)
+    TLS 1.2 not supported by Python install; network connections lack modern security (?)
+    SNI not supported by Python install; may have connectivity issues with some servers (?)
   checking Mercurial version (*) (glob)
   checking Mercurial custom build (*) (glob)
   checking module policy (*) (glob)
   checking installed modules (*mercurial)... (glob)
   checking templates (*mercurial?templates)... (glob)
   checking default template (*mercurial?templates?map-cmdline.default) (glob)
   checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
   checking username...
@@ -66,16 +75,19 @@  path variables are expanded (~ is the sa
 #if execbit
   $ chmod 755 tools/testeditor.exe
 #endif
   $ hg debuginstall --config ui.editor=~/tools/testeditor.exe
   checking encoding (ascii)...
   checking Python executable (*) (glob)
   checking Python version (*) (glob)
   checking Python lib (*lib*)... (glob)
+  checking Python security support (*) (glob)
+    TLS 1.2 not supported by Python install; network connections lack modern security (?)
+    SNI not supported by Python install; may have connectivity issues with some servers (?)
   checking Mercurial version (*) (glob)
   checking Mercurial custom build (*) (glob)
   checking module policy (*) (glob)
   checking installed modules (*mercurial)... (glob)
   checking templates (*mercurial?templates)... (glob)
   checking default template (*mercurial?templates?map-cmdline.default) (glob)
   checking commit editor... (* -c "import sys; sys.exit(0)") (glob)
   checking username (test)