From 7d93e84a43d3fae889f9a6d5f84ecd11a2c32df9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 1/9] Add support for git push via http using basic auth relying on API token Basically this commit adds support for http(s) git pushes but when the user is prompted by git for an username and password, they should supply an API token instead of their actual password. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/default_config.py b/pagure/default_config.py index 710581d..96bc476 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -354,6 +354,7 @@ ACLS = { "update_watch_status": "Update the watch status on a project", "pull_request_rebase": "Rebase a pull-request", "tag_project": "Allows adding git tags to a project", + "commit": "Commit to a git repository via http(s)", } # List of ACLs which a regular user is allowed to associate to an API token @@ -372,6 +373,7 @@ CROSS_PROJECT_ACLS = [ "modify_project", "update_watch_status", "pull_request_create", + "commit", ] # ACLs with which admins are allowed to create project-less API tokens diff --git a/pagure/lib/git_auth.py b/pagure/lib/git_auth.py index cd5a2e5..97a3abc 100644 --- a/pagure/lib/git_auth.py +++ b/pagure/lib/git_auth.py @@ -897,6 +897,9 @@ class PagureGitAuth(GitAuthHelper): self.info("Pull request required") return False + if username is None: + return False + # Determine whether the current user is allowed to push is_committer = is_repo_committer(project, username, session) deploykey = lookup_deploykey(project, username) diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index c70149e..7cb4004 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -10,6 +10,7 @@ from __future__ import unicode_literals, absolute_import +import base64 import logging import subprocess import tempfile @@ -33,6 +34,33 @@ from pagure.ui import UI_NS _log = logging.getLogger(__name__) +def _get_remote_user(): + """ Returns the remote user using either the content of + ``flask.g.remote_user`` or checking the headers for ``Authorization`` + and check if the provided API token is valid. + """ + remote_user = flask.request.remote_user + + if not remote_user: + # Check the headers + if "Authorization" in flask.request.headers: + auth = flask.request.headers["Authorization"] + if "Basic" in auth: + auth_token = auth.split("Basic ", 1)[-1] + info = base64.b64decode(auth_token).decode("utf-8") + if ":" in info: + _, token_str = info.split(":") + token = pagure.lib.query.get_api_token( + flask.g.session, token_str + ) + if token: + if not token.expired: + flask.g.authenticated = True + remote_user = token.user.username + + return remote_user + + def proxy_raw_git(): """ Proxy a request to Git or gitolite3 via a subprocess. @@ -40,13 +68,14 @@ def proxy_raw_git(): is not on repoSpanner. """ _log.debug("Raw git clone proxy started") + remote_user = _get_remote_user() # We are going to shell out to gitolite-shell. Prepare the env it needs. gitenv = { "PATH": os.environ["PATH"], # These are the vars git-http-backend needs "PATH_INFO": flask.request.path, - "REMOTE_USER": flask.request.remote_user, - "USER": flask.request.remote_user, + "REMOTE_USER": remote_user, + "USER": remote_user, "REMOTE_ADDR": flask.request.remote_addr, "CONTENT_TYPE": flask.request.content_type, "QUERY_STRING": flask.request.query_string, @@ -74,8 +103,8 @@ def proxy_raw_git(): "HOME": pagure_config["GITOLITE_HOME"], } ) - elif flask.request.remote_user: - gitenv.update({"GL_USER": flask.request.remote_user}) + elif remote_user: + gitenv.update({"GL_USER": remote_user}) # These keys are optional for key in ( @@ -236,9 +265,21 @@ def clone_proxy(project, username=None, namespace=None): if not pagure_config["ALLOW_HTTP_PUSH"]: # Pushing (git-receive-pack) over HTTP is not allowed flask.abort(403, description="HTTP pushing disabled") - if not flask.request.remote_user: + + remote_user = _get_remote_user() + if not remote_user: # Anonymous pushing... nope - flask.abort(403, description="Unauthenticated push not allowed") + headers = { + "WWW-Authenticate": 'Basic realm="pagure"', + "X-Frame-Options": "DENY", + } + response = flask.Response( + response="Authorization Required", + status=401, + headers=headers, + content_type="text/plain", + ) + flask.abort(response) project = pagure.lib.query.get_authorized_project( flask.g.session, @@ -250,7 +291,7 @@ def clone_proxy(project, username=None, namespace=None): if not project: _log.info( "%s could not find project: %s for user %s and namespace %s", - flask.request.remote_user, + remote_user, project, username, namespace, diff --git a/pagure/utils.py b/pagure/utils.py index 3225d3b..2bab9c5 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -50,7 +50,13 @@ def set_up_logging(app=None, force=False): def authenticated(): """ Utility function checking if the current user is logged in or not. """ - return hasattr(flask.g, "fas_user") and flask.g.fas_user is not None + fas_user = None + try: + fas_user = flask.g.fas_user + except (RuntimeError, AttributeError): + pass + + return fas_user is not None def api_authenticated(): diff --git a/tests/test_pagure_flask_ui_clone.py b/tests/test_pagure_flask_ui_clone.py index b3d3803..548c117 100644 --- a/tests/test_pagure_flask_ui_clone.py +++ b/tests/test_pagure_flask_ui_clone.py @@ -105,8 +105,8 @@ class PagureFlaskAppClonetests(tests.Modeltests): output = self.app.get( "/clonetest.git/info/refs?service=git-receive-pack" ) - self.assertEqual(output.status_code, 403) - self.assertIn("Unauthenticated push", output.get_data(as_text=True)) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) @patch.dict("pagure.config.config", {"ALLOW_HTTP_PULL_PUSH": True}) def test_http_clone_private_project_unauthed(self): diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index a675fc5..a922128 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -5643,6 +5643,7 @@ foo bar self.assertEqual( sorted([a.name for a in acls]), [ + "commit", "commit_flag", "create_branch", "create_project", From f2bbf935368c95febec70503d88c597101e5c9f2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 2/9] Move the logic checking if the username and password are valid to pagure.lib This will allow to re-use this code more easily in other part of the project. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/login.py b/pagure/lib/login.py index df45139..e34363c 100644 --- a/pagure/lib/login.py +++ b/pagure/lib/login.py @@ -20,12 +20,13 @@ except ImportError: random = random.SystemRandom() random_choice = random.choice + import string import hashlib import bcrypt import six -import pagure +import pagure.config from pagure.lib import model from cryptography.hazmat.primitives import constant_time @@ -112,3 +113,47 @@ def check_password(entered_password, user_password, seed=None): ) return constant_time.bytes_eq(password, user_password) + + +def check_username_and_password(session, username, password): + """ Check if the provided username and password match what is in the + database and raise an pagure.exceptions.PagureException if that is + not the case. + """ + + user_obj = pagure.lib.query.search_user(session, username=username) + if not user_obj: + raise pagure.exceptions.PagureException( + "Username or password invalid." + ) + + try: + password_checks = check_password( + password, + user_obj.password, + seed=pagure.config.config.get("PASSWORD_SEED", None), + ) + except pagure.exceptions.PagureException: + raise pagure.exceptions.PagureException( + "Username or password invalid." + ) + + if not password_checks: + raise pagure.exceptions.PagureException( + "Username or password invalid." + ) + + elif user_obj.token: + raise pagure.exceptions.PagureException( + "Invalid user, did you confirm the creation with the url " + "provided by email?" + ) + + else: + password = user_obj.password + if not isinstance(password, six.text_type): + password = password.decode("utf-8") + if not password.startswith("$2$"): + user_obj.password = generate_hashed_value(password) + session.add(user_obj) + session.flush() diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index 7cb4004..16c882b 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -252,6 +252,7 @@ def clone_proxy(project, username=None, namespace=None): flask.abort(403, description="HTTP pull/push is not allowed") service = None + remote_user = _get_remote_user() if flask.request.path.endswith("/info/refs"): service = flask.request.args.get("service") if not service: @@ -266,7 +267,6 @@ def clone_proxy(project, username=None, namespace=None): # Pushing (git-receive-pack) over HTTP is not allowed flask.abort(403, description="HTTP pushing disabled") - remote_user = _get_remote_user() if not remote_user: # Anonymous pushing... nope headers = { @@ -286,7 +286,7 @@ def clone_proxy(project, username=None, namespace=None): project, user=username, namespace=namespace, - asuser=flask.request.remote_user, + asuser=remote_user, ) if not project: _log.info( diff --git a/pagure/ui/login.py b/pagure/ui/login.py index a170104..1a0dbd2 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -15,11 +15,11 @@ import datetime import logging import flask -import six from sqlalchemy.exc import SQLAlchemyError from six.moves.urllib.parse import urljoin import pagure.login_forms as forms +import pagure.config import pagure.lib.login import pagure.lib.model as model import pagure.lib.model_base @@ -98,68 +98,41 @@ def do_login(): if form.validate_on_submit(): username = form.username.data - user_obj = pagure.lib.query.search_user( - flask.g.session, username=username - ) - if not user_obj: - flask.flash("Username or password invalid.", "error") - return flask.redirect(flask.url_for("auth_login")) - try: - password_checks = check_password( - form.password.data, - user_obj.password, - seed=pagure.config.config.get("PASSWORD_SEED", None), + pagure.lib.login.check_username_and_password( + flask.g.session, username, form.password.data ) - except pagure.exceptions.PagureException as err: - _log.exception(err) - flask.flash("Username or password of invalid format.", "error") + except pagure.exceptions.PagureException as ex: + _log.exception(ex) + flask.flash(str(ex), "error") return flask.redirect(flask.url_for("auth_login")) - if not password_checks: - flask.flash("Username or password invalid.", "error") - return flask.redirect(flask.url_for("auth_login")) - - elif user_obj.token: + user_obj = pagure.lib.query.search_user( + flask.g.session, username=username + ) + visit_key = pagure.lib.login.id_generator(40) + now = datetime.datetime.utcnow() + expiry = now + datetime.timedelta(days=30) + session = model.PagureUserVisit( + user_id=user_obj.id, + user_ip=flask.request.remote_addr, + visit_key=visit_key, + expiry=expiry, + ) + flask.g.session.add(session) + try: + flask.g.session.commit() + flask.g.fas_user = user_obj + flask.g.fas_session_id = visit_key + flask.g.fas_user.login_time = now + flask.flash("Welcome %s" % user_obj.username) + except SQLAlchemyError as err: # pragma: no cover flask.flash( - "Invalid user, did you confirm the creation with the url " - "provided by email?", + "Could not set the session in the db, " + "please report this error to an admin", "error", ) - return flask.redirect(flask.url_for("auth_login")) - - else: - password = user_obj.password - if not isinstance(password, six.text_type): - password = password.decode("utf-8") - if not password.startswith("$2$"): - user_obj.password = generate_hashed_value(form.password.data) - flask.g.session.add(user_obj) - flask.g.session.flush() - - visit_key = pagure.lib.login.id_generator(40) - now = datetime.datetime.utcnow() - expiry = now + datetime.timedelta(days=30) - session = model.PagureUserVisit( - user_id=user_obj.id, - user_ip=flask.request.remote_addr, - visit_key=visit_key, - expiry=expiry, - ) - flask.g.session.add(session) - try: - flask.g.session.commit() - flask.g.fas_user = user_obj - flask.g.fas_session_id = visit_key - flask.g.fas_user.login_time = now - flask.flash("Welcome %s" % user_obj.username) - except SQLAlchemyError as err: # pragma: no cover - flask.flash( - "Could not set the session in the db, " - "please report this error to an admin", - "error", - ) - _log.exception(err) + _log.exception(err) return flask.redirect(next_url) else: diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index c57209b..94e47f5 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -338,8 +338,7 @@ class PagureFlaskLogintests(tests.SimplePagureTest): output.get_data(as_text=True), ) self.assertIn( - "Username or password of invalid format.", - output.get_data(as_text=True), + "Username or password invalid.", output.get_data(as_text=True), ) # Check the password is still not of a known version From d9cdc958ca07c01c2acd058949e89e74013d25b6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 3/9] If pagure is set up for local auth, allow git push via https using it If pagure supports local authentication, let the users provide their username and password when doing a git push via http. If the username and password provided didn't match what is in the DB, fallback to check if the user isn't using an API token as for the other authentication systems. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index 16c882b..dcefd8b 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -49,14 +49,34 @@ def _get_remote_user(): auth_token = auth.split("Basic ", 1)[-1] info = base64.b64decode(auth_token).decode("utf-8") if ":" in info: - _, token_str = info.split(":") - token = pagure.lib.query.get_api_token( - flask.g.session, token_str - ) - if token: - if not token.expired: - flask.g.authenticated = True - remote_user = token.user.username + username, token_str = info.split(":") + auth = pagure_config.get("PAGURE_AUTH", None) + if auth == "local": + import pagure.lib.login + + try: + pagure.lib.login.check_username_and_password( + flask.g.session, username, token_str + ) + except pagure.exceptions.PagureException as ex: + _log.exception(ex) + else: + remote_user = username + + # We're doing a second check here, if the user/password + # approach above didn't work, the user may still be + # using an API token, so we want to check that as well. + if not remote_user: + token = pagure.lib.query.get_api_token( + flask.g.session, token_str + ) + if token: + if ( + not token.expired + and username == token.user.username + ): + flask.g.authenticated = True + remote_user = token.user.username return remote_user From a35c06a5863b2b006f8c346698864b123ea7a8a6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 4/9] Make the tests PagureFlaskApiIssuetests.test_api_assign_issue more robust It was relying on a hard-coded ACL id which is based on the order in which the ACL were loaded into the database. As new ACL are added this order may change (and do!) resulting in a weird failing test. With this change, we're directly retrieving the ACL id using the place this ACL has in the configuration file, making this much more robust. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 322e028..e590cd5 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -29,6 +29,7 @@ sys.path.insert( 0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") ) +import pagure.config import pagure.lib.query import tests @@ -3704,7 +3705,10 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): # Give `issue_change_status` to this token when `issue_comment` # is required - item = pagure.lib.model.TokenAcl(token_id="pingou_foo", acl_id=8) + acl_id = ( + sorted(pagure.config.config["ACLS"]).index("issue_comment") + 1 + ) + item = pagure.lib.model.TokenAcl(token_id="pingou_foo", acl_id=acl_id) self.session.add(item) self.session.commit() From f9a1d4c19103d1f77d7a542561c037c65c95791a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 5/9] Add tests for the http-base push Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/__init__.py b/tests/__init__.py index 8048fee..a3c3e19 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -58,6 +58,7 @@ import pagure.api from pagure.api.ci import jenkins import pagure.flask_app import pagure.lib.git +import pagure.lib.login import pagure.lib.model import pagure.lib.query import pagure.lib.tasks_mirror @@ -247,7 +248,7 @@ def create_user(session, username, fullname, emails): user = pagure.lib.model.User( user=username, fullname=fullname, - password=b"foo", + password=pagure.lib.login.generate_hashed_value("foo"), default_email=emails[0], ) session.add(user) diff --git a/tests/test_pagure_flask_ui_clone.py b/tests/test_pagure_flask_ui_clone.py index 548c117..6f0f4ab 100644 --- a/tests/test_pagure_flask_ui_clone.py +++ b/tests/test_pagure_flask_ui_clone.py @@ -10,6 +10,7 @@ from __future__ import unicode_literals, absolute_import +import base64 import datetime import unittest import shutil @@ -201,3 +202,117 @@ class PagureFlaskAppClonetests(tests.Modeltests): output_text = output.get_data(as_text=True) self.assertIn("# service=git-receive-pack", output_text) self.assertIn(" refs/heads/master\x00", output_text) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) + def test_http_push_api_token(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"pingou:aaabbbcccddd") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("# service=git-receive-pack", output_text) + self.assertIn(" refs/heads/master\x00", output_text) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) + def test_http_push_api_token_invalid_user(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"invalid:aaabbbcccddd") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) + def test_http_push_invalid_api_token(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"pingou:invalid_token") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + "PAGURE_AUTH": "local", + }, + ) + def test_http_push_local_auth(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" % base64.b64encode(b"pingou:foo") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("# service=git-receive-pack", output_text) + self.assertIn(" refs/heads/master\x00", output_text) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + "PAGURE_AUTH": "local", + }, + ) + def test_http_push_local_auth_invalid_username(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" % base64.b64encode(b"invalid:foo") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index 94e47f5..4fb3bb9 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -895,7 +895,7 @@ class PagureFlaskLogintests(tests.SimplePagureTest): ) data = { - "old_password": "foo", + "old_password": "bfoo", "password": "foo", "confirm_password": "foo", } @@ -958,7 +958,7 @@ class PagureFlaskLogintests(tests.SimplePagureTest): ) data = { - "old_password": "foo", + "old_password": "bfoo", "password": "foo", "confirm_password": "foo", } From e3afd215c89af5b88245b59d0e81071480985ac6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 6/9] Adjust the realm sent back based on the auth configured In an ideal world git would show the realm when asking for username and password, so in case the ideal world becomes reality, let's be nice and adjust the realm based on what we expect to be sent based on the authentication backend configured for this pagure instance. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index dcefd8b..f9f7e34 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -289,8 +289,11 @@ def clone_proxy(project, username=None, namespace=None): if not remote_user: # Anonymous pushing... nope + realm = "Pagure API token" + if pagure_config.get("PAGURE_AUTH") == "local": + realm = "Pagure password or API token" headers = { - "WWW-Authenticate": 'Basic realm="pagure"', + "WWW-Authenticate": 'Basic realm="%s"' % realm, "X-Frame-Options": "DENY", } response = flask.Response( From bde7ad75b191c7dbcc460a4463cc5fdf87cef6ca Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 08:45:42 +0000 Subject: [PATCH 7/9] Drop the proxying to git-http-backend via the apache configuration file Now that pagure has the capabilities to proxy to git-http-backend itself for both pull and push, we need to not let apache to this before pagure gets a chance to. Otherwise, pull will work, while push will not since the authorization is performed by pagure itself. Signed-off-by: Pierre-Yves Chibon --- diff --git a/files/pagure.conf b/files/pagure.conf index f6b6bea..cf61e7f 100644 --- a/files/pagure.conf +++ b/files/pagure.conf @@ -70,15 +70,6 @@ ## Section used to support cloning git repo over http (https in this case) #SetEnv GIT_PROJECT_ROOT /path/to/git/repositories - #AliasMatch ^/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$ /path/to/git/repositories/$1 - #AliasMatch ^/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$ /path/to/git/repositories/$1 - #ScriptAliasMatch \ - #"(?x)^/(.*/(HEAD | \ - #info/refs | \ - #objects/info/[^/]+ | \ - #git-(upload|receive)-pack))$" \ - #/usr/libexec/git-core/git-http-backend/$1 - # #WSGIProcessGroup pagure # From e2dba39364afdb189783aa5aabc554274a0c6b6a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 09:28:31 +0000 Subject: [PATCH 8/9] Check that the API token has the commit ACL Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index f9f7e34..b822675 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -74,6 +74,7 @@ def _get_remote_user(): if ( not token.expired and username == token.user.username + and "commit" in token.acls_list ): flask.g.authenticated = True remote_user = token.user.username diff --git a/tests/test_pagure_flask_ui_clone.py b/tests/test_pagure_flask_ui_clone.py index 6f0f4ab..1e25fb9 100644 --- a/tests/test_pagure_flask_ui_clone.py +++ b/tests/test_pagure_flask_ui_clone.py @@ -277,6 +277,31 @@ class PagureFlaskAppClonetests(tests.Modeltests): "ALLOW_HTTP_PULL_PUSH": True, "ALLOW_HTTP_PUSH": True, "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) + def test_http_push_invalid_acl_on_token(self): + """ Test that the HTTP push gets accepted. """ + tests.create_tokens(self.session, suffix="2") + tests.create_tokens_acl( + self.session, token_id="aaabbbcccddd2", acl_name="commit_flag" + ) + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"pingou:aaabbbcccddd2") + } + output = self.app.get( + "/test.git/info/refs?service=git-receive-pack", headers=headers, + ) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, "PAGURE_AUTH": "local", }, ) From 814bbf8f6c4a0d70e57fae1307c72cd208a4cfde Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 11 2020 09:28:31 +0000 Subject: [PATCH 9/9] Check if the API token is associated with the current project if any Basically API tokens can be project specific or project-less. In the case of project specific, we want to make sure that the API token is restricted to the project of interest, otherwise it defeats the purpose of this feature. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index b822675..e6f2750 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -34,7 +34,7 @@ from pagure.ui import UI_NS _log = logging.getLogger(__name__) -def _get_remote_user(): +def _get_remote_user(project): """ Returns the remote user using either the content of ``flask.g.remote_user`` or checking the headers for ``Authorization`` and check if the provided API token is valid. @@ -76,20 +76,28 @@ def _get_remote_user(): and username == token.user.username and "commit" in token.acls_list ): + if ( + project + and token.project + and token.project.fullname + != project.fullname + ): + return remote_user + flask.g.authenticated = True remote_user = token.user.username return remote_user -def proxy_raw_git(): +def proxy_raw_git(project): """ Proxy a request to Git or gitolite3 via a subprocess. This should get called after it is determined the requested project is not on repoSpanner. """ _log.debug("Raw git clone proxy started") - remote_user = _get_remote_user() + remote_user = _get_remote_user(project) # We are going to shell out to gitolite-shell. Prepare the env it needs. gitenv = { "PATH": os.environ["PATH"], @@ -273,7 +281,14 @@ def clone_proxy(project, username=None, namespace=None): flask.abort(403, description="HTTP pull/push is not allowed") service = None - remote_user = _get_remote_user() + # name it p1 so there is no risk of variable shadowing, we do not want + # this to be used elsewhere since there is no check here if the user + # is allowed to access this project (this is done lower down) + p1 = pagure.lib.query.get_authorized_project( + flask.g.session, project, user=username, namespace=namespace + ) + remote_user = _get_remote_user(p1) + if flask.request.path.endswith("/info/refs"): service = flask.request.args.get("service") if not service: @@ -325,7 +340,7 @@ def clone_proxy(project, username=None, namespace=None): if project.is_on_repospanner: return proxy_repospanner(project, service) else: - return proxy_raw_git() + return proxy_raw_git(project) def add_clone_proxy_cmds(): diff --git a/tests/test_pagure_flask_ui_clone.py b/tests/test_pagure_flask_ui_clone.py index 1e25fb9..11e9950 100644 --- a/tests/test_pagure_flask_ui_clone.py +++ b/tests/test_pagure_flask_ui_clone.py @@ -38,6 +38,10 @@ class PagureFlaskAppClonetests(tests.Modeltests): super(PagureFlaskAppClonetests, self).setUp() tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, "repos"), bare=True) + tests.add_content_git_repo( + os.path.join(self.path, "repos", "test.git") + ) tests.create_tokens(self.session) tests.create_tokens_acl(self.session) self.create_project_full("clonetest", {"create_readme": "y"}) @@ -219,6 +223,33 @@ class PagureFlaskAppClonetests(tests.Modeltests): % base64.b64encode(b"pingou:aaabbbcccddd") } output = self.app.get( + "/test.git/info/refs?service=git-receive-pack", headers=headers, + ) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("# service=git-receive-pack", output_text) + self.assertIn(" refs/heads/master\x00", output_text) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) + def test_http_push_projectless_api_token(self): + """ Test that the HTTP push gets accepted. """ + tests.create_tokens(self.session, project_id=None, suffix="2") + tests.create_tokens_acl( + self.session, token_id="aaabbbcccddd2", acl_name="commit" + ) + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"pingou:aaabbbcccddd2") + } + output = self.app.get( "/clonetest.git/info/refs?service=git-receive-pack", headers=headers, ) @@ -235,6 +266,28 @@ class PagureFlaskAppClonetests(tests.Modeltests): "HTTP_REPO_ACCESS_GITOLITE": None, }, ) + def test_http_push__invalid_project_for_api_token(self): + """ Test that the HTTP push gets accepted. """ + + headers = { + "Authorization": b"Basic %s" + % base64.b64encode(b"pingou:aaabbbcccddd") + } + output = self.app.get( + "/clonetest.git/info/refs?service=git-receive-pack", + headers=headers, + ) + self.assertEqual(output.status_code, 401) + self.assertIn("Authorization Required", output.get_data(as_text=True)) + + @patch.dict( + "pagure.config.config", + { + "ALLOW_HTTP_PULL_PUSH": True, + "ALLOW_HTTP_PUSH": True, + "HTTP_REPO_ACCESS_GITOLITE": None, + }, + ) def test_http_push_api_token_invalid_user(self): """ Test that the HTTP push gets accepted. """