From a51ef6a02d08551ae966ef6dac016673db69ba1f Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Oct 05 2017 20:12:54 +0000 Subject: Move krb_context from freshmaker.handlers to freshmaker.utils and ensure krb_login is called for any koji_service call when krb principal is set. --- diff --git a/freshmaker/config.py b/freshmaker/config.py index 6c940fb..59c949a 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -256,7 +256,7 @@ class Config(object): 'desc': 'Whether to acquire credential cache from a client keytab.'}, 'krb_auth_principal': { 'type': str, - 'default': True, + 'default': "", 'desc': 'Principal used to acquire credential cache, which must be' ' present in specified client keytab.'}, 'krb_auth_client_keytab': { diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index fc45be8..a5ae417 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -32,7 +32,6 @@ from freshmaker.mbs import MBS from freshmaker.models import ArtifactBuildState from freshmaker.types import ArtifactType from freshmaker.models import ArtifactBuild, Event -from krbcontext import krbContext from freshmaker.odcsclient import ODCS from freshmaker.odcsclient import AuthMech @@ -63,23 +62,6 @@ class BaseHandler(object): """ raise NotImplementedError() - @property - def krb_context(self): - if conf.krb_auth_use_keytab: - krb_ctx_opts = { - 'using_keytab': conf.krb_auth_use_keytab, - 'principal': conf.krb_auth_principal, - 'keytab_file': conf.krb_auth_client_keytab, - 'ccache_file': conf.krb_auth_ccache_file, - } - else: - krb_ctx_opts = { - 'principal': conf.krb_auth_principal, - 'ccache_file': conf.krb_auth_ccache_file, - } - - return krbContext(**krb_ctx_opts) - def build_module(self, name, branch, rev): """ Build a module in MBS. @@ -197,19 +179,6 @@ class ContainerBuildHandler(BaseHandler): :rtype: int """ with koji_service(profile=conf.koji_profile, logger=log) as service: - log.debug('Logging into %s with Kerberos authentication.', - service.server) - - proxyuser = conf.koji_build_owner if conf.koji_proxyuser else None - - with self.krb_context: - service.krb_login(proxyuser=proxyuser) - - # We are not logged in in dry run mode... - if not conf.dry_run and not service.logged_in: - log.error('Could not login server %s', service.server) - return None - log.info('Building container from source: %s, ' 'release=%r, parent=%r, target=%r', scm_url, release, koji_parent_build, target) diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 789ffcf..f6794b4 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -38,6 +38,7 @@ from freshmaker.errata import Errata from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.models import Event from freshmaker.consumer import work_queue_put +from freshmaker.utils import krb_context from odcs.client.odcs import ODCS from odcs.client.odcs import AuthMech @@ -200,7 +201,7 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): odcs = ODCS(conf.odcs_server_url, auth_mech=AuthMech.Kerberos, verify_ssl=conf.odcs_verify_ssl) if not conf.dry_run: - with self.krb_context: + with krb_context(): new_compose = odcs.new_compose( compose_source, 'tag', packages=packages) else: diff --git a/freshmaker/kojiservice.py b/freshmaker/kojiservice.py index 5b28081..067da6b 100644 --- a/freshmaker/kojiservice.py +++ b/freshmaker/kojiservice.py @@ -28,6 +28,7 @@ import re from freshmaker import log, conf from freshmaker.consumer import work_queue_put from freshmaker.events import BrewContainerTaskStateChangeEvent +from freshmaker.utils import krb_context class KojiService(object): @@ -162,7 +163,7 @@ class KojiService(object): @contextlib.contextmanager -def koji_service(profile=None, logger=None): +def koji_service(profile=None, logger=None, login=True): """A Koji service context manager that could be used with with Example:: @@ -179,6 +180,24 @@ def koji_service(profile=None, logger=None): ... """ service = KojiService(profile=profile) + + if login: + if not conf.krb_auth_principal: + log.error("Cannot login to Koji, krb_auth_principal not set") + else: + log.debug('Logging into %s with Kerberos authentication.', + service.server) + + proxyuser = conf.koji_build_owner if conf.koji_proxyuser else None + + with krb_context(): + service.krb_login(proxyuser=proxyuser) + + # We are not logged in in dry run mode... + if not conf.dry_run and not service.logged_in: + log.error('Could not login server %s', service.server) + yield None + try: yield service finally: diff --git a/freshmaker/utils.py b/freshmaker/utils.py index 5709531..3723a93 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -32,6 +32,24 @@ import tempfile import time from freshmaker import conf +from krbcontext import krbContext + + +def krb_context(): + if conf.krb_auth_use_keytab: + krb_ctx_opts = { + 'using_keytab': conf.krb_auth_use_keytab, + 'principal': conf.krb_auth_principal, + 'keytab_file': conf.krb_auth_client_keytab, + 'ccache_file': conf.krb_auth_ccache_file, + } + else: + krb_ctx_opts = { + 'principal': conf.krb_auth_principal, + 'ccache_file': conf.krb_auth_ccache_file, + } + + return krbContext(**krb_ctx_opts) def load_class(location): diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index 9ea008c..b86f4f5 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -453,8 +453,7 @@ class TestPrepareYumRepo(unittest.TestCase): 'ErrataAdvisoryRPMsSignedHandler._get_compose_source') @patch('time.sleep') @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') - @patch('freshmaker.handlers.BaseHandler.krb_context', - new_callable=PropertyMock) + @patch('freshmaker.utils.krbContext') def test_get_repo_url_when_succeed_to_generate_compose( self, krb_context, errata, sleep, _get_compose_source, _get_packages_for_compose, ODCS): @@ -497,7 +496,7 @@ class TestPrepareYumRepo(unittest.TestCase): 'ErrataAdvisoryRPMsSignedHandler._get_compose_source') @patch('time.sleep') @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') - @patch('freshmaker.handlers.BaseHandler.krb_context', + @patch('freshmaker.kojiservice.krb_context', new_callable=PropertyMock) def test_get_repo_url_packages_in_multiple_tags( self, krb_context, errata, sleep, _get_compose_source, @@ -528,7 +527,7 @@ class TestPrepareYumRepo(unittest.TestCase): 'ErrataAdvisoryRPMsSignedHandler._get_compose_source') @patch('time.sleep') @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') - @patch('freshmaker.handlers.BaseHandler.krb_context', + @patch('freshmaker.kojiservice.krb_context', new_callable=PropertyMock) def test_get_repo_url_packages_not_found_in_tag( self, krb_context, errata, sleep, _get_compose_source, diff --git a/tests/test_git_dockerfile_change_handler.py b/tests/test_git_dockerfile_change_handler.py index 15583a2..9c9a9ea 100644 --- a/tests/test_git_dockerfile_change_handler.py +++ b/tests/test_git_dockerfile_change_handler.py @@ -25,7 +25,7 @@ import unittest import fedmsg.config from mock import patch -from mock import MagicMock +from mock import MagicMock, PropertyMock from freshmaker import db, models from freshmaker.consumer import FreshmakerConsumer @@ -62,9 +62,11 @@ class GitDockerfileChangeHandlerTest(BaseTestCase): @patch('koji.read_config') @patch('koji.ClientSession') - @patch('freshmaker.handlers.krbContext') + @patch('freshmaker.utils.krbContext') + @patch("freshmaker.config.Config.krb_auth_principal", + new_callable=PropertyMock, return_value="user@example.com") def test_rebuild_if_dockerfile_changed( - self, krbContext, ClientSession, read_config): + self, auth_principal, krbContext, ClientSession, read_config): read_config.return_value = { 'server': 'https://localhost/kojihub', 'krb_rdns': False, @@ -117,7 +119,9 @@ class GitDockerfileChangeHandlerTest(BaseTestCase): @patch('koji.read_config') @patch('koji.ClientSession') - def test_ensure_do_nothing_if_fail_to_login_koji(self, ClientSession, read_config): + @patch("freshmaker.config.Config.krb_auth_principal", + new_callable=PropertyMock, return_value="user@example.com") + def test_ensure_do_nothing_if_fail_to_login_koji(self, auth_principal, ClientSession, read_config): ClientSession.return_value.krb_login.side_effect = RuntimeError read_config.return_value = { 'server': 'https://localhost/kojihub', diff --git a/tests/test_handler.py b/tests/test_handler.py index 3bf0837..fc51b3a 100644 --- a/tests/test_handler.py +++ b/tests/test_handler.py @@ -24,7 +24,7 @@ import json -from mock import patch +from mock import patch, PropertyMock from unittest import TestCase from freshmaker import db @@ -49,15 +49,17 @@ class TestKrbContextPreparedForBuildContainer(TestCase): """Test krb_context for BaseHandler.build_container""" def setUp(self): - self.koji_service = patch('freshmaker.handlers.koji_service') + self.koji_service = patch('freshmaker.kojiservice.KojiService') self.koji_service.start() def tearDown(self): self.koji_service.stop() - @patch('freshmaker.handlers.conf') - @patch('freshmaker.handlers.krbContext') - def test_prepare_with_keytab(self, krbContext, conf): + @patch('freshmaker.utils.conf') + @patch('freshmaker.utils.krbContext') + @patch("freshmaker.config.Config.krb_auth_principal", + new_callable=PropertyMock, return_value="user@example.com") + def test_prepare_with_keytab(self, auth_principal, krbContext, conf): conf.krb_auth_use_keytab = True conf.krb_auth_principal = 'freshmaker/hostname@REALM' conf.krb_auth_client_keytab = '/etc/freshmaker.keytab' @@ -73,9 +75,11 @@ class TestKrbContextPreparedForBuildContainer(TestCase): ccache_file='/tmp/freshmaker_cc', ) - @patch('freshmaker.handlers.conf') - @patch('freshmaker.handlers.krbContext') - def test_prepare_with_normal_user_credential(self, krbContext, conf): + @patch('freshmaker.utils.conf') + @patch('freshmaker.utils.krbContext') + @patch("freshmaker.config.Config.krb_auth_principal", + new_callable=PropertyMock, return_value="user@example.com") + def test_prepare_with_normal_user_credential(self, auth_principal, krbContext, conf): conf.krb_auth_use_keytab = False conf.krb_auth_principal = 'somebody@REALM' conf.krb_auth_ccache_file = '/tmp/freshmaker_cc' @@ -150,7 +154,7 @@ class TestBuildFirstBatch(TestCase): @patch('freshmaker.handlers.ODCS') @patch('koji.ClientSession') - @patch('freshmaker.handlers.krbContext') + @patch('freshmaker.utils.krbContext') def test_build_first_batch(self, krb, ClientSession, ODCS): """ Tests that only PLANNED images without a parent are submitted to