pushimage: allow for multiple keysets/image inputs

If we want to sign multiple images with multiple keysets, but the same
artifact type, today we can't because we assume a branch produces just
one of each (a single recovery or factory image, but multiple keysets).

With some firmware branches (like samus) though, we include multiple
firmware images that are the same type, but for different pieces of
hardware, which means different keysets.  This is a bit outside of the
original design/assumption of pushimage/branches, but we can do it!

Now the input insns format will check & merge overlays.  So a file that
looks something like:
  [insns]
  channel = dev
  [insns.one]
  keyset = Zinger
  input_files = zinger/ec.bin
  [insns.two]
  keyset = Hoho
  input_files = hoho/ec.bin

Will post two insn files:
  [insns]
  channel = dev
  keyset = Zinger
  input_files = zinger/ec.bin
And:
  [insns]
  channel = dev
  keyset = Hoho
  input_files = hoho/ec.bin

The keyset part doesn't seem terribly interesting (since we already allow
people to specify multiple keysets in [insns]), but makes more sense when
you tweak the other fields like channel, input_files, and output_names.

BUG=chrome-os-partner:47557
TEST=unittests pass

Change-Id: I4e8f6a88422c73e2b5b3c1042bf240afc3b80d83
Reviewed-on: https://chromium-review.googlesource.com/312300
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

4 files changed, 262 insertions, 45 deletions

diff --git a/scripts/pushimage.py b/scripts/pushimage.py
index e770d123e..342295f34 100644
--- a/scripts/pushimage.py
+++ b/scripts/pushimage.py
@@ -151,12 +151,37 @@ class InputInsns(object):
     """
     return self.SplitCfgField(self.cfg.get('insns', 'channel'))
 
-  def GetKeysets(self):
-    """Return the list of keysets to sign for this board."""
+  def GetKeysets(self, insns_merge=None):
+    """Return the list of keysets to sign for this board.
+
+    Args:
+      insns_merge: The additional section to look at over [insns].
+    """
+    # First load the default value from [insns.keyset] if available.
+    sections = ['insns']
+    # Then overlay the [insns.xxx.keyset] if requested.
+    if insns_merge is not None:
+      sections += [insns_merge]
+
+    keyset = ''
+    for section in sections:
+      try:
+        keyset = self.cfg.get(section, 'keyset')
+      except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
+        pass
+
+    # We do not perturb the order (e.g. using sorted() or making a set())
+    # because we want the behavior stable, and we want the input insns to
+    # explicitly control the order (since it has an impact on naming).
+    return self.SplitCfgField(keyset)
+
+  def GetAltInsnSets(self):
+    """Return the list of alternative insn sections."""
     # We do not perturb the order (e.g. using sorted() or making a set())
     # because we want the behavior stable, and we want the input insns to
     # explicitly control the order (since it has an impact on naming).
-    return self.SplitCfgField(self.cfg.get('insns', 'keyset'))
+    ret = [x for x in self.cfg.sections() if x.startswith('insns.')]
+    return ret if ret else [None]
 
   @staticmethod
   def CopyConfigParser(config):
@@ -176,9 +201,15 @@ class InputInsns(object):
 
     return ret
 
-  def OutputInsns(self, output_file, sect_insns, sect_general):
+  def OutputInsns(self, output_file, sect_insns, sect_general,
+                  insns_merge=None):
     """Generate the output instruction file for sending to the signer.
 
+    The override order is (later has precedence):
+      [insns]
+      [insns_merge]  (should be named "insns.xxx")
+      sect_insns
+
     Note: The format of the instruction file pushimage outputs (and the signer
     reads) is not exactly the same as the instruction file pushimage reads.
 
@@ -186,9 +217,16 @@ class InputInsns(object):
       output_file: The file to write the new instruction file to.
       sect_insns: Items to set/override in the [insns] section.
       sect_general: Items to set/override in the [general] section.
