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 FIPS compatibility for python 3.9+ #2189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidesba
Copy link

Starting from python 3.9, a keyword-only argument "usedforsecurity" is introduced in hashlib hashing algorithms.
In a FIPS environment MD5 algorithm can not be used in a security context.
This change sets this not security context for MD5 fingerprint.

@jun66j5
Copy link
Contributor

jun66j5 commented Feb 22, 2023

The same pull #1928 has been requested.

@bskinn
Copy link
Contributor

bskinn commented Feb 23, 2023

Indeed, @jun66j5, but #1928 takes a different approach. Both might be valuable to consider.

@bskinn bskinn added Feature Feature request eval needed Triage -> maintainer signal labels Feb 23, 2023
@jvrsantacruz
Copy link

Changes in this PR do not change functionality and it should be harmless to merge.

Could we have the fix merged before evaluating the other related PR (#1928) that might bring many changes?

@bskinn
Copy link
Contributor

bskinn commented Feb 27, 2023

Changes in this PR do not change functionality and it should be harmless to merge.

Perhaps so, but that's not the only consideration.

Are there any non-FIPS circumstances where it might be desirable for the usedforsecurity argument to be passed as True?

If so, then at least some discussion seems worthwhile as to whether usedforsecurity should also be exposed in the user-facing API.

@jvrsantacruz
Copy link

jvrsantacruz commented Feb 27, 2023

Are there any non-FIPS circumstances where it might be desirable for the usedforsecurity argument to be passed as True?

@bskinn luckily no, the usage of md5 in the code is to calculate the fingerprint of the public key which is not a security issue as it is not hashing passwords or secrets but the public key and always that, so adding usedforsecurity=False is just a way of reflecting the current usage and it should be never True in any case.

Changed in version 3.9: All hashlib constructors take a keyword-only argument usedforsecurity with default value True. A false value allows the use of insecure and blocked hashing algorithms in restricted environments. False indicates that the hashing algorithm is not used in a security context, e.g. as a non-cryptographic one-way compression function.

https://docs.python.org/3/library/hashlib.html#hash-algorithms

@bskinn
Copy link
Contributor

bskinn commented Feb 27, 2023

@bskinn luckily no, the usage of md5 in the code is to calculate the fingerprint of the public key which is not a security issue as it is not hashing passwords or secrets but the public key and always that, so adding usedforsecurity=False is just a way of reflecting the current usage and it should be never True in any case.

Ah, ok -- makes sense.

It would be good to document this explicitly, either with a code comment or a remark in the docstring.

If there's a straightforward test that could be added, that'd be ideal too.

This also calls for a CHANGELOG entry, I think.

@bskinn bskinn removed the Feature request eval needed Triage -> maintainer signal label Feb 27, 2023
@jvrsantacruz
Copy link

Hi! How are you?

Can we help somehow to make this advance and merge it?

We're deploying a fork of paramiko and it's not the best situation.

Thank you!

@bskinn
Copy link
Contributor

bskinn commented May 9, 2023

Flagging for consideration as part of the key/auth work on #387

@scovetta
Copy link

I'd rather see the hash algorithm changed to some SHA-256 or SHA-512, since IMHO, hashing public keys is a security context, presumably because the caller would be making some decision based on the hash. So marking usedforsecurity=False would be misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants