Merge lp:~jani/ubuntu-system-image/skip-gpg-verification into lp:~registry/ubuntu-system-image/client

[Jani Monoses]

system-image-cli: Add --skip-gpg-verification flag.

Allows skipping GPG verification while doing development.

[Barry Warsaw (barry)]

I'm finally getting a chance to review this! ;)

Thanks very much for your contribution. I'd like to see one more test to prove that --skip-gpg-verification actually works. The test would craft an update that has a broken signature, and would show that without this flag, the update fails. Then, the cli would be invoked again with the flag and the update would pass. Can you add such a test?

Other than that, it looks quite good. Thanks.

review: Needs Fixing

[Jani Monoses (jani)]

How should I craft the update? Should I add new files (tar.xz and .asc files) to data? Is there another testcase I can use as a starting point?

[Barry Warsaw (barry)]

As discussed on IRC, I will add the tests when I merge this.

[Barry Warsaw (barry)]

As discussed elsewhere, if we really want to be able to skip GPG signatures, we probably want to skip them in the -dbus process too, which would mean there would have to be a client.ini setting for this since obviously the command line switch is only useful for the -cli script.

Aren't we still debating whether this is a good idea though, given that it should be fairly easy to build a valid chain of keys?

[Jani Monoses (jani)]

There's a use case I am not sure a new chain of keys cover, that is having vanilla Ubuntu tarball signed by Ubuntu keys and a device tarball signed by a third party key. The device-signing key works for this, but an entirely new key chain would require signing the Ubuntu tarball too.
This is an issue in the case of a server we're using internally which does not touch ubuntu tarballs but refers to their links on the s-i.ubuntu.com server, but it does serve device tarballs.

As for a client.ini setting, I think that is ok. Such a config value and a test in gpg.py was my original take on this.

[Barry Warsaw (barry)]

After reviewing this again now, I think it's reasonable to add the switch to the cli. I'm much more skeptical about adding such a feature to the D-Bus process, so I'll ignore that for now and we can debate the merits of that later. If you think you need D-Bus support, please open a separate bug.

[Jani Monoses (jani)]

I agree with a cli only solution, that was my initial use case and the only one we're really interested in for development and testing. Thanks for the review :)

[Barry Warsaw (barry)]

I'm going to implement this a little differently, but I'll get the feature in.

One question: I could disable this flag if system-image-dev binary package isn't installed. That would make me feel better, but I'm guessing that won't work for you because you're testing on live devices (and normally, devices do not have that package installed).

Alternatively, we could hide --skip-gpg-verification from the --help output and/or the system-image-cli manpage. Is that worth it?

[Jani Monoses (jani)]

We mostly need this on regular devices that do not (yet) point to the official servers.

Is the threat a user being unknowingly lead into running with this flag and potentially compromising their install?

[Barry Warsaw (barry)]

On Jan 29, 2015, at 07:34 AM, Jani Monoses wrote:

>We mostly need this on regular devices that do not (yet) point to the
>official servers.
>
>Is the threat a user being unknowingly lead into running with this flag and
>potentially compromising their install?

Yep. It's just my general paranoia coming through. If there is a way to
trick the user or the system into setting this flag, all bets are off. I'd
like to see if we can come up with another safety check that doesn't make your
life more difficult but would be explicit and hard to invoke for normal users.

OTOH, most normal users will only use the D-Bus API, so maybe that's the
trick. I can think of a couple of things:

* Do not document --skip-gpg-verification in the manpage or si-cli --help
* Prevent --skip-gpg-verification from working when run under D-Bus

Would either of these give you hardship?

[Jani Monoses (jani)]

>
> trick the user or the system into setting this flag, all bets are off. I'd
> like to see if we can come up with another safety check that doesn't make
> your
> life more difficult but would be explicit and hard to invoke for normal
> users.
>
> Anything is an improvement over remounting root as rw and running sed on
the python files :)

OTOH, most normal users will only use the D-Bus API, so maybe that's the
> trick. I can think of a couple of things:
>
> * Do not document --skip-gpg-verification in the manpage or si-cli --help
>

That won't help if the user is tricked into running it no?

> * Prevent --skip-gpg-verification from working when run under D-Bus
>
>
Sure, I thought that is implied if it is only a command line flag. So while
it would be nice to be able to run the UI with no GPG checks I also think
the risks are not worth it.

