fix: sign and verify on-disk pickle/FAISS caches with HMAC (APPENG-5695, APPENG-5696) - #321
fix: sign and verify on-disk pickle/FAISS caches with HMAC (APPENG-5695, APPENG-5696)#321gnetanel wants to merge 1 commit into
Conversation
… read respectivly
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
zvigrinberg
left a comment
zvigrinberg
left a comment
There was a problem hiding this comment.
Hi @gnetanel
Please see my comments.
|
|
||
| def resolve_key_material() -> bytes: | ||
| """Resolve HMAC key material from env, shared secret, or a local key file.""" | ||
| for env_name in (CACHE_INTEGRITY_KEY_ENV, CREDENTIAL_ENCRYPTION_KEY_ENV): |
There was a problem hiding this comment.
@gnetanel If CACHE_INTEGRITY_KEY_ENV will be set in deployment and will used for the HMAC signing for creating pickle files , but in a restart it will be omitted, this will fall back to verify according to only CREDENTIAL_ENCRYPTION_KEY_ENV, which falsely will fail the verification before loading pickle files
Instead, go only for CREDENTIAL_ENCRYPTION_KEY_ENV, and omit the other one ( unless you want to add it to the deployment' secrets keys mounted into that env var)
| def _default_key_file() -> Path: | ||
| data_dir = os.environ.get("EXPLOIT_IQ_DATA_DIR", "./.cache/am_cache/") | ||
| return Path(data_dir) / ".secrets" / "cache_integrity_key" |
There was a problem hiding this comment.
@gnetanel This is quite dangerous, as the default_key_file ( when no env var available) , which is used to generate random key out of it, is persisted on the disk/pvc
So the attacker can gain access to it and using this key, can create any files he wants and sign them using the key so they will be considered legitimate.
But this is not the only problem...
What happens if it deleted ( by mistake or intentionally), next same request or request that uses the same pickle file, will use another random generated key in order to verify integrity and it will fail.
| if value: | ||
| return value.encode("utf-8") | ||
|
|
||
| key_file = Path(os.environ.get(CACHE_INTEGRITY_KEY_FILE_ENV, str(_default_key_file()))) |
There was a problem hiding this comment.
@gnetanel Don't fallback to default key file, it's security concern and error prone as well.
Instead, if env var is not populated ( you should decide if it's going to be CREDENTIAL_ENCRYPTION_KEY_ENV or CACHE_INTEGRITY_KEY_ENV) you should raise ValueError with an appropriate error message.
| ] | ||
|
|
||
|
|
||
| def integrity_manifest_path(folder_path: str | os.PathLike[str], index_name: str = "index") -> Path: |
There was a problem hiding this comment.
@gnetanel minor - index_name is not being used.
3 participants
dump_pickle_signed/load_pickle_verified).index.pkl) on VDB write/load.