#143 Port from pykerberos to python-gssapi
Merged 6 years ago by dcallagh. Opened 6 years ago by rharwood.
rharwood/waiverdb python-gssapi  into  master

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

  Flask-RESTful

  Flask-SQLAlchemy

  SQLAlchemy

- kerberos >= 1.1.1

+ gssapi

  flask-oidc

  systemd

  # packages for the unit tests
@@ -23,7 +23,7 @@ 

  click

  configparser

  openidc-client

- requests-kerberos

+ requests-gssapi

  

  # Database

  psycopg2-binary

file modified
+16 -24
@@ -1,7 +1,8 @@ 

  # SPDX-License-Identifier: GPL-2.0+

  

+ from base64 import b64encode

  import pytest

- import kerberos

+ import gssapi  # noqa

  import mock

  import json

  from werkzeug.exceptions import Unauthorized
@@ -10,32 +11,21 @@ 

  

  

  @pytest.mark.usefixtures('enable_kerberos')

- class TestKerberosAuthentication(object):

- 

-     def test_keytab_file_is_not_set_should_raise_error(self):

-         with pytest.raises(Unauthorized):

-             request = mock.MagicMock()

-             headers = {'Authorization': "babablaba"}

-             request.headers.return_value = mock.MagicMock(spec_set=dict)

-             request.headers.__getitem__.side_effect = headers.__getitem__

-             request.headers.__setitem__.side_effect = headers.__setitem__

-             request.headers.__contains__.side_effect = headers.__contains__

-             waiverdb.auth.get_user(request)

- 

+ class TestGSSAPIAuthentication(object):

      def test_unauthorized(self, client, monkeypatch):

          monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')

          r = client.post('/api/v1.0/waivers/', content_type='application/json')

          assert r.status_code == 401

          assert r.headers.get('www-authenticate') == 'Negotiate'

  

-     @mock.patch('kerberos.authGSSServerInit', return_value=(kerberos.AUTH_GSS_COMPLETE, object()))

-     @mock.patch('kerberos.authGSSServerStep', return_value=kerberos.AUTH_GSS_COMPLETE)

-     @mock.patch('kerberos.authGSSServerResponse', return_value='STOKEN')

-     @mock.patch('kerberos.authGSSServerUserName', return_value='foo@EXAMPLE.ORG')

-     @mock.patch('kerberos.authGSSServerClean')

-     @mock.patch('kerberos.getServerPrincipalDetails')

-     def test_authorized(self, principal, clean, name, response, step, init,

-                         client, monkeypatch, session):

+     @mock.patch.multiple("gssapi.SecurityContext", complete=True,

+                          __init__=mock.Mock(return_value=None),

+                          step=mock.Mock(return_value=b"STOKEN"),

+                          initiator_name="foo@EXAMPLE.ORG")

+     @mock.patch.multiple("gssapi.Credentials",

+                          __init__=mock.Mock(return_value=None),

+                          __new__=mock.Mock(return_value=None))

+     def test_authorized(self, client, monkeypatch):

          monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')

          data = {

              'subject': {'subject.test': 'subject'},
@@ -44,11 +34,13 @@ 

              'waived': True,

              'comment': 'it broke',

          }

+         headers = {'Authorization':

+                    'Negotiate %s' % b64encode(b"CTOKEN").decode()}

          r = client.post('/api/v1.0/waivers/', data=json.dumps(data),

-                         content_type='application/json',

-                         headers={'Authorization': 'Negotiate CTOKEN'})

+                         content_type='application/json', headers=headers)

          assert r.status_code == 201

-         assert r.headers.get('WWW-Authenticate') == 'negotiate STOKEN'

+         assert r.headers.get('WWW-Authenticate') == \

+             'negotiate %s' % b64encode(b"STOKEN").decode()

          res_data = json.loads(r.data.decode('utf-8'))

          assert res_data['username'] == 'foo'

  

file modified
+5 -5
@@ -149,8 +149,8 @@ 

  

  

  def test_oidc_auth_is_enabled(tmpdir):

-     # Skip if waiverdb is rebuilt for an environment where Kerberos authentication

-     # is used and python-openidc-client is not available.

+     # Skip if waiverdb is rebuilt for an environment where GSSAPI

+     # authentication is used and python-openidc-client is not available.

      pytest.importorskip('openidc_client')

      with patch('openidc_client.OpenIDCClient.send_request') as mock_oidc_req:

          mock_rv = Mock()
@@ -197,10 +197,10 @@ 

          assert result.output == 'Created waiver 15 for result with subject {"subject.test": "test", "s": "t"} and testcase test.testcase\n' # noqa

  

  

- def test_kerberos_is_enabled(tmpdir):

+ def test_gssapi_is_enabled(tmpdir):

      # Skip if waiverdb is rebuilt for an environment where OIDC authentication

-     # is used and python-requests-kerberos is not available.

-     pytest.importorskip('requests_kerberos')

+     # is used and python-requests-gssapi is not available.

+     pytest.importorskip('requests_gssapi')

      with patch('requests.request') as mock_request:

          mock_rv = Mock()

          mock_rv.json.return_value = {

file modified
+4 -4
@@ -20,7 +20,7 @@ 

  BuildRequires:  python3-flask-restful

  BuildRequires:  python3-flask-sqlalchemy

  BuildRequires:  python3-psycopg2

- BuildRequires:  python3-kerberos

+ BuildRequires:  python3-gssapi

  BuildRequires:  python3-systemd

  BuildRequires:  python3-pytest

  BuildRequires:  python3-mock
@@ -37,7 +37,7 @@ 

  BuildRequires:  python-flask-restful

  BuildRequires:  python-flask-sqlalchemy

  BuildRequires:  python-psycopg2

- BuildRequires:  python-kerberos

+ BuildRequires:  python-gssapi

  BuildRequires:  systemd-python

  BuildRequires:  pytest

  BuildRequires:  python-mock
@@ -57,7 +57,7 @@ 

  Requires:       python3-flask-restful

  Requires:       python3-flask-sqlalchemy

  Requires:       python3-psycopg2

- Requires:       python3-kerberos

+ Requires:       python3-gssapi

  Requires:       python3-systemd

  Requires:       python3-mock

  Requires:       python3-flask-oidc
@@ -71,7 +71,7 @@ 

  Requires:       python-flask-restful

  Requires:       python-flask-sqlalchemy

  Requires:       python-psycopg2

- Requires:       python-kerberos

+ Requires:       python-gssapi

  Requires:       systemd-python

  Requires:       python-mock

  Requires:       python-flask-oidc

file modified
+38 -73
@@ -1,85 +1,50 @@ 

  # SPDX-License-Identifier: GPL-2.0+

  

  

- import os

- import kerberos

+ import base64

+ import gssapi

  from flask import current_app, Response, g

- # Starting with Flask 0.9, the _app_ctx_stack is the correct one,

- # before that we need to use the _request_ctx_stack.

- try:

-     from flask import _app_ctx_stack as stack

- except ImportError:

-     from flask import _request_ctx_stack as stack

  from socket import gethostname

  from werkzeug.exceptions import Unauthorized, Forbidden

  

  

  # Inspired by https://github.com/mkomitee/flask-kerberos/blob/master/flask_kerberos.py

- class KerberosAuthenticate(object):

+ # Later cleaned and ported to python-gssapi

+ def process_gssapi_request(token):

+     if current_app.config['KERBEROS_HTTP_HOST']:

+         hostname = current_app.config['KERBEROS_HTTP_HOST']

+     else:

+         hostname = gethostname()

+ 

+     service_name = gssapi.Name("HTTP@%s" % hostname,

+                                gssapi.NameType.hostbased_service)

+ 

+     try:

+         stage = "initialize server context"

+         creds = gssapi.Credentials(name=service_name, usage="accept")

+         sc = gssapi.SecurityContext(usage="accept", creds=creds)

+ 

+         stage = "step context"

+         token = sc.step(token if token != "" else None)

+         token = token if token is not None else ""

  

-     def __init__(self):

-         if current_app.config['KERBEROS_HTTP_HOST']:

-             hostname = current_app.config['KERBEROS_HTTP_HOST']

-         else:

-             hostname = gethostname()

-         self.service_name = "HTTP@%s" % (hostname)

-         if 'KRB5_KTNAME' in os.environ:

-             try:

-                 principal = kerberos.getServerPrincipalDetails('HTTP', hostname)

-             except kerberos.KrbError as exc:

-                 raise Unauthorized("Authentication Kerberos Failure: %s" % exc.message[0])

-             else:

-                 current_app.logger.debug("Kerberos: server is identifying as %s" % principal)

-         else:

-             raise Unauthorized("Kerberos: set KRB5_KTNAME to your keytab file")

+         # The current architecture cannot support continuation here

+         stage = "checking completion"

+         if not sc.complete:

+             current_app.logger.error(

+                 'Multiple GSSAPI round trips not supported')

+             raise Forbidden("Attempted multiple GSSAPI round trips")

  

-     def _gssapi_authenticate(self, token):

-         '''

-         Performs GSSAPI Negotiate Authentication

-         On success also stashes the server response token for mutual authentication

-         at the top of request context with the name kerberos_token, along with the

-         authenticated user principal with the name kerberos_user.

-         '''

-         state = None

-         ctx = stack.top

-         try:

-             rc, state = kerberos.authGSSServerInit(self.service_name)

-             if rc != kerberos.AUTH_GSS_COMPLETE:

-                 current_app.logger.error('Unable to initialize server context')

-                 return None

-             rc = kerberos.authGSSServerStep(state, token)

-             if rc == kerberos.AUTH_GSS_COMPLETE:

-                 current_app.logger.debug('Completed GSSAPI negotiation')

-                 ctx.kerberos_token = kerberos.authGSSServerResponse(state)

-                 ctx.kerberos_user = kerberos.authGSSServerUserName(state)

-                 return rc

-             elif rc == kerberos.AUTH_GSS_CONTINUE:

-                 current_app.logger.debug('Continuing GSSAPI negotiation')

-                 return kerberos.AUTH_GSS_CONTINUE

-             else:

-                 current_app.logger.debug('Unable to step server context')

-                 return None

-         except kerberos.GSSError as e:

-             current_app.logger.error('Unable to authenticate: %s', e)

-             return None

-         finally:

-             if state:

-                 kerberos.authGSSServerClean(state)

+         current_app.logger.debug('Completed GSSAPI negotiation')

  

-     def process_request(self, token):

-         """

-         Authenticates the current request using Kerberos.

-         """

-         kerberos_user = None

-         kerberos_token = None

-         ctx = stack.top

-         rc = self._gssapi_authenticate(token)

-         if rc == kerberos.AUTH_GSS_COMPLETE:

-             kerberos_user = ctx.kerberos_user

-             kerberos_token = ctx.kerberos_token

-         elif rc != kerberos.AUTH_GSS_CONTINUE:

-             raise Forbidden("Invalid Kerberos ticket")

-         return kerberos_user, kerberos_token

+         stage = "getting remote user"

+         user = str(sc.initiator_name)

+         return user, token

+     except gssapi.exceptions.GSSError as e:

+         current_app.logger.error(

+             'Unable to authenticate: failed to %s: %s' %

+             (stage, e.gen_message()))

+         raise Forbidden("Authentication failed")

  

  

  def get_user(request):
@@ -107,11 +72,11 @@ 

              raise Unauthorized(response=response)

          header = request.headers.get("Authorization")

          token = ''.join(header.strip().split()[1:])

-         user, kerberos_token = KerberosAuthenticate().process_request(token)

+         user, token = process_gssapi_request(base64.b64decode(token))

          # remove realm

          user = user.split("@")[0]

-         if kerberos_token is not None:

-             headers = {'WWW-Authenticate': ' '.join(['negotiate', kerberos_token])}

+         headers = {'WWW-Authenticate': ' '.join(

+             ['negotiate', base64.b64encode(token).decode()])}

      elif current_app.config['AUTH_METHOD'] == 'SSL':

          # Nginx sets SSL_CLIENT_VERIFY and SSL_CLIENT_S_DN in request.environ

          # when doing SSL authentication.

file modified
+6 -4
@@ -151,17 +151,19 @@ 

          # Try to import this now so the user gets immediate feedback if

          # it isn't installed

          try:

-             import requests_kerberos  # noqa: F401

+             import requests_gssapi  # noqa: F401

          except ImportError:

-             raise click.ClickException('python-requests-kerberos needs to be installed')

-         auth = requests_kerberos.HTTPKerberosAuth(mutual_authentication=requests_kerberos.OPTIONAL)

+             raise click.ClickException(

+                 'python-requests-gssapi needs to be installed')

+         auth = requests_gssapi.HTTPKerberosAuth(

+             mutual_authentication=requests_gssapi.OPTIONAL)

          for data in data_list:

              resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')),

                                      data=json.dumps(data), auth=auth,

                                      headers={'Content-Type': 'application/json'},

                                      timeout=60)

              if resp.status_code == 401:

-                 raise click.ClickException('WaiverDB authentication using Kerberos failed. '

+                 raise click.ClickException('WaiverDB authentication using GSSAPI failed. '

                                             'Make sure you have a valid Kerberos ticket.')

              check_response(resp, data, data.get('result_id', None))

      elif auth_method == 'dummy':

These changes are part of Fedora's kerberos-in-python modernization efforts: https://fedoraproject.org/wiki/Changes/kerberos-in-python-modernization

The first commit migrates from requests_kerberos to requests_gssapi, which is a straightforward drop-in.

The second commit migrates from pykerberos to python-gssapi. This change is more involved: among other things, python-gssapi is object oriented, while pykerberos isn't. As part of this change, it is no longer mandatory to specify a keytab file, since the default of /etc/krb5.keytab will be used. (You still can if you want, of course.)

rebased onto 12925cf3b858f548ee668307ae3fa7c17fd240be

6 years ago

(addendum: I think the latest commits on master have broken the test suite; at the very least, it no longer works on my rawhide machine. My changes pass prior to rebase, of course.)

Pagure doesn't make it visible what the previous commit was you were using before you rebased. But I am guessing you missed the recent change on master which requires a local Postgres to run the tests. Without seeing what the actual failure is I can't help. But no, the tests are not currently broken.

Also, we have an existing issue about this: #55. I would rather get the Flask-Kerberos library fixed/enhanced as necessary, and switch (back) to using that, so that we don't have to keep carrying and enhancing this code here in our application. We have a basically identical copy of the same thing in Greenwave as well.

flask-kerberos isn't packaged for Fedora, so I'm not sure why that's preferable. It also uses python-kerberos, which makes the problem worse for me by introducing a new dependency on that into the distro. "Fixing" it to use python-gssapi (which probably entails forking it to make flask-gssapi) isn't something I'm thrilled about either, because unless upstream (or someone else) agrees to, it's another thing I have to maintain.

Regarding the tests: are there setup instructions somewhere? I can't find any, and it seems pretty clear that I don't have the database up - everything dies in connect with E OperationalError: (psycopg2.OperationalError) could not connect to server: No such file or directory

There are instructions for setting up Postgres in README.md. It's nothing special, the tests just want to create/drop databases so they assume superuser on a local Postgres.

https://pagure.io/waiverdb/c/3e56879bbc781f776af3a4e138401f803e20df08

There is also a dev guide in the docs, although I just noticed that it has fallen out of sync with the README. I will fix that.

The reason to prefer Flask-Kerberos is so that this code for handling Kerberos authentication only lives in one place, we wouldn't have to maintain it here and in Greenwave and in the various other projects our team has. And so that changes like this, porting from one set of Kerberos bindings to another, which has no real bearing on our application itself, can be done in that library instead of in every single application.

Why the big push to get rid of python-kerberos anyway? It is working fine right now. And is actively maintained by Apple with a Python 3 port. Does switching everything to GSSAPI bring any actual benefit? That might be why you are finding upstream projects reluctant to take these invasive patches which rewrite their entire Kerberos handling code for no observable benefit...

Thanks, not sure how I missed the stuff in the readme... anyway, all tests are green.

I haven't been finding reluctance; this is the first project that's given pushback about it :) Most seem to be enthusiastic because the interface is simpler and more idiomatic (it's actually object oriented!), brings in support for NTLM and other mechanisms without additional effort, and isn't beholden to Apple's bizarre notions of releasing (python-kerberos is ripped out of their calendar server tree).

python-kerberos, python-krbV, and python-requests-krb5 were marked deprecated in RHEL-7: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7-beta/html/7.5_release_notes/chap-red_hat_enterprise_linux-7.5_release_notes-deprecated_functionality and therefore "will likely not be supported in future major releases of this product and is not recommended for new deployments".

The "reluctance" I meant was with requests-kerberos and flask-kerberos.

I personally find neither the old code with python-kerberos nor the new code python-gssapi to be simple or easy to understand... and unfortunately we have no easy way to test this against a real KDC either. But anyway, if you say it works, then sure. :+1:

Sorry, this is conflicting because we just merged some changes for switching to Python 3. I think the conflict will just be around dependencies in the .spec. Would you mind rebasing again?

If you want tests against a real KDC, we have a library for that: https://github.com/pythongssapi/k5test (it takes a couple seconds at most to set up a test KDC for testing)

Anyway, rebased.

rebased onto 7e2bdb4f7bd5c9c312a83196d18430983920c53c

6 years ago

Thanks for the pointer about k5test, it looks very useful.

Flake8 warns that this variable is never used... Is it a bug?

There are a bunch of other miscellaneous flake8 complaints about unused imports etc too.

Fwiw, just replace it with gssapi.Name("HTTP@%s" % hostname, gssapi.NameType.hostbased_service)

Fixed imports.

A brief aside on the name: GSSAPI automatically uses an appropriate name from the keytab/ccache. However, it looks from the code like there's desire to run this in an environment where there might be multiple entries in the keytab (KERBEROS_HTTP_HOST), so I've made the override work. If this isn't desired, then that code can go away.

2 new commits added

  • Port from pykerberos to python-gssapi
  • Port CLI from requests-kerberos to requests-gssapi
6 years ago

Thanks for the update @rharwood.

I think we added the KERBEROS_HTTP_HOST setting because we anticipated deploying on a VM with a hostname like waiverdb.host.prod.eng.bos.redhat.com but users would access the app over a CNAME like waiverdb.engineering.redhat.com. But it turns out now we are deploying in Openshift instead, and all that is defeated on the client side by dns_canonicalize_hostname anyway.

And as you point out, with GSSAPI we don't need to actually know the hostname in advance, now we can just fill in all the possible hostnames in the keytab and let the library pick the right one. So we could probably just drop the KERBEROS_HTTP_HOST setting... But since we have it now, and we obey it, I guess we should keep it as is. We can revisit later down the track.

The unit tests are failing for me in TestGSSAPIAuthentication.test_authorized though...

Traceback (most recent call last):
  File "/home/dcallagh/work/waiverdb/waiverdb/auth.py", line 24, in process_gssapi_request
    creds = gssapi.Credentials(name=service_name, usage="accept")
  File "/usr/lib64/python3.6/site-packages/gssapi/creds.py", line 64, in __new__
    store=store)
  File "/usr/lib64/python3.6/site-packages/gssapi/creds.py", line 137, in acquire
    mechs, usage)
  File "gssapi/raw/creds.pyx", line 158, in gssapi.raw.creds.acquire_cred
gssapi.raw.misc.GSSError: Major (851968): Unspecified GSS failure.  Minor code may provide more information, Minor (2): Key table file '/etc/foo.keytab' not found

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/dcallagh/work/waiverdb/tests/test_auth.py", line 37, in test_authorized
    content_type='application/json', headers=headers)
[...]
  File "/home/dcallagh/work/waiverdb/waiverdb/api_v1.py", line 212, in post
    user, headers = waiverdb.auth.get_user(request)
  File "/home/dcallagh/work/waiverdb/waiverdb/auth.py", line 75, in get_user
    user, token = process_gssapi_request(base64.b64decode(token))
  File "/home/dcallagh/work/waiverdb/waiverdb/auth.py", line 46, in process_gssapi_request
    (stage, e.gen_msg()))
AttributeError: 'GSSError' object has no attribute 'gen_msg'

The gen_msg bit appears to be just a typo, should be gen_message. So I'm kind of glad we hit that exception handler. :-)

The actual failure seems to be just because it's missing a mock for gssapi.Credentials.

rebased onto bab46a9

6 years ago

Thanks, not sure why I wasn't hitting that. (Also rebased.)

Pull-Request has been merged by dcallagh

6 years ago