Merge lp:~oddbloke/charm-helpers/unison_patches into lp:charm-helpers

[Dan Watkins]

This is an update of https://code.launchpad.net/~rcj/charm-helpers/unison_patches/+merge/236028 with the changes asked for there.

[lp:~oddbloke/charm-helpers/unison_patches]

411. By Robert C Jennings on 2015-07-23

unison: Add handling for peer departed hook

This adds handing for the <peer>-relation-departed hook in
ssh_authorized_peers() and updates the documentation.

Adding ssh_authorized_peers(...) calls to the peer departed
relationship hook will remove the departing unit from the
authorized keys and list of peers.

412. By Robert C Jennings on 2015-07-23

unison: Add all known keys for each host

Adding only the rsa key is problematic. The remote host could be
using ecdsa and the first attmempt to connect will add that ecdsa key
and complain that the rsa key conflicts. Taking all key types adds
flexibility and future-proofing.

Without this, connecting to a host that presents an ecdsa host key will
warn that the rsa key conflicts and will require interactive input to
accept the ecdsa key; this breaks the charm.

413. By Robert C Jennings on 2015-07-23

charmhelpers.contrib.unison: Add support for ecdsa keys

Add support for choosing an ssh key type of ecdsa in addition
to the default rsa type already provided.

[Stuart Bishop (stub)]

All looks good and seems correct. Tests pass, except for an unrelated failure on trunk (grrr). I can't suggest any improvements or changes.

review: Approve

=== modified file 'charmhelpers/contrib/unison/__init__.py'
--- charmhelpers/contrib/unison/__init__.py	2015-04-03 15:23:46 +0000
+++ charmhelpers/contrib/unison/__init__.py	2015-07-23 10:49:49 +0000
@@ -16,7 +16,7 @@
 
 # Easy file synchronization among peer units using ssh + unison.
 #
-# From *both* peer relation -joined and -changed, add a call to
+# For the -joined, -changed, and -departed peer relations, add a call to
 # ssh_authorized_peers() describing the peer relation and the desired
 # user + group.  After all peer relations have settled, all hosts should
 # be able to connect to on another via key auth'd ssh as the specified user.
@@ -30,14 +30,21 @@
 # ...
 # ssh_authorized_peers(peer_interface='cluster',
 #                      user='juju_ssh', group='juju_ssh',
-#                      ensure_user=True)
+#                      ensure_local_user=True)
 # ...
 #
 # cluster-relation-changed:
 # ...
 # ssh_authorized_peers(peer_interface='cluster',
 #                      user='juju_ssh', group='juju_ssh',
-#                      ensure_user=True)
+#                      ensure_local_user=True)
+# ...
+#
+# cluster-relation-departed:
+# ...
+# ssh_authorized_peers(peer_interface='cluster',
+#                      user='juju_ssh', group='juju_ssh',
+#                      ensure_local_user=True)
 # ...
 #
 # Hooks are now free to sync files as easily as:
@@ -92,11 +99,18 @@
         raise Exception
 
 
-def create_private_key(user, priv_key_path):
+def create_private_key(user, priv_key_path, key_type='rsa'):
+    types_bits = {
+        'rsa': '2048',
+        'ecdsa': '521',
+    }
+    if key_type not in types_bits:
+        log('Unknown ssh key type {}, using rsa'.format(key_type), ERROR)
+        key_type = 'rsa'
     if not os.path.isfile(priv_key_path):
         log('Generating new SSH key for user %s.' % user)
-        cmd = ['ssh-keygen', '-q', '-N', '', '-t', 'rsa', '-b', '2048',
-               '-f', priv_key_path]
+        cmd = ['ssh-keygen', '-q', '-N', '', '-t', key_type,
+               '-b', types_bits[key_type], '-f', priv_key_path]
         check_call(cmd)
     else:
         log('SSH key already exists at %s.' % priv_key_path)
@@ -152,7 +166,7 @@
     known_hosts = os.path.join(ssh_dir, 'known_hosts')
     khosts = []
     for host in hosts:
