#29 Recognize and sign side tag builds
Merged 4 years ago by abompard. Opened 4 years ago by nphilipp.
nphilipp/robosignatory master--from-side-tags  into  master

file modified
+15 -2
@@ -66,7 +66,10 @@ 

  a key name (passed to the signing module to indicate which key to use) and

  keyid (passed to koji to indicate which signatures need to be written out).

  The entry also has a "to" tag, to indicate where builds need to be moved to

- after being signed.

+ after being signed. Each entry can have an optional dict "sidetags" which

+ specify how a side tag for the respective release and its corresponding "from"

+ and "to" tags would look like, as well as which Koji users are trusted to ask

+ RoboSignatory to sign such builds ("trusted_taggers", a list of strings).

  

  Note that "from" and "to" can be the same tag, in this case builds will not be

  moved after being signed.
@@ -77,7 +80,13 @@ 

      "from": "f26-pending",

      "to": "f26",

      "key": "fedora-26",

-     "keyid": "64dab85d"

+     "keyid": "64dab85d",

+     "sidetags": {

+         "pattern": "<to>-build-side-<seq_id>",

+         "from": "<sidetag>-pending-signing",

+         "to": "<sidetag>-testing",

+         "trusted_taggers": ["bodhi"]

+     }

    }

  

  This example would watch for any builds tagged into the f26-pending tag.
@@ -91,6 +100,10 @@ 

  issue a koji moveBuild operation, moving the build from "f26-pending" to "f26".

  After this, it is done signing the package, and continues to the next step.

  

+ Analogously, it would work for a side tag build which was e.g. tagged into

+ "f26-build-side-1234-pending-signing" and move it into

+ "f26-build-side-1234-testing".

+ 

  

  Testing Koji Tag Signer

  -----------------------

file modified
+6
@@ -97,6 +97,12 @@ 

              key = 'fedora26'

              keyid = 'xxxxxxxx'

  

+                 [consumer_config.koji_instances.primary.tags.sidetags]

+                 pattern = '<to>-build-side-<seq_id>'

+                 from = '<sidetag>-pending-signing'

+                 to = '<sidetag>-testing'

+                 trusted_taggers = ['bodhi']

+ 

              [[consumer_config.koji_instances.primary.tags]]

              from = 'rawhide-modular-signcandidate'

              to = 'rawhide-modular'

file modified
+2 -1
@@ -32,7 +32,8 @@ 

          log.info('CoreOSSigner ready for service')

  

      def consume(self, msg):

-         # Message structure: https://github.com/coreos/fedora-coreos-tracker/issues/198#issuecomment-513944390

+         # Message structure:

+         # https://github.com/coreos/fedora-coreos-tracker/issues/198#issuecomment-513944390

          log.info(

              'CoreOS wants to sign '

              '%(build_id)s on %(stream)s for %(basearch)s' % msg.body

file modified
+121 -16
@@ -1,12 +1,16 @@ 

  from __future__ import unicode_literals, absolute_import

  

+ import logging

+ import re

+ 

  import koji

  

+ import six

+ 

  import robosignatory.utils as utils

  

- import logging

- log = logging.getLogger("robosignatory.tagconsumer")

  

+ log = logging.getLogger("robosignatory.tagconsumer")

  

  KNOWN_TAG_TYPES = ['plain', 'modular']

  
@@ -38,6 +42,7 @@ 

                  raise Exception('Only SSL and kerberos authmethods supported')

  

              instance_obj = {'client': client,

+                             'sidetags': {},

                              'tags': {}}

              if 'mbs_user' in instance_info:

                  instance_obj['mbs_user'] = instance_info['mbs_user']
@@ -55,6 +60,51 @@ 

                      raise Exception('Invalid tag type detected: %s' % tag_type)

                  instance_obj['tags'][tag['from']]['type'] = tag_type

  

+                 sidetags = tag.get('sidetags')

+                 if not sidetags:

+                     continue

+ 

+                 # Fill in from, to tags in placeholders, this will be the key

+                 # in instance_obj['sidetags'].

+                 pattern_filled_in = sidetags['pattern'].replace(

+                     '<from>', tag['from']

+                 ).replace(

+                     '<to>', tag['to']

+                 )

+ 

+                 # Escape and construct regex to match the

+                 # '<sidetag>-pending-signing' tag to which we should react.

+                 from_tag_re = re.compile(

+                     sidetags['from'].replace(

+                         '<sidetag>',

+                         r'(?P<sidetag>'

+                         + re.escape(pattern_filled_in)

+                         + r')'

+                     ).replace(re.escape('<seq_id>'), r'(?P<seq_id>[1-9][0-9]*)')

+                 )

+ 

+                 # We don't want to accept '<sidetag>-pending-signing' tagged

+                 # builds from just anyone.

+ 

+                 # The 'trusted_taggers' option is a list which defines the user

+                 # ids from which we want to accept builds tagged into the

+                 # '<sidetag>-pending-signing' tag.

+                 trusted_taggers = sidetags['trusted_taggers']

+                 if (

+                     not isinstance(trusted_taggers, list)

+                     or not all(isinstance(x, six.text_type) for x in trusted_taggers)

+                 ):

+                     raise TypeError("`trusted_taggers` must be a list of strings.")

+ 

+                 instance_obj['sidetags'][pattern_filled_in] = {

+                     'pattern': sidetags['pattern'],

+                     'from_tag_re': from_tag_re,

+                     'from': sidetags['from'],

+                     'to': sidetags['to'],

+                     'tags_key': tag['from'],

+                     'trusted_taggers': trusted_taggers,

+                 }

+ 

              self.koji_clients[instance] = instance_obj

  

              log.info('TagSigner ready for service')
@@ -85,6 +135,54 @@ 

          self.dowork(build_nvr, build_id, tag, koji_instance,

                      skip_tagging=False)

  

+     def match_sidetag(self, build_nvr, build_id, tag, koji_instance):

+         instance = self.koji_clients[koji_instance]

+ 

+         for pattern_filled_in, sidetag_matched in instance['sidetags'].items():

+             from_tag_re = sidetag_matched['from_tag_re']

+ 

+             m = from_tag_re.match(tag)

+             if not m:

+                 continue

+ 

+             sidetag = m.group('sidetag')

+ 

+             # Verify that the build was tagged into the tag by one of the

+             # trusted taggers.

+             for hist_event in sorted(instance['client'].tagHistory(build_id),

+                                      key=lambda ev: ev['create_ts'],

+                                      reverse=True):

+                 # We want to verify the latest matching tagging event only.

+                 if (

+                     hist_event['active']

+                     and hist_event['tag_name'] == tag

+                 ):

+                     if hist_event['creator_name'] in sidetag_matched['trusted_taggers']:

+                         # This is how it should be, break out of the loop.

+                         break

+ 

+                     log.error("Side tag build not tagged into %s by trusted user (%s)"

+                               " but by '%s'!",

+                               tag, ", ".join(sidetag_matched['trusted_taggers']),

+                               hist_event['creator_name'])

+                     raise Exception('Side tag build tagged by untrusted user')

+             else:

+                 # We should find the tag as active in the tagging history

+                 # of the build. Bail out because we didn't find it above

+                 # i.e. didn't break out of the loop.

+                 log.error("Couldn't find tag %s in history of build %s (%s)!",

+                           tag, build_nvr, build_id)

+                 raise Exception("Tag not found in build history")

+ 

+             tag_info = instance['tags'][sidetag_matched['tags_key']]

+             tag_to = sidetag_matched['to'].replace('<sidetag>', sidetag)

+ 

+             tag_matched = True

+ 

+             return tag_matched, tag_info, tag_to

+ 

+         return False, None, None

+ 

      def dowork(self, build_nvr, build_id, tag, koji_instance,

                 skip_tagging=False):

          instance = self.koji_clients[koji_instance]
@@ -92,14 +190,22 @@ 

          if not build_id:

              build_id = instance['client'].findBuildID(build_nvr)

  

-         if tag not in instance['tags']:

+         tag_matched = False

+ 

+         if tag in instance['tags']:

+             tag_matched = True

+             tag_info = instance['tags'][tag]

+             tag_to = tag_info['to']

+         else:

+             tag_matched, tag_info, tag_to = self.match_sidetag(build_nvr, build_id, tag,

+                                                                koji_instance)

+ 

+         if not tag_matched:

              log.info('Tag not autosigned, skipping')

              return

-         tag_info = instance['tags'][tag]

  

          log.info('Going to sign %s with %s (%s) and move to %s',

-                  build_nvr, tag_info['key'], tag_info['keyid'],

-                  tag_info['to'])

+                  build_nvr, tag_info['key'], tag_info['keyid'], tag_to)

  

          if tag_info['type'] == 'plain':

              self.signwrite_single_build(build_nvr, build_id, tag_info, instance, koji_instance)
@@ -111,13 +217,11 @@ 

          if skip_tagging:

              log.info('Tagging skipped, done')

          else:

-             log.info('Packages correctly signed, moving to %s' %

-                      tag_info['to'])

-             if tag == tag_info['to']:

+             log.info('Packages correctly signed, moving to %s' % tag_to)

+             if tag == tag_to:

                  log.info('Non-gated, not moving')

              else:

-                 instance['client'].tagBuild(tag_info['to'], build_id, False,

-                                             tag)

+                 instance['client'].tagBuild(tag_to, build_id, False, tag)

  

      def signwrite_module(self, build_nvr, build_id, tag_info, instance, koji_instance):

          log.info('Signing module build %s', build_nvr)
@@ -129,9 +233,11 @@ 

          log.info('Signing all module content')

          for build in instance['client'].listTagged(content_koji_tag):

              if build['owner_name'] != instance['mbs_user']:

-                 log.error('Build %(build_id)s has owner %(owner_name)s, which is NOT mbs_user!' % build)

+                 log.error('Build %(build_id)s has owner %(owner_name)s, which is NOT mbs_user!'

+                           % build)

                  raise Exception('Modular content tag contains invalid owned build')

-             self.signwrite_single_build(build['nvr'], build['build_id'], tag_info, instance, koji_instance)

+             self.signwrite_single_build(build['nvr'], build['build_id'], tag_info, instance,

+                                         koji_instance)

  

      def signwrite_single_build(self, build_nvr, build_id, tag_info, instance, koji_instance):

          log.info('Signing and writing build %s', build_nvr)
@@ -140,9 +246,8 @@ 

                                build_id=build_id,

                                sigkey=tag_info['keyid'])

          log.info('RPMs to sign and move: %s',

-                  ['%s (%s, signed: %s)' %

-                     (key, rpm['id'], rpm['signed'])

-                     for key, rpm in rpms.items()])

+                  ['%s (%s, signed: %s)' % (key, rpm['id'], rpm['signed'])

+                   for key, rpm in rpms.items()])

          if len(rpms) < 1:

              log.info('Build contains no rpms, skipping signing and writing')

  

file modified
+3 -2
@@ -9,7 +9,7 @@ 

  

  class Struct:

      def __init__(self, **entries):

-             self.__dict__.update(entries)

+         self.__dict__.update(entries)

  

  

  def get_rpms(koji_client, build_nvr, build_id, sigkey=None):
@@ -24,12 +24,13 @@ 

          rpminfo['%s.%s' % (rpm['nvr'], rpm['arch'])] = info

      return rpminfo

  

+ 

  def get_builds_in_tag(koji_client, tag):

      """ Return the list of builds in Koji tag. """

  

      try:

          rpms, builds = koji_client.listTaggedRPMS(tag, latest=True)

-     except koji.GenericError as e:

+     except koji.GenericError:

          log.exception("Failed to list rpms in tag %r" % tag)

          # If the tag doesn't exist.. then there are no rpms in that tag.

          return []

file modified
+2 -2
@@ -38,8 +38,8 @@ 

  

      # Sigul writes it as 0600, which makes a lot of sense as a general file

      # mode for it, but this is just a signature file that we want published

-     os.chmod(commitmetapath, (stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP |

-                               stat.S_IROTH))

+     os.chmod(commitmetapath, (stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP

+                               | stat.S_IROTH))

  

      log.info('Commit was succesfully signed, writing ref %s' % ref)

  

file added
+8
@@ -0,0 +1,8 @@ 

