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 5 commits 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)
| 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.
e43539c to
5c2ffa5
Compare
|
/test vulnerability-analysis-on-pr |
2 similar comments
|
/test vulnerability-analysis-on-pr |
|
/test vulnerability-analysis-on-pr |
3 participants
dump_pickle_signed/load_pickle_verified).index.pkl) on VDB write/load.