From 166005abd180ca45ddeed91eebc157a75717da0c Mon Sep 17 00:00:00 2001 From: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com> Date: Thu, 23 Jul 2026 18:16:57 -0500 Subject: [PATCH 1/4] VM-1452: replace pickle cache serialization with JSON Remove insecure pickle.loads from the Ansible record cache path (CWE-502) and round-trip Record state via Fernet-encrypted JSON. Co-authored-by: Cursor --- .../keeper_secrets_manager_ansible/README.md | 5 + .../keeper_secrets_manager/README.md | 5 + .../tests/keeper_cache_encrypt_test.py | 147 ++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py diff --git a/integration/keeper_secrets_manager_ansible/README.md b/integration/keeper_secrets_manager_ansible/README.md index ee548fb05..9f73bc7b4 100644 --- a/integration/keeper_secrets_manager_ansible/README.md +++ b/integration/keeper_secrets_manager_ansible/README.md @@ -19,6 +19,11 @@ For more information see our official documentation page https://docs.keeper.io/ # Changes +## 1.4.1 +* **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization + - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk + - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade + ## 1.4.0 * KSM-827: Fixed Tower Execution Environment Docker image missing system packages required by AAP - Added `openssh-clients`, `sshpass`, `rsync`, and `git` to `additional_build_packages` in `execution-environment.yml` diff --git a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md index 42e63e1bf..7fbd746b8 100644 --- a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md +++ b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md @@ -119,6 +119,11 @@ configuration file or even a playbook. # Changes +## 1.4.1 +* **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization + - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk + - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade + ## 1.4.0 * KSM-827: Fixed Tower Execution Environment Docker image missing system packages required by AAP - Added `openssh-clients`, `sshpass`, `rsync`, and `git` to the EE image diff --git a/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py new file mode 100644 index 000000000..1ab6a3793 --- /dev/null +++ b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py @@ -0,0 +1,147 @@ +# -*- coding: utf-8 -*- +"""Unit tests for JSON-based record cache encrypt/decrypt (VM-1452 / CWE-502).""" + +import base64 +import io +import os +import pickle +import socket +import sys +import tempfile +import unittest +from unittest.mock import MagicMock, patch + +# ansible-core imports fcntl (unavailable on Windows). Stub ansible for unit tests +# that only exercise encrypt/decrypt crypto, not the full plugin runtime. +try: + import fcntl # noqa: F401 +except ImportError: + _ansible_stub = MagicMock() + sys.modules.setdefault("ansible", _ansible_stub) + sys.modules.setdefault("ansible.utils", _ansible_stub) + sys.modules.setdefault("ansible.utils.display", _ansible_stub) + sys.modules.setdefault("ansible.errors", _ansible_stub) + sys.modules.setdefault("ansible.module_utils", _ansible_stub) + sys.modules.setdefault("ansible.module_utils.basic", _ansible_stub) + sys.modules.setdefault("ansible.module_utils.common", _ansible_stub) + sys.modules.setdefault("ansible.module_utils.common.text", _ansible_stub) + sys.modules.setdefault("ansible.module_utils.common.text.converters", _ansible_stub) + _ansible_stub.errors.AnsibleError = Exception + _ansible_stub.module_utils.basic.missing_required_lib = lambda name: name + _ansible_stub.module_utils.common.text.converters.jsonify = lambda x: str(x) + _ansible_stub.utils.display.Display = MagicMock + +from cryptography.fernet import Fernet +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC +from keeper_secrets_manager_core.dto.dtos import Record + +from keeper_secrets_manager_ansible import KeeperAnsible + + +def _make_record(uid="uid123", title="Test Record", password="secret-pass"): + """Build a minimal Record suitable for cache serialize/deserialize.""" + record = Record.__new__(Record) + record.uid = uid + record.title = title + record.type = "login" + record.dict = { + "title": title, + "type": "login", + "fields": [ + {"type": "login", "value": ["user1"]}, + {"type": "password", "value": [password]}, + ], + "custom": [], + } + record.raw_json = None + record.record_key_bytes = os.urandom(32) + record.folder_uid = "" + record.inner_folder_uid = "" + record.revision = 1 + record.is_editable = True + record.password = password + record.links = [] + record.files = [] + return record + + +def _stub_keeper(cache_secret="unit-test-cache-secret"): + """Return a KeeperAnsible instance with __init__ bypassed.""" + with patch.object(KeeperAnsible, "__init__", lambda self, *a, **k: None): + keeper = KeeperAnsible.__new__(KeeperAnsible) + keeper.task_vars = {"keeper_record_cache_secret": cache_secret} + keeper.client = MagicMock() + keeper.action_module = None + return keeper + + +class KeeperCacheEncryptTest(unittest.TestCase): + + def test_encrypt_decrypt_round_trip(self): + keeper = _stub_keeper() + original = _make_record() + + ciphertext = keeper.encrypt([original]) + restored = keeper.decrypt(ciphertext) + + self.assertEqual(len(restored), 1) + self.assertEqual(restored[0].uid, original.uid) + self.assertEqual(restored[0].title, original.title) + self.assertEqual(restored[0].type, original.type) + self.assertEqual(restored[0].dict, original.dict) + self.assertEqual(restored[0].record_key_bytes, original.record_key_bytes) + self.assertEqual(restored[0].field("password"), ["secret-pass"]) + self.assertEqual(restored[0].field("login"), ["user1"]) + + def test_decrypt_rejects_pickle_payload(self): + """Encrypted pickle must not execute; decrypt must fail safely (VM-1452).""" + cache_secret = "attacker-controlled-secret-12345" + proof_path = None + + with tempfile.NamedTemporaryFile(delete=False) as tmp: + proof_path = tmp.name + try: + if os.path.exists(proof_path): + os.remove(proof_path) + + class Exploit: + def __reduce__(self): + return (open, (proof_path, "w")) + + hostname = socket.gethostname() + salt = hostname.zfill(32)[0:32] + kdf = PBKDF2HMAC( + algorithm=hashes.SHA256(), + length=32, + salt=salt.encode(), + iterations=390000, + ) + key = base64.urlsafe_b64encode(kdf.derive(cache_secret.encode())) + buf = io.BytesIO() + pickle.dump(Exploit(), buf) + malicious = Fernet(key).encrypt(buf.getvalue()) + + keeper = _stub_keeper(cache_secret=cache_secret) + with self.assertRaises(ValueError): + keeper.decrypt(malicious) + + self.assertFalse( + os.path.exists(proof_path), + "pickle payload must not execute during decrypt", + ) + finally: + if proof_path and os.path.exists(proof_path): + os.remove(proof_path) + + def test_decrypt_rejects_invalid_json_shape(self): + keeper = _stub_keeper() + secret_key = keeper.get_encryption_key() + bad = Fernet(secret_key).encrypt(b'{"not": "a list"}') + with self.assertRaises(ValueError) as ctx: + keeper.decrypt(bad) + self.assertIn("keeper_cache_records", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main() From 0dd0fe9aea499167c754c4d6dbd7b8b2d4be99b9 Mon Sep 17 00:00:00 2001 From: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:21:44 -0500 Subject: [PATCH 2/4] VM-1452: invalidate legacy cache and fall back to vault Ignore unusable pickle/invalid record caches without deserializing them, warn, and fetch from the vault so plays continue until keeper_cache_records rebuilds JSON. Co-authored-by: Cursor --- .../keeper_secrets_manager_ansible/README.md | 2 + .../keeper_secrets_manager/README.md | 2 + .../__init__.py | 50 +++++++++++++++-- .../tests/keeper_cache_encrypt_test.py | 54 +++++++++++++++++-- 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/integration/keeper_secrets_manager_ansible/README.md b/integration/keeper_secrets_manager_ansible/README.md index 9f73bc7b4..e9fc6dcb7 100644 --- a/integration/keeper_secrets_manager_ansible/README.md +++ b/integration/keeper_secrets_manager_ansible/README.md @@ -22,6 +22,8 @@ For more information see our official documentation page https://docs.keeper.io/ ## 1.4.1 * **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk + - Legacy or invalid registered caches are ignored; records are fetched from the vault until + `keeper_cache_records` rebuilds a JSON cache - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade ## 1.4.0 diff --git a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md index 7fbd746b8..df4980671 100644 --- a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md +++ b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md @@ -122,6 +122,8 @@ configuration file or even a playbook. ## 1.4.1 * **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk + - Legacy or invalid registered caches are ignored; records are fetched from the vault until + `keeper_cache_records` rebuilds a JSON cache - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade ## 1.4.0 diff --git a/integration/keeper_secrets_manager_ansible/keeper_secrets_manager_ansible/__init__.py b/integration/keeper_secrets_manager_ansible/keeper_secrets_manager_ansible/__init__.py index ccb65315d..5fa0f5206 100644 --- a/integration/keeper_secrets_manager_ansible/keeper_secrets_manager_ansible/__init__.py +++ b/integration/keeper_secrets_manager_ansible/keeper_secrets_manager_ansible/__init__.py @@ -46,6 +46,10 @@ display = Display() +class CacheUnusableError(ValueError): + """Encrypted record cache cannot be decrypted or deserialized; treat as a cache miss.""" + + class KeeperFieldType(Enum): FIELD = "field" CUSTOM_FIELD = "custom_field" @@ -391,9 +395,39 @@ def encrypt(self, data): def decrypt(self, ciphertext): secret_key = self.get_encryption_key() - json_bytes = Fernet(secret_key).decrypt(ciphertext) - record_dicts = json.loads(json_bytes) - return [KeeperAnsible._record_from_dict(d) for d in record_dicts] + try: + plaintext = Fernet(secret_key).decrypt(ciphertext) + except Exception as err: + raise CacheUnusableError( + "Unable to decrypt the record cache. Check keeper_record_cache_secret " + "or regenerate the cache with keeper_cache_records." + ) from err + + # Pickle protocol markers (e.g. 0x80) -- never call pickle.loads (CWE-502 / VM-1452). + if plaintext.startswith(b"\x80"): + raise CacheUnusableError( + "Unable to deserialize the record cache. The cache may be from an older " + "plugin version or is invalid. Regenerate the cache with keeper_cache_records." + ) + + try: + payload = json.loads(plaintext.decode("utf-8")) + except (UnicodeDecodeError, json.JSONDecodeError, TypeError, ValueError) as err: + raise CacheUnusableError( + "Unable to deserialize the record cache. The cache may be from an older " + "plugin version or is invalid. Regenerate the cache with keeper_cache_records." + ) from err + + if not isinstance(payload, list): + raise CacheUnusableError( + "Unable to deserialize the record cache. Expected a list of records. " + "Regenerate the cache with keeper_cache_records." + ) + + try: + return [KeeperAnsible._record_from_dict(d) for d in payload] + except (KeyError, TypeError, ValueError) as err: + raise CacheUnusableError(str(err)) from err @staticmethod def convert_records_into_dict(records): @@ -538,7 +572,15 @@ def get_records_from_cache(self, cache, uids=None, titles=None): def get_records(self, uids=None, titles=None, cache=None, encrypt=False): if cache is not None: - records = self.get_records_from_cache(cache, uids=uids, titles=titles) + try: + records = self.get_records_from_cache(cache, uids=uids, titles=titles) + except CacheUnusableError: + # Invalidate legacy/invalid cache and start from scratch via the vault. + display.warning( + "Keeper record cache is unusable (legacy or invalid format) and was ignored. " + "Fetching records from the vault. Regenerate the cache with keeper_cache_records." + ) + records = self.get_records_from_vault(uids=uids, titles=titles, encrypt=encrypt) else: records = self.get_records_from_vault(uids=uids, titles=titles, encrypt=encrypt) diff --git a/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py index 1ab6a3793..0d7f8c9fb 100644 --- a/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py +++ b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py @@ -16,7 +16,12 @@ try: import fcntl # noqa: F401 except ImportError: + class _FakeAnsibleError(Exception): + pass + _ansible_stub = MagicMock() + _ansible_stub.AnsibleError = _FakeAnsibleError + _ansible_stub.errors.AnsibleError = _FakeAnsibleError sys.modules.setdefault("ansible", _ansible_stub) sys.modules.setdefault("ansible.utils", _ansible_stub) sys.modules.setdefault("ansible.utils.display", _ansible_stub) @@ -26,7 +31,6 @@ sys.modules.setdefault("ansible.module_utils.common", _ansible_stub) sys.modules.setdefault("ansible.module_utils.common.text", _ansible_stub) sys.modules.setdefault("ansible.module_utils.common.text.converters", _ansible_stub) - _ansible_stub.errors.AnsibleError = Exception _ansible_stub.module_utils.basic.missing_required_lib = lambda name: name _ansible_stub.module_utils.common.text.converters.jsonify = lambda x: str(x) _ansible_stub.utils.display.Display = MagicMock @@ -36,7 +40,7 @@ from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC from keeper_secrets_manager_core.dto.dtos import Record -from keeper_secrets_manager_ansible import KeeperAnsible +from keeper_secrets_manager_ansible import CacheUnusableError, KeeperAnsible def _make_record(uid="uid123", title="Test Record", password="secret-pass"): @@ -123,7 +127,7 @@ def __reduce__(self): malicious = Fernet(key).encrypt(buf.getvalue()) keeper = _stub_keeper(cache_secret=cache_secret) - with self.assertRaises(ValueError): + with self.assertRaises(CacheUnusableError): keeper.decrypt(malicious) self.assertFalse( @@ -138,10 +142,52 @@ def test_decrypt_rejects_invalid_json_shape(self): keeper = _stub_keeper() secret_key = keeper.get_encryption_key() bad = Fernet(secret_key).encrypt(b'{"not": "a list"}') - with self.assertRaises(ValueError) as ctx: + with self.assertRaises(CacheUnusableError) as ctx: keeper.decrypt(bad) self.assertIn("keeper_cache_records", str(ctx.exception)) + def test_get_records_falls_back_to_vault_on_legacy_cache(self): + """Unusable cache is ignored; records are fetched from the vault.""" + keeper = _stub_keeper() + vault_record = _make_record(uid="from-vault", title="Vault Record") + keeper.client.get_secrets.return_value = [vault_record] + + secret_key = keeper.get_encryption_key() + legacy_cache = Fernet(secret_key).encrypt(b"\x80\x04legacy-pickle-bytes") + + with patch("keeper_secrets_manager_ansible.display.warning") as mock_warn: + records = keeper.get_records(uids=["from-vault"], cache=legacy_cache) + + self.assertEqual(len(records), 1) + self.assertEqual(records[0].uid, "from-vault") + keeper.client.get_secrets.assert_called_once_with(["from-vault"]) + mock_warn.assert_called_once() + self.assertIn("ignored", mock_warn.call_args[0][0]) + + def test_get_records_falls_back_to_vault_on_invalid_json_cache(self): + keeper = _stub_keeper() + vault_record = _make_record(uid="from-vault") + keeper.client.get_secrets.return_value = [vault_record] + + secret_key = keeper.get_encryption_key() + bad_cache = Fernet(secret_key).encrypt(b'{"not": "a list"}') + + records = keeper.get_records(uids=["from-vault"], cache=bad_cache) + self.assertEqual(records[0].uid, "from-vault") + keeper.client.get_secrets.assert_called_once_with(["from-vault"]) + + def test_get_records_legacy_cache_vault_miss_still_fails(self): + """After cache invalidate, missing vault records still raise.""" + keeper = _stub_keeper() + keeper.client.get_secrets.return_value = [] + + secret_key = keeper.get_encryption_key() + legacy_cache = Fernet(secret_key).encrypt(b"\x80\x04legacy") + + with self.assertRaises(Exception) as ctx: + keeper.get_records(uids=["missing-uid"], cache=legacy_cache) + self.assertIn("missing-uid", str(ctx.exception)) + if __name__ == "__main__": unittest.main() From f27b5afeca4478d0d63692affa1ebd5159bf0807 Mon Sep 17 00:00:00 2001 From: Ivan Dimov <78815270+idimov-keeper@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:24:14 -0500 Subject: [PATCH 3/4] VM-1452: avoid AnsibleError str() in cache fallback test Newer ansible-core can recurse in AnsibleError.__str__; assert on exception args instead so CI stays green. Co-authored-by: Cursor --- .../tests/keeper_cache_encrypt_test.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py index 0d7f8c9fb..b892d5432 100644 --- a/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py +++ b/integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py @@ -186,7 +186,12 @@ def test_get_records_legacy_cache_vault_miss_still_fails(self): with self.assertRaises(Exception) as ctx: keeper.get_records(uids=["missing-uid"], cache=legacy_cache) - self.assertIn("missing-uid", str(ctx.exception)) + + # Prefer args over str()/ .message — newer ansible-core AnsibleError.__str__ + # can recurse when formatting the exception message. + detail = ctx.exception.args[0] if ctx.exception.args else "" + self.assertIn("missing-uid", detail) + keeper.client.get_secrets.assert_called_once_with(["missing-uid"]) if __name__ == "__main__": From e371c6cea48488e14447b47e45d05f30cb59240d Mon Sep 17 00:00:00 2001 From: Stas Schaller Date: Mon, 27 Jul 2026 14:33:40 -0400 Subject: [PATCH 4/4] docs(ansible): correct changelog version 1.4.1 -> 1.5.0 --- integration/keeper_secrets_manager_ansible/README.md | 2 +- .../keepersecurity/keeper_secrets_manager/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/keeper_secrets_manager_ansible/README.md b/integration/keeper_secrets_manager_ansible/README.md index e9fc6dcb7..99698b3d1 100644 --- a/integration/keeper_secrets_manager_ansible/README.md +++ b/integration/keeper_secrets_manager_ansible/README.md @@ -19,7 +19,7 @@ For more information see our official documentation page https://docs.keeper.io/ # Changes -## 1.4.1 +## 1.5.0 * **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk - Legacy or invalid registered caches are ignored; records are fetched from the vault until diff --git a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md index df4980671..2636d5a7e 100644 --- a/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md +++ b/integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md @@ -119,7 +119,7 @@ configuration file or even a playbook. # Changes -## 1.4.1 +## 1.5.0 * **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk - Legacy or invalid registered caches are ignored; records are fetched from the vault until