Merge ~cjwatson/launchpad:stop-ppa-key-propagation into launchpad:master

[Colin Watson]

Stop propagating signing keys between an owner's PPAs

Things were perhaps different in 2009 when this feature was designed, but add-apt-repository has dealt with fetching keys on a per-archive basis for a long time now, and it makes more sense for keys to be per-archive. This also improves behaviour for users whose default archive was created long enough ago that it has a 1024-bit signing key.

[William Grant (wgrant)]

https://bugs.launchpad.net/launchpad/+bug/357177 was where the behaviour was implemented, which links to https://lists.launchpad.net/launchpad-users/msg04943.html.

There were in the past concerns about keyserver pollution, I believe. PPAs are now often pretty ephemeral, and frequently created and deleted, but all the keys will stay around forever, filling up name-based key search results. It would also reveal the name of private PPAs.

I don't think we should change the behaviour until we stop using the main keyserver network for PPA keys.

[Robert Hardy (rhardy)]

Like many other long term users on Launchpad, I need to be able to update my signing key on Launchpad. I have no trouble approving this outright but it would really surprise me if I have the rights to approve this alone.

I ask whomever does to serious reconsider this. Launchpad is only as good as its trust anchors. Right now for a lot of long term developers the under-pinnings i.e. a 1024 bit signing key are insecure.

If this is being stalled because others care about excessive key generation on keyserver network for PPA keys, why not change the proposed code so this only happens once if the existing signing key associated with the user is an insecure 1024 bit key. This would trigger a single badly needed key update only where it is needed.

review: Needs Fixing

Unmerged commits

ffc336d... by Colin Watson on 2020-10-21

Stop propagating signing keys between an owner's PPAs

Things were perhaps different in 2009 when this feature was designed,
but add-apt-repository has dealt with fetching keys on a per-archive
basis for a long time now, and it makes more sense for keys to be
per-archive. This also improves behaviour for users whose default
archive was created long enough ago that it has a 1024-bit signing key.

LP: #1700167

diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
index 4c3688d..2c09afa 100644
--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
@@ -14,7 +14,6 @@ __all__ = [
 import os
 
 import gpgme
-from twisted.internet import defer
 from twisted.internet.threads import deferToThread
 from zope.component import getUtility
 from zope.interface import implementer
@@ -250,32 +249,6 @@ class ArchiveGPGSigningKey(SignableArchive):
         assert self.archive.signing_key_fingerprint is None, (
             "Cannot override signing_keys.")
 
-        # Always generate signing keys for the default PPA, even if it
-        # was not specifically requested. The default PPA signing key
-        # is then propagated to the context named-ppa.
-        default_ppa = self.archive.owner.archive
-        if self.archive != default_ppa:
-            def propagate_key(_):
-                self.archive.signing_key_owner = default_ppa.signing_key_owner
-                self.archive.signing_key_fingerprint = (
-                    default_ppa.signing_key_fingerprint)
-                del get_property_cache(self.archive).signing_key
-
-            if default_ppa.signing_key_fingerprint is None:
-                d = IArchiveGPGSigningKey(default_ppa).generateSigningKey(
-                    log=log, async_keyserver=async_keyserver)
-            else:
-                d = defer.succeed(None)
-            # generateSigningKey is only asynchronous if async_keyserver is
-            # true; we need some contortions to keep it synchronous
-            # otherwise.
-            if async_keyserver:
-                d.addCallback(propagate_key)
-                return d
-            else:
-                propagate_key(None)
-                return
-
         key_displayname = (
             "Launchpad PPA for %s" % self.archive.owner.displayname)
         if getFeatureFlag(PUBLISHER_GPG_USES_SIGNING_SERVICE):
diff --git a/lib/lp/archivepublisher/tests/archive-signing.txt b/lib/lp/archivepublisher/tests/archive-signing.txt
index a487a3b..7e06104 100644
--- a/lib/lp/archivepublisher/tests/archive-signing.txt
+++ b/lib/lp/archivepublisher/tests/archive-signing.txt
@@ -266,72 +266,24 @@ key is available in the keyserver.
     >>> retrieved_key.fingerprint == signing_key.fingerprint
     True
 
-As documented in archive.txt, when a named-ppa is created it is
-already configured to used the same signing-key created for the
-default PPA. We will create a named-ppa for Celso.
+We will create a named PPA for Celso.
 
     >>> from lp.soyuz.enums import ArchivePurpose
     >>> named_ppa = getUtility(IArchiveSet).new(
     ...     owner=cprov, purpose=ArchivePurpose.PPA, name='boing')
 
-As expected it will use the same key used in Celso's default PPA.
+It initially has no signing key.
 
-    >>> print cprov.archive.signing_key.fingerprint
+    >>> print(cprov.archive.signing_key_fingerprint)
     0D57E99656BEFB0897606EE9A022DD1F5001B46D
 
-    >>> print named_ppa.signing_key.fingerprint
-    0D57E99656BEFB0897606EE9A022DD1F5001B46D
-
-We will reset the signing key of the just created named PPA,
-simulating the situation when a the default PPA and a named-ppas get
-created within the same cycle of the key-generator process.
-
-    >>> from lp.services.propertycache import get_property_cache
-    >>> login('[e-mail]')
-    >>> named_ppa.signing_key_owner = None
-    >>> named_ppa.signing_key_fingerprint = None
-    >>> del get_property_cache(named_ppa).signing_key
-    >>> login(ANONYMOUS)
-
-    >>> print named_ppa.signing_key
+    >>> print(named_ppa.signing_key_fingerprint)
     None
 
 Default PPAs are always created first and thus get their keys generated
-before the named-ppa for the same owner. We submit the named-ppa to
-the key generation procedure, as it would be normally in production.
-
-    >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
-    >>> named_ppa_signing_key.generateSigningKey()
-
-Instead of generating a new key, the signing key from the default ppa
-(Celso's default PPA) gets reused.
-
-    >>> print cprov.archive.signing_key.fingerprint
-    0D57E99656BEFB0897606EE9A022DD1F5001B46D
-
-    >>> print named_ppa.signing_key.fingerprint
-    0D57E99656BEFB0897606EE9A022DD1F5001B46D
-
-We will reset the signing-keys for both PPA of Celso.
-
-    >>> login('[e-mail]')
-    >>> cprov.archive.signing_key_owner = None
-    >>> cprov.archive.signing_key_fingerprint = None
-    >>> del get_property_cache(cprov.archive).signing_key
-    >>> named_ppa.signing_key_owner = None
-    >>> named_ppa.signing_key_fingerprint = None
-    >>> del get_property_cache(named_ppa).signing_key
-    >>> login(ANONYMOUS)
-
-    >>> print cprov.archive.signing_key
-    None
-
-    >>> print named_ppa.signing_key
-    None
-
-Then modify the GPGHandler utility to return a sampledata key instead
-of generating a new one, mainly for running the test faster and for
-printing the context the key is generated.
+before the named PPA for the same owner. As before, generating a real key
+is slow, so we modify the GPGHandler utility to return a sampledata key
+instead.
 
     >>> def mock_key_generator(name, logger=None):
     ...     print 'Generating:', name
@@ -343,19 +295,18 @@ printing the context the key is generated.
     >>> real_key_generator = naked_gpghandler.generateKey
     >>> naked_gpghandler.generateKey = mock_key_generator
 
-When the signing key for the named-ppa is requested, it is generated
-in the default PPA context then propagated to the named-ppa. The key is
-named after the user, even if the default PPA name is something different.
+The key is named after the user, even if the default PPA name is something
+different.
 
     >>> cprov.display_name = "Not Celso Providelo"
     >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
     >>> named_ppa_signing_key.generateSigningKey()
     Generating: Launchpad PPA for Not Celso Providelo
 
-    >>> print cprov.archive.signing_key.fingerprint
-    447DBF38C4F9C4ED752246B77D88913717B05A8F
+    >>> print(cprov.archive.signing_key_fingerprint)
+    0D57E99656BEFB0897606EE9A022DD1F5001B46D
 
-    >>> print named_ppa.signing_key.fingerprint
+    >>> print(named_ppa.signing_key_fingerprint)
     447DBF38C4F9C4ED752246B77D88913717B05A8F
 
 Restore the original functionality of GPGHandler.
@@ -371,6 +322,7 @@ for archive which already contains a 'signing_key'.
 
 Celso's default PPA will uses the testing signing key.
 
+    >>> from lp.services.propertycache import get_property_cache
     >>> login('[e-mail]')
     >>> cprov.archive.signing_key_owner = signing_key.owner
     >>> cprov.archive.signing_key_fingerprint = signing_key.fingerprint
diff --git a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
index b521e85..8fefb07 100644
--- a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
@@ -345,8 +345,9 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
     @defer.inlineCallbacks
     def test_generateSigningKey_local_non_default_ppa(self):
         # Generating a signing key locally using GPGHandler for a
-        # non-default PPA generates one for the user's default PPA first and
-        # then propagates it.
+        # non-default PPA ignores the user's default PPA.  (It used to
+        # generate one for the user's default PPA first and then propagate
+        # it.)
         self.useFixture(FakeGenerateKey("[e-mail]"))
         logger = BufferLogger()
         # Use a display name that matches the pregenerated sample key.
@@ -357,19 +358,19 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
         yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(
             log=logger, async_keyserver=True)
         self.assertThat(default_ppa, MatchesStructure(
+            signing_key=Is(None),
+            signing_key_owner=Is(None),
+            signing_key_fingerprint=Is(None)))
+        self.assertThat(another_ppa, MatchesStructure(
             signing_key=Not(Is(None)),
             signing_key_owner=Not(Is(None)),
             signing_key_fingerprint=Not(Is(None))))
         self.assertIsNotNone(
             getUtility(IGPGKeySet).getByFingerprint(
-                default_ppa.signing_key_fingerprint))
+                another_ppa.signing_key_fingerprint))
         self.assertIsNone(
             getUtility(ISigningKeySet).get(
-                SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint))
-        self.assertThat(another_ppa, MatchesStructure.byEquality(
-            signing_key=default_ppa.signing_key,
-            signing_key_owner=default_ppa.signing_key_owner,
-            signing_key_fingerprint=default_ppa.signing_key_fingerprint))
+                SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint))
 
     @defer.inlineCallbacks
     def test_generateSigningKey_signing_service(self):
