Patchwork sslutil: check for missing certificate and key files (issue5598)

login
register
mail settings
Submitter Gregory Szorc
Date July 11, 2017, 4:17 a.m.
Message ID <b9e2be8b5a75cbd1adef.1499746658@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/22212/
State Accepted
Headers show

Comments

Gregory Szorc - July 11, 2017, 4:17 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1499746186 25200
#      Mon Jul 10 21:09:46 2017 -0700
# Node ID b9e2be8b5a75cbd1adef810111a469a10c9846c5
# Parent  4672db164c986da4442bd864cd044512d975c3f2
sslutil: check for missing certificate and key files (issue5598)

Currently, sslutil._hostsettings() performs validation that web.cacerts
exists. However, client certificates are passed in to the function
and not all callers may validate them. This includes
httpconnection.readauthforuri(), which loads the [auth] section.

If a missing file is specified, the ssl module will raise a generic
IOException. And, it doesn't even give us the courtesy of telling
us which file is missing! Mercurial then prints a generic
"abort: No such file or directory" (or similar) error, leaving users
to scratch their head as to what file is missing.

This commit introduces explicit validation of all paths passed as
arguments to wrapsocket() and wrapserversocket(). Any missing file
is alerted about explicitly.

We should probably catch missing files earlier - as part of loading
the [auth] section. However, I think the sslutil functions should
check for file presence regardless of what callers do because that's
the only way to be sure that missing files are always detected.
Yuya Nishihara - July 11, 2017, 1:27 p.m.
On Mon, 10 Jul 2017 21:17:38 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1499746186 25200
> #      Mon Jul 10 21:09:46 2017 -0700
> # Node ID b9e2be8b5a75cbd1adef810111a469a10c9846c5
> # Parent  4672db164c986da4442bd864cd044512d975c3f2
> sslutil: check for missing certificate and key files (issue5598)

Queued. Thanks for fixing this.
Augie Fackler - July 11, 2017, 2:12 p.m.
On Mon, Jul 10, 2017 at 09:17:38PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1499746186 25200
> #      Mon Jul 10 21:09:46 2017 -0700
> # Node ID b9e2be8b5a75cbd1adef810111a469a10c9846c5
> # Parent  4672db164c986da4442bd864cd044512d975c3f2
> sslutil: check for missing certificate and key files (issue5598)

queued, thanks

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -343,6 +343,13 @@  def wrapsocket(sock, keyfile, certfile, 
     if not serverhostname:
         raise error.Abort(_('serverhostname argument is required'))
 
+    for f in (keyfile, certfile):
+        if f and not os.path.exists(f):
+            raise error.Abort(_('certificate file (%s) does not exist; '
+                                'cannot connect to %s') % (f, serverhostname),
+                              hint=_('restore missing file or fix references '
+                                     'in Mercurial config'))
+
     settings = _hostsettings(ui, serverhostname)
 
     # We can't use ssl.create_default_context() because it calls
@@ -499,6 +506,13 @@  def wrapserversocket(sock, ui, certfile=
 
     Typically ``cafile`` is only defined if ``requireclientcert`` is true.
     """
+    # This function is not used much by core Mercurial, so the error messaging
+    # doesn't have to be as detailed as for wrapsocket().
+    for f in (certfile, keyfile, cafile):
+        if f and not os.path.exists(f):
+            raise error.Abort(_('referenced certificate file (%s) does not '
+                                'exist') % f)
+
     protocol, options, _protocolui = protocolsettings('tls1.0')
 
     # This config option is intended for use in tests only. It is a giant
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -592,9 +592,22 @@  Test https with cert problems through pr
 
 #if sslcontext
 
+  $ cd test
+
+Missing certificate file(s) are detected
+
+  $ hg serve -p $HGPORT --certificate=/missing/certificate \
+  > --config devel.servercafile=$PRIV --config devel.serverrequirecert=true
+  abort: referenced certificate file (/missing/certificate) does not exist
+  [255]
+
+  $ hg serve -p $HGPORT --certificate=$PRIV \
+  > --config devel.servercafile=/missing/cafile --config devel.serverrequirecert=true
+  abort: referenced certificate file (/missing/cafile) does not exist
+  [255]
+
 Start hgweb that requires client certificates:
 
-  $ cd test
   $ hg serve -p $HGPORT -d --pid-file=../hg0.pid --certificate=$PRIV \
   > --config devel.servercafile=$PRIV --config devel.serverrequirecert=true
   $ cat ../hg0.pid >> $DAEMON_PIDS
@@ -631,4 +644,16 @@  with client certificate:
   abort: error: * (glob)
   [255]
 
+Missing certficate and key files result in error
+
+  $ hg id https://localhost:$HGPORT/ --config auth.l.cert=/missing/cert
+  abort: certificate file (/missing/cert) does not exist; cannot connect to localhost
+  (restore missing file or fix references in Mercurial config)
+  [255]
+
+  $ hg id https://localhost:$HGPORT/ --config auth.l.key=/missing/key
+  abort: certificate file (/missing/key) does not exist; cannot connect to localhost
+  (restore missing file or fix references in Mercurial config)
+  [255]
+
 #endif