From f0025c6d00f174db587726bb15b78713cbbcf996 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 2 Aug 2021 13:50:37 -0700 Subject: [PATCH] Better handling of bad public keys from minions (bsc#1189040) Add changelog for #57733 Fix pre-commit check Add missing test Fix test on older pythons --- changelog/57733.fixed | 1 + salt/crypt.py | 11 ++++++-- salt/exceptions.py | 6 ++++ salt/key.py | 15 ++++++++-- salt/transport/mixins/auth.py | 12 ++++---- .../pytests/integration/cli/test_salt_key.py | 28 +++++++++++++++++++ tests/pytests/unit/test_crypt.py | 20 +++++++++++++ 7 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 changelog/57733.fixed create mode 100644 tests/pytests/unit/test_crypt.py diff --git a/changelog/57733.fixed b/changelog/57733.fixed new file mode 100644 index 0000000000..0cd55b19a6 --- /dev/null +++ b/changelog/57733.fixed @@ -0,0 +1 @@ +Better handling of bad RSA public keys from minions diff --git a/salt/crypt.py b/salt/crypt.py index f3da78f9ba..789c562e25 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -36,6 +36,7 @@ import salt.utils.verify import salt.version from salt.exceptions import ( AuthenticationError, + InvalidKeyError, MasterExit, SaltClientError, SaltReqTimeoutError, @@ -220,10 +221,16 @@ def get_rsa_pub_key(path): with salt.utils.files.fopen(path, "rb") as f: data = f.read().replace(b"RSA ", b"") bio = BIO.MemoryBuffer(data) - key = RSA.load_pub_key_bio(bio) + try: + key = RSA.load_pub_key_bio(bio) + except RSA.RSAError: + raise InvalidKeyError("Encountered bad RSA public key") else: with salt.utils.files.fopen(path) as f: - key = RSA.importKey(f.read()) + try: + key = RSA.importKey(f.read()) + except (ValueError, IndexError, TypeError): + raise InvalidKeyError("Encountered bad RSA public key") return key diff --git a/salt/exceptions.py b/salt/exceptions.py index 033a19cc54..1da15f9e69 100644 --- a/salt/exceptions.py +++ b/salt/exceptions.py @@ -111,6 +111,12 @@ class AuthenticationError(SaltException): """ +class InvalidKeyError(SaltException): + """ + Raised when we encounter an invalid RSA key. + """ + + class CommandNotFoundError(SaltException): """ Used in modules or grains when a required binary is not available diff --git a/salt/key.py b/salt/key.py index 16d20b1303..3b931152cc 100644 --- a/salt/key.py +++ b/salt/key.py @@ -9,6 +9,7 @@ import itertools import logging import os import shutil +import sys import salt.cache import salt.client @@ -643,17 +644,27 @@ class Key: keydirs.append(self.REJ) if include_denied: keydirs.append(self.DEN) + invalid_keys = [] for keydir in keydirs: for key in matches.get(keydir, []): + key_path = os.path.join(self.opts["pki_dir"], keydir, key) + try: + salt.crypt.get_rsa_pub_key(key_path) + except salt.exceptions.InvalidKeyError: + log.error("Invalid RSA public key: %s", key) + invalid_keys.append((keydir, key)) + continue try: shutil.move( - os.path.join(self.opts["pki_dir"], keydir, key), - os.path.join(self.opts["pki_dir"], self.ACC, key), + key_path, os.path.join(self.opts["pki_dir"], self.ACC, key), ) eload = {"result": True, "act": "accept", "id": key} self.event.fire_event(eload, salt.utils.event.tagify(prefix="key")) except OSError: pass + for keydir, key in invalid_keys: + matches[keydir].remove(key) + sys.stderr.write("Unable to accept invalid key for {}.\n".format(key)) return self.name_match(match) if match is not None else self.dict_match(matches) def accept_all(self): diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 102af568f3..29b38d3027 100644 --- a/salt/transport/mixins/auth.py +++ b/salt/transport/mixins/auth.py @@ -174,11 +174,11 @@ class AESReqServerMixin: tagged "auth" and returns a dict with information about the auth event - # Verify that the key we are receiving matches the stored key - # Store the key if it is not there - # Make an RSA key with the pub key - # Encrypt the AES key as an encrypted salt.payload - # Package the return and return it + - Verify that the key we are receiving matches the stored key + - Store the key if it is not there + - Make an RSA key with the pub key + - Encrypt the AES key as an encrypted salt.payload + - Package the return and return it """ if not salt.utils.verify.valid_id(self.opts, load["id"]): @@ -450,7 +450,7 @@ class AESReqServerMixin: # and an empty request comes in try: pub = salt.crypt.get_rsa_pub_key(pubfn) - except (ValueError, IndexError, TypeError) as err: + except salt.crypt.InvalidKeyError as err: log.error('Corrupt public key "%s": %s', pubfn, err) return {"enc": "clear", "load": {"ret": False}} diff --git a/tests/pytests/integration/cli/test_salt_key.py b/tests/pytests/integration/cli/test_salt_key.py index 3ec87fe580..8f29929747 100644 --- a/tests/pytests/integration/cli/test_salt_key.py +++ b/tests/pytests/integration/cli/test_salt_key.py @@ -316,3 +316,31 @@ def test_keys_generation_keysize_max(salt_key_cli): ) assert ret.exitcode != 0 assert "error: The maximum value for keysize is 32768" in ret.stderr + +def test_keys_generation_keysize_max(salt_key_cli, tmp_path): + ret = salt_key_cli.run( + "--gen-keys", "minibar", "--gen-keys-dir", str(tmp_path), "--keysize", "32769" + ) + assert ret.exitcode != 0 + assert "error: The maximum value for keysize is 32768" in ret.stderr + + +def test_accept_bad_key(salt_master, salt_key_cli): + """ + test salt-key -d usage + """ + min_name = random_string("minibar-") + pki_dir = salt_master.config["pki_dir"] + key = os.path.join(pki_dir, "minions_pre", min_name) + + with salt.utils.files.fopen(key, "w") as fp: + fp.write("") + + try: + # Check Key + ret = salt_key_cli.run("-y", "-a", min_name) + assert ret.exitcode == 0 + assert "invalid key for {}".format(min_name) in ret.stderr + finally: + if os.path.exists(key): + os.remove(key) diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py new file mode 100644 index 0000000000..aa8f439b8c --- /dev/null +++ b/tests/pytests/unit/test_crypt.py @@ -0,0 +1,20 @@ +""" +tests.pytests.unit.test_crypt +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Unit tests for salt's crypt module +""" +import pytest +import salt.crypt +import salt.utils.files + + +def test_get_rsa_pub_key_bad_key(tmp_path): + """ + get_rsa_pub_key raises InvalidKeyError when encoutering a bad key + """ + key_path = str(tmp_path / "key") + with salt.utils.files.fopen(key_path, "w") as fp: + fp.write("") + with pytest.raises(salt.crypt.InvalidKeyError): + salt.crypt.get_rsa_pub_key(key_path) -- 2.33.0