+ [flake8]

+ show-source = True

+ max-line-length = 100

+ exclude = .git,.tox,dist,*egg,build,tools

+ ignore = E712,W503

+ # Configure flake8-import-order

+ application-import-names = robosignatory

+ import-order-style = pep8

file modified
+96 -17
@@ -35,6 +35,12 @@ 

                      'to': 'f31',

                      'key': 'fedora-31',

                      'keyid': 'deadbeef',

+                     'sidetags': {

+                         'pattern': '<to>-build-side-<seq_id>',

+                         'from': '<sidetag>-pending-signing',

+                         'to': '<sidetag>-testing',

+                         'trusted_taggers': ['bodhi'],

+                     },

                  },

                  {

                      'from': 'f30-signing-pending',
@@ -238,21 +244,97 @@ 

  

      # Test well-formed messages.

  

+     def _prep_sidetag_test(self, sidetag_pending, error):

+         tags_key = self.test_msg['body']['tag']  # normal tag where we find robosig configuration

+         tag_conf = self.instance_obj['tags'][tags_key]

+ 

+         for sidetag_conf in self.instance_obj['sidetags'].values():

+             if sidetag_conf['tags_key'] == tags_key:

+                 tag_conf['sidetags'] = sidetag_conf

+                 break

+         else:

+             raise RuntimeError("Can't find sidetag configuration for {}".format(

+                 tag_conf['from']))

+ 

+         if error == 'untrusted-tagger':

+             tagger = 'hamburglar'

+         else:

+             tagger = 'bodhi'

+ 

+         if error == 'missing-from-history':

+             tag_history = []

+         else:

+             tag_history = [

+                 {'active': True,

+                  'create_ts': 1554076800,

+                  'creator_name': tagger,

+                  'tag_name': sidetag_pending},

+             ]

+         self.koji_client.tagHistory.return_value = tag_history

+ 

+         return tag_conf, tagger

+ 

+     def _get_expected_messages_and_exceptions(self, sidetag_pending, error,

+                                               build_owner, tag_conf, tagger):

+         body = self.test_msg['body']

+         instance = body['instance']

+         build_nvr = '{name}-{version}-{release}'.format(**body)

+         from_tag = body['tag']

+         build_id = body['build_id']

+ 

+         expected_log_msgs = [

+             "Build {} ({}) tagged into {} on {}".format(

+                 build_nvr, build_id, from_tag, instance),

+         ]

+ 

+         if error == 'non-mbs-owner':

+             expected_log_msgs.append(

+                 "Build {} has owner {}, which is NOT mbs_user!".format(build_id, build_owner))

+             expected_exc_ctx = raises(Exception,

+                                       match="Modular content tag contains invalid owned build")

+         elif error == 'untrusted-tagger':

+             expected_log_msgs.append(

+                 "Side tag build not tagged into {} by trusted user ({}) but by '{}'!".format(

+                     sidetag_pending, ", ".join(tag_conf['sidetags']['trusted_taggers']), tagger

+                 )

+             )

+             expected_exc_ctx = raises(Exception,

+                                       match="Side tag build tagged by untrusted user")

+         elif error == 'missing-from-history':

+             expected_log_msgs.append(

+                 "Couldn't find tag {} in history of build {} ({})!".format(

+                     sidetag_pending, build_nvr, build_id

+                 )

+             )

+             expected_exc_ctx = raises(Exception,

+                                       match="Tag not found in build history")

+         else:

+             expected_exc_ctx = DummyContext()

+ 

+         return expected_log_msgs, expected_exc_ctx

+ 

      @mock.patch('robosignatory.tag.utils', new_callable=MockUtils())

-     @mark.parametrize('type_,error',

-                       ((None, None), ('plain', None), ('modular', None),

-                        ('modular', 'non-mbs-owner')))

-     def test_build_messages(self, utils, caplog, type_, error):

+     @mark.parametrize(

+         'type_,sidetag_pending,error',

+         ((None, None, None),

+          ('plain', None, None),

+          ('plain', 'f31-build-side-1234-pending-signing', None),

+          ('plain', 'f31-build-side-1234-pending-signing', 'untrusted-tagger'),

+          ('plain', 'f31-build-side-1234-pending-signing', 'missing-from-history'),

+          ('modular', None, None),

+          ('modular', None, 'non-mbs-owner')))

+     def test_build_messages(self, utils, caplog, type_, sidetag_pending, error):

          """Test build messages for various types."""

          caplog.set_level(logging.DEBUG)

  

          body = self.test_msg['body']

-         instance = body['instance']

          build_nvr = '{name}-{version}-{release}'.format(**body)

          from_tag = body['tag']

          build_id = body['build_id']

          tag_conf = self.instance_obj['tags'][from_tag]

          to_tag = tag_conf['to']

+         tagger = None

+         build_owner = None

  

          if type_ == 'modular':

              body['tag'] = from_tag = 'f30-modular-signing-pending'
@@ -266,20 +348,17 @@ 

                  {'owner_name': build_owner, 'nvr': build_nvr, 'build_id': build_id}

              ]

  

