Repository navigation
ngclient updater: API functions argument validation #1594
Description
Description of issue or feature request:
In the tuf/ngclient/updater.py module we are providing a couple API functions with arguments: Updater.__init__, get_one_valid_targetinfo, updated_targets and download_targets.
It seems to me it's a good idea to decide which arguments in those functions require validation and if there should be any what it should be.
Current behavior:
No validation is done for:
repository_dirinUpdater.__init__()target_pathinUpdater.get_targetinfo()- maybe
targetinfoinUpdater.download_target() - maybe
target_base_urlinUpdater.download_target()
Expected behavior:
Decide if we want to add validation and if there is any what it should be:
-
repository_dirinUpdater.__init__() -
target_pathinUpdater.get_targetinfo() -
targetinfoinUpdater.download_target() -
target_base_urlinUpdater.download_target()
Activity
MVrachev commentedon Sep 27, 2021
I will document some of my thoughts:
repository_dirinUpdater.__init__(): I think we should check if therepository_direxists during the initialization and fail early.Otherwise when calling
Updater._load_local_metadata("root")an exception will be thrown.target_pathinUpdater.get_one_valid_targetinfo(): We should validate thattarget_pathis a non-empty string.Otherwise, if
target_pathis not a string aKeyErrorwill be thrown when trying to access information intargets:python-tuf/tuf/ngclient/updater.py
Line 417 in 82b0967
Additionally, we should validate that `target_path points to a file and not a directory. As only files could be targets.
targetsinUpdater.updated_targets(): Not sure if validation is needed here. Traversing the list in for the cycle is already checking thattargetsis at least a container that could be iterated.destination_directoryinUpdater.updated_targets(): I think we should validate that the directory exists. OtherwiseFileNotFoundErrorwhen trying to open itpython-tuf/tuf/ngclient/updater.py
Line 203 in 82b0967
targetinfoinUpdater.download_target(): Not sure if we need validation here.targetinfois expected to be received fromget_one_valid_targetinfoand there will be validation.destination_directoryinUpdater.download_target(): I think we should validate that the directory exists. OtherwiseFileNotFoundErrorwhen trying to open it during persisting the target.target_base_urlinUpdater.download_target(): Not sure about that one as it already has some validation when calling_ensure_trailing_slash. This ensures that at least thetarget_base_urlis a string otherwiseAttributeErrorwill be raised becauseendswith()won't be an attribute to it:python-tuf/tuf/ngclient/updater.py
Line 464 in 82b0967
jku commentedon Sep 28, 2021
let's leave this on back burner until #1580 is resolved: arguments (and how they are used) is likely to change slightly
__init__()could be fixed already but I actually think the current FileNotFoundError is fine.MVrachev commentedon Nov 5, 2021
I have updated the issue description addressing the new changes merged in #1604.
After Ngclient api polish #1604, I no longer see a point in validating
targetinfoandfilepatharguments infind_cached_target.If
filepathis an empty string the os will complain it can't open the file. The same is if iftargetinfo.pathhas an empty string assigned forpath.If
targetInfohas to be validated as if it has an emptypathor 0lengththat will be noticedwhen calling
with self._fetcher.download_file(full_url, targetinfo.length)and either the file won't be found for download ormax_legthindownload_filewill be 0 and the download will fail.I am not sure about
target_base_urlas well. Again if it's a nonvalid URL that should be noticed when downloading the file right?I am wondering should we do something to validate
target_pathinUpdater.get_targetinfo()at all.It's used to retrieve check a target and return the target information.
Given that we are adding a sanity check that TARGETPATHs should be a non-empty string, then the responsibility of validating TARGETPATHs is given to Metadata API.
MVrachev commentedon Nov 9, 2021
What do you think @jku? Am I correct in my assumptions?
Then, the only one left is
repository_dirinUpdater.__init__().We can validate that
repository_dirdoes exist and throw an error otherwise. This will help the users to know during initialization that they have provided a non-existent directory to correct themselves.jku commentedon Nov 9, 2021
WRT validating TargetFile arguments, I guess the potential check would be "is this really a TargetFile instance?" but considering that A) we support static type checking (well almost anyway: #1633) and B) there is never any need to modify the targetfile in application code C) it will fail pretty gracefully anyway, I don't think that check is needed...
I think this currently ends up complaining that repository_dir/root.json does not exist -- which seems like an accurate description of the problem: the updater isn't interested in just the directory, it needs a directory with a root.json.
So potentially we don't need to do anything here: I'm fine with that
MVrachev commentedon Nov 9, 2021
@jku I assume you agree with my thoughts about the rest of the attributes?
I agree with your thoughts about
TargetFilearguments andrepository_dir.Then, we don't want to change anything and we can close this issue, correct?
jku commentedon Nov 9, 2021
Yep, I think we agree here, let's close