@@ -411,8 +412,8 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
     @defer.inlineCallbacks
     def test_generateSigningKey_signing_service_non_default_ppa(self):
         # Generating a signing key on the signing service for a non-default
-        # PPA generates one for the user's default PPA first and then
-        # propagates it.
+        # PPA ignores the user's default PPA.  (It used to generate one for
+        # the user's default PPA first and then propagate it.)
         self.useFixture(
             FeatureFixture({PUBLISHER_GPG_USES_SIGNING_SERVICE: "on"}))
         signing_service_client = self.useFixture(
@@ -430,16 +431,15 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
             log=logger, async_keyserver=True)
         self.assertThat(default_ppa, MatchesStructure(
             signing_key=Is(None),
+            signing_key_owner=Is(None),
+            signing_key_fingerprint=Is(None)))
+        self.assertThat(another_ppa, MatchesStructure(
+            signing_key=Is(None),
             signing_key_owner=Not(Is(None)),
             signing_key_fingerprint=Not(Is(None))))
         self.assertIsNone(
             getUtility(IGPGKeySet).getByFingerprint(
-                default_ppa.signing_key_fingerprint))
+                another_ppa.signing_key_fingerprint))
         signing_key = getUtility(ISigningKeySet).get(
-            SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint)
+            SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint)
         self.assertEqual(test_key, signing_key.public_key)
-        self.assertThat(another_ppa, MatchesStructure(
-            signing_key=Is(None),
-            signing_key_owner=Equals(default_ppa.signing_key_owner),
-            signing_key_fingerprint=Equals(
-                default_ppa.signing_key_fingerprint)))
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index bbe2d7c..a23a30d 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -2713,16 +2713,8 @@ class ArchiveSet:
                     "Person '%s' already has a PPA for %s named '%s'." %
                     (owner.name, distribution.name, name))
 
-        # Signing-key for the default PPA is reused when it's already present.
         signing_key_owner = None
         signing_key_fingerprint = None