-         expected_log_msgs = [

-             "Build {} ({}) tagged into {} on {}".format(

-                 build_nvr, build_id, from_tag, instance),

-         ]

+         if sidetag_pending:

+             tag_conf, tagger = self._prep_sidetag_test(sidetag_pending, error)

+             body['tag'] = from_tag = sidetag_pending

+             to_tag = sidetag_pending.replace('-pending-signing', '-testing')

+ 

+         expected_log_msgs, expected_exc_ctx = self._get_expected_messages_and_exceptions(

+             sidetag_pending, error, build_owner, tag_conf, tagger

+         )

  

-         if error == 'non-mbs-owner':

-             expected_log_msgs.append(

-                 "Build {} has owner {}, which is NOT mbs_user!".format(build_id, build_owner))

-             exc_ctx_mgr = raises(Exception,

-                                  match="Modular content tag contains invalid owned build")

-         else:

-             exc_ctx_mgr = DummyContext()

          msg = Message(**self.test_msg)

-         with exc_ctx_mgr:

+         with expected_exc_ctx:

              self.tag_signer.consume(msg)

  

          for msg in expected_log_msgs:

file modified
+1
@@ -4,6 +4,7 @@ 

  

  import robosignatory.utils

  

+ 

  class TestUtils(unittest.TestCase):

  

      def test_no_such_helper(self):

This is based off PR #27 which needs to be merged first, and fixes/implements issue #28.

10 new commits added

  • Recognize and sign side tag builds
  • improve PEP8 compliancy
  • Integrate pytest with setuptools
  • Use pytest when testing with tox
  • Test most of the TagSigner functionality
  • Migrate to pytest
  • Fix typo
  • Remove unused variable
  • Add missing % to interpolate exception message
  • Use Pythonic ways to iterate over dicts
4 years ago

rebased onto 0affc8c

4 years ago

Since we can do lists in the configuration, I think you should just require the value to be a list and not try to accept a comma-separated string.

It feels like this block should be its own function/method, to be easily tested independently and to make this already-long method shorter.

Could you please also add an example configuration in the robosignatory.toml file?

2 new commits added

  • Recognize and sign side tag builds
  • improve PEP8 compliancy
4 years ago

2 new commits added

  • Recognize and sign side tag builds
  • improve PEP8 compliancy
4 years ago

Hmm, this test is getting pretty complex, don't you think? Probably due to the parameters.
I mean when our unit tests have so many branches they need their own unit tests, something's going pretty wrong ;-)

I'm not sure what the best way would be to avoid cut-n-paste, maybe verification methods in the class?

2 new commits added

  • Recognize and sign side tag builds
  • improve PEP8 compliancy
4 years ago

2 new commits added

  • Recognize and sign side tag builds
  • improve PEP8 compliancy
4 years ago

I've broken out some code of that test method:

  • preparing/tweaking the test data
  • determining log messages and exceptions (if any) to expect

That's much clearer, thanks.

@puiterwijk do you want to have a look at this code, before I merge it? I'm asking because there's a security aspect to it. If you don't have time, don't worry.

@abompard I'm going to trust your judgement here :).
If you think it's fine, feel free to go ahead and merge.

Pull-Request has been merged by abompard

4 years ago