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

Fix analysis with FIPS mode #137

Merged
merged 5 commits into from
Feb 17, 2024
Merged

Conversation

MattTheCuber
Copy link
Contributor

With FIPS mode the following error is thrown.
ValueError: [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS

@roskakori
Copy link
Owner

Hm, from what I understand FIPS is a standard that enforces minimal security, and the MD5 hash used by pygount to quickly distinguish files does not comply.

Is there any other hashing algorithm in the Python standard library, that has similar performance as MD5 but conforms to FIPS? If so, I believe this would be a better solution.

(In the end, the quality of the hashing does not really matter because even if the hashes match, pygount still compares the content. This is much slower, but if it happens rarely enough, there won't be any difference.)

@roskakori roskakori self-assigned this Feb 13, 2024
@roskakori roskakori added the bug label Feb 13, 2024
@roskakori roskakori added this to the v1.6.2 milestone Feb 13, 2024
@roskakori
Copy link
Owner

@MattTheCuber I did a little digging. According to the hashlib documentation, SHA1 complies with FIPS.

According to this stackoverflow discussion, MD5 and SHA1 have similar performance. SHA1 tends to be a slower, but on some CPU's is even faster.

If you could chage the hashing from MD5 to SHA1, I would happily merge this.

@MattTheCuber
Copy link
Contributor Author

@MattTheCuber I did a little digging. According to the hashlib documentation, SHA1 complies with FIPS.

According to this stackoverflow discussion, MD5 and SHA1 have similar performance. SHA1 tends to be a slower, but on some CPU's is even faster.

If you could chage the hashing from MD5 to SHA1, I would happily merge this.

I'd be fine with changing it. However, the hashing isn't used for security from what I saw? If that is the case, then md5 works perfectly fine with the flag I provided and has identical performance.

@MattTheCuber
Copy link
Contributor Author

Hm, from what I understand FIPS is a standard that enforces minimal security, and the MD5 hash used by pygount to quickly distinguish files does not comply.

High security, not minimal. Often required on government machines.

@roskakori
Copy link
Owner

@MattTheCuber

I'd be fine with changing it. However, the hashing isn't used for security from what I saw? If that is the case, then md5 works perfectly fine with the flag I provided and has identical performance.

Yes, there is no real security requirement here, but then, nowadays md5 is bashed so much regardless of that, so it should be fun to use SHA1 and proudly claim FIPS compliance. 😉

I also checked if we can enable FIPS for the GitHub CI, but it seems we would need a Pro license for that: https://ubuntu.com/security/certifications/docs/2204/fips

High security, not minimal. Often required on government machines.

Bad wording from my side, that's what I meant.

@MattTheCuber
Copy link
Contributor Author

MattTheCuber commented Feb 14, 2024

Should I also update this doc to say SHA1?
image

@MattTheCuber
Copy link
Contributor Author

Just FYI, SHA-1 is going to be deprecated in FIPS 140-3 1 2.

@coveralls
Copy link

coveralls commented Feb 15, 2024

Coverage Status

coverage: 95.922%. remained the same
when pulling 4e83fee on MattTheCuber:patch-1
into 6acc3d6 on roskakori:master.

@roskakori
Copy link
Owner

@MattTheCuber

Just FYI, SHA-1 is going to be deprecated in FIPS 140-3 1 2.

Well, that escalated quickly.

Apparently openssl has its own benchmark one can run with

openssl speed md5 sha256

I did just that and on my MacBook Pro M1 it seems SHA256 is actually faster and FISP compliant for the foreseeable future:

The 'numbers' are in 1000s of bytes per second processed.
type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes  16384 bytes
md5              95448.96k   243805.00k   465110.78k   595160.10k   647231.02k   649816.75k
sha256          144828.15k   506919.57k  1283458.74k  1974111.04k  2353117.75k  2377405.78k

So I guess we should switch to that.

Should I also update this doc to say SHA1?

Yes, please (with SHA256 resp.).

@MattTheCuber
Copy link
Contributor Author

Will do.

@roskakori roskakori merged commit cbc7ef0 into roskakori:master Feb 17, 2024
4 of 6 checks passed
@roskakori
Copy link
Owner

@MattTheCuber Thanks, it's merged. Some CI runs failed but that's just coveralls being coveralls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants