Refactoring pr tuf initialization

[kairoaraujo]

The current work on Warehouse is a refactoring from the existing PR Draft using the old
python-tuf repository tools.

The mentioned repository tools are no longer available on the latest version.

The idea is to start implementing small PRs to evaluate and progress.

Current status

• Implemented the new repository tool using the new python-tuf Metadata API.

  Repository tool Design overview:

  • The repository warehouse.tuf.repository.MetadataRepository management
    tool was created standalone, not as zope.interface.Interface.

    The reason is that if a better Metadata Repository management implementation
    is available in the future, it could be replaced easily.

    Also, the MetadataRepository does not need to support different
    implementations such as SaaS once the StorageService is enough.

  • The IStorageService was used in the
    warehouse.tuf.repository.MetadataRepository as backend storage.

  • The warehouse.cli.tuf calls goes through warehouse.cli.tasks, meaning
    that all calls goes trough Celery.

    classDiagram 
      direction LR
      class MetadataRepository {
        <<tuf.repository>>
        +storage_backend
        +key_backend
        initialize()
        load_role()
        bump_role_version()
        timestamp_bump_version()
        snapshot_bump_version()
        snapshot_update_meta()
        delegate_targets_roles()
        add_targets()
      }
      class `tuf.interfaces` {
        zope.interface.Interface
        IKeyService(Interface)
        IStorageService(Interface)
        IRepositoryService(Interface)
      }
      class `tuf.services` {
        IKeyService
        IRepositoryService
        IStorageService        
        LocalKeyService(IKeyService)
        LocalStorageService(IStorageService)
        LocalRepositoryService(IRepositoryService)
      }
      class `tuf.tasks` {
        init_repository
        delegate_targets_bin_bins
        bump_snapshot
        bump_bin_ns
        add_targets_packages
      }
  
      class `cli.tuf`{
          dev keypairs
          dev new-repo
          dev add-targets
          admin delegate-targets
          admin bump-snapshot
          admin bump-bin-ns
      }
  
  
      `tuf.services` <|-- `tuf.interfaces`
      `tuf.services` --* MetadataRepository
      `tuf.tasks` -- `tuf.services`
      `cli.tuf` -- `tuf.tasks`
      warehouse -- `cli.tuf`
      warehouse -- `tuf.tasks`
  

  Loading

• Implemented Services/Interfaces for Storage and Storage Keys

• Implemented TUF CLI

  • Development Commands
    • Generate Key Pairs for the roles
    • Initialize the TUF Repository (keys and metadata) for Development
    • Add targets files (Packages from Database)
  • Administrative Commands
    • Generate the delegations Roles and Hashed bin roles (bins and bin-n)
    • Bump snapshot
    • Bump delegate bin-n hashed bin targets roles

• Tasks

  • Update Snapshot role metadata
  • Update delegated hash bin roles (bin-n) and their delegations

It's is possible use the new python-tuf client (ngclient) to download the
target.

Next steps:

• Polish the new Warehouse metadata repository tool

• Publish the new Draft PR

  Goals:

  • Limited for enabling tuf Metadata on development environment
  • Intent to validate the Metadata Repository tool

• PR to implement the simple detail index to the Metadata

• PR to implement the TUF in the Warehouse request flow

  • upload target file
  • delete target file
  • tasks for refreshing indexes/projects

Using the Warehouse development environment for TUF

Follow the official Warehouse until make initdb

$ make inittuf

The metadata is available at http://localhost:9001/metadata/

You can also upload a file using the Warehouse and add the targets using CLI

• Create a user using Web UI
• Validate the email
• Upload file using twine

docker-compose run --rm web python -m warehouse tuf dev add-targets

[lukpueh]

🎉 ❤️ 🚀 Amazing work, @kairoaraujo! On a first pass it all looks sound.

But I'd like to give it some more scrutiny to see if if it really is fully spec/pep compliant.

