From 5b75f0a9b4086a685360fa7a569ab6e1002f9998 Mon Sep 17 00:00:00 2001 From: drewwalters96 Date: Tue, 21 Nov 2017 19:32:02 +0000 Subject: [PATCH] feat(source): Add support for SSH key authentication - Add support for SSH key auth using existing config file value - Add authentication exceptions - Remove redundant git error handling from Armada handler Closes #169 Change-Id: Ia0f61e0b74893289bb90560a743a243393d89c56 --- armada/conf/default.py | 6 +- armada/exceptions/source_exceptions.py | 27 +++++++++ armada/handlers/armada.py | 47 ++++++++------- armada/tests/unit/handlers/test_armada.py | 3 +- armada/tests/unit/utils/test_source.py | 40 +++++++++++-- armada/utils/source.py | 70 +++++++++++++++++------ 6 files changed, 148 insertions(+), 45 deletions(-) diff --git a/armada/conf/default.py b/armada/conf/default.py index cd671d10..f8648a46 100644 --- a/armada/conf/default.py +++ b/armada/conf/default.py @@ -61,10 +61,14 @@ The Keystone project domain name used for authentication. default='admin', help=utils.fmt('The Keystone project name used for authentication.')), + # TODO(fmontei): Add support for multiple SSH keys, not just one site-wide + # one. cfg.StrOpt( 'ssh_key_path', default='/home/user/.ssh/', - help=utils.fmt('Path to SSH private key.')), + help=utils.fmt("""Optional path to an SSH private key used for +authenticating against a Git source repository. The path must be an absolute +path to the private key that includes the name of the key itself.""")), cfg.StrOpt( 'tiller_pod_labels', diff --git a/armada/exceptions/source_exceptions.py b/armada/exceptions/source_exceptions.py index 624bca8a..6d32315d 100644 --- a/armada/exceptions/source_exceptions.py +++ b/armada/exceptions/source_exceptions.py @@ -32,6 +32,21 @@ class GitException(SourceException): super(GitException, self).__init__(self._message) +class GitAuthException(SourceException): + '''Exception that occurs when authentication fails for cloning a repo.''' + + def __init__(self, repo_url, ssh_key_path): + self._repo_url = repo_url + self._ssh_key_path = ssh_key_path + + self._message = ('Failed to authenticate for repo {} with ssh-key at ' + 'path {}. Verify the repo exists and the correct ssh ' + 'key path was supplied in the Armada config ' + 'file.').format(self._repo_url, self._ssh_key_path) + + super(GitAuthException, self).__init__(self._message) + + class GitProxyException(SourceException): '''Exception when an error occurs cloning a Git repository through a proxy.''' @@ -43,6 +58,18 @@ class GitProxyException(SourceException): super(GitProxyException, self).__init__(self._message) +class GitSSHException(SourceException): + '''Exception that occurs when an SSH key could not be found.''' + + def __init__(self, ssh_key_path): + self._ssh_key_path = ssh_key_path + + self._message = ( + 'Failed to find specified SSH key: {}.'.format(self._ssh_key_path)) + + super(GitSSHException, self).__init__(self._message) + + class SourceCleanupException(SourceException): '''Exception that occurs for an invalid dir.''' diff --git a/armada/handlers/armada.py b/armada/handlers/armada.py index cd9e3052..adf4039a 100644 --- a/armada/handlers/armada.py +++ b/armada/handlers/armada.py @@ -159,13 +159,14 @@ class Armada(object): self.tag_cloned_repo(dep, repos) def tag_cloned_repo(self, ch, repos): - location = ch.get('chart').get('source').get('location') - ct_type = ch.get('chart').get('source').get('type') - subpath = ch.get('chart').get('source').get('subpath', '.') - proxy_server = ch.get('chart').get('source').get('proxy_server') + chart = ch.get('chart', {}) + chart_source = chart.get('source', {}) + location = chart_source.get('location') + ct_type = chart_source.get('type') + subpath = chart_source.get('subpath', '.') if ct_type == 'local': - ch.get('chart')['source_dir'] = (location, subpath) + chart['source_dir'] = (location, subpath) elif ct_type == 'tar': LOG.info('Downloading tarball from: %s', location) @@ -176,29 +177,33 @@ class Armada(object): else: tarball_dir = source.get_tarball(location, verify=CONF.cert) - ch.get('chart')['source_dir'] = (tarball_dir, subpath) + chart['source_dir'] = (tarball_dir, subpath) elif ct_type == 'git': - reference = ch.get('chart').get('source').get( - 'reference', 'master') + reference = chart_source.get('reference', 'master') repo_branch = (location, reference) if repo_branch not in repos: - try: - logstr = 'Cloning repo: {} branch: {}'.format(*repo_branch) - if proxy_server: - logstr += ' proxy: {}'.format(proxy_server) - LOG.info(logstr) - repo_dir = source.git_clone(*repo_branch, proxy_server) - except Exception: - raise source_exceptions.GitException( - '{} reference: {}'.format(*repo_branch)) + auth_method = chart_source.get('auth_method') + proxy_server = chart_source.get('proxy_server') + + logstr = 'Cloning repo: {} from branch: {}'.format( + *repo_branch) + if proxy_server: + logstr += ' proxy: {}'.format(proxy_server) + if auth_method: + logstr += ' auth method: {}'.format(auth_method) + LOG.info(logstr) + + repo_dir = source.git_clone(*repo_branch, + proxy_server=proxy_server, + auth_method=auth_method) repos[repo_branch] = repo_dir - ch.get('chart')['source_dir'] = (repo_dir, subpath) + + chart['source_dir'] = (repo_dir, subpath) else: - ch.get('chart')['source_dir'] = (repos.get(repo_branch), - subpath) + chart['source_dir'] = (repos.get(repo_branch), subpath) else: - chart_name = ch.get('chart').get('chart_name') + chart_name = chart.get('chart_name') raise source_exceptions.ChartSourceException(ct_type, chart_name) def get_releases_by_status(self, status): diff --git a/armada/tests/unit/handlers/test_armada.py b/armada/tests/unit/handlers/test_armada.py index 54863b1a..f7880d0b 100644 --- a/armada/tests/unit/handlers/test_armada.py +++ b/armada/tests/unit/handlers/test_armada.py @@ -157,7 +157,8 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): tiller_namespace='kube-system', tiller_port=44134) mock_source.git_clone.assert_called_once_with( - 'git://github.com/dummy/armada', 'master', None) + 'git://github.com/dummy/armada', 'master', auth_method=None, + proxy_server=None) @mock.patch.object(armada, 'source') @mock.patch('armada.handlers.armada.Tiller') diff --git a/armada/tests/unit/utils/test_source.py b/armada/tests/unit/utils/test_source.py index 9e7fb59d..c07de599 100644 --- a/armada/tests/unit/utils/test_source.py +++ b/armada/tests/unit/utils/test_source.py @@ -21,6 +21,7 @@ import mock import testtools from armada.exceptions import source_exceptions +from armada.tests.unit import base from armada.tests import test_utils from armada.utils import source @@ -39,7 +40,7 @@ def is_connected(): return False -class GitTestCase(testtools.TestCase): +class GitTestCase(base.ArmadaTestCase): def _validate_git_clone(self, repo_dir, expected_ref=None): self.assertTrue(os.path.isdir(repo_dir)) @@ -102,11 +103,14 @@ class GitTestCase(testtools.TestCase): is_connected(), 'git clone requires network connectivity.') def test_git_clone_fake_proxy(self): url = 'http://github.com/att-comdev/armada' + proxy_url = test_utils.rand_name( + 'not.a.proxy.that.works.and.never.will', + prefix='http://') + ":8080" self.assertRaises( source_exceptions.GitProxyException, source.git_clone, url, - proxy_server='http://not.a.proxy.that.works.and.never.will:8080') + proxy_server=proxy_url) @mock.patch('armada.utils.source.tempfile') @mock.patch('armada.utils.source.requests') @@ -128,7 +132,7 @@ class GitTestCase(testtools.TestCase): mock_requests.get(url).content) @mock.patch('armada.utils.source.tempfile') - @mock.patch('armada.utils.source.path') + @mock.patch('armada.utils.source.os.path') @mock.patch('armada.utils.source.tarfile') def test_tarball_extract(self, mock_tarfile, mock_path, mock_temp): mock_path.exists.return_value = True @@ -145,7 +149,7 @@ class GitTestCase(testtools.TestCase): mock_opened_file.extractall.assert_called_once_with('/tmp/armada') @test_utils.attr(type=['negative']) - @mock.patch('armada.utils.source.path') + @mock.patch('armada.utils.source.os.path') @mock.patch('armada.utils.source.tarfile') def test_tarball_extract_bad_path(self, mock_tarfile, mock_path): mock_path.exists.return_value = False @@ -186,7 +190,7 @@ class GitTestCase(testtools.TestCase): @test_utils.attr(type=['negative']) @mock.patch.object(source, 'LOG') @mock.patch('armada.utils.source.shutil') - @mock.patch('armada.utils.source.path') + @mock.patch('armada.utils.source.os.path') def test_source_cleanup_missing_git_path(self, mock_path, mock_shutil, mock_log): # Verify that passing in a missing path does nothing but log a warning. @@ -200,3 +204,29 @@ class GitTestCase(testtools.TestCase): self.assertEqual( ('Could not delete the path %s. Is it a git repository?', path), actual_call) + + @testtools.skipUnless( + is_connected(), 'git clone requires network connectivity.') + @test_utils.attr(type=['negative']) + @mock.patch.object(source, 'os') + def test_git_clone_ssh_auth_method_fails_auth(self, mock_os): + mock_os.path.exists.return_value = True + fake_user = test_utils.rand_name('fake_user') + url = ('ssh://%s@review.gerrithub.io:29418/att-comdev/armada' + % fake_user) + self.assertRaises( + source_exceptions.GitAuthException, source.git_clone, url, + ref='refs/changes/17/388517/5', auth_method='SSH') + + @testtools.skipUnless( + is_connected(), 'git clone requires network connectivity.') + @test_utils.attr(type=['negative']) + @mock.patch.object(source, 'os') + def test_git_clone_ssh_auth_method_missing_ssh_key(self, mock_os): + mock_os.path.exists.return_value = False + fake_user = test_utils.rand_name('fake_user') + url = ('ssh://%s@review.gerrithub.io:29418/att-comdev/armada' + % fake_user) + self.assertRaises( + source_exceptions.GitSSHException, source.git_clone, url, + ref='refs/changes/17/388517/5', auth_method='SSH') diff --git a/armada/utils/source.py b/armada/utils/source.py index 9934415d..2fa348da 100644 --- a/armada/utils/source.py +++ b/armada/utils/source.py @@ -16,50 +16,86 @@ import os import shutil import tarfile import tempfile -from os import path from git import exc as git_exc from git import Git from git import Repo +from oslo_config import cfg from oslo_log import log as logging import requests from requests.packages import urllib3 from armada.exceptions import source_exceptions +CONF = cfg.CONF LOG = logging.getLogger(__name__) -def git_clone(repo_url, ref='master', proxy_server=None): +def git_clone(repo_url, ref='master', proxy_server=None, auth_method=None): '''Clone a git repository from ``repo_url`` using the reference ``ref``. - :params repo_url: URL of git repo to clone. - :params ref: branch, commit or reference in the repo to clone. - :params proxy_server: optional, HTTP proxy to use while cloning the repo. + :param repo_url: URL of git repo to clone. + :param ref: branch, commit or reference in the repo to clone. Default is + 'master'. + :param proxy_server: optional, HTTP proxy to use while cloning the repo. + :param auth_method: Method to use for authenticating against the repository + specified in ``repo_url``. If value is "SSH" Armada attempts to + authenticate against the repository using the SSH key specified under + ``CONF.ssh_key_path``. If value is None, authentication is skipped. + Valid values include "SSH" or None. Note that the values are not + case sensitive. Default is None. :returns: Path to the cloned repo. + :raises GitException: If ``repo_url`` is invalid or could not be found. + :raises GitAuthException: If authentication with the Git repository failed. + :raises GitProxyException: If the repo could not be cloned due to a proxy + issue. + :raises GitSSHException: If the SSH key specified by ``CONF.ssh_key_path`` + could not be found and ``auth_method`` is "SSH". ''' - if repo_url == '': + if not repo_url: raise source_exceptions.GitException(repo_url) - os.environ['GIT_TERMINAL_PROMPT'] = '0' - _tmp_dir = tempfile.mkdtemp(prefix='armada') + env_vars = {'GIT_TERMINAL_PROMPT': '0'} + temp_dir = tempfile.mkdtemp(prefix='armada') + ssh_cmd = None + + if auth_method and auth_method.lower() == 'ssh': + LOG.debug('Attempting to clone the repo at %s using reference %s ' + 'with SSH authentication.', repo_url, ref) + + if not os.path.exists(CONF.ssh_key_path): + LOG.error('SSH auth method was specified for cloning repo but ' + 'the SSH key under CONF.ssh_key_path was not found.') + raise source_exceptions.GitSSHException(CONF.ssh_key_path) + + ssh_cmd = ( + 'ssh -i {} -o ConnectionAttempts=20 -o ConnectTimeout=10' + .format(os.path.expanduser(CONF.ssh_key_path)) + ) + env_vars.update({'GIT_SSH_COMMAND': ssh_cmd}) + else: + LOG.debug('Attempting to clone the repo at %s using reference %s ' + 'with no authentication.', repo_url, ref) try: if proxy_server: LOG.debug('Cloning [%s] with proxy [%s]', repo_url, proxy_server) - repo = Repo.clone_from(repo_url, _tmp_dir, + repo = Repo.clone_from(repo_url, temp_dir, env=env_vars, config='http.proxy=%s' % proxy_server) else: LOG.debug('Cloning [%s]', repo_url) - repo = Repo.clone_from(repo_url, _tmp_dir) + repo = Repo.clone_from(repo_url, temp_dir, env=env_vars) repo.remotes.origin.fetch(ref) g = Git(repo.working_dir) g.checkout('FETCH_HEAD') except git_exc.GitCommandError as e: LOG.exception('Encountered GitCommandError during clone.') - if 'Could not resolve proxy' in e.stderr: + if ssh_cmd and ssh_cmd in e.stderr: + raise source_exceptions.GitAuthException(repo_url, + CONF.ssh_key_path) + elif 'Could not resolve proxy' in e.stderr: raise source_exceptions.GitProxyException(proxy_server) else: raise source_exceptions.GitException(repo_url) @@ -67,7 +103,7 @@ def git_clone(repo_url, ref='master', proxy_server=None): LOG.exception('Encountered unknown Exception during clone.') raise source_exceptions.GitException(repo_url) - return _tmp_dir + return temp_dir def get_tarball(tarball_url, verify=False): @@ -98,17 +134,17 @@ def extract_tarball(tarball_path): ''' Extracts a tarball to /tmp and returns the path ''' - if not path.exists(tarball_path): + if not os.path.exists(tarball_path): raise source_exceptions.InvalidPathException(tarball_path) - _tmp_dir = tempfile.mkdtemp(prefix='armada') + temp_dir = tempfile.mkdtemp(prefix='armada') try: file = tarfile.open(tarball_path) - file.extractall(_tmp_dir) + file.extractall(temp_dir) except Exception: raise source_exceptions.TarballExtractException(tarball_path) - return _tmp_dir + return temp_dir def source_cleanup(git_path): @@ -119,7 +155,7 @@ def source_cleanup(git_path): :param str git_path: The git repository to delete. ''' - if path.exists(git_path): + if os.path.exists(git_path): try: # Internally validates whether the path points to an actual repo. Repo(git_path)