Repository module and example

[jku]

Calling this a repository module sounds a little too grand (it's literally an abstract class that does almost nothing)... but I think it works so maybe we start with this.

I'm adding some reviewers for visibility. If someone wants to see what it does:

• for demo, please try https://github.com/jku/python-tuf/blob/repository-lib/examples/client_example/README.md
• the implementation example is in https://github.com/jku/python-tuf/blob/repository-lib/examples/repository
• the "library" is in https://github.com/jku/python-tuf/blob/repository-lib/tuf/repository

Note that the Repository class is just a "ergonomic metadata editing framework" -- it makes producing valid and logical next versions of TUF metadata within a repository easier. So it's not really a "repository" (module name makes sense as this is for repository side tooling but the class name could still be debated)

examples/repository should also be extended (or other examples added):

• it should have a tiny developer API and there should be a developer tool (built using the same building blocks) that can sign and upload things to the example repository app
• some of the more complex repository details demonstrated in the manual examples should be implemented here as well (and eventually manual repo examples deleted)

Fixes #1136 maybe?

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

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

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.001%) to 98.19%

---

Files with Coverage Reduction	New Missed Lines	%
tuf/ngclient/updater.py	3	97.97%

Totals
Change from base Build 3609587369:	0.001%
Covered Lines:	1347
Relevant Lines:	1363

---

💛 - Coveralls

[jku]

for demo, please try https://github.com/jku/python-tuf/blob/repository-lib/examples/client_example/README.md

Forgot to mention: you may be tempted to run the server and use a browser to see the metadata being served. This sort of works but ultimately breaks things as at least Chrome does things that are incompatible with single thread servers like http.server.HTTPServer: it opens connections (just in case) and then does nothing.

[jku]

Having played with this for about a week now, I think there is one "Repository implementation" we may want to include in the library itself. While the repositories themselves may be wildly different, the "developer tools" or "signing tools" are probably very similar:

• they have a root of trust like a downloader client
• they have a cache for local, valid metadata like a downloader client
• they may update to current metadata by downloading from the remote repository like a downloader client would (but may also have other means like git pull)
• they do want to only operate on top of a valid metadata cache... but the definition of valid is not the same as in a downloader client: as an example they do want to load expired metadata so that they can make a new version of it...
• they likely never want to modify the metadata in cache: they'll want to modify the metadata to send a new version to the remote repository. The method of "sending" will vary (http post, git commit & push) but the idea is the same: the metadata cache should represent the remote repository state, and the modified metadata gets sent to that repository to integrate

So the result should be a mix of ngclient.Updater and the SimpleRepository from this PR. This is clearly future work and will not happen in in this PR, I'm just writing the idea out there.

[jku]

plain rebase, no changes

[jku]

A potential change to snapshot/timestamp:

• Current implementation requires "meta" as argument to both methods
• alternative is to require Repository to provide snapshot meta and timestamp meta as properties, this way the methods wouldn't need arguments.

I think this would be as flexible but would make calling snapshot()/timestamp() easier

EDIT: I've implemented this in later commit

[lukpueh]

Impressive work! I've given this a very close look, especially tuf/repository and examples/repository/_simplerepo.py, and only found some minor nits. I think we can get this merged and even released soon, but maybe note somewhere that this is more experimental than ngclient and metadata api.

[lukpueh]

Suggested change

	this repositorys. Keys are rolenames and values are lists of
	this repository. Keys are role names and values are lists of

[lukpueh]

Ping s/repositorys/repository

[jku]

(I can't believe I'm asking for bikeshedding but here goes...)

One thing that I would like bring up for discussion is naming:

• open()/close() -- on one hand these are descriptive but it's not perfect: as an example, not closing a file descriptor is a bug but not closing a role in this api is fine: you just decided not to write a new version after all. I have also considered open()/commit() but so far the symmetry of the current naming has won. Maybe this is not even an important detail as most users should be using edit() which handles open/close.
• snapshot_info and targets_infos: these caches are really hard to name without making a mess in the code: A timestamp role contains snapshot meta and snapshot role contains targets meta, and these two should sort of map to those metas. snapshot_info and targets_infos represent the current state of stored metadata while the metas are a view of the published repository at a specific point in time -- including already removed roles which these caches do not contain.

Another thing is the idea of the edit() contextmanager: does that make sense to others? Is the use of AbortEdit still understandable -- I imagine it won't be needed often but snapshot() does use it so it should be understandable...

[jku]

