Fix 736: Rebuild roledb in refresh() even if root unchanged #737

awwad · 2018-05-30T20:29:49Z

In order to avoid freezes if role info is deleted due to prior
validation failures, make sure that roledb is rebuilt during
client tuf.client.updater.refresh(), even if root did not need
to be updated.

WIP: I may want to add some tests and wrap a line.

See Issue 736 for more details.

Fixes issue #736

Please verify and check that the pull request fulfills the following
requirements:

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

In order to avoid freezes if role info is deleted due to prior validation failures, make sure that roledb is rebuilt during client tuf.client.updater.refresh(), even if root did not need to be updated. See Issue 736 for more details. Signed-off-by: Sebastien Awwad <[email protected]>

vladimir-v-diaz · 2018-05-30T20:44:42Z

The relocation of the call to _rebuild_key_and_role_db() looks good to me. Another thing that we should do is document, in the relevant docstrings, the assumption that the client has the latest info for the top-level roles before attempting to update any specific metadata.

Note: Travis failed, so we probably need to update 1 or 2 test conditions.

vladimir-v-diaz · 2018-06-08T12:26:32Z

@awwad I'd like to include this important fix in our next scheduled release, which is approaching soon.

Is it okay if I resolve the Travis/Appveyor failures and finish this pull request?

awwad · 2018-06-08T12:54:34Z

Sure

…

On Fri, Jun 8, 2018 at 08:26 Vladimir Diaz ***@***.***> wrote: @awwad <https://github.com/awwad> I'd like to include this important fix in our next scheduled release, which is approaching soon. Is it okay if I resolve the Travis/Appveyor failures and finish this pull request? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#737 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMpkOG2T3vVJrlomMS9THevGpPyalHc6ks5t6m15gaJpZM4UT9bT> .

Also document the assumption that the metadata store is the latest and exists in _update_metadata() Signed-off-by: Vladimir Diaz <[email protected]>

…adata in refresh() Signed-off-by: Vladimir Diaz <[email protected]>

Signed-off-by: Vladimir Diaz <[email protected]>

…ater.py Signed-off-by: Vladimir Diaz <[email protected]>

Signed-off-by: Vladimir Diaz <[email protected]>

vladimir-v-diaz · 2018-06-11T16:01:54Z

@awwad I went ahead and (1) added a test condition for the persistent freeze attack (2) wrapped the line in refresh() that sets consistent snapshot (3) revised a bunch of comments and docstring to document the new behavior and any assumptions (4) fixed the failing unit tests.

I think the pull request now just needs to re-sync with the develop branch.

Thanks for reporting the issue and submitting the initial fix!

vladimir-v-diaz · 2018-06-11T17:25:07Z

@trishankatdatadog
Heads up: This is an important change that addresses a persistent freeze attack. I plant to add it to the next stable release and v0.12.dev1 for testing.

awwad

LGTM but for a comment improvement and two questions.

awwad · 2018-06-11T17:13:25Z

tests/test_indefinite_freeze_attack.py

+    # expired Snapshot and the locally trusted one, which should now be out of
+    # date.  After the attack, attempt to re-issue a valid Snapshot to verify
+    # that the client is able to recover from the indefinite freeze attack via
+    # the snapshot metadata.


I think it will be a bit confusing to future readers to refer to this as an indefinite freeze attack here without context (since there is nothing that obviously represents an attack in the test below). Perhaps:

# Serve the client expired Snapshot. The client should reject the given, # expired Snapshot and the locally trusted one, which should now be out of # date. # After the attack, attempt to re-issue a valid Snapshot to verify that # the client is still able to update. (A bug previously caused snapshot # expiration or replay to result in an indefinite freeze.)

You could also point to the particular issue, #736, in the comment, though that's probably less important. If that looks good, I can add the commit.

I like the comment that you added, but we should also point to #736 so the reader can understand how the bug was caused.

I'd appreciate you adding the commit, if you are not busy.

awwad · 2018-06-11T17:27:35Z

tests/test_indefinite_freeze_attack.py

+        sorted(self.repository_updater.metadata['current']))
+
+
+


Test looks good.

awwad · 2018-06-11T17:29:02Z

tests/test_multiple_repositories_integration.py

-    self.assertEqual(sorted(['role1', 'root', 'snapshot', 'targets', 'timestamp']),
+    self.assertEqual(sorted(['root', 'snapshot', 'targets', 'timestamp']),
        sorted(tuf.roledb.get_rolenames('test_repository1')))
-    self.assertEqual(sorted(['role1', 'root', 'snapshot', 'targets', 'timestamp']),


I'm confused by this change. How did this result from the PR?

Edit: ah, probably from the targets_of_role change...?

refresh() rebuilds the roledb and only refreshes top-level metadata. See line 212.

awwad · 2018-06-11T17:32:27Z

tuf/client/updater.py

    securesystemslib.formats.RELPATH_SCHEMA.check_match(rolename)

+    self._refresh_targets_metadata(refresh_all_delegated_roles=True)
+


Why is refresh_all_delegated_roles necessary? This could be quite a few roles.

Next point is a bit complex:
targets_of_role remains a strange function to me if called on anything other than the Targets role itself. If the indicated role is a delegated role, it just isn't clear what it means to obtain trustworthy target info for whatever files are listed by a trustworthy version of that role. (Whose word are we taking on the latter?)
Perhaps the best we can do is to: obtain some not-validated or partially-validated version of the delegated role (because delegation pathways depend on file paths and we haven't specified one), get the list of targets specified by that not-validated role file, and then properly obtain trustworthy target info for those file paths (whether or not they are ultimately provided by the same role?).

This more complex point plays into the correct way to run targets_of_role on a delegated role.

My thinking is that all_targets() and targets_of_roles() should be for clients that do not know ahead of time which targets are available on the repo, or provided by a particular role.

Community repos will certainly use get_one_valid_targetinfo(), but we might want to provide all_targets() and targets_of_roles() for simpler use cases / adopters.

After an offline discussion, @awwad and I came to the conclusion that it's better to not support all_targets() and targets_of_role(). We'll open a separate issue to remove these two functions.

trishankatdatadog · 2018-06-11T18:02:05Z

Does this break backwards-compatibility?

vladimir-v-diaz · 2018-06-11T18:18:40Z

Does this break backwards-compatibility?

I don't think this pull request breaks backwards-compatibility. @awwad Can you confirm?

The change makes sure that the client has the latest metadata loaded into memory after a updater.refresh(), even if one of the top-level metadata hasn't been updated by the repository.

awwad · 2018-06-11T20:09:20Z

No, I can't think of how it would break backward compatibility. (If it causes a problem, of course, LMK.)

BTW, @trishankatdatadog, do you use targets_of_role or all_targets?

to indicate the source of the freeze issue Signed-off-by: Sebastien Awwad <[email protected]>

trishankatdatadog · 2018-06-11T20:37:32Z

@awwad No, we use updated_targets() https://github.com/DataDog/pip/blob/4b191d586172bc7dee665599f819ef05b45b40f1/src/pip/_internal/download.py#L136-L154

awwad · 2018-06-11T20:55:45Z

(And from there, it looks like you get target info via get_one_valid_targetinfo)

Okay, good. (:

trishankatdatadog · 2018-06-11T20:57:37Z

Thanks for checking and verifying, @awwad, much appreciated 🙇

trishankatdatadog · 2018-06-13T18:10:03Z

Hi, any idea when v0.12.dev1 will be released on PyPI for testing? Thanks!

vladimir-v-diaz · 2018-06-13T18:11:12Z

I can do it sometime today, will keep you posted here.

vladimir-v-diaz · 2018-06-13T21:15:36Z

@trishankatdatadog
update: v0.12.dev1 was released to PyPI for testing.

trishankatdatadog · 2018-06-13T21:54:23Z

I'll test it, thanks!

vladimir-v-diaz added 7 commits June 8, 2018 10:20

Discuss the special case where metadata is marked obselete and deleted

47dbdba

Also document the assumption that the metadata store is the latest and exists in _update_metadata() Signed-off-by: Vladimir Diaz <[email protected]>

Make slight revision to comment about always refreshing top-level met…

18a5aa4

…adata in refresh() Signed-off-by: Vladimir Diaz <[email protected]>

Document new refresh() change in its docstring

43efa42

Signed-off-by: Vladimir Diaz <[email protected]>

Fix test_multiple_repositories_integration and edit whitespace in upd…

e92680f

…ater.py Signed-off-by: Vladimir Diaz <[email protected]>

Refresh delegated metadata to fix test_mix_and_match_attack.py failure

9b6c91a

Signed-off-by: Vladimir Diaz <[email protected]>

Wrap the refresh() line that sets consistent snapshot

f493161

Signed-off-by: Vladimir Diaz <[email protected]>

Add test condition for the persistent freeze attack

621ec3a

Signed-off-by: Vladimir Diaz <[email protected]>

Merge branch 'develop' into fix_736_freeze

6373c26

awwad commented Jun 11, 2018

View reviewed changes

DOC: Clarify comment on freeze attack test

e9cd01e

to indicate the source of the freeze issue Signed-off-by: Sebastien Awwad <[email protected]>

vladimir-v-diaz merged commit 7403777 into develop Jun 12, 2018

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

Add deprecation warning for all_targets() and targets_of_role() #744

Merged

3 tasks

vladimir-v-diaz deleted the fix_736_freeze branch June 13, 2018 21:22

awwad changed the title ~~WIP: Fix 736: Rebuild roledb in refresh() even if root unchanged~~ Fix 736: Rebuild roledb in refresh() even if root unchanged Jun 20, 2018

		securesystemslib.formats.RELPATH_SCHEMA.check_match(rolename)

		self._refresh_targets_metadata(refresh_all_delegated_roles=True)

Fix 736: Rebuild roledb in refresh() even if root unchanged #737

Fix 736: Rebuild roledb in refresh() even if root unchanged #737

Uh oh!

Conversation

awwad commented May 30, 2018

Uh oh!

vladimir-v-diaz commented May 30, 2018

Uh oh!

vladimir-v-diaz commented Jun 8, 2018

Uh oh!

awwad commented Jun 8, 2018 via email

Uh oh!

vladimir-v-diaz commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladimir-v-diaz commented Jun 11, 2018

Uh oh!

awwad left a comment

Choose a reason for hiding this comment

Uh oh!

awwad Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

awwad Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

awwad Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

awwad Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

vladimir-v-diaz Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

trishankatdatadog commented Jun 11, 2018

Uh oh!

vladimir-v-diaz commented Jun 11, 2018

Uh oh!

awwad commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trishankatdatadog commented Jun 11, 2018

Uh oh!

awwad commented Jun 11, 2018

Uh oh!

trishankatdatadog commented Jun 11, 2018

Uh oh!

trishankatdatadog commented Jun 13, 2018

Uh oh!

vladimir-v-diaz commented Jun 13, 2018

Uh oh!

vladimir-v-diaz commented Jun 13, 2018

Uh oh!

trishankatdatadog commented Jun 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vladimir-v-diaz commented Jun 11, 2018 •

edited

Loading

awwad Jun 11, 2018 •

edited

Loading

awwad Jun 11, 2018 •

edited

Loading

awwad commented Jun 11, 2018 •

edited

Loading