From 935a5807caa17342da804f8c82b1d2f902cc4915 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Wed, 16 Feb 2022 17:36:05 -0700 Subject: [PATCH] Fix warts in new minion auth --- salt/crypt.py | 34 +++-- tests/pytests/unit/test_crypt.py | 106 +++++++++++++ tests/pytests/unit/transport/test_zeromq.py | 157 ++++++++++++++++++++ 3 files changed, 281 insertions(+), 16 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index b8a96ff29011..76870216fd67 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -266,9 +266,8 @@ def verify_signature(pubkey_path, message, signature): try: return pubkey.verify(digest, signature) except RSA.RSAError as exc: - if exc.args[0] == "bad signature": - return False - raise + log.debug("Signature verification failed: %s", exc.args[0]) + return False else: verifier = PKCS1_v1_5.new(pubkey) return verifier.verify( @@ -793,20 +792,23 @@ def handle_signin_response(self, sign_in_payload, payload): clear_signature = payload["sig"] payload = salt.payload.loads(clear_signed_data) - auth["aes"] = self.verify_master(payload, master_pub="token" in sign_in_payload) - if not auth["aes"]: - log.critical( - "The Salt Master server's public key did not authenticate!\n" - "The master may need to be updated if it is a version of Salt " - "lower than %s, or\n" - "If you are confident that you are connecting to a valid Salt " - "Master, then remove the master public key and restart the " - "Salt Minion.\nThe master public key can be found " - "at:\n%s", - salt.version.__version__, - m_pub_fn, + if "pub_key" in payload: + auth["aes"] = self.verify_master( + payload, master_pub="token" in sign_in_payload ) - raise SaltClientError("Invalid master key") + if not auth["aes"]: + log.critical( + "The Salt Master server's public key did not authenticate!\n" + "The master may need to be updated if it is a version of Salt " + "lower than %s, or\n" + "If you are confident that you are connecting to a valid Salt " + "Master, then remove the master public key and restart the " + "Salt Minion.\nThe master public key can be found " + "at:\n%s", + salt.version.__version__, + m_pub_fn, + ) + raise SaltClientError("Invalid master key") master_pubkey_path = os.path.join(self.opts["pki_dir"], self.mpub) if os.path.exists(master_pubkey_path) and not verify_signature( diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py index 3939aac06414..a40c34b9d52f 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -12,6 +12,92 @@ import salt.master import salt.utils.files +PRIV_KEY = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAoAsMPt+4kuIG6vKyw9r3+OuZrVBee/2vDdVetW+Js5dTlgrJ +aghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLnyHNJ/HpVhMG0M07MF6FMfILtDrrt8 +ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+fu6HYwu96HggmG2pqkOrn3iGfqBvV +YVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpef8vRUrNicRLc7dAcvfhtgt2DXEZ2 +d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvTIIPQIjR8htFxGTz02STVXfnhnJ0Z +k8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cYOwIDAQABAoIBABZUJEO7Y91+UnfC +H6XKrZEZkcnH7j6/UIaOD9YhdyVKxhsnax1zh1S9vceNIgv5NltzIsfV6vrb6v2K +Dx/F7Z0O0zR5o+MlO8ZncjoNKskex10gBEWG00Uqz/WPlddiQ/TSMJTv3uCBAzp+ +S2Zjdb4wYPUlgzSgb2ygxrhsRahMcSMG9PoX6klxMXFKMD1JxiY8QfAHahPzQXy9 +F7COZ0fCVo6BE+MqNuQ8tZeIxu8mOULQCCkLFwXmkz1FpfK/kNRmhIyhxwvCS+z4 +JuErW3uXfE64RLERiLp1bSxlDdpvRO2R41HAoNELTsKXJOEt4JANRHm/CeyA5wsh +NpscufUCgYEAxhgPfcMDy2v3nL6KtkgYjdcOyRvsAF50QRbEa8ldO+87IoMDD/Oe +osFERJ5hhyyEO78QnaLVegnykiw5DWEF02RKMhD/4XU+1UYVhY0wJjKQIBadsufB +2dnaKjvwzUhPh5BrBqNHl/FXwNCRDiYqXa79eWCPC9OFbZcUWWq70s8CgYEAztOI +61zRfmXJ7f70GgYbHg+GA7IrsAcsGRITsFR82Ho0lqdFFCxz7oK8QfL6bwMCGKyk +nzk+twh6hhj5UNp18KN8wktlo02zTgzgemHwaLa2cd6xKgmAyuPiTgcgnzt5LVNG +FOjIWkLwSlpkDTl7ZzY2QSy7t+mq5d750fpIrtUCgYBWXZUbcpPL88WgDB7z/Bjg +dlvW6JqLSqMK4b8/cyp4AARbNp12LfQC55o5BIhm48y/M70tzRmfvIiKnEc/gwaE +NJx4mZrGFFURrR2i/Xx5mt/lbZbRsmN89JM+iKWjCpzJ8PgIi9Wh9DIbOZOUhKVB +9RJEAgo70LvCnPTdS0CaVwKBgDJW3BllAvw/rBFIH4OB/vGnF5gosmdqp3oGo1Ik +jipmPAx6895AH4tquIVYrUl9svHsezjhxvjnkGK5C115foEuWXw0u60uiTiy+6Pt +2IS0C93VNMulenpnUrppE7CN2iWFAiaura0CY9fE/lsVpYpucHAWgi32Kok+ZxGL +WEttAoGAN9Ehsz4LeQxEj3x8wVeEMHF6OsznpwYsI2oVh6VxpS4AjgKYqeLVcnNi +TlZFsuQcqgod8OgzA91tdB+Rp86NygmWD5WzeKXpCOg9uA+y/YL+0sgZZHsuvbK6 +PllUgXdYxqClk/hdBFB7v9AQoaj7K9Ga22v32msftYDQRJ94xOI= +-----END RSA PRIVATE KEY----- +""" + + +PUB_KEY = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoAsMPt+4kuIG6vKyw9r3 ++OuZrVBee/2vDdVetW+Js5dTlgrJaghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLny +HNJ/HpVhMG0M07MF6FMfILtDrrt8ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+f +u6HYwu96HggmG2pqkOrn3iGfqBvVYVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpe +f8vRUrNicRLc7dAcvfhtgt2DXEZ2d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvT +IIPQIjR8htFxGTz02STVXfnhnJ0Zk8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cY +OwIDAQAB +-----END PUBLIC KEY----- +""" + +PRIV_KEY2 = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAp+8cTxguO6Vg+YO92VfHgNld3Zy8aM3JbZvpJcjTnis+YFJ7 +Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvTsMBZWvmUoEVUj1Xg8XXQkBvb9Ozy +Gqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc2cKeCVvWFqDi0GRFGzyaXLaX3PPm +M7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbuT1OqDfufXWQl/82JXeiwU2cOpqWq +7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww3oJSwvMbAmgzvOhqqhlqv+K7u0u7 +FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQbQIDAQABAoIBAADrqWDQnd5DVZEA +lR+WINiWuHJAy/KaIC7K4kAMBgbxrz2ZbiY9Ok/zBk5fcnxIZDVtXd1sZicmPlro +GuWodIxdPZAnWpZ3UtOXUayZK/vCP1YsH1agmEqXuKsCu6Fc+K8VzReOHxLUkmXn +FYM+tixGahXcjEOi/aNNTWitEB6OemRM1UeLJFzRcfyXiqzHpHCIZwBpTUAsmzcG +QiVDkMTKubwo/m+PVXburX2CGibUydctgbrYIc7EJvyx/cpRiPZXo1PhHQWdu4Y1 +SOaC66WLsP/wqvtHo58JQ6EN/gjSsbAgGGVkZ1xMo66nR+pLpR27coS7o03xCks6 +DY/0mukCgYEAuLIGgBnqoh7YsOBLd/Bc1UTfDMxJhNseo+hZemtkSXz2Jn51322F +Zw/FVN4ArXgluH+XsOhvG/MFFpojwZSrb0Qq5b1MRdo9qycq8lGqNtlN1WHqosDQ +zW29kpL0tlRrSDpww3wRESsN9rH5XIrJ1b3ZXuO7asR+KBVQMy/+NcUCgYEA6MSC +c+fywltKPgmPl5j0DPoDe5SXE/6JQy7w/vVGrGfWGf/zEJmhzS2R+CcfTTEqaT0T +Yw8+XbFgKAqsxwtE9MUXLTVLI3sSUyE4g7blCYscOqhZ8ItCUKDXWkSpt++rG0Um +1+cEJP/0oCazG6MWqvBC4NpQ1nzh46QpjWqMwokCgYAKDLXJ1p8rvx3vUeUJW6zR +dfPlEGCXuAyMwqHLxXgpf4EtSwhC5gSyPOtx2LqUtcrnpRmt6JfTH4ARYMW9TMef +QEhNQ+WYj213mKP/l235mg1gJPnNbUxvQR9lkFV8bk+AGJ32JRQQqRUTbU+yN2MQ +HEptnVqfTp3GtJIultfwOQKBgG+RyYmu8wBP650izg33BXu21raEeYne5oIqXN+I +R5DZ0JjzwtkBGroTDrVoYyuH1nFNEh7YLqeQHqvyufBKKYo9cid8NQDTu+vWr5UK +tGvHnwdKrJmM1oN5JOAiq0r7+QMAOWchVy449VNSWWV03aeftB685iR5BXkstbIQ +EVopAoGAfcGBTAhmceK/4Q83H/FXBWy0PAa1kZGg/q8+Z0KY76AqyxOVl0/CU/rB +3tO3sKhaMTHPME/MiQjQQGoaK1JgPY6JHYvly2KomrJ8QTugqNGyMzdVJkXAK2AM +GAwC8ivAkHf8CHrHa1W7l8t2IqBjW1aRt7mOW92nfG88Hck0Mbo= +-----END RSA PRIVATE KEY----- +""" + + +PUB_KEY2 = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAp+8cTxguO6Vg+YO92VfH +gNld3Zy8aM3JbZvpJcjTnis+YFJ7Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvT +sMBZWvmUoEVUj1Xg8XXQkBvb9OzyGqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc +2cKeCVvWFqDi0GRFGzyaXLaX3PPmM7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbu +T1OqDfufXWQl/82JXeiwU2cOpqWq7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww +3oJSwvMbAmgzvOhqqhlqv+K7u0u7FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQ +bQIDAQAB +-----END PUBLIC KEY----- +""" + def test_get_rsa_pub_key_bad_key(tmp_path): """ @@ -63,3 +149,23 @@ def test_cryptical_dumps_invalid_nonce(): assert isinstance(ret, bytes) with pytest.raises(salt.crypt.SaltClientError, match="Nonce verification error"): assert master_crypt.loads(ret, nonce="abcde") + + +def test_verify_signature(tmpdir): + tmpdir.join("foo.pem").write(PRIV_KEY.strip()) + tmpdir.join("foo.pub").write(PUB_KEY.strip()) + tmpdir.join("bar.pem").write(PRIV_KEY2.strip()) + tmpdir.join("bar.pub").write(PUB_KEY2.strip()) + msg = b"foo bar" + sig = salt.crypt.sign_message(str(tmpdir.join("foo.pem")), msg) + assert salt.crypt.verify_signature(str(tmpdir.join("foo.pub")), msg, sig) + + +def test_verify_signature_bad_sig(tmpdir): + tmpdir.join("foo.pem").write(PRIV_KEY.strip()) + tmpdir.join("foo.pub").write(PUB_KEY.strip()) + tmpdir.join("bar.pem").write(PRIV_KEY2.strip()) + tmpdir.join("bar.pub").write(PUB_KEY2.strip()) + msg = b"foo bar" + sig = salt.crypt.sign_message(str(tmpdir.join("foo.pem")), msg) + assert not salt.crypt.verify_signature(str(tmpdir.join("bar.pub")), msg, sig) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index fccf0073ee8d..1f0515c91a9a 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -1116,3 +1116,160 @@ async def test_req_chan_auth_v2_with_master_signing(pki_dir, io_loop): pki_dir.join("minion", "minion_master.pub").read() == pki_dir.join("master", "master.pub").read() ) + + +async def test_req_chan_auth_v2_new_minion_with_master_pub(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + ret = client.auth.handle_signin_response(signin_payload, ret) + assert ret == "retry" + + +async def test_req_chan_auth_v2_new_minion_with_master_pub_bad_sig(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + + # Give the master a different key than the minion has. + mapriv = pki_dir.join("master", "master.pem") + mapriv.remove() + mapriv.write(MASTER2_PRIV_KEY.strip()) + mapub = pki_dir.join("master", "master.pub") + mapub.remove() + mapub.write(MASTER2_PUB_KEY.strip()) + + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + with pytest.raises(salt.crypt.SaltClientError, match="Invalid signature"): + ret = client.auth.handle_signin_response(signin_payload, ret) + + +async def test_req_chan_auth_v2_new_minion_without_master_pub(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + pki_dir.join("minion", "minion_master.pub").remove() + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + ret = client.auth.handle_signin_response(signin_payload, ret) + assert ret == "retry"