Skip to content

ngclient: skipping visited roles #1528

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
jku opened this issue Aug 22, 2021 · 5 comments · Fixed by #1683
Closed

ngclient: skipping visited roles #1528

jku opened this issue Aug 22, 2021 · 5 comments · Fixed by #1683
Assignees
Labels
backlog Issues to address with priority for current development goals good first issue Bite-sized items for first time contributors ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Aug 22, 2021

spec says (for each role in delegation tree):

If this role has been visited before, then skip this role

ngclient does:

if (role_name, parent_role) in visited_role_names:
    logger.debug("Skipping visited current role %s", role_name)
    continue

So it only skips if role has been visited from same parent. If there is a good reason for allowing the same role multiple times in the delegation tree (against the spec), it should be documented somehow. Alternatively we should do what the spec says and make visited_role_names just a set of role names (and not a set of tuple)...

Pinging @sechkova for comment: there's probably a reason ngclient does this (considering legacy client does not do this), happen to remember what it is?

@jku jku added the ngclient label Aug 22, 2021
@jku
Copy link
Member Author

jku commented Aug 23, 2021

I guess the reasons are related to this theupdateframework/specification#177

Still, IMO the spec seems clear on what the requirements are now: a role can only be in a specific delegation tree traversal once by specification. So we should have at least comment explaining the reason for not doing that.

@sechkova
Copy link
Contributor

Pinging @sechkova for comment: there's probably a reason ngclient does this (considering legacy client does not do this), happen to remember what it is?

From what I remember, the only reason was a different reading of the spec. The author of theupdateframework/specification#177 has very well described the possible interpretations.
Given that skipping "role" is simpler and there seems to be an agreement that it is a correct way of implementing it, I am not against doing the change.

@sechkova sechkova added the backlog Issues to address with priority for current development goals label Sep 1, 2021
@sechkova sechkova added the good first issue Bite-sized items for first time contributors label Sep 29, 2021
@sechkova sechkova added this to the Sprint 9 milestone Sep 29, 2021
@avelichka
Copy link
Contributor

Could you please assign that to me ;)

@sechkova
Copy link
Contributor

sechkova commented Sep 29, 2021

Let's do as the old client does and skip a role (node on the delegations graph) without considering its parent.
As discussed in theupdateframework/specification#177 this is

  • the literal reading of the specification
  • the simpler and practical solution for single-role delegations and python-tuf does not support multi-role delegations at this point.

@ivanayov
Copy link
Collaborator

Can I take this?

ivanayov pushed a commit to ivanayov/python-tuf that referenced this issue Nov 18, 2021
This change edits the ngclient `Updater` to traverse the delegation
tree on nodes, instead of edges in order to skip already visited
nodes. For more detailed clarification, please review theupdateframework#177

Fixes theupdateframework#1528

Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
ivanayov pushed a commit to ivanayov/python-tuf that referenced this issue Nov 18, 2021
This change edits the ngclient `Updater` to traverse the delegation
tree on nodes, instead of edges in order to skip already visited
nodes.

For more detailed clarification, please review
theupdateframework/specification#177

Fixes theupdateframework#1528

Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
ivanayov pushed a commit to ivanayov/python-tuf that referenced this issue Nov 18, 2021
This change edits the ngclient `Updater` to traverse the delegation
tree on nodes, instead of edges in order to skip already visited
nodes.

For more detailed clarification, please review
theupdateframework/specification#177

Fixes theupdateframework#1528

Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
@jku jku closed this as completed in #1683 Nov 22, 2021
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 good first issue Bite-sized items for first time contributors ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants