File better-handling-of-bad-public-keys-from-minions... of Package salt

Overview Repositories Revisions Requests Users Attributes Meta

File better-handling-of-bad-public-keys-from-minions-bsc-.patch of Package salt (Revision 78b45b170b9cce29ca548ca5e0d4e221)

Currently displaying revision 78b45b170b9cce29ca548ca5e0d4e221 , Show latest

From cd64b9a063771829f85d6be0e42259825cfb10c8 Mon Sep 17 00:00:00 2001
From: "Daniel A. Wozniak" <dwozniak@saltstack.com>
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 0a8b728f50..e6e4f3181e 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,
@@ -217,10 +218,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 75777ede06..59090c979c 100644
--- a/salt/key.py
+++ b/salt/key.py
@@ -11,6 +11,7 @@ import fnmatch
 import logging
 import os
 import shutil
+import sys
 
 # Import salt libs
 import salt.cache
@@ -652,17 +653,27 @@ class Key(object):
             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 (IOError, 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 003cbd8275..0f0c615408 100644
--- a/salt/transport/mixins/auth.py
+++ b/salt/transport/mixins/auth.py
@@ -184,11 +184,11 @@ class AESReqServerMixin(object):
         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"]):
@@ -460,7 +460,7 @@ class AESReqServerMixin(object):
         # 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 0edb2cf86c..2583348ce6 100644
--- a/tests/pytests/integration/cli/test_salt_key.py
+++ b/tests/pytests/integration/cli/test_salt_key.py
@@ -328,3 +328,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.32.0

Places

File better-handling-of-bad-public-keys-from-minions-bsc-.patch of Package salt (Revision 78b45b170b9cce29ca548ca5e0d4e221)

Places