#708 Implement support for keytab in gssapi codepaths
Merged 6 years ago by mikem. Opened 6 years ago by puiterwijk.
puiterwijk/koji py3-keytab  into  master

file modified
+32 -14
@@ -2126,16 +2126,17 @@ 

          principal.  The principal must be in the "ProxyPrincipals" list on

          the server side."""

  

-         if principal is None and keytab is None and ccache is None:

-             try:

-                 # Silently try GSSAPI first

-                 if self.gssapi_login(proxyuser=proxyuser):

-                     return True

-             except:

-                 if krbV:

-                     pass

-                 else:

-                     raise

+         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(
@@ -2224,7 +2225,7 @@ 

          # else

          return host

  

-     def gssapi_login(self, proxyuser=None):

+     def gssapi_login(self, principal=None, keytab=None, ccache=None, proxyuser=None):

          if not HTTPKerberosAuth:

              raise PythonImportError(

                  "Please install python-requests-kerberos to use GSSAPI."
@@ -2241,21 +2242,38 @@ 

  

          # 60 second timeout during login

          sinfo = None

+         old_env = {}

          old_opts = self.opts

          self.opts = old_opts.copy()

-         self.opts['timeout'] = 60

-         self.opts['auth'] = HTTPKerberosAuth()

          try:

+             self.opts['timeout'] = 60

+             kwargs = {}

+             if keytab:

+                 old_env['KRB5_CLIENT_KTNAME'] = os.environ.get('KRB5_CLIENT_KTNAME')

+                 os.environ['KRB5_CLIENT_KTNAME'] = keytab

+             if ccache:

+                 old_env['KRB5CCNAME'] = os.environ.get('KRB5CCNAME')

+                 os.environ['KRB5CCNAME'] = ccache

+             if principal:

+                 kwargs['principal'] = principal

+             self.opts['auth'] = HTTPKerberosAuth(**kwargs)

              try:

                  # Depending on the server configuration, we might not be able to

                  # connect without client certificate, which means that the conn

                  # will fail with a handshake failure, which is retried by default.

                  sinfo = self._callMethod('sslLogin', [proxyuser], retry=False)

-             except:

+             except Exception as e:

+                 e_str = ''.join(traceback.format_exception_only(type(e), e))

+                 self.logger.debug('gssapi auth failed: %s', e_str)

                  # Auth with https didn't work. Restore for the next attempt.

                  self.baseurl = old_baseurl

          finally:

              self.opts = old_opts

+             for key in old_env:

+                 if old_env[key] is None:

+                     del os.environ[key]

+                 else:

+                     os.environ[key] = old_env[key]

          if not sinfo:

              raise AuthError('unable to obtain a session')

  

@@ -0,0 +1,67 @@ 

+ from __future__ import absolute_import

+ import mock

+ import os

+ import unittest

+ 

+ import koji

+ 

+ 

+ class TestGSSAPI(unittest.TestCase):

+ 

+     def setUp(self):

+         self.session = koji.ClientSession('https://koji.example.com/kojihub', {})

+         self.session._callMethod = mock.MagicMock(name='_callMethod')

+ 

+     def tearDown(self):

+         mock.patch.stopall()

+ 

+     maxDiff = None

+ 

+     @mock.patch('koji.HTTPKerberosAuth', new=None)

+     def test_gssapi_disabled(self):

+         with self.assertRaises(ImportError):

+             self.session.gssapi_login()

+ 

+     def test_gssapi_login(self):

+         old_environ = dict(**os.environ)

+         self.session.gssapi_login()

+         self.session._callMethod.assert_called_once_with('sslLogin', [None],

+                 retry=False)

+         self.assertEqual(old_environ, dict(**os.environ))

+ 

+     def test_gssapi_login_keytab(self):

+         principal = 'user@EXAMPLE.COM'

+         keytab = '/path/to/keytab'

+         ccache = '/path/to/cache'

+         old_environ = dict(**os.environ)

+         self.session.gssapi_login(principal, keytab, ccache)

+         self.session._callMethod.assert_called_once_with('sslLogin', [None],

+                 retry=False)

+         self.assertEqual(old_environ, dict(**os.environ))

+ 

+     def test_gssapi_login_error(self):

+         old_environ = dict(**os.environ)

+         self.session._callMethod.side_effect = Exception('login failed')

+         with self.assertRaises(koji.AuthError):

+             self.session.gssapi_login()

+         self.session._callMethod.assert_called_once_with('sslLogin', [None],

+                 retry=False)

+         self.assertEqual(old_environ, dict(**os.environ))

+ 

+     def test_gssapi_login_http(self):

+         old_environ = dict(**os.environ)

+         url1 = 'http://koji.example.com/kojihub'

+         url2 = 'https://koji.example.com/kojihub'

+ 

+         # successful gssapi auth should force https

+         self.session.baseurl = url1

+         self.session.gssapi_login()

+         self.assertEqual(self.session.baseurl, url2)

+ 

+         # failed gssapi auth should leave the url alone

+         self.session.baseurl = url1

+         self.session._callMethod.side_effect = Exception('login failed')

+         with self.assertRaises(koji.AuthError):

+             self.session.gssapi_login()

+         self.assertEqual(self.session.baseurl, url1)

+         self.assertEqual(old_environ, dict(**os.environ))

This had been a TODO that until now wasn't critical since python2 would fall back to krbV.
For python3, we only have gssapi, so implement keytabs and ccache for gssapi.

Signed-off-by: Patrick Uiterwijk patrick@puiterwijk.org

Thanks for this, looks good at a glance. I'll play with it some tomorrow.

Any concerns about different versions of python-requests-kerberos?

The principal argument has been supported since python-requests-kerberos 0.9.0, released on May 6, 2016. The rest of the stuff is handled by krb5-libs.

rebased onto 6157651bb64d910a272112374871d8aa6b1102be

6 years ago

Unfortunately, I can't fully test this myself since I don't have keytabs for a gssapi enabled koji instance. However, it seems to be sane and not break the existing auth.

A few things though...

  1. As long as we're here, we should get rid of the bare exception handlers.
  2. The code for restoring os.environ doesn't work. It looks like doing this properly might be a bit messing since os.environ is an odd type.
  3. We're trying to add unit tests as we go. In the process of reviewing this, I've written a few.
  4. The try..finally for resetting the environ should probably cover a wider span in case there is an error earlier.

I've addressed some of this here:
https://github.com/mikem23/koji-playground/commits/pagure/pr/708

This includes a unit test that is currently failing because the environ reset isn't working.

rebased onto 6bfef6c

6 years ago

Hi, thanks for those fixes.
I've pushed a new changeset to this PR, which includes your commits, and fixes the environment resetting, making the gssapi tests pass.

6 new commits added

  • Fix resetting the environment
  • unit tests for gssapi auth
  • fix another bare exception
  • be a little more paranoid about undoing opts/environ mangling
  • avoid bare exception and add some debug logging
  • Implement support for keytab in gssapi codepaths
6 years ago

whoops, one of my commits used old exception syntax. Fixed here:
https://github.com/mikem23/koji-playground/commits/pagure/pr/708

1 new commit added

  • fix exception syntax
6 years ago

Commit b320d2f fixes this pull-request

Pull-Request has been merged by mikem

6 years ago