Patchwork [RFC] sslutil: use base64 for key fingerprints (BC)

login
register
mail settings
Submitter Gregory Szorc
Date July 6, 2016, 6:36 a.m.
Message ID <6873eb9a931da6cfcb40.1467787005@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15757/
State Rejected
Headers show

Comments

Gregory Szorc - July 6, 2016, 6:36 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1467786847 25200
#      Tue Jul 05 23:34:07 2016 -0700
# Node ID 6873eb9a931da6cfcb40b2aba2f75260db5a8200
# Parent  73b0a50ff97787557e517779a33644149d25d0a8
sslutil: use base64 for key fingerprints (BC)

When I initially implemented support for SHA-256 and SHA-512
certificate pinning, I copied the strategy used for SHA-1 fingerprints,
which was to use hex fingerprints. SHA-256 and SHA-512 hashes are much
longer. Using base 16 to represent the hash can be cumbersome.

This patch switches the fingerprints for newer fingerprints (read:
everything in [hostsecurity]) to base64. The resulting strings are
smaller and easier on the eyes in error messages.

The printed base64 hashes have their trailing '=' stripped. This
matches the behavior of OpenSSH (and possibly other crypto tools).

When comparing hashes, trailing '=' are stripped. This code should
be scrutinized, as it may result in comparing truncated hashes and
weakening security. I /think/ things are safe since the '=' are there
for padding, but I've learned with crypto code to be paranoid about
such things.

One downside of this patch is that `openssl` doesn't appear to be
able to print the base64 fingerprint of certs easily. The output
from `openssl x509 -in <file> -noout -fingerprint -sha256` displays
the colon-delimited hex fingerprint, just like Mercurial did before
this patch. So, we lose the ability for users to easily copy
fingerprints from OpenSSL to Mercurial configs. We could come up with
a one-liner for printing the base64 fingerprint of a cert. Although
using something built-in to OpenSSL is nice and dropping that feels a
bit wrong. Then again, OpenSSL's CLI/API isn't exactly a pillar of
great design, so perhaps it isn't a huge loss.

The same criticism applies to browsers. Firefox prints the hex
fingerprints, as does Chrome. I couldn't figure out how to get
Edge to print security details. Again, I'm not a huge fan of losing
well-established "trust anchors" from which users can copy
fingerprints into Mercurial configs.

I'm marking this patch as BC because it technically is. However, the
[hostsecurity] section hasn't shipped in a release yet, so no
real world users should be using it and impacted by this change
(which drops support for defining hex fingerprints in [hostsecurity]).
Matt Mackall - July 6, 2016, 4:20 p.m.
On Tue, 2016-07-05 at 23:36 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1467786847 25200
> #      Tue Jul 05 23:34:07 2016 -0700
> # Node ID 6873eb9a931da6cfcb40b2aba2f75260db5a8200
> # Parent  73b0a50ff97787557e517779a33644149d25d0a8
> sslutil: use base64 for key fingerprints (BC)
> 
> When I initially implemented support for SHA-256 and SHA-512
> certificate pinning, I copied the strategy used for SHA-1 fingerprints,
> which was to use hex fingerprints. SHA-256 and SHA-512 hashes are much
> longer. Using base 16 to represent the hash can be cumbersome.
> 
> This patch switches the fingerprints for newer fingerprints (read:
> everything in [hostsecurity]) to base64. The resulting strings are
> smaller and easier on the eyes in error messages.

I've found it very useful to be able to check the fingerprint with my laptop's
browser. My browser's trust database is generally newer and more reliable than
the equivalent database (if any!) available to Mercurial on a random old server.
Breaking that seems like a bigger loss than is gained by compactness.

For the sake of discussion, the current fingerprint size is:

 n/4 + n/8 - 1

 160: 59
 256: 95
 512: 191

With base-64, it's going to be:

 int(n/24.0 + .5) * 4

 160: 28
 256: 44
 512: 84

..so we're not entirely saved from the awkward line length issue.

-- 
Mathematics is the supreme nostalgia of our time.
Gregory Szorc - July 6, 2016, 6:14 p.m.
On Wed, Jul 6, 2016 at 10:38 AM, Kevin Bullock <
kbullock+mercurial@ringworld.org> wrote:

