Skip to content

chore: drop unreachable pre Python 3.9 md5 fallback in hash - #9153

Open
Andrej730 wants to merge 2 commits into
aws:developfrom
Andrej730:hash-cleanup
Open

chore: drop unreachable pre Python 3.9 md5 fallback in hash#9153
Andrej730 wants to merge 2 commits into
aws:developfrom
Andrej730:hash-cleanup

Conversation

@Andrej730

@Andrej730 Andrej730 commented Aug 1, 2026

Copy link
Copy Markdown

Hello. Removing fallback for Python <3.9 missing usedforsecurity option in hashlib.md5.
Since project is targeting Python >= 3.10, this code is dead and can be safely removed.

requires-python = ">=3.10"

Mandatory Checklist

PRs will only be reviewed after checklist is complete

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Andrej730
Andrej730 requested a review from a team as a code owner August 1, 2026 10:42
@github-actions github-actions Bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Aug 1, 2026
@roger-zhangg

Copy link
Copy Markdown
Member

Thanks for this, and apologies for the slow first response.

I checked out the branch and specifically went looking for a FIPS-mode reason to keep the fallback, since that is the usual reason usedforsecurity code is load-bearing. There isn't one — the branch being deleted is the FIPS-hostile branch, not the FIPS-safe one. This is safe to merge as-is.

Minimum supported Python

pyproject.toml:10 declares requires-python = ">=3.10", and the classifiers block lists only 3.10 through 3.14. The bundled installers build against Python 3.11.15 (installer/pyinstaller/build-linux.sh:15, installer/pyinstaller/build-mac.sh:37). So nothing supported runs below 3.10.

The fallback cannot execute

usedforsecurity landed in hashlib in Python 3.9, one full minor release below the floor. I confirmed empirically that hashlib.md5(usedforsecurity=False) constructs fine on every interpreter I had available — 3.9.23, 3.10.18, 3.12.11, 3.13.7, and 3.14.0rc2 — so the else: return hashlib.md5() at samcli/lib/utils/hash.py:21 on develop is unreachable on every supported version (and even on the unsupported 3.9).

On FIPS specifically — the direction here is the opposite of risky

This is the part I want to be explicit about, because it is easy to get backwards. Under a FIPS-enforcing OpenSSL, it is the bare hashlib.md5() that fails with ValueError, because MD5 is not an approved digest. Passing usedforsecurity=False is precisely the escape hatch that marks the digest as a non-security use and permits it. So:

  • Branch being kept (hashlib.md5(usedforsecurity=False)) = the one that works in FIPS mode.
  • Branch being deleted (hashlib.md5()) = the one that raises in FIPS mode.

Deleting it therefore removes no FIPS capability. In fact the old condition had a latent bug in the FIPS-unfriendly direction: sys.version_info.major >= 3 and sys.version_info.minor >= 9 evaluates False on a hypothetical 4.0 (minor == 0), which would have silently routed to the bare md5() and broken under FIPS. Collapsing to the unconditional call eliminates that.

One honesty caveat: I could not execute the FIPS path — _hashlib.get_fips_mode() returns 0 on my machine — so the two bullets above rest on the documented semantics of the flag, not on a test I ran in a FIPS build.

The original history supports the reading too: the fallback came in via #4812 ("use sha256 where it is possible & update md5 call to include usedforsecurity=False"), i.e. usedforsecurity=False was the fix and the bare call was back-compat for the Python 3.7/3.8 that were still supported then. It was never a FIPS accommodation.

Blast radius

_get_md5, _MAJOR_PYTHON_VERSION, and _MINOR_PYTHON_VERSION are private and referenced only inside hash.py (call sites at :41, :81, :120). _get_md5() is also the only hashlib.md5 call site anywhere in samcli/ — every other hashing call already uses sha256/sha1 — so there is no second, unguarded MD5 construction that this reasoning would miss. After the change sys is unused in the file, so dropping that import is correct.

Test results

  • tests/unit/lib/utils449 passed, 0 failed.
  • tests/unit/lib/utils/test_hash.py + tests/unit/lib/build_module/test_dependency_hash_generator.py12 passed, 0 failed.
  • ruff check, ruff format --check, and mypy on the changed file — all clean.

One optional nit, non-blocking

tests/unit/lib/utils/test_hash.py:128-133 still mirrors the version check that this PR is deleting:

def test_md5_instantiation(self, patched_hashlib):
    str_checksum("dummy-data")
    if sys.version_info.major >= 3 and sys.version_info.minor >= 9:
        patched_hashlib.md5.assert_called_with(usedforsecurity=False)
    else:
        patched_hashlib.md5.assert_called_with()

It passes as written, so this is not a blocker. But the else is now dead for the same reason the production else was, and in the spirit of the PR it could collapse to a single unconditional patched_hashlib.md5.assert_called_with(usedforsecurity=False) (which would also let import sys go from that test module). Happy to see it merged either way.

On the merge state

GitHub reports mergeStateStatus: BLOCKED here, which I dug into since it can mean either of two things. It is not a stale branch needing a rebase: mergeable: MERGEABLE and reviewDecision: REVIEW_REQUIRED. It is purely awaiting maintainer approval. Nothing for you to do.

@Andrej730

Copy link
Copy Markdown
Author

Good finding about version check still present in test_hash, removed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants