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. diff --git a/salt/channel/server.py b/salt/channel/server.py index e4eabd400760..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()) != 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