> > On Jul 6, 2016, at 11:20, Matt Mackall <mpm@selenic.com> wrote:
> >
> > On Tue, 2016-07-05 at 23:36 -0700, Gregory Szorc wrote:
> >> # HG changeset patch
> >> # User Gregory Szorc <gregory.szorc@gmail.com>
> >> # Date 1467786847 25200
> >> #      Tue Jul 05 23:34:07 2016 -0700
> >> # Node ID 6873eb9a931da6cfcb40b2aba2f75260db5a8200
> >> # Parent  73b0a50ff97787557e517779a33644149d25d0a8
> >> sslutil: use base64 for key fingerprints (BC)
> >>
> >> When I initially implemented support for SHA-256 and SHA-512
> >> certificate pinning, I copied the strategy used for SHA-1 fingerprints,
> >> which was to use hex fingerprints. SHA-256 and SHA-512 hashes are much
> >> longer. Using base 16 to represent the hash can be cumbersome.
> >>
> >> This patch switches the fingerprints for newer fingerprints (read:
> >> everything in [hostsecurity]) to base64. The resulting strings are
> >> smaller and easier on the eyes in error messages.
> >
> > I've found it very useful to be able to check the fingerprint with my
> laptop's
> > browser. My browser's trust database is generally newer and more
> reliable than
> > the equivalent database (if any!) available to Mercurial on a random old
> server.
> > Breaking that seems like a bigger loss than is gained by compactness.
> >
> > For the sake of discussion, the current fingerprint size is:
> >
> >  n/4 + n/8 - 1
> >
> >  160: 59
> >  256: 95
> >  512: 191
>
> Can we make the colons optional in hex fingerprints (if they aren't
> already)? That takes it from the above to just n/4:
>
> 160: 40
> 256: 64
> 512: 128
>
> ...which isn't as good as base64, but retains the inspectability of the
> current arrangement.
>

The colons are already optional.

While I'm here, since I was on the fence about this and since mpm isn't a
fan, let's drop it.

In the future, we could add support for using base64 fingerprints in the
config. But I don't see a value in doing that today.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -4,16 +4,18 @@ 
 # Copyright 2006, 2007 Alexis S. L. Carvalho <alexis@cecm.usp.br>
 # Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com>
 #
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
 from __future__ import absolute_import
 
+import base64
+import binascii
 import hashlib
 import os
 import re
 import ssl
 import sys
 
 from .i18n import _
 from . import (
@@ -137,24 +139,28 @@  def _hostsettings(ui, hostname):
     for fingerprint in fingerprints:
         if not (fingerprint.startswith(('sha1:', 'sha256:', 'sha512:'))):
             raise error.Abort(_('invalid fingerprint for %s: %s') % (
                                 hostname, fingerprint),
                               hint=_('must begin with "sha1:", "sha256:", '
                                      'or "sha512:"'))
 
         alg, fingerprint = fingerprint.split(':', 1)
-        fingerprint = fingerprint.replace(':', '').lower()
         s['certfingerprints'].append((alg, fingerprint))
 
     # Fingerprints from [hostfingerprints] are always SHA-1.
     for fingerprint in ui.configlist('hostfingerprints', hostname, []):
         fingerprint = fingerprint.replace(':', '').lower()
-        s['certfingerprints'].append(('sha1', fingerprint))
-        s['legacyfingerprint'] = True
+        # Internally fingerprints are stored as base64.
+        try:
+            raw = binascii.unhexlify(fingerprint)
+            s['certfingerprints'].append(('sha1', base64.b64encode(raw)))
+            s['legacyfingerprint'] = True
+        except TypeError:
+            pass
 
     # If a host cert fingerprint is defined, it is the only thing that
     # matters. No need to validate CA certs.
     if s['certfingerprints']:
         s['verifymode'] = ssl.CERT_NONE
         s['allowloaddefaultcerts'] = False
 
     # If --insecure is used, don't take CAs into consideration.
@@ -499,40 +505,41 @@  def validatesocket(sock):
         ui.warn(_('warning: connection security to %s is disabled per current '
                   'settings; communication is susceptible to eavesdropping '
                   'and tampering\n') % host)
         return
 
     # If a certificate fingerprint is pinned, use it and only it to
     # validate the remote cert.
     peerfingerprints = {
-        'sha1': hashlib.sha1(peercert).hexdigest(),
-        'sha256': hashlib.sha256(peercert).hexdigest(),
-        'sha512': hashlib.sha512(peercert).hexdigest(),
+        'sha1': base64.b64encode(hashlib.sha1(peercert).digest()),
+        'sha256': base64.b64encode(hashlib.sha256(peercert).digest()),
+        'sha512': base64.b64encode(hashlib.sha512(peercert).digest()),
     }
 
-    def fmtfingerprint(s):
-        return ':'.join([s[x:x + 2] for x in range(0, len(s), 2)])
-
-    nicefingerprint = 'sha256:%s' % fmtfingerprint(peerfingerprints['sha256'])
+    nicefingerprint = 'sha256:%s' % peerfingerprints['sha256'].rstrip('=')
 
     if settings['certfingerprints']:
         for hash, fingerprint in settings['certfingerprints']:
-            if peerfingerprints[hash].lower() == fingerprint:
+            # Compare without '=' padding characters.
+            if peerfingerprints[hash].rstrip('=') == fingerprint.rstrip('='):
                 ui.debug('%s certificate matched fingerprint %s:%s\n' %
-                         (host, hash, fmtfingerprint(fingerprint)))
+                         (host, hash, fingerprint))
                 return
 
         # Pinned fingerprint didn't match. This is a fatal error.
         if settings['legacyfingerprint']:
             section = 'hostfingerprint'
