waiverdb

Clone

#332 SUPERUSERS follow PERMISSION_MAPPING
Closed 4 years ago by lholecek. Opened 4 years ago by lholecek.
lholecek/waiverdb superuser-permission  into  master

file modified
+25 
@@ -2,6 +2,31 @@ 
 

  Release Notes
 

  =============
 

  

+ WaiverDB 1.1.0
 

+ ==============
 

+ 

+ Released 15 May 2019
 

+ 

+ * Restrict waiver creation based on users/groups and testcase: introduce access
 

+   control based on the users/groups and the testcase.
 

+   Groups need to be defined in LDAP.
 

+   New configuration is required to enable this feature:
 

+ 

+   * ``PERMISSION_MAPPING``: dictionary with keys as regex applied to test cases
 

+     and values as dictionaries with "users" and "groups" allowed to submit waivers
 

+     for that matching test case.
 

+     If not specified, the feature is not enabled.
 

+   * ``LDAP_HOST`` and ``LDAP_BASE``: required to query the LDAP system.
 

+     Required if ``PERMISSION_MAPPING`` is defined.
 

+ 

+ * Add ``proxied_by`` in the CLI: in the API is possible to define a username on
 

+   whose behalf the caller is proxying the submittion of a waiver.
 

+   This change provides this possibility also in the CLI. This is updated also to
 

+   reflect the recent changes regarding the access control.
 

+ 

+ * Allow optional trailing slash for about endpoint: accessing "api/v1.0/about/"
 

+   won't give 404 anymore.
 

+ 

  WaiverDB 1.0.0
 

  ==============

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

  }
 

  def flagCommit(status, percent, comment) {
 

    withPagureCreds {
 

-     it.flagCommit(username: 'c3i-jenkins', uid: 'ci-post-merge', status: status,
 

+     it.flagCommit(username: 'c3i-jenkins', uid: "ci-post-merge-${env.WAIVERDB_GIT_COMMIT.substring(0, 7)}", status: status,
 

        url: env.BUILD_URL, percent: percent, comment: comment, commit: env.WAIVERDB_GIT_COMMIT)
 

    }
 

  }

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

  Flask
 

  Flask-RESTful!=0.3.6
 

  Flask-SQLAlchemy
 

+ flask-cors
 

  SQLAlchemy
 

  gssapi
 

  flask-oidc

file modified
+1 
@@ -31,6 +31,7 @@ 
 

        version=get_project_version(),
 

        description='An engine for storing waivers against test results.',
 

        long_description=README,
 

+       long_description_content_type="text/markdown",
 

        author='Red Hat, Inc.',
 

        author_email='qa-devel@lists.fedoraproject.org',
 

        license='GPLv2+',

file modified
+8 -11 
@@ -79,18 +79,15 @@ 
 

  

  

  @pytest.fixture()
 

- def enable_cors(app, monkeypatch):
 

-     monkeypatch.setitem(app.config, 'CORS_URL', 'https://bodhi.fedoraproject.org')
 

- 

- 

- @pytest.fixture()
 

  def enable_permission_mapping(app, monkeypatch):
 

-     monkeypatch.setitem(app.config, 'PERMISSION_MAPPING',
 

-                         {
 

-                             "^testcase1.*": {"groups": ["factory-2-0"], "users": []}, # noqa
 

-                             "^testcase2.*": {"groups": [], "users": ["foo"]}, # noqa
 

-                             "^testcase4.*": {"groups": [], "users": []} # noqa
 

-                         })
 

+     permission_mapping = {
 

+         "^testcase1.*": {"groups": ["factory-2-0"], "users": ["bodhi"]},
 

+         "^testcase2.*": {"groups": [], "users": ["bodhi", "foo"]},
 

+         "^testcase3.*": {"groups": [], "users": ["bodhi"]},
 

+         "^testcase4.*": {"groups": [], "users": []},
 

+         "^testcase5.*": {"groups": [], "users": ["foo"]},
 

+     }
 

+     monkeypatch.setitem(app.config, 'PERMISSION_MAPPING', permission_mapping)
 

  

  

  @pytest.fixture()

file modified
+19 -4 
@@ -119,8 +119,8 @@ 
 

                          content_type='application/json', headers=self.headers)
 

          res_data = json.loads(r.get_data(as_text=True))
 

          assert r.status_code == 401
 

-         assert res_data['message'] == ("You are not authorized to submit a waiver "
 

-                                        "for the test case testcase3")
 

+         assert res_data['message'] == \
 

+             "User foo is not authorized to waive test cases testcase3"
 

  

      @pytest.mark.usefixtures('enable_ldap_host')
 

      @pytest.mark.usefixtures('enable_ldap_base')
 
@@ -134,8 +134,8 @@ 
 

                          content_type='application/json', headers=self.headers)
 

          res_data = json.loads(r.get_data(as_text=True))
 

          assert r.status_code == 401
 

-         assert res_data['message'] == ("You are not authorized to submit a waiver "
 

-                                        "for the test case testcase3")
 

+         assert res_data['message'] == \
 

+             "User foo is not authorized to waive test cases testcase3"
 

  

      @pytest.mark.usefixtures('enable_ldap_host')
 

      @pytest.mark.usefixtures('enable_ldap_base')
 
@@ -162,6 +162,21 @@ 
 

      @pytest.mark.usefixtures('enable_ldap_host')
 

      @pytest.mark.usefixtures('enable_ldap_base')
 

      @mock.patch('waiverdb.auth.get_user', return_value=('bodhi', {}))
 

+     @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',
 

+                 return_value=(['factory-2-0', 'something-else']))
 

+     def test_superuser_has_no_permission(self, mocked_conn, mock_get_user, client, session):
 

+         self.data['testcase'] = 'testcase5'
 

+         self.data['username'] = 'foo'
 

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

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

+         res_data = json.loads(r.get_data(as_text=True))
 

+         assert r.status_code == 401
 

+         assert res_data['message'] == \
 

+             "User bodhi is not authorized to waive test cases testcase5"
 

+ 

+     @pytest.mark.usefixtures('enable_ldap_host')
 

+     @pytest.mark.usefixtures('enable_ldap_base')
 

+     @mock.patch('waiverdb.auth.get_user', return_value=('bodhi', {}))
 

      @mock.patch('requests.request',
 

                  side_effect=Unauthorized('You are not authorized to submit a waiver for the test '
 

                                           'case testcase3'))

file modified
+26 -56 
@@ -638,68 +638,38 @@ 
 

      assert output['auth_method'] == client.application.config['AUTH_METHOD']
 

  

  

- @pytest.mark.usefixtures('enable_cors')
 

- def test_cors_about(client, session):
 

-     r = client.get('/api/v1.0/about')
 

- 

-     assert 'Access-Control-Allow-Origin' in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Headers' in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Method' in list(r.headers.keys())
 

-     assert r.headers['Access-Control-Allow-Origin'] == 'https://bodhi.fedoraproject.org'
 

-     assert r.headers['Access-Control-Allow-Headers'] == 'Content-Type'
 

-     assert r.headers['Access-Control-Allow-Method'] == 'POST, OPTIONS'
 

- 

-     output = json.loads(r.get_data(as_text=True))
 

-     assert r.status_code == 200
 

-     assert output['version'] == __version__
 

- 

- 

- def test_no_cors_about(client, session):
 

-     r = client.get('/api/v1.0/about')
 

- 

-     assert 'Access-Control-Allow-Origin' not in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Headers' not in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Method' not in list(r.headers.keys())
 

- 

-     output = json.loads(r.get_data(as_text=True))
 

-     assert r.status_code == 200
 

-     assert output['version'] == __version__
 

- 

- 

- @pytest.mark.usefixtures('enable_cors')
 

- def test_cors_waivers(client, session):
 

-     for i in range(0, 3):
 

-         create_waiver(session, subject_type='koji_build', subject_identifier="%d" % i,
 

-                       testcase="case %d" % i, username='foo %d' % i,
 

-                       product_version='foo-%d' % i, comment='bla bla bla')
 

-     r = client.get('/api/v1.0/waivers/')
 

- 

-     assert 'Access-Control-Allow-Origin' in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Headers' in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Method' in list(r.headers.keys())
 

-     assert r.headers['Access-Control-Allow-Origin'] == 'https://bodhi.fedoraproject.org'
 

-     assert r.headers['Access-Control-Allow-Headers'] == 'Content-Type'
 

-     assert r.headers['Access-Control-Allow-Method'] == 'POST, OPTIONS'
 

+ def test_cors_good(client, session):
 

+     headers = {
 

+         'Access-Control-Request-Method': 'POST',
 

+         'Access-Control-Request-Headers': 'Content-Type',
 

+         'Origin': 'https://bodhi.fedoraproject.org',
 

+     }
 

+     r = client.options(
 

+         '/api/v1.0/waivers/',
 

+         content_type='Content-Type',
 

+         headers=headers
 

+     )
 

  

-     res_data = json.loads(r.get_data(as_text=True))
 

      assert r.status_code == 200
 

-     assert len(res_data['data']) == 3
 

+     assert r.headers.get('Access-Control-Allow-Origin') == 'https://bodhi.fedoraproject.org'
 

+     assert 'POST' in r.headers.get('Access-Control-Allow-Methods', '').split(', ')
 

  

  

- def test_no_cors_waivers(client, session):
 

-     for i in range(0, 3):
 

-         create_waiver(session, subject_type='koji_build', subject_identifier="%d" % i,
 

-                       testcase="case %d" % i, username='foo %d' % i,
 

-                       product_version='foo-%d' % i, comment='bla bla bla')
 

-     r = client.get('/api/v1.0/waivers/')
 

- 

-     assert 'Access-Control-Allow-Origin' not in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Headers' not in list(r.headers.keys())
 

-     assert 'Access-Control-Allow-Method' not in list(r.headers.keys())
 

+ def test_cors_bad(client, session):
 

+     headers = {
 

+         'Access-Control-Request-Method': 'POST',
 

+         'Access-Control-Request-Headers': 'Content-Type',
 

+         'Origin': 'localhost',
 

+     }
 

+     r = client.options(
 

+         '/api/v1.0/waivers/',
 

+         content_type='Content-Type',
 

+         headers=headers
 

+     )
 

  

-     res_data = json.loads(r.get_data(as_text=True))
 

      assert r.status_code == 200
 

-     assert len(res_data['data']) == 3
 

+     assert 'Access-Control-Allow-Origin' not in r.headers
 

+     assert 'Access-Control-Allow-Methods' not in r.headers
 

  

  

  @patch('waiverdb.auth.get_user', return_value=('foo', {}))

file modified
+4 -2 
@@ -1,4 +1,4 @@ 
 

- %global upstream_version 1.0.0
 

+ %global upstream_version 1.1.0
 

  

  %if 0%{?fedora} || 0%{?rhel} > 7
 

  %bcond_without server
 
@@ -9,7 +9,7 @@ 
 

  %endif
 

  

  Name:           waiverdb
 

- Version:        1.0.0
 

+ Version:        1.1.0
 

  Release:        1%{?dist}
 

  Summary:        Service for waiving results in ResultsDB
 

  License:        GPLv2+
 
@@ -31,6 +31,7 @@ 
 

  BuildRequires:  python3-sphinxcontrib-issuetracker
 

  BuildRequires:  python3-flask
 

  BuildRequires:  python3-sqlalchemy
 

+ BuildRequires:  python3-flask-cors
 

  BuildRequires:  python3-flask-restful
 

  BuildRequires:  python3-flask-sqlalchemy
 

  BuildRequires:  python3-psycopg2
 
@@ -47,6 +48,7 @@ 
 

  BuildRequires:  python3-six
 

  Requires:       python3-flask
 

  Requires:       python3-sqlalchemy
 

+ Requires:       python3-flask-cors
 

  Requires:       python3-flask-restful
 

  Requires:       python3-flask-sqlalchemy
 

  Requires:       python3-psycopg2

file modified
+1 -1 
@@ -1,2 +1,2 @@ 
 

  # SPDX-License-Identifier: GPL-2.0+
 

- __version__ = '1.0.0'
 

+ __version__ = '1.1.0'

file modified
+4 -2 
@@ -362,8 +362,8 @@ 
 

              raise Unauthorized(f'Couldn\'t find user {user} in LDAP')
 

          if set(group_membership) & set(allowed_groups):
 

              return True
 

-         raise Unauthorized(('You are not authorized to submit a waiver '
 

-                             f'for the test case {testcase}'))
 

+         raise Unauthorized(
 

+             f'User {user} is not authorized to waive test cases {testcase}')
 

  

      def _create_waiver(self, args, user):
 

          proxied_by = None
 
@@ -419,6 +419,8 @@ 
 

          if not args['testcase']:
 

              raise BadRequest({'testcase': 'Missing required parameter in the JSON body'})
 

  

+         if proxied_by:
 

+             self.verify_authorization(proxied_by, args['testcase'])
 

          self.verify_authorization(user, args['testcase'])
 

  

          # brew-build is an alias for koji_build

file modified
+15 -14 
@@ -8,6 +8,7 @@ 
 

      from urlparse import urlparse, urlunsplit
 

  

  from flask import Flask, current_app
 

+ from flask_cors import CORS
 

  from flask_migrate import Migrate
 

  from sqlalchemy import event
 

  from sqlalchemy.exc import ProgrammingError
 
@@ -23,6 +24,18 @@ 
 

  from waiverdb.monitor import db_hook_event_listeners
 

  

  

+ def enable_cors(app):
 

+     """
 

+     Enables CORS headers.
 

+     """
 

+     # backward compatibility with old CORS_URL option
 

+     cors_url = app.config.get('CORS_URL')
 

+     if cors_url:
 

+         app.config['CORS_ORIGINS'] = cors_url
 

+ 

+     CORS(app)
 

+ 

+ 

  def load_config(app):
 

      # Load default config, then override that with a config file
 

      if os.getenv('DEV') == 'true':
 
@@ -63,18 +76,6 @@ 
 

      app.config['SQLALCHEMY_DATABASE_URI'] = dburi
 

  

  

- def insert_headers(response):
 

-     """ Insert the CORS headers for the give response if there are any
 

-     configured for the application.
 

-     """
 

-     cors_url = current_app.config.get('CORS_URL')
 

-     if cors_url:
 

-         response.headers['Access-Control-Allow-Origin'] = cors_url
 

-         response.headers['Access-Control-Allow-Headers'] = 'Content-Type'
 

-         response.headers['Access-Control-Allow-Method'] = 'POST, OPTIONS'
 

-     return response
 

- 

- 

  # applicaiton factory http://flask.pocoo.org/docs/0.12/patterns/appfactories/
 

  def create_app(config_obj=None):
 

      app = Flask(__name__)
 
@@ -107,11 +108,11 @@ 
 

      app.add_url_rule('/healthcheck', view_func=healthcheck)
 

      register_event_handlers(app)
 

  

-     app.after_request(insert_headers)
 

- 

      # initialize DB event listeners from the monitor module
 

      app.before_first_request(db_hook_event_listeners)
 

  

+     enable_cors(app)
 

+ 

      return app

file modified
+2 
@@ -62,3 +62,5 @@ 
 

      OIDC_REQUIRED_SCOPE = 'waiverdb_scope'
 

      OIDC_RESOURCE_SERVER_ONLY = True
 

      SUPERUSERS = ['bodhi']
 

+ 

+     CORS_ORIGINS = 'https://bodhi.fedoraproject.org'

Signed-off-by: Lukas Holecek hluk@email.cz

:+1: seems reasonable to me.

Doesn't this mean we'll have to create double (or more) mappings for all permissions? One for the user who should be able to apply the waiver, and another for each service that might act as a proxy for that user (Bodhi, Errata Tool, CVP, CPaaS, etc). This seems like a lot of extra maintenance for little extra security.

I agree with Mike. Additionally if we merge this, "superusers" then should have a rule that says that they can waive everything, or it gets pretty hard to maintain the rules for the superusers too. Like in the case of bodhi, bodhi should have permission for everything, because potentially users can request for so many different waivers... that becomes basically every waiver.
And if we put such rule this code is pretty useless.
I see how that can become not maintainable once greenwave, and consequently waiverdb, gets more popular with time.

This PR is my reaction to: https://gitlab.cee.redhat.com/devops/factory2-openshift-templates/merge_requests/273

The user in SUPERUSERS list cannot waive anything if it hasn't permission to do so in PERMISSION_MAPPING.

Maybe I misunderstood something.

Allowing web sites to submit any waiver for anything in the permission mapping does not seem like a good idea to me. It makes the mapping bit redundant.

This PR is my reaction to: https://gitlab.cee.redhat.com/devops/factory2-openshift-templates/merge_requests/273

The user in SUPERUSERS list cannot waive anything if it hasn't permission to do so in PERMISSION_MAPPING.

Maybe I misunderstood something.

I replied earlier today on IRC about that and in the other PR. I guess I didn't explain myself.

Allowing web sites to submit any waiver for anything in the permission mapping does not seem like a good idea to me. It makes the mapping bit redundant.

I don't understand why it's such a big deal now that we have no control at all. This feature it's there since waiverdb exists. We are increasing the security here, not decreasing it.

I replied earlier today on IRC about that and in the other PR. I guess I didn't explain myself.

Do you mean following?

I mean that the superuser cannot waive anything for him/herself if he/she doesn't have permission.

Superuser can just look into permission mapping and pick other user who can waive the test case.

I don't understand why it's such a big deal now that we have no control at all. This feature it's there since waiverdb exists. We are increasing the security here, not decreasing it.

This was only used by Bodhi and when there was no permission mapping. If I can exploit web site which can do this I might be also able to waive anything. This can be mitigated by using the permission mapping for superusers.

But I'm OK with adding the new superuser if it's temporary workaround for bad CORS headers. The we can drop this PR ... SUPERUSER should be used only sparingly.

I understand the desire to not give a service access to do anything, but that's kind of what the idea of a SUPERUSER is. Assuming we do merge this, could we create a meaningful policy that would improve security? For example, Bodhi would essentially need to be able to waive anything, right? Are there other SUPERUSERS that should only be able to waive a subset of test cases? Could we prevent SUPERUSERs from waiving security-related tests?

Assuming we do merge this, could we create a meaningful policy that would improve security?

https://gitlab.cee.redhat.com/devops/factory2-openshift-templates/merge_requests/273#note_663604

For example, Bodhi would essentially need to be able to waive anything, right?

Not sure if this is happening but if it's really needed, then it could be:

PERMISSION_MAPPING = {
    r'':  {'users': ['bodhi']},
}

I replied earlier today on IRC about that and in the other PR. I guess I didn't explain myself.

Do you mean following?

I mean that the superuser cannot waive anything for him/herself if he/she doesn't have permission.

What I meant with that sentence is that if you run this as a superuser (let's say bodhi) and the superuser doesn't have an entry in PERMISSION_MAPPING, but user "pippo" does have permission in PERMiSSION_MAPPING then:
waiverdb-cli <parameters> --username=pippo
this works.

But if you run:
waiverdb <same parameters>
it doesn't work.

Instead it seems to me what you understood from my sentence is that permission mapping gets checked for both "bodhi" and "pippo".
I'm just explaining the sentence here, not trying to say this is right or wrong.

Superuser can just look into permission mapping and pick other user who can waive the test case.

I assume we trust "superuser" if it is in that list, otherwise we wouldn't put it/he/she there.
The same can happen if my (or your) own account gets compromised, there's no difference.

This was only used by Bodhi and when there was no permission mapping. If I can exploit web site which can do this I might be also able to waive anything. This can be mitigated by using the permission mapping for superusers.

But the point here is that if they should be able to act as "any" user, they should have a rule that says they have permission on everything. And that makes this code useless.

rebased onto 362cc4d
4 years ago

Pull-Request has been closed by lholecek
4 years ago

Closing. Probably not needed since SUPERUSERS solution is temporary.
Metadata
None
No Tags
Flags
c3i-jenkins
success (100%)
Build passed.
4 years ago
Changes Summary 12
+25 -0
file changed
docs/release-notes.rst
+1 -1
file changed
openshift/pipelines/templates/waiverdb-build.Jenkinsfile
+1 -0
file changed
requirements.txt
+1 -0
file changed
setup.py
+8 -11
file changed
tests/conftest.py
+19 -4
file changed
tests/test_access_control.py
+26 -56
file changed
tests/test_api_v10.py
+4 -2
file changed
waiverdb.spec
+1 -1
file changed
waiverdb/__init__.py
+4 -2
file changed
waiverdb/api_v1.py
+15 -14
file changed
waiverdb/app.py
+2 -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.