#421 Extend allowed_scms format to allow explicit blocks
Merged 5 years ago by mikem. Opened 5 years ago by mikem.
mikem/koji scm-deny  into  master

file modified
+30 -13
@@ -1104,29 +1104,46 @@ 

      Kojid will not attempt kerberos authentication to the koji-hub unless the

      username field is commented out

  

- Optional Configuration SourceCodeManagement

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

+ Source Control Configuration

+ ----------------------------

  

  /etc/kojid/kojid.conf

  ^^^^^^^^^^^^^^^^^^^^^

  

- The pattern is as follows:

-     hostname:/path/match:checkout /common?, hostname2:/path/match:checkout /common?

- 

- ``checkout /common? is 'true' unless set to false``

+ The *allowed_scms* setting controls which source control systems the builder

+ will accept. It is a space-separated list of entries in one of the following

+ forms:

  

  ::

  

-     allowed_scms=scm-server.example.com:/repo/base/repos*/:false

+     hostname:path[:use_common[:source_cmd]]

+     !hostname:path

+ 

+ where

+ 

+     *hostname* is a glob pattern matched against SCM hosts.

+ 

+     *path* is a glob pattern matched against the SCM path.

+ 

+     *use_common* is boolean setting (yes/no, on/off, true/false) that indicates

+     whether koji should also check out /common from the SCM host. The default

+     is on.

+ 

+     *source_cmd* is a shell command to be run before building the

+     srpm, with commas instead of spaces. It defaults to ``make,sources``.

+ 

+ The second form (``!hostname:path``) is used to explicitly block a host:path

+ pattern. In particular, it provides the option to block specific subtrees of

+ a host, but allow from it otherwise

  

- Once the code is checked out kojid will run the following:

  

  ::

  

-     make sources

-     mv *.spec <rpmbuild>/SPECS

-     mv * <rpmbuild>/SOURCES

-     rpmbuild -bs <rpmbuild>/SPECS/*.spec

+     allowed_scms=

+         !scm-server.example.com:/blocked/path/*

+         scm-server.example.com:/repo/base/repos*/:no

+         alt-server.example.com:/repo/dist/repos*/:no:fedpkg,sources

+ 

  

  Add the host to the createrepo channel

  --------------------------------------
@@ -1174,7 +1191,7 @@ 

  clicking on the hosts tab. This will show you a list of builders in the

  database and the status of each builder.

  

- Kojira - Dnd|Yum repository creation and maintenance

+ Kojira - Dnf|Yum repository creation and maintenance

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

  

  Configuration Files

file modified
+58 -29
@@ -225,7 +225,7 @@ 

              if self.scheme in schemes:

                  self.scmtype = scmtype

                  break

-         else:

+         else:   # pragma: no cover

              # should never happen

              raise koji.GenericError('Invalid SCM URL: %s' % url)

  
@@ -267,7 +267,8 @@ 

          # check for validity: params should be empty, query may be empty, everything else should be populated

          if params:

              raise koji.GenericError('Unable to parse SCM URL: %s . Params element %s should be empty.' % (self.url, params))

-         if not scheme:

+         if not scheme:  #pragma: no cover

+             # should not happen because of is_scm_url check earlier

              raise koji.GenericError('Unable to parse SCM URL: %s . Could not find the scheme element.' % self.url)

          if not netloc:

              raise koji.GenericError('Unable to parse SCM URL: %s . Could not find the netloc element.' % self.url)
@@ -281,40 +282,68 @@ 

  

      def assert_allowed(self, allowed):

          """

-         Verify that the host and repository of this SCM is in the provided list of

-         allowed repositories.

+         Check this scm against allowed list and apply options

+ 

+         allowed is a space-separated list of entries in one of the following

+         forms:

+ 

+             host:repository[:use_common[:source_cmd]]

+             !host:repository

+ 

+         Incorrectly-formatted entries will be skipped with a warning.

+ 

+         The first form allows a host:repository pattern and optionally sets a

+         few options for it.

+ 

+         The second form explicitly blocks a host:repository pattern

+ 

+         Both host and repository are treated as glob patterns

+ 

+         If there is a matching entry, then the optional fields, if given, will

+         be applied to the instance.

  

-         allowed is a space-separated list of host:repository[:use_common[:source_cmd]] tuples.  Incorrectly-formatted

-         tuples will be ignored.

+         If there is no matching entry, or if the host:repository is blocked

+         then BuildError is raised.

  

-         If use_common is not present, kojid will attempt to checkout a common/ directory from the

-         repository.  If use_common is set to no, off, false, or 0, it will not attempt to checkout a common/

-         directory.

+         The use_common option defaults to on.  If it is set to no, off, false

+         or 0, it will be disabled.  If the option is on, then kojid will

+         attempt to checkout a common/ directory from the repository.

  

-         source_cmd is a shell command (args separated with commas instead of spaces) to run before building the srpm.

-         It is generally used to retrieve source files from a remote location.  If no source_cmd is specified,

-         "make sources" is run by default.

+         The source_command is a shell command to be run before building the

+         srpm.  It defaults to "make sources".  This can be overridden by the

+         matching allowed entry.  The command must be encoded with commas

+         instead of spaces (e.g. "make,sources").

          """

+         is_allowed = False

          for allowed_scm in allowed.split():

              scm_tuple = allowed_scm.split(':')

-             if len(scm_tuple) >= 2:

-                 if fnmatch(self.host, scm_tuple[0]) and fnmatch(self.repository, scm_tuple[1]):

-                     # SCM host:repository is in the allowed list

-                     # check if we specify a value for use_common

-                     if len(scm_tuple) >= 3:

-                         if scm_tuple[2].lower() in ('no', 'off', 'false', '0'):

-                             self.use_common = False

-                     # check if we specify a custom source_cmd

-                     if len(scm_tuple) >= 4:

-                         if scm_tuple[3]:

-                             self.source_cmd = scm_tuple[3].split(',')

-                         else:

-                             # there was nothing after the trailing :, so they don't want to run a source_cmd at all

-                             self.source_cmd = None

-                     break

-             else:

+             if len(scm_tuple) < 2:

                  self.logger.warn('Ignoring incorrectly formatted SCM host:repository: %s' % allowed_scm)

-         else:

+                 continue

+             host_pat = scm_tuple[0]

+             repo_pat = scm_tuple[1]

+             invert = False

+             if host_pat.startswith('!'):

+                 invert = True

+                 host_pat = host_pat[1:]

+             if fnmatch(self.host, host_pat) and fnmatch(self.repository, repo_pat):

+                 # match

+                 if invert:

+                     break

+                 is_allowed = True

+                 # check if we specify a value for use_common

+                 if len(scm_tuple) >= 3:

+                     if scm_tuple[2].lower() in ('no', 'off', 'false', '0'):

+                         self.use_common = False

+                 # check if we specify a custom source_cmd

+                 if len(scm_tuple) >= 4:

+                     if scm_tuple[3]:

+                         self.source_cmd = scm_tuple[3].split(',')

+                     else:

+                         # there was nothing after the trailing :, so they don't want to run a source_cmd at all

+                         self.source_cmd = None

+                 break

+         if not is_allowed:

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

  

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

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

+ import mock

+ import unittest

+ 

+ import logging

+ import shutil

+ import tempfile

+ 

+ from pprint import pprint

+ 

+ import koji

+ import koji.daemon

+ 

+ from koji.daemon import SCM

+ 

+ 

+ class TestSCM(unittest.TestCase):

+ 

+     def test_urlcheck(self):

