From e678f7911d6043244e197df996f6b18c7ebf1064 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Tue, 14 Mar 2023 11:47:39 +0800 Subject: [PATCH 01/12] Fix permission of the private key --- cloudinit/config/cc_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py index 5712977606b..f551282fa18 100644 --- a/cloudinit/config/cc_ssh.py +++ b/cloudinit/config/cc_ssh.py @@ -280,7 +280,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: if gid != -1: # perform same "sanitize permissions" as sshd-keygen os.chown(keyfile, -1, gid) - os.chmod(keyfile, 0o640) + os.chmod(keyfile, 0o600) os.chmod(keyfile + ".pub", 0o644) except subp.ProcessExecutionError as e: err = util.decode_binary(e.stderr).lower() From 9d542c69e2c552032ecbfb0e6945f9f8246f6c34 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 17 Mar 2023 08:48:11 +0800 Subject: [PATCH 02/12] get OpenSSH Major Version --- cloudinit/ssh_util.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index eb5c9f64b1d..d08eaac0d9c 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -8,6 +8,7 @@ import os import pwd +import subprocess from typing import List, Sequence, Tuple from cloudinit import log as logging @@ -641,5 +642,9 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG): preserve_mode=True, ) +def getOpensshMajorVersion(): + result = subprocess.getstatusoutput("ssh -V") + version = result[1][8] + return version # vi: ts=4 expandtab From 2b08f35e0e169da088ca0a79a33a76409a4e5848 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 17 Mar 2023 08:49:54 +0800 Subject: [PATCH 03/12] update function name --- cloudinit/ssh_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index d08eaac0d9c..aaf6d9da761 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -642,7 +642,7 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG): preserve_mode=True, ) -def getOpensshMajorVersion(): +def get_openssh_major_version(): result = subprocess.getstatusoutput("ssh -V") version = result[1][8] return version From 3f7042ec988b380f3a232bea1eb7b72ca46e189e Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 17 Mar 2023 08:51:45 +0800 Subject: [PATCH 04/12] change permission of the private key --- cloudinit/config/cc_ssh.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py index f551282fa18..a34800d15fd 100644 --- a/cloudinit/config/cc_ssh.py +++ b/cloudinit/config/cc_ssh.py @@ -277,10 +277,13 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: sys.stdout.write(util.decode_binary(out)) gid = util.get_group_id("ssh_keys") + permissions_private = 0o600; + if int(ssh_util.get_openssh_major_version()) < 9: + permissions_private = 0o640; if gid != -1: # perform same "sanitize permissions" as sshd-keygen os.chown(keyfile, -1, gid) - os.chmod(keyfile, 0o600) + os.chmod(keyfile, permissions_private) os.chmod(keyfile + ".pub", 0o644) except subp.ProcessExecutionError as e: err = util.decode_binary(e.stderr).lower() From e8b90c25e93cac3f4cd514902d036400dd5bcdca Mon Sep 17 00:00:00 2001 From: TheRealFalcon Date: Sun, 19 Mar 2023 19:36:42 +0800 Subject: [PATCH 05/12] Optimize to obtain the openssh version. Here, "sshd - V" is used to obtain the version. Idea from TheRealFalcon --- cloudinit/config/cc_ssh.py | 7 +++--- cloudinit/ssh_util.py | 49 +++++++++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py index a34800d15fd..660bc142da6 100644 --- a/cloudinit/config/cc_ssh.py +++ b/cloudinit/config/cc_ssh.py @@ -277,9 +277,10 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: sys.stdout.write(util.decode_binary(out)) gid = util.get_group_id("ssh_keys") - permissions_private = 0o600; - if int(ssh_util.get_openssh_major_version()) < 9: - permissions_private = 0o640; + 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) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index aaf6d9da761..d7639d5b777 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -9,10 +9,11 @@ import os import pwd import subprocess -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,9 +643,45 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG): preserve_mode=True, ) -def get_openssh_major_version(): - result = subprocess.getstatusoutput("ssh -V") - version = result[1][8] - return version +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) + # vi: ts=4 expandtab From 4586d6141e267ac8cb0aa119a9188030e7132b8b Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Sun, 19 Mar 2023 20:01:43 +0800 Subject: [PATCH 06/12] fix pylint --- cloudinit/ssh_util.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index d7639d5b777..eb74874960c 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -8,7 +8,6 @@ import os import pwd -import subprocess from contextlib import suppress from typing import List, Optional, Sequence, Tuple @@ -643,6 +642,7 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG): preserve_mode=True, ) + def get_opensshd_version() -> Optional[str]: """Get the full version of the OpenSSH sshd daemon on the system. @@ -671,7 +671,7 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: """ full_version = get_opensshd_version() if full_version is None: - return + return None elif "p" in full_version: upstream_version = full_version[: full_version.find("p")] elif " " in full_version: From 5e83ea298a1f8442917132eb34ca5b274fd17fdc Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Sun, 19 Mar 2023 20:12:47 +0800 Subject: [PATCH 07/12] fix mypy: commands failed --- cloudinit/ssh_util.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index eb74874960c..bede9dacbd5 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -660,6 +660,7 @@ def get_opensshd_version() -> Optional[str]: for line in err.split("\n"): if line.startswith(prefix): return line[len(prefix) : line.find(",")] + return None def get_opensshd_upstream_version() -> Optional[util.Version]: @@ -670,8 +671,9 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: `1.2` """ full_version = get_opensshd_version() + upstream_version = None if full_version is None: - return None + return upstream_version elif "p" in full_version: upstream_version = full_version[: full_version.find("p")] elif " " in full_version: @@ -679,9 +681,10 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: else: upstream_version = full_version try: - return util.Version.from_str(upstream_version) + upstream_version = util.Version.from_str(upstream_version) except (ValueError, TypeError): LOG.warning("Could not parse sshd version: %s", upstream_version) - + finally: + return upstream_version # vi: ts=4 expandtab From 36098292e6c493b20d62f9549edf9593b14d6ba3 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 31 Mar 2023 21:45:02 +0800 Subject: [PATCH 08/12] add default version of openssh --- cloudinit/ssh_util.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index bede9dacbd5..2d598ae9647 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -671,7 +671,8 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: `1.2` """ full_version = get_opensshd_version() - upstream_version = None + # The default version of openssh is 9.0 + upstream_version = util.Version(9, 0) if full_version is None: return upstream_version elif "p" in full_version: From 1f61ee3229abd99b8f390c68798433d2f8c38c0c Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 31 Mar 2023 22:04:11 +0800 Subject: [PATCH 09/12] fix return statement in finally block may swallow exception --- cloudinit/ssh_util.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 2d598ae9647..af9bf5aef91 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -671,7 +671,7 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: `1.2` """ full_version = get_opensshd_version() - # The default version of openssh is 9.0 + # The default version of openssh is not less than 9.0 upstream_version = util.Version(9, 0) if full_version is None: return upstream_version @@ -683,9 +683,9 @@ def get_opensshd_upstream_version() -> Optional[util.Version]: upstream_version = full_version try: upstream_version = util.Version.from_str(upstream_version) + return upstream_version except (ValueError, TypeError): LOG.warning("Could not parse sshd version: %s", upstream_version) - finally: - return upstream_version + # vi: ts=4 expandtab From 8a9c7643e2fbba54bb466f9702bf19189eda4f18 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 31 Mar 2023 22:20:54 +0800 Subject: [PATCH 10/12] fix mypy --- cloudinit/ssh_util.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index af9bf5aef91..55910ec7db6 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -643,7 +643,7 @@ def append_ssh_config(lines: Sequence[Tuple[str, str]], fname=DEF_SSHD_CFG): ) -def get_opensshd_version() -> Optional[str]: +def get_opensshd_version(): """Get the full version of the OpenSSH sshd daemon on the system. On an ubuntu system, this would look something like: @@ -663,19 +663,19 @@ def get_opensshd_version() -> Optional[str]: return None -def get_opensshd_upstream_version() -> Optional[util.Version]: +def get_opensshd_upstream_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() # The default version of openssh is not less than 9.0 - upstream_version = util.Version(9, 0) + upstream_version = "9.0" + full_version = get_opensshd_version() if full_version is None: - return upstream_version - elif "p" in full_version: + return util.Version.from_str(upstream_version) + if "p" in full_version: upstream_version = full_version[: full_version.find("p")] elif " " in full_version: upstream_version = full_version[: full_version.find(" ")] From f57706460c99597d4954f5c34545eac1ffd3a8c6 Mon Sep 17 00:00:00 2001 From: sxt1001 Date: Fri, 31 Mar 2023 22:47:09 +0800 Subject: [PATCH 11/12] F401 'typing.Optional' imported but unused --- cloudinit/ssh_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py index 55910ec7db6..eee399f227a 100644 --- a/cloudinit/ssh_util.py +++ b/cloudinit/ssh_util.py @@ -9,7 +9,7 @@ import os import pwd from contextlib import suppress -from typing import List, Optional, Sequence, Tuple +from typing import List, Sequence, Tuple from cloudinit import log as logging from cloudinit import subp, util From f2c31cf017d3734cd2f1cdd6905db76160d737eb Mon Sep 17 00:00:00 2001 From: James Falcon Date: Sun, 2 Apr 2023 10:55:28 -0500 Subject: [PATCH 12/12] Only get sshd version when the relevant group exists. Also, add unit test --- cloudinit/config/cc_ssh.py | 10 +++--- tests/unittests/config/test_cc_ssh.py | 46 ++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py index 660bc142da6..7c9ae36b909 100644 --- a/cloudinit/config/cc_ssh.py +++ b/cloudinit/config/cc_ssh.py @@ -277,15 +277,15 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: 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 + permissions_private = 0o600 + ssh_version = ssh_util.get_opensshd_upstream_version() + if ssh_version and ssh_version < util.Version(9, 0): + permissions_private = 0o640 os.chown(keyfile, -1, gid) os.chmod(keyfile, permissions_private) - os.chmod(keyfile + ".pub", 0o644) + os.chmod(f"{keyfile}.pub", 0o644) except subp.ProcessExecutionError as e: err = util.decode_binary(e.stderr).lower() if e.exit_code == 1 and err.lower().startswith( diff --git a/tests/unittests/config/test_cc_ssh.py b/tests/unittests/config/test_cc_ssh.py index 3fa9fcf84b0..524fb81d419 100644 --- a/tests/unittests/config/test_cc_ssh.py +++ b/tests/unittests/config/test_cc_ssh.py @@ -7,7 +7,7 @@ import pytest -from cloudinit import ssh_util +from cloudinit import ssh_util, util from cloudinit.config import cc_ssh from cloudinit.config.schema import ( SchemaValidationError, @@ -284,6 +284,50 @@ def test_handle_publish_hostkeys( expected_calls == cloud.datasource.publish_host_keys.call_args_list ) + @pytest.mark.parametrize( + "ssh_keys_group_exists,sshd_version,expected_private_permissions", + [(False, 0, 0), (True, 8, 0o640), (True, 10, 0o600)], + ) + @mock.patch(MODPATH + "subp.subp", return_value=("", "")) + @mock.patch(MODPATH + "util.get_group_id", return_value=10) + @mock.patch(MODPATH + "ssh_util.get_opensshd_upstream_version") + @mock.patch(MODPATH + "os.path.exists", return_value=False) + @mock.patch(MODPATH + "os.chown") + @mock.patch(MODPATH + "os.chmod") + def test_ssh_hostkey_permissions( + self, + m_chmod, + m_chown, + m_exists, + m_sshd_version, + m_gid, + m_subp, + m_setup_keys, + ssh_keys_group_exists, + sshd_version, + expected_private_permissions, + ): + """Test our generated hostkeys use same perms as sshd-keygen. + + SSHD version < 9.0 should apply 640 permissions to the private key. + Otherwise, 600. + """ + m_gid.return_value = 10 if ssh_keys_group_exists else -1 + m_sshd_version.return_value = util.Version(sshd_version, 0) + key_path = cc_ssh.KEY_FILE_TPL % "rsa" + cloud = get_cloud(distro="ubuntu") + cc_ssh.handle("name", {"ssh_genkeytypes": ["rsa"]}, cloud, []) + if ssh_keys_group_exists: + m_chown.assert_called_once_with(key_path, -1, 10) + assert m_chmod.call_args_list == [ + mock.call(key_path, expected_private_permissions), + mock.call(f"{key_path}.pub", 0o644), + ] + else: + m_sshd_version.assert_not_called() + m_chown.assert_not_called() + m_chmod.assert_not_called() + @pytest.mark.parametrize("with_sshd_dconf", [False, True]) @mock.patch(MODPATH + "util.ensure_dir") @mock.patch(MODPATH + "ug_util.normalize_users_groups")