Remove global state from repository_tool

[davidstrauss]

Description of issue or feature request:

As part of my PHP-TUF work, I'm developing a range of fixtures for our conformance testing. (I would also love to get this work upstream.) Unfortunately, tuf.repository_tool seems to maintain some in-memory state that interferes with generating multiple repositories from the same script. The code below demonstrates the issue, and the second repository object is largely broken.

While it's not shown here, it also seems to hold onto targets and then get confused when they're no longer available from the expected relative path in the newly initialized repo.

I've noticed a workaround is to instantiate each repository with a unique second argument (third if you count the implied self) for repository_tool.create_new_repository(), but it seems like this should be unnecessary if they're separate objects with their own directories already.

Code to reproduce:

from tuf import repository_tool as rt

rt.generate_and_write_ed25519_keypair('mykey', password='pw')
public_key = rt.import_ed25519_publickey_from_file('mykey.pub')
private_key = rt.import_ed25519_privatekey_from_file('mykey', password='pw')

repo = rt.create_new_repository('repo')
repo.root.add_verification_key(public_key)
repo.root.load_signing_key(private_key)

repo = rt.create_new_repository('repo2')
repo.status()
repo.root.add_verification_key(public_key)
repo.root.load_signing_key(private_key)

Current behavior:

Output:

Creating '/var/home/straussd/Projects/php-tuf/repo'
Creating '/var/home/straussd/Projects/php-tuf/repo/metadata.staged'
Creating '/var/home/straussd/Projects/php-tuf/repo/targets'
Creating '/var/home/straussd/Projects/php-tuf/repo2'
Creating '/var/home/straussd/Projects/php-tuf/repo2/metadata.staged'
Creating '/var/home/straussd/Projects/php-tuf/repo2/targets'
'targets' role contains 0 / 1 public keys.
'snapshot' role contains 0 / 1 public keys.
'timestamp' role contains 0 / 1 public keys.
'root' role contains 1 / 1 signatures.
'targets' role contains 0 / 1 signatures.
Adding a verification key that has already been used.

Expected behavior:

Output (altered from above). Please note the 'root' role contains and missing "already been used" lines.

Creating '/var/home/straussd/Projects/php-tuf/repo'
Creating '/var/home/straussd/Projects/php-tuf/repo/metadata.staged'
Creating '/var/home/straussd/Projects/php-tuf/repo/targets'
Creating '/var/home/straussd/Projects/php-tuf/repo2'
Creating '/var/home/straussd/Projects/php-tuf/repo2/metadata.staged'
Creating '/var/home/straussd/Projects/php-tuf/repo2/targets'
'targets' role contains 0 / 1 public keys.
'snapshot' role contains 0 / 1 public keys.
'timestamp' role contains 0 / 1 public keys.
'root' role contains 0 / 1 signatures.
'targets' role contains 0 / 1 signatures.

[lukpueh]

Thanks for the detailed and reproducible issue report, @davidstrauss!

I've noticed a workaround is to instantiate each repository with a unique second argument (third if you count the implied self) for repository_tool.create_new_repository()

I fear this is not a workaround but the intended use.

To provide some context: Part of the global state you mention is kept in roledb, which stores repositories by name (i.e. the 2nd non-self argument to create_new_repository). If no name is passed, it defaults to "default" and any consecutive calls will override previously created default repos in memory. Signing keys, on the other hand, are stored in the globally kept keydb and associated with repos by repo name.

So what happens in your example is that "repo2" replaces "repo" in roledb under the name "default", but status() still finds a root signing key for "default" in keydb and is thus able to create a signature and report "'root' role contains 1 / 1 signatures." even though you haven't loaded a key for root in "repo2".

but it seems like this should be unnecessary if they're separate objects with their own directories already.

I agree!

[lukpueh]

A mildly related FYI in case you work on the develop branch of TUF:
The order of arguments in rt.generate_and_write_ed25519_keypair('mykey', password='pw') has changed due to a recent updated. It's now rt.generate_and_write_ed25519_keypair(password='pw', filepath='mykey')

[davidstrauss]

I fear this is not a workaround but the intended use.

I suspected this as well. I'm concerned about the risk of leaky context between fixtures I'm generating, which could make any given fixture dependent on state created by prior fixtures if the namespacing in components isn't comprehensive.

Would it be possible to leverage an aspect of the instantiated object's identity (or create an aspect) that auto-namespaces to avoid these effects? I can't think of why you would want to instantiate two repository objects that share an undefined subset of state.

Also, thanks for the info about the updated parameter order.

[davidstrauss]

Would it be possible to leverage an aspect of the instantiated object's identity (or create an aspect) that auto-namespaces to avoid these effects? I can't think of why you would want to instantiate two repository objects that share an undefined subset of state.

This, or maybe offer the interface as procedural and namespace-only (to go the other way)? There's a note in Slack about "an unfortunate legacy of considering OOP bad practice for security." I feel like consumers of the library being able to reason about the behavior (and have actual behavior meet those expectations) is also important for security. A mixed OO/namespace model for state is hard to reason about.

[lukpueh]

Would it be possible to leverage an aspect of the instantiated object's identity (or create an aspect) that auto-namespaces to avoid these effects?

Yes, that would be possible. But I'm a bit reluctant to spend any efforts on repository_tool that aren't strictly necessary for current operations. I'd rather push forward the planned refactor (most notably in this context: #1136).
The plan is to no longer use different in-memory representations (e.g. objects in repository_tool and dictionaries in roledb) for the same TUF role metadata.

Until then, would your use case allow to just pass unique names to create_new_repository?

I can't think of why you would want to instantiate two repository objects that share an undefined subset of state.

Me neither.

[jku]

Closing this issue as it was filed against (what is now known as) the legacy codebase: issue seems to not be relevant anymore. Please re-open or file a new issue if you feel that the issue is revelant to current python-tuf.

The current code base does not seem to suffer from the issue described but note that we don't currently have a replacement for repository_tool: examples/repo_example/ shows how to use the lower level Metadata API for repository purposes.

More details

Current source code (and upcoming 1.0 release) only contains the modern components

• a low-level Metadata API (tuf.api) and
• tuf.ngclient that implements the client workflow,

Legacy components (e.g. tuf.client, tuf.repository_tool, tuf.repository_lib as well as the repo and client scripts) are no longer included. See announcement and API reference for more details.