TUF Offline Functionality

[emboman13]

Fixes #2359

This implements an offline configuration for Python TUF. It allows for the updater to exclusively function off of local metadata, given that a configuration in its constructor is set.

Changes are located within ngclient; specifically to config.py, updater.py, and to _internal/trusted_metadata_set.py. These changes include adding an additional configuration option to config.py, changing refresh() within updater.py to use said configuration option, and changing the load functions to support exclusive usage of local data.

Test implementation of this within sigstore can be found here:
https://github.com/emilejbm/sigstore-python/tree/offline-flag

[jku]

I'm writing my first review comments as they come to mind -- I may have some additional thoughts but would appreciate your takes on these.

• I like that it's quite clean already: I was worried it may make the code too cluttered but I think we can work with this...
• I would like for this feature to be very difficult to misuse -- that you could not e.g. accidentally download targets using an "offline updater": that seems like a user mistake. I think this can easily happen with this design
• What do you think about making "offline" something you enable at TrustedMetadataSet construction time (which practically means Updater construction time)?
  • then the TrustedMetadataSet could have an offline attribute (or skip_expiry attribute to be more descriptive)
  • it might mean some test setup changes, but the actual code may be simpler as the methods no longer need the arg and you could use the attribute in lower layers of the code (see a code comment)
  • Updater could then fail loudly in download_target() if its being used with a TrustedMetadataSet that is offline
• I haven't looked closely at the test yet but one additional thing we do need to test is the full use case: basically make sure we can get the locally cached target even if metadata has expired:
  • (setup: make sure metadata and a target are locally cached, and that metadata is now considered expired)
  • create offline Updater
  • get_targetinfo() for the target
  • find_cached_target()

I'm open to additional tests happening in other PRs if that seems to complicate this one too much

[jku]

I usually don't mind duplicating a bit of code but this has a bit of a smell... I think we could avoid this if the whole TrustedMetadataSet instance was "offline": then the lower level code (in this instance _check_final_snapshot()) could have the differing code paths and this code here would not need changes

even in he simpler cases, like _check_final_timestamp(), it might make the code easier to understand if the "offline" check was made inside the function (instead of in two separate places where the function is called)...

[emboman13]

Yea we were going back and forth for a little while on how we should deal with this. We'll modify _check_final_snapshot() to have differing codepaths

[emilejbm]

Thanks for all the feedback! We've made changes to TrustedMetadataSet so that it allows the offline flag to be set within this object and all methods now take advantage of this.

[jku]

I'll keep an eye on this PR too but if you want to discuss things more interactively the python-tuf slack channel is a good choice (I know slack is a pain if you're not on it already, sorry)

On the test results:

• squashing to a single commit is fine by me for something as well contained as your work is (then you'll only have to deal with DCO signed-off-by in that one commit and not try to edit all commits)
• linting is available locally as well, it just doesn't run automatically when the actual test suite runs: tox -e lint

[emboman13]

@jku we've implemented the changes you've suggested and ran a lint on our code, is there anything else we need to do?

[jku]

Have not looked at code yet but quick comments:

• the doc build failure is unrelated, don't mind that
• something wen't wrong with the signed-off-by: it looks like you've added signed-off-bys to commits that already exist in develop branch: this is not ok. Squashing your changes to single commit is ok if you'd rather do that.

[jku]

Code looks pretty good.

• linter is correctly flagging some expression-not-assigned -- I think these are just leftovers from the first version
• linter complaint too-many-instance-attributes can be disabled for this specific case in config.py. git grep "pylint: disable" will show examples of this
• I would like download_target() to fail if updater is offline (RuntimeError("Cannot download when offline") maybe?)
• what happens when you do get_targetinfo() while offline, when the target is delegated, and our local cache does not contain the delegated roles metadata? I think this should fail instead of downloading the metadata.

I've not tested this yet myself or looked at the test code, was planning to do that once lint and tests are passing -- let me know if you'd like assistance before that

[emboman13]

Have not looked at code yet but quick comments:

• the doc build failure is unrelated, don't mind that

• something wen't wrong with the signed-off-by: it looks like you've added signed-off-bys to commits that already exist in develop branch: this is not ok. Squashing your changes to single commit is ok if you'd rather do that.

Yea, believe it's an issue with catching commits that were picked up when syncing our fork. We should be able to revert back to before the retroactive signing and just do a final signed squash

[emboman13]

Code looks pretty good.

• linter is correctly flagging some expression-not-assigned -- I think these are just leftovers from the first version

• linter complaint too-many-instance-attributes can be disabled for this specific case in config.py. git grep "pylint: disable" will show examples of this

• I would like download_target() to fail if updater is offline (RuntimeError("Cannot download when offline") maybe?)

• what happens when you do get_targetinfo() while offline, when the target is delegated, and our local cache does not contain the delegated roles metadata? I think this should fail instead of downloading the metadata.

I've not tested this yet myself or looked at the test code, was planning to do that once lint and tests are passing -- let me know if you'd like assistance before that

Will take a look into this first chance I get.

[emboman13]

Sorry for the delay. We have been busy with graduation/post-graduation travels. We will be looking into this this weekend.

[emboman13]

We've made the changes you recommended; are there any other fixes you want us to make before this is in a state to merge? We do think our test cases are a little rudimentary at the moment and could use some outside input. @jku

[jku]

Yes the test case(s) do need a bit of work...

• there seems to be some confusion about how TestCase.assertRaises() TestCase.assertTrue() work. The test file has usage examples and unittest docs are decent: https://docs.python.org/3/library/unittest.html
• the usage of this new API within the test seems a bit weird now after the latest changes as the updater is created with default config (I assume this creates TrustedMetadataSet with offline=False argument)... and then afterwards updater.config.offline is set. This seems wrong and I'd rather see the configuration being a constructor argument to Updater -- in the test code you could add an optional "offline" argument to _init_updater()
• find_cached_target() should also be tested while offline

but lets's get the PR to an otherwise good state first: there is still something wrong in the first commit. It now contains changes that are clearly unrelated -- I'm guessing these are merged commits from upstream develop to your branch that were all squashed into one commit with your changes? I did say that "Squashing your changes to single commit is ok" but the effective term there was your changes :) Please try to fix that. If it feels like you can't salvage it, let me know and I'll give it a shot.

Rough outline of what I'd try:

• rebase the branch on top of current develop -- expect a few conflicts because of the strange changes
• fix any conflicts and finish the rebase
• check that unrelated files are not being changed in the commits in your branch (basically git diff --stat origin/main..<mybranch> assuming origin is upstream python-tuf repo) -- if there are still changes are, remove them in a new commit
• final check that changes in the branch look correct

For next PR I would suggest using a topic branch (like in this case a branch called "offline" instead of "develop"): keeping rebases and merges in check is easier when branches don't all have the same name

[emboman13]

Yes the test case(s) do need a bit of work...

• there seems to be some confusion about how TestCase.assertRaises() TestCase.assertTrue() work. The test file has usage examples and unittest docs are decent: https://docs.python.org/3/library/unittest.html

• the usage of this new API within the test seems a bit weird now after the latest changes as the updater is created with default config (I assume this creates TrustedMetadataSet with offline=False argument)... and then afterwards updater.config.offline is set. This seems wrong and I'd rather see the configuration being a constructor argument to Updater -- in the test code you could add an optional "offline" argument to _init_updater()

• find_cached_target() should also be tested while offline

but lets's get the PR to an otherwise good state first: there is still something wrong in the first commit. It now contains changes that are clearly unrelated -- I'm guessing these are merged commits from upstream develop to your branch that were all squashed into one commit with your changes? I did say that "Squashing your changes to single commit is ok" but the effective term there was your changes :) Please try to fix that. If it feels like you can't salvage it, let me know and I'll give it a shot.

Rough outline of what I'd try:

• rebase the branch on top of current develop -- expect a few conflicts because of the strange changes

• fix any conflicts and finish the rebase

• check that unrelated files are not being changed in the commits in your branch (basically git diff --stat origin/main..<mybranch> assuming origin is upstream python-tuf repo) -- if there are still changes are, remove them in a new commit

• final check that changes in the branch look correct

For next PR I would suggest using a topic branch (like in this case a branch called "offline" instead of "develop"): keeping rebases and merges in check is easier when branches don't all have the same name

I should be able to directly fix the conflicts via text editor here. I'm pretty sure just discarding all the code on this fork that's conflicting with the main repo should fix the issue

[jku]

The following suggestions about using the _trusted_set offline state instead of config.offline are related to the confusion that I think is going on in the test case.

The suggestions should not affect functionality if caller is doing the right thing, but if the user incorrectly modifies the config during the lifetime of the updater (like I believe current test does), our understanding of "offline" should not change: The TrustedMetadataSet offline state (and thus Updater offline state) should be immutable.

I'll have a closer look at testing next next week but the comments so far might already be useful

[jku]

This should probably check self._trusted_set.offline

[emboman13]

Would that be necessary? That is just set as self.config,offline on line 106-107?

[jku]

