Merge lp:~jason-hobbs/maas/use-key-text into lp:~maas-committers/maas/trunk

[Jason Hobbs]

Insert the repository key directly into the yaml and preseed rather than retrieving it insecurely over http. This prevents MITM attacks when adding packages from the repository.

[Scott Moser (smoser)]

 * hard coded 'trusty' seems wrong in multiple places
 * you replaced what *was* a public key in
   apt-setup/local0/key
   with a keyring. Does something under the covers just handle either?

   Will updates to that repo work ? ie, will the key be installed
   into the target system properly?

 * test "$expected_sha256" = "$actual_sha256"
   no one is going to be able to look at a log and see what went wrong here
   this is repeated line again with the udeb.

 * it'd be nice to have some doc on how you make a keyring from a key
   to reduce the unfriendly binary content in the drivers.yaml file.

   Also, info on how to list what key is in that keyring.

 * i wonder if upgrades work (replacement of 'key' and 'key_binary' in third_party_drivers.py)

 * modprobe <driver>
   what if the driver is already loaded ? (which is likely if the booted kernel
   contains this driver).

[Jason Hobbs (jason-hobbs)]

Scott,

Thanks for the review; responses below.

On 04/14/2014 03:37 PM, Scott Moser wrote:
> * hard coded 'trusty' seems wrong in multiple places

Yes, although it isn't new in this patch.

> * you replaced what *was* a public key in
> apt-setup/local0/key
> with a keyring. Does something under the covers just handle either?

Although it's called a keyring, it's just a key. It's the same as if you
exported a single key from gpg. I can rename it to pubkey.gpg to make
it clearer that it's just a single key.

> Will updates to that repo work ? ie, will the key be installed
> into the target system properly?

Yes, this has been tested.

> * test "$expected_sha256" = "$actual_sha256"
> no one is going to be able to look at a log and see what went wrong here
> this is repeated line again with the udeb.

Ok, good point, I will make it output some error message.

> * it'd be nice to have some doc on how you make a keyring from a key
> to reduce the unfriendly binary content in the drivers.yaml file.
>
> Also, info on how to list what key is in that keyring.

Ok, will do.

> * i wonder if upgrades work (replacement of 'key' and 'key_binary' in third_party_drivers.py)

This has never been released so it doesn't matter, but aren't unmodified
conf files replaced on upgrade?

> * modprobe <driver>
> what if the driver is already loaded ? (which is likely if the booted kernel
> contains this driver).

modprobe returns 0 if the driver is already loaded.

Thanks,
Jason

[Andres Rodriguez (andreserl)]

Thr trusty bits shiould be changed to {{node.release}} IIRC

[Jason Hobbs (jason-hobbs)]

I'm not overly inclined to deal with the trusty hardcoded issue in this branch since it didn't originate in this branch and isn't needed to accomplish this branch's goal. If it needs to be addressed soon, I have a cleanup branch going it can go into.

[Jason Hobbs (jason-hobbs)]

Scott - I think all the points you brought up should be addressed now.

[Jason Hobbs (jason-hobbs)]

> Scott - I think all the points you brought up should be addressed now.

With the exception of the trusty hardcoded thing!

[Andres Rodriguez (andreserl)]

lgtm now! hardcoding of trusty fix is coming in a different branch.

review: Approve

=== modified file 'contrib/preseeds_v2/generic'
--- contrib/preseeds_v2/generic	2014-04-10 16:41:55 +0000
+++ contrib/preseeds_v2/generic	2014-04-14 21:33:28 +0000
@@ -27,15 +27,14 @@
 {{def driver_preseed_data}}
 {{if third_party_drivers and driver}}
 # Third party driver support.
-d-i preseed/early_command \
-    string wget -O - {{driver['repository']}}/dists/trusty/main/debian-installer/binary-amd64/Packages \
-    | wget -O /tmp/driver.udeb {{driver['repository']}}/$(grep ^Filename.*$(uname -r) \
-    | cut -f 2 -d ' ' | sort -ru | head -n 1) \
-    && udpkg -i /tmp/driver.udeb && depmod && modprobe {{driver['module']}}
+d-i preseed/early_command string \
+    echo -en '{{''.join(['\\x%x' % x for x in map(ord, str(install_udeb))])}}' > /tmp/install_udeb.sh && \
+    chmod +x /tmp/install_udeb.sh && \
+    /tmp/install_udeb.sh
 
 d-i apt-setup/local0/repository string deb {{driver['repository']}} trusty main
 d-i apt-setup/local0/comment string {{driver['comment']}}