+      insns_merge: The alternative insns.xxx section to merge.
     """
     # Create a copy so we can clobber certain fields.
     config = self.CopyConfigParser(self.cfg)
+    sect_insns = sect_insns.copy()
+
+    # Merge in the alternative insns section if need be.
+    if insns_merge is not None:
+      for k, v in config.items(insns_merge):
+        sect_insns.setdefault(k, v)
 
     # Clear channel entry in instructions file, ensuring we only get
     # one channel for the signer to look at.  Then provide all the
@@ -200,6 +238,10 @@ class InputInsns(object):
       for k, v in fields.iteritems():
         config.set(sect, k, v)
 
+    # Now prune the alternative sections.
+    for alt in self.GetAltInsnSets():
+      config.remove_section(alt)
+
     output = cStringIO.StringIO()
     config.write(output)
     data = output.getvalue()
@@ -468,48 +510,51 @@ def PushImage(src_path, board, versionrev=None, profile=None, priority=50,
                      gs_artifact_path)
         continue
 
-      # Figure out which keysets have been requested for this type.  We sort the
-      # forced set so tests/runtime behavior is stable, and because we need/want
-      # list since we'll be indexing it below w/multiple keysets.
-      keysets = sorted(force_keysets)
-      if not keysets:
-        keysets = input_insns.GetKeysets()
+      first_image = True
+      for alt_insn_set in input_insns.GetAltInsnSets():
+        # Figure out which keysets have been requested for this type.
+        # We sort the forced set so tests/runtime behavior is stable.
+        keysets = sorted(force_keysets)
         if not keysets:
-          logging.warning('Skipping %s image signing due to no keysets',
-                          image_type)
-
-      for keyset in keysets:
-        sect_insns['keyset'] = keyset
-
-        # Generate the insn file for this artifact that the signer will use,
-        # and flag it for signing.
-        with tempfile.NamedTemporaryFile(
-            bufsize=0, prefix='pushimage.insns.') as insns_path:
-          input_insns.OutputInsns(insns_path.name, sect_insns, sect_general)
-
-          gs_insns_path = '%s/%s' % (dst_path, dst_name)
-          if keyset != keysets[0]:
-            gs_insns_path += '-%s' % keyset
-          gs_insns_path += '.instructions'
-
-          try:
-            ctx.Copy(insns_path.name, gs_insns_path)
-          except gs.GSContextException:
-            unknown_error[0] = True
-            logging.error('Unknown error while uploading insns %s',
-                          gs_insns_path, exc_info=True)
-            continue
-
-          try:
-            MarkImageToBeSigned(ctx, tbs_base, gs_insns_path, priority)
-          except gs.GSContextException:
-            unknown_error[0] = True
-            logging.error('Unknown error while marking for signing %s',
-                          gs_insns_path, exc_info=True)
-            continue
-          logging.info('Signing %s image with keyset %s at %s', image_type,
-                       keyset, gs_insns_path)
-          instruction_urls.setdefault(channel, []).append(gs_insns_path)
+          keysets = input_insns.GetKeysets(insns_merge=alt_insn_set)
+          if not keysets:
+            logging.warning('Skipping %s image signing due to no keysets',
+                            image_type)
+
+        for keyset in keysets:
+          sect_insns['keyset'] = keyset
+
+          # Generate the insn file for this artifact that the signer will use,
+          # and flag it for signing.
+          with tempfile.NamedTemporaryFile(
+              bufsize=0, prefix='pushimage.insns.') as insns_path:
+            input_insns.OutputInsns(insns_path.name, sect_insns, sect_general,
+                                    insns_merge=alt_insn_set)
+
+            gs_insns_path = '%s/%s' % (dst_path, dst_name)
+            if not first_image:
+              gs_insns_path += '-%s' % keyset
+            first_image = False
+            gs_insns_path += '.instructions'
+
+            try:
+              ctx.Copy(insns_path.name, gs_insns_path)
+            except gs.GSContextException:
+              unknown_error[0] = True
+              logging.error('Unknown error while uploading insns %s',
+                            gs_insns_path, exc_info=True)
+              continue
+
+            try:
+              MarkImageToBeSigned(ctx, tbs_base, gs_insns_path, priority)
+            except gs.GSContextException:
+              unknown_error[0] = True
+              logging.error('Unknown error while marking for signing %s',
+                            gs_insns_path, exc_info=True)
+              continue
+            logging.info('Signing %s image with keyset %s at %s', image_type,
+                         keyset, gs_insns_path)
+            instruction_urls.setdefault(channel, []).append(gs_insns_path)
 
   if unknown_error[0]:
     raise PushError('hit some unknown error(s)', instruction_urls)
diff --git a/scripts/pushimage_unittest.py b/scripts/pushimage_unittest.py
index ac9b492dc..a423143f8 100644
--- a/scripts/pushimage_unittest.py
+++ b/scripts/pushimage_unittest.py
@@ -74,6 +74,7 @@ create_nplusone = true
 """
 
     insns = pushimage.InputInsns('test.board')
