Skip to content

Set usedforsecurity=False in hashlib methods (FIPS compliance) #27483

New issue

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

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

Already on GitHub? # to your account

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Nov 14, 2023

Solves #27034 (cc @DueViktor).

This PR makes transformers FIPS-compliant regarding hashlib usage by setting usedforsecurity=False in every hashlib method (used for file checking, not cryptography purposes). It's based on utilities added in huggingface/huggingface_hub#1782 and released in huggingface_hub==v0.19.0.

Note: before merging this we need to release a new tokenizers version that would allow new huggingface_hub version (see huggingface/tokenizers#1385). Tests are currently failing because of this which is expected. cc @ArthurZucker what's the status on the next release?

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for updating!

Happy to merge once all dependencies have been handled. Would be good to have a quick review from @ydshieh who is the packages and CI master 👑

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

LGTM.

Usually we don't touch any file udner examples/research_projects, but it's fine here (for me).

(not sure the name insecure_hashlib is the best name, but that is not the point of this PR 😄 )

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks! Not 100% sure we need to rely on the utility as it's a pretty small piece of code for something pretty code + not very intuitive to me that we need to import from hf hub. If others are fine good for me as well

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 14, 2023

Thanks! Not 100% sure we need to rely on the utility as it's a pretty small piece of code for something pretty code + not very intuitive to me that we need to import from hf hub. If others are fine good for me as well

Agree it's not big piece of code. But having it from an explicitly named insecure_hashlib module makes it more easily distinguishable IMO => no need to check each hashlib call to verify that the parameter is correctly set. Also you would have to duplicate the logic that checks whether you are running on python 3.9 or not (2 lines, but still not best to copy-paste around).
Also the dependency on huggingface_hub already exists so no big deal IMO (and updating to latest version forces to get the latest fixes we shipped -that's more of a cool side-effect)

@ArthurZucker
Copy link
Collaborator

You are right! 🤗

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 15, 2023

Thanks for merging #27494 @ArthurZucker ! I merged main into this PR and once CI is green I think we'll be good to merge :)

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 15, 2023

Still some issue with the dependencies... 😕
I opened a PR in huggingface_hub and will make another patch release later.

@Wauplin Wauplin marked this pull request as draft November 15, 2023 17:41
@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 15, 2023

I converted this PR to draft because of the dependency issues we're having. I'll push a new fix in huggingface_hub to definitely settle this (see slack thread -internal- for explanations).

Please do not merge even if CI is green.

@Wauplin Wauplin marked this pull request as ready for review November 16, 2023 14:03
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for all the investigative work on this!

@amyeroberts
Copy link
Collaborator

@Wauplin I can see the PR was marked as ready for review again. Does this mean it's now mergeable?

Would be good to get a final 👍 from @ydshieh to make sure all relevant files for our CI have been updated.

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 16, 2023

Look good!

@amyeroberts
Copy link
Collaborator

Confirmed with @Wauplin over slack that the PR is good to go - merging!

@amyeroberts amyeroberts merged commit fd65aa9 into main Nov 16, 2023
@amyeroberts amyeroberts deleted the 27034-fips-compliance-regarding-hashlib branch November 16, 2023 14:29
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
…gingface#27483)

* Set usedforsecurity=False in hashlib methods (FIPS compliance)

* trigger ci

* tokenizers version

* deps

* bump hfh version

* let's try this
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants