Repository: encode/starlette
Pull request: fix: pass usedforsecurity to hashlib.md5 to fix error on FIPS-enabled systems
Closes #1365 

Problem statement:
ETag checksum fails on FIPS-enabled systems when using MD5
### Checklist

<!-- Please make sure you check all these items before submitting your bug report. -->

- [x] The bug is reproducible against the latest release and/or `master`.
- [x] There are no similar issues or pull requests to fix it yet.

### Describe the bug

The ETag checksum fails when using MD5. This is causing Starlette to not work at all under Red Hat Enterprise Linux when FIPS mode is enabled.

### Debugging material

Here's the exception that's thrown:

```
INFO:     10.42.1.7:34422 - "GET /app/static/foo.html HTTP/1.1" 500 Internal Server Error
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/uvicorn/protocols/http/h11_impl.py", line 373, in run_asgi
    result = await app(self.scope, self.receive, self.send)
  File "/usr/local/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py", line 75, in __call__
    return await self.app(scope, receive, send)
  File "/usr/local/lib/python3.8/site-packages/fastapi/applications.py", line 208, in __call__
    await super().__call__(scope, receive, send)
  File "/usr/local/lib/python3.8/site-packages/starlette/applications.py", line 112, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/usr/local/lib/python3.8/site-packages/starlette/middleware/errors.py", line 181, in __call__
    raise exc
  File "/usr/local/lib/python3.8/site-packages/starlette/middleware/errors.py", line 159, in __call__
    await self.app(scope, receive, _send)
  File "/usr/local/lib/python3.8/site-packages/starlette/exceptions.py", line 82, in __call__
    raise exc
  File "/usr/local/lib/python3.8/site-packages/starlette/exceptions.py", line 71, in __call__
    await self.app(scope, receive, sender)
  File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 656, in __call__
    await route.handle(scope, receive, send)
  File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 408, in handle
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.8/site-packages/starlette/staticfiles.py", line 97, in __call__
    response = await self.get_response(path, scope)
  File "/usr/local/lib/python3.8/site-packages/starlette/staticfiles.py", line 118, in get_response
    return self.file_response(full_path, stat_result, scope)
  File "/usr/local/lib/python3.8/site-packages/starlette/staticfiles.py", line 173, in file_response
    response = FileResponse(
  File "/usr/local/lib/python3.8/site-packages/starlette/responses.py", line 267, in __init__
    self.set_stat_headers(stat_result)
  File "/usr/local/lib/python3.8/site-packages/starlette/responses.py", line 273, in set_stat_headers
    etag = hashlib.md5(etag_base.encode()).hexdigest()
ValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS
```

### Environment

- OS: Red Hat Enterprise Linux 8 in FIPS mode
- Python version: 3.8.8
- Starlette version: 0.16.0

Hints from issue discussion:
`nginx` uses the following logic:

> Unit’s ETag response header fields use the `MTIME-FILESIZE` format, where `MTIME` stands for file modification timestamp and `FILESIZE` stands for file size in bytes, both in hexadecimal.

Ref.: https://unit.nginx.org/configuration/#static-files

I've found [this](http://blog.serindu.com/2019/11/12/django-in-fips-mode/) article stating the same problem for Django. Which lead me to https://code.djangoproject.com/ticket/28401, and finally to [this PR](https://github.com/django/django/pull/14763) (merged 2 months ago) that solves the problem above.

We should probably follow the same path? As we're not on stable release, we can also change the algorithm. Check more about it [here](https://bugs.python.org/issue9216#msg285895).
What I can review, prior discussion, is a PR that supports "Red Hat Enterprise Linux when FIPS mode is enabled" using the same solution as Django, i.e. checking `usedforsecurity` flag.

As for changing the algorithm, we need further discussion, and other members should be involved.

I'm going to share this on our Gitter. 
> The ETag checksum should use a more secure hash than MD5

@landtuna could you clarify is this is a statement of fact that you are making based on your expertise in security or if this is just something that you think may be true? My understanding (which I believe is also what @Kludex is getting at above) is that ETags _may_ be used for caching and such, but _should not_ be considered secure / foolproof (for such a use case, the client should get the data and hash it with a secure hash algorithm). But I'm happy to be wrong, so just checking.
@adriangb sorry, you're right. "Should" was too strong, but if you don't change the algorithm, you'll have to wrap or monkey patch `hashlib`. I guess you'd want to stick with MD5 if changing away from it is a performance concern. Looks like django inspects `hashlib` and wraps it.
We probably want to add the `usedforsecurity=False` flag, which is available in Python 3.9 onwards.

Commit message: Merge branch 'master' into 1365-use-usedforsecurity-parameter

Patch to review:
diff --git a/starlette/_crypto.py b/starlette/_crypto.py
new file mode 100644
index 000000000..a9a36a1b2
--- /dev/null
+++ b/starlette/_crypto.py
@@ -0,0 +1,14 @@
+import hashlib
+import inspect
+
+
+# TODO: Remove when dropping support for PY38. inspect.signature() is used to
+# detect whether the usedforsecurity argument is available as this fix may also
+# have been applied by downstream package maintainers to other versions in
+# their repositories.
+if "usedforsecurity" in inspect.signature(hashlib.md5).parameters:
+    def md5_hexdigest(data: bytes, *, usedforsecurity: bool = True) -> str:
+        return hashlib.md5(data, usedforsecurity=usedforsecurity).hexdigest()
+else:
+    def md5_hexdigest(data: bytes, *, usedforsecurity: bool = True) -> str:
+        return hashlib.md5(data).hexdigest()
diff --git a/starlette/responses.py b/starlette/responses.py
index 1f9c43a21..b12cea738 100644
--- a/starlette/responses.py
+++ b/starlette/responses.py
@@ -1,4 +1,3 @@
-import hashlib
 import http.cookies
 import json
 import os
@@ -12,6 +11,7 @@
 
 import anyio
 
+from starlette._crypto import md5_hexdigest
 from starlette.background import BackgroundTask
 from starlette.concurrency import iterate_in_threadpool
 from starlette.datastructures import URL, MutableHeaders
@@ -287,7 +287,7 @@ def set_stat_headers(self, stat_result: os.stat_result) -> None:
         content_length = str(stat_result.st_size)
         last_modified = formatdate(stat_result.st_mtime, usegmt=True)
         etag_base = str(stat_result.st_mtime) + "-" + str(stat_result.st_size)
-        etag = hashlib.md5(etag_base.encode()).hexdigest()
+        etag = md5_hexdigest(etag_base.encode(), usedforsecurity=False)
 
         self.headers.setdefault("content-length", content_length)
         self.headers.setdefault("last-modified", last_modified)


Code review comments:
diff_hunk: @@ -12,6 +11,7 @@
 
 import anyio
 
+from starlette._crypto import md5_hexdigest
path: starlette/responses.py
text: @author:
I made this a "private" module. I know there isn't much history of this in Starlette, but I figure this is as good as any other use case to start with.

@user1:
I think it's fine. Although I think we should use `_compat.py` to be aligned with `httpx` and `httpcore`

@author:
done. I just thought `_crypto.py` might get us some clout / stars

diff_hunk: @@ -0,0 +1,14 @@
+import hashlib
+import inspect
+
+
+# TODO: Remove when dropping support for PY38. inspect.signature() is used to
+# detect whether the usedforsecurity argument is available as this fix may also
+# have been applied by downstream package maintainers to other versions in
+# their repositories.
+if "usedforsecurity" in inspect.signature(hashlib.md5).parameters:
+    def md5_hexdigest(data: bytes, *, usedforsecurity: bool = True) -> str:
path: starlette/_crypto.py
text: @author:
Do we straight up want to raise an error if `usedforsecurity=True` in either of these implementations? Seems like overkill but it'd also be easy to implement.

@user1:
Why would we want to do that? On which scenario?

@author:
We currently don't have any scenarios in which we'd want to pass `usedforsecurity=True` (and arguably md5 should never be used for security). Adding an error would just make it very explicit and add a barrier to misusing md5 for security in future changes.

@author:
I think there are 2 scenarios this can help with:
1. Keeps us from shooting ourselves in the foot in the future once this PR is long forgotten.
1. Keeps users from misusing this function down the road (the module is private by name, but this is Python so someone could still use it).

@user2:
No, let's just make the minimum possible change here.

diff_hunk: @@ -0,0 +1,14 @@
+import hashlib
+import inspect
+
+
+# TODO: Remove when dropping support for PY38. inspect.signature() is used to
+# detect whether the usedforsecurity argument is available as this fix may also
+# have been applied by downstream package maintainers to other versions in
+# their repositories.
+if "usedforsecurity" in inspect.signature(hashlib.md5).parameters:
path: starlette/_crypto.py
text: @author:
I decided to just check for the parameter directly here. Django has a bunch of utility functions around this, but we're only doing it once here so that seemed like overkill.

@user1:
Just for my curiosity, is the `usedforsecurity` based exclusively on the Python version? Is there an environment with Python 3.8+ that will not have `usedforsecurity` parameter?

@author:
I think the reason Django did that is that some enterprises might have backported the fix to an internal fork, or something like that. Figured we might as well follow what they did. But I'm happy to just check `sys.version_info`.

Since this is done once at import time, I don't think performance matters.

@user1:
I don't see how a _backport_ by checking the versions would be different from checking the parameter :thinking:
It looks like both are interchangeable, or am I missing something? Because if I'm not missing anything, then we could avoid the comment and just check the version :thinking: 


@author:
I think the point is that the parameter _could_ be present in Python 3.7 if someone backported it internally. Invoking @user2 (author of the Django PR) in case they can clarify.

@user2:
Yes, some distros have in fact backported this parameter.  Red Hat/Fedora/Centos has backported this to python 3.6 IIRC.

@author:
Awesome, thank you for confirming!