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

md5 OpenSSL FIPS mode fix #7611 #7614

Merged
merged 4 commits into from May 5, 2020
Merged

md5 OpenSSL FIPS mode fix #7611 #7614

merged 4 commits into from May 5, 2020

Conversation

lhupfeldt
Copy link
Contributor

This is a fix for #7611

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits.

@@ -170,6 +170,22 @@ def __setstate__(self, state: Set[str]) -> None:
self._existing = state


def fips_safe_md5(data=b'', **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to rename this to md5() simply.

@tk0miya
Copy link
Member

tk0miya commented May 5, 2020

BTW, we've used hashlib.sha1 also. Is it needed to give usedforsecurity=False option to them too?

@tk0miya tk0miya added this to the 3.0.4 milestone May 5, 2020
@lhupfeldt
Copy link
Contributor Author

I did not see any errors from sha1 when running the test suite on the FIPS enabled server. Thi is strange as sha1 is not allowed by FIPS either I think. It may depend on a policy on the server? I have not checked whether sha1 has the 'usedforsecurity' option.

@tk0miya
Copy link
Member

tk0miya commented May 5, 2020

I guess you did not use features that uses SHA-1. Because Sphinx uses it only at 3 modules.

$ grep -r hashlib sphinx | grep sha1
sphinx/ext/graphviz.py:from hashlib import sha1
sphinx/ext/imgmath.py:from hashlib import sha1
sphinx/transforms/post_transforms/images.py:from hashlib import sha1

Could you check that sha1() accepts the option? If you don't have time, I'll make an environment that enables FIPS.

@lhupfeldt
Copy link
Contributor Author

lhupfeldt commented May 5, 2020

I ran the sphinx test suite, does that not test it?
I did have one error from image convert, because imagemagick is not installed on the server, maybe that is why I did not see any errors from sha1, it seems the use is image related. If that is the case, then I will not be able to test, imagemagick is not available for the version of redhat installed on the server.

Is it possible to handle possible sha1 issues in another patch, so that this can go in quickly?

@tk0miya
Copy link
Member

tk0miya commented May 5, 2020

Yes, some external tools are needed to enable these modules.

I guess you can check it via python console. Could you check this please?

$ python
Python 3.8.2 (default, Mar  2 2020, 00:44:41)
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from hashlib import sha1
>>> sha1(b'abcdefg').hexdigest()
'2fb5e13419fc89246865e7a324f476ec624e8740'

I think FIPS also disallows to use SHA-1, it raises an error like MD5.

@lhupfeldt
Copy link
Contributor Author

lhupfeldt commented May 5, 2020

I checked the hashlib source, it seems usedforsecurity is supported generically for all algorithms.
I also ran sha1 as you suggested. I do not get an error like I did for md5 - this does not mean that it should not be called with usedforsecurity=False, I think it may still fail depending version of FIPS (or server policy?).

@lhupfeldt
Copy link
Contributor Author

@lhupfeldt
Copy link
Contributor Author

lhupfeldt commented May 5, 2020

I did the same change for sha1. I checked from console on the FIPS enabled server that it supports the flag. I ran the test suite on my laptop where all tests pass (ImageMagick is installed), so I think there should be no negative impact from this, but it may help in cases where FIPS forbids sha1.

@tk0miya tk0miya merged commit d670214 into sphinx-doc:3.0.x May 5, 2020
@tk0miya
Copy link
Member

tk0miya commented May 5, 2020

Great work! Thank you for your contribution :-)

@lhupfeldt lhupfeldt deleted the 3.0.x branch May 5, 2020 12:14
@lhupfeldt
Copy link
Contributor Author

You're welcome.
Since the usedforsecurity flag seems to become standard from Python 3.9, you should consider reversing the order of the calls at that time.

@lhupfeldt
Copy link
Contributor Author

@tk0miya Can I ask when 3.0.4 is expected to be released? We are unable to build documentation at the moment.

@tk0miya
Copy link
Member

tk0miya commented May 21, 2020

I don't have a plan for the release. But I understand your situation. Okay, let's ship it in this weekend.

@lhupfeldt
Copy link
Contributor Author

lhupfeldt commented May 22, 2020 via email

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants