From b2cb9c129ec1eebba0c8a21157fff2f9b86b1a0d Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Sep 20 2018 08:55:15 +0000 Subject: Implement dynamic ACL checks This allows checking ACLs in git hooks rather than gitolite. The advantage is that there are no files to regenerate, and they also apply for pull request merges. Signed-off-by: Patrick Uiterwijk --- diff --git a/pagure/exceptions.py b/pagure/exceptions.py index 5ff4655..dcd569e 100644 --- a/pagure/exceptions.py +++ b/pagure/exceptions.py @@ -111,3 +111,9 @@ class PagureEncodingException(PagureException, ValueError): """ pass + + +class PagurePushDenied(PagureException): + """ Exception raised if a remote hook rejected a push """ + + pass diff --git a/pagure/hooks/__init__.py b/pagure/hooks/__init__.py index 7f567ef..d5467fc 100644 --- a/pagure/hooks/__init__.py +++ b/pagure/hooks/__init__.py @@ -21,6 +21,7 @@ from pagure.config import config as pagure_config from pagure.exceptions import FileNotFoundException import pagure.lib import pagure.lib.git +from pagure.lib.git_auth import get_git_auth_helper from pagure.lib.plugins import get_enabled_plugins @@ -243,7 +244,15 @@ class BaseHook(object): def run_project_hooks( - session, username, project, hooktype, repotype, repodir, changes + session, + username, + project, + hooktype, + repotype, + repodir, + changes, + is_internal, + pull_request, ): """ Function to run the hooks on a project @@ -264,9 +273,63 @@ def run_project_hooks( or post-receive changes (dict): A dict with keys being the ref to update, values being a tuple of (from, to). + is_internal (bool): Whether this push originated from Pagure internally + pull_request (model.PullRequest or None): The pull request whose merge + is initiating this hook run. """ debug = pagure_config.get("HOOK_DEBUG", False) + # First we run dynamic ACLs + authbackend = get_git_auth_helper() + + if ( + is_internal + and username == "pagure" + and repotype in ("tickets", "requests") + ): + if debug: + print("This is an internal push, dynamic ACL is pre-approved") + elif not authbackend.is_dynamic: + if debug: + print("Auth backend %s is static-only" % authbackend) + else: + if debug: + print( + "Checking push request against auth backend %s" % authbackend + ) + todeny = [] + for refname in changes: + change = changes[refname] + authresult = authbackend.check_acl( + session, + project, + username, + refname, + is_update=hooktype == "update", + revfrom=change[0], + revto=change[1], + is_internal=is_internal, + pull_request=pull_request, + repotype=repotype, + ) + if debug: + print( + "Auth result for ref %s: %s" + % (refname, "Accepted" if authresult else "Denied") + ) + if not authresult: + print( + "Denied push for ref '%s' for user '%s'" + % (refname, username) + ) + todeny.append(refname) + for toremove in todeny: + del changes[toremove] + if not changes: + print("All changes have been rejected") + sys.exit(1) + + # Now we run the hooks for plugins for plugin, _ in get_enabled_plugins(project): if not plugin.runner: if debug: @@ -368,7 +431,17 @@ def run_hook_file(hooktype): else: raise ValueError("Hook type %s not valid" % hooktype) + session = pagure.lib.create_session(pagure_config["DB_URL"]) + if not session: + raise Exception("Unable to initialize db session") + pushuser = os.environ.get("GL_USER") + is_internal = os.environ.get("internal", False) == "yes" + pull_request = None + if "pull_request_uid" in os.environ: + pull_request = pagure.lib.get_request_by_uid( + session, os.environ["pull_request_uid"] + ) if pagure_config.get("HOOK_DEBUG", False): print("Changes: %s" % changes) @@ -381,14 +454,18 @@ def run_hook_file(hooktype): repo, ) = pagure.lib.git.get_repo_info_from_path(gitdir) - session = pagure.lib.create_session(pagure_config["DB_URL"]) - if not session: - raise Exception("Unable to initialize db session") - project = pagure.lib._get_project( session, repo, user=username, namespace=namespace ) run_project_hooks( - session, pushuser, project, hooktype, repotype, gitdir, changes + session, + pushuser, + project, + hooktype, + repotype, + gitdir, + changes, + is_internal, + pull_request, ) diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 8e304aa..50b8c1d 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -974,20 +974,37 @@ class TemporaryClone(object): ) try: + _log.debug( + "Running a git push of %s to %s" + % (pushref, self._project.fullname) + ) env = os.environ.copy() env["GL_USER"] = username env.update(extra) - subprocess.check_output( + out = subprocess.check_output( ["git"] + opts + ["push", "origin", pushref], cwd=self.repopath, stderr=subprocess.STDOUT, env=env, ) + _log.debug("Output: %s" % out) except subprocess.CalledProcessError as ex: # This should never really happen, since we control the repos, but # this way, we can be sure to get the output logged - _log.exception("Error pushing. Output: %s", ex.output) - raise + remotes = [] + for line in ex.output.decode("utf-8").split("\n"): + if line.startswith("remote: "): + _log.debug("Remote: %s" % line) + remotes.append(line[len("remote: ") :].strip()) + if remotes: + _log.info("Remote rejected with: %s" % remotes) + raise pagure.exceptions.PagurePushDenied( + "Remote hook declined the push: %s" % "\n".join(remotes) + ) + else: + # Something else happened, pass the original + _log.exception("Error pushing. Output: %s", ex.output) + raise def _update_file_in_git( diff --git a/pagure/lib/git_auth.py b/pagure/lib/git_auth.py index c7a2f58..eed98e3 100644 --- a/pagure/lib/git_auth.py +++ b/pagure/lib/git_auth.py @@ -10,6 +10,7 @@ from __future__ import print_function, unicode_literals import abc +import json import logging import os import pkg_resources @@ -64,6 +65,8 @@ class GitAuthHelper(with_metaclass(abc.ABCMeta, object)): helper. """ + is_dynamic = False + @classmethod @abc.abstractmethod def generate_acls(self, project, group=None): @@ -106,6 +109,45 @@ class GitAuthHelper(with_metaclass(abc.ABCMeta, object)): """ pass + @classmethod + # This method can't be marked as abstract, since it's new and that would + # break backwards compatibility + def check_acl(cls, session, project, username, refname, **info): + """ This method is used in Dynamic Git Auth helpers to check acls. + + It is acceptable for implementations to print things, which will be + returned to the user. + + Please make sure to add a **kwarg in any implementation, even if + specific keyword arguments are added for the known fields, to make + sure your implementation remains working if new items are added. + + Args: + session (sqlalchemy.Session): Database session + project (model.Project): Project instance push is for + username (string): The name of the user trying to push + refname (string): The name of the ref being pushed to + Kwargs: + Extra arguments to help in deciding whether to approve or deny a + push. This may get additional possible values later on, but will + have at least: + - is_update (bool): Whether this is being run at the "update" hook + moment. See the return type notes to see the differences. + - revfrom (string): The commit hash the update is happening from. + - revto (string): The commit hash the update is happening to. + - pull_request (model.PullRequest or None): The PR that is trying + to be merged. + Returns (bool): Whether to allow this push. + If is_update is False and the ACL returns False, the entire push + is aborted. If is_update is True and the ACL returns True, only + a single ref update is blocked. So if you want to block just a + single ref from being updated, only return False if is_update + is True. + """ + raise NotImplementedError( + "check_acl on static Git Auth Backend called" + ) + def _read_file(filename): """ Reads the specified file and return its content. @@ -778,6 +820,8 @@ class Gitolite3Auth(Gitolite2Auth): class GitAuthTestHelper(GitAuthHelper): """ Simple test auth module to check the auth customization system. """ + is_dynamic = True + @classmethod def generate_acls(cls, project, group=None): """ Print a statement when called, useful for debugging, only. @@ -816,3 +860,28 @@ class GitAuthTestHelper(GitAuthHelper): ) print(out) return out + + @classmethod + def check_acl( + cls, session, project, username, refname, pull_request, **info + ): + testfile = pagure_config.get("TEST_AUTH_STATUS", None) + if not testfile or not os.path.exists(testfile): + # If we are not configured, we will assume allowed + return True + + with open(testfile, "r") as statusfile: + status = json.loads(statusfile.read()) + + if status is True or status is False: + return status + + # Other option would be a dict with ref->allow + # (with allow True, pronly), missing means False) + if refname not in status: + print("ref '%s' not in status" % refname) + return False + elif status[refname] is True: + return True + elif status[refname] == "pronly": + return pull_request is not None diff --git a/tests/__init__.py b/tests/__init__.py index d24842c..aeb0d2e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -73,6 +73,9 @@ PAGLOG = logging.getLogger('pagure') PAGLOG.setLevel(logging.CRITICAL) PAGLOG.handlers = [] +if 'PYTHONPATH' not in os.environ: + os.environ['PYTHONPATH'] = os.path.normpath(os.path.join(HERE, '../')) + CONFIG_TEMPLATE = """ GIT_FOLDER = '%(path)s/repos' ENABLE_DOCS = %(enable_docs)s @@ -94,6 +97,8 @@ CELERY_CONFIG = { "task_always_eager": True, #"task_eager_propagates": True, } +GIT_AUTH_BACKEND = '%(authbackend)s' +TEST_AUTH_STATUS = '%(path)s/testauth_status.json' REPOSPANNER_NEW_REPO = %(repospanner_new_repo)s REPOSPANNER_NEW_REPO_ADMIN_OVERRIDE = %(repospanner_admin_override)s REPOSPANNER_NEW_FORK = %(repospanner_new_fork)s @@ -377,6 +382,7 @@ class SimplePagureTest(unittest.TestCase): 'enable_tickets': True, 'tickets_folder': '%s/repos/tickets' % self.path, 'global_path': tests_state["path"], + 'authbackend': 'gitolite3', 'repospanner_gitport': '8443', 'repospanner_new_repo': 'None', @@ -451,6 +457,11 @@ class SimplePagureTest(unittest.TestCase): self.session.execute("SET FOREIGN_KEY_CHECKS = 1") self.session.commit() + def set_auth_status(self, value): + """ Set the return value for the test auth """ + with open(os.path.join(self.path, 'testauth_status.json'), 'w') as statusfile: + statusfile.write(six.u(json.dumps(value))) + def get_csrf(self, url='/new', output=None): """Retrieve a CSRF token from given URL.""" if output is None: @@ -498,6 +509,29 @@ class Modeltests(SimplePagureTest): tests_state["broker_client"].flushall() super(Modeltests, self).tearDown() + def create_project_full(self, projectname): + """ Create a project via the API. + + This makes sure that the repo is fully setup the way a normal new + project would be, with hooks and all setup. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'name': projectname, + 'description': 'A test repo', + } + + # Valid request + output = self.app.post( + '/api/0/new/', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Project "%s" created' % projectname} + ) + class FakeGroup(object): # pylint: disable=too-few-public-methods """ Fake object used to make the FakeUser object closer to the diff --git a/tests/test_pagure_lib_git_auth.py b/tests/test_pagure_lib_git_auth.py new file mode 100644 index 0000000..d6e0b6a --- /dev/null +++ b/tests/test_pagure_lib_git_auth.py @@ -0,0 +1,227 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015-2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + Patrick Uiterwijk + +""" + +from __future__ import unicode_literals + +__requires__ = ['SQLAlchemy >= 0.8'] + +import pkg_resources + +import datetime +import os +import shutil +import sys +import tempfile +import time +import unittest + +import pygit2 +import six +from mock import patch, MagicMock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure +import pagure.lib.git +import tests + +from pagure.lib.repo import PagureRepo + + +class PagureLibGitAuthtests(tests.Modeltests): + """ Tests for pagure.lib.git_auth """ + config_values = {'authbackend': 'test_auth'} + + def setUp(self): + super(PagureLibGitAuthtests, self).setUp() + + tests.create_projects(self.session) + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + self.create_project_full('hooktest') + + def test_edit_with_all_allowed(self): + """Tests that editing a file is possible if ACLs say allowed.""" + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + # Add some content to the git repo + tests.add_content_git_repo( + os.path.join(self.path, 'repos', 'hooktest.git')) + + data = { + 'content': 'foo\n bar\n baz', + 'commit_title': 'test commit', + 'commit_message': 'Online commits from the gure.lib.get', + 'email': 'bar@pingou.com', + 'branch': 'master', + 'csrf_token': self.get_csrf(), + } + + output = self.app.post( + '/hooktest/edit/master/f/sources', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Commits - hooktest - Pagure', output_text) + self.assertIn('test commit', output_text) + + # Check file after the commit + output = self.app.get('/hooktest/raw/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertEqual(output_text, 'foo\n bar\n baz') + + def test_edit_with_all_denied(self): + """Tests that editing a file is not possible if ACLs say denied.""" + self.set_auth_status(False) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + # Add some content to the git repo + tests.add_content_git_repo( + os.path.join(self.path, 'repos', 'hooktest.git')) + + output = self.app.get('/hooktest/edit/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + csrf_token = output_text.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + data = { + 'content': 'foo\n bar\n baz', + 'commit_title': 'test commit', + 'commit_message': 'Online commits from the gure.lib.get', + 'email': 'bar@pingou.com', + 'branch': 'master', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/hooktest/edit/master/f/sources', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + "Remote hook declined the push: " + "Denied push for ref 'refs/heads/master' for user 'pingou'\n" + "All changes have been rejected", + output_text + ) + + # Check file after the commit: + output = self.app.get('/hooktest/raw/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertEqual(output_text, 'foo\n bar') + + def test_edit_pr(self): + """Tests the ACLs if they only accept PRs.""" + self.set_auth_status({'refs/heads/master': 'pronly', + 'refs/heads/source': True}) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + # Add some content to the git repo + tests.add_content_git_repo( + os.path.join(self.path, 'repos', 'hooktest.git')) + + output = self.app.get('/hooktest/edit/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + csrf_token = output_text.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + # Try editing master branch, should fail (only PRs allowed) + data = { + 'content': 'foo\n bar\n baz', + 'commit_title': 'test commit', + 'commit_message': 'Online commits from the gure.lib.get', + 'email': 'bar@pingou.com', + 'branch': 'master', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/hooktest/edit/master/f/sources', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + "Remote hook declined the push: " + "Denied push for ref 'refs/heads/master' for user 'pingou'\n" + "All changes have been rejected", + output_text + ) + + # Change something in the "source" branch + data = { + 'content': 'foo\n bar\n baz', + 'commit_title': 'test commit', + 'commit_message': 'Online commits from the gure.lib.get', + 'email': 'bar@pingou.com', + 'branch': 'source', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/hooktest/edit/master/f/sources', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Commits - hooktest - Pagure', output_text) + self.assertIn('test commit', output_text) + + # Check file after the commit: + output = self.app.get('/hooktest/raw/source/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertEqual(output_text, 'foo\n bar\n baz') + + # Create the PRs + project = pagure.lib.get_authorized_project(self.session, 'hooktest') + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=project, + branch_from="source", + repo_to=project, + branch_to='master', + title='PR to master', + user='pingou', + ) + self.session.add(req) + self.session.commit() + + # Check file before the merge + output = self.app.get('/hooktest/raw/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertEqual(output_text, 'foo\n bar') + + # Try to merge (should work) + output = self.app.post( + '/hooktest/pull-request/1/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Overview - hooktest - Pagure', + output_text + ) + + # Check file after the merge + output = self.app.get('/hooktest/raw/master/f/sources') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertEqual(output_text, 'foo\n bar\n baz')