Remove (deprecated) apt-key (SC-89) by holmanb · Pull Request #1068 · canonical/cloud-init

holmanb · 2021-10-14T15:17:26Z

Remove apt-key dependency.

Add the "signed by" option to source definitions. This enables users
to limit the scope of trust for individual keys.

LP: #1836336

Test Steps

Use this example config* from the docs. Start an instance and run apt-key list. Note that the keys from the config were installed in /etc/apt/trusted.gpg.d/.

*Note: this example's config is technically incorrect and may throw an exception, this should get fixed in PR #1065, but if you happen to test before that PR gets merged either define an architecture in security or remove the security source to avoid the .

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

TheRealFalcon

Thanks @holmanb , overall the approach looks good. I left some comments inline. Additionally, I think you can move the apt.py functions into cc_apt_configure.py. I don't think we'll use them outside of there. You've also got some pep8 and docstring formatting issues. I'm not going to comment individually on those, but remember to fix those up too.

holmanb · 2021-10-19T20:44:48Z

The latest commit adds support for the "signed-by" option in apt source lists.

Consider the following apt source config:

    my-repo5.list:
      source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/curtin-dev/test-archive/ubuntu xenial main"
      keyid: F430BBA5 # GPG key ID published on the key server

When $KEY_FILE is detected in the source definition, the associated key will get written to a directory that is not implicitly trusted by apt. The $KEY_FILE variable will be populated with the path of the key.

root@me:~# cat /etc/apt/sources.list.d/my-repo5.list
deb [signed-by=/etc/apt/cloud-init.gpg.d/my-repo5.gpg] http://ppa.launchpad.net/curtin-dev/test-archive/ubuntu xenial main
root@me:~# ls /etc/apt/cloud-init.gpg.d/my-repo5.gpg
/etc/apt/cloud-init.gpg.d/my-repo5.gpg

This approach attempts to minimize user impact (opt-in), while allowing a config that limits the trust that apt gives to signing keys.

This PR requires updated docs and tests for this new feature.

TheRealFalcon

Left a few more comments inline, but again nothing major. Overall this is looking really good!

This has nothing to do with what you're doing, but while you're in the code and have a better understanding than you will in 3 months, it might be worth updating the docstring of add_apt_sources. There's some setup that happens before we get there, so I'm not entirely what's supposed to be in srcdict. A small example in the docstring would be helpful.

Since you're still working on tests, I didn't really look at the tests yet.

TheRealFalcon · 2021-10-20T16:03:03Z

+
+        returns filepath to new keyring
+        """
+        try:


If we catch an exception from here, we're likely to have more exceptions down the line as this will be returned as None. It looks like in one path the return here will make it into template_params and then I haven't traced what exactly happens in source = templater.render_string(source, template_params), but I'm guessing it won't like None.

Good catch. I think that returning the string "/dev/null" might be appropriate here. It looks like apt behaves differently when "signed-by=" points to a garbage value. Giving apt a value that is actually a file seems sensible. It will still throw an exception (due to rc=100), and it will hopefully signal to a user that is debugging that "signed by /dev/null is obviously a garbage value".

…ed .gpg

TheRealFalcon

Not fully through the review, but I'm having trouble running the integration tests.

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/cloudinit/config/cc_apt_configure.py", line 788, in add_apt_sources
    subp.subp(["add-apt-repository", source], target=target)
  File "/usr/lib/python3/dist-packages/cloudinit/subp.py", line 298, in subp
    cmd=args)
cloudinit.subp.ProcessExecutionError: Unexpected error while running command.
Command: ['add-apt-repository', 'ppa:simplestreams-dev/trunk']
Exit code: 100
Reason: -
Stdout:  
         More info: https://launchpad.net/~simplestreams-dev/+archive/ubuntu/trunk
        Err:1 http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu bionic InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
        Err:2 http://badarchive.ubuntu.com/ubuntu bionic InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
        Err:3 http://badsecurity.ubuntu.com/ubuntu bionic-security InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
        Err:4 http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu bionic InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
        Err:5 http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu bionic InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
        Reading package lists...
Stderr: E: The repository 'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu bionic InRelease' is not signed.
        E: Failed to fetch http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu/dists/bionic/InRelease  403  Forbidden [IP: 91.189.89.11 3128]
        E: The repository 'http://badarchive.ubuntu.com/ubuntu bionic InRelease' is no longer signed.
        E: Failed to fetch http://badarchive.ubuntu.com/ubuntu/dists/bionic/InRelease  403  Forbidden [IP: 91.189.89.11 3128]
        E: The repository 'http://badsecurity.ubuntu.com/ubuntu bionic-security InRelease' is no longer signed.
        E: Failed to fetch http://badsecurity.ubuntu.com/ubuntu/dists/bionic-security/InRelease  403  Forbidden [IP: 91.189.89.11 3128]
        E: Failed to fetch http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu/dists/bionic/InRelease  403  Forbidden [IP: 91.189.89.11 3128]
        E: The repository 'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu bionic InRelease' is not signed.
        E: Failed to fetch http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu/dists/bionic/InRelease  403  Forbidden [IP: 91.189.89.11 3128]
        E: The repository 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu bionic InRelease' is not signed.

I would expect the errors from the bad archive, but the other URL failures point to a problem with the key handling more generally.

Also, why are we no longer checking the same keys? Lines 176 and 226 in particular.

holmanb · 2021-10-27T16:18:29Z

Not fully through the review, but I'm having trouble running the integration tests.

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/cloudinit/config/cc_apt_configure.py", line 788, in add_apt_sources
    subp.subp(["add-apt-repository", source], target=target)
  File "/usr/lib/python3/dist-packages/cloudinit/subp.py", line 298, in subp
    cmd=args)
cloudinit.subp.ProcessExecutionError: Unexpected error while running command.
Command: ['add-apt-repository', 'ppa:simplestreams-dev/trunk']
Exit code: 100
Reason: -
Stdout:  
         More info: https://launchpad.net/~simplestreams-dev/+archive/ubuntu/trunk
        Err:1 http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu bionic InRelease
          403  Forbidden [IP: 91.189.89.11 3128]
<snip>

I didn't change anything that should have affected these tests in my latest commit, but the integration tests just passed. 403 is not a failure code I would expect for this test.

There was some chatter about Launchpad being down yesterday around 12:30pm MT, and the commit that I pushed was committed at 11:45 am (I don't know the exact time that I pushed, but seems plausible that there was a cause/effect relationship).

        E: The repository 'http://ppa.launchpad.net/cloud-init-raharper/curtin-dev/ubuntu bionic InRelease' is not signed.

I think this line was because I oopsed[1].

I would expect the errors from the bad archive, but the other URL failures point to a problem with the key handling more generally.

Also, why are we no longer checking the same keys? Lines 176 and 226 in particular.

[1] Oops.

I also added a new key to the tests to get some RSA 4096 coverage. I didn't look very hard, but I didn't see any 4096B keys. I doubt it matters, since that's handled by gpg directly, but more coverage is generally a good thing.

…erprint change.

TheRealFalcon · 2021-10-28T13:13:07Z

Thanks, I was able to run integration tests. I have a few proposed changes:

diff --git a/tests/integration_tests/modules/test_apt.py b/tests/integration_tests/modules/test_apt.py
index ee2a57264..890650a76 100644
--- a/tests/integration_tests/modules/test_apt.py
+++ b/tests/integration_tests/modules/test_apt.py
@@ -46,9 +46,9 @@ apt:
       keyserver: keyserver.ubuntu.com
       source: "ppa:simplestreams-dev/trunk"
     test_signed_by:
-      keyid: 3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85
+      keyid: A2EB2DEC0BD7519B7B38BE38376A290EC8068B11
       keyserver: keyserver.ubuntu.com
-      source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main"
+      source: "deb [signed-by=$KEY_FILE] http://ppa.launchpad.net/juju/stable/ubuntu $RELEASE main"
     test_key:
       source: "deb http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu $RELEASE main"
       key: |
@@ -93,11 +93,9 @@ EXPECTED_REGEXES = [
 ]
 
 TEST_KEYSERVER_KEY = "7260 0DB1 5B8E 4C8B 1964  B868 038A CC97 C660 A937"
-
 TEST_PPA_KEY = "3552 C902 B4DD F7BD 3842  1821 015D 28D7 4416 14D8"
-
-TEST_KEY_1 = "1FF0 D853 5EF7 E719 E5C8  1B9C 083D 06FB E4D3 04DF"
-TEST_KEY_2 = "3A3E F34D FDED B3B7 F3FD  F603 F83F 7712 9A5E BD85"
+TEST_KEY = "1FF0 D853 5EF7 E719 E5C8  1B9C 083D 06FB E4D3 04DF"
+TEST_SIGNED_BY_KEY = "A2EB 2DEC 0BD7 519B 7B38  BE38 376A 290E C806 8B11"
 
 
 @pytest.mark.ci
@@ -173,7 +171,7 @@ class TestApt:
             'http://ppa.launchpad.net/simplestreams-dev/trunk/ubuntu'
         ) in ppa_path_contents
 
-        assert TEST_KEY_1 in self.get_keys(class_client)
+        assert TEST_PPA_KEY in self.get_keys(class_client)
 
     def test_signed_by(self, class_client: IntegrationInstance):
         """Test the apt signed-by functionality.
@@ -181,7 +179,7 @@ class TestApt:
         release = ImageSpecification.from_os_image().release
         source = (
             "deb [signed-by=/etc/apt/cloud-init.gpg.d/test_signed_by.gpg] "
-            "http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu"
+            "http://ppa.launchpad.net/juju/stable/ubuntu"
             " {} main".format(release))
 
         path_contents = class_client.read_from_file(
@@ -192,7 +190,7 @@ class TestApt:
             'gpg --no-default-keyring --with-fingerprint --list-keys '
             '--keyring /etc/apt/cloud-init.gpg.d/test_signed_by.gpg')
 
-        assert TEST_KEY_2 in key
+        assert TEST_SIGNED_BY_KEY in key
 
     def test_key(self, class_client: IntegrationInstance):
         """Test the apt key functionality.
@@ -207,7 +205,7 @@ class TestApt:
         assert (
             'http://ppa.launchpad.net/cloud-init-dev/test-archive/ubuntu'
         ) in test_archive_contents
-        assert TEST_KEYSERVER_KEY in self.get_keys(class_client)
+        assert TEST_KEY in self.get_keys(class_client)
 
     def test_keyserver(self, class_client: IntegrationInstance):
         """Test the apt keyserver functionality.

I think we should use different repos/keys for different tests. I just chose a random juju one here. Also, The keys should keep the same names as their sources.list so it's easier to know which key goes with which test. Finally, we should make sure the correct keys are used with the correct tests.

TheRealFalcon

I finally got around to reviewing tests. Most of my comments are inline.

When it comes to the mock assert methods, see the second bullet here:
https://cloudinit.readthedocs.io/en/latest/topics/testing.html#mocking-and-assertions

I don't think you need to change the tests given that they already worked this way (and I verified the spelling of them), but in general we try to avoid them if they're not autospecced. Once we upgrade our python version this shouldn't be necessary as newer versions of pytest will raise on misspellings of "assert".

TheRealFalcon · 2021-10-28T13:33:05Z

+    '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85'
+
+
+class TestAptKey(helpers.FilesystemMockingTestCase):


In general, we try to avoid these base test classes in new tests unless it's a real PITA to not use them. AFAIK, your tests are mocked well enough that they still work fine without the base class or the setUp method.

Fine by me. Do we try to avoid them just to avoid unnecessary setup? Or is there another reason? I agree they aren't needed.

That and simplicity. Test functionality can become fairly opaque when you're using them. They provide a lot of base helper functionality, so sometimes it's still easiest to use them, but if we can do it just as easily with a couple of mocks, it makes the tests a lot easier to understand just using the mocks. Also, these were written before we were using pytest, which generally relies on fixtures for setup.

Base class is still here 😄

TheRealFalcon · 2021-10-28T14:33:32Z

+        with self.assertRaises(ValueError):
+            gpg.dearmor(None)
+
+    def test_dearmor_bad_value(self):


This is more of a general comment pointed at a specific example, but I don't see a whole lot of value in these types of tests. In the test, we're telling subp to do something, then verifying that subp did the thing we just told it to do. The only application code we're testing is essentially, "did subp get called at some point during this call", and there's already other tests in this class that are testing that. If we had application logic dealing with this exception, that would be different and a valid thing to check.

I think it makes more sense to have a single test that verifies subp got called with the arguments we're expecting. You can do a with mock.patch.object(...) as m_subp: and then call m_subp.call_args or m_subp.call_args_list.

That's fair. This comment forced me to take a harder look at the testing of gpg. In the latest iteration, I ripped out some not-so-great tests, added some better ones, and also came to the realization that the ValueError exception doesn't really make sense to have anyways, so I simplified one of the gpg functions too.

holmanb · 2021-10-28T22:14:56Z

I don't think you need to change the tests given that they already worked this way (and I verified the spelling of them), but in general we try to avoid them if they're not autospecced. Once we upgrade our python version this shouldn't be necessary as newer versions of pytest will raise on misspellings of "assert".

Noted

I'm starting to like the way this is shaping up. I think I've addressed all of the comments so far. Thanks for the various recommendations and pointers @TheRealFalcon!

TheRealFalcon

One more minor thing. With that and once travis passed, I think we're good to go!

TheRealFalcon · 2021-10-29T16:45:28Z

+    '3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85'
+
+
+class TestAptKey(helpers.FilesystemMockingTestCase):


Base class is still here 😄

------------------------------- - remove unnecessary base classes - use mock decorators - increase the value density of tests Additionally: ------------- - add comments to new tests to explain/justify purpose - add second integration test for failure mode - eliminated unnecessary ValueError exception from gpg.dearmor(): - empty value will lead to to ProcessExecutionError when subp.subp() gets called, and that's a more appropriate error handling path anyways (will trigger if user sets key: "") - idk I'm sure I forgot something

TheRealFalcon

Thanks @holmanb !

julian-klode · 2021-11-03T15:10:14Z

This does not seem to be super useful. It still uses gpg, the reason apt-key goes away is because gpg is not needed anymore, there should really be no gpg on images anymore. Filed as https://bugs.launchpad.net/cloud-init/+bug/1949602

holmanb marked this pull request as draft October 14, 2021 15:22

TheRealFalcon reviewed Oct 14, 2021

View reviewed changes

Comment thread cloudinit/apt.py Outdated

Comment thread cloudinit/apt.py Outdated

Comment thread cloudinit/gpg.py

Comment thread cloudinit/apt.py Outdated

Comment thread cloudinit/apt.py Outdated

Comment thread cloudinit/apt.py Outdated

holmanb force-pushed the holmanb/remove-apt-key branch 3 times, most recently from 924064a to 66e5808 Compare October 19, 2021 20:34

TheRealFalcon reviewed Oct 20, 2021

View reviewed changes

holmanb force-pushed the holmanb/remove-apt-key branch 3 times, most recently from ae80321 to 2c99b00 Compare October 26, 2021 03:01

holmanb marked this pull request as ready for review October 26, 2021 03:40

holmanb and others added 11 commits October 25, 2021 21:51

wip - remove (depreciated) apt-key

f8303d8

remove apt-key from tests

2fde927

fixup tests

ce9b5e0

Add support for apt source "signed-by" option.

cd1e350

Fix error handling code + various cleanups

f5c4817

add gpg unit tests

880568c

add apt-key tests

393e835

add integration test

d8d6ba2

if filename not known, log an error and skip - don't write a file nam…

ef56c9e

…ed .gpg

test fixup

d1c7dce

mock out subp

4371fd1

holmanb force-pushed the holmanb/remove-apt-key branch from 2c99b00 to 4371fd1 Compare October 26, 2021 03:52

holmanb changed the title ~~WIP for review - Remove (deprecated) apt-key (SC-89)~~ Remove (deprecated) apt-key (SC-89) Oct 26, 2021

add example to docs

085c688

TheRealFalcon reviewed Oct 26, 2021

View reviewed changes

Comment thread tests/integration_tests/modules/test_apt.py Outdated

holmanb mentioned this pull request Oct 27, 2021

Fix unhandled apt_configure case (SC-526) #1065

Merged

3 tasks

update docstring for add_apt_sources

60679b1

Update comment about implementing apt-key, revert accidental gpg fing…

3e03f92

…erprint change.

TheRealFalcon reviewed Oct 28, 2021

View reviewed changes

holmanb force-pushed the holmanb/remove-apt-key branch 3 times, most recently from 0a076e2 to d46fc9c Compare October 29, 2021 16:42

TheRealFalcon reviewed Oct 29, 2021

View reviewed changes

holmanb force-pushed the holmanb/remove-apt-key branch from d46fc9c to d37499a Compare October 29, 2021 16:55

holmanb force-pushed the holmanb/remove-apt-key branch from d37499a to 6ac2fcb Compare October 29, 2021 17:16

TheRealFalcon approved these changes Oct 29, 2021

View reviewed changes

TheRealFalcon merged commit 2858198 into canonical:main Oct 29, 2021

This was referenced May 11, 2023

cloud-init shouldn't use apt-key #3405

Closed

Path of open-vm-tools libdeployPkgPlugin.so is now multi-arch compliant breaking cloud-init #3909

Closed

hotplug causing cloud-init to spike CPU usage #3912

Closed

Release 21.4 #3921

Closed

		'3A3EF34DFDEDB3B7F3FDF603F83F77129A5EBD85'


		class TestAptKey(helpers.FilesystemMockingTestCase):

Conversation

holmanb commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Steps

Checklist:

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

holmanb commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

holmanb commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealFalcon commented Oct 28, 2021

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

holmanb commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

julian-klode commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

holmanb commented Oct 14, 2021 •

edited

Loading

holmanb commented Oct 19, 2021 •

edited

Loading

holmanb Oct 21, 2021 •

edited

Loading

holmanb commented Oct 27, 2021 •

edited

Loading

holmanb commented Oct 28, 2021 •

edited

Loading

julian-klode commented Nov 3, 2021 •

edited

Loading