From 984d0f132e4727124d51db940637561827600fdb Mon Sep 17 00:00:00 2001 From: Slavek Kabrda Date: Nov 23 2018 12:44:02 +0000 Subject: Enable token authentication on internal endpoints. Fixes #4000 --- diff --git a/alembic/versions/6a3ed02ee160_new_api_token_internal_access_acl.py b/alembic/versions/6a3ed02ee160_new_api_token_internal_access_acl.py new file mode 100644 index 0000000..5cd5205 --- /dev/null +++ b/alembic/versions/6a3ed02ee160_new_api_token_internal_access_acl.py @@ -0,0 +1,29 @@ +"""empty message + +Revision ID: 6a3ed02ee160 +Revises: 9cb4580e269a +Create Date: 2018-11-22 14:36:59.024463 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '6a3ed02ee160' +down_revision = '9cb4580e269a' + + +def upgrade(): + """ Insert the new ACL into the database. """ + op.execute( + 'INSERT INTO acls ("name", "description", "created") ' + "VALUES ('internal_access', 'Access Pagure''s internal APIs', NOW());" + ) + + +def downgrade(): + """ Remove the added ACL from the database. """ + op.execute( + "REMOVE FROM acls WHERE name = 'internal_access';" + ) diff --git a/doc/configuration.rst b/doc/configuration.rst index 4355b16..5e23710 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -1699,6 +1699,20 @@ to sshd, in the same format as AuthorizedKeysFile (see "AUTHORIZED_KEYS FILE FORMAT" in sshd(8)). +SSH_ADMIN_TOKEN +~~~~~~~~~~~~~~~ + +If not set to ``None``, ``aclchecker`` and ``keyhelper`` will use this api +admin token to get authorized to internal endpoints that they use. The token +must have the ``internal_access`` ACL. + +This is useful when the IP address of sshd service is not predictable +(e.g. because of running in a distributed cloud environment) and so +it's not possible to use the ``IP_ALLOWED_INTERNAL`` address list. + +Defaults to: ``None`` + + SSH_COMMAND_REPOSPANNER ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/files/aclchecker.py b/files/aclchecker.py index 049b4bc..982de1a 100644 --- a/files/aclchecker.py +++ b/files/aclchecker.py @@ -67,7 +67,10 @@ if not gitdir.endswith(".git"): url = "%s/pv/ssh/checkaccess/" % pagure_config["APP_URL"] data = {"gitdir": gitdir, "username": remoteuser} -resp = requests.post(url, data=data) +headers = {} +if pagure_config.get("SSH_ADMIN_TOKEN"): + headers["Authorization"] = "token %s" % pagure_config["SSH_ADMIN_TOKEN"] +resp = requests.post(url, data=data, headers=headers) if not resp.status_code == 200: print( "Error during lookup request: status: %s" % resp.status_code, diff --git a/files/keyhelper.py b/files/keyhelper.py index 60ec0a9..2f5cf5a 100644 --- a/files/keyhelper.py +++ b/files/keyhelper.py @@ -65,7 +65,10 @@ url = "%s/pv/ssh/lookupkey/" % pagure_config["APP_URL"] data = {"search_key": fingerprint} if username_lookup: data["username"] = username -resp = requests.post(url, data=data) +headers = {} +if pagure_config.get("SSH_ADMIN_TOKEN"): + headers["Authorization"] = "token %s" % pagure_config["SSH_ADMIN_TOKEN"] +resp = requests.post(url, data=data, headers=headers) if not resp.status_code == 200: print( "Error during lookup request: status: %s" % resp.status_code, diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 6a65a53..2453d97 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -35,7 +35,7 @@ import pagure.lib.tasks # noqa: E402 from pagure.config import config as pagure_config # noqa: E402 from pagure.doc_utils import load_doc, modify_rst, modify_html # noqa: E402 from pagure.exceptions import APIError # noqa: E402 -from pagure.utils import authenticated # noqa: E402 +from pagure.utils import authenticated, check_api_acls # noqa: E402 _log = logging.getLogger(__name__) @@ -134,57 +134,6 @@ def get_request_data(): return flask.request.form or flask.request.get_json() or {} -def check_api_acls(acls, optional=False): - """ Checks if the user provided an API token with its request and if - this token allows the user to access the endpoint desired. - """ - if authenticated(): - return - - flask.g.token = None - flask.g.fas_user = None - token = None - token_str = None - - if "Authorization" in flask.request.headers: - authorization = flask.request.headers["Authorization"] - if "token" in authorization: - token_str = authorization.split("token", 1)[1].strip() - - token_auth = False - if token_str: - token = pagure.lib.query.get_api_token(flask.g.session, token_str) - if token and not token.expired: - flask.g.authenticated = True - if acls and set(token.acls_list).intersection(set(acls)): - token_auth = True - flask.g.fas_user = token.user - # To get a token, in the `fas` auth user must have signed - # the CLA, so just set it to True - flask.g.fas_user.cla_done = True - flask.g.token = token - flask.g.authenticated = True - elif not acls and optional: - token_auth = True - flask.g.fas_user = token.user - # To get a token, in the `fas` auth user must have signed - # the CLA, so just set it to True - flask.g.fas_user.cla_done = True - flask.g.token = token - flask.g.authenticated = True - elif optional: - return - - if not token_auth: - output = { - "error_code": APIERROR.EINVALIDTOK.name, - "error": APIERROR.EINVALIDTOK.value, - } - jsonout = flask.jsonify(output) - jsonout.status_code = 401 - return jsonout - - def api_login_required(acls=None): """ Decorator used to indicate that authentication is required for some API endpoint. diff --git a/pagure/default_config.py b/pagure/default_config.py index fde4590..59f7a17 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -327,6 +327,7 @@ ACLS = { "commit_flag": "Flag a commit", "fork_project": "Fork a project", "generate_acls_project": "Generate the Gitolite ACLs on a project", + "internal_access": "Access Pagure's internal APIs", "issue_assign": "Assign issue to someone", "issue_change_status": "Change the status of a ticket", "issue_comment": "Comment on a ticket", @@ -350,7 +351,11 @@ ACLS = { # List of ACLs which a regular user is allowed to associate to an API token # from the ACLs above -USER_ACLS = [key for key in ACLS.keys() if key != "generate_acls_project"] +USER_ACLS = [ + key + for key in ACLS.keys() + if key not in ["generate_acls_project", "internal_access"] +] # From the ACLs above lists which ones are tolerated to be associated with # an API token that isn't linked to a particular project. @@ -363,6 +368,7 @@ CROSS_PROJECT_ACLS = [ # ACLs with which admins are allowed to create project-less API tokens ADMIN_API_ACLS = [ + "internal_access", "issue_comment", "issue_create", "issue_change_status", @@ -551,6 +557,10 @@ SSH_KEYS_USERNAME_EXPECT = None SSH_KEYS_OPTIONS = ( 'restrict,command="/usr/libexec/pagure/aclchecker.py %(username)s"' ) +# If not set to None, aclchecker and keyhelper will use this api admin +# token to get authorized to internal endpoints that they use. The token +# must have the internal_access ACL. +SSH_ADMIN_TOKEN = None # ACL Checker options SSH_COMMAND_REPOSPANNER = ( diff --git a/pagure/internal/__init__.py b/pagure/internal/__init__.py index 23c01ff..8ede612 100644 --- a/pagure/internal/__init__.py +++ b/pagure/internal/__init__.py @@ -59,8 +59,13 @@ MERGE_OPTIONS = { } -def localonly(function): - """ Decorator used to check if the request is local or not. +def internal_access_only(function): + """ Decorator used to check if the request is iternal or not. + + The request must either come from one of the addresses listed + in IP_ALLOWED_INTERNAL or it must have the "Authentication" + header set to "token " and the token must + have "internal_access" ACL. """ @wraps(function) @@ -70,20 +75,24 @@ def localonly(function): ip_allowed = pagure_config.get( "IP_ALLOWED_INTERNAL", ["127.0.0.1", "localhost", "::1"] ) - if flask.request.remote_addr not in ip_allowed: + if "Authorization" in flask.request.headers: + res = pagure.utils.check_api_acls(acls=["internal_access"]) + if res: + return res + elif flask.request.remote_addr not in ip_allowed: _log.debug( - "IP: %s is not in the list of allowed IPs: %s" + "IP: %s is not in the list of allowed IPs: %s " + "and 'Authorization' header not provided" % (flask.request.remote_addr, ip_allowed) ) flask.abort(403) - else: - return function(*args, **kwargs) + return function(*args, **kwargs) return decorated_function @PV.route("/ssh/lookupkey/", methods=["POST"]) -@localonly +@internal_access_only def lookup_ssh_key(): """ Looks up an SSH key by search_key for keyhelper.py """ search_key = flask.request.form["search_key"] @@ -109,7 +118,7 @@ def lookup_ssh_key(): @PV.route("/ssh/checkaccess/", methods=["POST"]) -@localonly +@internal_access_only def check_ssh_access(): """ Determines whether a user has any access to the requested repo. """ gitdir = flask.request.form["gitdir"] @@ -158,7 +167,7 @@ def check_ssh_access(): @PV.route("/pull-request/comment/", methods=["PUT"]) -@localonly +@internal_access_only def pull_request_add_comment(): """ Add a comment to a pull-request. """ @@ -208,7 +217,7 @@ def pull_request_add_comment(): @PV.route("/ticket/comment/", methods=["PUT"]) -@localonly +@internal_access_only def ticket_add_comment(): """ Add a comment to an issue. """ diff --git a/pagure/utils.py b/pagure/utils.py index a523cda..8ef3d35 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -59,6 +59,60 @@ def api_authenticated(): ) +def check_api_acls(acls, optional=False): + """ Checks if the user provided an API token with its request and if + this token allows the user to access the endpoint desired. + """ + import pagure.api + import pagure.lib.query + + if authenticated(): + return + + flask.g.token = None + flask.g.fas_user = None + token = None + token_str = None + + if "Authorization" in flask.request.headers: + authorization = flask.request.headers["Authorization"] + if "token" in authorization: + token_str = authorization.split("token", 1)[1].strip() + + token_auth = False + if token_str: + token = pagure.lib.query.get_api_token(flask.g.session, token_str) + if token and not token.expired: + flask.g.authenticated = True + if acls and set(token.acls_list).intersection(set(acls)): + token_auth = True + flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have signed + # the CLA, so just set it to True + flask.g.fas_user.cla_done = True + flask.g.token = token + flask.g.authenticated = True + elif not acls and optional: + token_auth = True + flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have signed + # the CLA, so just set it to True + flask.g.fas_user.cla_done = True + flask.g.token = token + flask.g.authenticated = True + elif optional: + return + + if not token_auth: + output = { + "error_code": pagure.api.APIERROR.EINVALIDTOK.name, + "error": pagure.api.APIERROR.EINVALIDTOK.value, + } + jsonout = flask.jsonify(output) + jsonout.status_code = 401 + return jsonout + + def is_safe_url(target): # pragma: no cover """ Checks that the target url is safe and sending to the current website not some other malicious one. diff --git a/tests/test_pagure_admin.py b/tests/test_pagure_admin.py index c35a478..e5270a8 100644 --- a/tests/test_pagure_admin.py +++ b/tests/test_pagure_admin.py @@ -340,7 +340,7 @@ class PagureAdminAdminTokentests(tests.Modeltests): """ Test the do_info_admin_token function of pagure-admin. """ # Create an admin token to use conf.return_value = True - rinp.return_value = '1,3,4' + rinp.return_value = '2,4,5' args = munch.Munch({'user': 'pingou'}) pagure.cli.admin.do_create_admin_token(args) diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index f664c54..8feb4cf 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -713,7 +713,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): } ) - @patch('pagure.api.check_api_acls', MagicMock(return_value=None)) + @patch('pagure.utils.check_api_acls', MagicMock(return_value=None)) def test_api_new_issue_raise_db_error(self): """ Test the api_new_issue method of the flask api. """ tests.create_projects(self.session) @@ -3410,7 +3410,7 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): # is required item = pagure.lib.model.TokenAcl( token_id='pingou_foo', - acl_id=7, + acl_id=8, ) self.session.add(item) self.session.commit() diff --git a/tests/test_pagure_flask_internal.py b/tests/test_pagure_flask_internal.py index 29e9224..b2cd203 100644 --- a/tests/test_pagure_flask_internal.py +++ b/tests/test_pagure_flask_internal.py @@ -47,6 +47,33 @@ class PagureFlaskInternaltests(tests.Modeltests): pagure.config.config['GIT_FOLDER'] = os.path.join( self.path, 'repos') + @patch.dict('pagure.config.config', {'IP_ALLOWED_INTERNAL': []}) + def test_internal_access_only(self): + output = self.app.post('/pv/ssh/lookupkey/') + self.assertEqual(output.status_code, 403) + + # no internal IP addresses => will fail + output = self.app.post('/pv/ssh/lookupkey/') + self.assertEqual(output.status_code, 403) + # wrong token => will fail + output = self.app.post( + '/pv/ssh/lookupkey/', + headers={"Authorization": "token doesntexist"} + ) + self.assertEqual(output.status_code, 401) + # correct token => will work + pagure.lib.query.add_token_to_user( + self.session, None, ['internal_access'], 'pingou' + ) + token = pagure.lib.query.search_token( + self.session, acls=['internal_access'], user='pingou' + ) + output = self.app.post( + '/pv/ssh/lookupkey/', + headers={"Authorization": "token %s" % token[0].id} + ) + self.assertEqual(output.status_code, 400) + @patch('pagure.lib.notify.send_email') def test_pull_request_add_comment(self, send_email): """ Test the pull_request_add_comment function. """ @@ -121,7 +148,7 @@ class PagureFlaskInternaltests(tests.Modeltests): self.assertEqual(len(request.comments), 1) self.assertEqual(len(request.discussion), 1) - # Check the @localonly + # Check the @internal_access_only before = pagure.config.config['IP_ALLOWED_INTERNAL'][:] pagure.config.config['IP_ALLOWED_INTERNAL'] = [] @@ -199,7 +226,7 @@ class PagureFlaskInternaltests(tests.Modeltests): issue = repo.issues[0] self.assertEqual(len(issue.comments), 1) - # Check the @localonly + # Check the @internal_access_only pagure.config.config['IP_ALLOWED_INTERNAL'].remove(None) before = pagure.config.config['IP_ALLOWED_INTERNAL'][:] pagure.config.config['IP_ALLOWED_INTERNAL'] = [] @@ -288,7 +315,7 @@ class PagureFlaskInternaltests(tests.Modeltests): issue = repo.issues[0] self.assertEqual(len(issue.comments), 1) - # Check the @localonly + # Check the @internal_access_only before = pagure.config.config['IP_ALLOWED_INTERNAL'][:] pagure.config.config['IP_ALLOWED_INTERNAL'] = [] @@ -416,7 +443,7 @@ class PagureFlaskInternaltests(tests.Modeltests): issue = repo.issues[0] self.assertEqual(len(issue.comments), 2) - # Check the @localonly + # Check the @internal_access_only before = pagure.config.config['IP_ALLOWED_INTERNAL'][:] pagure.config.config['IP_ALLOWED_INTERNAL'] = [] diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index caa1452..5d5e3e8 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -4971,7 +4971,7 @@ index 0000000..fb7093d self.assertIn('Create a new token', output_text) self.assertEqual( output_text.count('