Merge ~cjwatson/launchpad:gpg-handle-preload into launchpad:master

[Colin Watson]

Defer GnuPG home directory setup to support app preloading

The `launchpad-appserver` charm uses Gunicorn's `preload_app` setting as part of its arrangements to support rolling restart during upgrades. However, this causes a problem with the GnuPG handler. Because it sets up its temporary GnuPG home directory in `GPGHandler.__init__`, this is done during ZCML initialization, and when application preloading is enabled this happens in the Gunicorn parent ("arbiter") process and is shared between all the child ("worker") processes. Unfortunately, this means that when a worker process exits, which can happen during normal operation, the `atexit` handler that's registered to clean up the temporary GnuPG home directory is called, thus leaving all other running workers without a GnuPG home directory. At this point all GnuPG operations will fail until the entire service is restarted.

There's certainly work we can do to make the behaviour of our `atexit` handlers less surprising with child processes in general, but in the meantime I think it makes sense to ensure that we have a separate GnuPG home directory for each child process even when using application preloading; I doubt GnuPG is completely robust against having multiple processes writing to the same home directory in parallel anyway. We achieve this by deferring setup of the GnuPG home directory until we need to perform an actual GnuPG operation.

I've always been uncomfortable with the way that `GPGHandler` sets `GNUPGHOME` in the process environment; it seems too much like implicit action at a distance. Most of the relevant tools now support passing this in some other way, either via a GPGME engine parameter or via a command-line option, so I've switched to that where possible. There are a couple of cases where this isn't possible, and in those cases I've arranged to set the environment variable in a more localized way.

[Jürgen Gmach (jugmac00)]

LGTM with one question

review: Approve

diff --git a/lib/lp/services/gpg/handler.py b/lib/lp/services/gpg/handler.py
index a7a1a20..bfd0393 100644
--- a/lib/lp/services/gpg/handler.py
+++ b/lib/lp/services/gpg/handler.py
@@ -8,13 +8,11 @@ __all__ = [
     "PymeUserId",
 ]
 
-import atexit
 import http.client
 import os
 import shutil
 import subprocess
 import sys
-import tempfile
 from contextlib import contextmanager
 from datetime import datetime, timezone
 from io import BytesIO