> Would either of these give you hardship?
>
>
No, if an OTA can be done by running system-image-cli without changing
anything else on the system that is good for us :)
Thanks

[Barry Warsaw (barry)]

On Jan 29, 2015, at 03:00 PM, Jani Monoses wrote:

>> * Do not document --skip-gpg-verification in the manpage or si-cli --help
>
>That won't help if the user is tricked into running it no?

True. It's purely security through obscurity.

>> * Prevent --skip-gpg-verification from working when run under D-Bus
>>
>Sure, I thought that is implied if it is only a command line flag. So while
>it would be nice to be able to run the UI with no GPG checks I also think
>the risks are not worth it.

There might be ways to structure the feature so that it's simply impossible to
invoke when run under D-Bus.

>> Would either of these give you hardship?
>>
>No, if an OTA can be done by running system-image-cli without changing
>anything else on the system that is good for us :)

Cool, thanks!

[Barry Warsaw (barry)]

I've landed this in trunk. I gave you credit in the NEWS file, but I marked this branch as Rejected only because I implemented a few things differently. Thanks for the contribution!

Unmerged revisions

261. By Jani Monoses on 2014-06-11

system-image-cli: Add --skip-gpg-verification flag.

This allows testing upgrades from the command line before a signing infrastructure is in place.

=== modified file 'systemimage/config.py'
--- systemimage/config.py	2014-02-26 16:11:09 +0000
+++ systemimage/config.py	2014-06-11 08:52:29 +0000
@@ -57,6 +57,7 @@
         ini_path = resource_filename('systemimage.data', 'client.ini')
         self.load(ini_path)
         self._override = False
+        self.validate_gpg_signatures = True
         # 2013-10-14 BAW This is a placeholder for rendezvous between the
         # downloader and the D-Bus service.  When running udner D-Bus and we
         # get a `paused` signal from the download manager, we need this to

=== modified file 'systemimage/gpg.py'
--- systemimage/gpg.py	2014-06-02 20:09:06 +0000
+++ systemimage/gpg.py	2014-06-11 08:52:29 +0000
@@ -211,6 +211,8 @@
             keyrings involved in the verification, as well as the blacklist
             file if there is one.
         """
+        if not config.validate_gpg_signatures:
+            return
         if not self.verify(signature_path, data_path):
             raise SignatureError(signature_path, data_path,
                                  self.keyring_paths, self.blacklist_path)

=== modified file 'systemimage/main.py'
--- systemimage/main.py	2014-05-30 14:20:38 +0000
+++ systemimage/main.py	2014-06-11 08:52:29 +0000
@@ -110,6 +110,10 @@
                         default=None, action='store', metavar='CHANNEL',
                         help="""Switch to the given channel.  This is
                                 equivalent to `-c CHANNEL -b 0`.""")
+    parser.add_argument('--skip-gpg-verification',
+                        default=False, action='store_true',
+                        help="""Do not validate the GPG signatures of files.
+                                Useful for testing and development.""")
     # Settings options.
     parser.add_argument('--show-settings',
                         default=False, action='store_true',
@@ -154,6 +158,10 @@
         # process, so just return as normal.
         return 0
 
+    # Check whether to skip GPG signature verification.
+    if args.skip_gpg_verification:
+        config.validate_gpg_signatures = False
+
     # Handle all settings arguments.  They are mutually exclusive.
     if sum(bool(arg) for arg in
            (args.set, args.get, args.delete, args.show_settings)) > 1:

=== modified file 'systemimage/tests/test_main.py'
--- systemimage/tests/test_main.py	2014-06-02 20:09:06 +0000
+++ systemimage/tests/test_main.py	2014-06-11 08:52:29 +0000
@@ -585,6 +585,13 @@
             exit_code = cli_main()
         self.assertEqual(exit_code, 1)
 
+    def test_skip_gpg_verification(self):
+        self.assertTrue(config.validate_gpg_signatures)
+        self._resources.enter_context(
+            patch('systemimage.main.sys.argv',
+                   ['argv0', '--skip-gpg-verification']))
+        cli_main()
+        self.assertFalse(config.validate_gpg_signatures)
 
 class TestCLIMainDryRun(ServerTestBase):
     INDEX_FILE = 'index_14.json'