[Kimchi-devel] [PATCH v2 1/3] Issue #377: Validate repository URLs

Aline Manera alinefm at linux.vnet.ibm.com
Wed Aug 6 18:21:05 UTC 2014


On 08/04/2014 04:41 PM, Crístian Viana wrote:
> The repository URLs have a specific syntax and should point to a valid
> destination. Currently, Kimchi does not check those constraints before
> creating and updating a repository.
>
> Add URL validation before creating and updating a repository. The URLs
> must have one of the valid protocols (i.e. http://, https://, ftp://,
> file://) and the URL address must point to a valid destination. That
> means that if the URL protocol is file://, it should point to an
> existing file on the system, otherwise it should point to an available
> remote location.
>
> Signed-off-by: Crístian Viana <vianac at linux.vnet.ibm.com>
> ---
>   src/kimchi/mockmodel.py    | 25 ++++++++++++++++++++++---
>   src/kimchi/repositories.py | 11 +++++++++++
>   src/kimchi/utils.py        | 15 +++++++++++++++
>   tests/test_model.py        | 42 ++++++++++++++++++++++++++++++++++++++++--
>   tests/test_rest.py         | 39 ++++++++++++++++++++++++++++++++++++++-
>   5 files changed, 126 insertions(+), 6 deletions(-)
>
> diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py
> index a42f2dd..4f267bb 100644
> --- a/src/kimchi/mockmodel.py
> +++ b/src/kimchi/mockmodel.py
> @@ -52,7 +52,7 @@ from kimchi.model.storageservers import STORAGE_SERVERS
>   from kimchi.model.utils import get_vm_name
>   from kimchi.objectstore import ObjectStore
>   from kimchi.screenshot import VMScreenshot
> -from kimchi.utils import pool_name_from_uri
> +from kimchi.utils import pool_name_from_uri, validate_repo_url
>   from kimchi.utils import template_name_from_uri
>   from kimchi.vmtemplate import VMTemplate
>   
> @@ -1241,13 +1241,22 @@ class MockRepositories(object):
>           # Create and enable the repository
>           repo_id = params['repo_id']
>           config = params.get('config', {})
> +        baseurl = params.get('baseurl')
> +        mirrorlist = config.get('mirrorlist', "")
> +
> +        if baseurl:
> +            validate_repo_url(baseurl)
> +
> +        if mirrorlist:
> +            validate_repo_url(mirrorlist)
> +
>           repo = {'repo_id': repo_id,
> -                'baseurl': params.get('baseurl'),
> +                'baseurl': baseurl,
>                   'enabled': True,
>                   'config': {'repo_name': config.get('repo_name', repo_id),
>                              'gpgkey': config.get('gpgkey', []),
>                              'gpgcheck': True,
> -                           'mirrorlist': config.get('mirrorlist', "")}
> +                           'mirrorlist': mirrorlist}
>                   }
>   
>           self._repos[repo_id] = repo
> @@ -1292,6 +1301,16 @@ class MockRepositories(object):
>           if repo_id not in self._repos.keys():
>               raise NotFoundError("KCHREPOS0012E", {'repo_id': repo_id})
>   
> +        baseurl = params.get('baseurl', None)
> +        config = params.get('config', {})
> +        mirrorlist = config.get('mirrorlist', None)
> +
> +        if baseurl:
> +            validate_repo_url(baseurl)
> +
> +        if mirrorlist:
> +            validate_repo_url(mirrorlist)
> +
>           info = self._repos[repo_id]
>           info.update(params)
>           del self._repos[repo_id]
> diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py
> index 414f146..5a0c58a 100644
> --- a/src/kimchi/repositories.py
> +++ b/src/kimchi/repositories.py
> @@ -27,6 +27,7 @@ from kimchi.basemodel import Singleton
>   from kimchi.config import kimchiLock
>   from kimchi.exception import InvalidOperation
>   from kimchi.exception import OperationFailed, NotFoundError, MissingParameter
> +from kimchi.utils import validate_repo_url
>   
>   
>   class Repositories(object):
> @@ -177,6 +178,12 @@ class YumRepo(object):
>           if not baseurl and not mirrorlist:
>               raise MissingParameter("KCHREPOS0013E")
>   
> +        if baseurl:
> +            validate_repo_url(baseurl)
> +
> +        if mirrorlist:
> +            validate_repo_url(mirrorlist)
> +
>           repo_id = params.get('repo_id', None)
>           if repo_id is None:
>               repo_id = "kimchi_repo_%s" % str(int(time.time() * 1000))
> @@ -260,12 +267,14 @@ class YumRepo(object):
>           mirrorlist = config.get('mirrorlist', None)
>   
>           if baseurl is not None:
> +            validate_repo_url(baseurl)
>               entry.baseurl = baseurl
>   
>           if mirrorlist == '':
>               mirrorlist = None
>   
>           if mirrorlist is not None:
> +            validate_repo_url(mirrorlist)
>               entry.mirrorlist = mirrorlist
>   
>           entry.id = params.get('repo_id', repo_id)
> @@ -414,6 +423,8 @@ class AptRepo(object):
>           dist = config['dist']
>           comps = config.get('comps', [])
>   