@@ -47,6 +45,7 @@ from lp.services.gpg.interfaces import (
     IPymeUserId,
     MoreThanOneGPGKeyFound,
     SecretGPGKeyImportDetected,
+    get_gpg_home_directory,
     get_gpg_path,
     get_gpgme_context,
     valid_fingerprint,
@@ -83,49 +82,6 @@ def gpgme_timeline(name, detail):
 class GPGHandler:
     """See IGPGHandler."""
 
-    def __init__(self):
-        """Initialize environment variable."""
-        self._setNewHome()
-        os.environ["GNUPGHOME"] = self.home
-
-    def _setNewHome(self):
-        """Create a new directory containing the required configuration.
-
-        This method is called inside the class constructor and generates
-        a new directory (name randomly generated with the 'gpg-' prefix)
-        containing the proper file configuration and options.
-
-        Also installs an atexit handler to remove the directory on normal
-        process termination.
-        """
-        self.home = tempfile.mkdtemp(prefix="gpg-")
-        confpath = os.path.join(self.home, "gpg.conf")
-        with open(confpath, "w") as conf:
-            # Avoid wasting time verifying the local keyring's consistency.
-            conf.write("no-auto-check-trustdb\n")
-            # Use the loopback mode to allow using password callbacks.
-            conf.write("pinentry-mode loopback\n")
-            # Assume "yes" on most questions; this is needed to allow
-            # deletion of secret keys via GPGME.
-            conf.write("yes\n")
-            # Prefer a SHA-2 hash where possible, otherwise GPG will fall
-            # back to a hash it can use.
-            conf.write(
-                "personal-digest-preferences SHA512 SHA384 SHA256 SHA224\n"
-            )
-        agentconfpath = os.path.join(self.home, "gpg-agent.conf")
-        with open(agentconfpath, "w") as agentconf:
-            agentconf.write("allow-loopback-pinentry\n")
-        # create a local atexit handler to remove the configuration directory
-        # on normal termination.
-
-        def removeHome(home):
-            """Remove GNUPGHOME directory."""
-            if os.path.exists(home):
-                shutil.rmtree(home)
-
-        atexit.register(removeHome, self.home)
-
     def sanitizeFingerprint(self, fingerprint):
         """See IGPGHandler."""
         return sanitize_fingerprint(fingerprint)
@@ -133,6 +89,7 @@ class GPGHandler:
     def resetLocalState(self):
         """See IGPGHandler."""
         # Remove the public keyring, private keyring and the trust DB.
+        home = get_gpg_home_directory()
         for filename in (
             "pubring.gpg",
             "pubring.kbx",
@@ -140,7 +97,7 @@ class GPGHandler:
             "private-keys-v1.d",
             "trustdb.gpg",
         ):
-            filename = os.path.join(self.home, filename)
+            filename = os.path.join(home, filename)
             if os.path.exists(filename):
                 if os.path.isdir(filename):
                     shutil.rmtree(filename)
@@ -148,7 +105,12 @@ class GPGHandler:
                     os.remove(filename)
         # Kill any running gpg-agent for GnuPG 2
         if shutil.which("gpgconf"):
-            subprocess.check_call(["gpgconf", "--kill", "gpg-agent"])
+            # XXX cjwatson 2023-04-16: Ubuntu 16.04's gpgconf doesn't
+            # support --homedir, so we have to set an environment variable.
+            # This can be simplified once we require Ubuntu 20.04.
+            env = dict(os.environ)
+            env["GNUPGHOME"] = home
+            subprocess.check_call(["gpgconf", "--kill", "gpg-agent"], env=env)
 
     def getVerifiedSignatureResilient(self, content, signature=None):
         """See IGPGHandler."""
@@ -717,6 +679,8 @@ class PymeKey:
             return subprocess.run(
                 [
                     get_gpg_path(),
+                    "--homedir",
+                    get_gpg_home_directory(),
                     "--export-secret-keys",
                     "-a",
                     "--passphrase-fd",
diff --git a/lib/lp/services/gpg/interfaces.py b/lib/lp/services/gpg/interfaces.py
index 0302da0..6dec53a 100644
--- a/lib/lp/services/gpg/interfaces.py
+++ b/lib/lp/services/gpg/interfaces.py
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
+    "get_gpg_home_directory",
     "get_gpg_path",
     "get_gpgme_context",
     "GPG_INJECT",
@@ -24,8 +25,12 @@ __all__ = [
     "valid_keyid",
 ]
 
+import atexit
 import http.client
+import os.path
 import re
+import shutil
+import tempfile
 
 from lazr.enum import DBEnumeratedType, DBItem
 from lazr.restful.declarations import error_status
@@ -60,12 +65,56 @@ def get_gpg_path():
     return "/usr/bin/gpg2"
 
 
+_gpg_home = None
+
+
+def get_gpg_home_directory():
+    """Create a new GnuPG home directory for this process.
+
+    This also installs an atexit handler to remove the directory on normal
+    process termination.
+    """
+    global _gpg_home
+
+    if _gpg_home is not None and os.path.exists(_gpg_home):
+        return _gpg_home
+
+    _gpg_home = tempfile.mkdtemp(prefix="gpg-")
+    confpath = os.path.join(_gpg_home, "gpg.conf")
+    with open(confpath, "w") as conf:
+        # Avoid wasting time verifying the local keyring's consistency.
+        conf.write("no-auto-check-trustdb\n")
+        # Use the loopback mode to allow using password callbacks.
+        conf.write("pinentry-mode loopback\n")
+        # Assume "yes" on most questions; this is needed to allow
+        # deletion of secret keys via GPGME.
+        conf.write("yes\n")
+        # Prefer a SHA-2 hash where possible, otherwise GPG will fall
+        # back to a hash it can use.
+        conf.write("personal-digest-preferences SHA512 SHA384 SHA256 SHA224\n")
+    agentconfpath = os.path.join(_gpg_home, "gpg-agent.conf")
+    with open(agentconfpath, "w") as agentconf:
+        agentconf.write("allow-loopback-pinentry\n")
+
+    def removeHome(home):
+        """Remove GnuPG home directory."""
+        if os.path.exists(home):
+            shutil.rmtree(home)
+
+    # Remove the configuration directory on normal termination.
+    atexit.register(removeHome, _gpg_home)
+
+    return _gpg_home
+
+
 def get_gpgme_context():
     """Return a new appropriately-configured GPGME context."""
     import gpgme
 
     context = gpgme.Context()
-    context.set_engine_info(gpgme.PROTOCOL_OpenPGP, get_gpg_path(), None)
+    context.set_engine_info(
+        gpgme.PROTOCOL_OpenPGP, get_gpg_path(), get_gpg_home_directory()
+    )
     context.armor = True
     return context
 
diff --git a/lib/lp/services/gpg/tests/test_gpghandler.py b/lib/lp/services/gpg/tests/test_gpghandler.py
index ec2da7e..1199ea0 100644
--- a/lib/lp/services/gpg/tests/test_gpghandler.py
+++ b/lib/lp/services/gpg/tests/test_gpghandler.py
@@ -23,6 +23,7 @@ from lp.services.gpg.interfaces import (
     GPGKeyMismatchOnServer,
     GPGKeyTemporarilyNotFoundError,
     IGPGHandler,
+    get_gpg_home_directory,
     get_gpg_path,
     get_gpgme_context,
 )
@@ -87,22 +88,26 @@ class TestGPGHandler(TestCase):
         """Get a gpghandler and login"""
         super().setUp()
         login(ANONYMOUS)
+        self.addCleanup(logout)
         self.gpg_handler = getUtility(IGPGHandler)
         self.gpg_handler.resetLocalState()
-
-    def tearDown(self):
-        """Zero out the gpg database"""
-        # XXX Stuart Bishop 2005-10-27:
-        # This should be a zope test cleanup thing per SteveA.
-        self.gpg_handler.resetLocalState()
-        logout()
-        super().tearDown()
+        self.addCleanup(self.gpg_handler.resetLocalState)
 
     def populateKeyring(self):
         for email in iter_test_key_emails():
             pubkey = test_pubkey_from_email(email)
             self.gpg_handler.importPublicKey(pubkey)
 
+    def test_first_call_creates_home_directory(self):
+        original_home = get_gpg_home_directory()
+        shutil.rmtree(original_home)
+        list(self.gpg_handler.localKeys())
+        home = get_gpg_home_directory()
+        self.assertNotEqual(original_home, home)
+        self.assertTrue(os.path.exists(os.path.join(home, "gpg.conf")))
+        list(self.gpg_handler.localKeys())
+        self.assertEqual(home, get_gpg_home_directory())
+
     # This sequence might fit better as a doctest. Hmm.
     def testEmptyGetKeys(self):
         """The initial local key list should be empty."""
@@ -541,6 +546,8 @@ class TestGPGHandler(TestCase):
                 gpg_proc = subprocess.Popen(
                     [
                         get_gpg_path(),
+                        "--homedir",
+                        get_gpg_home_directory(),
                         "--quiet",
                         "--status-fd",
                         "1",
diff --git a/lib/lp/soyuz/tests/fakepackager.py b/lib/lp/soyuz/tests/fakepackager.py
index a42cefe..963b94d 100644
--- a/lib/lp/soyuz/tests/fakepackager.py
+++ b/lib/lp/soyuz/tests/fakepackager.py
@@ -22,7 +22,11 @@ from zope.component import getUtility
 from lp.archiveuploader.nascentupload import NascentUpload
 from lp.archiveuploader.uploadpolicy import findPolicyByName
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.services.gpg.interfaces import IGPGHandler, get_gpg_path
+from lp.services.gpg.interfaces import (
+    IGPGHandler,
+    get_gpg_home_directory,
+    get_gpg_path,
+)
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.enums import PackageUploadStatus
 from lp.testing.gpgkeys import import_secret_test_key
@@ -219,7 +223,7 @@ class FakePackager:
         changelog_file.write(previous_content)
         changelog_file.close()
 
-    def _runSubProcess(self, script, extra_args=None):
+    def _runSubProcess(self, script, extra_args=None, extra_env=None):
         """Run the given script and collects STDOUT and STDERR.
 
         :raises AssertionError: If the script returns a non-Zero value.
@@ -228,8 +232,11 @@ class FakePackager:
             extra_args = []
         args = [script]
         args.extend(extra_args)
+        env = dict(os.environ)
+        if extra_env:
+            env.update(extra_env)
         process = subprocess.Popen(
-            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE
+            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env
         )
         stdout, stderr = process.communicate()
 
@@ -403,7 +410,11 @@ class FakePackager:
         current_path = os.getcwd()
         os.chdir(self.upstream_directory)
 
-        self._runSubProcess("dpkg-buildpackage", dpkg_buildpackage_options)
+        self._runSubProcess(
+            "dpkg-buildpackage",
+            extra_args=dpkg_buildpackage_options,
+            extra_env={"GNUPGHOME": get_gpg_home_directory()},
+        )
 
         os.chdir(current_path)