#2951 kojid: extend SCM.assert_allowed with hub policy
Merged 9 months ago by tkopecek. Opened 10 months ago by julian8628.
julian8628/koji issue/2757  into  master

file modified
+81 -12
@@ -1633,7 +1633,17 @@ 

          self.opts = opts

  

          scm = SCM(url)

-         scm.assert_allowed(self.options.allowed_scms)

+         scm_policy_opts = {

+             'user_id': self.taskinfo['owner'],

+             'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                strict=True)['name'],

+             'scratch': self.opts.get('scratch')

+         }

+         scm.assert_allowed(allowed=self.options.allowed_scms,

+                            session=self.session,

+                            by_config=self.options.allowed_scms_use_config,

+                            by_policy=self.options.allowed_scms_use_policy,

+                            policy_data=scm_policy_opts)

          repo_id = opts.get('repo_id')

          if not repo_id:

              raise koji.BuildError('A repo_id must be provided')
@@ -1707,7 +1717,11 @@ 

          if self.opts.get('patches'):

              patchlog = self.workdir + '/patches.log'

              patch_scm = SCM(self.opts.get('patches'))

-             patch_scm.assert_allowed(self.options.allowed_scms)

+             patch_scm.assert_allowed(allowed=self.options.allowed_scms,

+                                      session=self.session,

+                                      by_config=self.options.allowed_scms_use_config,

+                                      by_policy=self.options.allowed_scms_use_policy,

+                                      policy_data=scm_policy_opts)

              self.run_callbacks('preSCMCheckout', scminfo=patch_scm.get_info(),

                                 build_tag=build_tag, scratch=opts.get('scratch'),

                                 buildroot=buildroot)
@@ -1990,7 +2004,16 @@ 

                  assert False  # pragma: no cover

  

          scm = SCM(spec_url)

-         scm.assert_allowed(self.options.allowed_scms)

+         scm.assert_allowed(allowed=self.options.allowed_scms,

+                            session=self.session,

+                            by_config=self.options.allowed_scms_use_config,

+                            by_policy=self.options.allowed_scms_use_policy,

+                            policy_data={

+                                'user_id': self.taskinfo['owner'],

+                                'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                   strict=True)['name'],

+                                'scratch': opts.get('scratch')

+                            })

  

          repo_id = opts.get('repo_id')

          if not repo_id:
@@ -2979,7 +3002,16 @@ 

          self.logger.debug("ksfile = %s" % ksfile)

          if self.opts.get('ksurl'):

              scm = SCM(self.opts['ksurl'])

-             scm.assert_allowed(self.options.allowed_scms)

