Skip to content
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

ngclient: consistent targets handled differently than legacy updater #1576

Closed
jku opened this issue Sep 10, 2021 · 7 comments · Fixed by #1591
Closed

ngclient: consistent targets handled differently than legacy updater #1576

jku opened this issue Sep 10, 2021 · 7 comments · Fixed by #1591
Assignees
Labels
backlog Issues to address with priority for current development goals bug ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Sep 10, 2021

The definition of consistent targets in the spec is quite bad (as we're working with a URL but the spec insists on talking about filenames) so I'm not really sure what it wants:

Spec on consistent targets in repository:

consistent target files should be written to non-volatile storage as digest.filename.ext

Spec on consistent targets when client builds the download URL:

the filename is of the form HASH.FILENAME.EXT

Issue

  • legacy updater has decided this means a target path a/b will translate to a download url path a/{HASH}.b
  • ngclient translates target path a/b to a download url path {HASH}.a/b

So there's a couple of hings here:

  • first we probably should not just change how we do this translation <--- this issue is about fixing this
  • second, this translation means our target paths have a undocumented requirement that URLs in general do not: the URL must contain a filename part -- or this translation needs to define what happens with a missing filename part

CC @MVrachev

@joshuagl
Copy link
Member

The definition of consistent targets in the spec is quite bad (as we're working with a URL but the spec insists on talking about filenames) so I'm not really sure what it wants:

Spec on consistent targets in repository:

consistent target files should be written to non-volatile storage as digest.filename.ext

Spec on consistent targets when client builds the download URL:

the filename is of the form HASH.FILENAME.EXT

Could you file an issue against the specification?

Issue

  • legacy updater has decided this means a target path a/b will translate to a download url path a/{HASH}.b
  • ngclient translates target path a/b to a download url path {HASH}.a/b

Please let's fix that to do translation the same as legacy client.

  • second, this translation means our target paths have a undocumented requirement that URLs in general do not: the URL must contain a filename part -- or this translation needs to define what happens with a missing filename part

What I think you're saying here is that https://example.com/foo is a valid URL and therefore seemingly a valid TARGETPATH, but that does not include a filename part?

The specification has a lot of file-system assumptions baked in and we could be clearer here, all we specifically ask for in TARGETPATH definition is "path to a file". Could you file this as an issue against the specification too?

@jku
Copy link
Member Author

jku commented Sep 10, 2021

What I think you're saying here is that https://example.com/foo is a valid URL and therefore seemingly a valid TARGETPATH, but that does not include a filename part?

Yeah I think we talk about the same thing: If the base target URL is https://example.com/ then all of these are urls that could result into a perfectly valid target paths if we want them to:

https://example.com/a/ -> target path "a/"
https://example.com/?a=b -> target path "?a=b"
https://example.com/#a -> target path "#a"

In all of these cases the filename is I guess an empty string which we could of course still prefix with {HASH}....

Alternatively we could also say that we expect the URLs to always contain a non-empty "filename": maybe not an unreasonable request but definitely something we should validate if we do expect it -- injecting and parsing the HASH from the middle of the URL is very tricky so unexpected data will break things.

@jku
Copy link
Member Author

jku commented Sep 10, 2021

The specification has a lot of file-system assumptions baked in and we could be clearer here, all we specifically ask for in TARGETPATH definition is "path to a file". Could you file this as an issue against the specification too?

I'm not even 100% sure the file requirement makes sense in the context of target paths-- maybe it does does but we already know that many valid URLs don't cleanly map to local file paths so realistically a good client implementation needs to be prepared to handle almost any URL anyway, and that might not be any more difficult than handling "URLs that include a filename like part"

@jku
Copy link
Member Author

jku commented Sep 10, 2021

Specification issue theupdateframework/specification#183

@jku
Copy link
Member Author

jku commented Sep 11, 2021

If the base target URL is https://example.com/ then all of these are urls that could result into a perfectly valid target paths if we want them to:

https://example.com/a/ -> target path "a/"
https://example.com/?a=b -> target path "?a=b"
https://example.com/#a -> target path "#a"

Actually I'm wrong about the last two, they are not valid target paths since its defined to be a path-relative-URL string so can contain only path segments.

@MVrachev MVrachev added the backlog Issues to address with priority for current development goals label Sep 15, 2021
@sechkova sechkova added this to the Sprint 8 milestone Sep 15, 2021
@MVrachev
Copy link
Collaborator

If the base target URL is https://example.com/ then all of these are urls that could result into a perfectly valid target paths if we want them to:

https://example.com/a/ -> target path "a/"

I am not sure why this example is considered a valid target path?
The spec about TARGETPATH defines it as:
A string giving the path to a file that is relative to a mirror’s base URL of targets. and in this example, the TARGETPATH is pointing to a directory.

@jku
Copy link
Member Author

jku commented Sep 24, 2021

"file" and "directory" are definitions that don't exist in URLs as far as I know so that definition is a bit suspect. Anyway, I'm not trying to argue about what is a valid target path (maybe we could start being more strict about it... in another issue), just pointing out some complications you might want to test for when doing the change at hand with the target path handling we have.

EDIT: or in other words: the legacy updater will handle an "empty filename" just fine AFAICT so I think we should too. Then we can also discuss what are valid target paths

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals bug ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants