raise ValueError when "no_create_home" or "system" is set and ssh_authorized_keys is also provided

[jf]

Proposed Commit Message

summary: raise error when home should not be created AND ssh keys are provided

An invalid user config would be if no_create_home or system is set to
True, AND (ssh_authorized_keys or ssh_import_id or ssh_redirect_user)
is also provided. Right now, cloud-init proceeds to create the keys
(which would obviously create home for the user).

This change will cause an error to be raised instead for this invalid
config.

Additional Context

Test Steps

sample user data:

#cloud-config

system_info:
  default_user:
    name: jf

users:
  - default

  - name: nch
    no_create_home: true
    ssh_authorized_keys:
      - list
      - of
      - keys

  - name: system
    system: true
    ssh_authorized_keys:
      - list
      - of
      - keys

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 the PR @jf. I have a couple of requests for this PR. Please let me know what you think:

1. I think we need to be aware both of either ssh_authorized_keys and ssh_import_id as mutually exclusive to no_create_home and system because
   running ssh-import-id as a user without a home directory results in Permission denied errors because /home/ doens't exist
2. I think ssh_redirect_user key also falls into this camp of a config value that conflicts with no_create_home and system because it also tries to create a home_dir and add lines to an ~/.ssh/authorized_keys file
3. I think it's worth consolidating the checks for mutual exclusive keys so both ssh_redirect_user validation and no_create_home , system config settings present the same context sensitive error message telling about incompatible keys that are being used.
4. Can we git rebase upstream/main on this PR and either revert the "no_create_home/system" checks in cloudinit/config/cc_ssh_authkey_fingerprints.py now that we have an explicity failure here or add a LOG.debug("Skip printing ssh fingerprints for user %s because no home directory is created) to cc_ssh_authkey_fingerprints.py? I don't like that we are silently skipping inconsistent configuration. When we encounter misconfig, we should be load about that failure (as your current PR does :) ).
5. I'd like us to add a unittest for this if we can in tests/unittests/config/test_cc_users_groups.py to cover at least the raise of the ValueError due to providing (no_create_home|system) and (ssh_authorized_keys|ssh_import_id)

Here's a functional diff of what I was thinking to see what you think about whether this meets the intent of your PR

diff --git a/cloudinit/config/cc_users_groups.py b/cloudinit/config/cc_users_groups.py
index 92076b19d..29aa88f44 100644
--- a/cloudinit/config/cc_users_groups.py
+++ b/cloudinit/config/cc_users_groups.py
@@ -140,21 +140,28 @@ LOG = logging.getLogger(__name__)
 frequency = PER_INSTANCE
 
 
+# KEYS_NEED_HOME and KEYS_NO_HOME are mutually exclusive options
+KEYS_NEED_HOME = ("ssh_import_id", "ssh_authorized_keys", "ssh_redirect_user")
+KEYS_NO_HOME = ("no_create_home", "system")
+
+
 def handle(name, cfg, cloud, _log, _args):
     (users, groups) = ug_util.normalize_users_groups(cfg, cloud.distro)
     (default_user, _user_config) = ug_util.extract_default(users)
     cloud_keys = cloud.get_public_ssh_keys() or []
+    keys_force_no_homedir = ("no_create_home", "system")
     for (name, members) in groups.items():
         cloud.distro.create_group(name, members)
     for (user, config) in users.items():
         ssh_redirect_user = config.pop("ssh_redirect_user", False)
+        require_home = [key for key in KEYS_NEED_HOME if config.get(key)]
+        prevent_home = [key for key in KEYS_NO_HOME if config.get(key)]
+        if require_home and prevent_home:
+            raise ValueError(
+                f"Not creating user {user}. Key(s) {require_home.join(',')}"
+                f" cannot be provided with {prevent_home.join(',')}"
+            )
         if ssh_redirect_user:
-            if "ssh_authorized_keys" in config or "ssh_import_id" in config:
-                raise ValueError(
-                    "Not creating user %s. ssh_redirect_user cannot be"
-                    " provided with ssh_import_id or ssh_authorized_keys"
-                    % user
-                )
             if ssh_redirect_user not in (True, "default"):
                 raise ValueError(
                     "Not creating user %s. Invalid value of"
@@ -173,15 +180,6 @@ def handle(name, cfg, cloud, _log, _args):
             else:
                 config["ssh_redirect_user"] = default_user
                 config["cloud_public_ssh_keys"] = cloud_keys
-
-        if (
-            config.get("no_create_home") or config.get("system")
-        ) and config.get("ssh_authorized_keys"):
-            raise ValueError(
-                "Not creating user %s. ssh_authorized_keys cannot be"
-                " provided with no_create_home: true or system: true" % user
-            )
-
         cloud.distro.create_user(user, **config)

[blackboxsw]

I think we want to actually check both ssh_authorized_keys and ssh_import_id as they both will result in the same problem (where home dir gets created in order to actually store the ssh-import-id ). That said, since the ssh_redirect_user conditional above also checks this condition it probably makes sense for us to set a install_keys local variable for this compound condition so we can reuse it for both ssh_redirect_user and no_create_home/system checks.

[blackboxsw]

I just realized after testing point 4 above, we still need to avoid the errant creation of the homedir due to ssh_authkey_fingerprints... so if we can add a log message there about the skip of printing. I'm satisfied. Thanks again

[jf]

ok. I've rebased and then added commits to the PR. Please see if this is ok?

[jf]

@blackboxsw is this ok for you?

[blackboxsw]

I'm good with raising a ValueError here as it misconfigured users are likely unidentified until much later in a boot/deployment process which costs more time than an upfront error that is surfaced directly from cloud-init status and cloud-init's systemd services to show the system in degraded state. +1 on this approach to error early here and stop setting up other users. minor fix on the suggested patch I gave and behavior looks good.

there is a minor merge conflict too that I'll fix and this PR should be good to land.
Thank you @jf

[blackboxsw]

Fixup bogus code from me

Suggested change

	f"Not creating user {user}. Key(s) {need_home.join(',')}"
	f" cannot be provided with {no_home.join(',')}"
	)
	f"Not creating user {user}. Key(s) {', '.join(need_home)}"
	f" cannot be provided with {', '.join(no_home)}"
	)