Let AES decrypt read both salt/IV normalizations, and annotate the md5 calls

[wintonzheng]

Ticket

No Linear ticket. Driven by an Oneleet code-security finding against skyvern/forge/sdk/encrypt/aes.py line 48 (high severity), plus its counterpart on line 50 of the sync source.

Problem

The finding reports Use of weak MD5 hash for security. Consider usedforsecurity=False. Two md5() calls in this file are missing that annotation — default_iv and the fallback IV.

The bigger issue is that this file has drifted from the repo it is synced from. This copy normalizes salt/IV with sha256 on the primary path and keeps md5 for decrypt fallbacks; the source copy still uses md5 for both. The sync is a whole-file copy, so the next upstream change that touches aes.py overwrites this file — and the source copy has no sha256 decrypt candidates. Any self-hosted deployment that stored ciphertext under the sha256 normalization would silently lose the ability to decrypt it. Credentials and org auth tokens go through this path.

That is not a hypothetical: it happens automatically on the next sync that touches this file.

Solution

Converge both copies on one file that reads every normalization ever written, so the sync becomes a no-op here instead of a data-loss event.

• Decrypt becomes a superset. _decrypt_fallbacks builds candidates from both the md5 and sha256 normalizations, for the primary pair and every configured fallback, deduped and skipping the primary.
• Every md5() call is annotated usedforsecurity=False, with a comment giving the accurate reason: every input is a fixed public constant or a 256-bit HMAC-SHA256 digest from bootstrap.py, so what makes a derived salt/IV unguessable is the secrecy of that input, not md5's collision resistance.

Please read this part before approving. Converging means this copy's encrypt path moves from sha256 back to md5, matching the sync source. Nothing becomes unreadable — sha256 is now a decrypt candidate, so anything already stored still opens, and the rollback direction works too (verified below). But new ciphertext is written with md5-derived parameters again.

That is deliberate and temporary. The constraint is that the sync source cannot change its encrypt path yet: doing so would strand anything written between deploy and a rollback. Moving encrypt is only safe once every running instance can already read the target format, which is exactly what this change establishes. The follow-up flips both copies to sha256, and then to random per-message IVs in a versioned payload.

On the IV specifically: md5 is not the weakness there. The IV is deterministic — one per deployment rather than one per message — which costs AES-CBC its semantic security. Changing the hash does not fix that; random IVs do.

bootstrap.py is intentionally untouched. This copy always inserts the primary pair into the decrypt fallbacks and the source copy only does so when salt and IV are unset; the new _decrypt_fallbacks covers the primary pair either way, so the difference no longer matters.

How Has This Been Tested?

Both upgrade directions were simulated by loading this branch's module and current main's side by side, with explicit salt and IV as bootstrap.py supplies them:

• Ciphertext written by current main (sha256) → readable by this branch.
• Ciphertext written by this branch (md5) → readable by current main, so a rollback is safe.

Two tests added, both mutation-checked by reverting the mechanism singly:

Test	Guards	Reverting →
test_decrypt_reads_sha256_normalized_ciphertext	upgrading off a sha256-normalizing release	drop the sha256 candidates → fails
test_encrypt_output_still_opens_with_md5_parameters_alone	rollback path	move encrypt to sha256 → fails

The existing test_decrypts_ciphertext_created_with_legacy_primary_normalization golden ciphertext still passes unchanged, and has been added to the sync source's copy so a future sync stops stripping it.

Run against the identical files in the source repo: test_aes_fallback_decrypt.py, test_encrypt_bootstrap.py, test_secret_encryption.py — 29 passed; pytest tests/unit -k "encrypt or aes or secret or credential or token" — 2285 passed. ruff format clean; ruff check reports 4 pre-existing findings in encrypt/decrypt that are identical on main.

Artifacts (if appropriate):

OSS-main ciphertext readable after upgrade : True
OSS-main can read post-upgrade ciphertext  : True

Residual risk: more decrypt candidates means marginally more chances for strict PKCS#7 to accept wrong-key garbage on a decrypt that would otherwise fail outright. That tradeoff already exists in the multi-key loop; this widens it from 1 candidate to 2 per configured pair, and only on the failure path.