- Create the log file with 640 permissions

[rjschwei]

• Security scanners are often simple minded and complain on arbitrary
  settings such as file permissions. For /var/log/* having world read is
  one of these cases.

Proposed Commit Message

Appease some simple minded "security" scanners

summary: Change log file to 640 permissions

Security scanners are often simple minded and complain on arbitrary
settings such as file permissions. For /var/log/* having world read is
one of these cases.

Additional Context

Test Steps

Checklist:

• [ x] 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

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag mitechie to reopen it.)

[rjschwei]

@mitechie some action needed here

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag mitechie to reopen it.)

[OddBloke]

Hey @rjschwei, thanks for this submission! I think modifying the default permissions on this log file likely makes sense.

This code looks correct: we'll need some unit testing and, ideally, an integration test.

@smoser @raharper @blackboxsw @TheRealFalcon, any objections to adopting this as default behaviour (for new instances) going forward?

(From an Ubuntu perspective, I anticipate that we would retain the existing permissions in current releases, but adopt the new permissions going forward.)

[TheRealFalcon]

Given that nobody else has any objections here, I think we're ok to go ahead with this.

@rjschwei would you able to add the requisite testing? If you take a look at test_stages.py, the unit tests at the end of the file would be related.

[blackboxsw]

Thnks @rjschwei. This change is totally reasonable.

I'm not concerned about backporting and disabling this feature on stable releases given that:

• cloud-init preserves the file mode if it already exists
• the default user is in the adm group which still has read access to /var/log/cloud-init.log
• cloud-init.log is not something easily parsed, so I'm also not concerned about third-party apps trying to consume this logfile that will not retain read permission

That said; I've added the test (and fixed existing test_stages.py) to properly assert that the created file exists with the right perms.

Robert, if ok, I'd like us to merge this unit test fix and land your branch

commit d444bdaf18b43b92ba64b5a03acba6e6351c6555 (HEAD)
Author: Chad Smith <chad.smith@canonical.com>
Date:   Tue Jun 1 10:40:26 2021 -0600

    tests: validate mode 640 on cloud-init.log when absent. Fix init fixture

diff --git a/cloudinit/tests/test_stages.py b/cloudinit/tests/test_stages.py
index d2d1b37f..28551040 100644
--- a/cloudinit/tests/test_stages.py
+++ b/cloudinit/tests/test_stages.py
@@ -356,22 +356,20 @@ class TestInit_InitializeFilesystem:
         """A fixture which yields a stages.Init instance with paths and cfg set
 
         As it is replaced with a mock, consumers of this fixture can set
-        `init.cfg` if the default empty dict configuration is not appropriate.
+        `init._cfg` if the default empty dict configuration is not appropriate.
         """
-        with mock.patch(
-            "cloudinit.stages.Init.cfg", mock.PropertyMock(return_value={})
-        ):
-            with mock.patch("cloudinit.stages.util.ensure_dirs"):
-                init = stages.Init()
-                init._paths = paths
-                yield init
+        with mock.patch("cloudinit.stages.util.ensure_dirs"):
+            init = stages.Init()
+            init._cfg = {}
+            init._paths = paths
+            yield init
 
     @mock.patch("cloudinit.stages.util.ensure_file")
     def test_ensure_file_not_called_if_no_log_file_configured(
         self, m_ensure_file, init
     ):
         """If no log file is configured, we should not ensure its existence."""
-        init.cfg = {}
+        init._cfg = {}
 
         init._initialize_filesystem()
 
@@ -380,11 +378,13 @@ class TestInit_InitializeFilesystem:
     def test_log_files_existence_is_ensured_if_configured(self, init, tmpdir):
         """If a log file is configured, we should ensure its existence."""
         log_file = tmpdir.join("cloud-init.log")
-        init.cfg = {"def_log_file": str(log_file)}
+        init._cfg = {"def_log_file": str(log_file)}
 
         init._initialize_filesystem()
 
-        assert log_file.exists
+        assert log_file.exists()
+        # Assert we create it 0o640  by default if it doesn't already exist
+        assert 0o640 == stat.S_IMODE(log_file.stat().mode)
 
     def test_existing_file_permissions_are_not_modified(self, init, tmpdir):
         """If the log file already exists, we should not modify its permissions
@@ -397,7 +397,7 @@ class TestInit_InitializeFilesystem:
         log_file = tmpdir.join("cloud-init.log")
         log_file.ensure()
         log_file.chmod(mode)
-        init.cfg = {"def_log_file": str(log_file)}
+        init._cfg = {"def_log_file": str(log_file)}
 
         init._initialize_filesystem()
 

[rjschwei]

@blackboxsw thanks for jumping in and helping me with he necessary testing, much appreciated.

Proposed changes LGTM.