-        cmd = ['ssh-keyscan', '-H', '-t', 'rsa', host]
+        cmd = ['ssh-keyscan', host]
         remote_key = check_output(cmd, universal_newlines=True).strip()
         khosts.append(remote_key)
     log('Syncing known_hosts @ %s.' % known_hosts)
@@ -179,7 +193,8 @@
     hook = hook_name()
     if hook == '%s-relation-joined' % peer_interface:
         relation_set(ssh_pub_key=pub_key)
-    elif hook == '%s-relation-changed' % peer_interface:
+    elif hook == '%s-relation-changed' % peer_interface or \
+            hook == '%s-relation-departed' % peer_interface:
         hosts = []
         keys = []
 

=== modified file 'tests/contrib/unison/test_unison.py'
--- tests/contrib/unison/test_unison.py	2015-04-03 15:23:46 +0000
+++ tests/contrib/unison/test_unison.py	2015-07-23 10:49:49 +0000
@@ -74,7 +74,7 @@
             self.assertIn(call(_call), self.check_call.call_args_list)
 
     @patch('os.path.isfile')
-    def test_create_private_key(self, isfile):
+    def test_create_private_key_rsa(self, isfile):
         create_cmd = [
             'ssh-keygen', '-q', '-N', '', '-t', 'rsa', '-b', '2048',
             '-f', '/home/foo/.ssh/id_rsa']
@@ -100,6 +100,36 @@
         _ensure_perms()
 
     @patch('os.path.isfile')
+    def test_create_private_key_ecdsa(self, isfile):
+        create_cmd = [
+            'ssh-keygen', '-q', '-N', '', '-t', 'ecdsa', '-b', '521',
+            '-f', '/home/foo/.ssh/id_ecdsa']
+
+        def _ensure_perms():
+            cmds = [
+                ['chown', 'foo', '/home/foo/.ssh/id_ecdsa'],
+                ['chmod', '0600', '/home/foo/.ssh/id_ecdsa'],
+            ]
+            self._ensure_calls_in(cmds)
+
+        isfile.return_value = False
+        unison.create_private_key(
+            user='foo',
+            priv_key_path='/home/foo/.ssh/id_ecdsa',
+            key_type='ecdsa')
+        self.assertIn(call(create_cmd), self.check_call.call_args_list)
+        _ensure_perms()
+        self.check_call.call_args_list = []
+
+        isfile.return_value = True
+        unison.create_private_key(
+            user='foo',
+            priv_key_path='/home/foo/.ssh/id_ecdsa',
+            key_type='ecdsa')
+        self.assertNotIn(call(create_cmd), self.check_call.call_args_list)
+        _ensure_perms()
+
+    @patch('os.path.isfile')
     def test_create_public_key(self, isfile):
         create_cmd = ['ssh-keygen', '-y', '-f', '/home/foo/.ssh/id_rsa']
         isfile.return_value = True
@@ -273,6 +303,33 @@
         write_hosts.assert_called_with('foo', ['host1', 'host2'])
         self.relation_set.assert_called_with(ssh_authorized_hosts='host1:host2')
 
+    @patch.object(unison, 'write_known_hosts')
+    @patch.object(unison, 'write_authorized_keys')
+    @patch.object(unison, 'get_keypair')
+    @patch.object(unison, 'ensure_user')
+    def test_ssh_auth_peer_departed(self, ensure_user, get_keypair,
+                                    write_keys, write_hosts):
+        get_keypair.return_value = ('privkey', 'pubkey')
+
+        self.hook_name.return_value = 'cluster-relation-departed'
+
+        self.relation_get.side_effect = [
+            'key1',
+            'host1',
+            'key2',
+            'host2',
+            '', ''
+        ]
+        unison.ssh_authorized_peers(peer_interface='cluster',
+                                    user='foo', group='foo',
+                                    ensure_local_user=True)
+
+        ensure_user.assert_called_with('foo', 'foo')
+        get_keypair.assert_called_with('foo')
+        write_keys.assert_called_with('foo', ['key1', 'key2'])
+        write_hosts.assert_called_with('foo', ['host1', 'host2'])
+        self.relation_set.assert_called_with(ssh_authorized_hosts='host1:host2')
+
     def test_collect_authed_hosts(self):
         # only one of the hosts in fake environment has auth'd
         # the local peer