> +        _validate_repo_url(uri)
> +

I think this function does not exist anymore, right?

>           kimchiLock.acquire()
>           try:
>               repos = self._get_repos()
> diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py
> index c070c00..ad458ac 100644
> --- a/src/kimchi/utils.py
> +++ b/src/kimchi/utils.py
> @@ -280,3 +280,18 @@ def probe_file_permission_as_user(file, user):
>       p.start()
>       p.join()
>       return queue.get()
> +
> +def validate_repo_url(url):
> +    url_parts = url.split('://') # [0] = prefix, [1] = rest of URL
> +
> +    if url_parts[0] == '':
> +        raise InvalidParameter("KCHREPOS0002E")
> +
> +    if url_parts[0] in ['http', 'https', 'ftp']:
> +        if not check_url_path(url):
> +            raise InvalidParameter("KCHUTILS0001E", {'uri': url})
> +    elif url_parts[0] == 'file':
> +        if not os.path.exists(url_parts[1]):
> +            raise InvalidParameter("KCHUTILS0001E", {'uri': url})
> +    else:
> +        raise InvalidParameter("KCHREPOS0002E")
> diff --git a/tests/test_model.py b/tests/test_model.py
> index 9cfa312..85b11fa 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -1189,7 +1189,7 @@ class ModelTests(unittest.TestCase):
>                         'baseurl': 'http://www.fedora.org'},
>                        {'repo_id': 'fedora-updates-fake',
>                         'config':
> -                      {'mirrorlist': 'http://www.fedora.org/updates',
> +                      {'mirrorlist': 'http://www.fedoraproject.org',
>                          'gpgkey': 'file:///tmp/KEY-fedora-updates-fake-19'}}]
>   
>           deb_repos = [{'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake',
> @@ -1207,6 +1207,27 @@ class ModelTests(unittest.TestCase):
>               # skip test case
>               return
>   
> +        invalid_urls = [
> +                'www.fedora.org',               # missing protocol
> +                '://www.fedora.org',            # missing protocol
> +                'http://www.fedora',            # invalid domain name
> +                'file:///home/userdoesnotexist' # invalid path
> +        ]
> +
> +        # create repositories with invalid baseurl
> +        for url in invalid_urls:
> +            repo = { 'repo_id': 'repo_fake',
> +                     'baseurl': url,
> +                     'config': { 'dist': 'quantal' } }
> +            self.assertRaises(InvalidParameter, inst.repositories_create, repo)
> +
> +        # create repositories with invalid mirrorlist
> +        for url in invalid_urls:
> +            repo = { 'repo_id': 'repo_fake',
> +                     'config': { 'mirrorlist': url,
> +                                 'dist': 'quantal' } }
> +            self.assertRaises(InvalidParameter, inst.repositories_create, repo)
> +
>           for repo in test_repos:
>               system_host_repos = len(inst.repositories_get_list())
>               repo_id = inst.repositories_create(repo)
> @@ -1242,7 +1263,7 @@ class ModelTests(unittest.TestCase):
>   
>           yum_repo = {'repo_id': 'fedora-fake',
>                       'baseurl': 'http://www.fedora.org'}
> -        yum_new_repo = {'baseurl': 'http://www.fedora.org/updates'}
> +        yum_new_repo = {'baseurl': 'http://www.fedoraproject.org'}
>   
>           deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake',
>                       'config': {'dist': 'quantal'}}
> @@ -1266,6 +1287,23 @@ class ModelTests(unittest.TestCase):
>           host_repos = inst.repositories_get_list()
>           self.assertEquals(system_host_repos + 1, len(host_repos))
>   
> +        invalid_urls = [
> +                'www.fedora.org',               # missing protocol
> +                '://www.fedora.org',            # missing protocol
> +                'http://www.fedora',            # invalid domain name
> +                'file:///home/userdoesnotexist' # invalid path
> +        ]
> +
> +        # update repositories with invalid baseurl
> +        for url in invalid_urls:
> +            wrong_repo = { 'baseurl': url }
> +            self.assertRaises(InvalidParameter, inst.repository_update, repo_id, wrong_repo)
> +
> +        # update repositories with invalid mirrorlist
> +        for url in invalid_urls:
> +            wrong_repo = { 'config': { 'mirrorlist': url } }
> +            self.assertRaises(InvalidParameter, inst.repository_update, repo_id, wrong_repo)
> +
>           new_repo_id = inst.repository_update(repo_id, new_repo)
>           repo_info = inst.repository_lookup(new_repo_id)
>   
> diff --git a/tests/test_rest.py b/tests/test_rest.py
> index 1455205..bcb85f5 100644
> --- a/tests/test_rest.py
> +++ b/tests/test_rest.py
> @@ -1771,6 +1771,27 @@ class RestTests(unittest.TestCase):
>           # Already have one repo in Kimchi's system
>           self.assertEquals(1, len(json.loads(resp.read())))
>   
> +        invalid_urls = [
> +                'www.fedora.org',               # missing protocol
> +                '://www.fedora.org',            # missing protocol
> +                'http://www.fedora',            # invalid domain name
> +                'file:///home/userdoesnotexist' # invalid path
> +        ]
> +
> +        # Create repositories with invalid baseurl
> +        for url in invalid_urls:
> +            repo = { 'repo_id': 'fedora-fake', 'baseurl': url }
> +            req = json.dumps(repo)
> +            resp = self.request(base_uri, req, 'POST')
> +            self.assertEquals(400, resp.status)
> +
> +        # Create repositories with invalid mirrorlist
> +        for url in invalid_urls:
> +            repo = { 'repo_id': 'fedora-fake', 'mirrorlist': url }
> +            req = json.dumps(repo)
> +            resp = self.request(base_uri, req, 'POST')
> +            self.assertEquals(400, resp.status)
> +
>           # Create a repository
>           repo = {'repo_id': 'fedora-fake',
>                   'baseurl': 'http://www.fedora.org'}
> @@ -1782,9 +1803,25 @@ class RestTests(unittest.TestCase):
>           res = json.loads(self.request('%s/fedora-fake' % base_uri).read())
>           verify_repo(repo, res)
>   
> +        # Update repositories with invalid baseurl
> +        for url in invalid_urls:
> +            params = {}
> +            params['baseurl'] = url
> +            resp = self.request('%s/fedora-fake' % base_uri, json.dumps(params),
> +                                'PUT')
> +            self.assertEquals(400, resp.status)
> +
> +        # Update repositories with invalid mirrorlist
> +        for url in invalid_urls:
> +            params = {}
> +            params['mirrorlist'] = url
> +            resp = self.request('%s/fedora-fake' % base_uri, json.dumps(params),
> +                                'PUT')
> +            self.assertEquals(400, resp.status)
> +
>           # Update the repository
>           params = {}
> -        params['baseurl'] = repo['baseurl'] = 'http://www.fedora.org/update'
> +        params['baseurl'] = repo['baseurl'] = 'http://www.fedoraproject.org'
>           resp = self.request('%s/fedora-fake' % base_uri, json.dumps(params),
>                               'PUT')
>   




More information about the Kimchi-devel mailing list