ngclient: downloading targets fails if targetpath includes subdirectories #1571
Comments
|
I think this is actually a good time to get rid of this idea of using target paths as local path with directories (as it has several issues, not just that we don't create the subdirectories)... I'll take this |
jku
changed the title
|
We might want to fix this with #1412 |
MVrachev
added
the
backlog
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the destination argument to be either a directory or a filename so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <[email protected]>
@jku I am sorry I created a commit before reading this issue and noticing you are assigned to it. What other do you mean besides this one if we consider targetpaths as local paths? |
That's fine, I'm sure you needed to do something to test -- and we are annoyingly touching the same bits of code. We'll see how bad the merging/rebasing gets...
I don't want to create local directories based on a URL -- a specific TUF implementation might know that is a reasonable thing to do but I don't think the library can know that. I'm suggesting we use the whole target path (url encoded) as a filename, and then also offer a way for the client application to decide the filepath themselves (so if the client knows using target path as path is safe, it can create the directories). |
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the destination argument to be either a directory or a filename so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. This is part of bigger plan in theupdateframework#1580 Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <[email protected]>
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the arguments so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. This is clearly a "filesystem API break" for anyone depending on the actual target file names, and does not make sense if we do not plan to go forward with other updated_targets()/download_target() changes listed in theupdateframework#1580. This is part of bigger plan in theupdateframework#1580 Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <[email protected]>
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the arguments so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. This is clearly a "filesystem API break" for anyone depending on the actual target file names, and does not make sense if we do not plan to go forward with other updated_targets()/download_target() changes listed in theupdateframework#1580. This is part of bigger plan in theupdateframework#1580 Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <[email protected]>
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the arguments so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. This is clearly a "filesystem API break" for anyone depending on the actual target file names, and does not make sense if we do not plan to go forward with other updated_targets()/download_target() changes listed in theupdateframework#1580. This is part of bigger plan in theupdateframework#1580 Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <[email protected]>
ngclient has so far copied the legacy client decision of downloading files to the same directory structure as the repository uses. It has however removed the code that creates the directory structure.
So downloading any targets with paths like
"path/to/file"will fail because the local directories do not exist.The text was updated successfully, but these errors were encountered: