PR#2951: kojid: extend SCM.assert_allowed with hub policy - koji

		`@@ -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()`

builder/kojid.conf

file modified

		`@@ -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`

docs/source/access_controls.rst

file modified

+10

		`@@ -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.`

docs/source/defining_hub_policies.rst

file modified

+25

		`@@ -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``

docs/source/server_howto.rst

file modified

		`@@ -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`
		`--------------------------------------`

docs/source/writing_koji_code.rst

file modified

+11 -2

		`@@ -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`

hub/kojihub.py

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()`

hub/kojixmlrpc.py

file modified

		`@@ -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`

koji/daemon.py

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:`

koji/tasks.py

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']`

tests/test_scm.py

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):`

vm/kojivmd

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:`

vm/kojivmd.conf

file modified

		`@@ -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`

julian8628 commented 10 months ago

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

ignatenkobrain commented 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?

julian8628 commented 10 months ago

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.

tkopecek commented 9 months ago

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

julian8628 commented 9 months ago

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).

tkopecek commented 9 months ago

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.

mikem commented 9 months ago

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

Edited 9 months ago by mikem

mikem commented 9 months ago

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

julian8628 commented 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.