Handle file permission errors in commands by aciba90 · Pull Request #1440 · canonical/cloud-init

aciba90 · 2022-05-09T07:01:35Z

Proposed Commit Message

Redact files with permission errors in commands.

Add tests covering this behavior for query, status and render
cmds.

Migrate `test_render.py` and `test_status.py` to Pytest.

LP: #1953430
SC-658

Additional Context

Test Steps

$ CONTAINER_NAME=some-ubuntu
$ lxc launch ubuntu-daily:jammy $CONTAINER_NAME
$ lxc exec $CONTAINER_NAME -- cloud-init status --wait
$ lxc exec $CONTAINER_NAME -- \
    chmod 400 '/etc/cloud/cloud.cfg.d/90_dpkg.cfg'

# Case 1
$ lxc exec $CONTAINER_NAME --user 1000 -- cloud-init query --all
<config with redacted parts>

# Case 2
$ lxc exec $CONTAINER_NAME --user 1000 -- cloud-init status --long
status: done
time: Tue, 10 May 2022 07:25:25 +0000
detail:
DataSourceLXD

Case 1 did exist with 1. Shows config with redacted parts.
Case 2 did raise an exception. Shows correct output. Log messages do not appear because of how the log is configured atm.

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

blackboxsw

Thanks for this PR @aciba90 and for both pytest migrations and type hints.

While the solution you have cleans up the traceback, I think we might want to make the behavior of cloud-init status and cloud-init query still work for non-root users. Raising the exception prevents any non-root user from querying cloud-init status or cloud-init query until a root admin removes or changes perms on the offending files. If we can, let's instead emit REDACTED warnings for any configuration file that a non-root user doesn't have permission to that aligns with the messaging we currently have for cloud-init query vendordata for non-root. Given that cloud-init status and cloud-init query are really only trying to source Paths.run_dir the likelihood that a root-read-only file in /etc/cloud/cloud.cfg.d is overriding system_info: paths: is very slim to have an impact on the functionality of those tools.

As such, I'm thinking we can proceed with something more like the following:

diff --git a/cloudinit/util.py b/cloudinit/util.py
index 9fb0c6616..90c6c7a47 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -32,7 +32,7 @@ import subprocess
 import sys
 import time
 from base64 import b64decode, b64encode
-from errno import ENOENT
+from errno import EACCES, ENOENT
 from functools import lru_cache
 from typing import List
 from urllib import parse
@@ -1010,8 +1010,13 @@ def read_conf_d(confd):
     # Load them all so that they can be merged
     cfgs = []
     for fn in confs:
-        cfgs.append(read_conf(os.path.join(confd, fn)))
-
+        try:
+            cfgs.append(read_conf(os.path.join(confd, fn)))
+        except OSError as e:
+            if e.errno == EACCES:
+                LOG.warning(
+                    f"REDACTED config part {confd}/{fn} for non-root user"
+                )
     return mergemanydict(cfgs)

Additionally something I'd like to see in this PR is that if we get an error in cloud-init query --all due to permissions issues or some other unexpected error, we get no message printed to console, just and exit 1. I'd like us to handle exceptions and emit an appropriate error message in that event.

blackboxsw · 2022-05-09T19:44:16Z

    init = Init(ds_deps=[])
-    init.read_cfg()
+    try:
+        init.read_cfg()


I don't think we want to raise an error in this case for non-root users. If we raise an error on any supplemental configuration files from /etc/cloud/cloud.cfg.d/* then non-root people/scripts running cloud-init status or cloud-init query will be unable to use that utility without root perms.

I believe we should probably align with the behavior we have in cloud-init query for non-root users. Any files to which you don't have permission get redacted with a comment/log or REDACTED label <redacted for non-root user> See cloud-init query vendordata or cloud-init query userdata when non-root user.

Given that cloud-init query and cloud-init status are read-only utilities for introspection of the system state and instance-data, I think we want to get as far as we can to allow consumers of those utilities without raising an error if possible. We can emit warnings about any config that is redacted to alert folks which have about configuration files which are ignored/redacted for non-root user.

Problematic files redacted.

aciba90 · 2022-05-11T12:55:37Z

Changed to redact configurations coming from files with permission errors.
Extra tests added to check that the sys-output is empty and that the log messages are correctly emitted.

Thank you for the comments and review. It is ready to be reviewed @blackboxsw

blackboxsw

Thanks @aciba90 a couple of notes on this behavior I think we may be able to drop a couple of the exception handling cases since you are coping with EACCES errors within read_conf_d.

Please let me know if I've misunderstood some elements.

blackboxsw · 2022-05-17T02:46:59Z

+            if e.errno == errno.EACCES:
+                pass  # Already logged in `config_loaders`.
+            else:
+                raise


Just a bit less code. Take if you feel like it or ignore if you don't.

Suggested change

if e.errno == errno.EACCES:

pass # Already logged in `config_loaders`.

else:

raise

if e.errno != errno.EACCES: # Already logged in config_loaders

raise

blackboxsw · 2022-05-17T04:12:00Z

-    )
+    config_loaders = [
+        # builtin config, hardcoded in settings.py.
+        util.get_builtin_cfg,


While I appreciate the sentiment in the event we get permissions errors, the following callables will not generate EACCES errors so it might be over-engineering to cope with scenarios which won't raise EACCESS:

get_builtin_cfg (we are sourcing a static variable in the python code)

read_runtime_config pulls from /run/cloud-init which we set at world-readable

read_conf_from_cmdline pulls from /proc/cmdline which is also world-readable

The only function/callable that could generate that EACCES is read_conf_with_confd, so I don't think we need to add the layer of test/except handling here in a for loop if we handle this warning down in read_conf_with_confd

After previous modifications, this is definitely not needed. Removed.

blackboxsw · 2022-05-17T04:13:37Z

+    except OSError as e:
+        if e.errno == EACCES:
+            LOG.warning("REDACTED config part %s for non-root user", cfgfile)
+        raise


I don't think we want a raise here, we want to proceed to other "conf_d" files if available.

Reworked to proceed to other "conf_d files" if available.

blackboxsw · 2022-05-17T04:16:45Z

+        try:
+            paths = read_cfg_paths()
+        except (IOError, OSError):
+            return 1


Since init.read_cfg() no longer raises an OSError/IOError (and instead logs warnings) I don't think we need this try/except here anymore right?

blackboxsw · 2022-05-17T04:17:40Z



 def read_runtime_config():
-    return util.read_conf(RUN_CLOUD_CONFIG)


Cloud-init owns and creates this file as world-readable, so I don't believe we need the try/except handling around this.

Removed, thanks.

- Unify the way of reading cfg paths in status.py by using `devel.read_cfg_paths`. - Add error handling for `devel.read_cfg_paths`. It outputs an error and sys-exists with 1. - Add tests covering this behavior for query, status and render cmds. - Migrate `test_render.py` to Pytest.

- Remove error handling from stages.read_runtime_config as /run/cloud-init/cloud.cfg is controlled by us. - Rework read_conf_with_confd to work even if cfgfile is not accessible by reading the confd directory only. - Remove error handling from stages.fetch_base_config as none of the functions raise. - Same in render.handle_args. - Adapt unit tests.

Do not except unhandled exceptions.

aciba90 · 2022-05-17T09:51:33Z

Thanks for your suggestions and insights @blackboxsw . Now the code is much more simple. Ready to be reviewed.

blackboxsw

Good work on this PR and addressing all facets of the failure. Validated on server live install environments. LGTM

aciba90 marked this pull request as draft May 9, 2022 07:17

aciba90 marked this pull request as ready for review May 9, 2022 10:10

blackboxsw requested changes May 9, 2022

View reviewed changes

aciba90 force-pushed the SC-658/query_userdata_root_perm branch from 4bc4603 to 1604081 Compare May 11, 2022 12:28

aciba90 marked this pull request as draft May 11, 2022 12:29

aciba90 marked this pull request as ready for review May 11, 2022 12:55

blackboxsw self-assigned this May 16, 2022

blackboxsw requested changes May 17, 2022

View reviewed changes

aciba90 added 6 commits May 17, 2022 08:55

Migrate test_status.py to pytest.

c7a3220

Fix test.

1700565

Redact confs coming from permission errors.

c647e96

Fix function docstr.

77c3477

aciba90 force-pushed the SC-658/query_userdata_root_perm branch from 99d1c11 to f1ff067 Compare May 17, 2022 08:52

aciba90 added 2 commits May 17, 2022 11:02

Remove error handling in status.handle_status_args

338ad91

Do not except unhandled exceptions.

Remove unneeded test.

e0f9b78

aciba90 force-pushed the SC-658/query_userdata_root_perm branch from ddf10bf to e0f9b78 Compare May 17, 2022 09:37

blackboxsw approved these changes May 17, 2022

View reviewed changes

blackboxsw merged commit 3e554d1 into canonical:main May 17, 2022

aciba90 deleted the SC-658/query_userdata_root_perm branch May 23, 2022 09:12

This was referenced May 12, 2023

Top-level 'network' key results in error using v2 config #3815

Closed

Release 22.2 #3976

Closed



		def read_runtime_config():
		return util.read_conf(RUN_CLOUD_CONFIG)

Conversation

aciba90 commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

blackboxsw left a comment

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!

aciba90 commented May 11, 2022

Uh oh!

blackboxsw 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!

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!

Choose a reason for hiding this comment

Uh oh!

aciba90 commented May 17, 2022

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aciba90 commented May 9, 2022 •

edited

Loading