From 25122cb53ee7644b531ea2f997de417a00519a8d Mon Sep 17 00:00:00 2001 From: mprahl Date: Dec 18 2018 21:05:55 +0000 Subject: Modify MBS to use a separate Kerberos context per thread so both threads can use the thread keyring to store the Kerberos cache This commit includes the backport of the changes to `krb_login` in https://pagure.io/koji/pull-request/1187. This change is required for our separate threads to use a separate Kerberos context per thread. --- diff --git a/module_build_service/__init__.py b/module_build_service/__init__.py index 01a5a0f..cd5016f 100644 --- a/module_build_service/__init__.py +++ b/module_build_service/__init__.py @@ -41,7 +41,6 @@ for a number of tasks: """ import pkg_resources -import os from flask import Flask, has_app_context, url_for from flask_sqlalchemy import SQLAlchemy from sqlalchemy.pool import StaticPool @@ -70,9 +69,6 @@ app.wsgi_app = ReverseProxy(app.wsgi_app) conf = init_config(app) -# We want to use a separate Kerberos cache per thread to avoid Kerberos cache corruption -os.environ['KRB5CCNAME'] = 'KEYRING:thread:mbs' - class MBSSQLAlchemy(SQLAlchemy): """ diff --git a/module_build_service/builder/KojiModuleBuilder.py b/module_build_service/builder/KojiModuleBuilder.py index 3804156..6a4160f 100644 --- a/module_build_service/builder/KojiModuleBuilder.py +++ b/module_build_service/builder/KojiModuleBuilder.py @@ -49,6 +49,7 @@ from module_build_service import log, conf, models import module_build_service.scm import module_build_service.utils from module_build_service.builder.utils import execute_cmd +from module_build_service.builder.koji_backports import ClientSession as KojiClientSession from module_build_service.errors import ProgrammingError from module_build_service.builder.base import GenericBuilder @@ -455,7 +456,7 @@ chmod 644 %buildroot/etc/rpm/macros.zz-modules address = koji_config.server log.info("Connecting to koji %r.", address) - koji_session = koji.ClientSession(address, opts=koji_config) + koji_session = KojiClientSession(address, opts=koji_config) if not login: return koji_session @@ -463,13 +464,23 @@ chmod 644 %buildroot/etc/rpm/macros.zz-modules authtype = koji_config.authtype log.info("Authenticate session with %r.", authtype) if authtype == "kerberos": + try: + import krbV + except ImportError: + raise RuntimeError( + "python-krbV must be installed to authenticate with Koji using Kerberos") keytab = getattr(config, "krb_keytab", None) principal = getattr(config, "krb_principal", None) if not keytab and principal: raise ValueError( "The Kerberos keytab and principal aren't set for Koji authentication") log.debug(" keytab: %r, principal: %r" % (keytab, principal)) - koji_session.krb_login(principal=principal, keytab=keytab) + # We want to create a context per thread to avoid Kerberos cache corruption + ctx = krbV.Context() + # We want to use the thread keyring for the ccache to ensure we have one cache per + # thread to avoid Kerberos cache corruption + ccache = "KEYRING:thread:mbs" + koji_session.krb_login(principal=principal, keytab=keytab, ctx=ctx, ccache=ccache) elif authtype == "ssl": koji_session.ssl_login( os.path.expanduser(koji_config.cert), diff --git a/module_build_service/builder/koji_backports.py b/module_build_service/builder/koji_backports.py new file mode 100644 index 0000000..c4e3dd0 --- /dev/null +++ b/module_build_service/builder/koji_backports.py @@ -0,0 +1,98 @@ +# flake8: noqa +import base64 +import traceback + +import koji +# Import krbV from here so we don't have to redo the whole try except that Koji does +from koji import krbV, PythonImportError, AuthError, AUTHTYPE_KERB + + +class ClientSession(koji.ClientSession): + """The koji.ClientSession class with patches from upstream.""" + + # This backport comes from https://pagure.io/koji/pull-request/1187 + def krb_login(self, principal=None, keytab=None, ccache=None, proxyuser=None, ctx=None): + """Log in using Kerberos. If principal is not None and keytab is + not None, then get credentials for the given principal from the given keytab. + If both are None, authenticate using existing local credentials (as obtained + from kinit). ccache is the absolute path to use for the credential cache. If + not specified, the default ccache will be used. If proxyuser is specified, + log in the given user instead of the user associated with the Kerberos + principal. The principal must be in the "ProxyPrincipals" list on + the server side. ctx is the Kerberos context to use, and should be unique + per thread. If ctx is not specified, the default context is used.""" + + try: + # Silently try GSSAPI first + if self.gssapi_login(principal, keytab, ccache, proxyuser=proxyuser): + return True + except Exception as e: + if krbV: + e_str = ''.join(traceback.format_exception_only(type(e), e)) + self.logger.debug('gssapi auth failed: %s', e_str) + pass + else: + raise + + if not krbV: + raise PythonImportError( + "Please install python-krbV to use kerberos." + ) + + if not ctx: + ctx = krbV.default_context() + + if ccache != None: + ccache = krbV.CCache(name=ccache, context=ctx) + else: + ccache = ctx.default_ccache() + + if principal != None: + if keytab != None: + cprinc = krbV.Principal(name=principal, context=ctx) + keytab = krbV.Keytab(name=keytab, context=ctx) + ccache.init(cprinc) + ccache.init_creds_keytab(principal=cprinc, keytab=keytab) + else: + raise AuthError('cannot specify a principal without a keytab') + else: + # We're trying to log ourself in. Connect using existing credentials. + cprinc = ccache.principal() + + self.logger.debug('Authenticating as: %s', cprinc.name) + sprinc = krbV.Principal(name=self._serverPrincipal(cprinc), context=ctx) + + ac = krbV.AuthContext(context=ctx) + ac.flags = krbV.KRB5_AUTH_CONTEXT_DO_SEQUENCE | krbV.KRB5_AUTH_CONTEXT_DO_TIME + ac.rcache = ctx.default_rcache() + + # create and encode the authentication request + (ac, req) = ctx.mk_req(server=sprinc, client=cprinc, + auth_context=ac, ccache=ccache, + options=krbV.AP_OPTS_MUTUAL_REQUIRED) + req_enc = base64.encodestring(req) + + # ask the server to authenticate us + (rep_enc, sinfo_enc, addrinfo) = self.callMethod('krbLogin', req_enc, proxyuser) + # Set the addrinfo we received from the server + # (necessary before calling rd_priv()) + # addrinfo is in (serveraddr, serverport, clientaddr, clientport) + # format, so swap the pairs because clientaddr is now the local addr + ac.addrs = tuple((addrinfo[2], addrinfo[3], addrinfo[0], addrinfo[1])) + + # decode and read the reply from the server + rep = base64.decodestring(rep_enc) + ctx.rd_rep(rep, auth_context=ac) + + # decode and decrypt the login info + sinfo_priv = base64.decodestring(sinfo_enc) + sinfo_str = ac.rd_priv(sinfo_priv) + sinfo = dict(zip(['session-id', 'session-key'], sinfo_str.split())) + + if not sinfo: + self.logger.warn('No session info received') + return False + self.setSession(sinfo) + + self.authtype = AUTHTYPE_KERB + return True diff --git a/tests/test_builder/test_koji.py b/tests/test_builder/test_koji.py index 58a8c28..78b5ff2 100644 --- a/tests/test_builder/test_koji.py +++ b/tests/test_builder/test_koji.py @@ -324,7 +324,7 @@ class TestKojiBuilder: expected_calls = [mock.call(1, 'foo'), mock.call(2, 'foo'), mock.call(1, 'bar')] assert mock_session.untagBuild.mock_calls == expected_calls - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights(self, ClientSession): session = ClientSession.return_value session.getLoggedInUser.return_value = {"id": 123} @@ -347,7 +347,7 @@ class TestKojiBuilder: # getLoggedInUser requires to a logged-in session session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights_no_task_id(self, ClientSession): session = ClientSession.return_value session.getLoggedInUser.return_value = {"id": 123} @@ -368,7 +368,7 @@ class TestKojiBuilder: assert session.getTaskDescendents.mock_calls == expected_calls session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights_no_build(self, ClientSession): session = ClientSession.return_value session.getLoggedInUser.return_value = {"id": 123} @@ -389,7 +389,7 @@ class TestKojiBuilder: assert session.getTaskDescendents.mock_calls == expected_calls session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights_listBuilds_failed(self, ClientSession): session = ClientSession.return_value session.getLoggedInUser.return_value = {"id": 123} @@ -406,7 +406,7 @@ class TestKojiBuilder: assert session.listBuilds.mock_calls == expected_calls session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights_getPackageID_failed(self, ClientSession): session = ClientSession.return_value session.getLoggedInUser.return_value = {"id": 123} @@ -421,7 +421,7 @@ class TestKojiBuilder: session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_build_weights_getLoggedInUser_failed(self, ClientSession): session = ClientSession.return_value session.getAverageBuildDuration.return_value = None @@ -546,7 +546,7 @@ class TestKojiBuilder: expected_calls = [] assert session.packageListBlock.mock_calls == expected_calls - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_built_rpms_in_module_build(self, ClientSession): session = ClientSession.return_value session.listTaggedRPMS.return_value = ([ @@ -601,7 +601,7 @@ class TestKojiBuilder: [] ), )) - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_filtered_rpms_on_self_dep(self, ClientSession, br_filtered_rpms, expected): session = ClientSession.return_value session.listTaggedRPMS.return_value = ( @@ -679,14 +679,14 @@ class TestKojiBuilder: else: mock_koji_cg.koji_import.assert_not_called() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_anonymous_session(self, ClientSession): mbs_config = mock.Mock(koji_profile='koji', koji_config='conf/koji.conf') session = KojiModuleBuilder.get_session(mbs_config, login=False) assert ClientSession.return_value == session assert ClientSession.return_value.krb_login.assert_not_called - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_ensure_builder_use_a_logged_in_koji_session(self, ClientSession): builder = KojiModuleBuilder('owner', MagicMock(), conf, 'module-tag', []) builder.koji_session.krb_login.assert_called_once() diff --git a/tests/test_content_generator.py b/tests/test_content_generator.py index 7e8137a..04f1e2d 100644 --- a/tests/test_content_generator.py +++ b/tests/test_content_generator.py @@ -85,7 +85,7 @@ class TestBuild: except OSError: pass - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") @patch("subprocess.Popen") @patch("subprocess.check_output", return_value='1.4') @patch("pkg_resources.get_distribution") @@ -142,7 +142,7 @@ class TestBuild: # Ensure an anonymous Koji session works koji_session.krb_login.assert_not_called() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") @patch("subprocess.Popen") @patch("subprocess.check_output", return_value='1.4') @patch("pkg_resources.get_distribution") @@ -206,7 +206,7 @@ class TestBuild: with open(path.join(dir_path, "modulemd.i686.txt")) as mmd: assert len(mmd.read()) == 255 - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_tag_cg_build(self, ClientSession): """ Test that the CG build is tagged. """ koji_session = ClientSession.return_value @@ -221,7 +221,7 @@ class TestBuild: # tagBuild requires logging into a session in advance. koji_session.krb_login.assert_called_once() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_tag_cg_build_fallback_to_default_tag(self, ClientSession): """ Test that the CG build is tagged to default tag. """ koji_session = ClientSession.return_value @@ -238,7 +238,7 @@ class TestBuild: # tagBuild requires logging into a session in advance. koji_session.krb_login.assert_called_once() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_tag_cg_build_no_tag_set(self, ClientSession): """ Test that the CG build is not tagged when no tag set. """ koji_session = ClientSession.return_value @@ -252,7 +252,7 @@ class TestBuild: # tagBuild requires logging into a session in advance. koji_session.krb_login.assert_called_once() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_tag_cg_build_no_tag_available(self, ClientSession): """ Test that the CG build is not tagged when no tag available. """ koji_session = ClientSession.return_value @@ -332,7 +332,7 @@ class TestBuild: 'type': 'file' } - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_koji_rpms_in_tag(self, ClientSession): koji_session = ClientSession.return_value koji_session.getUser.return_value = GET_USER_RV @@ -420,7 +420,7 @@ class TestBuild: # Listing tagged RPMs does not require to log into a session koji_session.krb_login.assert_not_called() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_koji_rpms_in_tag_empty_tag(self, ClientSession): koji_session = ClientSession.return_value koji_session.getUser.return_value = GET_USER_RV @@ -432,7 +432,7 @@ class TestBuild: assert rpms == [] koji_session.multiCall.assert_not_called() - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_koji_rpms_in_tag_empty_headers(self, ClientSession): koji_session = ClientSession.return_value koji_session.getUser.return_value = GET_USER_RV diff --git a/tests/test_scheduler/test_poller.py b/tests/test_scheduler/test_poller.py index b486c9f..eb815cf 100644 --- a/tests/test_scheduler/test_poller.py +++ b/tests/test_scheduler/test_poller.py @@ -98,7 +98,7 @@ class TestPoller: assert len(start_build_component.mock_calls) == expected_build_calls - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_trigger_new_repo_when_failed( self, ClientSession, create_builder, global_consumer, dbg): """ @@ -131,7 +131,7 @@ class TestPoller: koji_session.newRepo.assert_called_once_with( "module-testmodule-master-20170219191323-c40c156c-build") - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_trigger_new_repo_when_succeeded( self, ClientSession, create_builder, global_consumer, dbg): """ @@ -203,7 +203,7 @@ class TestPoller: for component in components: assert component.state is None - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_old_build_targets_are_not_associated_with_any_module_builds( self, ClientSession, create_builder, global_consumer, dbg): consumer = mock.MagicMock() @@ -223,7 +223,7 @@ class TestPoller: koji_session.deleteBuildTarget.assert_not_called() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_dont_delete_base_module_build_target( self, ClientSession, create_builder, global_consumer, dbg): module_build = models.ModuleBuild.query.filter_by(id=3).one() @@ -248,7 +248,7 @@ class TestPoller: koji_session.deleteBuildTarget.assert_not_called() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_dont_delete_build_target_for_unfinished_module_builds( self, ClientSession, create_builder, global_consumer, dbg): module_build = models.ModuleBuild.query.filter_by(id=3).one() @@ -275,7 +275,7 @@ class TestPoller: koji_session.deleteBuildTarget.assert_not_called() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_only_delete_build_target_with_allowed_koji_tag_prefix( self, ClientSession, create_builder, global_consumer, dbg): module_build_2 = models.ModuleBuild.query.filter_by(id=2).one() @@ -320,7 +320,7 @@ class TestPoller: koji_session.deleteBuildTarget.assert_called_once_with(1) koji_session.krb_login.assert_called_once() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_cant_delete_build_target_if_not_reach_delete_time( self, ClientSession, create_builder, global_consumer, dbg): module_build_2 = models.ModuleBuild.query.filter_by(id=2).one() diff --git a/tests/test_utils/test_ursine.py b/tests/test_utils/test_ursine.py index 82a9c84..2eff754 100644 --- a/tests/test_utils/test_ursine.py +++ b/tests/test_utils/test_ursine.py @@ -121,7 +121,7 @@ class TestGetModulemdsFromUrsineContent: def teardown_method(self, test_method): clean_database() - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_return_empty_if_no_ursine_build_tag_is_found(self, ClientSession): session = ClientSession.return_value @@ -141,7 +141,7 @@ class TestGetModulemdsFromUrsineContent: assert [] == modulemds @patch.object(conf, 'koji_tag_prefixes', new=['module']) - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_get_modulemds(self, ClientSession): session = ClientSession.return_value @@ -246,7 +246,7 @@ class TestRecordStreamCollisionModules: @patch.object(conf, 'base_module_names', new=['platform', 'project-platform']) @patch('module_build_service.utils.ursine.get_modulemds_from_ursine_content') @patch('module_build_service.resolver.GenericResolver.create') - @patch('koji.ClientSession') + @patch('module_build_service.builder.KojiModuleBuilder.KojiClientSession') def test_add_collision_modules(self, ClientSession, resolver_create, get_modulemds_from_ursine_content): xmd = { diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 38fd247..b714772 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -417,7 +417,7 @@ class TestViews: for key, part in zip(nsvc_keys, nsvc_parts): assert item[key] == part - @patch("koji.ClientSession") + @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_query_builds_with_binary_rpm(self, ClientSession): """ Test for querying MBS with the binary rpm filename. MBS should return all the modules,