Offline mode

[jku]

Fixes #2359, supersedes #2363

Summary

This is based 2363 code by Emile Baez, Emerson Rounds and omartounsi7 (see first commit). I've fixed some merge issues and linting problems, added tests and finally done some refactoring.

The PR exposes a new config option offline: bool. When offline, Updater behaviour changes in two ways:

1. nothing is downloaded during refresh(), get_target_info() and find_cached_target(): the operations are done using locally available metadata only.
2. Metadata expiry is no longer respected. All other security checks are still in operation

What this means is that an offline Updater can be used when client

• has all the required metadata and targets cached locally (either because it has used Updater in online mode before or because some other process has filled the caches out-of-band)
• is fine with the compromise in security that using potentially revoked keys means

Implementation notes

These are mostly about changes I did on top of 2363:

• The TrustedMetadataSet argument was renamed: as the component does not know anything about downloads, offline seemed less useful. The argument is now respect_expiry which I think is accurate. The expiry checks were consolidated in one method: if metadata has expired it either raises or logs a warning based on respect_expiry
• Updater exceptions were standardized: If Updater would have to download something but is offline, it now raises DownloadError
• Testing feels solid now
  • There's a new set of tests for Updater with offline=True: these try to ensure nothing is downloaded while offline, basic offline cases work and that offline errors are what we expect
  • TrustedMetadataSet tests were essentially doubled with subtests to cover both possible respect_expiry values
  • It would be an option to do the same subtest treatment to Updater tests to ensure that offline=True does not change anything we don't expect it to change... but I feel like this is a reasonable amount of tests
• I've moved the Updater offline checks to the Updater._load_*() methods: You can check the commit-before-last if you want to see what the alternative option looks like

• 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]

Pull Request Test Coverage Report for Build 6328656993

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 4 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.03%) to 97.551%

---

Files with Coverage Reduction	New Missed Lines	%
tuf/ngclient/_internal/trusted_metadata_set.py	1	99.49%
tuf/ngclient/updater.py	3	98.17%

Totals
Change from base Build 6311653947:	0.03%
Covered Lines:	1350
Relevant Lines:	1377

---

💛 - Coveralls

[jku]

I'm still considering whether the if-else paths in Updater are in the right places:

• currently they are in refresh() and _preorder_depth_first_walk()
• they could instead be a level lower: _load_root(), _load_timestamp(), _load_snapshot(), _load_targets()

I think the latter could be slightly more code but more understandable. Now that tests exist, I think I will try that option as well to see what it looks like.

EDIT: I've now implemented the latter option in the last commit, I think it's decent

[jku]

I didn't introduce a new exception subclass since the context seems to make this useful enough for apps: if your updater is offline, then DownloadError means the requested action is not possible with the currently cached data.

[jku]

A possible complaint on this PR is that it exposes a very specific new mode: so for example expiry is either fully respected or not at all:

• caller can't know if some of the metadata was expired (apart from warnings in the log)
• caller can't say "I want offline mode but always respect expiry"

I felt like exposing more knobs in the API would make it more complicated to use, to test and to build in a safe manner. I'm happy to consider doing that though if there are real use cases where these knobs would make a difference.

[lukpueh]

Great patch! Implementation-wise this looks good. I still think we should give clear answers to the following questions somewhere in the ngclient documentation:

• What exactly is the use of offline mode over not using ngclient at all when offline?
• More specifically, what security guarantees exactly do we sacrifice in offline mode ("timeliness" alone feels to vague) and what guarantees do we preserve?

[lukpueh]

IMO "works normally" downplays the difference quite a bit. Wouldn't it be fairer to reverse the description and underline the difference a long the lines of ... ?

when offline, Updater does not update metadata or target files anymore. It can still be used to retrieve and vet locally available target files against locally available metadata with the important caveat that metadata expiry is ignored.

[lukpueh]

It's hard for a client to know, if they really have all the required metadata and targets cached locally, if they don't consult with the repository and ignore expirations.

Also, while I can see how it is useful for a client to use the metadata and targets cached locally, I am still sceptical about the usefulness of using the TUF client for this. In other words, what is the benefit of using the updater in offline mode vs. not using the updater at all?

We should make this clear somewhere. Probably not here in the docstring.

[lukpueh]

sacrifice timeliness AND compromise recovery

[jku]

I have forgotten to update this one:

I discussed this with lukas and from sigstore perspective with William woodward and came these conclusions:

• this patch has value if the local metadata can be expected to be more trustworthy than targets cache: currently this is not the case, at least Updater feature request: verify chain of trust from bootstrapped root metadata #1168 would be needed for that
• Adding complexity and new features into the library (with those features essentially being not useful until other work) does not seem useful
• sigstore can achieve the same results by just trusting the local target cache when offline

That should answer Lukas questions in the comment: this PR alone does not have clear security benefits. We could get protection against maliciously modified local cache (both metadata and targets) if we can assume:

• this PR,
• 1168 (verifying local metadata from bootstrapped root),
• more secure initial metadata (e.g. the initial metadata is installed as non user modifiable, like if sigstore-python was installed as a Debian package)

So my instinct is to not merge this PR. We can re-evaluate if 1168 gets fixed.