Skip to content

Conflicting file path assertions in delegate() and add_paths() #963

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
lukpueh opened this issue Nov 29, 2019 · 5 comments
Closed

Conflicting file path assertions in delegate() and add_paths() #963

lukpueh opened this issue Nov 29, 2019 · 5 comments

Comments

@lukpueh
Copy link
Member

lukpueh commented Nov 29, 2019

Description of issue or feature request:

Related to #957

The Targets.delegate()method fails if one of the file paths or patterns passed via the paths argument is absolute, i.e. starts with a directory separator. At the same time it warns if it the path does not have self._targets_directory as prefix, which very well may be an absolute path.

https://github.com/theupdateframework/tuf/blob/409eef1a4872a88b610e81b375f36b07a8e0f996/tuf/repository_tool.py#L2286-L2294

The Targets.add_paths() methods emits a similar message (with debug level). Note that the method does not check whether the passed paths have a leading directory separator, although it should, in order to be consistent with above function.

https://github.com/theupdateframework/tuf/blob/409eef1a4872a88b610e81b375f36b07a8e0f996/tuf/repository_tool.py#L1868-L1872

Current behavior:

  • Targets.delegate() and Targets.add_paths() emit warning/debug message if a passed path or path pattern is not prefixed with the targets directory base path, i.e. self._targets_directory.
  • Targets.add_paths does not check if the passed paths are indeed relative.

Expected behavior:

Either,

  • Targets.delegate() and Targets.add_paths() should not care about the prefix of the passed paths or path patterns.
  • Targets.add_paths should probably assert that the passed paths are relative, to be consistent with the sibling the corresponding functionality of delegate().
lukpueh added a commit that referenced this issue Dec 2, 2019
- Ask the reader to ignore a misleading warning about the location of
  a delegation path pattern.
  The comment may be removed when fixing the warning in
  #963.

- Comment out text that has become obsolete when commenting out
  the "Revoke Delegated Role" section (in an earlier commit).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit that referenced this issue Dec 2, 2019
- Ask the reader to ignore a misleading warning about the location of
  a delegation path pattern.
  The comment may be removed when fixing the warning in
  #963.

- Comment out text that has become obsolete when commenting out
  the "Revoke Delegated Role" section (in an earlier commit).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Dec 2, 2019

When fixing this issue, don't forget to remove the comment in a tutorial code snippet that asks the reader to ignore a misleading warning about the location of a delegation path pattern (see 41d7bbd of #775).

lukpueh added a commit that referenced this issue Dec 16, 2019
- Ask the reader to ignore a misleading warning about the location of
  a delegation path pattern.
  The comment may be removed when fixing the warning in
  #963.

- Comment out text that has become obsolete when commenting out
  the "Revoke Delegated Role" section (in an earlier commit).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@joshuagl
Copy link
Member

joshuagl commented Mar 4, 2020

The specification states:

A TARGETPATH is a path to a file that is relative to a mirror's base URL of targets. To avoid surprising behavior when resolving paths, it is RECOMMENDED that a TARGETPATH uses the forward slash (/) as directory separator and does not start with a directory separator. The recommendation for TARGETPATH aligns with the "path-relative-URL string" definition in the WHATWG URL specification.

Therefore I think the latter expected behaviour is most consistent with the spec, i.e. we should change such that:

Targets.add_paths should probably assert that the passed paths are relative, to be consistent with the sibling the corresponding functionality of delegate().

@lukpueh
Copy link
Member Author

lukpueh commented Mar 6, 2020

To make sure we are on the same page, we are not talking about TARGETPATH but PATHPATTERN here. But the specification states something similar:

To avoid surprising behavior when matching targets with PATHPATTERN, it is
RECOMMENDED that PATHPATTERN uses the forward slash (/) as directory
separator and does not start with a directory separator, akin to
TARGETSPATH.

Also, just to double check, do others think raising an exception is reasonable? The spec only recommends relative paths, but IIRC we agreed (theupdateframework/specification#63, theupdateframework/specification#67) that an implementation can be more assertive than the spec in this regard. So I think leaving the strict behavior in delegate and adding it to add_paths is indeed reasonable.

@lukpueh
Copy link
Member Author

lukpueh commented Mar 6, 2020

I also just saw that I didn't suggest an expected behavior for the other issue, i.e. the warning/debug messages on not path.startswith(targets_directory).

I strongly suggest to remove them, because if a PATHPATTERN does indeed start with a targets directory, I think, it won't even match TARGETPATHs, as they explicitly leftstrip the targets directory, see e.g. add_target:
https://github.com/theupdateframework/tuf/blob/409eef1a4872a88b610e81b375f36b07a8e0f996/tuf/repository_tool.py#L1962

@lukpueh
Copy link
Member Author

lukpueh commented Apr 8, 2020

Fixed in #1008

@joshuagl joshuagl closed this as completed Apr 9, 2020
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

No branches or pull requests

2 participants