Before that I have a couple of high-level questions:

IIUC the call paths to create and modify the TUF metadata repository are as follows:

• cli.tuf -> tuf.repository
• cli.tuf -> tuf.utils.repository_* -> tuf.repository
• tuf.tasks -> tuf.utils.repository_* -> tuf.repository

I must admit that the boundaries between those levels of abstraction are not completely obvious to me. Would you mind sharing your thoughts about each module and why you implemented any given functionality in a given module?

Let me try to be more concrete, you added two new data structures RolesPayload and TargetsPayload, which resemble the data structures from the python-tuf metadata API quite a bit. Now if you look e.g. at add_targets or delegate_targets_roles in the cli.tuf,
there you iterate over some input data (db.query, hash bin generator) to construct a payload container, which you send over to the your tuf.repository, where you again iterate over the data, i.e. to container, to generate and write actual TUF metadata. Wouldn't it be possible to skip this intermediate wrapping and unwrapping of data? I think that would make it easier to see if the resulting TUF metadata is correct, at least for me.

That said, I am sure you have good reasons for your design, and I'd love to understand them. :)

I also left some comments inline, including some that are not very important...

[lukpueh]

Why pass snapshot_metadata only to return it unused?

[kairoaraujo]

You found a 🐛 here.
It is changed to Optional once it depends on the scenario.

The snapshot_metadata is important if you create multiple delegations and update the Snapshot Meta many times to avoid bumping multiple Snapshot versions.
For example, when we create the hashed bin roles.

[lukpueh]

Minor nit: Why use the private _is_initialized when there is also a public is_initialized? Also, in initialize(), which you call below the bool is checked again and a FileExistsError is raised on True, which you do handle handle.

On a more general note, I wonder if you really need the initialization state variable? It looks like a lot of code and effort to keep it up-to-date, and easy to get wrong. Maybe it's easier to handle the existence / non-existence of the files on per-case basis?

[kairoaraujo]

I didn't fix it yet.

[lukpueh]

I think in production the ultimately desired signing threshold is higher than the available signing keys at the time of bootstrapping the top-level metadata, at least for the root role (and I think also for top-level targets and bins roles, which according to the PEP should also be signed with offline keys).

If this initialize method is for testing/development purposes (currently it is only used by the tuf dev new-repo command) maybe it does not have to be part of the MetadataRepository API?

[kairoaraujo]

The intention here is also to have an admin command, giving the local keys as parameters.
The idea is that during the creation of the initial Metadata Repository, we could use it in an offline environment avoiding a manual bootstrap and possible mistakes.
We could do the process as a command line for the Metadata Repository bootstrap and BIN-S delegations.

[kairoaraujo]

In the current development, I increased the number of keys/threshold to 2 for the Targets role, in a sense to test multiple keys.

[kairoaraujo]

Before that I have a couple of high-level questions:

IIUC the call paths to create and modify the TUF metadata repository are as follows:

• cli.tuf -> tuf.repository
• cli.tuf -> tuf.utils.repository_* -> tuf.repository
• tuf.tasks -> tuf.utils.repository_* -> tuf.repository

As you can see, I tried to improve it in the last commit.

I must admit that the boundaries between those levels of abstraction are not completely obvious to me. Would you mind sharing your thoughts about each module and why you implemented any given functionality in a given module?

Let me try to be more concrete, you added two new data structures RolesPayload and TargetsPayload, which resemble the data structures from the python-tuf metadata API quite a bit. Now if you look e.g. at add_targets or delegate_targets_roles in the cli.tuf, there you iterate over some input data (db.query, hash bin generator) to construct a payload container, which you send over to the your tuf.repository, where you again iterate over the data, i.e. to container, to generate and write actual TUF metadata. Wouldn't it be possible to skip this intermediate wrapping and unwrapping of data? I think that would make it easier to see if the resulting TUF metadata is correct, at least for me.

That said, I am sure you have good reasons for your design, and I'd love to understand them. :)

I also left some comments inline, including some that are not very important...

As we discussed, the idea is to send to the MetadataRepository a bucket of data and operate once, avoiding bumping the delegated roles, Snapshot, Timestamp multiple times.
But we can discuss if there is a better way to do it.

[lukpueh]

This is getting better and better! With your recent changes I can focus on tuf.services and tuf.repository to review spec/pep compliance. ❤️

I'm not fully finished yet, but have a couple of comments I'd like to share with you in the meanwhile...

[lukpueh]

Good morning, @kairoaraujo! I have two questions about tuf.interfaces (full disclosure: I am not familiar with zope)

[kairoaraujo]

I made some improvements to the CLI descriptions.

root@9f0fc37a57c0:/opt/warehouse/src# python -m warehouse tuf dev
Usage: python -m warehouse tuf dev [OPTIONS] COMMAND [ARGS]...

  TUF Development purposes commands

Options:
  -h, --help  Show this message and exit.

Commands:
  add-all-indexes   Collect every PyPI project Index and add as targets.
  add-all-packages  Collect every PyPI package and add as targets.
  bump-bin-n-roles  Bump delegated targets roles (BIN-N).
  bump-snapshot     Bump Snapshot metadata.
  init-delegations  Create delegated targets roles (BINS and BIN-N).
  init-repo         Initialize a new TUF repository if it does not exist.
  keypair           Generate a new TUF keypair for top-level roles.

[lukpueh]

I think this is something we should run by the other PEP authors.

[lukpueh]

One more thing I noticed wrt interface boundary between tuf.services and tuf.repository:

All service methods that update targets metadata, i.e. init_targets_delegation, add_hashed_targets and bump_bin_n_roles transitively bump 'snapshot' and 'timestamp'. But while the latter does this explicitly, the former two do it implicitly by calling methods in tuf.repository that do it for them.
bump_snapshot follows the same principle as bump_bin_n_roles, as it transitively but explicitly bumps 'timestamp'.

I think this makes the 'tuf.repository' interface slightly inconsistent. Is performing the chain of inter-dependent metadata updates a repository task, or is it a task for the application code (service)? I don't say we should change this, but I wanted to take a note about it somewhere.

[kairoaraujo]

One more thing I noticed wrt interface boundary between tuf.services and tuf.repository:

All service methods that update targets metadata, i.e. init_targets_delegation, add_hashed_targets and bump_bin_n_roles transitively bump 'snapshot' and 'timestamp'. But while the latter does this explicitly, the former two do it implicitly by calling methods in tuf.repository that do it for them. bump_snapshot follows the same principle as bump_bin_n_roles, as it transitively but explicitly bumps 'timestamp'.

I think this makes the 'tuf.repository' interface slightly inconsistent. Is performing the chain of inter-dependent metadata updates a repository task, or is it a task for the application code (service)? I don't say we should change this, but I wanted to take a note about it somewhere.

That was an excellent catch @lukpueh ❤️

I removed the implicitly calls for bumping snapshot and timestamp version in the repository (repository.tuf).
The only remaining call is for bump_role_version when adding new targets. For this one, I need to think of a way to avoid degrading the performance.

[lukpueh]

From a TUF perspective it is enough to bump snapshot only once after having updated all targets metadata (targets, bins, bin-n). And from a performance perspective it would be a good idea too.

I guess the reason why you bump snapshot here is to write the new snapshot to storage, so that delegate_targets_roles below, which loads snapshot from storage doesn't have the old version. Is that correct?

[kairoaraujo]

Yes and no. One of the first implementations I did, the delegated_target_roles, has the metadata_snapshot as optional (to load or not). Further, I decided to have it more explicit and registered in the snapshots versions for each set of delegations (targets v1 -> bins v2 -> bins-n v3). Should I revert it?

[lukpueh]

What would be the benefit of multiple versions?