self.config.offline can be modified by the user after the Updater has been initialized but before the other methods are called. This does not make a lot of sense, but technically it is possible. In other words, if we check check self.config.offline here it may well be False even though self._trusted_set has been initialized as offline.

The current test case seems to be a good example of this.

[jku]

This should probably check self._trusted_set.offline

[jku]

This should probably check self._trusted_set.offline

[emilejbm]

Thanks for all the feedback! We've drafted a correction adding the optional flag for _init_updater() as well as correctly using assertRaises() and checking for the offline state of _trusted_set instead of the Updater.

We realized the first case we had written might not be needed at all. Here, we wanted to make sure the Updater (without metadata) behaved as it did before, with the addition of having the offline flag (set to False). Before committing anything, do you think we should remove this case altogether, given that it should technically already be tested for?

self.sim.fetch_tracker.metadata.clear()
with patch("datetime.datetime", mock_time):
    updater = self._init_updater()
    with self.assertRaises(
            (OSError, RepositoryError, DownloadError),
        ):
        updater.refresh()

Regarding the test for find_cached_target() while offline, is doing the following sound?

1. Initialize the Updater (offline=True)
2. Get the target info
3. Make sure download_target() raises RuntimeError
4. Assert the updater.find_cached_target(info) is None

[jku]

We realized the first case we had written might not be needed at all. Here, we wanted to make sure the Updater (without metadata) behaved as it did before, with the addition of having the offline flag (set to False). Before committing anything, do you think we should remove this case altogether, given that it should technically already be tested for?

I'm not quite sure how to answer since the test as is does not quite work... I'll write some thoughts down hope it helps.

• only mock the time (by patching datetime) if you need to, and when you do please add comment why
• note that you can create multiple updaters in a single test: e.g. first an updater to populate the cache without offline, then another offline one to test a specific scenario
• possible test cases for test_updater_top_level_update:
  • initialize new Updater with offline=True, refresh. Expect failure because top level metadata is missing locally
  • initialize an Updater, refresh to get metadata in local cache. initialize new Updater with offline=True, refresh. Expect success, use fetch_tracker.metadata to ensure nothing was downloaded while offline
  • initialize an Updater, refresh. mock time so that some metadata is now expired. Initialize new Updater with offline=True, refresh. Expect success because expiry is no longer a failure case when offline. use fetch_tracker.metadata to ensure nothing was downloaded
• possible test cases for artifact download tests (assume test repository that contains target files artifactA in top level targets and artifactB in a delegated role):
  • initialize an Updater, refresh to get local metadata. initialize new Updater with offline=True. Run get_targetinfo() on artifactA, then find_cached_target(). Axpect failure because the artifact is not locally cached.
  • initialize an Updater, refresh() to get local metadata. initialize new Updater with offline=True. Run get_targetinfo() on artifactB , expect failure because the delegated metadata is not locally cached.
  • initialize an Updater, run get_targetinfo+download_target() on artifactB to get the toplevel and delegated metadata and target file in local cache. initialize a new Updater with offline=True, run get_targetinfo+find_cached_target() on artifactB. Expect Success because the target and all metadata is in cache

[woodruffw]

Gentle ping on this: sigstore/sigstore-python#483 is blocked upstream by an offline configuration for TUF; it'd be great to have an expected/supported API here rather than brute-forcing offline support in on our end 🙂

[jku]

Gentle ping on this: sigstore/sigstore-python#483 is blocked upstream by an offline configuration for TUF; it'd be great to have an expected/supported API here rather than brute-forcing offline support in on our end slightly

Thanks. I'll be out for next week but I can put a bit of effort in this after that.

Can you confirm that #2359 (comment) describes the functionality you want? There's clearly multiple possible definitions of "offline" and I don't want python-tuf to end up with one that isn't useful: adding complexity to the verification code is not great so I want to be sure it's at least useful complexity

[woodruffw]

Can you confirm that #2359 (comment) describes the functionality you want? There's clearly multiple possible definitions of "offline" and I don't want python-tuf to end up with one that isn't useful: adding complexity to the verification code is not great so I want to be sure it's at least useful complexity

Yep, I just took a look and left a follow-up there -- I think it's 99% of what we want, with one open question about whether we should "fail open" in offline mode with expired metadata.

[jku]

I've dropped the ball on this one (I think I was waiting for some tests to appear), sorry. I will have a look this week

[jku]

This has been superseded by #2472 which contains this branch contents, loads of tests and some improvements from me.

I'm sorry for not including the original commits in that branch: this because of some merge issues in this branch. I've preserved the author info with a "Co-Authored-By", I hope that is acceptable to everyone. If there's feedback on this or anything else, please comment in #2472.