+             scm.assert_allowed(allowed=self.options.allowed_scms,

+                                session=self.session,

+                                by_config=self.options.allowed_scms_use_config,

+                                by_policy=self.options.allowed_scms_use_policy,

+                                policy_data={

+                                    'user_id': self.taskinfo['owner'],

+                                    'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                       strict=True)['name'],

+                                    'scratch': self.opts.get('scratch')

+                                })

              logfile = os.path.join(self.workdir, 'checkout.log')

              self.run_callbacks('preSCMCheckout', scminfo=scm.get_info(),

                                 build_tag=build_tag, scratch=self.opts.get('scratch'),
@@ -3453,7 +3485,16 @@ 

              can find the checked out templates.

          """

          scm = SCM(self.opts['lorax_url'])

-         scm.assert_allowed(self.options.allowed_scms)

+         scm.assert_allowed(allowed=self.options.allowed_scms,

+                            session=self.session,

+                            by_config=self.options.allowed_scms_use_config,

+                            by_policy=self.options.allowed_scms_use_policy,

+                            policy_data={

+                                'user_id': self.taskinfo['owner'],

+                                'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                   strict=True)['name'],

+                                'scratch': self.opts.get('scratch')

+                            })

          logfile = os.path.join(self.workdir, 'lorax-templates-checkout.log')

          checkout_dir = scm.checkout(build_root.tmpdir(),

                                      self.session, self.getUploadDir(), logfile)
@@ -3700,7 +3741,16 @@ 

          self.logger.debug("ksfile = %s" % ksfile)

          if self.opts.get('ksurl'):

              scm = SCM(self.opts['ksurl'])

-             scm.assert_allowed(self.options.allowed_scms)

+             scm.assert_allowed(allowed=self.options.allowed_scms,

+                                session=self.session,

+                                by_config=self.options.allowed_scms_use_config,

+                                by_policy=self.options.allowed_scms_use_policy,

+                                policy_data={

+                                    'user_id': self.taskinfo['owner'],

+                                    'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                       strict=True)['name'],

+                                    'scratch': self.opts.get('scratch')

+                                })

              logfile = os.path.join(self.workdir, 'checkout-%s.log' % self.arch)

              self.run_callbacks('preSCMCheckout', scminfo=scm.get_info(),

                                 build_tag=build_tag, scratch=self.opts.get('scratch'))
@@ -4527,7 +4577,16 @@ 

          self.logger.debug("filepath = %s" % filepath)

          if fileurl:

              scm = SCM(fileurl)

-             scm.assert_allowed(self.options.allowed_scms)

+             scm.assert_allowed(allowed=self.options.allowed_scms,

+                                session=self.session,

+                                by_config=self.options.allowed_scms_use_config,

+                                by_policy=self.options.allowed_scms_use_policy,

+                                policy_data={

+                                    'user_id': self.taskinfo['owner'],

+                                    'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                       strict=True)['name'],

+                                    'scratch': self.opts.get('scratch')

+                                })

              self.run_callbacks('preSCMCheckout', scminfo=scm.get_info(),

                                 build_tag=build_tag, scratch=self.opts.get('scratch'))

              logfile = os.path.join(self.workdir, 'checkout.log')
@@ -4935,12 +4994,20 @@ 

          return self.checkHostArch(tag, hostdata)

  

      def handler(self, url, build_tag, opts=None):

-         # will throw a BuildError if the url is invalid

-         scm = SCM(url)

-         scm.assert_allowed(self.options.allowed_scms)

- 

          if opts is None:

              opts = {}

+         # will throw a BuildError if the url is invalid

+         scm = SCM(url)

+         scm.assert_allowed(allowed=self.options.allowed_scms,

+                            session=self.session,

+                            by_config=self.options.allowed_scms_use_config,

+                            by_policy=self.options.allowed_scms_use_policy,

+                            policy_data={

+                                'user_id': self.taskinfo['owner'],

+                                'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                   strict=True)['name'],

+                                'scratch': opts.get('scratch')

+                            })

          repo_id = opts.get('repo_id')

          if not repo_id:

              raise koji.BuildError("A repo id must be provided")
@@ -6362,6 +6429,8 @@ 

                  'mock_bootstrap_image': False,

                  'pkgurl': None,

                  'allowed_scms': '',

+                 'allowed_scms_use_config': True,

+                 'allowed_scms_use_policy': False,

                  'scm_credentials_dir': None,

                  'support_rpm_source_layout': True,

                  'yum_proxy': None,
@@ -6390,7 +6459,7 @@ 

              elif name in ['offline_retry', 'use_createrepo_c', 'createrepo_skip_stat',

                            'createrepo_update', 'use_fast_upload', 'support_rpm_source_layout',

                            'build_arch_can_fail', 'no_ssl_verify', 'log_timestamps',

-                           'allow_noverifyssl']:

+                           'allow_noverifyssl', 'allowed_scms_use_config', 'allowed_scms_use_policy']:

                  defaults[name] = config.getboolean('kojid', name)

              elif name in ['plugin', 'plugins']:

                  defaults['plugin'] = value.split()

file modified
+8
@@ -77,6 +77,14 @@ 

  ; is run by default.

  allowed_scms=scm.example.com:/cvs/example git.example.org:/example svn.example.org:/users/*:no

  

+ ; If use the option allowed_scms above for allowing / denying SCM, default: true

+ ; allowed_scms_use_config = true

+ 

+ ; If use hub policy build_from_scm for allowing / denying SCM, default: false

+ ; notice that if both options are enabled, both assertions will be applied, and user_common and

+ ; source_cmd will be overridden by the policy's result.

+ ; allowed_scms_use_policy = false

+ 

  ; A directory to bind mount into Source RPM creation so that some

  ; credentials can be supplied when required to fetch sources, e.g.

  ; when the place the sources are fetched from requires all accesses to

@@ -15,6 +15,8 @@ 

  

  Details can be found at :ref:`auth-config`

  

+ .. _allowed-scms:

+ 

  Allowed SCMs

  ============

  
@@ -24,6 +26,13 @@ 

  

  Details of the ``allowed_scms`` option are covered under :ref:`scm-config`

  

+ We also provides ``build_from_scm`` hub policy for the same purpose, you can choose either/both

+ of the two approaches by the switch options in ``/etc/kojid.conf`` per build:

+ 

+     * ``allowed_scms_use_config``, default: ``true``

+     * ``allowed_scms_use_policy``, default: ``false``

+ 

+ For more details of the ``build_from_scm``, please read :doc:`defining_hub_policies`.

  

  Hub Policies

  ============
@@ -43,6 +52,7 @@ 

  * vm: control which windows build tasks are allowed

  * dist_repo: control which distRepo tasks are allowed

  * build_from_srpm: control whether builds from srpm are allowed

+ * build_from_scm: control whether builds from the SCM are allowed and the behavior of the SCM

  * build_from_repo_id: control whether builds from user-specified repos ids are allowed

  

  Note that not all policies are access control policies.

@@ -7,6 +7,8 @@ 

  

  * tag/untag/move operations

  * allowing builds from srpm

+ * allowing builds from SCM, and managing properties/behaviors related to the SCM

+   if it is allowed

  * allowing builds from expired repos

  * managing the package list for a tag

  * managing which channel a task goes to
@@ -19,6 +21,11 @@ 

  

  * tag/untag/move operations are governed by tag locks/permissions

  * builds from srpm are only allowed for admins

+ * builds from any SCM are only allowed for admins. It's used when

+   ``allowed_scms_use_policy`` is ``true`` in ``/etc/kojid.conf`` of the builders

+   (``false`` by default). And the SCM's properies: ``use_common`` and

+   ``source_cmd`` are set to their default values: ``False`` and

+   ``['make', 'source']``

  * builds from expired repos are only allowed for admins

  * only admins and users with ``tag`` permission may modify package lists

  * tasks go to the default channel
@@ -126,6 +133,7 @@ 

  * ``tag``: checked during tag/untag/move operations

  * ``build_from_srpm``: checked when a build from srpm (not an SCM reference) is

    requested.

+ * ``build_from_scm``: checked when a build task from SCM is executing on builder

  * ``build_from_repo_id``: checked when a build from a specified repo id is

    requested

  * ``package_list``: checked when the package list for a tag is modified
@@ -193,6 +201,23 @@ 

  ``adjust -<int>``

      * decrement default priority

  

+ The **build_from_scm** policy is used to assert if the SCM is allowed or not,

+ like the basic allow/deny one. It is also used to manage the SCM's properties as

+ the same as the ``allowed_scms`` option of the koji builder. The actions could

+ be defined as:

+ 

+ ``allow [use_common] [<source_cmd>]``

+     * allow the SCM

+     * use(clone) the /common repo when ``use_common`` follows ``allow``

+     * ``<source_cmd>`` is a *optional* shell command for preparing the source

+       between checkout and srpm build. If it is omitted, it will follow the

+       default value: ``make source``. The explicit value: ``none`` means **No**

+       ``source_cmd`` is defined.

+ 

+ ``deny [<reason>]``

+     * disallow the SCM

+     * ``<reason>`` is the error message which is shown as the task result

+ 

  Available tests

  ===============

  ``true``

@@ -1216,6 +1216,11 @@ 

  ``source_cmd``). In such case spec file named same as a checkout directory will

  be selected.

  

+ .. note::

+     We provide ``build_from_scm`` hub policy as an equivalent in version 1.26.0.

+ 

+     For more details, please refer to :ref:`allowed-scms` and

+     :doc:`Defining Hub Policies <defining_hub_policies>`.

  

  Add the host to the createrepo channel

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

@@ -460,12 +460,21 @@ 

  ``koji/daemon.py``. They take a specially formed URL as an argument to

  the constructor. Here's an example use. The second line is important, it

  makes sure the SCM is in the whitelist of SCMs allowed in

- ``/etc/kojid/kojid.conf``.

+ ``/etc/kojid/kojid.conf`` or in ``build_from_scm`` section of hub policy.

  

  ::

  

      scm = SCM(url)

-     scm.assert_allowed(self.options.allowed_scms)

+     scm.assert_allowed(allowed=self.options.allowed_scms,

+                        session=self.session,

+                        by_config=self.options.allowed_scms_use_config,

+                        by_policy=self.options.allowed_scms_use_policy,

+                        policy_data={

+                            'user_id': self.taskinfo['owner'],

+                            'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                               strict=True)['name'],

+                            'scratch': opts.get('scratch')

+                        }))

      directory = scm.checkout('/checkout/path', session, uploaddir, logfile)

  

  Checking out takes 4 arguments: where to checkout, a session object

file modified
+17 -4
@@ -10099,6 +10099,20 @@ 

      raise koji.ActionNotAllowed(err_str)

  

  

+ def eval_policy(name, data):

+     """Evaluate named policy with given data and return the result

+ 

+     :param str name: the policy name

+     :param dict data: the policy data

+     :returns the action as a string

+     :raises koji.GenericError if the policy is empty or not found

+     """

+     ruleset = context.policy.get(name)

+     if not ruleset:

+         raise koji.GenericError("no such policy: %s" % name)

+     return ruleset.apply(data)

+ 

+ 

  def policy_data_from_task(task_id):

      """Calculate policy data from task id

  
@@ -10653,6 +10667,8 @@ 

          q += """ ORDER BY id DESC LIMIT 1"""

          return _singleRow(q, values, fields, strict=True)

  

+     evalPolicy = staticmethod(eval_policy)

+ 

      def makeTask(self, *args, **opts):

          """Creates task manually. This is mainly for debugging, only an admin

          can make arbitrary tasks. You need to supply all *args and **opts
@@ -14622,10 +14638,7 @@ 

          """Evaluate named policy with given data and return the result"""

          host = Host()

          host.verify()

-         ruleset = context.policy.get(name)

-         if not ruleset:

-             raise koji.GenericError("no such policy: %s" % name)

-         return ruleset.apply(data)

+         return eval_policy(name, data)

  

      def newBuildRoot(self, repo, arch, task_id=None):

          host = Host()

file modified
+7
@@ -524,6 +524,13 @@ 

              has_perm admin :: allow

              all :: deny

              ''',

+     'build_from_scm': '''

+             has_perm admin :: allow

+             # match scm_type CVS CVS+SSH && match scm_host scm.example.com && match scm_repository /cvs/example :: allow

+             # match scm_type GIT GIT+SSH && match scm_host git.example.org && match scm_repository /example :: allow

+             # match scm_type SVN SVN+SSH && match scm_host svn.example.org && match scm_repository /users/* :: allow

+             all :: deny

+             ''',  # noqa: E501

      'package_list': '''

              has_perm admin :: allow

              has_perm tag :: allow

file modified
+100 -1
@@ -26,6 +26,7 @@ 

  import hashlib

  import logging

  import os

+ import re

  import signal

  import subprocess

  import sys
@@ -327,7 +328,36 @@ 

          # return parsed values

          return (scheme, user, netloc, path, query, fragment)

  

-     def assert_allowed(self, allowed):

+     def assert_allowed(self, allowed='', session=None, by_config=True, by_policy=False,

+                        policy_data=None):

+         """

+         Check whether this scm is allowed and apply options by either/both approach below:

+ 

+             - "allowed_scms" from config file, see

+               :func:`~koji.daemon.SCM.assert_allowed_by_config`

+             - "build_from_scm" hub policy, see :func:`~koji.daemon.SCM.assert_allowed_by_policy`

+ 

+         When both approaches are applied, the config one will be applied, then the policy one.

+ 

+         :param str allowed: The allowed_scms config content which is used for by-config approach

+         :param koji.ClientSession session: The allowed_scms config content which is used for

+                                            by-policy approach

+         :param bool by_config: Using config or not, Default: True

+         :param bool by_policy: Using policy or not, Default: False

+         :param dict policy_data: The policy data which will be merged with the generated scm info,

+                                  then will be passed to hub call for policy assertion when using

+                                  policy.

+         :raises koji.BuildError: if the scm is denied.

+         """

+         if by_config:

+             self.assert_allowed_by_config(allowed or '')

+         if by_policy:

+             if session is None:

+                 raise koji.ConfigurationError(

+                     'When allowed SCM assertion is by policy, session must be passed in.')

+             self.assert_allowed_by_policy(session, **(policy_data or {}))

+ 

+     def assert_allowed_by_config(self, allowed):

          """

          Check this scm against allowed list and apply options

  
@@ -396,6 +426,75 @@ 

              raise koji.BuildError(

                  '%s:%s is not in the list of allowed SCMs' % (self.host, self.repository))

  

+     def assert_allowed_by_policy(self, session, **extra_data):

+         """

+         Check this scm against hub policy: build_from_scm and apply options

+ 

+         The policy data is the combination of scminfo with scm_ prefix and kwargs.

+         It should at least contain following keys:

+ 

+             - scm_url

+             - scm_scheme

+             - scm_user

+             - scm_host

+             - scm_repository

+             - scm_module

+             - scm_revision

+             - scm_type

+ 

+         More keys could be added as kwargs(extra_data). You can pass any reasonable data which

+         could be handled by policy tests, like:

+ 

+             - scratch (if the task is scratch)

+             - channel (which channel the task is assigned)

+             - user_id (the task owner)

+ 

+         If the key in extra_data is one of scm_* listed above, it will override the one generated

+         from scminfo.

+ 

+         The format of the action returned from build_from_scm could be one of following forms::

+ 

+             allow [use_common] [<source_cmd>]

+             deny [<reason>]

+ 

+         If use_common is not set, use_common property is False.

+         If source_cmd is none, it will be parsed as None. If it not set, the default value:

+         ['make', 'sources'], or the value set by :func:`~koji.daemon.SCM.assert_allowed_by_config`

+         will be set.

+ 

+         Policy example:

+ 

+             build_from_scm =

+                 bool scratch :: allow none

+                 match scm_host scm.example.com :: allow use_common make sources

+                 match scm_host scm2.example.com :: allow

+                 all :: deny

+ 

+ 

+         :param koji.ClientSession session: the session object to call hub xmlrpc APIs.

+                                            It should be a host session.

+ 

+         :raises koji.BuildError: if the scm is denied.

+         """

+         policy_data = {}

+         for k, v in six.iteritems(self.get_info()):

+             policy_data[re.sub(r'^(scm_?)?', 'scm_', k)] = v

+         policy_data.update(extra_data)

+         result = (session.evalPolicy('build_from_scm', policy_data) or '').split()

+         is_allowed = result and result[0].lower() in ('yes', 'true', 'allow', 'allowed')

+         if not is_allowed:

+             raise koji.BuildError(

+                 'SCM: %s:%s is not allowed, reason: %s' % (self.host, self.repository,

+                                                            ' '.join(result[1:]) or None))

+         # Apply options when it's allowed

+         applied = result[1:]

+         self.use_common = len(applied) != 0 and applied[0].lower() == 'use_common'

+         idx = 1 if self.use_common else 0

+         self.source_cmd = applied[idx:] or self.source_cmd

+         if self.source_cmd is not None and len(self.source_cmd) > 0 \

+            and self.source_cmd[0].lower() == 'none':

+             self.source_cmd = None

+ 

      def checkout(self, scmdir, session=None, uploadpath=None, logfile=None):

          """

          Checkout the module from SCM.  Accepts the following parameters:

file modified
+12 -6
@@ -312,6 +312,7 @@ 

          self.workdir = workdir

          self.logger = logging.getLogger("koji.build.BaseTaskHandler")

          self.manager = None

+         self.taskinfo = None

  

      def setManager(self, manager):

          """Set the manager attribute
@@ -597,15 +598,20 @@ 

  

      def run_callbacks(self, plugin, *args, **kwargs):

          if 'taskinfo' not in kwargs:

-             try:

-                 taskinfo = self.taskinfo

-             except AttributeError:

-                 self.taskinfo = self.session.getTaskInfo(self.id, request=True)

-                 taskinfo = self.taskinfo

-             kwargs['taskinfo'] = taskinfo

+             kwargs['taskinfo'] = self.taskinfo

          kwargs['session'] = self.session

          koji.plugin.run_callbacks(plugin, *args, **kwargs)

  

+     @property

+     def taskinfo(self):

+         if not getattr(self, '_taskinfo', None):

+             self._taskinfo = self.session.getTaskInfo(self.id, request=True, strict=True)

+         return self._taskinfo

+ 

+     @taskinfo.setter

+     def taskinfo(self, taskinfo):

+         self._taskinfo = taskinfo

+ 

  

  class FakeTask(BaseTaskHandler):

      Methods = ['someMethod']

file modified
+223 -3
@@ -8,9 +8,40 @@ 

  

  import koji

  import koji.daemon

+ import koji.policy

  

  from koji.daemon import SCM

  

+ policy = {

+     'one': '''

+         match scm_host goodserver :: allow none

+         match scm_host badserver :: deny

+         match scm_host maybeserver && match scm_repository /badpath/* :: deny

+         all :: allow

+     ''',

+     'two': '''

+         match scm_host default :: allow

+         match scm_host nocommon :: allow

+         match scm_host common :: allow use_common

+         match scm_host srccmd :: allow fedpkg sources

+         match scm_host nosrc :: allow none

+         match scm_host mixed && match scm_repository /foo/* :: allow

+         match scm_host mixed && match scm_repository /bar/* :: allow use_common

+         match scm_host mixed && match scm_repository /baz/* :: allow fedpkg sources

+         match scm_host mixed && match scm_repository /foobar/* :: allow use_common fedpkg sources

+         match scm_host mixed && match scm_repository /foobaz/* :: allow use_common none

+     '''

+ }

+ 

+ class FakePolicy(object):

+ 

+     def __init__(self, rule):

+         base_tests = koji.policy.findSimpleTests(vars(koji.policy))

+         self.ruleset = koji.policy.SimpleRuleSet(rule.splitlines(), base_tests)

+ 

+     def evalPolicy(self, name, data):

+         return self.ruleset.apply(data)

+ 

  

  class TestSCM(unittest.TestCase):

  
@@ -67,8 +98,22 @@ 

          self.assertEqual(scm.source_cmd, ['make', 'sources'])

          self.assertEqual(scm.scmtype, 'GIT')

  

+     def test_assert_allowed_basic(self):

+         scm = SCM("git://scm.example.com/path1#1234")

+ 

+         # session must be passed

+         with self.assertRaises(koji.GenericError) as cm:

+             scm.assert_allowed(session=None, by_config=False, by_policy=True)

+         self.assertEqual(str(cm.exception),

+                          'When allowed SCM assertion is by policy, session must be passed in.')

+ 

+         # allowed could not be None

+         scm.assert_allowed_by_config = mock.MagicMock()

+         scm.assert_allowed(allowed=None, by_config=True, by_policy=False)

+         scm.assert_allowed_by_config.assert_called_once_with('')

+ 

      @mock.patch('logging.getLogger')

-     def test_allowed(self, getLogger):

+     def test_allowed_by_config(self, getLogger):

          config = '''

              goodserver:*:no

              !badserver:*
@@ -104,7 +149,7 @@ 

                  raise AssertionError("allowed bad url: %s" % url)

  

      @mock.patch('logging.getLogger')

-     def test_badrule(self, getLogger):

+     def test_badrule_by_config(self, getLogger):

          config = '''

              bogus-entry-should-be-ignored

              goodserver:*:no
@@ -115,7 +160,7 @@ 

          scm.assert_allowed(config)

  

      @mock.patch('logging.getLogger')

-     def test_opts(self, getLogger):

+     def test_opts_by_config(self, getLogger):

          config = '''

              default:*

              nocommon:*:no
@@ -196,6 +241,181 @@ 

          with self.assertRaises(koji.BuildError):

              scm.assert_allowed(config)

  

+     def test_allowed_by_policy(self):

+         good = [

+             "git://goodserver/path1#1234",

+             "git+ssh://maybeserver/path1#1234",

+             ]

+         bad = [

+             "cvs://badserver/projects/42#ref",

+             "svn://badserver/projects/42#ref",

+             "git://maybeserver/badpath/project#1234",

+             "git://maybeserver//badpath/project#1234",

+             "git://maybeserver////badpath/project#1234",

+             "git://maybeserver/./badpath/project#1234",

+             "git://maybeserver//.//badpath/project#1234",

+             "git://maybeserver/goodpath/../badpath/project#1234",

+             "git://maybeserver/goodpath/..//badpath/project#1234",

+             "git://maybeserver/..//badpath/project#1234",

+             ]

+         session = mock.MagicMock()

+         session.evalPolicy.side_effect = FakePolicy(policy['one']).evalPolicy

+         for url in good:

+             scm = SCM(url)

+             scm.assert_allowed(session=session, by_config=False, by_policy=True)

+         for url in bad:

+             scm = SCM(url)

+             with self.assertRaises(koji.BuildError) as cm:

+                 scm.assert_allowed(session=session, by_config=False, by_policy=True)

+             self.assertRegexpMatches(str(cm.exception), '^SCM: .* is not allowed, reason: None$')

+ 

+     def test_opts_by_policy(self):

+         session = mock.MagicMock()

+         session.evalPolicy.side_effect = FakePolicy(policy['two']).evalPolicy

+ 

+         url = "git://default/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://nocommon/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://common/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://srccmd/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://nosrc/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, None)

+ 

+         url = "git://mixed/foo/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/bar/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/baz/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://mixed/koji.git#1234"

+         scm = SCM(url)

+         with self.assertRaises(koji.BuildError):

+             scm.assert_allowed_by_policy(session=session)

+ 

+         url = "git://mixed/foo/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/bar/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/baz/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://mixed/foobar/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://mixed/foobaz/koji.git#1234"

+         scm = SCM(url)

+         scm.assert_allowed_by_policy(session=session)

+         self.assertEqual(scm.use_common, True)

+         self.assertIsNone(scm.source_cmd)

+ 

+         url = "git://nomatch/koji.git#1234"

+         scm = SCM(url)

+         with self.assertRaises(koji.BuildError):

+             scm.assert_allowed_by_policy(session=session)

+ 

+     def test_assert_allowed_by_both(self):

+         config = '''

+             default:*:no:

+             mixed:/foo/*:yes

+             mixed:/bar/*:no

+             mixed:/baz/*:no:centpkg,sources

+             mixed:/foobar/*:no:

+             mixed:/foobaz/*:no:centpkg,sources

+             '''

+ 

+         session = mock.MagicMock()

+         session.evalPolicy.side_effect = FakePolicy(policy['two']).evalPolicy

+ 

+         url = "git://default/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host default :: allow

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, False)

+         self.assertIsNone(scm.source_cmd)

+ 

+         url = "git://mixed/foo/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host mixed && match scm_repository /foo/* :: allow

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/bar/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host mixed && match scm_repository /bar/* :: allow use_common

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['make', 'sources'])

+ 

+         url = "git://mixed/baz/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host mixed && match scm_repository /baz/* :: allow fedpkg sources

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, False)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://mixed/foobar/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host mixed && match scm_repository /foobar/* :: allow use_common fedpkg sources

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, True)

+         self.assertEqual(scm.source_cmd, ['fedpkg', 'sources'])

+ 

+         url = "git://mixed/foobaz/koji.git#1234"

+         scm = SCM(url)

+         # match scm_host mixed && match scm_repository /foobaz/* :: allow use_common none

+         scm.assert_allowed(allowed=config, session=session, by_config=True, by_policy=True)

+         self.assertEqual(scm.use_common, True)

+         self.assertIsNone(scm.source_cmd)

+ 

  

  class TestSCMCheckouts(unittest.TestCase):

  

file modified
+14 -3
@@ -138,6 +138,8 @@ 

                  'offline_retry': True,

                  'offline_retry_interval': 120,

                  'allowed_scms': '',

+                 'allowed_scms_by_config': True,

+                 'allowed_scms_by_policy': False,

                  'cert': None,

                  'serverca': None}

      if config.has_section('kojivmd'):
@@ -149,7 +151,8 @@ 

                      defaults[name] = int(value)

                  except ValueError:

                      quit("value for %s option must be a valid integer" % name)

-             elif name in ['offline_retry', 'no_ssl_verify']:

+             elif name in ['offline_retry', 'no_ssl_verify', 'allowed_scms_by_config',

+                           'allowed_scms_by_policy']:

                  defaults[name] = config.getboolean('kojivmd', name)

              elif name in ['plugin', 'plugins']:

                  defaults['plugin'] = value.split()
@@ -325,8 +328,16 @@ 

          # verify the urls before passing them to the VM

          for url in [source_url] + koji.util.to_list(subopts.values()):

              scm = SCM(url)

-             scm.assert_allowed(self.options.allowed_scms)

- 

+             scm.assert_allowed(allowed=self.options.allowed_scms,

+                                session=self.session,

+                                by_config=self.options.allowed_scms_use_config,

+                                by_policy=self.options.allowed_scms_use_policy,

+                                policy_data={

+                                    'user_id': self.taskinfo['owner'],

+                                    'channel': self.session.getChannel(self.taskinfo['channel_id'],

+                                                                       strict=True)['name'],

+                                    'scratch': opts.get('scratch')

+                                })

          task_info = self.session.getTaskInfo(self.id)

          target_info = self.session.getBuildTarget(target)

          if not target_info:

file modified
+8
@@ -27,6 +27,14 @@ 

  ; dir, and will raise an exception if it cannot.

  allowed_scms=scm.example.com:/cvs/example git.example.org:/example svn.example.org:/users/*:no

  

+ ; If use the option allowed_scms above for allowing / denying SCM, default: true

+ ; allowed_scms_use_config = true

+ 

+ ; If use hub policy: build_from_scm for allowing / denying SCM, default: false

+ ; notice that if both options are enabled, both assertions will be applied, and user_common

+ ; will be overridden by the policy's result.

+ ; allowed_scms_use_policy = false

+ 

  ; The mail host to use for sending email notifications

  smtphost=example.com

  

This is a simple extention of SCM.assert_allowed

  • assert_allowed_by_policy will set the default "use_common" to False which is different to the old behavior
  • channel, user_id, scratch are passed in the policy_data with scminfo right now.

This is a prototype for this change, and there are some other solutions could be implemented too

  • Use a scmpolicy plugin in postSCMCheckout callback. The pro is that we can do more checks after the source is initialized on builder, meanwhile, the con is that the source will be downloaded even it is denied by policy. It might be a potential risk?
  • Do the scm check in hub's make_task, this looks straightforward, but may lack some builder's information

fixes: #2757

rebased onto 5288730f7ffbfb7b071d11e62bc3083f52ac80a5

10 months ago

rebased onto 8d720585b663190db405156f683764406e23e80e

10 months ago

rebased onto 840c8fc4483ffd8c2f09f382040af63b4104848b

10 months ago

I really like this approach!

Use a scmpolicy plugin in postSCMCheckout callback. The pro is that we can do more checks after the source is initialized on builder, meanwhile, the con is that the source will be downloaded even it is denied by policy. It might be a potential risk?

In some cases (hello blender repo in fedora), the cloning itself can take hours and timeout so I'd prefer to not do it that way (+ you know, those vulnerabilities here and there).

Do the scm check in hub's make_task, this looks straightforward, but may lack some builder's information

Do you have some hint which one, for example?

Do the scm check in hub's make_task, this looks straightforward, but may lack some builder's information

Do you have some hint which one, for example?

Like which host the task is assigned to. This doesn't happen yet in make_task() 1. BTW, this info is not used by the current approach too.

And, we have to guess which argument is the source, especially for custom tasks.

What do you see as a benefit of checking it in make_task? Is it just that builder is making additional call here?

What do you see as a benefit of checking it in make_task? Is it just that builder is making additional call here?

And a faster response if the source is denied (no need to create a SCM-related task on the builder).

I would leave it in srpm task then. make_task is generic, so we would need to detect if it is some type of task involving SCM. For plugin tasks it would be unusable, as plugin will need to run the check anyway.

This is tricky and I think it might take a few iterations to get right.

I'm not sure I like the idea of working this into the existing assert_allowed call. It doesn't really get us much benefit as it still requires all invocations of the call to have significant updates to take advantage.

OTOH, maybe this is ok for now.

A couple things to consider:

  • What we create here is likely to eventually replace the existing allowed_scms setting, so we should design it with that in mind
  • Other tools will want to use this. Builder plugins, content generators, possibly more. We should make it as straightforward as possible. In particular, an external cg will not be able to use host.evalPolicy, so we might consider adding a hub call for this

When flattening the scm info, I think it would be better to use scm_ as the prefix.

The opts parameter is only used to update the policy_data, so it seems to be misnamed. It is not a general options parameter, but explicitly policy data info.

I'm not sure I like that the default policy doesn't work out of the box, but maybe that is ok? I guess our default allowed_scms='' also blocks builds out of the box.

rebased onto c5de1364a5a6a6d60e53d59a57246d2797a06268

9 months ago

@mikem, updated.

I'm not sure I like that the default policy doesn't work out of the box, but maybe that is ok? I guess our default allowed_scms='' also blocks builds out of the box.

The default policy only allows admin's builds. And although the default allowed_scms='' blocks all builds, the allowed_scms defined in the default kojid.conf contains some dummy items. That's almost equal to "all denied". So I believe it is ok in most cases.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

9 months ago

pretty please pagure-ci rebuild

9 months ago

pretty please pagure-ci rebuild

9 months ago

pretty please pagure-ci rebuild

9 months ago

rebased onto 47c4b5d

9 months ago

Functionality is tested, I add testing-done tag. @julian8628 please, update policy hub documentation related to these changes.

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

9 months ago

1 new commit added

  • [doc][defining_hub_policies] update the doc
9 months ago

7 new commits added

  • [doc][defining_hub_policies] update the doc
  • fix config typo
  • [hub] add non-host evalPolicy API
  • use scm_ as the prefix instead of scm for scminfo
  • update doc
  • more reasonable parameter name, and more doc strs
  • kojid: extend SCM.assert_allowed with hub policy
9 months ago

7 new commits added

  • [doc][defining_hub_policies] update the doc
  • fix config typo
  • [hub] add non-host evalPolicy API
  • use scm_ as the prefix instead of scm for scminfo
  • update doc
  • more reasonable parameter name, and more doc strs
  • kojid: extend SCM.assert_allowed with hub policy
9 months ago

Commit 6d4abed fixes this pull-request

Pull-Request has been merged by tkopecek

9 months ago