waiverdb

Clone

#20 support OIDC authentication
Merged 7 years ago by mjia. Opened 7 years ago by mjia.
mjia/waiverdb oidc_authentication  into  master

file added
+11 
@@ -0,0 +1,11 @@ 
 

+ {
 

+     "web": {
 

+         "redirect_uris": ["http://localhost:5005/"],
 

+         "token_uri": "https://iddev.fedorainfracloud.org/openidc/Token",
 

+         "auth_uri": "https://iddev.fedorainfracloud.org/openidc/Authorization",
 

+         "client_id": "D-e69a1ac7-30fa-4d18-9001-7468c4f34c3c",
 

+         "client_secret": "qgz8Bzjg6nO7JWCXoB0o8L49KfI5atLF",

Is this file supposed to be committed? Whatever this secret is, it's not a secret anymore :-)

Is this more like, an example config for development purposes? In that case, it makes sense I guess... Will it work unmodified for anyone who wants to run it? And does it matter that we have now leaked this secret here?

mjia commented 7 years ago

Yes, it is used for development purpose. It should work for anyone who has a FAS account. There is nothing to be worried about as the secret is generated from the development server.

 
+         "userinfo_uri": "https://iddev.fedorainfracloud.org/openidc/UserInfo",
 

+         "token_introspection_uri": "https://iddev.fedorainfracloud.org/openidc/TokenInfo"
 

+     }
 

+ }

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

  Flask-SQLAlchemy
 

  SQLAlchemy
 

  kerberos >= 1.1.1
 

+ flask-oidc
 

  

+ # packages for the unit tests
 

  pytest >= 2.4.2
 

  mock

file added
+11 
@@ -0,0 +1,11 @@ 
 

+ {
 

+     "web": {
 

+         "redirect_uris": ["http://localhost:5005/"],
 

+         "token_uri": "https://iddev.fedorainfracloud.org/openidc/Token",
 

+         "auth_uri": "https://iddev.fedorainfracloud.org/openidc/Authorization",
 

+         "client_id": "randomid",
 

+         "client_secret": "nosecret",
 

+         "userinfo_uri": "https://iddev.fedorainfracloud.org/openidc/UserInfo",
 

+         "token_introspection_uri": "https://iddev.fedorainfracloud.org/openidc/TokenInfo"
 

+     }
 

+ }

file modified
+5 -1 
@@ -11,7 +11,6 @@ 
 

  #
 

  

  import pytest
 

- 

  from waiverdb.app import create_app, init_db
 

  

  
@@ -59,3 +58,8 @@ 
 

      """
 

      with app.test_client() as client:
 

          yield client
 

+ 

+ 

+ @pytest.fixture()
 

+ def enable_kerberos(app, monkeypatch):
 

+     monkeypatch.setitem(app.config, 'AUTH_METHOD', 'Kerberos')

file modified
+2 
@@ -23,10 +23,12 @@ 
 

  

  class NoZmqConfig(config.Config):
 

      ZEROMQ_PUBLISH = False
 

+     AUTH_METHOD = None
 

  

  

  class ZmqConfig(config.Config):
 

      ZEROMQ_PUBLISH = True
 

+     AUTH_METHOD = None
 

  

  

  @mock.patch('waiverdb.app.event.listen')

file modified
+47 
@@ -15,13 +15,20 @@ 
 

  import json
 

  from werkzeug.exceptions import Unauthorized
 

  import waiverdb.auth
 

+ import flask_oidc
 

  

  

+ @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)
 

  

      def test_unauthorized(self, client, monkeypatch):
 
@@ -52,3 +59,43 @@ 
 

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

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

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

+ 

+ 

+ class TestOIDCAuthentication(object):
 

+ 

+     def test_get_user_without_token(self, session):
 

+         with pytest.raises(Unauthorized) as excinfo:
 

+             request = mock.MagicMock()
 

+             waiverdb.auth.get_user(request)
 

+         assert "No 'Authorization' header found" in str(excinfo.value)
 

+ 

+     @mock.patch.object(flask_oidc.OpenIDConnect, '_get_token_info')
 

+     def test_get_user_with_invalid_token(self, mocked_get_token, session):
 

+         # http://vsbattles.wikia.com/wiki/Son_Goku
 

+         name = 'Son Goku'
 

+         mocked_get_token.return_value = {'active': False, 'username': name,
 

+                                          'scope': 'openid waiverdb_scope'}
 

+         headers = {'Authorization': 'Bearer invalid'}
 

+         request = mock.MagicMock()
 

+         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__
 

+         with pytest.raises(Unauthorized) as excinfo:
 

+             waiverdb.auth.get_user(request)
 

+         assert 'Token required but invalid' in str(excinfo.value)
 

+ 

+     @mock.patch.object(flask_oidc.OpenIDConnect, '_get_token_info')
 

+     def test_get_user_good(self, mocked_get_token, session):
 

+         # http://vsbattles.wikia.com/wiki/Son_Goku
 

+         name = 'Son Goku'
 

+         mocked_get_token.return_value = {'active': True, 'username': name,
 

+                                          'scope': 'openid waiverdb_scope'}
 

+         headers = {'Authorization': 'Bearer foobar'}
 

+         request = mock.MagicMock()
 

+         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__
 

+         user, header = waiverdb.auth.get_user(request)
 

+         assert user == name

file modified
+3 
@@ -18,6 +18,7 @@ 
 

  from waiverdb.logger import init_logging
 

  from waiverdb.api_v1 import api_v1
 

  from waiverdb.models import db
 

+ from flask_oidc import OpenIDConnect
 

  

  

  def load_default_config(app):
 
@@ -48,6 +49,8 @@ 
 

          raise Warning("You need to change the app.secret_key value for production")
 

      if app.config['SHOW_DB_URI']:
 

          app.logger.debug('using DBURI: %s' % app.config['SQLALCHEMY_DATABASE_URI'])
 

+     if app.config['AUTH_METHOD'] == 'OIDC':
 

+         app.oidc = OpenIDConnect(app)
 

      # initialize db
 

      db.init_app(app)
 

      # initialize logging

file modified
+22 -5 
@@ -9,9 +9,10 @@ 
 

  # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 

  # GNU General Public License for more details.
 

  

+ 

  import os
 

  import kerberos
 

- from flask import current_app, Response
 

+ 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:
 
@@ -93,12 +94,28 @@ 
 

  def get_user(request):
 

      user = None
 

      headers = dict()
 

-     if 'KRB5_KTNAME' in os.environ:
 

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

-         if not header:
 

+     if current_app.config['AUTH_METHOD'] == 'OIDC':
 

+         if 'Authorization' not in request.headers:
 

+             raise Unauthorized("No 'Authorization' header found.")
 

+         token = request.headers.get("Authorization").strip()
 

+         prefix = 'Bearer '
 

+         if not token.startswith(prefix):
 

+             raise Unauthorized('Authorization headers must start with %r' % prefix)
 

+         token = token[len(prefix):].strip()
 

+         required_scopes = [
 

+             'openid',
 

+             current_app.config['OIDC_REQUIRED_SCOPE'],
 

+         ]
 

+         validity = current_app.oidc.validate_token(token, required_scopes)
 

+         if validity is not True:
 

+             raise Unauthorized(validity)
 

+         user = g.oidc_token_info['username']
 

+     elif current_app.config['AUTH_METHOD'] == 'Kerberos':
 

+         if 'Authorization' not in request.headers:
 

              response = Response('Unauthorized', 401, {'WWW-Authenticate': 'Negotiate'})
 

              raise Unauthorized(response=response)
 

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

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

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

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

          # remove realm
 

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

file modified
+16 
@@ -9,6 +9,8 @@ 
 

  # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 

  # GNU General Public License for more details.
 

  

+ import os
 

+ 

  

  class Config(object):
 

      """
 
@@ -29,6 +31,7 @@ 
 

      # need to explicitly turn this off
 

      # https://github.com/flask-restful/flask-restful/issues/449
 

      ERROR_404_HELP = False
 

+     AUTH_METHOD = 'OIDC'  # Specify OIDC or Kerberos for authentication
 

      # Change it if the Kerberos service is not running on which the waiverdb is run.
 

      KERBEROS_HTTP_HOST = None
 

      ZEROMQ_PUBLISH = True
 
@@ -44,9 +47,22 @@ 
 

      TRAP_BAD_REQUEST_ERRORS = True
 

      SQLALCHEMY_DATABASE_URI = 'sqlite:////var/tmp/waiverdb_db.sqlite'
 

      SHOW_DB_URI = True
 

+     # The location of the client_secrets.json file used for API authentication
 

+     OIDC_CLIENT_SECRETS = os.path.join(
 

+         os.path.dirname(os.path.dirname(os.path.abspath(__file__))), 'conf',
 

+         'client_secrets.json'
 

+     )
 

+     OIDC_REQUIRED_SCOPE = 'https://waiverdb.fedoraproject.org/oidc/create-waiver'
 

+     OIDC_RESOURCE_SERVER_ONLY = True
 

  

  

  class TestingConfig(Config):
 

      SQLALCHEMY_TRACK_MODIFICATIONS = True
 

      TRAP_BAD_REQUEST_ERRORS = True
 

      TESTING = True
 

+     OIDC_CLIENT_SECRETS = os.path.join(
 

+         os.path.dirname(os.path.dirname(os.path.abspath(__file__))), 'tests',
 

+         'client_secrets.json'
 

+     )
 

+     OIDC_REQUIRED_SCOPE = 'waiverdb_scope'
 

+     OIDC_RESOURCE_SERVER_ONLY = True

no initial comment

rebased
7 years ago

Looks OK to me. +1

rebased
7 years ago

Is requests not pulled in by requests_oauthlib?

I just checked and it's pulled in, so I think you can just add this to the requirements.txt.

Is there a reason this doesn't use flask-oidc?

Is requests not pulled in by requests_oauthlib?
Yeah, not sure why tox complained that requests was missing last time.

Is there a reason this doesn't use flask-oidc?

It's because flask-oidc is not in EPEL7.
Edited 7 years ago by mjia

rebased
7 years ago

Well, we could either get flask-oidc into EPEL7 :-) or drop EPEL7 as a deployment target, and go for Fedora 25. I don't think anyone ever made any decision about that -- I was just always trying to target EPEL7 by default, because that's what Fedora infra prefers.

Just depends whether using flask-oidc is a lot nicer than not using it?

Well, we could either get flask-oidc into EPEL7 :-) or drop EPEL7 as a deployment target, and go for Fedora 25. I don't think anyone ever made any decision about that -- I was just always trying to target EPEL7 by default, because that's what Fedora infra prefers.
Just depends whether using flask-oidc is a lot nicer than not using it?

I think it would be nice to use it, then we have less code to maintain.

Would be nice to just refer to some other existing copy of this script (there must be many), rather than keeping our own copy committed here -- I am assuming there is nothing waiverdb specific in it.

Your first commit is making flake8 happy, not tox (maybe just tweak the commit message).

BTW if we are planning to keep flake8 happy we better get it into the Jenkinsfile otherwise we will keep accidentally regressing on it I'm sure.

Okay, I will take it out and probably move it to my own repo.
Edited 7 years ago by mjia

rebased
7 years ago

I would like to get this merged first as it is working. Then I will try out flask-oidc and see if it would make things nicer or not.

Ah ha.. python-flask-oidc has an epel7 branch: https://admin.fedoraproject.org/pkgdb/package/rpms/python-flask-oidc/

But there are no builds or updates for it: https://apps.fedoraproject.org/packages/python-flask-oidc/

The python-flask-oidc package is well-done and looks nearly ready for EPEL7 with py3 conditionals.

Here's a scratch build which shows that we're missing another dep: https://koji.fedoraproject.org/koji/taskinfo?taskID=18664993

That is python-oauth2client which also has an epel7 branch, but no build or update.

That missing build is tracked here. https://bugzilla.redhat.com/show_bug.cgi?id=1356121

I've applied for co-maintainership of both.

@mbaldessari and @skrzepto - do either of you mind if I help with EPEL7 branches for python-flask-oidc and python-oauth2client?

I've applied for rights:

Okay :-) so in that case, it seems like we should go ahead with this approach (not using flask-oidc) and we can revisit later in case flask-oidc makes things simpler for us.

Is this file supposed to be committed? Whatever this secret is, it's not a secret anymore :-)

Is this more like, an example config for development purposes? In that case, it makes sense I guess... Will it work unmodified for anyone who wants to run it? And does it matter that we have now leaked this secret here?

Seems like an unrelated change? What's the reason for it?

flask-oidc was built for EL7 as of yesterday.

Yes, it is used for development purpose. It should work for anyone who has a FAS account. There is nothing to be worried about as the secret is generated from the development server.

Yeah, I should move it into a separate patch as I was convinced by this patch in the mbs

https://pagure.io/fm-orchestrator/c/a4763ee316e23d89eabbfa55ad4f3655128bcbb0

flask-oidc was built for EL7 as of yesterday.

Cool, I'll give it go with flask-oidc.

rebased
7 years ago

LGTM, :+1:

rebased
7 years ago

rebased
7 years ago

@dcallagh any feedback as I want to get this one merged?

Sorry I didn't realise this was ready for review again. Pagure does not send any notifications when you amend the PR. @mjia in future we all better remember we have to write a comment in that case.

:+1: nice work!

Sorry I didn't realise this was ready for review again. Pagure does not send any notifications when you amend the PR. @mjia in future we all better remember we have to write a comment in that case.

I will add your email address in the public notifications and you should be able to receive a notification when a new comment is made.

Pull-Request has been merged by mjia
7 years ago

No I already get comments on the PRs.

The problem is 4 days ago you say "I'll try using flask-oidc", then 2 days ago you posted the amended version which uses it but you didn't write any comment at that point. So I didn't realise you had actually posted it.

Okay, I see what you mean. Pagure doesn't send any notification when the patch is reabased, sigh!
Metadata
None
No Tags
Changes Summary 9
+11
file added
conf/client_secrets.json
+2 -0
file changed
requirements.txt
+11
file added
tests/client_secrets.json
+5 -1
file changed
tests/conftest.py
+2 -0
file changed
tests/test_app.py
+47 -0
file changed
tests/test_auth.py
+3 -0
file changed
waiverdb/app.py
+22 -5
file changed
waiverdb/auth.py
+16 -0
file changed
waiverdb/config.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.