Skip to content

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Apr 1, 2022

Fixes #4102 (comment).

This does not change the functionality in any way, but allows the use of our datasets on FIPS compliant systems that use Python >= 3.9.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 1, 2022

💊 CI failures summary and remediations

As of commit 402270b (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@NicolasHug
Copy link
Member

NicolasHug commented Apr 1, 2022

Thanks @pmeier ,

I guess I'm missing some context but I'm a little unsure about the need for this change, or even about the utility of such parameter. As developers we just have to set usedforsecurity=False everywhere, and downstream users will just take our word for it?
Sounds like this flag should be set somehow by consumers after they audit our code, not by us?

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

I'll approve, but I don't undestand how this usedforsecurity parameter will prevent any sort of security issue. If our code was indeed used for security, there is absolutely nothing that prevents us from setting usedforsecurity=False, and downstream FIPS system will happily run it? I'm not sure what the goal is, but I hope I'm completely missing the point.

As discussed offline, it'd be useful to add a comment with a link to this PR, with more context.

@pmeier
Copy link
Collaborator Author

pmeier commented Apr 4, 2022

I'm not sure what the goal is, but I hope I'm completely missing the point.

I don't think you are, but this shouldn't be our concern. I agree, the design of the flag is dubious at best. But we are a good faith actor here. The annotation is correct: we are not using the MD5 checksum for security (read cryptography). Thus, it is on the user to evaluate if they trust the libraries to set this flag correctly. Worst case, the environment flat out forbids MD5 checksum and we are back at the situation we had before. If however the environment accepts the annotation (like FIPS seems to do), we enable users to use torchvision.datasets which was not possible for them before.

@pmeier pmeier merged commit 129455f into pytorch:main Apr 4, 2022
@pmeier pmeier deleted the nonsecurity-md5 branch April 4, 2022 07:30
@NicolasHug
Copy link
Member

this shouldn't be our concern

I'm sorry, it is our concern. You and I have spent time on this thing. We're adding maintenance burden with this PR

facebook-github-bot pushed a commit that referenced this pull request Apr 6, 2022
Summary:
* indicate md5 checksum is not used for security

* add explanation

Reviewed By: NicolasHug

Differential Revision: D35393171

fbshipit-source-id: 2e1a573d89dbeb8bbfee25e871b2c4d333835eb3
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.

3 participants