Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable md5 checksums in FIPS mode with Python 3.9 and later #2769

Closed
1 of 2 tasks
johnwheffner opened this issue Sep 29, 2022 · 8 comments
Closed
1 of 2 tasks

Enable md5 checksums in FIPS mode with Python 3.9 and later #2769

johnwheffner opened this issue Sep 29, 2022 · 8 comments
Labels
feature-request This issue requests a feature. p1 This is a high priority issue

Comments

@johnwheffner
Copy link

Describe the feature

Currently on a FIPS-enabled system -- at least one where hashlib uses openssl -- the md5 hash is disabled. The botocore library currently detects this and avoids calculating the md5 digest where it's optional and throws an MD5UnavailableError for operations where it's mandatory. Unfortunately, this limits significantly functionality on FIPS systems. For instance, S3 SSE-C and delete_objects() (#2308) require md5.

Hashlib in Python 3.9 and later now has an argument usedforsecurity that enable use of "insecure" hashes like md5 even when FIPS mode is enabled, by indicating the use of the hash is not security related. (Red Hat has also added this argument to the 3.6-based platform python they ship with RHEL 8.) I'd argue that AWS's use of md5 is generally to protect against faults rather than for security, and it seems reasonable to use this feature to enable md5 when available.

Use Case

Some significant functionality is currently unavailable on FIPS-enabled systems.

Proposed Solution

A simple solution would be to enhance the feature detection in botocore/compat.py to also check for usedforsecurity argument support, and use it if available in get_md5.

Since md5 calculation is exposed as a public function, it might not be appropriate to categorically say that all uses are not for security, so an "opt-in" approach that involves updating all callers may be necessary.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

1.21.10

Environment details (OS name and version, etc.)

Linux, RHEL 8

@johnwheffner johnwheffner added feature-request This issue requests a feature. needs-triage This issue or PR still needs to be triaged. labels Sep 29, 2022
@tim-finnigan
Copy link
Contributor

Thanks for the feature request. This is something that should be brought up with team for further discussion. I'll mark this issue as needing review and in the meantime others can 👍 or comment on this issue if they have any additional feedback to share related to this request.

@tim-finnigan tim-finnigan added needs-review This issue or pull request needs review from a core team member. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 29, 2022
@RyanFitzSimmonsAK RyanFitzSimmonsAK added the p2 This is a standard priority issue label Nov 3, 2022
@szelenka
Copy link

this is a pretty basic code change, if we submit a PR will a maintainer review?

@dpb-bah
Copy link

dpb-bah commented Dec 8, 2023

Can confirm this solution works on a testing environment that is FIPS enabled with a forked repo with this change. Without it, a crucial feature of our application is not functional and so we cannot wait for a newer version of botocore to implement this.

Forked Repo: https://github.com/fedspendingtransparency/botocore-python39-md5/tree/md5-fix (based on botocore 1.31.64)
Change: fedspendingtransparency@1898f1d

@johnbumgardner
Copy link

Wanted to bump this. Encountering a similar problem on our application working with the ACM cli

@kellertk kellertk added p1 This is a high priority issue and removed p2 This is a standard priority issue labels Mar 14, 2024
@stav-bitanski
Copy link

stav-bitanski commented Jun 27, 2024

Same issue here, why isn't there a solution yet? @kellertk @johnwheffner

@kellertk kellertk removed the needs-review This issue or pull request needs review from a core team member. label Jun 27, 2024
@kellertk
Copy link
Contributor

MD5 usage in general across our services is something we're addressing this year, precisely because this is a sharp edge in FIPS environments. S3 isn't the only service that uses MD5 for integrity - SQS does as well. There are some changes planned to AWS services that will eventually get rid of MD5 even as an integrity check. Although I'm not sure when that might be released, that will be the long term solution for this.

We can't simply use usedforsecurity, because we still support Python 3.8, and as you've noted this is only available in 3.9+. Because of that, we're not going to be able to change this until there are changes to the affected AWS services.

P.S.: Please do not ask owners of this repository for status updates on platforms that aren't GitHub, like LinkedIn.

@kellertk kellertk closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

@jpveooys
Copy link

jpveooys commented Oct 8, 2024

We can't simply use usedforsecurity, because we still support Python 3.8, and as you've noted this is only available in 3.9+.

What about conditionally using usedforsecurity=False when running on Python 3.9+?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requests a feature. p1 This is a high priority issue
Projects
None yet
Development

No branches or pull requests

9 participants