Fix permission of the private key when openssh is not earlier than 9.0

[xiaoge1001]

Proposed Commit Message

With this change the private key is 0600
LP: #2011291

[xiaoge1001]

https://github.com/canonical/cloud-init/blob/main/cloudinit/config/cc_ssh.py#L212

the condition is met, the patch #1971 takes effect. However, if the else branch is entered, the permission on the generated private key file is 640.

[xiaoge1001]

Same PR found:
#1121

[xiaoge1001]

Other related PRs:
#1070

[xiaoge1001]

https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/sec-creating_ssh_ca_certificate_signing-keys#doc-wrapper

[xiaoge1001]

Restore upstream default host key permissions (rhbz#2141272)
https://src.fedoraproject.org/rpms/openssh/c/b615362fd0b4da657d624571441cb74983de6e3f?branch=rawhide

[TheRealFalcon]

@esposem , looks like sshd-keygen has changed its behavior. Does this look correct to you? Will openscap work with these permissions or do we need distro-specific permissions?

[xiachen-rh]

From that commit, we can see the sshd-keygen has changed its behavior on fedora, but we don't see the changes on RHEL.

[esposem]

Thanks @xiachen-rh for the answer and investigating into this.
@TheRealFalcon basically RHEL 9.3 has openssh-8.7p1-28.el9.x86_64, which means for RHEL my reasoning still holds: #1070 and https://github.com/canonical/cloud-init/pull/1971/files/e49852d44aba7e32e763244af2cb152b214b2306#r1084150227

# cat /usr/libexec/openssh/sshd-keygen

# create new keys
if ! $KEYGEN -q -t $KEYTYPE -f $KEY -C '' -N '' >&/dev/null; then
	exit 1
fi

# sanitize permissions
/usr/bin/chgrp ssh_keys $KEY
/usr/bin/chmod 640 $KEY
/usr/bin/chmod 644 $KEY.pub
if [[ -x /usr/sbin/restorecon ]]; then
	/usr/sbin/restorecon $KEY{,.pub}
fi

So for RHEL changing the permissions as proposed in this PR is still a NACK.

[xiaoge1001]

Can we handle different private key file permissions based on the version of openssh installed on the machine or different linux distributions?

[esposem]

yes, I would suggest based on openssh version which I think should reflect more what is its behaviour and is not distro-specific. We will eventually switch to the new openssh version too.

[TheRealFalcon]

The linked commit shows the change showing up in version 9.0p1. So are we saying that for openssh version < 9.0 use 640, otherwise use 600? @xiaoge1001 and @esposem , does that work for both of you?

[xiaoge1001]

Yes, it works well for me.

[TheRealFalcon]

@xiaoge1001 , I think we should get the version of sshd rather than ssh. I'm guessing almost all installs will be the same version, but they don't have to be. Cloud-init also has a subprocess wrapper that we use for shelling out that we should use. Also, since string parsing can be a bit fragile, I'd like to put some extra protections around it so that failures continue to use the default functionality and so that we don't mis-compare things.

How does something like this look instead?

diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py
index 1ec889f3b..30b2f851b 100644
--- a/cloudinit/config/cc_ssh.py
+++ b/cloudinit/config/cc_ssh.py
@@ -278,10 +278,14 @@ def handle(
                         sys.stdout.write(util.decode_binary(out))
 
                     gid = util.get_group_id("ssh_keys")
+                    permissions_private = 0o600
+                    ssh_version = ssh_util.get_opensshd_upstream_version()
+                    if ssh_version and ssh_version < util.Version(9, 0):
+                        permissions_private = 0o640
                     if gid != -1:
                         # perform same "sanitize permissions" as sshd-keygen
                         os.chown(keyfile, -1, gid)
-                        os.chmod(keyfile, 0o640)
+                        os.chmod(keyfile, permissions_private)
                         os.chmod(keyfile + ".pub", 0o644)
                 except subp.ProcessExecutionError as e:
                     err = util.decode_binary(e.stderr).lower()
diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py
index eb5c9f64b..1567e824e 100644
--- a/cloudinit/ssh_util.py
+++ b/cloudinit/ssh_util.py
@@ -8,10 +8,11 @@
 
 import os
 import pwd
-from typing import List, Sequence, Tuple
+from contextlib import suppress
+from typing import List, Optional, Sequence, Tuple
 
 from cloudinit import log as logging
-from cloudinit import util
+from cloudinit import subp, util
 
 LOG = logging.getLogger(__name__)
 
@@ -642,4 +643,42 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG):
     )
 
 
-# vi: ts=4 expandtab
+def get_opensshd_version() -> Optional[str]:
+    """Get the full version of the OpenSSH sshd daemon on the system.
+
+    On an ubuntu system, this would look something like:
+    1.2p1 Ubuntu-1ubuntu0.1
+
+    If we can't find `sshd` or parse the version number, return None.
+    """
+    # -V isn't actually a valid argument, but it will cause sshd to print
+    # out its version number to stderr.
+    err = ""
+    with suppress(subp.ProcessExecutionError):
+        _, err = subp.subp(["sshd", "-V"], rcs=[0, 1])
+    prefix = "OpenSSH_"
+    for line in err.split("\n"):
+        if line.startswith(prefix):
+            return line[len(prefix) : line.find(",")]
+
+
+def get_opensshd_upstream_version() -> Optional[util.Version]:
+    """Get the upstream version of the OpenSSH sshd dameon on the system.
+
+    This will NOT include the portable number, so if the Ubuntu version looks
+    like `1.2p1 Ubuntu-1ubuntu0.1`, then this function would return
+    `1.2`
+    """
+    full_version = get_opensshd_version()
+    if full_version is None:
+        return
+    elif "p" in full_version:
+        upstream_version = full_version[: full_version.find("p")]
+    elif " " in full_version:
+        upstream_version = full_version[: full_version.find(" ")]
+    else:
+        upstream_version = full_version
+    try:
+        return util.Version.from_str(upstream_version)
+    except (ValueError, TypeError):
+        LOG.warning("Could not parse sshd version: %s", upstream_version)

[xiaoge1001]

It is very great.

[TheRealFalcon]

There's some linting failures, but also, sorry, I liked your old test but I didn't provide one with my implementation. Can we have a similar unit test to the one you had before that checks the file write behavior for different versions?

[xiaoge1001]

Thank you for your reply. I can do this recently.

[TheRealFalcon]

@xiaoge1001 Thanks for the updates. I took the liberty of pushing a small change to only check version if we're using the proper group along with a unit test. I'll merge if all checks pass.