Clarify client traversal of role delegation graph

[ethan-lowman-dd]

Spec v1.0.19 Section 5.6.7 describes how the client should traverse the delegation graph to update the targets role. The wording on cycle avoidance could use some clarification.

The spec says:

If this role has been visited before, then skip this role (so that cycles in the delegation graph are avoided)

A "role" in this context could either refer to 1) a delegated role (a node in the delegation graph) or 2) a role entry in the roles array of the DELEGATIONS object, which represents not a role, but a delegation from one role to another (a directed edge in the delegation graph).

As a result, there are two ways the traversal could be interpreted:

1. As we're traversing the delegation graph, skip nodes (roles) that have already been visited. This avoids visiting any one role twice, and produces the most intuitive pre-order traversals for graphs where some nodes have multiple parents (i.e. when the graph is not a tree). However, it's not true cycle detection and we may actually want to consider every edge, since different edges leading to the same role may represent different conditions of delegation (e.g. different keys, different paths, etc.).
2. As we're traversing the delegation graph, skip edges (delegations) that have already been visited. This follows the typical definition of cycle detection. However, it produces counter-intuitive "pre-order" traversals for graphs that aren't trees, and this interpretation is not obvious from the wording in section 5.6.7.

Below are two concrete examples.

Example A:

Interpretation	Traversal
1: skip visited nodes	A, B, D, C
2: skip visited edges	A, B, D, C, D

---

Example B: (assume B's outgoing edges/delegations are ordered as [A, D]

Interpretation	Traversal
1: skip visited nodes	A, B, D, C
2: skip visited edges	A, B, A, C, D

cc: @trishankatdatadog

[trishankatdatadog]

I think in the more general case (especially multi-role delegations), you want to avoid delegations you have seen before.

So, in the typical single-role delegation case, that means you want to avoid roles you have seen before.

Even in the multi-role delegation case, you want to avoid the exact combination of roles (i.e., AND of thresholds of public keys) you have seen before, and even that can broken down into single roles (i.e., if you have seen a rolename before, even if it is being delegated a different threshold of different public keys now, you know can you avoid it because you clearly haven't found what you were looking for).

Did I miss anything @mnm678?

[mnm678]

This is a good catch, and certainly something that we should clarify.

I agree with @trishankatdatadog. We want to avoid visiting the same node multiple times so that roles don't accidentally get added multiple times to a threshold, etc.

While TUF does not actually require delegations to be a tree, in most cases they will be. Further, delegators are aware of TUF semantics, and can, for example, create a new role with some similar keys if for some reason this role needs to exist at multiple levels of the graph.

[joshuagl]

The legacy client in the reference implementation keeps a set() of visited role names and skips any role name (node) that it has visited before. See _preorder_depth_first_walk() in tuf/client/updater.py.

[raphaelgavache]

On the new ngclient _preorder_depth_first_walk keeps a set of edges: (node, parent_node) any skips visited edges. Do you know why it was implemented differently?

[trishankatdatadog]

(i.e., if you have seen a rolename before, even if it is being delegated a different threshold of different public keys now, you know can you avoid it because you clearly haven't found what you were looking for)

I should clarify: my statement is true for some role X that you have visited alone (i.e., a single-role delegation) before seeing it later as part of a multi-role delegation. OTOH, it is not true if you visited X as part of some multi-role delegation earlier, before encountering it again later in some new multi-role delegation...

[joshuagl]

Thank you for the detailed issue @raphaelgavache!

Apologies all for the noise of the drive-by comment earlier, I wanted to some implementation examples and submitted before the comment was complete.

On the new ngclient _preorder_depth_first_walk keeps a set of edges: (node, parent_node) any skips visited edges. Do you know why it was implemented differently?

I believe a new set of contributors drew different conclusions from the specification, further indicating the need to clarify this part of the detailed client workflow.

I'd certainly defer to @trishankatdatadog and @mnm678 here, but I my current understanding is in agreement with the ngclient authors conclusion (and yours, I believe, @raphaelgavache) that we should skip visited delegations/edges not visited roles/nodes.

[jku]

I'd certainly defer to @trishankatdatadog and @mnm678 here, but I my current understanding is in agreement with the ngclient authors conclusion (and yours, I believe, @raphaelgavache) that we should skip visited delegations/edges not visited roles/nodes.

I can see the different potential approaches but I'm considering reverting this decision in ngclient:

• the current spec language still seems pretty clear "If this role has been visited before, then skip this role"
• I'd rather we do the same thing that the old client does unless we have a good reason

So I think I'll propose changing that (thanks raphael for pointing that out). If anyone has opinions, please -> https://github.com/theupdateframework/tuf/issues/1528