From 5bf673e2edeba67123033b400cd714648f2e507a Mon Sep 17 00:00:00 2001
From: Patrick Uiterwijk <puiterwijk@redhat.com>
Date: Nov 01 2016 09:45:45 +0000
Subject: [PATCH 1/8] Make client support both krbV and gssapi


Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>

---

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 <puiterwijk@redhat.com>
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 <puiterwijk@redhat.com>

---

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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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)

