Merge ~ltrager/maas:enlistment_sshkeys into maas:master

[Lee Trager]

Provide all administrator SSH keys to the enlistment environment.

When a new node enlists it does not have an owner. Previously that meant no
SSH keys were sent. If enlistment failed it was impossible to debug as there
was no way to access the system. Now the SSH keys for all administrators are
sent so any administrator can debug the problem.

[Andres Rodriguez (andreserl)]

I’m not quite sure about this. We should talk about this. If there are many admins the keys of all of them would be here and this may change once we do rbac.

review: Needs Information

[MAAS Lander (maas-lander)]

UNIT TESTS
-b enlistment_sshkeys lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 80a8ed48ac86c5c689eb91cae3633c40185e2a92

review: Approve

c0d4fe7... by Lee Trager on 2018-09-11

Merge branch 'master' into enlistment_sshkeys

[MAAS Lander (maas-lander)]

UNIT TESTS
-b enlistment_sshkeys lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: c0d4fe72da62b751a8dfc18a6ceacf42d594a46d

review: Approve

[Blake Rouse (blake-rouse)]

Rejecting due to inactivity.

Unmerged commits

c0d4fe7... by Lee Trager on 2018-09-11

Merge branch 'master' into enlistment_sshkeys

80a8ed4... by Lee Trager on 2018-08-14

Provide all administrator SSH keys to the enlistment environment.

When a new node enlists it does not have an owner. Previously that meant no
SSH keys were sent. If enlistment failed it was impossible to debug as there
was no way to access the system. Now the SSH keys for all administrators are
sent so any administrator can debug the problem.

diff --git a/src/maasserver/models/sshkey.py b/src/maasserver/models/sshkey.py
index f0dfdf3..3933e21 100644
--- a/src/maasserver/models/sshkey.py
+++ b/src/maasserver/models/sshkey.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """:class:`SSHKey` and friends."""
@@ -32,6 +32,11 @@ class SSHKeyManager(Manager):
         """Return the text of the ssh keys associated with a user."""
         return SSHKey.objects.filter(user=user).values_list('key', flat=True)
 
+    def get_keys_for_all_admins(self):
+        """Return all keys from all admins."""
+        return SSHKey.objects.filter(user__is_superuser=True).values_list(
+            'key', flat=True)
+
 
 def validate_ssh_public_key(value):
     """Validate that the given value contains a valid SSH public key."""
diff --git a/src/maasserver/models/tests/test_sshkey.py b/src/maasserver/models/tests/test_sshkey.py
index 03bde0e..1a6d4a6 100644
--- a/src/maasserver/models/tests/test_sshkey.py
+++ b/src/maasserver/models/tests/test_sshkey.py
@@ -1,10 +1,11 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the SSHKey model."""
 
 __all__ = []
 
+from itertools import chain
 import random
 
 from django.core.exceptions import ValidationError
@@ -335,3 +336,16 @@ class SSHKeyManagerTest(MAASServerTestCase):
         factory.make_user_with_keys(n_keys=2)
         keys = SSHKey.objects.get_keys_for_user(user1)
         self.assertItemsEqual([key.key for key in created_keys], keys)
+
+    def test_get_keys_for_all_admins_no_keys(self):
+        for _ in range(3):
+            factory.make_admin()
+        keys = SSHKey.objects.get_keys_for_all_admins()
+        self.assertItemsEqual([], keys)
+
+    def test_get_keys_for_all_admins_with_keys(self):
+        admin1, key1 = factory.make_admin_with_keys(n_keys=3)
+        admin2, key2 = factory.make_admin_with_keys(n_keys=4)
+        user, key3 = factory.make_user_with_keys()
+        keys = SSHKey.objects.get_keys_for_all_admins()
+        self.assertItemsEqual([key.key for key in chain(key1, key2)], keys)
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index c79ef07..b5ac5c0 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -1547,6 +1547,31 @@ class Factory(maastesting.factory.Factory):
         user.userprofile.save()
         return user
 
+    def make_admin_with_keys(
+            self, n_keys=2, user=None, keysource=None, **kwargs):
+        """Create an admin with n `SSHKey`.  If user is not None, use this user
+        instead of creating one.  If keysource is not None, use this keysource
+        instaed of creating one.
+
+        Additional keyword arguments are passed to `make_user()`.
+        """
+        if n_keys > MAX_PUBLIC_KEYS:
+            raise RuntimeError(
+                "Cannot create more than %d public keys.  If you need more: "
+                "add more keys in src/maasserver/tests/data/."
+                % MAX_PUBLIC_KEYS)
+        if user is None:
+            user = self.make_admin(**kwargs)
+        if keysource is None:
+            keysource = self.make_KeySource()
+        keys = []
+        for i in range(n_keys):
+            key_string = get_data('data/test_rsa%d.pub' % i)
+            key = SSHKey(user=user, key=key_string, keysource=keysource)
+            key.save()
+            keys.append(key)
+        return user, keys
+
     def make_FileStorage(self, filename=None, content=None, owner=None):
         fake_file = self.make_file_upload(filename, content)
         return FileStorage.objects.save_file(fake_file.name, fake_file, owner)
diff --git a/src/metadataserver/api.py b/src/metadataserver/api.py
index f096347..602c4c1 100644
--- a/src/metadataserver/api.py
+++ b/src/metadataserver/api.py
@@ -1102,28 +1102,51 @@ class EnlistMetaDataHandler(OperationsHandler):
 
     create = update = delete = None
 
-    data = {
-        'instance-id': 'i-maas-enlistment',
-        'local-hostname': "maas-enlisting-node",
-        'public-keys': "",
-    }
-
     def read(self, request, version, item=None):
         check_version(version)
 
+        producers = {
+            'instance-id': self.instance_id,
+            'local-hostname': self.local_hostname,
+            'public-keys': self.public_keys,
+        }
+
         # Requesting the list of attributes, not any particular attribute.
         if item is None or len(item) == 0:
-            keys = sorted(self.data.keys())
-            # There's nothing in public-keys, so we don't advertise it.
-            # But cloud-init does ask for it and it's not worth logging
-            # a traceback for.
-            keys.remove('public-keys')
-            return make_list_response(keys)
-
-        if item not in self.data:
+            subfields = list(producers.keys())
+            # Add public-keys to the list of attributes, if the
+            # an admin has registered SSH keys.
+            if not SSHKey.objects.get_keys_for_all_admins().exists():
+                subfields.remove('public-keys')
+            return make_list_response(sorted(subfields))
+
+        if item not in producers:
             raise MAASAPINotFound("Unknown metadata attribute: %s" % item)
 
-        return make_text_response(self.data[item])
+        return producers[item]()
+
+    def instance_id(self):
+        """Return a generic instance id.
+
+        A permanent instance id will be generated when the node object is
+        created.
+        """
+        return make_text_response('i-maas-enlistment')
+
+    def local_hostname(self):
+        """Return a generic hostname.
+
+        A unique hostname will be generated when the node object is created.
+        """
+        return make_text_response('maas-enlisting-node')
+
+    def public_keys(self):
+        """Return all SSH keys from all admins.
+
+        As the enlistment environment does not have an owner return all SSH
+        keys from all admins to allow debugging.
+        """
+        return make_list_response(SSHKey.objects.get_keys_for_all_admins())
 
 
 class EnlistUserDataHandler(OperationsHandler):
diff --git a/src/metadataserver/tests/test_api.py b/src/metadataserver/tests/test_api.py
index 6f5c67a..68a7339 100644
--- a/src/metadataserver/tests/test_api.py
+++ b/src/metadataserver/tests/test_api.py
@@ -2842,14 +2842,14 @@ class TestEnlistViews(MAASServerTestCase):
         # just insist content is non-empty. It doesn't matter what it is.
         self.assertTrue(response.content)
 
-    def test_public_keys_returns_empty(self):
-        # An enlisting node has no SSH keys, but it does request them.
-        # If the node insists, we give it the empty list.
+    def test_public_keys_returns_admin_keys(self):
+        _, keys = factory.make_admin_with_keys(n_keys=1)
+        factory.make_user_with_keys()
         md_url = reverse(
             'enlist-metadata-meta-data', args=['latest', 'public-keys'])
         response = self.client.get(md_url)
         self.assertEqual(
-            (http.client.OK, ""),
+            (http.client.OK, ''.join([key.key for key in keys])),
             (response.status_code,
              response.content.decode(settings.DEFAULT_CHARSET)))
 
diff --git a/src/metadataserver/user_data/templates/enlistment.template b/src/metadataserver/user_data/templates/enlistment.template
index f389108..18f5333 100644
--- a/src/metadataserver/user_data/templates/enlistment.template
+++ b/src/metadataserver/user_data/templates/enlistment.template
@@ -53,7 +53,7 @@ main() {
     echo $output | jq .
     if [ $ret -ne 0 ]; then
 	echo "Enlistment on {{server_url}} failed!"
-	sleep 10
+	sleep 120
 	exit 1
     fi