+         good = [

+             "git://server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "git+ssh://server2/other/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "svn://server/path/to/code#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "svn+ssh://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "cvs://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "cvs+ssh://server/some/path#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             ]

+         bad = [

+             "http://localhost/foo.html",

+             "foo-1.1-1.src.rpm",

+             "https://server/foo-1.1-1.src.rpm",

+             ]

+         for url in good:

+             self.assertTrue(SCM.is_scm_url(url))

+         for url in bad:

+             self.assertFalse(SCM.is_scm_url(url))

+ 

+     @mock.patch('logging.getLogger')

+     def test_init(self, getLogger):

+         bad = [

+             "git://user@@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "git://user:pass@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67",

+             "git://server/foo.git;params=not_allowed",

+             "git://server#asdasd",  # no path

+             "git://server/foo.git",  # no fragment

+             "http://localhost/foo.html",

+             "git://@localhost/foo/?a=bar/",

+             "http://localhost/foo.html?a=foo/",

+             "foo-1.1-1.src.rpm",

+             "git://",

+             "https://server/foo-1.1-1.src.rpm",

+             ]

+         for url in bad:

+             with self.assertRaises(koji.GenericError):

+                 scm = SCM(url)

+ 

+         url = "git://user@server/foo.git#bab0c73900241ef5c465d7e873e9d8b34c948e67"

+         scm = SCM(url)

+         self.assertEqual(scm.scheme, 'git://')

+         self.assertEqual(scm.user, 'user')

+         self.assertEqual(scm.host, 'server')

+         self.assertEqual(scm.repository, '/foo.git')

+         self.assertEqual(scm.module, '')

+         self.assertEqual(scm.revision, 'bab0c73900241ef5c465d7e873e9d8b34c948e67')

+         self.assertEqual(scm.use_common, True)

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

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

+ 

+     @mock.patch('logging.getLogger')

+     def test_allowed(self, getLogger):

+         config = '''

+             goodserver:*:no

+             !badserver:*

+             !maybeserver:/badpath/*

+             maybeserver:*:no

+             '''

+         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",

+             ]

+         for url in good:

+             scm = SCM(url)

+             scm.assert_allowed(config)

+         for url in bad:

+             scm = SCM(url)

+             try:

+                 scm.assert_allowed(config)

+             except koji.BuildError:

+                 pass

+             else:

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

+ 

+     @mock.patch('logging.getLogger')

+     def test_badrule(self, getLogger):

+         config = '''

+             bogus-entry-should-be-ignored

+             goodserver:*:no

+             !badserver:*

+             '''

+         url = "git://goodserver/path1#1234"

+         scm = SCM(url)

+         scm.assert_allowed(config)

+ 

+     @mock.patch('logging.getLogger')

+     def test_opts(self, getLogger):

+         config = '''

+             default:*

+             nocommon:*:no

+             srccmd:*:no:fedpkg,sources

+             nosrc:*:no:

+             mixed:/foo/*:no

+             mixed:/bar/*:yes

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

+             '''

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(config)

+         self.assertEqual(scm.use_common, True)

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

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(config)

+         self.assertEqual(scm.use_common, False)

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

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(config)

+         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(config)

+         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(config)

+         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(config)

+         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(config)

+         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(config)

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(config)

+         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(config)

+         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(config)

+         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(config)

+ 

+ 

+ class TestSCMCheckouts(unittest.TestCase):

+ 

+     def setUp(self):

+         self.symlink = mock.patch('os.symlink').start()

+         self.getLogger = mock.patch('logging.getLogger').start()

+         self.log_output = mock.patch('koji.daemon.log_output').start()

+         self.log_output.return_value = None

+         self.tempdir = tempfile.mkdtemp()

+         self.session = mock.MagicMock()

+         self.uploadpath = mock.MagicMock()

+         self.logfile = mock.MagicMock()

+         self.config = '''

+             default:*

+             nocommon:*:no

+             srccmd:*:no:fedpkg,sources

+             nosrc:*:no:

+             '''

+ 

+     def tearDown(self):

+         mock.patch.stopall()

+         shutil.rmtree(self.tempdir)

+ 

+     def test_checkout_git_nocommon(self):

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['git', 'clone', '-n', 'git://nocommon/koji.git',

+                 self.tempdir + '/koji']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         cmd = ['git', 'reset', '--hard', 'asdasd']

+         call2 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir + '/koji',

+                         logerror=1, append=True, env=None)

+         self.log_output.assert_has_calls([call1, call2])

+ 

+     def test_checkout_gitssh_nocommon(self):

+ 

+         url = "git+ssh://user@nocommon/koji.git#asdasd"

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['git', 'clone', '-n', 'git+ssh://user@nocommon/koji.git',

+                 self.tempdir + '/koji']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         cmd = ['git', 'reset', '--hard', 'asdasd']

+         call2 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir + '/koji',

+                         logerror=1, append=True, env=None)

+         self.log_output.assert_has_calls([call1, call2])

+ 

+     def test_checkout_git_common(self):

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, True)

+         self.symlink.assert_called_once()

+         # expected commands

+         cmd = ['git', 'clone', '-n', 'git://default/koji.git',

+                 self.tempdir + '/koji']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         cmd = ['git', 'reset', '--hard', 'asdasd']

+         call2 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir + '/koji',

+                         logerror=1, append=True, env=None)

+         cmd = ['git', 'clone', 'git://default/common.git', 'common']

+         call3 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir,

+                         logerror=1, append=True, env=None)

+         self.log_output.assert_has_calls([call1, call2, call3])

+ 

+     def test_checkout_error_in_command(self):

+ 

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

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         self.log_output.return_value = 1

+         with self.assertRaises(koji.BuildError):

+             scm.checkout(self.tempdir, session=self.session,

+                     uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['git', 'clone', '-n', 'git://nocommon/koji.git',

+                 self.tempdir + '/koji']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         # should have errored after first command

+         self.log_output.assert_has_calls([call1])

+ 

+     def test_checkout_cvs_common(self):

+ 

+         url = "cvs://default/cvsisdead?rpms/foo/EL3#sometag"

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, True)

+         self.symlink.assert_called_once()

+         # expected commands

+         cmd = ['cvs', '-d', ':pserver:anonymous@default:/cvsisdead', 'checkout',

+                 '-r', 'sometag', 'rpms/foo/EL3']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         cmd = ['cvs', '-d', ':pserver:anonymous@default:/cvsisdead', 'checkout',

+                 'common']

+         call2 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=True, env=None)

+         self.log_output.assert_has_calls([call1, call2])

+ 

+     def test_checkout_cvs_ssh(self):

+ 

+         url = "cvs+ssh://user@nocommon/cvsisdead?rpms/foo/EL3#sometag"

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['cvs', '-d', ':ext:user@nocommon:/cvsisdead', 'checkout', '-r',

+                 'sometag', 'rpms/foo/EL3']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env={'CVS_RSH': 'ssh'})

+         self.log_output.assert_has_calls([call1])

+ 

+     def test_checkout_svn(self):

+ 

+         url = "svn://nocommon/dist?rpms/foo/EL3#revision"

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['svn', 'checkout', '-r', 'revision',

+                 'svn://nocommon/dist/rpms/foo/EL3', 'rpms/foo/EL3']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         self.log_output.assert_has_calls([call1])

+ 

+     def test_checkout_svn_ssh(self):

+ 

+         url = "svn+ssh://user@nocommon/dist?rpms/foo/EL3#revision"

+         scm = SCM(url)

+         scm.assert_allowed(self.config)

+         scm.checkout(self.tempdir, session=self.session,

+                 uploadpath=self.uploadpath, logfile=self.logfile)

+         self.assertEqual(scm.use_common, False)

+         self.symlink.assert_not_called()

+         # expected commands

+         cmd = ['svn', 'checkout', '-r', 'revision',

+                 'svn+ssh://user@nocommon/dist/rpms/foo/EL3', 'rpms/foo/EL3']

+         call1 = mock.call(self.session, cmd[0], cmd, self.logfile,

+                         self.uploadpath, cwd=self.tempdir, logerror=1,

+                         append=False, env=None)

+         self.log_output.assert_has_calls([call1])

See the updated docstring for details. This change adds the ability to have entries like:

!host:repository

Which would explicitly block a host:repository pattern. This allows for somewhat more robust rules. In particular, it provides the option
to block specific subtrees of a host, but allow from it otherwise. E.g.

!my_pagure:forks/*
my_pagure:*:no

also included: unit tests for the SCM class

@pingou this should cover that one issue we were discussing

Should we warn so admins can see in the logs that something is wrong in the config?

Nevermind, we're already warning :)

If you want to follow pep8 you'll need to remove one line here :)

Completely nitpicking, but confused me for about half a second, maybe we could name the variable "configuration" rather than "allowed".

Couple of nitpicks but nothing major or blocking for me.

:ship: it!

1 new commit added

  • rename variable in unit test. clarify docstring
5 years ago

The variable in the tests was named to match the parameter in the call, but I suppose config is more accurate. I'd change it there too, but I'd rather not change a function signature in the library (apart from extension) for something like this.

Does it make sense to mention it in docs in addition to code?

1 new commit added

  • update docs for allowed_scms
5 years ago

rebased

5 years ago

Pull-Request has been merged by mikem

5 years ago

Does this also let us filter based on branches?

Does this also let us filter based on branches?

No, this check only considers the path. The revision is in the url fragment, and interpreting that correctly is much more complicated.