overhaul cc_ca_certs functionality

[dermotbradley]

Proposed Commit Message

summary: overhaul cc_ca_certs functionality

Change "ca-certs" references to "ca_certs".

New certificates are written to individual files, with an incrementing
number as part of their filename, rather than all being placed in a
single file. This resolves issues caused when certificate files
containing more than a single certificate are placed in /etc/ssl/certs
(by utilities such as "update-ca-certificates" run by ca_certs).

Alpine / Debian / Ubuntu:

The current behaviour, whilst it works, is incorrect with regard to
the design of the underlying OS utilities for managing certificates.
For "remove_defaults" the system-installed certificate files should
not be actually deleted (otherwise it becomes problematic
if someone wishes to later re-enable one or more of them), rather
they should be deactivated and these OSes already provide the
means to do so - this MR modifies the certificate entries in the
/etc/ca-certificates.conf file by prefixing them with "!" - when the
update-ca-certificate utility is then run it will *not* place such
delimited certificates into either the /etc/ssl/certs/ directory
(via symlinks) nor add them to the (re)generated certificates
bundle file.

Additionally it is incorrect for added certificates to be placed in
the /usr/share/ca-certificates directory - this location is intended
for standard/"official" certificates, the
/usr/local/share/ca-certificates directory is intended for "local"
or "site-specific" certificates and so this PR adds them there
instead - for certs in /usr/local/share/ca-certificates the
update-ca-certificates utility will automatically use them,
there is *no* need to add their filenames to the
/etc/ca-certificates.conf file.

LP: #1931174

Additional Context

Test Steps

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

[dermotbradley]

The testcases fail because FreeBSD does a subp for "ifconfig" whenever get_cloud() is called. I tried to handle this in _mock_init as suggested by @igalic but it did not work.

The FreeBSD functionality was developed by reading documentation online - @igalic could you test this for real on FreeBSD?

Regarding RHEL/Fedora, I have been unable to determine the correct method to disable/delete system certs. I thought I could use trust list to get IDs for each cert and then for each run trust anchor remove "<id>" but it turns out all the standard certs are marked as "read only" inside the cert store and so that command will not remove them. The old behaviour of deleting files from /etc/pki/ca-trust/ and /usr/share/pki/ca-trust-source/ is wrong, especially as, for example, there are JVM and UEFI related cert stuff in /etc/pki/ca-trust/extracted/edk2 and /etc/pki/ca-trust/extracted/java

[igalic]

first look. i haven't tested it yet.
it would be cool, if someone knowledgeable could help us out the FreeBSD mocks into one place, instead of every place

[holmanb]

@esposem Requesting a review for the RHEL-specific bits, if you get a chance.

[holmanb]

First pass, still grokking.

I thought I could use trust list to get IDs for each cert and then for each run trust anchor remove "" but it turns out all the standard certs are marked as "read only" inside the cert store and so that command will not remove them.

I poked at Fedora a little myself. I think this is a bug, since behavior doesn't match the docs.

I've filed here:
https://bugzilla.redhat.com/show_bug.cgi?id=2163554

[esposem]

Thanks for pinging me @holmanb!
I had minor comments, but I would like to have this rhel/fedora behavior fixed and correctly implemented in this PR before merging it. Or if this is really urgent, we will for sure need a new PR where we enable this feature for RHEL too.
I would wait for comments in the fedora BZ, and in the meanwhile I will look for someone at RH that can fix/backport the eventual fix for us too.

[holmanb]

Thanks for pinging me @holmanb!

No problem @esposem!

I would wait for comments in the fedora BZ, and in the meanwhile I will look for someone at RH that can fix/backport the eventual fix for us too.

Based on the comment in the BZ, it looks like they aren't going to try to support deleting system-certs. Granted, I didn't bother explaining the use case in the BZ, but it would be nice if RHEL/Fedora had more dynamic/configurable CA management so that we could have a nicer approach than "delete them", since we're concerned that deleting them all might not always be the best thing for this feature. See the conversation between minimal, meena and I in IRC for some of the concerns that have been raised.

[igalic]

okay, first test on FreeBSD yielded nothing, because we're not enabling the module for FreeBSD in cloud.cfg.tmpl

[dermotbradley]

okay, first test on FreeBSD yielded nothing, because we're not enabling the module for FreeBSD in cloud.cfg.tmpl

Fixed now.

[igalic]

works pretty well, aside from the idempotency, if the module runs more than once on the same system.

[igalic]

dunno what to do about this, but otherwise this looks okay.

[dermotbradley]

I have removed FreeBSD and Fedora/RHEL related changes from this PR.

[holmanb]

Tested on ubuntu 22.04 with:

root@jammy-cloud1:~# cloud-init query userdata
#cloud-config
ca_certs:
  remove_defaults: true

Https request to google fails, as expected.

root@jammy-cloud1:~# wget https://www.google.com
--2023-02-06 19:25:19--  https://www.google.com/
Resolving www.google.com (www.google.com)... 142.250.191.132, 2607:f8b0:4009:807::2004
Connecting to www.google.com (www.google.com)|142.250.191.132|:443... connected.
ERROR: cannot verify www.google.com's certificate, issued by ‘CN=GTS CA 1C3,O=Google Trust Services LLC,C=US’:
  Unable to locally verify the issuer's authority.
To connect to www.google.com insecurely, use `--no-check-certificate'.

all certs are disabled after run

root@jammy-cloud1:~# grep -v -e '^#' -e '^!' /etc/ca-certificates.conf | wc -l
0

Logs look clean:
ca.log

One difference from the old implementation in the logs is that the old implementation spammed per-cert that was disabled. Someone used to that might be surprised not to see that, but I'd rather not do that anymore anyways - it was a bit excessive and the release notes should tell users that something changed should they want to understand what changed.

I left a couple of minor change requests inline, but the rest looks good to me.

Given the security aspect of this change I'd prefer to get a review from @blackboxsw or @aciba90 before merging.

[esposem]

Fedora docs were updated: https://docs.fedoraproject.org/en-US/quick-docs/using-shared-system-certificates/#proc_adding-new-certificates
When adding a package with trust anchor --store, you can remove it with trust anchor --remove. If you add it manually by copying it in the folder and calling update-ca-trust, then you can't remove it with trust.

[mdeslaur]

ACK from the Ubuntu security team on switching to this approach.

[blackboxsw]

Thanks for this PR! Looks really good. A couple of nits, minor unittest addition and integration test fixes. Otherwise +1.

1. Let's get the example docs fixed to document the deselect/disable instead of just remove.
2. might be nice to use enumerate for the certificate file naming
3. integration test fixes for the new cert file names/links

Combined diff suggestion, take what you will. throw out the rest :)

diff --git a/cloudinit/config/cc_ca_certs.py b/cloudinit/config/cc_ca_certs.py
index 2cd49eede..578dd4fbb 100644
--- a/cloudinit/config/cc_ca_certs.py
+++ b/cloudinit/config/cc_ca_certs.py
@@ -20,7 +20,7 @@ LOG = logging.getLogger(__name__)
 DEFAULT_CONFIG = {
     "ca_cert_path": None,
     "ca_cert_local_path": "/usr/local/share/ca-certificates/",
-    "ca_cert_filename": "cloud-init-ca-cert-%(number)s.crt",
+    "ca_cert_filename": "cloud-init-ca-cert-{cert_index}.crt",
     "ca_cert_config": "/etc/ca-certificates.conf",
     "ca_cert_update_cmd": ["update-ca-certificates"],
 }
@@ -28,7 +28,7 @@ DISTRO_OVERRIDES = {
     "rhel": {
         "ca_cert_path": "/etc/pki/ca-trust/",
         "ca_cert_local_path": "/usr/share/pki/ca-trust-source/anchors/",
-        "ca_cert_filename": "cloud-init-ca-cert-%(number)s.crt",
+        "ca_cert_filename": "cloud-init-ca-cert-{cert_index}.crt",
         "ca_cert_config": None,
         "ca_cert_update_cmd": ["update-ca-trust"],
     },
@@ -111,15 +111,12 @@ def add_ca_certs(distro_cfg, certs):
     if not certs:
         return
     # Write each certificate to a separate file.
-    cert_number = 0
-    for c in certs:
+    for cert_index, c in enumerate(certs, 1):
         # First ensure they are strings...
         cert_file_contents = str(c)
-        cert_number += 1
-
-        cert_file_name = distro_cfg["ca_cert_full_path"] % {
-            "number": str(cert_number)
-        }
+        cert_file_name = distro_cfg["ca_cert_full_path"].format(
+            cert_index=cert_index
+        )
         util.write_file(cert_file_name, cert_file_contents, mode=0o644)
 
 
@@ -153,15 +150,27 @@ def disable_system_ca_certs(distro_cfg):
     """
     if distro_cfg["ca_cert_config"] is None:
         return
+    header_comment = (
+        "# Modified by cloud-init to deselect certs due to user-data"
+    )
+    added_header = False
     if os.stat(distro_cfg["ca_cert_config"]).st_size != 0:
         orig = util.load_file(distro_cfg["ca_cert_config"])
-        out = ""
+        out_lines = []
         for line in orig.splitlines():
-            if line.startswith("#") or line.startswith("!") or line == "":
-                out += line + "\n"
+            if line == header_comment:
+                added_header = True
+                out_lines.append(line)
+            elif line == "" or line[0] in ("#", "!"):
+                out_lines.append(line)
             else:
-                out += "!" + line + "\n"
-    util.write_file(distro_cfg["ca_cert_config"], out, omode="wb")
+                if not added_header:
+                    out_lines.append(header_comment)
+                    added_header = True
+                out_lines.append("!" + line)
+    util.write_file(
+        distro_cfg["ca_cert_config"], "\n".join(out_lines) + "\n", omode="wb"
+    )
 
 
 def remove_default_ca_certs(distro_cfg):
@@ -210,17 +219,16 @@ def handle(
     ca_cert_cfg = cfg.get("ca_certs", cfg.get("ca-certs"))
     distro_cfg = _distro_ca_certs_configs(cloud.distro.name)
 
-    # If there is a remove_defaults option set to true, remove the system
+    # If there is a remove_defaults option set to true, disable the system
     # default trusted CA certs first.
     if "remove-defaults" in ca_cert_cfg:
         LOG.warning(
             "DEPRECATION: key 'ca-certs.remove-defaults' is now deprecated."
             " Use 'ca_certs.remove_defaults' instead."
         )
-        if ca_cert_cfg.get("remove-defaults", False):
-            LOG.debug("Disabling/removing default certificates")
-            disable_default_ca_certs(cloud.distro.name, distro_cfg)
-    elif ca_cert_cfg.get("remove_defaults", False):
+    if ca_cert_cfg.get(
+        "remove_defaults", ca_cert_cfg.get("remove-defaults", False)
+    ):
         LOG.debug("Disabling/removing default certificates")
         disable_default_ca_certs(cloud.distro.name, distro_cfg)
 
diff --git a/doc/examples/cloud-config-ca-certs.txt b/doc/examples/cloud-config-ca-certs.txt
index 9f7beb054..3c1c47fb6 100644
--- a/doc/examples/cloud-config-ca-certs.txt
+++ b/doc/examples/cloud-config-ca-certs.txt
@@ -8,11 +8,12 @@
 # It should be passed as user-data when starting the instance.
 
 ca_certs:
-  # If present and set to True, the 'remove_defaults' parameter will remove
-  # all the default trusted CA certificates that are normally shipped with
-  # Ubuntu.
-  # This is mainly for paranoid admins - most users will not need this
-  # functionality.
+  # If present and set to True, the 'remove_defaults' parameter will either
+  # disable all the trusted CA certifications  normally shipped with
+  # Alpine, Debian or Ubuntu. On RedHat, this action will delete those
+  # certificates.
+  # This is mainly for very security-sensitive use cases - most users will not
+  # need this functionality.
   remove_defaults: true
 
   # If present, the 'trusted' parameter should contain a certificate (or list
diff --git a/tests/integration_tests/modules/test_ca_certs.py b/tests/integration_tests/modules/test_ca_certs.py
index 8d18fb765..2baedda9c 100644
--- a/tests/integration_tests/modules/test_ca_certs.py
+++ b/tests/integration_tests/modules/test_ca_certs.py
@@ -76,10 +76,10 @@ class TestCaCerts:
                 unlinked_files.append(filename)
 
         assert ["ca-certificates.crt"] == unlinked_files
-        assert "cloud-init-ca-certs.pem" == links["a535c1f3.0"]
+        assert "cloud-init-ca-cert-1.pem" == links["a535c1f3.0"]
         assert (
-            "/usr/share/ca-certificates/cloud-init-ca-certs.crt"
-            == links["cloud-init-ca-certs.pem"]
+            "/usr/local/share/ca-certificates/cloud-init-ca-cert-1.crt"
+            == links["cloud-init-ca-cert-1.pem"]
         )
 
     def test_cert_installed(self, class_client: IntegrationInstance):
diff --git a/tests/unittests/config/test_cc_ca_certs.py b/tests/unittests/config/test_cc_ca_certs.py
index f17f549cd..3c83bf2a7 100644
--- a/tests/unittests/config/test_cc_ca_certs.py
+++ b/tests/unittests/config/test_cc_ca_certs.py
@@ -249,7 +249,7 @@ class TestAddCaCerts(TestCase):
                 mock_write.assert_has_calls(
                     [
                         mock.call(
-                            conf["ca_cert_full_path"] % {"number": "1"},
+                            conf["ca_cert_full_path"].format(cert_index=1),
                             cert,
                             mode=0o644,
                         )
@@ -277,12 +277,12 @@ class TestAddCaCerts(TestCase):
                 mock_write.assert_has_calls(
                     [
                         mock.call(
-                            conf["ca_cert_full_path"] % {"number": "1"},
+                            conf["ca_cert_full_path"].format(cert_index=1),
                             expected_cert_1_file,
                             mode=0o644,
                         ),
                         mock.call(
-                            conf["ca_cert_full_path"] % {"number": "2"},
+                            conf["ca_cert_full_path"].format(cert_index=2),
                             expected_cert_2_file,
                             mode=0o644,
                         ),
@@ -314,7 +314,10 @@ class TestRemoveDefaultCaCerts(TestCase):
 
     def test_commands(self):
         ca_certs_content = "# line1\nline2\nline3\n"
-        expected = "# line1\n!line2\n!line3\n"
+        expected = (
+            "# line1\n# Modified by cloud-init to deselect certs due to"
+            " user-data\n!line2\n!line3\n"
+        )
 
         for distro_name in cc_ca_certs.distros:
             conf = cc_ca_certs._distro_ca_certs_configs(distro_name)
@@ -344,6 +347,7 @@ class TestRemoveDefaultCaCerts(TestCase):
                             mock.call(conf["ca_cert_local_path"]),
                         ]
                     )
+                    self.assertEqual([], mock_subp.call_args_list)
                 elif distro_name in ["alpine", "debian", "ubuntu"]:
                     mock_load.assert_called_once_with(conf["ca_cert_config"])
                     mock_write.assert_called_once_with(

[igalic]

after the FreeBSD changes were dropped, I hadn't invested much more time in reviewing this PR, but i do really like @blackboxsw' suggestions here. they're feel a lot easier on the pythonic eye

[dermotbradley]

@blackboxsw I also noticed a subtle change had been introduced (compared to original behaviour) after rolling back the RHEL changes - I've corrected the values of ca_cert_local_path and ca_cert_filename for RHEL so that the deletion occurs the level above the "anchors" directory.

[holmanb]

Thanks for this @dermotbradley. LGTM.

[blackboxsw]

Thank you for the updates and effort here minimal. +1 let's merge this and get it into 23.1