fix(agent,tools,gateway,plugins): add usedforsecurity=False to non-security hashlib calls - #52783
fix(agent,tools,gateway,plugins): add usedforsecurity=False to non-security hashlib calls#52783AlexFucuson9 wants to merge 1 commit into
Conversation
…curity hashlib calls hashlib.md5() and hashlib.sha1() without usedforsecurity=False raise ValueError on FIPS-enabled systems (OpenSSL 3.0+ with FIPS provider). All 16 call sites use hashing for cache keys, content dedup, or upload integrity checks — none are security-sensitive (passwords, signatures). Skip gateway/platforms/wecom/wecom_crypto.py — crypto signature use.
Related to #51962 (same author, FIPS |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Looks Good
- Mechanical fix: adds
usedforsecurity=Falseto hashlib calls across 8 files - Correctly marks non-security hash operations (content dedup, file upload checksums, etc.)
- Prevents FIPS compliance issues on restricted environments
- No behavioral changes — purely compliance safety
- Touches agent, tools, CLI, gateway, and plugins uniformly
Reviewed by Hermes Agent
|
Note: This PR is a superset of #51962 (now closed). It covers all 5 files from that PR plus additional |
|
Caveat up front: I'm an integration guy, not a programmer, and don't normally contribute — so grain of salt. I hit the same FIPS issue on Rocky Linux 9.8 and can confirm this PR fixes it there. One gap remains though: some hardened setups reject MD5/SHA-1 even with With some AI assistance I built a follow-up commit on top of this PR: keep the legacy digest where the runtime allows it, fall back to SHA-256 only where it refuses. Internal values only — output shapes unchanged, so non-FIPS systems behave identically. Protocol-defined hashes (QQ Bot, WeCom, Weixin, Yuanbao) are untouched, and a test guards that boundary. Changed:
Tests simulate the strict runtime by patching Commit: AquaBearCo@0be9fa5 Feel free to cherry-pick — I'd rather extend your fix than open a competing PR. If the approach is wrong, happy to learn. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating the FIPS hardening work. The current-main premise is real, but this needs a narrower and more complete salvage.
Problems
tools/skills_hub.py:2474still computes theclawhub_search_catalog_v1cache key with barehashlib.md5; the PR updates four related cache keys but leaves this current sibling path able to fail.- The changed
_cos_signdigest is part of a COS Authorization derivation:gateway/platforms/yuanbao_media.py:305-344builds HMAC-SHA1 signatures around thesha1_of_httpvalue. That is not equivalent to the cache/dedup uses elsewhere, and the remaining HMAC-SHA1 calls are unchanged. - No regression test exercises FIPS-style constructor behavior. Existing QQ Bot tests at
tests/gateway/test_qqbot.py:725-745validate output shape only.
Suggested changes
- Cover the remaining Skills Hub cache-key call at
tools/skills_hub.py:2474and add constructor-keyword regression tests. - Keep COS protocol signing out of this non-security sweep unless its full compatibility behavior is separately verified.
Automated hermes-sweeper review.
| @@ -308,7 +308,7 @@ def _cos_sign( | |||
| ]) | |||
There was a problem hiding this comment.
This SHA-1 digest feeds StringToSign in the COS Authorization flow; the same function uses HMAC-SHA1 immediately before and after it. Please keep protocol-signing code out of this non-security sweep unless the complete FIPS behavior of the authorization algorithm is verified.
| @@ -2131,7 +2131,7 @@ def search(self, query: str, limit: int = 10) -> List[SkillMeta]: | |||
|
|
|||
There was a problem hiding this comment.
Please also cover the current ClawHub catalog cache key at tools/skills_hub.py:2474 (clawhub_search_catalog_v1), which remains a bare hashlib.md5 call outside this four-site batch.
Summary
hashlib.md5()andhashlib.sha1()withoutusedforsecurity=FalseraiseValueErroron FIPS-enabled systems (OpenSSL 3.0+ with FIPS provider).All 16 call sites across 8 files use hashing for cache keys, content dedup, or upload integrity checks — none are security-sensitive (passwords, signatures).
Files changed
agent/context_compressor.pyagent/codex_responses_adapter.pyplugins/platforms/wecom/adapter.pytools/skills_sync.pytools/skills_hub.pygateway/platforms/weixin.pygateway/platforms/yuanbao_media.pygateway/platforms/qqbot/chunked_upload.pyIntentionally skipped
gateway/platforms/wecom/wecom_crypto.py:63— crypto signature (sha1for HMAC-like verification), security-sensitive use.Test plan
python3 -m py_compileon all 8 changed files