+    self.assertEqual(insns.GetAltInsnSets(), [None])
     m = self.PatchObject(osutils, 'WriteFile')
     insns.OutputInsns('/bogus', {}, {})
     self.assertTrue(m.called)
@@ -111,6 +112,57 @@ config_board = test.board
     content = m.call_args_list[0][0][1]
     self.assertEqual(content.rstrip(), exp_content.rstrip())
 
+  def testOutputInsnsMergeAlts(self):
+    """Verify handling of alternative insns.xxx sections"""
+    TEMPLATE_CONTENT = """[insns]
+channel = %(channel)s
+chromeos_shell = false
+ensure_no_password = true
+firmware_update = true
+security_checks = true
+create_nplusone = true
+override = sect_insns
+keyset = %(keyset)s
+%(extra)s
+[general]
+board = board
+config_board = test.board
+"""
+
+    exp_alts = ['insns.one', 'insns.two', 'insns.hotsoup']
+    exp_fields = {
+        'one': {'channel': 'dev canary', 'keyset': 'OneKeyset', 'extra': ''},
+        'two': {'channel': 'best', 'keyset': 'TwoKeyset', 'extra': ''},
+        'hotsoup': {
+            'channel': 'dev canary',
+            'keyset': 'ColdKeyset',
+            'extra': 'soup = cheddar\n',
+        },
+    }
+
+    # Make sure this overrides the insn sections.
+    sect_insns = {
+        'override': 'sect_insns',
+    }
+    sect_insns_copy = sect_insns.copy()
+    sect_general = {
+        'config_board': 'test.board',
+        'board': 'board',
+    }
+
+    insns = pushimage.InputInsns('test.multi')
+    self.assertEqual(insns.GetAltInsnSets(), exp_alts)
+    m = self.PatchObject(osutils, 'WriteFile')
+
+    for alt in exp_alts:
+      m.reset_mock()
+      insns.OutputInsns('/a/file', sect_insns, sect_general, insns_merge=alt)
+      self.assertEqual(sect_insns, sect_insns_copy)
+      self.assertTrue(m.called)
+      content = m.call_args_list[0][0][1]
+      exp_content = TEMPLATE_CONTENT % exp_fields[alt[6:]]
+      self.assertEqual(content.rstrip(), exp_content.rstrip())
+
 
 class MarkImageToBeSignedTest(gs_unittest.AbstractGSContextTest):
   """Tests for MarkImageToBeSigned()"""
@@ -296,6 +348,30 @@ class PushImageTests(gs_unittest.AbstractGSContextTest):
                                  force_keysets=('key1', 'key2', 'key3'))
     self.assertEqual(urls, EXPECTED)
 
+  def testMultipleAltInsns(self):
+    """Verify behavior when processing an insn w/multiple insn overlays"""
+    EXPECTED = {
+        'canary': [
+            ('gs://chromeos-releases/canary-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi.instructions'),
+            ('gs://chromeos-releases/canary-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi-TwoKeyset.instructions'),
+            ('gs://chromeos-releases/canary-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi-ColdKeyset.instructions'),
+        ],
+        'dev': [
+            ('gs://chromeos-releases/dev-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi.instructions'),
+            ('gs://chromeos-releases/dev-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi-TwoKeyset.instructions'),
+            ('gs://chromeos-releases/dev-channel/test.multi/1.0.0/'
+             'ChromeOS-recovery-R1-1.0.0-test.multi-ColdKeyset.instructions'),
+        ],
+    }
+    with mock.patch.object(gs.GSContext, 'Exists', return_value=True):
+      urls = pushimage.PushImage('/src', 'test.multi', 'R1-1.0.0')
+    self.assertEqual(urls, EXPECTED)
+
 
 class MainTests(cros_test_lib.MockTestCase):
   """Tests for main()"""
