From daf0b5579965bfca231b57e28b9c6f2d48a320e1 Mon Sep 17 00:00:00 2001 From: Justin Zandbergen Date: Fri, 23 Feb 2024 10:50:22 +0100 Subject: [PATCH 1/5] fix: Older keys end with a newline, this breaks minion auth. --- salt/channel/server.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/salt/channel/server.py b/salt/channel/server.py index 4d7cd23fc29e..ba577c3dadb0 100644 --- a/salt/channel/server.py +++ b/salt/channel/server.py @@ -371,7 +371,15 @@ def _auth(self, load, sign_messages=False): elif os.path.isfile(pubfn): # The key has been accepted, check it with salt.utils.files.fopen(pubfn, "r") as pubfn_handle: - if salt.crypt.clean_key(pubfn_handle.read()) != load["pub"]: + keyFromDisk = pubfn_handle.read() + + # if the keyFromDisk has a final newline it is a oldstyle key + # if we clean it, it will not match. Only clean the key if it + # is a new style key. + if keyFromDisk[-1:] != "\n": + keyFromDisk = salt.crypt.clean(orgkey) + + if keyFromDisk != load["pub"]: log.error( "Authentication attempt from %s failed, the public " "keys did not match. This may be an attempt to compromise " From f69f4cd1ac75c2f13ebf2ad671cfc95966af3e59 Mon Sep 17 00:00:00 2001 From: Justin Zandbergen Date: Fri, 23 Feb 2024 11:04:31 +0100 Subject: [PATCH 2/5] fix: typo's --- salt/channel/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/channel/server.py b/salt/channel/server.py index ba577c3dadb0..68c3005d6858 100644 --- a/salt/channel/server.py +++ b/salt/channel/server.py @@ -377,7 +377,7 @@ def _auth(self, load, sign_messages=False): # if we clean it, it will not match. Only clean the key if it # is a new style key. if keyFromDisk[-1:] != "\n": - keyFromDisk = salt.crypt.clean(orgkey) + keyFromDisk = salt.crypt.clean_key(keyFromDisk) if keyFromDisk != load["pub"]: log.error( From 7469a705a63e34dcec816da8a3937f92d2054428 Mon Sep 17 00:00:00 2001 From: Justin Zandbergen Date: Mon, 26 Feb 2024 10:09:46 +0100 Subject: [PATCH 3/5] Simply check against cleaned key from disk. --- salt/channel/server.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/salt/channel/server.py b/salt/channel/server.py index 68c3005d6858..796607204859 100644 --- a/salt/channel/server.py +++ b/salt/channel/server.py @@ -371,15 +371,7 @@ def _auth(self, load, sign_messages=False): elif os.path.isfile(pubfn): # The key has been accepted, check it with salt.utils.files.fopen(pubfn, "r") as pubfn_handle: - keyFromDisk = pubfn_handle.read() - - # if the keyFromDisk has a final newline it is a oldstyle key - # if we clean it, it will not match. Only clean the key if it - # is a new style key. - if keyFromDisk[-1:] != "\n": - keyFromDisk = salt.crypt.clean_key(keyFromDisk) - - if keyFromDisk != load["pub"]: + if salt.crypt.clean_key(pubfn_handle.read()) != salt.crypt.clean_key(load["pub"]) log.error( "Authentication attempt from %s failed, the public " "keys did not match. This may be an attempt to compromise " From 90a5ce05fda6379db088e2ac8b19fef25b49caef Mon Sep 17 00:00:00 2001 From: Justin Zandbergen Date: Mon, 26 Feb 2024 10:40:10 +0100 Subject: [PATCH 4/5] Add changelog entry --- changelog/66126.fixed.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog/66126.fixed.md diff --git a/changelog/66126.fixed.md b/changelog/66126.fixed.md new file mode 100644 index 000000000000..9879189e6446 --- /dev/null +++ b/changelog/66126.fixed.md @@ -0,0 +1,2 @@ +Fixed a issue with server channel where a minion's public key +would be rejected if it contained a final newline character. From 453b76efd1a9927e5cf3dfd45f8ca05d1b825b19 Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Wed, 28 Feb 2024 09:27:39 -0700 Subject: [PATCH 5/5] Refactor and add some tests Added the check to a few other places in channel server --- salt/channel/server.py | 16 +++++++-- tests/pytests/unit/channel/__init__.py | 0 tests/pytests/unit/channel/test_server.py | 43 +++++++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/pytests/unit/channel/__init__.py create mode 100644 tests/pytests/unit/channel/test_server.py diff --git a/salt/channel/server.py b/salt/channel/server.py index aced83165163..47b8f96c0d8f 100644 --- a/salt/channel/server.py +++ b/salt/channel/server.py @@ -52,6 +52,16 @@ def factory(cls, opts, **kwargs): transport = salt.transport.request_server(opts, **kwargs) return cls(opts, transport) + @classmethod + def compare_keys(cls, key1, key2): + """ + Normalize and compare two keys + + Returns: + bool: ``True`` if the keys match, otherwise ``False`` + """ + return salt.crypt.clean_key(key1) == salt.crypt.clean_key(key2) + def __init__(self, opts, transport): self.opts = opts self.transport = transport @@ -371,7 +381,7 @@ def _auth(self, load, sign_messages=False): elif os.path.isfile(pubfn): # The key has been accepted, check it with salt.utils.files.fopen(pubfn, "r") as pubfn_handle: - if salt.crypt.clean_key(pubfn_handle.read()) != salt.crypt.clean_key(load["pub"]) + if not self.compare_keys(pubfn_handle.read(), load["pub"]): log.error( "Authentication attempt from %s failed, the public " "keys did not match. This may be an attempt to compromise " @@ -480,7 +490,7 @@ def _auth(self, load, sign_messages=False): # case. Otherwise log the fact that the minion is still # pending. with salt.utils.files.fopen(pubfn_pend, "r") as pubfn_handle: - if salt.crypt.clean_key(pubfn_handle.read()) != load["pub"]: + if not self.compare_keys(pubfn_handle.read(), load["pub"]): log.error( "Authentication attempt from %s failed, the public " "key in pending did not match. This may be an " @@ -536,7 +546,7 @@ def _auth(self, load, sign_messages=False): # so, pass on doing anything here, and let it get automatically # accepted below. with salt.utils.files.fopen(pubfn_pend, "r") as pubfn_handle: - if salt.crypt.clean_key(pubfn_handle.read()) != load["pub"]: + if not self.compare_keys(pubfn_handle.read(), load["pub"]): log.error( "Authentication attempt from %s failed, the public " "keys in pending did not match. This may be an " diff --git a/tests/pytests/unit/channel/__init__.py b/tests/pytests/unit/channel/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/pytests/unit/channel/test_server.py b/tests/pytests/unit/channel/test_server.py new file mode 100644 index 000000000000..7e72f2f03d0c --- /dev/null +++ b/tests/pytests/unit/channel/test_server.py @@ -0,0 +1,43 @@ +import pytest + +import salt.channel.server as server + + +@pytest.fixture +def key_data(): + return [ + "-----BEGIN PUBLIC KEY-----", + "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoe5QSDYRWKyknbVyRrIj", + "rm1ht5HgKzAVUber0x54+b/UgxTd1cqI6I+eDlx53LqZSH3G8Rd5cUh8LHoGedSa", + "E62vEiLAjgXa+RdgcGiQpYS8+Z2RvQJ8oIcZgO+2AzgBRHboNWHTYRRmJXCd3dKs", + "9tcwK6wxChR06HzGqaOTixAuQlegWbOTU+X4dXIbW7AnuQBt9MCib7SxHlscrqcS", + "cBrRvq51YP6cxPm/rZJdBqZhVrlghBvIpa45NApP5PherGi4AbEGYte4l+gC+fOA", + "osEBis1V27djPpIyQS4qk3XAPQg6CYQMDltHqA4Fdo0Nt7SMScxJhfH0r6zmBFAe", + "BQIDAQAB", + "-----END PUBLIC KEY-----", + ] + + +@pytest.mark.parametrize("linesep", ["\r\n", "\r", "\n"]) +def test_compare_keys(key_data, linesep): + src_key = linesep.join(key_data) + tgt_key = "\n".join(key_data) + assert server.ReqServerChannel.compare_keys(src_key, tgt_key) is True + + +@pytest.mark.parametrize("linesep", ["\r\n", "\r", "\n"]) +def test_compare_keys_newline_src(key_data, linesep): + src_key = linesep.join(key_data) + linesep + tgt_key = "\n".join(key_data) + assert src_key.endswith(linesep) + assert not tgt_key.endswith("\n") + assert server.ReqServerChannel.compare_keys(src_key, tgt_key) is True + + +@pytest.mark.parametrize("linesep", ["\r\n", "\r", "\n"]) +def test_compare_keys_newline_tgt(key_data, linesep): + src_key = linesep.join(key_data) + tgt_key = "\n".join(key_data) + "\n" + assert not src_key.endswith(linesep) + assert tgt_key.endswith("\n") + assert server.ReqServerChannel.compare_keys(src_key, tgt_key) is True