From 5bf673e2edeba67123033b400cd714648f2e507a Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Nov 01 2016 09:45:45 +0000 Subject: [PATCH 1/8] Make client support both krbV and gssapi Signed-off-by: Patrick Uiterwijk --- diff --git a/koji/__init__.py b/koji/__init__.py index 54d7356..9873323 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -23,10 +23,16 @@ import sys try: + import gssapi +except ImportError: # pragma: no cover + gssapi = None +try: import krbV except ImportError: # pragma: no cover - sys.stderr.write("Warning: Could not install krbV module. Kerberos support will be disabled.\n") - sys.stderr.flush() + krbV = None + if not gssapi: + sys.stderr.write("Warning: Could not install krbV or gssapi module. Kerberos support will be disabled.\n") + sys.stderr.flush() import base64 import datetime import ConfigParser @@ -1910,21 +1916,23 @@ class ClientSession(object): sinfo = self.callMethod('subsession') return type(self)(self.baseurl, self.opts, sinfo) - def krb_login(self, principal=None, keytab=None, ccache=None, proxyuser=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.""" - - if not krbV: - raise exceptions.ImportError( - "Please install python-krbV to use kerberos." - ) + def krb_gssapi_login(self, principal, keytab, ccache, proxyuser): + if keytab: + os.environ['KRB5_CLIENT_KTNAME'] = keytab + n = gssapi.Name('%s@%s' % (self.opts.get('krbservice', 'host'), + self._krbServername()), + gssapi.NameType.hostbased_service) + r = gssapi.SecurityContext(name=n, flags=[gssapi.RequirementFlag.mutual_authentication, + gssapi.RequirementFlag.replay_detection, gssapi.RequirementFlag.out_of_sequence_detection]) + req_enc = base64.encodestring(r.step()) + (rep_enc, sinfo_enc, addrinfo) = self.callMethod('gssapiLogin', req_enc, proxyuser) + rep = base64.decodestring(rep_enc) + r.step(rep) + sinfo_priv = base64.decodestring(sinfo_enc) + sinfo_str = r.decrypt(sinfo_priv) + return sinfo_str + def krb_krbV_login(self, principal, keytab, ccache, proxyuser): ctx = krbV.default_context() if ccache != None: @@ -1944,7 +1952,7 @@ class ClientSession(object): # We're trying to log ourself in. Connect using existing credentials. cprinc = ccache.principal() - sprinc = krbV.Principal(name=self._serverPrincipal(cprinc), context=ctx) + sprinc = krbV.Principal(name=self._serverPrincipal(cprinc.realm), context=ctx) ac = krbV.AuthContext(context=ctx) ac.flags = krbV.KRB5_AUTH_CONTEXT_DO_SEQUENCE|krbV.KRB5_AUTH_CONTEXT_DO_TIME @@ -1972,6 +1980,33 @@ class ClientSession(object): # decode and decrypt the login info sinfo_priv = base64.decodestring(sinfo_enc) sinfo_str = ac.rd_priv(sinfo_priv) + return sinfo_str + + def _krb_login_methods(self, principal, keytab, ccache, proxyuser): + methods = xmlrpclib.ServerProxy('http://%s%s' % + (self._host, self._path)).system.listMethods() + + if gssapi and 'gssapiLogin' in methods: + return self.krb_gssapi_login(principal, keytab, ccache, proxyuser) + + if krbV: + return self.krb_krbV_login(principal, keytab, ccache, proxyuser) + + def krb_login(self, principal=None, keytab=None, ccache=None, proxyuser=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.""" + + if not krbV and not gssapi: + raise exceptions.ImportError( + "Please install either python-gssapi or python-krbV to use kerberos." + ) + sinfo_str = self._krb_login_methods(principal, keytab, ccache, proxyuser) sinfo = dict(zip(['session-id', 'session-key'], sinfo_str.split())) if not sinfo: @@ -1982,17 +2017,19 @@ class ClientSession(object): self.authtype = AUTHTYPE_KERB return True - def _serverPrincipal(self, cprinc): - """Get the Kerberos principal of the server we're connecting - to, based on baseurl.""" + def _krbServername(self): if self.opts.get('krb_rdns', True): - servername = socket.getfqdn(self._host) + return socket.getfqdn(self._host) else: - servername = self._host + return self._host + + def _serverPrincipal(self, realm): + """Get the Kerberos principal of the server we're connecting + to, based on baseurl.""" + servername = self._krbServername() #portspec = servername.find(':') #if portspec != -1: # servername = servername[:portspec] - realm = cprinc.realm service = self.opts.get('krbservice', 'host') return '%s/%s@%s' % (service, servername, realm) From 520afcc2abef2115968b36bafbfcca25e77280af Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Nov 01 2016 09:45:46 +0000 Subject: [PATCH 2/8] Make the server accept both krbV and gssapi Signed-off-by: Patrick Uiterwijk --- diff --git a/hub/kojixmlrpc.py b/hub/kojixmlrpc.py index f3057a1..de9ef1f 100644 --- a/hub/kojixmlrpc.py +++ b/hub/kojixmlrpc.py @@ -285,12 +285,12 @@ class ModXMLRPCRequestHandler(object): context.session.validate() except koji.AuthLockError: #might be ok, depending on method - if context.method not in ('exclusiveSession', 'login', 'krbLogin', 'logout'): + if context.method not in ('exclusiveSession', 'login', 'gssapiLogin', 'krbLogin', 'logout'): raise def enforce_lockout(self): if context.opts.get('LockOut') and \ - context.method not in ('login', 'krbLogin', 'sslLogin', 'logout') and \ + context.method not in ('login', 'gssapiLogin', 'krbLogin', 'sslLogin', 'logout') and \ not context.session.hasPerm('admin'): raise koji.ServerOffline, "Server disabled for maintenance" @@ -796,6 +796,7 @@ def get_registry(opts, plugins): registry.register_instance(functions) registry.register_module(hostFunctions, "host") registry.register_function(koji.auth.login) + registry.register_function(koji.auth.gssapiLogin) registry.register_function(koji.auth.krbLogin) registry.register_function(koji.auth.sslLogin) registry.register_function(koji.auth.logout) diff --git a/koji/auth.py b/koji/auth.py index 3dbe1cc..5353678 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -24,6 +24,8 @@ import string import random import base64 import krbV +import gssapi +import os import koji import cgi #for parse_qs from context import context @@ -285,6 +287,60 @@ class Session(object): context.cnx.commit() return sinfo + def _krbProcess(self, cprinc, proxyuser): + # Successfully authenticated via Kerberos, now log in + if proxyuser: + proxyprincs = [princ.strip() for princ in context.opts.get('ProxyPrincipals', '').split(',')] + if cprinc in proxyprincs: + login_principal = proxyuser + else: + raise koji.AuthError, \ + 'Kerberos principal %s is not authorized to log in other users' % cprinc + else: + login_principal = cprinc + user_id = self.getUserIdFromKerberos(login_principal) + if not user_id: + if context.opts.get('LoginCreatesUser'): + user_id = self.createUserFromKerberos(login_principal) + else: + raise koji.AuthError, 'Unknown Kerberos principal: %s' % login_principal + + self.checkLoginAllowed(user_id) + + hostip = context.environ['REMOTE_ADDR'] + #XXX - REMOTE_ADDR not promised by wsgi spec + if hostip == '127.0.0.1': + hostip = socket.gethostbyname(socket.gethostname()) + + sinfo = self.createSession(user_id, hostip, koji.AUTHTYPE_KERB) + return '%(session-id)s %(session-key)s' % sinfo + + def gssapiLogin(self, krb_req, proxyuser=None): + """Authenticate the user using the base64-encoded + gssapi initial step. If proxyuser is not None, + log in that user instead of the user associated with the + kerberos principal. The principal must be an authorized + "proxy_principal" in the server config.""" + if self.logged_in: + raise koji.AuthError, "Already logged in" + + if not (context.opts.get('AuthPrincipal') and context.opts.get('AuthKeytab')): + raise koji.AuthError, 'not configured for Kerberos authentication' + + conninfo = self.getConnInfo() + os.environ['KRB5_KTNAME'] = context.opts.get('AuthKeytab') + r = gssapi.SecurityContext() + req = base64.decodestring(krb_req) + rep = r.step(req) + rep_enc = base64.encodestring(rep) + cprinc = str(r.initiator_name) + + sinfo = self._krbProcess(cprinc, proxyuser) + sinfo_priv = r.encrypt(sinfo) + sinfo_enc = base64.encodestring(sinfo_priv) + + return (rep_enc, sinfo_enc, conninfo) + def krbLogin(self, krb_req, proxyuser=None): """Authenticate the user using the base64-encoded AP_REQ message in krb_req. If proxyuser is not None, @@ -311,40 +367,15 @@ class Session(object): ac, opts, sprinc, ccreds = ctx.rd_req(req, server=srvprinc, keytab=srvkt, auth_context=ac, options=krbV.AP_OPTS_MUTUAL_REQUIRED) - cprinc = ccreds[2] - - # Successfully authenticated via Kerberos, now log in - if proxyuser: - proxyprincs = [princ.strip() for princ in context.opts.get('ProxyPrincipals', '').split(',')] - if cprinc.name in proxyprincs: - login_principal = proxyuser - else: - raise koji.AuthError, \ - 'Kerberos principal %s is not authorized to log in other users' % cprinc.name - else: - login_principal = cprinc.name - user_id = self.getUserIdFromKerberos(login_principal) - if not user_id: - if context.opts.get('LoginCreatesUser'): - user_id = self.createUserFromKerberos(login_principal) - else: - raise koji.AuthError, 'Unknown Kerberos principal: %s' % login_principal - - self.checkLoginAllowed(user_id) - - hostip = context.environ['REMOTE_ADDR'] - #XXX - REMOTE_ADDR not promised by wsgi spec - if hostip == '127.0.0.1': - hostip = socket.gethostbyname(socket.gethostname()) - - sinfo = self.createSession(user_id, hostip, koji.AUTHTYPE_KERB) + cprinc = ccreds[2].name + rep = ctx.mk_rep(auth_context=ac) # encode the reply - rep = ctx.mk_rep(auth_context=ac) rep_enc = base64.encodestring(rep) # encrypt and encode the login info - sinfo_priv = ac.mk_priv('%(session-id)s %(session-key)s' % sinfo) + sinfo = self._krbProcess(cprinc, proxyuser) + sinfo_priv = ac.mk_priv(sinfo) sinfo_enc = base64.encodestring(sinfo_priv) return (rep_enc, sinfo_enc, conninfo) @@ -693,6 +724,9 @@ def get_user_data(user_id): def login(*args, **opts): return context.session.login(*args, **opts) +def gssapiLogin(*args, **opts): + return context.session.gssapiLogin(*args, **opts) + def krbLogin(*args, **opts): return context.session.krbLogin(*args, **opts) From 066743fde8da6bfe7600ad35caa8b643d37b6af4 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:45:53 +0000 Subject: [PATCH 3/8] fix/extend krb unit test --- diff --git a/tests/test_krbv.py b/tests/test_krbv.py index b8a88e1..51b9146 100644 --- a/tests/test_krbv.py +++ b/tests/test_krbv.py @@ -9,10 +9,30 @@ import koji class KrbVTestCase(unittest.TestCase): @mock.patch('koji.krbV', new=None) + @mock.patch('koji.gssapi', new=None) @mock.patch('koji.ClientSession._setup_connection') - def test_krbv_disabled(self, krbV): - """ Test that when krbV is absent, we behave rationally. """ + def test_krbv_disabled(self, _setup_connection): + """ Test that when krb libs are absent, we behave rationally. """ self.assertEquals(koji.krbV, None) + self.assertEquals(koji.gssapi, None) session = koji.ClientSession('whatever') with self.assertRaises(ImportError): session.krb_login() + + @mock.patch('koji.krbV', new=None) + @mock.patch('koji.gssapi', new=True) + @mock.patch('koji.ClientSession.krb_gssapi_login') + @mock.patch('koji.ClientSession.krb_krbV_login') + @mock.patch('koji.ClientSession._setup_connection') + @mock.patch('koji.ClientSession._callMethod') + def test_krbv_disabled(self, _callMethod, _setup_connection, krb_krbV_login, krb_gssapi_login): + """ Test that gssapi codepath gets used """ + #import pdb; pdb.set_trace() + self.assertEquals(koji.krbV, None) + self.assertEquals(koji.gssapi, True) + krb_gssapi_login.return_value = '23 fnord' #session id and key + session = koji.ClientSession('whatever') + login_args = ('principal', 'keytab', 'ccache', 'proxyuser') + session.krb_login(*login_args) + krb_krbV_login.assert_not_called() + krb_gssapi_login.assert_called_with(*login_args) From e7d3b367da8deb36c8ab72fa2583b62bb2b7c5fa Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:46:09 +0000 Subject: [PATCH 4/8] update krb unit test --- diff --git a/tests/test_krbv.py b/tests/test_krbv.py index 51b9146..bcd47a6 100644 --- a/tests/test_krbv.py +++ b/tests/test_krbv.py @@ -10,8 +10,8 @@ class KrbVTestCase(unittest.TestCase): @mock.patch('koji.krbV', new=None) @mock.patch('koji.gssapi', new=None) - @mock.patch('koji.ClientSession._setup_connection') - def test_krbv_disabled(self, _setup_connection): + @mock.patch('koji.ClientSession._setup_connection', new=mock.MagicMock()) + def test_krbv_disabled(self): """ Test that when krb libs are absent, we behave rationally. """ self.assertEquals(koji.krbV, None) self.assertEquals(koji.gssapi, None) @@ -20,19 +20,35 @@ class KrbVTestCase(unittest.TestCase): session.krb_login() @mock.patch('koji.krbV', new=None) - @mock.patch('koji.gssapi', new=True) + @mock.patch('koji.gssapi', new=None) + @mock.patch('koji.ClientSession._setup_connection', new=mock.MagicMock()) + @mock.patch('koji.ClientSession._callMethod', new=mock.MagicMock()) + @mock.patch('koji.ClientSession.logout', new=mock.MagicMock()) @mock.patch('koji.ClientSession.krb_gssapi_login') @mock.patch('koji.ClientSession.krb_krbV_login') - @mock.patch('koji.ClientSession._setup_connection') - @mock.patch('koji.ClientSession._callMethod') - def test_krbv_disabled(self, _callMethod, _setup_connection, krb_krbV_login, krb_gssapi_login): - """ Test that gssapi codepath gets used """ - #import pdb; pdb.set_trace() - self.assertEquals(koji.krbV, None) - self.assertEquals(koji.gssapi, True) - krb_gssapi_login.return_value = '23 fnord' #session id and key - session = koji.ClientSession('whatever') + def test_krbv_disabled(self, krb_krbV_login, krb_gssapi_login): + """ Test that correct krb codepath is used """ + + mocks = (krb_krbV_login, krb_gssapi_login) + sinfo = {'session-id': '23', 'session-key': 'fnord'} + sinfo_str = '23 fnord' login_args = ('principal', 'keytab', 'ccache', 'proxyuser') - session.krb_login(*login_args) - krb_krbV_login.assert_not_called() - krb_gssapi_login.assert_called_with(*login_args) + + with mock.patch('koji.gssapi', new=True): + krb_gssapi_login.return_value = sinfo_str + session = koji.ClientSession('whatever') + session.krb_login(*login_args) + krb_krbV_login.assert_not_called() + krb_gssapi_login.assert_called_with(*login_args) + assert session.sinfo == sinfo + + for m in mocks: + m.reset_mock() + + with mock.patch('koji.krbV', new=True): + krb_krbV_login.return_value = sinfo_str + session = koji.ClientSession('whatever') + session.krb_login(*login_args) + krb_gssapi_login.assert_not_called() + krb_krbV_login.assert_called_with(*login_args) + assert session.sinfo == sinfo From c01ea298cc4473117fbdfea4294ab05953b269b4 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:46:17 +0000 Subject: [PATCH 5/8] avoid stalls due to ClientSession.__del__ in unit tests --- diff --git a/tests/test_krbv.py b/tests/test_krbv.py index bcd47a6..b8da32c 100644 --- a/tests/test_krbv.py +++ b/tests/test_krbv.py @@ -6,6 +6,13 @@ import mock import koji +class ClientSession(koji.ClientSession): + """Override __del__ method, which causes stalls on some test failures""" + + def __del__(self): + pass + + class KrbVTestCase(unittest.TestCase): @mock.patch('koji.krbV', new=None) @@ -15,7 +22,7 @@ class KrbVTestCase(unittest.TestCase): """ Test that when krb libs are absent, we behave rationally. """ self.assertEquals(koji.krbV, None) self.assertEquals(koji.gssapi, None) - session = koji.ClientSession('whatever') + session = ClientSession('whatever') with self.assertRaises(ImportError): session.krb_login() @@ -36,7 +43,7 @@ class KrbVTestCase(unittest.TestCase): with mock.patch('koji.gssapi', new=True): krb_gssapi_login.return_value = sinfo_str - session = koji.ClientSession('whatever') + session = ClientSession('whatever') session.krb_login(*login_args) krb_krbV_login.assert_not_called() krb_gssapi_login.assert_called_with(*login_args) @@ -47,7 +54,7 @@ class KrbVTestCase(unittest.TestCase): with mock.patch('koji.krbV', new=True): krb_krbV_login.return_value = sinfo_str - session = koji.ClientSession('whatever') + session = ClientSession('whatever') session.krb_login(*login_args) krb_gssapi_login.assert_not_called() krb_krbV_login.assert_called_with(*login_args) From 4d5a50a85371674dfb3c5ac052ec9b7d1b4bb39c Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:46:26 +0000 Subject: [PATCH 6/8] avoid using xmlrpclib.ServerProxy --- diff --git a/koji/__init__.py b/koji/__init__.py index 9873323..3617817 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -1983,8 +1983,7 @@ class ClientSession(object): return sinfo_str def _krb_login_methods(self, principal, keytab, ccache, proxyuser): - methods = xmlrpclib.ServerProxy('http://%s%s' % - (self._host, self._path)).system.listMethods() + methods = self.system.listMethods() if gssapi and 'gssapiLogin' in methods: return self.krb_gssapi_login(principal, keytab, ccache, proxyuser) From c7c229e62bf9dbce727db03520e39f0b39fa5241 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:46:41 +0000 Subject: [PATCH 7/8] no need to call listMethods if gssapi module not available --- diff --git a/koji/__init__.py b/koji/__init__.py index 3617817..33d7d6c 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -1983,9 +1983,7 @@ class ClientSession(object): return sinfo_str def _krb_login_methods(self, principal, keytab, ccache, proxyuser): - methods = self.system.listMethods() - - if gssapi and 'gssapiLogin' in methods: + if gssapi and 'gssapiLogin' in self.system.listMethods(): return self.krb_gssapi_login(principal, keytab, ccache, proxyuser) if krbV: From b374c3d33be4d86cbd1c835c79a7ac25e01e4d6c Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 01 2016 09:46:51 +0000 Subject: [PATCH 8/8] mock system.listMethods call --- diff --git a/tests/test_krbv.py b/tests/test_krbv.py index b8da32c..efe5221 100644 --- a/tests/test_krbv.py +++ b/tests/test_krbv.py @@ -13,6 +13,12 @@ class ClientSession(koji.ClientSession): pass +def mock_callMethod(name, args, kwargs=None): + if name == 'system.listMethods': + return ['gssapiLogin'] + return mock.DEFAULT + + class KrbVTestCase(unittest.TestCase): @mock.patch('koji.krbV', new=None) @@ -29,11 +35,11 @@ class KrbVTestCase(unittest.TestCase): @mock.patch('koji.krbV', new=None) @mock.patch('koji.gssapi', new=None) @mock.patch('koji.ClientSession._setup_connection', new=mock.MagicMock()) - @mock.patch('koji.ClientSession._callMethod', new=mock.MagicMock()) @mock.patch('koji.ClientSession.logout', new=mock.MagicMock()) + @mock.patch('koji.ClientSession._callMethod', side_effect=mock_callMethod) @mock.patch('koji.ClientSession.krb_gssapi_login') @mock.patch('koji.ClientSession.krb_krbV_login') - def test_krbv_disabled(self, krb_krbV_login, krb_gssapi_login): + def test_krbv_codepath(self, krb_krbV_login, krb_gssapi_login, _callMethod): """ Test that correct krb codepath is used """ mocks = (krb_krbV_login, krb_gssapi_login)