Merge ~cjwatson/launchpad:stop-ppa-key-propagation into launchpad: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
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.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
2index 4c3688d..2c09afa 100644
3--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
4+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
5@@ -14,7 +14,6 @@ __all__ = [
6 import os
7
8 import gpgme
9-from twisted.internet import defer
10 from twisted.internet.threads import deferToThread
11 from zope.component import getUtility
12 from zope.interface import implementer
13@@ -250,32 +249,6 @@ class ArchiveGPGSigningKey(SignableArchive):
14 assert self.archive.signing_key_fingerprint is None, (
15 "Cannot override signing_keys.")
16
17- # Always generate signing keys for the default PPA, even if it
18- # was not specifically requested. The default PPA signing key
19- # is then propagated to the context named-ppa.
20- default_ppa = self.archive.owner.archive
21- if self.archive != default_ppa:
22- def propagate_key(_):
23- self.archive.signing_key_owner = default_ppa.signing_key_owner
24- self.archive.signing_key_fingerprint = (
25- default_ppa.signing_key_fingerprint)
26- del get_property_cache(self.archive).signing_key
27-
28- if default_ppa.signing_key_fingerprint is None:
29- d = IArchiveGPGSigningKey(default_ppa).generateSigningKey(
30- log=log, async_keyserver=async_keyserver)
31- else:
32- d = defer.succeed(None)
33- # generateSigningKey is only asynchronous if async_keyserver is
34- # true; we need some contortions to keep it synchronous
35- # otherwise.
36- if async_keyserver:
37- d.addCallback(propagate_key)
38- return d
39- else:
40- propagate_key(None)
41- return
42-
43 key_displayname = (
44 "Launchpad PPA for %s" % self.archive.owner.displayname)
45 if getFeatureFlag(PUBLISHER_GPG_USES_SIGNING_SERVICE):
46diff --git a/lib/lp/archivepublisher/tests/archive-signing.txt b/lib/lp/archivepublisher/tests/archive-signing.txt
47index a487a3b..7e06104 100644
48--- a/lib/lp/archivepublisher/tests/archive-signing.txt
49+++ b/lib/lp/archivepublisher/tests/archive-signing.txt
50@@ -266,72 +266,24 @@ key is available in the keyserver.
51 >>> retrieved_key.fingerprint == signing_key.fingerprint
52 True
53
54-As documented in archive.txt, when a named-ppa is created it is
55-already configured to used the same signing-key created for the
56-default PPA. We will create a named-ppa for Celso.
57+We will create a named PPA for Celso.
58
59 >>> from lp.soyuz.enums import ArchivePurpose
60 >>> named_ppa = getUtility(IArchiveSet).new(
61 ... owner=cprov, purpose=ArchivePurpose.PPA, name='boing')
62
63-As expected it will use the same key used in Celso's default PPA.
64+It initially has no signing key.
65
66- >>> print cprov.archive.signing_key.fingerprint
67+ >>> print(cprov.archive.signing_key_fingerprint)
68 0D57E99656BEFB0897606EE9A022DD1F5001B46D
69
70- >>> print named_ppa.signing_key.fingerprint
71- 0D57E99656BEFB0897606EE9A022DD1F5001B46D
72-
73-We will reset the signing key of the just created named PPA,
74-simulating the situation when a the default PPA and a named-ppas get
75-created within the same cycle of the key-generator process.
76-
77- >>> from lp.services.propertycache import get_property_cache
78- >>> login('foo.bar@canonical.com')
79- >>> named_ppa.signing_key_owner = None
80- >>> named_ppa.signing_key_fingerprint = None
81- >>> del get_property_cache(named_ppa).signing_key
82- >>> login(ANONYMOUS)
83-
84- >>> print named_ppa.signing_key
85+ >>> print(named_ppa.signing_key_fingerprint)
86 None
87
88 Default PPAs are always created first and thus get their keys generated
89-before the named-ppa for the same owner. We submit the named-ppa to
90-the key generation procedure, as it would be normally in production.
91-
92- >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
93- >>> named_ppa_signing_key.generateSigningKey()
94-
95-Instead of generating a new key, the signing key from the default ppa
96-(Celso's default PPA) gets reused.
97-
98- >>> print cprov.archive.signing_key.fingerprint
99- 0D57E99656BEFB0897606EE9A022DD1F5001B46D
100-
101- >>> print named_ppa.signing_key.fingerprint
102- 0D57E99656BEFB0897606EE9A022DD1F5001B46D
103-
104-We will reset the signing-keys for both PPA of Celso.
105-
106- >>> login('foo.bar@canonical.com')
107- >>> cprov.archive.signing_key_owner = None
108- >>> cprov.archive.signing_key_fingerprint = None
109- >>> del get_property_cache(cprov.archive).signing_key
110- >>> named_ppa.signing_key_owner = None
111- >>> named_ppa.signing_key_fingerprint = None
112- >>> del get_property_cache(named_ppa).signing_key
113- >>> login(ANONYMOUS)
114-
115- >>> print cprov.archive.signing_key
116- None
117-
118- >>> print named_ppa.signing_key
119- None
120-
121-Then modify the GPGHandler utility to return a sampledata key instead
122-of generating a new one, mainly for running the test faster and for
123-printing the context the key is generated.
124+before the named PPA for the same owner. As before, generating a real key
125+is slow, so we modify the GPGHandler utility to return a sampledata key
126+instead.
127
128 >>> def mock_key_generator(name, logger=None):
129 ... print 'Generating:', name
130@@ -343,19 +295,18 @@ printing the context the key is generated.
131 >>> real_key_generator = naked_gpghandler.generateKey
132 >>> naked_gpghandler.generateKey = mock_key_generator
133
134-When the signing key for the named-ppa is requested, it is generated
135-in the default PPA context then propagated to the named-ppa. The key is
136-named after the user, even if the default PPA name is something different.
137+The key is named after the user, even if the default PPA name is something
138+different.
139
140 >>> cprov.display_name = "Not Celso Providelo"
141 >>> named_ppa_signing_key = IArchiveGPGSigningKey(named_ppa)
142 >>> named_ppa_signing_key.generateSigningKey()
143 Generating: Launchpad PPA for Not Celso Providelo
144
145- >>> print cprov.archive.signing_key.fingerprint
146- 447DBF38C4F9C4ED752246B77D88913717B05A8F
147+ >>> print(cprov.archive.signing_key_fingerprint)
148+ 0D57E99656BEFB0897606EE9A022DD1F5001B46D
149
150- >>> print named_ppa.signing_key.fingerprint
151+ >>> print(named_ppa.signing_key_fingerprint)
152 447DBF38C4F9C4ED752246B77D88913717B05A8F
153
154 Restore the original functionality of GPGHandler.
155@@ -371,6 +322,7 @@ for archive which already contains a 'signing_key'.
156
157 Celso's default PPA will uses the testing signing key.
158
159+ >>> from lp.services.propertycache import get_property_cache
160 >>> login('foo.bar@canonical.com')
161 >>> cprov.archive.signing_key_owner = signing_key.owner
162 >>> cprov.archive.signing_key_fingerprint = signing_key.fingerprint
163diff --git a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
164index b521e85..8fefb07 100644
165--- a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
166+++ b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
167@@ -345,8 +345,9 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
168 @defer.inlineCallbacks
169 def test_generateSigningKey_local_non_default_ppa(self):
170 # Generating a signing key locally using GPGHandler for a
171- # non-default PPA generates one for the user's default PPA first and
172- # then propagates it.
173+ # non-default PPA ignores the user's default PPA. (It used to
174+ # generate one for the user's default PPA first and then propagate
175+ # it.)
176 self.useFixture(FakeGenerateKey("ppa-sample@canonical.com.sec"))
177 logger = BufferLogger()
178 # Use a display name that matches the pregenerated sample key.
179@@ -357,19 +358,19 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
180 yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(
181 log=logger, async_keyserver=True)
182 self.assertThat(default_ppa, MatchesStructure(
183+ signing_key=Is(None),
184+ signing_key_owner=Is(None),
185+ signing_key_fingerprint=Is(None)))
186+ self.assertThat(another_ppa, MatchesStructure(
187 signing_key=Not(Is(None)),
188 signing_key_owner=Not(Is(None)),
189 signing_key_fingerprint=Not(Is(None))))
190 self.assertIsNotNone(
191 getUtility(IGPGKeySet).getByFingerprint(
192- default_ppa.signing_key_fingerprint))
193+ another_ppa.signing_key_fingerprint))
194 self.assertIsNone(
195 getUtility(ISigningKeySet).get(
196- SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint))
197- self.assertThat(another_ppa, MatchesStructure.byEquality(
198- signing_key=default_ppa.signing_key,
199- signing_key_owner=default_ppa.signing_key_owner,
200- signing_key_fingerprint=default_ppa.signing_key_fingerprint))
201+ SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint))
202
203 @defer.inlineCallbacks
204 def test_generateSigningKey_signing_service(self):
205@@ -411,8 +412,8 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
206 @defer.inlineCallbacks
207 def test_generateSigningKey_signing_service_non_default_ppa(self):
208 # Generating a signing key on the signing service for a non-default
209- # PPA generates one for the user's default PPA first and then
210- # propagates it.
211+ # PPA ignores the user's default PPA. (It used to generate one for
212+ # the user's default PPA first and then propagate it.)
213 self.useFixture(
214 FeatureFixture({PUBLISHER_GPG_USES_SIGNING_SERVICE: "on"}))
215 signing_service_client = self.useFixture(
216@@ -430,16 +431,15 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
217 log=logger, async_keyserver=True)
218 self.assertThat(default_ppa, MatchesStructure(
219 signing_key=Is(None),
220+ signing_key_owner=Is(None),
221+ signing_key_fingerprint=Is(None)))
222+ self.assertThat(another_ppa, MatchesStructure(
223+ signing_key=Is(None),
224 signing_key_owner=Not(Is(None)),
225 signing_key_fingerprint=Not(Is(None))))
226 self.assertIsNone(
227 getUtility(IGPGKeySet).getByFingerprint(
228- default_ppa.signing_key_fingerprint))
229+ another_ppa.signing_key_fingerprint))
230 signing_key = getUtility(ISigningKeySet).get(
231- SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint)
232+ SigningKeyType.OPENPGP, another_ppa.signing_key_fingerprint)
233 self.assertEqual(test_key, signing_key.public_key)
234- self.assertThat(another_ppa, MatchesStructure(
235- signing_key=Is(None),
236- signing_key_owner=Equals(default_ppa.signing_key_owner),
237- signing_key_fingerprint=Equals(
238- default_ppa.signing_key_fingerprint)))
239diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
240index bbe2d7c..a23a30d 100644
241--- a/lib/lp/soyuz/model/archive.py
242+++ b/lib/lp/soyuz/model/archive.py
243@@ -2713,16 +2713,8 @@ class ArchiveSet:
244 "Person '%s' already has a PPA for %s named '%s'." %
245 (owner.name, distribution.name, name))
246
247- # Signing-key for the default PPA is reused when it's already present.
248 signing_key_owner = None
249 signing_key_fingerprint = None
250- if purpose == ArchivePurpose.PPA:
251- if owner.archive is not None:
252- signing_key_owner = owner.archive.signing_key_owner
253- signing_key_fingerprint = owner.archive.signing_key_fingerprint
254- else:
255- # owner.archive is a cached property and we've just cached it.
256- del get_property_cache(owner).archive
257
258 new_archive = Archive(
259 owner=owner, distribution=distribution, name=name,
260diff --git a/lib/lp/soyuz/stories/webservice/xx-archive.txt b/lib/lp/soyuz/stories/webservice/xx-archive.txt
261index 8e2ddc6..74fba4e 100644
262--- a/lib/lp/soyuz/stories/webservice/xx-archive.txt
263+++ b/lib/lp/soyuz/stories/webservice/xx-archive.txt
264@@ -1226,7 +1226,7 @@ the IArchive context, in this case only Celso has it.
265 require_virtualized: True
266 resource_type_link: 'http://.../#archive'
267 self_link: 'http://.../~cprov/+archive/ubuntu/p3a'
268- signing_key_fingerprint: 'ABCDEF0123456789ABCDDCBA0000111112345678'
269+ signing_key_fingerprint: None
270 suppress_subscription_notifications: False
271 web_link: 'http://launchpad.../~cprov/+archive/ubuntu/p3a'
272
273diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
274index 5c3f3b6..a59a3fe 100644
275--- a/lib/lp/soyuz/tests/test_archive.py
276+++ b/lib/lp/soyuz/tests/test_archive.py
277@@ -64,10 +64,7 @@ from lp.services.gpg.interfaces import (
278 IGPGHandler,
279 )
280 from lp.services.job.interfaces.job import JobStatus
281-from lp.services.propertycache import (
282- clear_property_cache,
283- get_property_cache,
284- )
285+from lp.services.propertycache import clear_property_cache
286 from lp.services.timeout import default_timeout
287 from lp.services.webapp.interfaces import OAuthPermission
288 from lp.services.worlddata.interfaces.country import ICountrySet
289@@ -1869,13 +1866,15 @@ class TestArchiveDependencies(TestCaseWithFactory):
290 def test_private_sources_list(self):
291 """Entries for private dependencies include credentials."""
292 p3a = self.factory.makeArchive(name='p3a', private=True)
293+ dependency = self.factory.makeArchive(
294+ name='dependency', private=True, owner=p3a.owner)
295 with InProcessKeyServerFixture() as keyserver:
296 yield keyserver.start()
297 key_path = os.path.join(gpgkeysdir, 'ppa-sample@canonical.com.sec')
298 yield IArchiveGPGSigningKey(p3a).setSigningKey(
299 key_path, async_keyserver=True)
300- dependency = self.factory.makeArchive(
301- name='dependency', private=True, owner=p3a.owner)
302+ yield IArchiveGPGSigningKey(dependency).setSigningKey(
303+ key_path, async_keyserver=True)
304 with person_logged_in(p3a.owner):
305 bpph = self.factory.makeBinaryPackagePublishingHistory(
306 archive=dependency, status=PackagePublishingStatus.PUBLISHED,
307@@ -4039,31 +4038,6 @@ class TestDisplayName(TestCaseWithFactory):
308 archive.displayname = "My testing packages"
309
310
311-class TestSigningKeyPropagation(TestCaseWithFactory):
312- """Signing keys are shared between PPAs owned by the same person/team."""
313-
314- layer = DatabaseFunctionalLayer
315-
316- def test_ppa_created_with_no_signing_key(self):
317- ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
318- self.assertIsNone(ppa.signing_key_fingerprint)
319-
320- def test_default_signing_key_propagated_to_new_ppa(self):
321- person = self.factory.makePerson()
322- ppa = self.factory.makeArchive(
323- owner=person, purpose=ArchivePurpose.PPA, name="ppa")
324- self.assertEqual(ppa, person.archive)
325- self.factory.makeGPGKey(person)
326- key = person.gpg_keys[0]
327- removeSecurityProxy(person.archive).signing_key_owner = key.owner
328- removeSecurityProxy(person.archive).signing_key_fingerprint = (
329- key.fingerprint)
330- del get_property_cache(person.archive).signing_key
331- ppa_with_key = self.factory.makeArchive(
332- owner=person, purpose=ArchivePurpose.PPA)
333- self.assertEqual(person.gpg_keys[0], ppa_with_key.signing_key)
334-
335-
336 class TestGetSigningKeyData(TestCaseWithFactory):
337 """Test `Archive.getSigningKeyData`.
338

Subscribers

People subscribed via source and target branches

to status/vote changes: