vmware-archive / repository-editor-for-tuf

Command line tool for editing and maintaining a TUF repository
Apache License 2.0
5 stars 3 forks source link

Add support for succinct hash bin delegation #34

Closed MVrachev closed 2 years ago

MVrachev commented 2 years ago

Note this pr is dependant on #33

With this pr I add support for succinct hash bin delegation.

The pr introduces two commands:

  1. tufrepo edit X add-delegation --succinct <options> edits targets role X, making the delegation into a succinct one
  2. tufrepo init-succinct-roles X reads the delegation and initializes all the delegated metadata files

There are additional commits that are aiming to simplify or improve the codebase.

MVrachev commented 2 years ago

The tests are failing due to python-tuf not yet releasing a new version that will add support for succinct hash bin delegation.

When such a release is made I will make this pr non-draft.

jku commented 2 years ago

Looks pretty good. I'll leave a naming comment already:

  1. tufrepo edit X add-delegation --succinct <options> edits targets role X, making the delegation into a succinct one

this does not seem to be true... you actually have tufrepo edit X add-succinct-delegation ... and a separate tufrepo edit X add-delegation for paths and hash_prefixes. I think I still prefer a single command with options to switch between the three modes (paths/hash_prefixes/succinct) but I'm willing to be persuaded.

Also, I think there's no good default name prefix: there's nothing special about "bin" and adding two succinct delegations with default name to the same repo would or should fail anyway. It seems reasonable to me to make both number and name-prefix required when adding a succinct delegation.

jku commented 2 years ago

WRT succinct CLI option: according to click docs this should give you a Tuple[str, int]: @click.option('--succinct', type=(str, int)) that should allow tufrepo edit X add-delegation --succinct bin 32: not amazing UI but not horrible

jku commented 2 years ago
@edit.command()
@click.pass_context
@click.option("--terminating/--non-terminating", default=False)
@click.option("--path", "paths", multiple=True)
@click.option("--hash-prefix", "hash_prefixes", multiple=True)
@click.option("--succinct", "bin_count", type=int, default=0)
@click.argument("delegate")
def add_delegation(
    ctx: Context,
    terminating: bool,
    paths: Tuple[str],
    hash_prefixes: Tuple[str],
    bin_count: int,
    delegate: str,
):

still mulling this over... the above could work: the only surprising thing here is that in the succinct case "delegate" is the name prefix for the delegate, and I think that makes some sense:

tufrepo edit X add-delegate --path "a/b" --path "c/d" delegaterole
tufrepo edit X add-delegate --hash-prefix "00" --hash-prefix "01" delegaterole
tufrepo edit X add-delegate --succinct 32 bin

these look reasonable to me...

MVrachev commented 2 years ago

@jku I followed your suggestion and made add-delegation, so it can be used both for adding a new delegated role and adding succinct hash bin delegation. Additionally, I noticed some small stuff that needed polishing when testing a command that we expect to fail. See https://github.com/vmware-labs/repository-editor-for-tuf/pull/34/commits/f6a48d156fbca3dab20803254f8d13776fa67814

jku commented 2 years ago

Additionally, I noticed some small stuff that needed polishing when testing a command that we expect to fail.

I don't fully agree with that change... We can merge this but for future:

I would suggest to use e.g. ClickException to get reasonable output on input validation failure instead (see examples in e.g. verifier).

MVrachev commented 2 years ago

I am going to propose a pr addressing all of those comments.

I have a question you said:

there is a check for powers of two that is broken

In which case is broken?

jku commented 2 years ago

there is a check for powers of two that is broken

In which case is broken?

Most cases, (i % 2) is just not a check for powers of two.

MVrachev commented 2 years ago

Ohh... myyy bad. You are correct. I played myself my testing with an odd number - 3 instead of testing with an even number that it's not a power of 2.