Introduce the idea of trusted/untrusted snapshot #1605

MVrachev · 2021-10-05T15:22:04Z

Description of the changes being introduced by the pull request:

If you do the following steps with a repository that contains hashes in meta dicts:

call Updater.refresh() and load, verify and cache all metadata files
modify timestamp snapshot meta information:
(One or more of hashes or length for snapshot changes here)
call Updater.refresh() again
root and timestamp will be updated to their latest versions
local snapshot will be loaded, but hashes/length will be different
then the ones in timestamp.snapshot_meta and that will prevent loading
remote snapshot is loaded and verification starts

then when executing step 6 the rollback checks will not be done because
the old snapshot was not loaded on step 5.

In order to resolve this issue, we are introducing the idea of trusted and
untrusted snapshot.
Trusted snapshot is the locally available cached version. This version has
been verified at least once meaning hashes and length were already checked
against timestamp.snapshot_meta hashes and length.
That's why we can allow loading a trusted snapshot version even if there is a
mismatch between the current timestamp.snapshot_meta hashes/length and
hashes/length inside the trusted snapshot.
Untrusted snapshot is the one downloaded from the web. It hasn't been verified
before and that's why we mandate that timestamp.snapshot_meta hashes and length
should match the hashes and legth calculated on this untrusted version of
snapshot.

As the TrustedMetadataSet doesn't have information on which snapshot is trusted or
not, so possibly the best solution is to add a new argument "trusted"
to update_snapshot.
Even though this is ugly as the rest of the update functions doesn't
have such an argument, it seems the best solution as it seems to work
in all cases:

when loading a local snapshot, we know the data has at some point been
trusted (signatures have been checked): it doesn't need to match hashes
now
if there is no local snapshot and we're updating from remote, the
remote data must match meta hashes in timestamp
if there is a local snapshot and we're updating from remote, the remote
data must match meta hashes in timestamp

Lastly, I want to point out that hash checks for metadata files are not
essential to TUF security guarantees: they are just an additional layer of
security that allows us to avoid even parsing json that could be malicious -
we already know the malicious metadata would be stopped at metadata
verification after the parsing.

Signed-off-by: Martin Vrachev mvrachev@vmware.com

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

coveralls · 2021-10-05T15:47:32Z

Pull Request Test Coverage Report for Build 1336473453

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 98.36%

Totals
Change from base Build 1333131434:	0.001%
Covered Lines:	3718
Relevant Lines:	3746

💛 - Coveralls

jku

I think the commit/PR title is misleading (ans so are some of the comments): the snapshot rollback checks require local snapshot, and do not work without local snapshot. What you are doing is allowing the loading of local snapshot as intermediate snapshot even if the hashes do not match (because, like meta version, the meta hashes reference the new remote snapshot). We want to load the local snapshot even though we already know it's not valid as final because the local snapshot allows us to do rollback checks on the new remote snapshot.

What might be worth mentioning is that the hash checks for metadata files are not essential to TUF security guarantees: they are just an additional layer of security that allows us to avoid even parsing json that could be malicious (we already know the malicious metadata would be stopped at metadata verification after the parsing).

It's a complex situation and because of the complexity we absolutely must be able to clearly express what happens. If we are not able to do that then we should avoid implementing the metafile hashing altogether...

tests/test_updater_with_simulator.py

tuf/ngclient/_internal/trusted_metadata_set.py

tests/test_updater_with_simulator.py

MVrachev · 2021-10-08T08:57:52Z

@jku I tried to address all of your comments.
First and foremost I changed the commit, pr and test description.
I know they are lengthy but didn't found another way to describe the situation.
Can you give me some ideas on how to shorten it?

Second, I experimented with adding hashes and length calculations in the Repository simulator.
What do you think?

Finally, I updated the trusted description as requested, but again it's a little long...

jku

Are you sure this actually runs the hash computing code? Can you double check with --dump or debug prints or something?

the docstrings are indeed hard, I'll maybe try to suggest something when I'm more caffeinated... otherwise left two code suggestions.

tests/repository_simulator.py

tests/test_updater_with_simulator.py

tests/repository_simulator.py

MVrachev · 2021-10-08T13:57:50Z