the core usability complaint I have with edit() is that it doesn't handle better static typing:

with edit("targets") as targets:
    # mypy sees targets as type Signed, not Targets

Most issues would just be solved by adding a typed helper:

    def edit_targets(
        self, role: str, init: bool = False
    ) -> Generator[Targets, None, None]:

(not sure if the other types even need a helper: timestamp and snapshot at least don't need so much editing: Repository handles the usual cases already)

[lukpueh]

I am not very exited about adding a helper just for typing. What's the implication if mypy doesn't know the subtype?

[jku]

The implication is that more complex code in the with-block (like this attempt at easy target removal) won't have type suggestions in your editor and won't be type checked by mypy unless you do some extra work.

The workaround I use in that example makes VSCode suggestions work but doesn't make mypy happy... I think the least disruptive workaround is probably a assert(isinstance(Targets, targets)) as the first line inside the with-block.

[jku]

but just to be clear: I'm not suggesting to add a helper in this PR -- just bringing up potential areas of improvement

[lukpueh]

(I can't believe I'm asking for bikeshedding but here goes...)

One thing that I would like bring up for discussion is naming:

Fair. Naming is important, and hard.

• open()/close() -- on one hand these are descriptive but it's not perfect: as an example, not closing a file descriptor is a bug but not closing a role in this api is fine: you just decided not to write a new version after all. I have also considered open()/commit() but so far the symmetry of the current naming has won. Maybe this is not even an important detail as most users should be using edit() which handles open/close.

I had similar thoughts. These names do have other connotations. What about more abstract terms, like start/end, enter/exit, pre_edit/post_edit? ... otoh, those make less sense, when used outside of edit. Maybe open/close are just fine.

• snapshot_info and targets_infos: these caches are really hard to name without making a mess in the code: A timestamp role contains snapshot meta and snapshot role contains targets meta, and these two should sort of map to those metas. snapshot_info and targets_infos represent the current state of stored metadata while the metas are a view of the published repository at a specific point in time -- including already removed roles which these caches do not contain.

Can't think of better names. Let's solve this with documentation. Maybe add a note to look at timestamp and snapshot methods, and e.g. SimpleRepository, when in doubt about how these are maintained.

For that it might be helpful to rename the second return value in snapshot (e.g. to
replaced/updated/old/previous), to make it clear that this does not contradict with "the dictionary published by snapshot() will also include metadata that no longer exists in the repository".

Btw., it also took me a bit to understand the magical defaultdict+lambda -powered assignment in SimpleRepository, when I tried to understand targets_infos. But maybe that's just me. And it's definitely a cool too. Maybe a comment would help?

Another thing is the idea of the edit() contextmanager: does that make sense to others?

I think it does. One thing I was wondering was whether open/close arguments should also be left unspecified given that open and close can be anything the implementer wants them to be. E.g. they could both accept **kwargs passed on like so:

def edit(role: str, open_kwargs: Dict[str, Any] = None, close_kwargs: Dict[str, Any] = None)

Is the use of AbortEdit still understandable -- I imagine it won't be needed often but snapshot() does use it so it should be understandable...

I think it's fine.

[jku]

Btw., it also took me a bit to understand the magical defaultdict+lambda -powered assignment in SimpleRepository, when I tried to understand targets_infos. But maybe that's just me. And it's definitely a cool too. Maybe a comment would help?

yeah, that defaultdict is likely only useful once in in close() ... so the case of missing key could be handled manually there. But on the other hand role_cache and signer_cache use the same mechanism (it would even look the same if MetaFile() had a default version value).

I'll just add a comment

[jku]

For that it might be helpful to rename the second return value in snapshot (e.g. to
replaced/updated/old/previous), to make it clear that this does not contradict with "the dictionary published by snapshot() will also include metadata that no longer exists in the repository".

I've kept the names but I tweaked the docstring of the methods to make it clear that the returned values are metadata versions that are no longer part of snapshot/timestamp meta.

I think I've now addressed everything so far.

[jku]

From chat with lukas:

open(self, role: str, init: bool = False)

this does not really need the init argument...

could also move sign() out of the ABC -- then close also does not need the sign_only argument.

[jku]

Two changes:

• init argument was removed from open: it's just not necessary
• sign() (meaning signing an already existing version) was removed from the ABC: this should be done by implementations that really need this. It's not clear if the open/close concept works in that case at all

[lukpueh]

Please fix two typos and one outdated docstring and this is good to go.