From 48664c3771583101ef444486ac30c50c92d84959 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
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 <pingou@pingoured.fr>

---

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 <pingou@pingoured.fr>
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 <pingou@pingoured.fr>

---

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 <pingou@pingoured.fr>
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 <pingou@pingoured.fr>

---

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 <pingou@pingoured.fr>
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 <pingou@pingoured.fr>

---

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.

