Test expired metadata from cache

[ivanayov]

This tests that an expired timestamp/snapshot/targets when loaded
from cache is not stored as final but is used to verify the new
timestamp

Addresses #1681

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• [n/a] Docs have been added for the bug fix or new feature

[ivanayov]

I'm leaving this with the non-matching description on purpose, as the expected behaviour in
the issue description is to load expired snapshot/targets, which is not the case. Not loading them
is actually correct from what I understand from the spec - you shouldn't be able to use outdated timestamp for verification, thus you cannot verify snapshot and targets as well. Please, correct me if I misunderstand.
So we could ether reword the requirement, of if there's a bug we can fix it.

[coveralls]

Pull Request Test Coverage Report for Build 1865335270

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

• 2 of 2(100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 98.574%

---

Totals
Change from base Build 1852769022:	0.2%
Covered Lines:	1095
Relevant Lines:	1107

---

💛 - Coveralls

[MVrachev]

I have some ideas about this pr and will have a conversation with Ivana soon.

[jku]

the expected behaviour in
the issue description is to load expired snapshot/targets, which is not the case. Not loading them
is actually correct from what I understand from the spec

This may have been clarified already but I'l say it here:

We do load expired metadata with one big caveat: the expired metadata can only be used to validate a newer version of the same type of metadata. In practice:

• local, possibly expired, metadata is loaded and used to validate newer downloaded metadata. "validate" here means signature verification for root and various version checks for timestamp, snapshot and targets.
• even the downloaded versions may be expired (in practice only root is, because we download all root versions up to the current, hopefully non-expired version)
• the python-tuf implementation will fail the update once the process moves to the next type of metadata and the previous metadata is still expired (e.g. if final root version is expired, refresh() will fail when it tries to validate timestamp)

[jku]

the core issue here is that verifying that local files are opened does not mean they are actually used to verify new downloaded metadata (this is made clear even in the test itself: it opens targets.json but we know that's not used for anything). This does not mean the test is worthless but at least some descriptions in the PR and commit are not describing it correctly. I would like to see a clear explanation of what the test does so that
A) we can be sure we don't already have a test that does the same checks
B) future readers won't be deceived by inaccurate descriptions
I get that this may be the hardest part of this PR: let's talk if you don't have a good idea already

Just to be clear: I think this is a useful test that probably does not exist yet... but test_updater_top_level_update.py has some clearly related tests like test_new_timestamp_expired(): maybe it should have a companion test_local_timestamp_expired()? I don't have strong opinions here but maybe @sechkova has ideas about where to put tests like this -- and whether other tests exist already?

Also (and this is a small detail in comparison), I've been trying to advocate not meddling with the client metadata cache manually: in this case I think it would be better to not do that and instead mock current time (pretend that time now > expiry). Search for mock_time in tests for examples of this.

[sechkova]

Just to be clear: I think this is a useful test that probably does not exist yet... but test_updater_top_level_update.py has some clearly related tests like test_new_timestamp_expired(): maybe it should have a companion test_local_timestamp_expired()? I don't have strong opinions here but maybe @sechkova has ideas about where to put tests like this -- and whether other tests exist already?

I admit I find it hard to follow the intentions here, I am sorry if I've put a misleading issue description.
If I understand correctly, test_trusted_root_expired from test_updater_top_level_update.py is doing something very similar and can be used as an example.

[ivanayov]

@sechkova the idea was to simulate an expired metadata in cache without manually modifying the cache files. I might have misunderstood this mock - the intention is to simulate change in system time, so that what's stored in cache is considered expired, without need of directly touching the local metadata.

[jku]

I admit I find it hard to follow the intentions here, I am sorry if I've put a misleading issue description. If I understand correctly, test_trusted_root_expired from test_updater_top_level_update.py is doing something very similar and can be used as an example.

It is pretty similar, except that I think we don't actually persist downloaded non-root metadata if it's expired, even though we probably could (rado pointed this out to me -- I had not noticed). This difference isn't that important for actual working repos but for testing it means we need to mock system time (or utcnow() as you point out) to "make the local metadata expired".

[sechkova]

I admit I find it hard to follow the intentions here, I am sorry if I've put a misleading issue description. If I understand correctly, test_trusted_root_expired from test_updater_top_level_update.py is doing something very similar and can be used as an example.

It is pretty similar, except that I think we don't actually persist downloaded non-root metadata if it's expired, even though we probably could (rado pointed this out to me -- I had not noticed). This difference isn't that important for actual working repos but for testing it means we need to mock system time (or utcnow() as you point out) to "make the local metadata expired".

Thanks, now I think I got all pieces together. So I agree, test_updater_top_level_update.py could host the additional tests for loading expired snapshot and timestamp and since expired metadata and timestamp are not persisted, mocking seems to be the way.
I think expired local "targets" should be treated the same way as an invalid (unsigned) local metadata and may be part of another set of tests?

[jku]

I'm marking this a draft since the issue pointed out by Teodora (mocking the wrong thing) is still there: I might have other comments but I've been postponing review until the basics are there. I'm available for a chat if anything is unclear.

[ivanayov]

@jku I changed to mocking datetime as Teodora advised, so it's now available for review.

[jku]

marking request changes so it's visible on PR list: we talked about this over zoom, and found an issue with the timeline of expiries and the mocked refresh: ivana promised to fix

[ivanayov]

I've pushed an update.

[jku]

Nice, all test code seems correct and clear.

• Left some minor tweak suggestions in code
• These tests should have a docstring to explain what they are testing: they are some of the trickiest tests we have. Something like "Local timestamp must be used in rollback checks even if it is expired" -- maybe the same text that's already before the actual assertRaises

Additionally maybe the dosctring should also include a timeline, example of one test:

 * day 0: first updater refresh
* day 0 (a bit later): repository publishes timestamp v2
* day 7: timestamp v1 expiry
* day 18: second updater refresh: assert that rollback check uses expired v1 here
* day 21: timestamp v2 expiry

not sure about that last thing though: it's a lot of lines... but may be helpful to understand what's going on? I'll leave decision to you

[ivanayov]

Thank you @jku , I added docstrings to all the test methods for clarification.

[jku]

my comment about reusing the datetime.datetime.utcnow() result is marked resolved but all functions still call utcnow() twice...

That is a trivial complaint so let's go with this version, I think it's a good set of very tricky tests. Thanks.

[ivanayov]

@jku ah, I resolved by inertia with the microseconds updates... will note to address in following changes. Thanks