-            nice = fmtfingerprint(peerfingerprints['sha1'])
+            raw = base64.b64decode(peerfingerprints['sha1'])
+            hexprint = binascii.hexlify(raw)
+            nice = ':'.join([hexprint[x:x + 2]
+                             for x in range(0, len(hexprint), 2)])
         else:
             section = 'hostsecurity'
-            nice = '%s:%s' % (hash, fmtfingerprint(peerfingerprints[hash]))
+            nice = '%s:%s' % (hash, peerfingerprints[hash].rstrip('='))
         raise error.Abort(_('certificate for %s has unexpected '
                             'fingerprint %s') % (host, nice),
                           hint=_('check %s configuration') % section)
 
     # Security is enabled but no CAs are loaded. We can't establish trust
     # for the cert so abort.
     if not sock._hgstate['caloaded']:
         raise error.Abort(
diff --git a/tests/test-https.t b/tests/test-https.t
--- a/tests/test-https.t
+++ b/tests/test-https.t
@@ -71,17 +71,17 @@  we are able to load CA certs.
   $ hg clone https://localhost:$HGPORT/ copy-pull
   abort: error: *certificate verify failed* (glob)
   [255]
 #endif
 
 #if no-defaultcacerts
   $ hg clone https://localhost:$HGPORT/ copy-pull
   abort: localhost certificate error: no certificate received
-  (set hostsecurity.localhost:certfingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 config setting or use --insecure to connect insecurely)
+  (set hostsecurity.localhost:certfingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA config setting or use --insecure to connect insecurely)
   [255]
 #endif
 
 Specifying a per-host certificate file that doesn't exist will abort
 
   $ hg --config hostsecurity.localhost:verifycertsfile=/does/not/exist clone https://localhost:$HGPORT/
   abort: path specified by hostsecurity.localhost:verifycertsfile does not exist: /does/not/exist
   [255]
@@ -130,31 +130,31 @@  A per-host certificate with multiple cer
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 4 changes to 4 files
 
 Defining both per-host certificate and a fingerprint will print a warning
 
-  $ hg --config hostsecurity.localhost:verifycertsfile="$CERTSDIR/pub.pem" --config hostsecurity.localhost:fingerprints=sha1:914f1aff87249c09b6859b88b1906d30756491ca clone -U https://localhost:$HGPORT/ caandfingerwarning
+  $ hg --config hostsecurity.localhost:verifycertsfile="$CERTSDIR/pub.pem" --config hostsecurity.localhost:fingerprints=sha1:kU8a/4cknAm2hZuIsZBtMHVkkco clone -U https://localhost:$HGPORT/ caandfingerwarning
   (hostsecurity.localhost:verifycertsfile ignored when host fingerprints defined; using host fingerprints for verification)
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 4 changes to 4 files
 
   $ DISABLECACERTS="--config devel.disableloaddefaultcerts=true"
 
 Inability to verify peer certificate will result in abort
 
   $ hg clone https://localhost:$HGPORT/ copy-pull $DISABLECACERTS
   abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
-  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA to trust this server)
   [255]
 
   $ hg clone --insecure https://localhost:$HGPORT/ copy-pull
   warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   requesting all changes
   adding changesets
   adding manifests
   adding file changes
@@ -176,17 +176,17 @@  Inability to verify peer certificate wil
 pull without cacert
 
   $ cd copy-pull
   $ echo '[hooks]' >> .hg/hgrc
   $ echo "changegroup = printenv.py changegroup" >> .hg/hgrc
   $ hg pull $DISABLECACERTS
   pulling from https://localhost:$HGPORT/
   abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
-  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA to trust this server)
   [255]
 
   $ hg pull --insecure
   pulling from https://localhost:$HGPORT/
   warning: connection security to localhost is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   adding changesets
   adding manifests
@@ -240,17 +240,17 @@  empty cacert file
 #endif
 
 cacert mismatch
 
   $ hg -R copy-pull pull --config web.cacerts="$CERTSDIR/pub.pem" \
   > https://127.0.0.1:$HGPORT/
   pulling from https://127.0.0.1:$HGPORT/
   abort: 127.0.0.1 certificate error: certificate is for localhost
