torch.hub security improvement: add new trust_repo parameter #72060
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit c03c9dd (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. |
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @vmoens , I took a brief look but this looks good. I have a few comments below, LMK what you think. We'll also need to add a few tests ideally (although I'm not sure yet how to handle the prompt)
torch/hub.py
Outdated
| if is_trusted: | ||
| print("The repository is already trusted.") | ||
| return |
There was a problem hiding this comment.
I feel like we should just return without a message here. IIUC this should only ever happen if the user used trust_repo=False while the repo is already trusted?
There was a problem hiding this comment.
yes that's the case.
I thought that if one asks for trust_repo=False then answers yes to the prompt, then a notification that this was already trusted could be useful. But happy to remove this if there's no use.
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
_PREDEFINED_TRUSTED list to tuple
I have written 4 tests:
|
_TRUSTED_REPO_PREFIXES
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks a lot @vmoens ,
I added a few tests and also made sure to check how many times input() was called, to make sure we prompt the user only when we need to. I also removed the use of the legacy_file and instead just check the content of the cache directly, as discussed offline. I'll approve the PR, if you're OK with my own change let's merge this next week and road-test it :) !
NicolasHug
changed the title
Awesome sounds good to me! Thanks for the many improvements |
|
@pytorchmergebot please merge this |
|
@pytorchmergebot merge this please .... I'm begging ? |
|
Hey @vmoens. |
Summary: As pointed by #71205, `torch.hub.load` assumes that the user trusts the repo from where the code is gathered and exececuted. We propose a solution to make sure that the user is aware of the security threat that this can represent. **Solution**: Adds a `trust_repo` parameter to the `load`, `list` and `help` functions in torch.hub. For now, the default `trust_repo=None` warns that, in the future, the user will need to authorize explicitly every repo before downloading it. Once the repo has been trusted (via `trust_repo=True` or via a command prompt input) it will be added to the list of trusted repositories. Pull Request resolved: #72060 Approved by: https://github.com/NicolasHug Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/41992791e82566eb977d0a2e062cca0c7fd9d56d Reviewed By: b0noI Differential Revision: D35404306 Pulled By: vmoens fbshipit-source-id: 30eb34135a4961abdc8fba70e3cb085dc5c073d0
As pointed by #71205,
torch.hub.loadassumes that the user trusts the repo from where the code is gathered and exececuted. We propose a solution to make sure that the user is aware of the security threat that this can represent.Solution: Adds a
trust_repoparameter to theload,listandhelpfunctions in torch.hub.For now, the default
trust_repo=Nonewarns that, in the future, the user will need to authorize explicitly every repo before downloading it.Once the repo has been trusted (via
trust_repo=Trueor via a command prompt input) it will be added to the list of trusted repositories.