Skip to content

Is the TUF target path separator always "/"? #63

New issue

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

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

Already on GitHub? Sign in to your account

Closed
erickt opened this issue Nov 21, 2019 · 7 comments · Fixed by #67
Closed

Is the TUF target path separator always "/"? #63

erickt opened this issue Nov 21, 2019 · 7 comments · Fixed by #67

Comments

@erickt
Copy link
Contributor

erickt commented Nov 21, 2019

I noticed that in 4.4.5 spec defines as TARGETPATH:

Each key of the TARGETS object is a TARGETPATH. A TARGETPATH is a path to a file that is relative to a mirror's base URL of targets. It should not have a leading path separator to avoid surprising behavior when constructing paths on disk.

Presumably this should be /, it says it's a file relative to the URL, and URLs use that as the path separator, but it'd be helpful if this was explicit.

This is relevant because in rust-tuf, we have a PathTranslator in order to use alternative path separators to support converting / to \ on windows, but I'm pretty sure this logic really should be moved to our FileSystemRepository.

@lukpueh
Copy link
Member

lukpueh commented Nov 21, 2019

Thanks for submitting this issue, @erickt. AFAIK TARGETPATHs are always unix-style paths with forward-slash path separators. And I can confirm that the reference implementation converts \\ to / when adding target paths to tuf metadata (in the repository tool). Do you want to submit a PR to clarify this in the spec?

@trishankatdatadog
Copy link
Member

This really shouldn't be mandatory. / and \\ should both be allowed.

@erickt
Copy link
Contributor Author

erickt commented Nov 21, 2019

@trishankatdatadog Just to be safe, are you suggesting the targets role metadata should allow for both / and \\ path separators? Or that client libraries should internally map \\ to / before trying to fetch a target, but the metadata should use / as the path separator?

If we support \\ as separators in the metadata, then we could run into some confusing errors if a windows client tries to download a target foo\\bar, but to actually fetch the target, we can't just naively concatenate the mirror URL with the target path.

@trishankatdatadog
Copy link
Member

My opinion is that the specification should be tolerant enough to accommodate both UNIX and Windows file separators. (You know never know what you're going to find out there.) It is up to the implementation to correctly handle file separators across platforms. WDYT?

@lukpueh
Copy link
Member

lukpueh commented Nov 22, 2019

I think you're right, @trishankatdatadog, it may not be necessary to make this mandatory. But it's probably still a good idea for an implementation to normalize paths to unix style for the reason that @erickt has mentioned, and also to behave unsurprisingly when expanding unix shell-style wildcards as the we do in delegation path patterns (see spec#L877-L878).

The metadata examples exclusively use forward-slashes. Is it worth adding a recommendation?

@trishankatdatadog
Copy link
Member

trishankatdatadog commented Nov 22, 2019

@lukpueh Yes and yes, please

@erickt
Copy link
Contributor Author

erickt commented Nov 22, 2019

I personally vote for us standardizing on the target path to be the URL spec's path relative url string, since presumably all OSs should have some mechanism for resolving a file:// path.

Otherwise, if we allow different path separators, then we run into ambiguity with a target path foo\bar. If we try to naively open that on linux, it will try to open a file named "foo\bar". On Windows though, it'd open the file "bar" in the directory "foo". Things would get even more confusing if someone tries to store a target path using some of the other Windows path formats, like "\\.\UNC\LOCALHOST\c$\foo\bar".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants