WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

awwad · 2018-01-16T22:00:46Z

Removes an incorrect check that prevents delegating to a role if any role has previously delegated to that role. (Previously, if X delegated to A, then Y was prevented from delegating to A.)

Adds a few tests.

See #589 for more details. (Note that the central fix, a removal of a few lines, has been merged already in #638, and so has been removed from this commit history.)

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

vladimir-v-diaz · 2018-01-17T15:33:19Z

Can you please edit the corresponding test case?https://github.com/theupdateframework/tuf/blob/aa2ab218f22d8682e03c992ea98f88efd155cffd/tests/test_repository_tool.py#L1192-L1196

vladimir-v-diaz · 2018-01-17T15:38:15Z

tuf/repository_tool.py

@@ -2222,11 +2222,6 @@ def delegate(self, rolename, public_keys, paths, threshold=1,
    if path_hash_prefixes is not None:
      securesystemslib.formats.PATH_HASH_PREFIXES_SCHEMA.check_match(path_hash_prefixes)

-    # Check if 'rolename' is not already a delegation.


It might be good to mention in the docstring, or in a comment, that 2+ roles can delegate to a single role.
https://github.com/awwad/tuf/blob/2e590485e540b822117de76d1ec949731720ba6f/tuf/repository_tool.py#L2134-L2144

Actually, this doesn't feel natural -- it might just be confusing to explain. There's no reason to suspect that 2+ roles couldn't delegate to a single role in the code, docstring, or comments. I'll take a look at the documentation - maybe there's somewhere to note that there.

Edit: Adding it to docs along with a note about cycles, in tuf/README.md in the section on delegations.

awwad · 2018-01-17T15:55:22Z

Will do

awwad · 2018-01-23T15:41:06Z

Rebased to remove conflicts tidily.

awwad · 2018-01-23T15:42:31Z

Fixed the test case to instead expect success, but we have to add some more detailed testing that covers these delegation scenarios, and until we do, this is still WIP. (In particular, test_updater should have a few tests in which multiple delegations delegate to the same delegation, and the right information is retrieved.)

awwad · 2018-01-23T16:13:06Z

Some tests to add before this PR is OK:

Scenario 1:

Targets delegates "*" to A, which delegates "a*" to C
Targets delegates "*" to B, which delegates "b*" to C
All roles are signed by all keys expected of them. (e.g. C is signed by a threshold of all keys that A and B expect of it for the delegations listed)
C lists targets a1.zip and b1.zip.

Tests needed:

all delegations above must succeed
get_one_valid_targetinfo(b1.zip) should successfully fetch the fileinfo C listed for b1.zip
get_one_valid_targetinfo(a1.zip) should successfully fetch the fileinfo C listed for a1.zip

Scenario 2:

Same as Scenario 1, except that role C is NOT signed by the keys expected of it by role A (but still is signed by the keys expected of it by role B). (This is important to avoid a particular "attack" whose explanation I'm sitting on and will get to in a fresh issue later.)

Tests needed:

get_one_valid_targetinfo(b1.zip) should successfully fetch the fileinfo C listed for b1.zip
get_one_valid_targetinfo(a1.zip) should of course fail to fetch the fileinfo C listed for a1.zip

I may add some tests to this when I go back over my notes later.

awwad · 2018-03-13T20:58:32Z

Will rebase and conflict resolve this due to merging of #638, which performs part of what this pull request was intended to do. This PR will then add the tests I recommended.

Add a note to the delegation creation sections of docs/CLI.md and docs/TUTORIAL.md that makes clear that two roles A and B can delegate to the same role C without a problem - that the delegation graph need not be a tree. Also corrects a minor typo in TUTORIAL.md Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

A and B both delegating to C works and is OK. We should probably add more testing for this, covering more complex situations. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2018-03-14T17:59:17Z

Rebased. The check I removed here, which you also removed in the later-merged #638, is no longer in the commit history here. I've changed the docs changes as well: while the changes were previously in tuf/README.md, that has been split into docs/CLI.md and docs/TUTORIAL.md, and so the same notes as were added before to tuf/README.md have now been added to those two documents. @vladimir-v-diaz, you may want to take a peek at the changed docs/CLI.md and make sure the extra note isn't too disruptive.

vladimir-v-diaz · 2018-03-14T18:07:20Z

I don't find the changes to CLI.md too disruptive.

Adds a new test method to test_updater, test_6_get_one_valid_targetinfo__promiscuous() (The nomenclature in the test files is a bit constraining.) The new test tests updater.get_one_valid_targetinfo() in two scenarios where multiple roles delegate to the same role (which should be fine). Delegations are performed, and the updater attempts to gather target info for files delegated down the two delegation pathways to the same final role. WIP Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

tuf.SignatureError is now securesystemslib.exceptions.BadSignatureError. This commit just corrects a docstring that refers to the old error name instead. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

A now-merged PR disallows leading '/' in repository paths (whether in a target info request, target addition, path delegation, etc., I hope). This commit corrects the test being added in this PR to fit those new expectations. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

vladimir-v-diaz · 2018-06-13T15:32:05Z

@awwad
I want to validate the correctness of the API redesign against this pull request, if it makes sense.

Has the task/test in scenario 2 (#590 (comment)) been implemented in this pull request? I see that Travis is failing, but that is expected, correct?

awwad · 2018-06-13T15:39:49Z

The checks in the comment capture the current state -- the last two tests in the list still need to be added. There are more tests I'd want for the API redesign, I think, though I'd have to make sure I understand what you mean by that: is the redesign the fix for #660? (For example, is the expectation that the redesign will provide assurance that validation of a delegated role always consults the appropriate delegating role to determine the keys to expect?)

vladimir-v-diaz · 2018-06-13T15:47:26Z

is the redesign the fix for #660? (For example, is the expectation that the redesign will result in validation of a delegated role to always consult the appropriate delegating role to determine the keys to expect?)

That's correct! The redesign of the API is outlined in #574 (comment) and being worked on in https://github.com/vladimir-v-diaz/tuf/tree/refactor_low_level_api.

vladimir-v-diaz · 2018-06-26T19:28:33Z

@awwad Is it okay if I finish the last two tests in #590 (comment)? I'd like to use this pull request to test #757.

I can let you know when pull request #757 is ready for a review.

awwad · 2018-06-27T18:55:35Z

Certainly

joshuagl · 2020-09-10T11:17:46Z

Due to imminent refactor efforts this code is unlikely to merge.

awwad force-pushed the allow_multiple_delegations_to_same_role branch 2 times, most recently from 5dc96ac to 2e59048 Compare January 16, 2018 22:02

vladimir-v-diaz reviewed Jan 17, 2018

View reviewed changes

awwad force-pushed the allow_multiple_delegations_to_same_role branch from 5b035bf to 628b938 Compare January 23, 2018 15:40

vladimir-v-diaz mentioned this pull request Mar 5, 2018

CLI --sign: Support signing of delegated metadata and ability to import any public key type #638

Merged

3 tasks

awwad added 2 commits March 14, 2018 13:55

Fix repository_tool test to expect promiscuous delegation to work

e5ce022

A and B both delegating to C works and is OK. We should probably add more testing for this, covering more complex situations. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad force-pushed the allow_multiple_delegations_to_same_role branch from 628b938 to e5ce022 Compare March 14, 2018 17:56

awwad force-pushed the allow_multiple_delegations_to_same_role branch from ecc9810 to fecb3aa Compare April 13, 2018 20:34

awwad added 2 commits April 13, 2018 16:35

DOC: updater docstring correction: old error name switched

294d9d4

tuf.SignatureError is now securesystemslib.exceptions.BadSignatureError. This commit just corrects a docstring that refers to the old error name instead. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

vladimir-v-diaz mentioned this pull request Jun 25, 2018

Use the keyids and threshold set by delegating roles to verify delegated metadata #757

Closed

3 tasks

joshuagl closed this Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

awwad commented Jan 16, 2018 •

edited

Loading

vladimir-v-diaz commented Jan 17, 2018

vladimir-v-diaz Jan 17, 2018

awwad Jan 23, 2018 •

edited

Loading

awwad commented Jan 17, 2018

awwad commented Jan 23, 2018

awwad commented Jan 23, 2018 •

edited

Loading

awwad commented Jan 23, 2018 •

edited

Loading

awwad commented Mar 13, 2018

awwad commented Mar 14, 2018 •

edited

Loading

vladimir-v-diaz commented Mar 14, 2018

vladimir-v-diaz commented Jun 13, 2018

awwad commented Jun 13, 2018 •

edited

Loading

vladimir-v-diaz commented Jun 13, 2018

vladimir-v-diaz commented Jun 26, 2018

awwad commented Jun 27, 2018

joshuagl commented Sep 10, 2020

WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

Conversation

awwad commented Jan 16, 2018 • edited Loading

vladimir-v-diaz commented Jan 17, 2018

vladimir-v-diaz Jan 17, 2018

Choose a reason for hiding this comment

awwad Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

awwad commented Jan 17, 2018

awwad commented Jan 23, 2018

awwad commented Jan 23, 2018 • edited Loading

awwad commented Jan 23, 2018 • edited Loading

Scenario 1:

Scenario 2:

awwad commented Mar 13, 2018

awwad commented Mar 14, 2018 • edited Loading

vladimir-v-diaz commented Mar 14, 2018

vladimir-v-diaz commented Jun 13, 2018

awwad commented Jun 13, 2018 • edited Loading

vladimir-v-diaz commented Jun 13, 2018

vladimir-v-diaz commented Jun 26, 2018

awwad commented Jun 27, 2018

joshuagl commented Sep 10, 2020

awwad commented Jan 16, 2018 •

edited

Loading

awwad Jan 23, 2018 •

edited

Loading

awwad commented Jan 23, 2018 •

edited

Loading

awwad commented Jan 23, 2018 •

edited

Loading

awwad commented Mar 14, 2018 •

edited

Loading

awwad commented Jun 13, 2018 •

edited

Loading