Are you sure this actually runs the hash computing code? Can you double check with --dump or debug prints or something?

Ohh... I didn't run it because I was assigning self.compute_metafile_hashes_length to True
instead of self.sim.compute_metafile_hashes_length 🤦.
Great catch!

I think I addressed your comments.

the docstrings are indeed hard, I'll maybe try to suggest something when I'm more caffeinated... otherwise left two code suggestions.

Will wait for your other review.

jku

I would maybe move the compute_metafile_hashes_length = True to the start of the test: we should try to make tests that don't modify the repository in the middle of the test in unrealistic ways and here it seems we should be testing against a repository that includes hashes for all meta versions, not just a single snapshot update.

Can you make sure the client testing plan (document or issue) includes this new repository setting as a configuration we should keep in mind when improving tests (so that this doesn't remain the single test using this configuration)

I left some specific comments, the important ones are the new bug in RepositorySimulator and how the hash computation is still more complex than it needs to be

tests/repository_simulator.py

jku · 2021-10-11T12:17:37Z

As a review of my original RepositorySimulator design:

This use case shows a (sort of) disadvantage of signing on demand: since the meta hashes require signatures to be included in the hash, the snapshot update now requires signing every targets metadata (which in practice means that files will typically be signed twice per refresh(): once to create snapshot, once to serve the actual file). I think signing on demand is still probably a reasonable/correct approach: manually signing sounds like a lot of work we don't want to do in the tests

MVrachev · 2021-10-12T10:50:06Z

I would maybe move the compute_metafile_hashes_length = True to the start of the test: we should try to make tests that don't modify the repository in the middle of the test in unrealistic ways and here it seems we should be testing against a repository that includes hashes for all meta versions, not just a single snapshot update.

Agreed. Changed that.

Can you make sure the client testing plan (document or issue) includes this new repository setting as a configuration we should keep in mind when improving tests (so that this doesn't remain the single test using this configuration)

Yes, left a comment here: #1579 (comment)

I left some specific comments, the important ones are the new bug in RepositorySimulator and how the hash computation is still more complex than it needs to be

I think I addressed all of your comments.
IMO the only questionable item left are the comments.
Will appreciate a review from you as one who writes documentation in a more succinct way.

jku

I think this looks correct.

The case really is kind of a corner case of a corner case so very hard to explain clearly. @joshuagl does this look reasonable to you?

tuf/ngclient/_internal/trusted_metadata_set.py

MVrachev · 2021-10-12T11:38:10Z

🤷 My version is even longer but it does try to explain the purpose of the argument. Up to you.

Liked your version a little more.
Added it.
Additionally, I added a return annotation to _compute_hashes_and_length.

joshuagl

This looks reasonable to me, thank you both.

tests/repository_simulator.py

joshuagl · 2021-10-12T13:17:31Z

tuf/ngclient/updater.py

@@ -360,7 +360,7 @@ def _load_snapshot(self) -> None:
 version = snapshot_meta.version

 data = self._download_metadata("snapshot", length, version)
- self._trusted_set.update_snapshot(data)
+ self._trusted_set.update_snapshot(data, trusted=False)


Suggested change

self._trusted_set.update_snapshot(data, trusted=False)

self._trusted_set.update_snapshot(data)

Nit: we defined a default, should we just use it? Is being explicit here a style recommendation?

Even though we have a default value I prefer if we are explicit about this.
That way it's easier to distinguish when we consider the data as trusted and when we do not.
In this case, I see the default value more as a defense mechanism that trusted will be set.

Not requiring any change here. But if we want to force ourselves to be explicit about how we call this internal API, why have the default?

What do you think @jku? Should we have a default value for trusted?

I don't care very much, but defining a default in internal API and then explicitly avoiding using that default as defense mechanism seems weird. So I'd say use the default or don't define it in the first place... (but it's really not a required change)

I checked and if I decide to change the code to mandate trusted it will have too many other unrelated changes, so the tests could pass. It doesn't worth it.
I the argument from this function call as Joshua suggested.

tests/repository_simulator.py

If you do the following steps: 1. call Updater.refresh() and load, verify and cache all metadata files 2. modify timestamp snapshot meta information: (One or more of hashes or length for snapshot changes here) 3. call Updater.refresh() again 4. root and timestamp will be updated to their latest versions 5. local snapshot will be loaded, but hashes/length will be different than the ones in timestamp.snapshot_meta and that will prevent loading 6. remote snapshot is loaded and verification starts then when executing step 6 the rollback checks will not be done because the old snapshot was not loaded on step 5. In order to resolve this issue, we are introducing the idea of trusted and untrusted snapshot. Trusted snapshot is the locally available cached version. This version has been verified at least once meaning hashes and length were already checked against timestamp.snapshot_meta hashes and length. That's why we can allow loading a trusted snapshot version even if there is a mismatch between the current timestamp.snapshot_meta hashes/length and hashes/length inside the trusted snapshot. Untrusted snapshot is the one downloaded from the web. It hasn't been verified before and that's why we mandate that timestamp.snapshot_meta hashes and length should match the hashes and legth calculated on this untrusted version of snapshot. As the TrustedMetadataSet doesn't have information which snapshot is trusted or not, so possibly the best solution is to add a new argument "trusted" to update_snapshot. Even though this is ugly as the rest of the update functions doesn't have such an argument, it seems the best solution as it seems to work in all cases: - when loading a local snapshot, we know the data has at some point been trusted (signatures have been checked): it doesn't need to match hashes now - if there is no local snapshot and we're updating from remote, the remote data must match meta hashes in timestamp - if there is a local snapshot and we're updating from remote, the remote data must match meta hashes in timestamp Lastly, I want to point out that hash checks for metadata files are not essential to TUF security guarantees: they are just an additional layer of security that allows us to avoid even parsing json that could be malicious - we already know the malicious metadata would be stopped at metadata verification after the parsing. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Add an option to calculate the hashes and length for timestamp/snapshot meta. This will help to cover more use cases with the repository simulator. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Modify RepositorySimulator function delegates() to all_targets(), so that all targets can be traversed and updated with one cycle when calling update_snapshot() (which is the only use case for now for delegates()). Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev · 2021-10-13T07:46:05Z

The changes I made:

followed Joshua's advice and didn't pass trusted at all when calling it with downloaded data.
fixed the return type annotation error pointed out by Jussi.

MVrachev · 2021-10-13T10:32:53Z

I am puzzled why the windows checks failed.
Do we want to restart the run or merge it as it is?

jku · 2021-10-13T10:34:28Z

Update - Currently investigating inability to process Windows jobs. Linux and Mac jobs are not impacted.
Oct 13, 09:44 UTC

let's try again a little later

MVrachev requested a review from jku October 5, 2021 15:27

jku requested changes Oct 6, 2021

View reviewed changes

tests/test_updater_with_simulator.py Outdated Show resolved Hide resolved

tuf/ngclient/_internal/trusted_metadata_set.py Outdated Show resolved Hide resolved

tests/test_updater_with_simulator.py Outdated Show resolved Hide resolved

MVrachev force-pushed the snapshot-hashes-length-check branch 2 times, most recently from eb425f1 to fa6c0c5 Compare October 8, 2021 08:53

MVrachev changed the title ~~Trigger rollback check even without local snapshot~~ Introduce the idea of trusted/untrusted snapshot Oct 8, 2021

MVrachev requested a review from jku October 8, 2021 09:03

jku reviewed Oct 8, 2021

View reviewed changes

tests/repository_simulator.py Outdated Show resolved Hide resolved

tests/test_updater_with_simulator.py Outdated Show resolved Hide resolved

jku reviewed Oct 8, 2021

View reviewed changes

tests/repository_simulator.py Outdated Show resolved Hide resolved

MVrachev force-pushed the snapshot-hashes-length-check branch from fa6c0c5 to c7f1581 Compare October 8, 2021 13:55

MVrachev requested a review from jku October 8, 2021 13:57

jku requested changes Oct 11, 2021

View reviewed changes

tests/repository_simulator.py Outdated Show resolved Hide resolved

tests/repository_simulator.py Outdated Show resolved Hide resolved

tests/repository_simulator.py Outdated Show resolved Hide resolved

MVrachev mentioned this pull request Oct 12, 2021

Design test strategy #1579

Closed

MVrachev force-pushed the snapshot-hashes-length-check branch from c7f1581 to 578aba0 Compare October 12, 2021 10:42

MVrachev requested a review from jku October 12, 2021 10:50

jku approved these changes Oct 12, 2021

View reviewed changes

tuf/ngclient/_internal/trusted_metadata_set.py Outdated Show resolved Hide resolved

MVrachev force-pushed the snapshot-hashes-length-check branch from 578aba0 to 4e1d06a Compare October 12, 2021 11:36

joshuagl approved these changes Oct 12, 2021

View reviewed changes

MVrachev force-pushed the snapshot-hashes-length-check branch from 4e1d06a to 32b47c9 Compare October 12, 2021 15:05

MVrachev mentioned this pull request Oct 12, 2021

New implementation: do we need a separate DEFAULT_HASH_ALGORITHM config option in TUF? #1613

Closed

jku reviewed Oct 13, 2021

View reviewed changes

tests/repository_simulator.py Outdated Show resolved Hide resolved

MVrachev added 3 commits October 13, 2021 10:40

Repository_simulator: add a flag to compute hashes

f1f76d2

Add an option to calculate the hashes and length for timestamp/snapshot meta. This will help to cover more use cases with the repository simulator. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the snapshot-hashes-length-check branch from 32b47c9 to 717eef9 Compare October 13, 2021 07:44

jku approved these changes Oct 13, 2021

View reviewed changes

jku merged commit 4d8cbc7 into theupdateframework:develop Oct 13, 2021

MVrachev deleted the snapshot-hashes-length-check branch October 13, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the idea of trusted/untrusted snapshot #1605

Introduce the idea of trusted/untrusted snapshot #1605

MVrachev commented Oct 5, 2021 •

edited

Loading

coveralls commented Oct 5, 2021 •

edited

Loading

jku left a comment •

edited

Loading

MVrachev commented Oct 8, 2021

jku left a comment •

edited

Loading

MVrachev commented Oct 8, 2021 •

edited

Loading

jku left a comment •

edited

Loading

jku commented Oct 11, 2021 •

edited

Loading

MVrachev commented Oct 12, 2021

jku left a comment

MVrachev commented Oct 12, 2021

joshuagl left a comment

joshuagl Oct 12, 2021

MVrachev Oct 12, 2021

joshuagl Oct 12, 2021

MVrachev Oct 12, 2021

jku Oct 13, 2021 •

edited

Loading

MVrachev Oct 13, 2021

MVrachev commented Oct 13, 2021

MVrachev commented Oct 13, 2021

jku commented Oct 13, 2021

	self._trusted_set.update_snapshot(data, trusted=False)
	self._trusted_set.update_snapshot(data)

Introduce the idea of trusted/untrusted snapshot #1605

Introduce the idea of trusted/untrusted snapshot #1605

Conversation

MVrachev commented Oct 5, 2021 • edited Loading

coveralls commented Oct 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1336473453

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jku left a comment • edited Loading

Choose a reason for hiding this comment

MVrachev commented Oct 8, 2021

jku left a comment • edited Loading

Choose a reason for hiding this comment

MVrachev commented Oct 8, 2021 • edited Loading

jku left a comment • edited Loading

Choose a reason for hiding this comment

jku commented Oct 11, 2021 • edited Loading

MVrachev commented Oct 12, 2021

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Oct 12, 2021

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl Oct 12, 2021

Choose a reason for hiding this comment

MVrachev Oct 12, 2021

Choose a reason for hiding this comment

joshuagl Oct 12, 2021

Choose a reason for hiding this comment

MVrachev Oct 12, 2021

Choose a reason for hiding this comment

jku Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

MVrachev Oct 13, 2021

Choose a reason for hiding this comment

MVrachev commented Oct 13, 2021

MVrachev commented Oct 13, 2021

jku commented Oct 13, 2021

MVrachev commented Oct 5, 2021 •

edited

Loading

coveralls commented Oct 5, 2021 •

edited

Loading

jku left a comment •

edited

Loading

jku left a comment •

edited

Loading

MVrachev commented Oct 8, 2021 •

edited

Loading

jku left a comment •

edited

Loading

jku commented Oct 11, 2021 •

edited

Loading

jku Oct 13, 2021 •

edited

Loading