GitSCM: add GitHub token support (bug 1934475) (#189)

* Makefile: typo
* GitSCM: redact repo auth details in exceptions
* GitSCM: add GitHub token support (bug 1934475) 
* dependencies: update requirements.txt
* doc: document environment parameters
* settings: deprecate LANDO_GITHUB_ACCESS_TOKEN

Makefile

@@ -1,7 +1,7 @@
 SHELL := /bin/bash
 DOCKER := $(shell which docker)
 DOCKER_COMPOSE := ${DOCKER} compose
-ARGS_TEST ?=
+ARGS_TESTS ?=
 
 SUITE_STAMP=.test-use-suite
 

README.md

@@ -29,6 +29,46 @@ The above command will run any database migrations and start the development ser
 
 The above command will shut down the containers running lando.
 
+## Configuring the server
+
+Lando relies on environment variables to configure its behaviour.
+
+Parameters of interest are the following.
+
+- Database parameters
+  - `DEFAULT_DB_HOST`
+  - `DEFAULT_DB_NAME`
+  - `DEFAULT_DB_PASSWORD`
+  - `DEFAULT_DB_PORT`
+  - `DEFAULT_DB_USER`
+- [GitHub application][github-app] authentication (needs to be
+  [installed][github-app-install] on all target repos)
+
+  - `GITHUB_APP_ID`
+  - `GITHUB_APP_PRIVKEY` (PEM)
+
+- HgMO authentication
+  - `SSH_PRIVATE_KEY` (PEM)
+- Mozilla services
+  - `PHABRICATOR_ADMIN_API_KEY`
+  - `PHABRICATOR_UNPRIVILEGED_API_KEY`
+  - `PHABRICATOR_URL` (URL)
+  - `TREESTATUS_URL` (URL)
+- OIDC parameters:
+  - `OIDC_DOMAIN` (domain name, no scheme)
+  - `OIDC_RP_CLIENT_ID`
+  - `OIDC_RP_CLIENT_SECRET`
+
+Have a look at all variables set via `os.getenv` in
+[`src/lando/settings.py`](src/lando/settings.py) for a more authoritative list.
+
+The `compose.yaml` file relies on a `.env` file for additional configuration.
+This is particularly useful for secrets and other transient parameters which
+cannot be included in the repo. That file is listed in the `.gitignore` list.
+
+Note that, currently, this environment file is also used by the [Conduit suite]
+when running a lando stack from the local working copy.
+
 ## Testing
 
 To run the test suite, invoke the following command:
@@ -56,8 +96,7 @@ Alternatively, you can run the `lando tests` command directly from n the Lando c
     docker compose run --rm lando lando tests -x -- failed-first --verbose
 
 It is also possible to run the tests in an existing stack from the
-[Conduit suite](https://github.com/mozilla-conduit/suite), by specifying the
-`INSUITE=1` parameter.
+[Conduit suite], by specifying the `INSUITE=1` parameter.
 
     make test INSUITE=1
 
@@ -86,3 +125,7 @@ and restore the default with
 ## Support
 
 To chat with Lando users and developers, join them on [Matrix](https://chat.mozilla.org/#/room/#conduit:mozilla.org).
+
+[Conduit suite]: https://github.com/mozilla-conduit/suite
+[github-app]: https://docs.github.com/en/enterprise-cloud@latest/apps/creating-github-apps/registering-a-github-app/registering-a-github-app
+[github-app-install]: https://docs.github.com/en/enterprise-cloud@latest/apps/using-github-apps/installing-your-own-github-app

compose.yaml

@@ -53,6 +53,18 @@ services:
       db:
         condition: service_healthy
 
+  git-landing-worker:
+    image: lando
+    command: lando start_landing_worker git
+    environment:
+      DEFAULT_GRACE_SECONDS: 10
+      LANDING_WORKER_USERNAME: app
+    env_file:
+      - .env
+    depends_on:
+      db:
+        condition: service_healthy
+
   proxy:
     build: ./nginx
     ports:

pyproject.toml

@@ -30,6 +30,7 @@ dependencies = [
     "rs-parsepatch",
     "sentry-sdk",
     "setuptools-scm",
+    "simple-github",
     "uwsgi",
 ]
 name = "lando"

src/lando/api/legacy/workers/landing_worker.py

@@ -314,7 +314,7 @@ def run_job(self, job: LandingJob) -> bool:
                 # Try again, this is a temporary failure.
                 return False
             except Exception as e:
-                message = f"Unexpected error while fetching repo from {repo.pull_path}."
+                message = f"Unexpected error while fetching repo from {repo.name}."
                 logger.exception(message)
                 job.transition_status(
                     LandingJobAction.FAIL,
@@ -423,7 +423,7 @@ def run_job(self, job: LandingJob) -> bool:
                 job.transition_status(LandingJobAction.DEFER, message=message)
                 return False  # Try again, this is a temporary failure.
             except Exception as e:
-                message = f"Unexpected error while pushing to {repo.push_path}.\n{e}"
+                message = f"Unexpected error while pushing to {repo.name}.\n{e}"
                 logger.exception(message)
                 job.transition_status(
                     LandingJobAction.FAIL,

src/lando/main/scm/git.py

@@ -1,12 +1,17 @@
+import asyncio
 import logging
 import os
+import re
 import subprocess
 import tempfile
 import uuid
 from contextlib import contextmanager
 from pathlib import Path
 from typing import ContextManager, Optional
 
+from django.conf import settings
+from simple_github import AppAuth, AppInstallationAuth
+
 from lando.main.scm.consts import SCM_TYPE_GIT
 from lando.main.scm.exceptions import SCMException
 
@@ -17,6 +22,23 @@
 ENV_COMMITTER_NAME = "GIT_COMMITTER_NAME"
 ENV_COMMITTER_EMAIL = "GIT_COMMITTER_EMAIL"
 
+# From RFC-3986 [0]:
+#
+#     userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )
+#
+#     unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
+#     pct-encoded   = "%" HEXDIG HEXDIG
+#     sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
+#                 / "*" / "+" / "," / ";" / "=
+#
+# [0] https://www.rfc-editor.org/rfc/rfc3986
+URL_USERINFO_RE = re.compile(
+    "(?P<userinfo>[-A-Za-z0-9:._~%!$&'*()*+;=]*@)", flags=re.MULTILINE
+)
+GITHUB_URL_RE = re.compile(
+    f"https://{URL_USERINFO_RE.pattern}?github.com/(?P<owner>[-A-Za-z0-9]+)/(?P<repo>[^/]+)"
+)
+
 
 class GitSCM(AbstractSCM):
     """An implementation of the AbstractVCS for Git, for use by the Repo and LandingWorkers."""
@@ -56,13 +78,64 @@ def push(
     ):
         """Push local code to the remote repository."""
         command = ["push"]
+
         if force_push:
             command += ["--force"]
+
+        if match := re.match(GITHUB_URL_RE, push_path):
+            # We only fetch a token if no authentication is explicitly specified in
+            # the push_url.
+            if not match["userinfo"]:
+                logger.info(
+                    "Obtaining fresh GitHub token repo",
+                    extra={
+                        "push_path": push_path,
+                        "repo_name": match["repo"],
+                        "repo_owner": match["owner"],
+                    },
+                )
+
+                token = self._get_github_token(match["owner"], match["repo"])
+                if token:
+                    push_path = f"https://git:{token}@github.com/{match['owner']}/{match['repo']}"
+
         command += [push_path]
+
         if push_target:
             command += [f"HEAD:{push_target}"]
+
         self._git_run(*command, cwd=self.path)
 
+    @staticmethod
+    def _get_github_token(repo_owner: str, repo_name: str) -> Optional[str]:
+        """Obtain a fresh GitHub token to push to the specified repo.
+
+        This relies on GITHUB_APP_ID and GITHUB_APP_PRIVKEY to be set in the
+        environment. Returns None if those are missing.
+
+        The app with ID GITHUB_APP_ID needs to be enabled for the target repo.
+
+        """
+        app_id = settings.GITHUB_APP_ID
+        app_privkey = settings.GITHUB_APP_PRIVKEY
+
+        if not app_id or not app_privkey:
+            logger.warning(
+                "Missing GITHUB_APP_ID or GITHUB_APP_PRIVKEY to authenticate against GitHub",
+                extra={
+                    "repo_name": repo_name,
+                    "repo_owner": repo_owner,
+                },
+            )
+            return None
+
+        app_auth = AppAuth(
+            app_id,
+            app_privkey,
+        )
+        session = AppInstallationAuth(app_auth, repo_owner, repositories=[repo_name])
+        return asyncio.run(session.get_token())
+
     def last_commit_for_path(self, path: str) -> str:
         """Find last commit to touch a path."""
         command = ["log", "--max-count=1", "--format=%H", "--", path]
@@ -204,10 +277,11 @@ def _git_run(cls, *args, cwd: Optional[str] = None) -> str:
         correlation_id = str(uuid.uuid4())
         path = cwd or "/"
         command = ["git"] + list(args)
+        sanitised_command = [cls._redact_url_userinfo(a) for a in command]
         logger.info(
             "running git command",
             extra={
-                "command": command,
+                "command": sanitised_command,
                 "command_id": correlation_id,
                 "path": cwd,
             },
@@ -218,10 +292,11 @@ def _git_run(cls, *args, cwd: Optional[str] = None) -> str:
         )
 
         if result.returncode:
+            redacted_stderr = cls._redact_url_userinfo(result.stderr)
             raise SCMException(
-                f"Error running git command; {command=}, {path=}, {result.stderr}",
-                result.stdout,
-                result.stderr,
+                f"Error running git command; {sanitised_command=}, {path=}, {redacted_stderr}",
+                cls._redact_url_userinfo(result.stdout),
+                redacted_stderr,
             )
 
         out = result.stdout.strip()
@@ -238,6 +313,10 @@ def _git_run(cls, *args, cwd: Optional[str] = None) -> str:
 
         return out
 
+    @staticmethod
+    def _redact_url_userinfo(url: str) -> str:
+        return re.sub(URL_USERINFO_RE, "[REDACTED]@", url)
+
     @classmethod
     def _git_env(cls):
         env = os.environ.copy()

src/lando/main/tests/test_git.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+from lando.main.scm.exceptions import SCMException
 from lando.main.scm.git import GitSCM
 
 
@@ -138,6 +139,38 @@ def test_GitSCM_clean_repo(
     ), f"strip_non_public_commits not honoured for {new_file}"
 
 
+def test_GitSCM_push_get_github_token(git_repo: Path):
+    scm = GitSCM(str(git_repo))
+    scm._git_run = MagicMock()
+    scm._get_github_token = MagicMock()
+    scm._get_github_token.side_effect = ["ghs_yolo"]
+
+    scm.push("https://github.com/some/repo")
+
+    assert scm._git_run.call_count == 1, "_git_run wasn't called when pushing"
+    assert (
+        scm._get_github_token.call_count == 1
+    ), "_get_github_token wasn't called when pushing to a github-like URL"
+    assert (
+        "git:[email protected]" in scm._git_run.call_args[0][1]
+    ), "github token not found in rewritten push_path"
+
+
+def test_GitSCM_git_run_redact_url_userinfo(git_repo: Path):
+    scm = GitSCM(str(git_repo))
+    userinfo = "user:password"
+    with pytest.raises(SCMException) as exc:
+        scm.push(
+            f"http://{userinfo}@this-shouldn-t-resolve-otherwise-this-will-timeout-and-this-test-will-take-longer/some/repo"
+        )
+
+    assert userinfo not in exc.value.out
+    assert userinfo not in exc.value.err
+    assert userinfo not in str(exc.value)
+    assert userinfo not in repr(exc.value)
+    assert "[REDACTED]" in exc.value.err
+
+
 def _monkeypatch_scm(monkeypatch, scm: GitSCM, method: str) -> MagicMock:
     """
     Mock a method on `scm` to test the call, but let it continue with its original side

src/lando/settings.py

@@ -199,7 +199,6 @@
 ]
 
 LINT_PATHS = (BASE_DIR,)
-GITHUB_ACCESS_TOKEN = os.getenv("LANDO_GITHUB_ACCESS_TOKEN")
 PHABRICATOR_URL = os.getenv("PHABRICATOR_URL", "http://phabricator.test")
 PHABRICATOR_ADMIN_API_KEY = os.getenv("PHABRICATOR_ADMIN_API_KEY", "")
 PHABRICATOR_UNPRIVILEGED_API_KEY = os.getenv("PHABRICATOR_UNPRIVILEGED_API_KEY", "")
@@ -229,3 +228,6 @@
 
 COMMITTER_NAME = os.getenv("LANDO_COMMITTER_NAME", LANDO_USER_NAME)
 COMMITTER_EMAIL = os.getenv("LANDO_COMMITTER_EMAIL", LANDO_USER_EMAIL)
+
+GITHUB_APP_ID = os.getenv("GITHUB_APP_ID")
+GITHUB_APP_PRIVKEY = os.getenv("GITHUB_APP_PRIVKEY")

src/lando/test_settings.py

@@ -10,7 +10,6 @@
 OIDC_IDENTIFIER = (
     "lando-api"  # Added for compatibility with tests, should not be needed.
 )
-GITHUB_ACCESS_TOKEN = ""
 PHABRICATOR_URL = "http://phabricator.test"
 PHABRICATOR_ADMIN_API_KEY = "api-thiskeymustbe32characterslen"
 PHABRICATOR_UNPRIVILEGED_API_KEY = "api-thiskeymustbe32characterslen"