From 48664c3771583101ef444486ac30c50c92d84959 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 28 2020 10:11:15 +0000 Subject: [PATCH 1/4] Add a dedicated logger for everything that is auth related Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/default_config.py b/pagure/default_config.py index 96bc476..5ab85ff 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -460,6 +460,11 @@ LOGGING = { "class": "logging.StreamHandler", "stream": "ext://sys.stdout", }, + "auth_handler": { + "formatter": "standard", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout", + }, "email": { "level": "ERROR", "formatter": "email_format", @@ -480,6 +485,11 @@ LOGGING = { "level": "DEBUG", "propagate": True, }, + "pagure_auth": { + "handlers": ["auth_handler"], + "level": "DEBUG", + "propagate": False, + }, "flask": { "handlers": ["console"], "level": "INFO", diff --git a/pagure/internal/__init__.py b/pagure/internal/__init__.py index 3fa7edf..5013e6a 100644 --- a/pagure/internal/__init__.py +++ b/pagure/internal/__init__.py @@ -37,6 +37,7 @@ from pagure.config import config as pagure_config # noqa: E402 _log = logging.getLogger(__name__) +_auth_log = logging.getLogger("pagure.auth") MERGE_OPTIONS = { @@ -97,6 +98,11 @@ def lookup_ssh_key(): """ Looks up an SSH key by search_key for keyhelper.py """ search_key = flask.request.form["search_key"] username = flask.request.form.get("username") + _auth_log.info( + "User is trying to access pagure using the ssh key: %s -- " + "|user: %s|IP: %s|method: N/A|repo: N/A|query: N/A" + % (search_key, username, flask.request.remote_addr) + ) key = pagure.lib.query.find_ssh_key(flask.g.session, search_key, username) if not key: @@ -123,6 +129,11 @@ def check_ssh_access(): """ Determines whether a user has any access to the requested repo. """ gitdir = flask.request.form["gitdir"] remoteuser = flask.request.form["username"] + _auth_log.info( + "User is asking to access a project via ssh -- " + "|user: %s|IP: %s|method: N/A|repo: %s|query: N/A" + % (remoteuser, flask.request.remote_addr, gitdir) + ) # Build a fake path so we can use get_repo_info_from_path path = os.path.join(pagure_config["GIT_FOLDER"], gitdir) @@ -142,6 +153,12 @@ def check_ssh_access(): if repo is None: _log.info("Project name could not be extracted from path") + _auth_log.info( + "The path specified by the user could not be matched with a " + "project -- " + "|user: %s|IP: %s|method: N/A|repo: %s|query: N/A" + % (remoteuser, flask.request.remote_addr, gitdir) + ) return flask.jsonify({"access": False}) project = pagure.lib.query.get_authorized_project( @@ -153,6 +170,11 @@ def check_ssh_access(): ) if not project: + _auth_log.info( + "User tried to access a private project they don't have access " + "to -- |user: %s|IP: %s|method: N/A|repo: %s|query: N/A" + % (remoteuser, flask.request.remote_addr, gitdir) + ) _log.info("Project not found with this path") return flask.jsonify({"access": False}) _log.info("Checking ACLs on project: %s" % project.fullname) @@ -163,6 +185,11 @@ def check_ssh_access(): # Deploy keys are not allowed on ticket and PR repos but they are # allowed for main and docs repos. _log.info("%s is not a contributor to this project" % remoteuser) + _auth_log.info( + "User tried to access a projec they do not have access to -- " + "|user: %s|IP: %s|method: N/A|repo: %s|query: N/A" + % (remoteuser, flask.request.remote_addr, gitdir) + ) return flask.jsonify({"access": False}) _log.info("Access granted to %s on: %s" % (remoteuser, project.fullname)) diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index e6f2750..43debfe 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -32,6 +32,7 @@ from pagure.config import config as pagure_config from pagure.ui import UI_NS _log = logging.getLogger(__name__) +_auth_log = logging.getLogger("pagure.auth") def _get_remote_user(project): @@ -121,6 +122,17 @@ def proxy_raw_git(project): "HTTP_CONTENT_ENCODING": flask.request.content_encoding, } + _auth_log.info( + "Serving git to |user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + project.path, + flask.request.query_string, + ) + ) + gitolite = pagure_config["HTTP_REPO_ACCESS_GITOLITE"] if gitolite: gitenv.update( @@ -278,6 +290,16 @@ def clone_proxy(project, username=None, namespace=None): access to the attempted repository. """ if not pagure_config["ALLOW_HTTP_PULL_PUSH"]: + _auth_log.info( + "User tried to access the git repo via http but this is not " + "enabled -- |user: N/A|IP: %s|method: %s|repo: %s|query: %s" + % ( + flask.request.remote_addr, + flask.request.method, + project, + flask.request.query_string, + ) + ) flask.abort(403, description="HTTP pull/push is not allowed") service = None @@ -287,6 +309,9 @@ def clone_proxy(project, username=None, namespace=None): p1 = pagure.lib.query.get_authorized_project( flask.g.session, project, user=username, namespace=namespace ) + p1_path = "invalid repo" + if p1: + p1_path = p1.path remote_user = _get_remote_user(p1) if flask.request.path.endswith("/info/refs"): @@ -294,12 +319,45 @@ def clone_proxy(project, username=None, namespace=None): if not service: # This is a Git client older than 1.6.6, and it doesn't work with # the smart protocol. We do not support the old protocol via HTTP. + _auth_log.info( + "User is using a git client to old (pre-1.6.6) -- " + "|user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + p1_path, + flask.request.query_string, + ) + ) flask.abort(400, description="Please switch to newer Git client") if service not in ("git-upload-pack", "git-receive-pack"): + _auth_log.info( + "User asked for an unknown service " + "|user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + p1_path, + flask.request.query_string, + ) + ) flask.abort(400, description="Unknown service requested") if "git-receive-pack" in flask.request.full_path: if not pagure_config["ALLOW_HTTP_PUSH"]: + _auth_log.info( + "User tried a git push over http while this is not enabled -- " + "|user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + p1_path, + flask.request.query_string, + ) + ) # Pushing (git-receive-pack) over HTTP is not allowed flask.abort(403, description="HTTP pushing disabled") @@ -312,6 +370,17 @@ def clone_proxy(project, username=None, namespace=None): "WWW-Authenticate": 'Basic realm="%s"' % realm, "X-Frame-Options": "DENY", } + _auth_log.info( + "User tried a git push over http but was not authenticated -- " + "|user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + p1_path, + flask.request.query_string, + ) + ) response = flask.Response( response="Authorization Required", status=401, @@ -328,6 +397,17 @@ def clone_proxy(project, username=None, namespace=None): asuser=remote_user, ) if not project: + _auth_log.info( + "User asked to access a git repo that they are not allowed to " + "access -- |user: %s|IP: %s|method: %s|repo: %s|query: %s" + % ( + remote_user, + flask.request.remote_addr, + flask.request.method, + p1.path, + flask.request.query_string, + ) + ) _log.info( "%s could not find project: %s for user %s and namespace %s", remote_user, From 3b6928e366f2e94b4f44836f6372bd3c1a6aade5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 28 2020 10:11:15 +0000 Subject: [PATCH 2/4] Rename the project variable to avoid variable shadowing Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/clone.py b/pagure/ui/clone.py index 43debfe..61615cc 100644 --- a/pagure/ui/clone.py +++ b/pagure/ui/clone.py @@ -389,14 +389,14 @@ def clone_proxy(project, username=None, namespace=None): ) flask.abort(response) - project = pagure.lib.query.get_authorized_project( + project_obj = pagure.lib.query.get_authorized_project( flask.g.session, project, user=username, namespace=namespace, asuser=remote_user, ) - if not project: + if not project_obj: _auth_log.info( "User asked to access a git repo that they are not allowed to " "access -- |user: %s|IP: %s|method: %s|repo: %s|query: %s" @@ -404,7 +404,7 @@ def clone_proxy(project, username=None, namespace=None): remote_user, flask.request.remote_addr, flask.request.method, - p1.path, + p1_path, flask.request.query_string, ) ) @@ -417,10 +417,10 @@ def clone_proxy(project, username=None, namespace=None): ) flask.abort(404, description="Project not found") - if project.is_on_repospanner: - return proxy_repospanner(project, service) + if project_obj.is_on_repospanner: + return proxy_repospanner(project_obj, service) else: - return proxy_raw_git(project) + return proxy_raw_git(project_obj) def add_clone_proxy_cmds(): From 33c20aa6f5527298cf4ef14ae04424f65e34ba2e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 28 2020 10:11:15 +0000 Subject: [PATCH 3/4] Update the default LOGGING configuration - Update the documented default content to reflect what actually is the default content. - Add a note about the ``pagure_auth`` log and how to make it log in a file. Signed-off-by: Pierre-Yves Chibon --- diff --git a/doc/configuration.rst b/doc/configuration.rst index 942aeb8..21b6769 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -811,73 +811,84 @@ The default value is: :: - LOGGING = { - 'version': 1, - 'disable_existing_loggers': False, - 'formatters': { - 'standard': { - 'format': '%(asctime)s [%(levelname)s] %(name)s: %(message)s' + LOGGING = { + "version": 1, + "disable_existing_loggers": False, + "formatters": { + "standard": { + "format": "%(asctime)s [%(levelname)s] %(name)s: %(message)s" }, - 'email_format': { - 'format': MSG_FORMAT - } + "email_format": {"format": MSG_FORMAT}, }, - 'filters': { - 'myfilter': { - '()': ContextInjector, - } - }, - 'handlers': { - 'console': { - 'level': 'INFO', - 'formatter': 'standard', - 'class': 'logging.StreamHandler', - 'stream': 'ext://sys.stdout', + "filters": {"myfilter": {"()": ContextInjector}}, + "handlers": { + "console": { + "formatter": "standard", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout", + }, + "auth_handler": { + "formatter": "standard", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout", }, - 'email': { - 'level': 'ERROR', - 'formatter': 'email_format', - 'class': 'logging.handlers.SMTPHandler', - 'mailhost': 'localhost', - 'fromaddr': 'pagure@localhost', - 'toaddrs': 'root@localhost', - 'subject': 'ERROR on pagure', - 'filters': ['myfilter'], + "email": { + "level": "ERROR", + "formatter": "email_format", + "class": "logging.handlers.SMTPHandler", + "mailhost": "localhost", + "fromaddr": "pagure@localhost", + "toaddrs": "root@localhost", + "subject": "ERROR on pagure", + "filters": ["myfilter"], }, }, # The root logger configuration; this is a catch-all configuration # that applies to all log messages not handled by a different logger - 'root': { - 'level': 'INFO', - 'handlers': ['console'], - }, - 'loggers': { - 'pagure': { - 'handlers': ['console'], - 'level': 'DEBUG', - 'propagate': True + "root": {"level": "INFO", "handlers": ["console"]}, + "loggers": { + "pagure": { + "handlers": ["console"], + "level": "DEBUG", + "propagate": True, }, - 'flask': { - 'handlers': ['console'], - 'level': 'INFO', - 'propagate': False + "pagure_auth": { + "handlers": ["auth_handler"], + "level": "DEBUG", + "propagate": False, }, - 'sqlalchemy': { - 'handlers': ['console'], - 'level': 'WARN', - 'propagate': False + "flask": { + "handlers": ["console"], + "level": "INFO", + "propagate": False, }, - 'binaryornot': { - 'handlers': ['console'], - 'level': 'WARN', - 'propagate': True + "sqlalchemy": { + "handlers": ["console"], + "level": "WARN", + "propagate": False, }, - 'pagure.lib.encoding_utils': { - 'handlers': ['console'], - 'level': 'WARN', - 'propagate': False + "binaryornot": { + "handlers": ["console"], + "level": "WARN", + "propagate": True, }, - } + "MARKDOWN": { + "handlers": ["console"], + "level": "WARN", + "propagate": True, + }, + "PIL": {"handlers": ["console"], "level": "WARN", "propagate": True}, + "chardet": { + "handlers": ["console"], + "level": "WARN", + "propagate": True, + }, + "pagure.lib.encoding_utils": { + "handlers": ["console"], + "level": "WARN", + "propagate": False, + }, + }, } .. note:: as you can see there is an ``email`` handler defined. It's not used @@ -888,6 +899,22 @@ The default value is: 'handlers': ['console', 'email'], +.. note:: The ``pagure_auth`` logger is a special one logging all activities + regarding read/write access to git repositories. It will be a pretty + important log for auditing if needed. + You can separate this log into its own file if you like by using the + following handler: + :: + + "auth_handler": { + "formatter": "standard", + "class": "logging.FileHandler", + "filename": "/var/log/pagure/pagure_auth.log", + } + + Beware if you do this that you will also likely want to enable logrotate + on the system. + ITEM_PER_PAGE ~~~~~~~~~~~~~ From 40b0048a60f7d3a339b32c8081a502836dd9d264 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 28 2020 10:11:15 +0000 Subject: [PATCH 4/4] Fix warning when compiling the doc, title had an underline too short Signed-off-by: Pierre-Yves Chibon --- diff --git a/doc/configuration.rst b/doc/configuration.rst index 21b6769..75b4203 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -1016,7 +1016,7 @@ Defaults to: ``None`` SMTP_CERTFILE -^^^^^^^^^^^^ +^^^^^^^^^^^^^ This configuration key allows to specify a certificate file to be used in the `starttls` command when connecting to the smtp server.