-        if purpose == ArchivePurpose.PPA:
-            if owner.archive is not None:
-                signing_key_owner = owner.archive.signing_key_owner
-                signing_key_fingerprint = owner.archive.signing_key_fingerprint
-            else:
-                # owner.archive is a cached property and we've just cached it.
-                del get_property_cache(owner).archive
 
         new_archive = Archive(
             owner=owner, distribution=distribution, name=name,
diff --git a/lib/lp/soyuz/stories/webservice/xx-archive.txt b/lib/lp/soyuz/stories/webservice/xx-archive.txt
index 8e2ddc6..74fba4e 100644
--- a/lib/lp/soyuz/stories/webservice/xx-archive.txt
+++ b/lib/lp/soyuz/stories/webservice/xx-archive.txt
@@ -1226,7 +1226,7 @@ the IArchive context, in this case only Celso has it.
     require_virtualized: True
     resource_type_link: 'http://.../#archive'
     self_link: 'http://.../~cprov/+archive/ubuntu/p3a'
-    signing_key_fingerprint: 'ABCDEF0123456789ABCDDCBA0000111112345678'
+    signing_key_fingerprint: None
     suppress_subscription_notifications: False
     web_link: 'http://launchpad.../~cprov/+archive/ubuntu/p3a'
 
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index 5c3f3b6..a59a3fe 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -64,10 +64,7 @@ from lp.services.gpg.interfaces import (
     IGPGHandler,
     )
 from lp.services.job.interfaces.job import JobStatus
-from lp.services.propertycache import (
-    clear_property_cache,
-    get_property_cache,
-    )
+from lp.services.propertycache import clear_property_cache
 from lp.services.timeout import default_timeout
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.worlddata.interfaces.country import ICountrySet
@@ -1869,13 +1866,15 @@ class TestArchiveDependencies(TestCaseWithFactory):
     def test_private_sources_list(self):
         """Entries for private dependencies include credentials."""
         p3a = self.factory.makeArchive(name='p3a', private=True)
+        dependency = self.factory.makeArchive(
+            name='dependency', private=True, owner=p3a.owner)
         with InProcessKeyServerFixture() as keyserver:
             yield keyserver.start()
             key_path = os.path.join(gpgkeysdir, '[e-mail]')
             yield IArchiveGPGSigningKey(p3a).setSigningKey(
                 key_path, async_keyserver=True)
-        dependency = self.factory.makeArchive(
-            name='dependency', private=True, owner=p3a.owner)
+            yield IArchiveGPGSigningKey(dependency).setSigningKey(
+                key_path, async_keyserver=True)
         with person_logged_in(p3a.owner):
             bpph = self.factory.makeBinaryPackagePublishingHistory(
                 archive=dependency, status=PackagePublishingStatus.PUBLISHED,
@@ -4039,31 +4038,6 @@ class TestDisplayName(TestCaseWithFactory):
             archive.displayname = "My testing packages"
 
 
-class TestSigningKeyPropagation(TestCaseWithFactory):
-    """Signing keys are shared between PPAs owned by the same person/team."""
-
-    layer = DatabaseFunctionalLayer
-
-    def test_ppa_created_with_no_signing_key(self):
-        ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
-        self.assertIsNone(ppa.signing_key_fingerprint)
-
-    def test_default_signing_key_propagated_to_new_ppa(self):
-        person = self.factory.makePerson()
-        ppa = self.factory.makeArchive(
-            owner=person, purpose=ArchivePurpose.PPA, name="ppa")
-        self.assertEqual(ppa, person.archive)
-        self.factory.makeGPGKey(person)
-        key = person.gpg_keys[0]
-        removeSecurityProxy(person.archive).signing_key_owner = key.owner
-        removeSecurityProxy(person.archive).signing_key_fingerprint = (
-            key.fingerprint)
-        del get_property_cache(person.archive).signing_key
-        ppa_with_key = self.factory.makeArchive(
-            owner=person, purpose=ArchivePurpose.PPA)
-        self.assertEqual(person.gpg_keys[0], ppa_with_key.signing_key)
-
-
 class TestGetSigningKeyData(TestCaseWithFactory):
     """Test `Archive.getSigningKeyData`.