Support FIPS enabled machines with MD5 hashing

[MattTheCuber]

FIPS enabled machines prohibit MD5 hashing for security reasons. Using hashlib.md5 in Python will throw the error ValueError: [digital envelope routines] unsupported. There are typically 2 resolutions to this problem: use the usedforsecrutiy=False flag or use SHA1 hashing. The usedforsecurity flag is easier to implement and was introduced in Python 3.9, which is the minimum Python version for this library, so that is the method I chose.

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

[mgoin]

Looks reasonable to me (cc security expert @russellb :)). You could consider making an "insecure_md5" function that provides a common wrapper for hashlib.md5(..., usedforsecurity=False)

[MattTheCuber]

You could consider making an "insecure_md5" function that provides a common wrapper for hashlib.md5(..., usedforsecurity=False)

I can do that. Let me know if you want this.

[jaredmcqueen]

excited to see traction on this. Thanks @mgoin and @MattTheCuber

[russellb]

Thank you for the report and PR! I definitely want to get this fixed.

I'm a little torn on the change. Hashing here does have security implications. See this advisory where I changed this code recently:

GHSA-rm76-4mrf-v9r8

It was previously easy to predict KV cache conflicts when using vLLM with Python 3.12. It's much more difficult now after the change since we now always start with a random base number that's different each time your run vLLM. It's a fair argument that the use of MD5 makes it more feasible to calculate and abuse hash conflicts, though it doesn't seem very feasible unless you could find out the random number used as the base of our hash calculations.

I think I'm on the side of just accepting the proposed change since I think it's "secure enough" that exploiting md5 hash collisions seems not practical with our current code. I didn't want to approve the change without giving that context first, though.

If you all are still comfortable with the change with the understanding that there IS a slight security implication here, then I'm OK with it.

Switching to SHA1 isn't trivial since it takes up a bit more memory than MD5 hashes and is slightly more expensive to calculate. We'd really need to measure the impact of the change carefully.

[MattTheCuber]

Changing to SHA1 is definitely the right solution under this context. SHA1 is FIPS certified and is the best hashing standard when needing to balance security and speed. I can send you performance evaluations later (if you want) since I don't have them handy, but SHA1 is significantly faster than MD5 hashing. I can make that swap tomorrow if you would like? I just don't know if anything externally uses the hashes and would break. If you can confirm nothing does, I don't see why to not swap. Unless you are making hundreds of thousands of hashes, I don't think memory will be a problem.

[russellb]

Thanks for the response. I had a feeling consensus wouldn't stay on MD5 with that context. I'll share this PR with some others that maintain this part of the code.

[russellb]

It looks like SHA-1 is getting retired from FIPS? Maybe we should look at skipping SHA-1 for something else?

https://csrc.nist.gov/news/2022/nist-transitioning-away-from-sha-1-for-all-apps

[russellb]

OK I need to back up. I greatly apologize, but I didn't actually read the patch and just assumed what code this was talking about and I was completely wrong. I thought this was about how we do hashing for prefix caching, but that's using Python's built in hash() function, not MD5.

Please forget everything I said in the last few messages, it is not relevant here.

I'm fine with this change, though switching to another algorithm should also be fine, I think. I'll go ahead and approve as-is though.

[MattTheCuber]

Oh man, I didn't realize that. I guess switching to SHA2 would be the best choice? I will test performance tomorrow on SHA2 vs MD5. I know SHA1 is much faster but I don't know about SHA2. Although, unless you are doing hundreds of thousands of hashes in a short time, the slowdown will likely be insignificant.

[russellb]

Thanks again for raising this. Let me know if you want to talk about any other issues that affect security compliance!

[neverpanic]

Following up on this, you have correctly identified that SHA-1 is not a good choice for FIPS because NIST has deprecated it already, and new FIPS certifications that still support SHA-1 are already getting a limited lifetime.

However, if you care about speed, you should probably just move to SHA-256. With hardware acceleration, it's probably faster than MD-5 in a lot of configurations. For example, on my M3 Pro MacBook with OpenSSL 3.5.0:

The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
md5             125530.50k   331224.17k   625809.78k   808032.60k   881958.91k   887117.14k
sha1            171406.30k   596037.87k  1588520.28k  2539942.87k  3029068.46k  3073245.18k
sha256          172196.24k   651331.16k  1671791.70k  2569031.34k  3030299.99k  3074615.98k

SHA-256 is actually the fastest of those. This may not be the case on all architectures and with all versions of OpenSSL, but it's not as simple as "let's just use MD-5 for speed reasons".

If the hash isn't used for security purposes, you can also use non-FIPS hashes, such as Blake2, for example.