[kairoaraujo]

Not really. I just did it to test it and see if my steps were correct.

[lukpueh]

I suggest to revert it then.

[lukpueh]

I just realized that delegate_targets_roles takes a dictionary of {"<delegator>": ["<delegatee>", ...]}. So you could actually build your delegate_roles_payload with "targets" and "bins" and call delegate_targets_roles only once (and bump_snapshot) afterwards.

[lukpueh]

What's the benefit of a state property self.is_initialized, if we don't tust it to be up-to-date?

[kairoaraujo]

That isn't very clear, I agree.
What do you think about merging all to self.is_initialized and giving the status in the runtime? Or any suggestion?
example:

    @property
    def is_initialized(self) -> bool:
        """State of repository

        Returns:
            bool
        """
        try:
            if any(
                role
                for role in TOP_LEVEL_ROLE_NAMES
                if isinstance(self.load_role(role), Metadata)
            ):
                return True
        except StorageError as err:
            if "Can't open" in str(err):
                return False

        return False

[lukpueh]

I think this is a lot better, if we want to stick to LBYL. Generally I wonder if we can't just go EAFP here.

[kairoaraujo]

Don't we go EAFP here? 🤔I try to load any TOP_LEVEL_ROLE_NAMES without checking if the files exist?

[lukpueh]

That's true. But I'm thinking about places where you will use this property.

LYBL:

if is_initialized:
  use_metadata()
else:
  handle_unitialized_repo()

EAFP:

try:
  use_metadata()
except StorageError:
  handle_unitialized_repo() # Omit try/except if no need to handle

[kairoaraujo]

OK, I thought about the suggestion I shared.
For example, it wouldn't protect the repository initialization (initialize()).

[lukpueh]

Looks like this one could just call bump_role_version?

[kairoaraujo]

Same as above, I did it to be more explicit. But I'm open.

[lukpueh]

I think it's different than in timestamp_bump_version, because here you don't assign snapshot's meta field, so it's pretty much the same functionality as bump_role_version. But we don't have to change this now.

[lukpueh]

Any keypair really, also for non-top-level roles, bins and bin-n, right?

I can quickly polish these docstrings, if you want.

[kairoaraujo]

Yes. It can generate any key keypair

[lukpueh]

Summarizing some architectural discussions I had with @kairoaraujo on slack:

The current design implements Warehouse-specific TUF app code in tuf.services and a generic TUF repo abstraction in tuf.repository.

The intention is to hide TUF metadata handling from Warehouse. And, in the future when better TUF repository tooling is available replace the repo abstraction by such tooling (see theupdateframework/python-tuf#1136).

But it seems hard to draw a clear line between application and repository responsibilities:

• Although the app code doesn't directly interact with the Metadata API, it provides most of its inputs and passes them to the intermediary repo code in a format that is already very similar to the TUF Metadata API classes (see RolesPayload, TargetsPayload), which makes the blackbox argument less strong.
• Inter-dependent metadata updates prescribed by the TUF spec currently occur in app and repo code. E.g. "update timestamp when snaphshot is updated" (app), or "update snaphsot when targets is updated" (repo).
• Metadata needs to be loaded from storage in both app and repo, but also passed back and forth with multiple side-effects (in storage and in memory).

The added complexity of the repository abstraction together with an unclear distribution of responsibilities makes it harder to review the correctness of the implementation in terms of PEP and TUF spec compliance.

Maybe it would be easier to remove the abstraction and implement all in app code? The two relevant classes RepositoryService (app) and MetadataRepository (repo) already have a 1-1 relationship and look a lot alike in terms of attributes.

All that said, we decided that we will stick to the current design, and wait for feedback from upstream.

(cc @jku, who has spent a lot of thoughts on TUF repository abstractions.)

[lukpueh]

Minor nit: This is the only docstring in this module, and the tasks all call into service methods, which are well documented. Can we just remove this docstring here?