-d-i apt-setup/local0/key string {{driver['key']}}
+d-i apt-setup/local0/key string file:///tmp/maas-{{driver['package']}}/repo_key.gpg
 {{endif}}
 {{enddef}}
 
@@ -56,3 +55,56 @@
 {{endif}}
     true
 {{enddef}}
+
+{{def install_udeb}}
+{{if third_party_drivers and driver}}
+#!/usr/bin/env sh
+
+set -eu
+
+REPO={{driver['repository']}}
+KERNEL_VERSION=`uname -r`
+TMPDIR=/tmp/maas-{{driver['package']}}
+mkdir $TMPDIR
+
+echo -en '{{''.join(['\\x%x' % x for x in map(ord, driver['key_binary'])])}}' > $TMPDIR/repo_key.gpg
+
+# Retrieve the Release file and verify it against the repository's key.
+wget -O $TMPDIR/Release $REPO/dists/trusty/Release
+wget -O $TMPDIR/Release.gpg $REPO/dists/trusty/Release.gpg
+gpgv --keyring $TMPDIR/repo_key.gpg $TMPDIR/Release.gpg $TMPDIR/Release
+
+# Retrieve the Packages file and verify it against the Releases file.
+wget -O $TMPDIR/Packages $REPO/dists/trusty/main/debian-installer/binary-amd64/Packages
+expected_sha256=`sed -n -e '/^SHA256:$/,$p' $TMPDIR/Release | grep 'main/debian-installer/binary-amd64/Packages$' | cut -f 2 -d ' '`
+actual_sha256=`sha256sum $TMPDIR/Packages | cut -f 1 -d ' '`
+if [ "$expected_sha256" != "$actual_sha256" ]
+then
+    echo "Packages sha256 value mismatch."
+    echo "expected: $expected_sha256, actual: $actual_sha256"
+    exit 1
+fi
+
+# Retrieve the udeb and verify it against the Packages file. This method
+# of locating the sha256 sum for the udeb within the Packages file
+# relies on the SHA256 line coming after the Filename line in the udeb's
+# record in the Packages file.
+filename=`grep ^Filename.*$KERNEL_VERSION $TMPDIR/Packages  | cut -f 2 -d ' ' | sort -ru | head -n 1`
+wget -O $TMPDIR/driver.udeb $REPO/$filename
+basename=${filename##*/}
+sed_expression="/$basename"'$/,$p'
+expected_udeb_sha256=`sed -n -e $sed_expression $TMPDIR/Packages | grep ^SHA256: | cut -f 2 -d ' ' | head -n 1`
+actual_udeb_sha256=`sha256sum $TMPDIR/driver.udeb | cut -f 1 -d ' '`
+if [ "$expected_udeb_sha256" != "$actual_udeb_sha256" ]
+then
+    echo "udeb sha256 value mismatch."
+    echo "expected: $expected_udeb_sha256, actual: $actual_udeb_sha256"
+    exit 1
+fi
+
+# Install the udeb and load the kernel module.
+udpkg -i $TMPDIR/driver.udeb
+depmod
+modprobe {{driver['module']}}
+{{endif}}
+{{enddef}}

=== modified file 'etc/maas/drivers.yaml'
--- etc/maas/drivers.yaml	2014-04-10 20:07:41 +0000
+++ etc/maas/drivers.yaml	2014-04-14 21:33:28 +0000
@@ -1,7 +1,14 @@
 drivers:
 - blacklist: ahci
   comment: HPVSA driver
-  key: http://keyserver.ubuntu.com/pks/lookup?search=0x509C5B70C2755E20F737DC27933312C3CF700356&op=get
+  key_binary: !!binary |
+         mI0EUXBVgAEEAMMuSjjc/OYlR76O9dRcMBipKp5/gTUoYovm0dxVDb+JnyOqsVlad+pZbUhTiQnl
+         ogoZdbt4i/yffP/8TydipLd3emw6xjyN5wkhGZfVvnslopZcPsjvXIdnC1sSbdWe/NwOLGn9svc+
+         FimbIrwvVx4sL/Z50QK1/4Cu6Gaxh531ABEBAAG0I0xhdW5jaHBhZCBQUEEgZm9yIEhQIElTUyBM
+         aW51eCBUZWFtiLgEEwECACIFAlFwVYACGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJEJMz
+         EsPPcANWfQ8D/RYq9u2QfyQej+2r7nGlETXl79xvaFzqwfkzuYwrUnxDuxUGTZz+yrLRBdPmYN4y
+         d7neu+9NsDAoHkpL5wyCgC7FSVSXZOT7PsMvj5IncQQc4ktnBp6vKTdfoQ1bIGaQEPrbliz6whxP
+         YfNe1eT+RyB4YP5/lclGsj+dtOdLzKh9sAIAAw==
   modaliases: 
   - 'pci:v00001590d00000047sv00001590sd00000047bc*sc*i*'
   - 'pci:v00001590d00000045sv00001590sd00000045bc*sc*i*'

=== modified file 'src/maasserver/tests/test_third_party_drivers.py'
--- src/maasserver/tests/test_third_party_drivers.py	2014-04-10 20:07:41 +0000
+++ src/maasserver/tests/test_third_party_drivers.py	2014-04-14 21:33:28 +0000
@@ -110,35 +110,14 @@
 
 class TestDriversConfig(MAASTestCase):
 
-    production_config = {
-        'drivers': [
-            {
-                'blacklist': 'ahci',
-                'comment': 'HPVSA driver',
-                'key': ('http://keyserver.ubuntu.com/pks/lookup?search='
-                        '0x509C5B70C2755E20F737DC27933312C3CF700356&op=get'),
-                'modaliases': [
-                    'pci:v00001590d00000047sv00001590sd00000047bc*sc*i*',
-                    'pci:v00001590d00000045sv00001590sd00000045bc*sc*i*',
-                    'pci:v00008086d00001D04sv00001590sd00000048bc*sc*i*',
-                    'pci:v00008086d00008C04sv00001590sd00000084bc*sc*i*',
-                    'pci:v00008086d00008C06sv00001590sd00000084bc*sc*i*',
-                    'pci:v00008086d00001C04sv00001590sd0000006Cbc*sc*i*',
-                ],
-                'module': 'hpvsa',
-                'repository':
-                'http://ppa.launchpad.net/hp-iss-team/hpvsa-update/ubuntu',
-                'package': 'hpvsa',
-            },
-        ]
-    }
-
     def test_get_defaults_returns_empty_drivers_list(self):
         observed = DriversConfig.get_defaults()
         self.assertEqual({'drivers': []}, observed)
 
     def test_load_from_yaml(self):
         filename = os.path.join(root, "etc", "maas", "drivers.yaml")
-        self.assertEqual(
-            self.production_config,
-            DriversConfig.load(filename))
+        for entry in DriversConfig.load(filename)['drivers']:
+            self.assertItemsEqual(
+                ['blacklist', 'comment', 'key_binary', 'modaliases',
+                 'module', 'repository', 'package'],
+                entry)

=== modified file 'src/maasserver/third_party_drivers.py'
--- src/maasserver/third_party_drivers.py	2014-04-10 20:07:41 +0000
+++ src/maasserver/third_party_drivers.py	2014-04-14 21:33:28 +0000
@@ -3,8 +3,7 @@
 
 """Third party driver support.
 
-The current implementation is limited in a number of ways.
-
+The current implementation is limited in a number of ways:
 - Third party driver locations are currently hardcoded. Eventually,
   they will be fetched from a stream.
 - Only one third party driver can be matched per system.
@@ -55,7 +54,31 @@
 comment - A comment string that will be added to the sources.list file
 on the target.
 
-key - The URL to the public key for the repository.
+key_binary - The public key for the repository in binary. Starting with a
+binary gpg key file (NOT ascii armored) containing the key for a repository,
+you can generate the key binary string like this:
+
+>>>> import yaml
+>>>> key_text = open('my_key.gpg').read()
+>>>> print yaml.dump(key_text)
+
+NOTE: If you start off with an ascii armored key, you can convert it into
+a binary key by importing it into a gpg keyring, then exporting it into
+a file without using -a/--armor.
+
+You can inspect a key from drivers.yaml by dumping it back into a file
+and using gpg to manipulate it:
+
+>>> import yaml
+>>> drivers_config = yaml.load(open('etc/maas/drivers.yaml').read())
+>>> drivers_list = drivers_config['drivers']
+>>> first_driver = drivers_list[0]
+>>> open('some_key.gpg', 'w').write(first_driver['key_binary'])
+
+$ gpg --import -n some_key.gpg
+gpg: key CF700356: "Launchpad PPA for Some Driver" not changed
+gpg: Total number processed: 1
+gpg:              unchanged: 1
 
 modaliases - The list of modaliases patterns to match when deciding when
 to use this driver. MAAS collects modalias strings for nodes during
@@ -78,7 +101,7 @@
 
     blacklist = String()
     comment = String()
-    key = String()
+    key_binary = String()
     modaliases = ForEach(String)
     module = String()
     package = String()