#2280 Use requests_gssapi instead of requests_kerberos
Merged 2 years ago by mikem. Opened 2 years ago by simo.

file modified
+8 -4
@@ -75,10 +75,14 @@ 

      krbV = None

  

  try:

-     import requests_kerberos

-     Krb5Error = requests_kerberos.exceptions.RequestException

+     import requests_gssapi as reqgssapi

+     Krb5Error = reqgssapi.exceptions.RequestException

  except ImportError:  # pragma: no cover

-     requests_kerberos = None

+     try:

+         import requests_kerberos as reqgssapi

+         Krb5Error = reqgssapi.exceptions.RequestException

+     except ImportError:  # pragma: no cover

+         reqgssapi = None

  

  try:

      import librepo
@@ -6544,7 +6548,7 @@ 

              quit("Error: Unable to log in. Bad credentials?")

          except requests.exceptions.ConnectionError:

              quit("Error: Unable to connect to server %s" % (options.server))

-     elif krbV or requests_kerberos:

+     elif krbV or reqgssapi:

          krb_principal = options.krb_principal

          if krb_principal is None:

              krb_principal = options.host_principal_format % socket.getfqdn()

file modified
+4
@@ -149,7 +149,11 @@ 

  %endif

  Requires: python%{python3_pkgversion}-pyOpenSSL

  Requires: python%{python3_pkgversion}-requests

+ %if 0%{?fedora} >= 32

+ Requires: python%{python3_pkgversion}-requests-gssapi

+ %else

  Requires: python%{python3_pkgversion}-requests-kerberos

+ %endif

  Requires: python%{python3_pkgversion}-dateutil

  Requires: python%{python3_pkgversion}-six

  

file modified
+10 -7
@@ -82,9 +82,12 @@ 

      # we ignore it here

      pass

  try:

-     import requests_kerberos

+     import requests_gssapi as reqgssapi

  except ImportError:  # pragma: no cover

-     requests_kerberos = None

+     try:

+         import requests_kerberos as reqgssapi

+     except ImportError:  # pragma: no cover

+         reqgssapi = None

  try:

      import rpm

  except ImportError:
@@ -2589,9 +2592,9 @@ 

          return host

  

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

-         if not requests_kerberos:

+         if not reqgssapi:

              raise PythonImportError(

-                 "Please install python-requests-kerberos to use GSSAPI."

+                 "Please install python-requests-gssapi to use GSSAPI."

              )

          # force https

          old_baseurl = self.baseurl
@@ -2618,14 +2621,14 @@ 

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

                  os.environ['KRB5CCNAME'] = ccache

              if principal:

-                 if re.match(r'0[.][1-8]\b', requests_kerberos.__version__):

+                 if re.match(r'0[.][1-8]\b', reqgssapi.__version__):

                      raise PythonImportError(

-                         'python-requests-kerberos >= 0.9.0 required for '

+                         'python-requests-gssapi >= 0.9.0 required for '

                          'keytab auth'

                      )

                  else:

                      kwargs['principal'] = principal

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

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

              try:

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

                  # connect without client certificate, which means that the conn

file modified
+1 -1
@@ -14,7 +14,7 @@ 

      requires = [

          'python-dateutil',

          'requests',

-         'requests-kerberos',

+         'requests-gssapi',

          'six',

          # 'libcomps',

          # 'rpm-py-installer', # it is optional feature

file modified
+10 -10
@@ -21,7 +21,7 @@ 

  

      maxDiff = None

  

-     @mock.patch('koji.requests_kerberos', new=None)

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

      def test_gssapi_disabled(self):

          with self.assertRaises(ImportError):

              self.session.gssapi_login()
@@ -33,13 +33,13 @@ 

                                                           retry=False)

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

  

-     @mock.patch('requests_kerberos.HTTPKerberosAuth')

+     @mock.patch('koji.reqgssapi.HTTPKerberosAuth')

      def test_gssapi_login_keytab(self, HTTPKerberosAuth_mock):

          principal = 'user@EXAMPLE.COM'

          keytab = '/path/to/keytab'

          ccache = '/path/to/cache'

          old_environ = dict(**os.environ)

-         current_version = koji.requests_kerberos.__version__

+         current_version = koji.reqgssapi.__version__

          accepted_versions = ['0.12.0.beta1',

                               '0.12.0dev',

                               '0.12.0a1',
@@ -47,7 +47,7 @@ 

                               '0.10.0',

                               '0.9.0']

          for accepted_version in accepted_versions:

-             koji.requests_kerberos.__version__ = accepted_version

+             koji.reqgssapi.__version__ = accepted_version

              rv = self.session.gssapi_login(principal, keytab, ccache)

              self.session._callMethod.assert_called_once_with('sslLogin',

                                                               [None],
@@ -55,14 +55,14 @@ 

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

              self.assertTrue(rv)

              self.session._callMethod.reset_mock()

-         koji.requests_kerberos.__version__ = current_version

+         koji.reqgssapi.__version__ = current_version

  

-     def test_gssapi_login_keytab_unsupported_requests_kerberos_version(self):

+     def test_gssapi_login_keytab_unsupported_requests_kerberos(self):

          principal = 'user@EXAMPLE.COM'

          keytab = '/path/to/keytab'

          ccache = '/path/to/cache'

          old_environ = dict(**os.environ)

-         current_version = koji.requests_kerberos.__version__

+         current_version = koji.reqgssapi.__version__

          old_versions = ['0.8.0',

                          '0.7.0',

                          '0.6.1',
@@ -72,15 +72,15 @@ 

                          '0.2',

                          '0.1']

          for old_version in old_versions:

-             koji.requests_kerberos.__version__ = old_version

+             koji.reqgssapi.__version__ = old_version

              with self.assertRaises(koji.PythonImportError) as cm:

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

              self.assertEqual(cm.exception.args[0],

-                              'python-requests-kerberos >= 0.9.0 required for '

+                              'python-requests-gssapi >= 0.9.0 required for '

                               'keytab auth')

              self.session._callMethod.assert_not_called()

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

-         koji.requests_kerberos.__version__ = current_version

+         koji.reqgssapi.__version__ = current_version

  

      def test_gssapi_login_error(self):

          old_environ = dict(**os.environ)

file modified
+2 -2
@@ -14,7 +14,7 @@ 

  

  class KrbVTestCase(unittest.TestCase):

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

-     @mock.patch('koji.requests_kerberos', new=None)

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

      def test_krbv_disabled(self):

          """Test that when krbV and gssapi are absent, we behave rationally"""

          self.assertEquals(koji.krbV, None)
@@ -25,7 +25,7 @@ 

      # this case should work on python3, but skipped still

      @unittest.skipIf(six.PY3, "skipped on python3 since missing of python-krbV")

      @mock.patch('koji.krbV', create=True)

-     @mock.patch('requests_kerberos.__version__', new='0.7.0')

+     @mock.patch('koji.reqgssapi.__version__', new='0.7.0')

      @mock.patch('koji.ClientSession._serverPrincipal')

      def test_krbv_old_requests_kerberos(self, _serverPrincipal_mock, krbV_mock):

          self.assertIsNotNone(koji.krbV)

file modified
+1 -1
@@ -374,7 +374,7 @@ 

      elif options.user:

          # authenticate using user/password

          session.login()

-     elif koji.requests_kerberos:

+     elif koji.reqgssapi:

          session.gssapi_login(principal=options.principal, keytab=options.keytab,

                               proxyuser=options.runas)

      if not options.noauth and not session.logged_in:

file modified
+1 -1
@@ -1152,7 +1152,7 @@ 

      elif options.user:

          # authenticate using user/password

          session.login()

-     elif (koji.krbV or koji.requests_kerberos) and options.principal and options.keytab:

+     elif (koji.krbV or koji.reqgssapi) and options.principal and options.keytab:

          session.krb_login(options.principal, options.keytab, options.ccache)

      else:

          quit("No username/password/certificate supplied and Kerberos missing or not configured")

Retain ability to use the old requests_kerberos where request_gssapi is
not available yet.

rebased onto d5b53fc126f156437633a0d5b3ea6b5fa4143c0a

2 years ago

@tkopecek thanks for the help, CI passes now!
(following up from #2274)

Thank you for writing this patch!

I'm confused why we don't simply import requests_gssapi without renaming it to the obscure reqgssapi name.

This would allow us to set requests_kerberos = None, and this would make it easier to read and understand that hairy code below that tests requests_kerberos.__version__ for the principal kwarg support. Like "If we have requests_gssapi, assume that the principal kwarg will always work without checking __version__"

I just didn't want to have divergence at this stage, once we remove the support for requests-kerberos and the even older krbV cleanup can be done.

I see there were some comments in #2274 about the lack of python-requests-gssapi on el6 and el7. As a Fedora packager, is that something I can help with, or are those platforms too old for the latest versions of python-gssapi + python-requests-gssapi? It would simplify QE if we could reduce the options.

I do not know about EL7, but almost certainly EL6 can't do python-gssapi/request-gssapi easily enough, we just need to wait for EL6 to die a graceful death ...

python-requests-gssapi is packaged for epel7.

el6 doesn't support new enough python tooling (cython, etc.) for python-gssapi. Heck, I have to patch around cython bugs just to build in el7, even.

Better would be to stop caring about el6, given even el7 is in maintenance mode.

any obstacle to merging this one ?

Capacity :- ) We need to test it more - I've scheduled if to 1.23 already but if there is some strong reason to squeeze it into 1.22 we can discuss it.

When is 1.23 going to land in Fedora ?
I am providing patches like this because we want to drop python-kerberos from Fedora, ans we'll never do it if we can't make a concerted effort to remove all dependencies and then ban re-entry :-)

It would be around October. But yes, it make sense to squeeze it into 1.22

the version check doesn't seem necessary anymore. requests-gssapi's first GA is 1.0.0

@julian8628 as mentioned before that code runs with both requests-gssapi AND requests-kerberos, for gssapi it is redundant but it is still needed if request-kerberos is the only available.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

breaking automerge, removing testing-ready temporarily

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

2 years ago

Is there anything I need to do here ?

We need to merge all other testing-ready PRs first, so it is rebasing correctly. No need for action.

there's no python2-requests-gssapi on fedora32+ (python-requests-gssapi is python3-requests-gssapithere, and py2 ver is available on fedora29 and 28)
I think we could just remove this part for python2-koji

Do you want me to amend this PR with that commit ?

@simo Yes please. Appreciated that!

rebased onto 3b9fcc6

2 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

2 years ago

Commit 18e0e88 fixes this pull-request

Pull-Request has been merged by mikem

2 years ago

resolved minor merge conflicts with #2244

Are you sure the change in util/kojira is correct ?
It seem to undo something I was not touching in my original commit

to be clear I think gssapi_login _> krb_login is wrong in your conflict resolution