diff --git a/signing/signer_instructions/README b/signing/signer_instructions/README
new file mode 100644
index 000000000..e7f90f124
--- /dev/null
+++ b/signing/signer_instructions/README
@@ -0,0 +1,76 @@
+=== PREFACE ===
+NOTE: The files in chromite/ are currently only used for testing.  The actual
+files used by releases live in crostools/signer_instructions/.  The program
+managers would prefer to keep them internal for now.
+
+=== OVERVIEW ===
+This directory holds instruction files that are used when uploading files for
+signing with official keys.  The pushimage script will process them to create
+output instruction files which are then posted to a Google Storage bucket that
+the signing processes watch.  The input files tell pushimage how to operate,
+and output files tell the signer how to operate.
+
+This file covers things that pushimage itself cares about.  It does not get into
+the fields that the signer utilizes.  See REFERENCES below for that.
+
+=== FILES ===
+DEFAULT.instructions - default values for all boards/artifacts; loaded first
+DEFAULT.$TYPE.instructions - default values for all boards for a specific type
+$BOARD.instructions - default values for all artifacts for $BOARD, and used for
+                      recovery images
+$BOARD.$TYPE.instructions - values specific to a board and artifact type; see
+                            the --sign-types argument to pushimage
+
+=== FORMAT ===
+There are a few main sections that pushimage cares about:
+[insns]
+[insns.XXX]  (Where XXX can be anything)
+[general]
+
+Other sections are passed through to the signer untouched, and many fields in
+the above sections are also unmodified.
+
+The keys that pushimage looks at are:
+[insns]
+channels = comma/space delimited list of the channels to flag for signing
+keysets = comma/space delimited list of the keysets to use when signing
+
+A bunch of fields will also be clobbered in the [general] section as pushimage
+writes out metadata based on the command line flags/artifacts.
+
+=== MULTI CHANNEL/KEYSET ===
+When you want to sign a single board/artifact type for multiple channels or
+keysets, simply list them in insns.channels and insn.keysets.  The pushimage
+script will take care of posting to the right subdirs and creating unique
+filenames based on those.
+
+=== MULTI INPUTS ===
+When you want to sign multiple artifacts for a single board (and all the same
+artifact type), you need to use the multiple input form instead.  When you
+create multiple sections that start with "insns.", pushimage will overlay that
+on top of the insns section, and then produce multiple ouput requests.
+
+So if you wrote a file like:
+  [insns]
+  channel = dev
+  [insns.one]
+  keyset = Zinger
+  input_files = zinger/ec.bin
+  [insns.two]
+  keyset = Hoho
+  input_files = hoho/ec.bin
+
+Pushimage will produce two requests for the signer:
+  [insns]
+  channel = dev
+  keyset = Zinger
+  input_files = zinger/ec.bin
+And:
+  [insns]
+  channel = dev
+  keyset = Hoho
+  input_files = hoho/ec.bin
+
+=== REFERENCES ===
+For details on the fields that the signer uses:
+https://sites.google.com/a/google.com/chromeos/resources/engineering/releng/signer-documentation
diff --git a/signing/signer_instructions/test.multi.instructions b/signing/signer_instructions/test.multi.instructions
new file mode 100644
index 000000000..2ac94a1c0
--- /dev/null
+++ b/signing/signer_instructions/test.multi.instructions
@@ -0,0 +1,20 @@
+[insns]
+channel = dev canary
+chromeos_shell = false
+ensure_no_password = true
+firmware_update = true
+security_checks = true
+create_nplusone = true
+override = base
+
+[insns.one]
+keyset = OneKeyset
+override = alt
+
+[insns.two]
+keyset = TwoKeyset
+channel = best
+
+[insns.hotsoup]
+keyset = ColdKeyset
+soup = cheddar