Patchwork [9,of,9,V2] sslutil: print the fingerprint from the last hash used

login
register
mail settings
Submitter Gregory Szorc
Date June 1, 2016, 2:22 a.m.
Message ID <2bdb54fc690f460e7c20.1464747725@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15315/
State Superseded
Headers show

Comments

Gregory Szorc - June 1, 2016, 2:22 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1464648649 25200
#      Mon May 30 15:50:49 2016 -0700
# Node ID 2bdb54fc690f460e7c20e393c495a4e32458dfa6
# Parent  3b08559256d040147d29caa73e5b982a0016f78a
sslutil: print the fingerprint from the last hash used

Before, we would always print the unprefixed SHA-1 fingerprint when
fingerprint comparison failed. Now, we print the fingerprint of the
last hash used, including the prefix if necessary. This helps ensure
that the printed hash type matches what is in the user configuration.

There are still some cases where this can print a mismatched hash type.
e.g. if there are both SHA-1 and SHA-256 fingerprints in the config,
we could print a SHA-1 hash if it comes after the SHA-256 hash. But
I'm inclined to ignore this edge case.
Yuya Nishihara - June 1, 2016, 1:26 p.m.
On Tue, 31 May 2016 19:22:05 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1464648649 25200
> #      Mon May 30 15:50:49 2016 -0700
> # Node ID 2bdb54fc690f460e7c20e393c495a4e32458dfa6
> # Parent  3b08559256d040147d29caa73e5b982a0016f78a
> sslutil: print the fingerprint from the last hash used

> +        nice = fmtfingerprint(peerfingerprints[hash])
> +        if not settings['legacyfingerprint']:
> +            nice = '%s:%s' % (hash, nice)

If the last hash is not 'sha1' and if 'legacyfingerprint' is set, it would
print non-sha1 hash with "check hostfingerprint configuration" message. But
I confirmed it would never happen, so this patch seems fine.
Pierre-Yves David - June 2, 2016, 12:47 a.m.
On 06/01/2016 03:26 PM, Yuya Nishihara wrote:
> On Tue, 31 May 2016 19:22:05 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1464648649 25200
>> #      Mon May 30 15:50:49 2016 -0700
>> # Node ID 2bdb54fc690f460e7c20e393c495a4e32458dfa6
>> # Parent  3b08559256d040147d29caa73e5b982a0016f78a
>> sslutil: print the fingerprint from the last hash used
> 
>> +        nice = fmtfingerprint(peerfingerprints[hash])
>> +        if not settings['legacyfingerprint']:
>> +            nice = '%s:%s' % (hash, nice)
> 
> If the last hash is not 'sha1' and if 'legacyfingerprint' is set, it would
> print non-sha1 hash with "check hostfingerprint configuration" message. But
> I confirmed it would never happen, so this patch seems fine.

Thanks for going through the trouble of confirming it. I would advocate
for adding a comment about it on site to save the next poor soul the effort.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -375,33 +375,35 @@  def validatesocket(sock):
         'sha1': util.sha1(peercert).hexdigest(),
         'sha256': util.sha256(peercert).hexdigest(),
         'sha512': util.sha512(peercert).hexdigest(),
     }
 
     def fmtfingerprint(s):
         return ':'.join([s[x:x + 2] for x in range(0, len(s), 2)])
 
-    legacyfingerprint = fmtfingerprint(peerfingerprints['sha1'])
     nicefingerprint = 'sha256:%s' % fmtfingerprint(peerfingerprints['sha256'])
 
     if settings['legacyfingerprint']:
         section = 'hostfingerprint'
     else:
         section = 'hostsecurity'
 
     if settings['certfingerprints']:
         for hash, fingerprint in settings['certfingerprints']:
             if peerfingerprints[hash].lower() == fingerprint:
                 ui.debug('%s certificate matched fingerprint %s:%s\n' %
                          (host, hash, fmtfingerprint(fingerprint)))
                 return
 
+        nice = fmtfingerprint(peerfingerprints[hash])
+        if not settings['legacyfingerprint']:
+            nice = '%s:%s' % (hash, nice)
         raise error.Abort(_('certificate for %s has unexpected '
-                           'fingerprint %s') % (host, legacyfingerprint),
+                           'fingerprint %s') % (host, nice),
                           hint=_('check %s configuration') % section)
 
     if not sock._hgstate['caloaded']:
         ui.warn(_('warning: certificate for %s not verified '
                   '(set hostsecurity.%s:certfingerprints=%s or web.cacerts '
                   'config settings)\n') % (host, host, nicefingerprint))
         return
 
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -309,17 +309,17 @@  Fingerprints
 - multiple fingerprints specified and none match
 
   $ hg --config 'hostfingerprints.localhost=deadbeefdeadbeefdeadbeefdeadbeefdeadbeef, aeadbeefdeadbeefdeadbeefdeadbeefdeadbeef' -R copy-pull id https://localhost:$HGPORT/ --insecure
   abort: certificate for localhost has unexpected fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca
   (check hostfingerprint configuration)
   [255]
 
   $ hg --config 'hostsecurity.localhost:fingerprints=sha1:deadbeefdeadbeefdeadbeefdeadbeefdeadbeef, sha1:aeadbeefdeadbeefdeadbeefdeadbeefdeadbeef' -R copy-pull id https://localhost:$HGPORT/
-  abort: certificate for localhost has unexpected fingerprint 91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca
+  abort: certificate for localhost has unexpected fingerprint sha1:91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca
   (check hostsecurity configuration)
   [255]
 
 - fails when cert doesn't match hostname (port is ignored)
   $ hg -R copy-pull id https://localhost:$HGPORT1/ --config hostfingerprints.localhost=914f1aff87249c09b6859b88b1906d30756491ca
   abort: certificate for localhost has unexpected fingerprint 28:ff:71:bf:65:31:14:23:ad:62:92:b4:0e:31:99:18:fc:83:e3:9b
   (check hostfingerprint configuration)
   [255]