-  (set hostsecurity.127.0.0.1:certfingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 config setting or use --insecure to connect insecurely)
+  (set hostsecurity.127.0.0.1:certfingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA config setting or use --insecure to connect insecurely)
   [255]
   $ hg -R copy-pull pull --config web.cacerts="$CERTSDIR/pub.pem" \
   > https://127.0.0.1:$HGPORT/ --insecure
   pulling from https://127.0.0.1:$HGPORT/
   warning: connection security to 127.0.0.1 is disabled per current settings; communication is susceptible to eavesdropping and tampering
   searching for changes
   no changes found
   $ hg -R copy-pull pull --config web.cacerts="$CERTSDIR/pub-other.pem"
@@ -286,48 +286,53 @@  Test server cert which no longer is vali
 
 Fingerprints
 
 - works without cacerts (hostkeyfingerprints)
   $ hg -R copy-pull id https://localhost:$HGPORT/ --insecure --config hostfingerprints.localhost=91:4f:1a:ff:87:24:9c:09:b6:85:9b:88:b1:90:6d:30:75:64:91:ca
   5fed3813f7f5
 
 - works without cacerts (hostsecurity)
-  $ hg -R copy-pull id https://localhost:$HGPORT/ --config hostsecurity.localhost:fingerprints=sha1:914f1aff87249c09b6859b88b1906d30756491ca
+  $ hg -R copy-pull id https://localhost:$HGPORT/ --config hostsecurity.localhost:fingerprints=sha1:kU8a/4cknAm2hZuIsZBtMHVkkco
   5fed3813f7f5
 
-  $ hg -R copy-pull id https://localhost:$HGPORT/ --config hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30
+  $ hg -R copy-pull id https://localhost:$HGPORT/ --config hostsecurity.localhost:fingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA
   5fed3813f7f5
 
 - multiple fingerprints specified and first matches
   $ hg --config 'hostfingerprints.localhost=914f1aff87249c09b6859b88b1906d30756491ca, deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' -R copy-pull id https://localhost:$HGPORT/ --insecure
   5fed3813f7f5
 
-  $ hg --config 'hostsecurity.localhost:fingerprints=sha1:914f1aff87249c09b6859b88b1906d30756491ca, sha1:deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' -R copy-pull id https://localhost:$HGPORT/
+  $ hg --config 'hostsecurity.localhost:fingerprints=sha1:kU8a/4cknAm2hZuIsZBtMHVkkco, sha1:deadbeefdeadbeefdeadbeefdead' -R copy-pull id https://localhost:$HGPORT/
   5fed3813f7f5
 
 - multiple fingerprints specified and last matches
   $ hg --config 'hostfingerprints.localhost=deadbeefdeadbeefdeadbeefdeadbeefdeadbeef, 914f1aff87249c09b6859b88b1906d30756491ca' -R copy-pull id https://localhost:$HGPORT/ --insecure
   5fed3813f7f5
 
-  $ hg --config 'hostsecurity.localhost:fingerprints=sha1:deadbeefdeadbeefdeadbeefdeadbeefdeadbeef, sha1:914f1aff87249c09b6859b88b1906d30756491ca' -R copy-pull id https://localhost:$HGPORT/
+  $ hg --config 'hostsecurity.localhost:fingerprints=sha1:deadbeefdeadbeefdeadbeefdead, sha1:kU8a/4cknAm2hZuIsZBtMHVkkco' -R copy-pull id https://localhost:$HGPORT/
   5fed3813f7f5
 
 - 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 sha1: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:kU8a/4cknAm2hZuIsZBtMHVkkco
   (check hostsecurity configuration)
   [255]
 
+- trailing = is optional in fingerprint
+
+  $ hg --config 'hostsecurity.localhost:fingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA=' -R copy-pull id https://localhost:$HGPORT/
+  5fed3813f7f5
+
 - 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]
 
 
 - ignores that certificate doesn't match hostname
diff --git a/tests/test-patchbomb-tls.t b/tests/test-patchbomb-tls.t
--- a/tests/test-patchbomb-tls.t
+++ b/tests/test-patchbomb-tls.t
@@ -89,17 +89,17 @@  Without certificates:
   $ try --debug
   this patch series consists of 1 patches.
   
   
   (using smtps)
   sending mail: smtp host localhost, port * (glob)
   (verifying remote certificate)
   abort: unable to verify security of localhost (no loaded CA certificates); refusing to connect
-  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:62:09:97:2f:97:60:e3:65:8f:12:5d:78:9e:35:a1:36:7a:65:4b:0e:9f:ac:db:c3:bc:6e:b6:a3:c0:16:e0:30 to trust this server)
+  (see https://mercurial-scm.org/wiki/SecureConnections for how to configure Mercurial to avoid this error or set hostsecurity.localhost:fingerprints=sha256:YgmXL5dg42WPEl14njWhNnplSw6frNvDvG62o8AW4DA to trust this server)
   [255]
 
 With global certificates:
 
   $ try --debug --config web.cacerts="$CERTSDIR/pub.pem"
   this patch series consists of 1 patches.