Launchpad itself

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

  1. Git
  2. lp:~cjwatson/launchpad
  3. stop-ppa-key-propagation
  4. Merge into master
Proposed by Colin Watson
Status: Needs review
Proposed branch: ~cjwatson/launchpad:stop-ppa-key-propagation
Merge into: launchpad:master
Diff against target: 337 lines (+36/-145)
6 files modified
lib/lp/archivepublisher/archivegpgsigningkey.py (+0/-27)
lib/lp/archivepublisher/tests/archive-signing.txt (+13/-61)
lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py (+17/-17)
lib/lp/soyuz/model/archive.py (+0/-8)
lib/lp/soyuz/stories/webservice/xx-archive.txt (+1/-1)
lib/lp/soyuz/tests/test_archive.py (+5/-31)
Reviewer Review Type Date Requested Status
Robert Hardy (community) Needs Fixing
Launchpad code reviewers Pending
Review via email: mp+392627@code.launchpad.net

Commit message

Stop propagating signing keys between an owner's PPAs

Description of the change

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.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

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.

Reply
Revision history for this message
Robert Hardy (rhardy) wrote :

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
Reply

Unmerged commits

ffc336d... by 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.

LP: #1700167

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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__ = [
14import os14import os
1515
16import gpgme16import gpgme
17from twisted.internet import defer
18from twisted.internet.threads import deferToThread17from twisted.internet.threads import deferToThread
19from zope.component import getUtility18from zope.component import getUtility
20from zope.interface import implementer19from zope.interface import implementer
@@ -250,32 +249,6 @@ class ArchiveGPGSigningKey(SignableArchive):
250 assert self.archive.signing_key_fingerprint is None, (249 assert self.archive.signing_key_fingerprint is None, (
251 "Cannot override signing_keys.")250 "Cannot override signing_keys.")
252251
253 # Always generate signing keys for the default PPA, even if it
254 # was not specifically requested. The default PPA signing key
255 # is then propagated to the context named-ppa.
256 default_ppa = self.archive.owner.archive
257 if self.archive != default_ppa:
258 def propagate_key(_):
259 self.archive.signing_key_owner = default_ppa.signing_key_owner
260 self.archive.signing_key_fingerprint = (
261 default_ppa.signing_key_fingerprint)
262 del get_property_cache(self.archive).signing_key
263
264 if default_ppa.signing_key_fingerprint is None:
265 d = IArchiveGPGSigningKey(default_ppa).generateSigningKey(
266 log=log, async_keyserver=async_keyserver)
267 else:
268 d = defer.succeed(None)
269 # generateSigningKey is only asynchronous if async_keyserver is
270 # true; we need some contortions to keep it synchronous
271 # otherwise.
272 if async_keyserver:
273 d.addCallback(propagate_key)
274 return d
275 else:
276 propagate_key(None)
277 return
278
279 key_displayname = (252 key_displayname = (
280 "Launchpad PPA for %s" % self.archive.owner.displayname)253 "Launchpad PPA for %s" % self.archive.owner.displayname)
281 if getFeatureFlag(PUBLISHER_GPG_USES_SIGNING_SERVICE):254 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.
266 >>> retrieved_key.fingerprint == signing_key.fingerprint266 >>> retrieved_key.fingerprint == signing_key.fingerprint
267 True267 True
268268
269As documented in archive.txt, when a named-ppa is created it is269We will create a named PPA for Celso.
270already configured to used the same signing-key created for the
271default PPA. We will create a named-ppa for Celso.
272270
273 >>> from lp.soyuz.enums import ArchivePurpose271 >>> from lp.soyuz.enums import ArchivePurpose
274 >>> named_ppa = getUtility(IArchiveSet).new(272 >>> named_ppa = getUtility(IArchiveSet).new(
275 ... owner=cprov, purpose=ArchivePurpose.PPA, name='boing')273 ... owner=cprov, purpose=ArchivePurpose.PPA, name='boing')
276274
277As expected it will use the same key used in Celso's default PPA.275It initially has no signing key.
278276
279 >>> print cprov.archive.signing_key.fingerprint277 >>> print(cprov.archive.signing_key_fingerprint)
280 0D57E99656BEFB0897606EE9A022DD1F5001B46D278 0D57E99656BEFB0897606EE9A022DD1F5001B46D
281279
282 >>> print named_ppa.signing_key.fingerprint280 >>> print(named_ppa.signing_key_fingerprint)
283 0D57E99656BEFB0897606EE9A022DD1F5001B46D
284
285We will reset the signing key of the just created named PPA,
286simulating the situation when a the default PPA and a named-ppas get
287created within the same cycle of the key-generator process.
288
289 >>> from lp.services.propertycache import get_property_cache
290 >>> login('foo.bar@canonical.com')
291 >>> named_ppa.signing_key_owner = None
292 >>> named_ppa.signing_key_fingerprint = None
293 >>> del get_property_cache(named_ppa).signing_key
294 >>> login(ANONYMOUS)
295
296 >>> print named_ppa.signing_key
297 None281 None
298282
299Default PPAs are always created first and thus get their keys generated283Default PPAs are always created first and thus get their keys generated
300before the named-ppa for the same owner. We submit the named-ppa to284before the named PPA for the same owner. As before, generating a real key
301the key generation procedure, as it would be normally in production.285is slow, so we modify the GPGHandler utility to return a sampledata key
302286instead.
303 >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
304 >>> named_ppa_signing_key.generateSigningKey()
305
306Instead of generating a new key, the signing key from the default ppa
307(Celso's default PPA) gets reused.
308
309 >>> print cprov.archive.signing_key.fingerprint
310 0D57E99656BEFB0897606EE9A022DD1F5001B46D
311
312 >>> print named_ppa.signing_key.fingerprint
313 0D57E99656BEFB0897606EE9A022DD1F5001B46D
314
315We will reset the signing-keys for both PPA of Celso.
316
317 >>> login('foo.bar@canonical.com')
318 >>> cprov.archive.signing_key_owner = None
319 >>> cprov.archive.signing_key_fingerprint = None
320 >>> del get_property_cache(cprov.archive).signing_key
321 >>> named_ppa.signing_key_owner = None
322 >>> named_ppa.signing_key_fingerprint = None
323 >>> del get_property_cache(named_ppa).signing_key
324 >>> login(ANONYMOUS)
325
326 >>> print cprov.archive.signing_key
327 None
328
329 >>> print named_ppa.signing_key
330 None
331
332Then modify the GPGHandler utility to return a sampledata key instead
333of generating a new one, mainly for running the test faster and for
334printing the context the key is generated.
335287
336 >>> def mock_key_generator(name, logger=None):288 >>> def mock_key_generator(name, logger=None):
337 ... print 'Generating:', name289 ... print 'Generating:', name
@@ -343,19 +295,18 @@ printing the context the key is generated.
343 >>> real_key_generator = naked_gpghandler.generateKey295 >>> real_key_generator = naked_gpghandler.generateKey
344 >>> naked_gpghandler.generateKey = mock_key_generator296 >>> naked_gpghandler.generateKey = mock_key_generator
345297
346When the signing key for the named-ppa is requested, it is generated298The key is named after the user, even if the default PPA name is something
347in the default PPA context then propagated to the named-ppa. The key is299different.
348named after the user, even if the default PPA name is something different.
349300
350 >>> cprov.display_name = "Not Celso Providelo"301 >>> cprov.display_name = "Not Celso Providelo"
351 >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)302 >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
352 >>> named_ppa_signing_key.generateSigningKey()303 >>> named_ppa_signing_key.generateSigningKey()
353 Generating: Launchpad PPA for Not Celso Providelo304 Generating: Launchpad PPA for Not Celso Providelo
354305
355 >>> print cprov.archive.signing_key.fingerprint306 >>> print(cprov.archive.signing_key_fingerprint)
356 447DBF38C4F9C4ED752246B77D88913717B05A8F307 0D57E99656BEFB0897606EE9A022DD1F5001B46D
357308
358 >>> print named_ppa.signing_key.fingerprint309 >>> print(named_ppa.signing_key_fingerprint)
359 447DBF38C4F9C4ED752246B77D88913717B05A8F310 447DBF38C4F9C4ED752246B77D88913717B05A8F
360311
361Restore the original functionality of GPGHandler.312Restore the original functionality of GPGHandler.
@@ -371,6 +322,7 @@ for archive which already contains a 'signing_key'.
371322
372Celso's default PPA will uses the testing signing key.323Celso's default PPA will uses the testing signing key.
373324
325 >>> from lp.services.propertycache import get_property_cache
374 >>> login('foo.bar@canonical.com')326 >>> login('foo.bar@canonical.com')
375 >>> cprov.archive.signing_key_owner = signing_key.owner327 >>> cprov.archive.signing_key_owner = signing_key.owner
376 >>> cprov.archive.signing_key_fingerprint = signing_key.fingerprint328 >>> 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):
345 @defer.inlineCallbacks345 @defer.inlineCallbacks
346 def test_generateSigningKey_local_non_default_ppa(self):346 def test_generateSigningKey_local_non_default_ppa(self):
347 # Generating a signing key locally using GPGHandler for a347 # Generating a signing key locally using GPGHandler for a
348 # non-default PPA generates one for the user's default PPA first and348 # non-default PPA ignores the user's default PPA. (It used to
349 # then propagates it.349 # generate one for the user's default PPA first and then propagate
350 # it.)
350 self.useFixture(FakeGenerateKey("ppa-sample@canonical.com.sec"))351 self.useFixture(FakeGenerateKey("ppa-sample@canonical.com.sec"))
351 logger = BufferLogger()352 logger = BufferLogger()
352 # Use a display name that matches the pregenerated sample key.353 # Use a display name that matches the pregenerated sample key.
@@ -357,19 +358,19 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
357 yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(358 yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(
358 log=logger, async_keyserver=True)359 log=logger, async_keyserver=True)
359 self.assertThat(default_ppa, MatchesStructure(360 self.assertThat(default_ppa, MatchesStructure(
361 signing_key=Is(None),
362 signing_key_owner=Is(None),
363 signing_key_fingerprint=Is(None)))
364 self.assertThat(another_ppa, MatchesStructure(
360 signing_key=Not(Is(None)),365 signing_key=Not(Is(None)),
361 signing_key_owner=Not(Is(None)),366 signing_key_owner=Not(Is(None)),
362 signing_key_fingerprint=Not(Is(None))))367 signing_key_fingerprint=Not(Is(None))))
363 self.assertIsNotNone(368 self.assertIsNotNone(
364 getUtility(IGPGKeySet).getByFingerprint(369 getUtility(IGPGKeySet).getByFingerprint(
365 default_ppa.signing_key_fingerprint))370 another_ppa.signing_key_fingerprint))
366 self.assertIsNone(371 self.assertIsNone(
367 getUtility(ISigningKeySet).get(372 getUtility(ISigningKeySet).get(
368 SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint))373 SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint))
369 self.assertThat(another_ppa, MatchesStructure.byEquality(
370 signing_key=default_ppa.signing_key,
371 signing_key_owner=default_ppa.signing_key_owner,
372 signing_key_fingerprint=default_ppa.signing_key_fingerprint))
373374
374 @defer.inlineCallbacks375 @defer.inlineCallbacks
375 def test_generateSigningKey_signing_service(self):376 def test_generateSigningKey_signing_service(self):
@@ -411,8 +412,8 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
411 @defer.inlineCallbacks412 @defer.inlineCallbacks
412 def test_generateSigningKey_signing_service_non_default_ppa(self):413 def test_generateSigningKey_signing_service_non_default_ppa(self):
413 # Generating a signing key on the signing service for a non-default414 # Generating a signing key on the signing service for a non-default
414 # PPA generates one for the user's default PPA first and then415 # PPA ignores the user's default PPA. (It used to generate one for
415 # propagates it.416 # the user's default PPA first and then propagate it.)
416 self.useFixture(417 self.useFixture(
417 FeatureFixture({PUBLISHER_GPG_USES_SIGNING_SERVICE: "on"}))418 FeatureFixture({PUBLISHER_GPG_USES_SIGNING_SERVICE: "on"}))
418 signing_service_client = self.useFixture(419 signing_service_client = self.useFixture(
@@ -430,16 +431,15 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
430 log=logger, async_keyserver=True)431 log=logger, async_keyserver=True)
431 self.assertThat(default_ppa, MatchesStructure(432 self.assertThat(default_ppa, MatchesStructure(
432 signing_key=Is(None),433 signing_key=Is(None),
434 signing_key_owner=Is(None),
435 signing_key_fingerprint=Is(None)))
436 self.assertThat(another_ppa, MatchesStructure(
437 signing_key=Is(None),
433 signing_key_owner=Not(Is(None)),438 signing_key_owner=Not(Is(None)),
434 signing_key_fingerprint=Not(Is(None))))439 signing_key_fingerprint=Not(Is(None))))
435 self.assertIsNone(440 self.assertIsNone(
436 getUtility(IGPGKeySet).getByFingerprint(441 getUtility(IGPGKeySet).getByFingerprint(
437 default_ppa.signing_key_fingerprint))442 another_ppa.signing_key_fingerprint))
438 signing_key = getUtility(ISigningKeySet).get(443 signing_key = getUtility(ISigningKeySet).get(
439 SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint)444 SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint)
440 self.assertEqual(test_key, signing_key.public_key)445 self.assertEqual(test_key, signing_key.public_key)
441 self.assertThat(another_ppa, MatchesStructure(
442 signing_key=Is(None),
443 signing_key_owner=Equals(default_ppa.signing_key_owner),
444 signing_key_fingerprint=Equals(
445 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:
2713 "Person '%s' already has a PPA for %s named '%s'." %2713 "Person '%s' already has a PPA for %s named '%s'." %
2714 (owner.name, distribution.name, name))2714 (owner.name, distribution.name, name))
27152715
2716 # Signing-key for the default PPA is reused when it's already present.
2717 signing_key_owner = None2716 signing_key_owner = None
2718 signing_key_fingerprint = None2717 signing_key_fingerprint = None
2719 if purpose == ArchivePurpose.PPA:
2720 if owner.archive is not None:
2721 signing_key_owner = owner.archive.signing_key_owner
2722 signing_key_fingerprint = owner.archive.signing_key_fingerprint
2723 else:
2724 # owner.archive is a cached property and we've just cached it.
2725 del get_property_cache(owner).archive
27262718
2727 new_archive = Archive(2719 new_archive = Archive(
2728 owner=owner, distribution=distribution, name=name,2720 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.
1226 require_virtualized: True1226 require_virtualized: True
1227 resource_type_link: 'http://.../#archive'1227 resource_type_link: 'http://.../#archive'
1228 self_link: 'http://.../~cprov/+archive/ubuntu/p3a'1228 self_link: 'http://.../~cprov/+archive/ubuntu/p3a'
1229 signing_key_fingerprint: 'ABCDEF0123456789ABCDDCBA0000111112345678'1229 signing_key_fingerprint: None
1230 suppress_subscription_notifications: False1230 suppress_subscription_notifications: False
1231 web_link: 'http://launchpad.../~cprov/+archive/ubuntu/p3a'1231 web_link: 'http://launchpad.../~cprov/+archive/ubuntu/p3a'
12321232
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 (
64 IGPGHandler,64 IGPGHandler,
65 )65 )
66from lp.services.job.interfaces.job import JobStatus66from lp.services.job.interfaces.job import JobStatus
67from lp.services.propertycache import (67from lp.services.propertycache import clear_property_cache
68 clear_property_cache,
69 get_property_cache,
70 )
71from lp.services.timeout import default_timeout68from lp.services.timeout import default_timeout
72from lp.services.webapp.interfaces import OAuthPermission69from lp.services.webapp.interfaces import OAuthPermission
73from lp.services.worlddata.interfaces.country import ICountrySet70from lp.services.worlddata.interfaces.country import ICountrySet
@@ -1869,13 +1866,15 @@ class TestArchiveDependencies(TestCaseWithFactory):
1869 def test_private_sources_list(self):1866 def test_private_sources_list(self):
1870 """Entries for private dependencies include credentials."""1867 """Entries for private dependencies include credentials."""
1871 p3a = self.factory.makeArchive(name='p3a', private=True)1868 p3a = self.factory.makeArchive(name='p3a', private=True)
1869 dependency = self.factory.makeArchive(
1870 name='dependency', private=True, owner=p3a.owner)
1872 with InProcessKeyServerFixture() as keyserver:1871 with InProcessKeyServerFixture() as keyserver:
1873 yield keyserver.start()1872 yield keyserver.start()
1874 key_path = os.path.join(gpgkeysdir, 'ppa-sample@canonical.com.sec')1873 key_path = os.path.join(gpgkeysdir, 'ppa-sample@canonical.com.sec')
1875 yield IArchiveGPGSigningKey(p3a).setSigningKey(1874 yield IArchiveGPGSigningKey(p3a).setSigningKey(
1876 key_path, async_keyserver=True)1875 key_path, async_keyserver=True)
1877 dependency = self.factory.makeArchive(1876 yield IArchiveGPGSigningKey(dependency).setSigningKey(
1878 name='dependency', private=True, owner=p3a.owner)1877 key_path, async_keyserver=True)
1879 with person_logged_in(p3a.owner):1878 with person_logged_in(p3a.owner):
1880 bpph = self.factory.makeBinaryPackagePublishingHistory(1879 bpph = self.factory.makeBinaryPackagePublishingHistory(
1881 archive=dependency, status=PackagePublishingStatus.PUBLISHED,1880 archive=dependency, status=PackagePublishingStatus.PUBLISHED,
@@ -4039,31 +4038,6 @@ class TestDisplayName(TestCaseWithFactory):
4039 archive.displayname = "My testing packages"4038 archive.displayname = "My testing packages"
40404039
40414040
4042class TestSigningKeyPropagation(TestCaseWithFactory):
4043 """Signing keys are shared between PPAs owned by the same person/team."""
4044
4045 layer = DatabaseFunctionalLayer
4046
4047 def test_ppa_created_with_no_signing_key(self):
4048 ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
4049 self.assertIsNone(ppa.signing_key_fingerprint)
4050
4051 def test_default_signing_key_propagated_to_new_ppa(self):
4052 person = self.factory.makePerson()
4053 ppa = self.factory.makeArchive(
4054 owner=person, purpose=ArchivePurpose.PPA, name="ppa")
4055 self.assertEqual(ppa, person.archive)
4056 self.factory.makeGPGKey(person)
4057 key = person.gpg_keys[0]
4058 removeSecurityProxy(person.archive).signing_key_owner = key.owner
4059 removeSecurityProxy(person.archive).signing_key_fingerprint = (
4060 key.fingerprint)
4061 del get_property_cache(person.archive).signing_key
4062 ppa_with_key = self.factory.makeArchive(
4063 owner=person, purpose=ArchivePurpose.PPA)
4064 self.assertEqual(person.gpg_keys[0], ppa_with_key.signing_key)
4065
4066
4067class TestGetSigningKeyData(TestCaseWithFactory):4041class TestGetSigningKeyData(TestCaseWithFactory):
4068 """Test `Archive.getSigningKeyData`.4042 """Test `Archive.getSigningKeyData`.
40694043

Subscribers

People subscribed via source and target branches

to status/vote changes: