ng client: support for prefix_targets_with_hash when downloading targets

[MVrachev]

Fixes #1479

Description of the changes being introduced by the pull request:

ng client: support for prefix_targets_with_hash

Add support for prefixing targets with their hashes when downloading or
using HASH.FILENAME.EXT as target names.
The introduction of prefix_targets_with_hash was necessary, because
there are use cases like Warehouse where you could use
consistent_snapshot, but without adding a hash prefix to your targets.

When prefix_targets_with_hash is set to True, target files conforming
the format HASH.FILENAME.EXT will be downloaded from the server, but
they will be saved on the client side without their hash prefixes or
FILENAME.EXT.
This makes sure the client won't understand the usage of
prefix_targets_with_hash.

Still, if you want to use HASH.FILENAME.EXT as target names when
downloading, then additionally you need to provide consistent_snapshot
set to True in your root.json. The reason is that the specification uses
consistent_snapshot for the same purpose:

If consistent snapshots are not used (see § 6.2 Consistent snapshots),
then the filename used to download the target file is of the fixed form
FILENAME.EXT (e.g., foobar.tar.gz). Otherwise, the filename is of the
form HASH.FILENAME.EXT
(e.g., c14aeb4ac9f4a8fc0d83d12482b9197452f6adf3eb710e3b1e2b79e8d14cb681.foobar.tar.gz),
where HASH is one of the hashes of the targets file listed in the
targets metadata file found earlier in step § 5.6 Update the targets role.
In either case, the client MUST write the file to non-volatile
storage as FILENAME.EXT.

See chapter 5.7.3:
https://theupdateframework.github.io/specification/latest/index.html#fetch-target
By default, prefix_targets_with_hash is set to true to make it easier
to the user to provide uniquely identifiable targets file names by
using consistent_snapshot set to True.

The same behavior of using two flags is used in the legacy code when
calling tuf.client.updater.download_target() in a repository using
prefix_targets_with_hash and consistent_snapshot:
https://github.com/theupdateframework/tuf/blob/develop/tuf/client/updater.py#L1302

Signed-off-by: Martin Vrachev mvrachev@vmware.com

Please verify and check that the pull request fulfills the following
requirements:

• 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

[MVrachev]

I will need more time investigating and when I am ready I will update this pr and make it non-draft.

[MVrachev]

This pr is ready for review.
I have changed the way I construct the consistent target name.

More importantly, I made sure that when consistent_snapshot is True, in download_target we are
downloading target files with names conforming to the schema HASH.FILENAME.EXT, BUT when
they are saved on the client side they are saved with their non-consistent names or FILENAME.EXT.
That's what the current code does when calling get_target_file:
https://github.com/theupdateframework/tuf/blob/8e9677d2620c937e3a64e5420e7386f2464f26bd/tuf/client/updater.py#L3183
but the target file itself is saved with the name destination which is constructed without the hash prefix:
constructing destination:
https://github.com/theupdateframework/tuf/blob/8e9677d2620c937e3a64e5420e7386f2464f26bd/tuf/client/updater.py#L3161
persisting the temporary file:
https://github.com/theupdateframework/tuf/blob/8e9677d2620c937e3a64e5420e7386f2464f26bd/tuf/client/updater.py#L3186

That way the usage of consistent_snapshot doesn't affect the client in any way.

[sechkova]

Please reword the commit.

[MVrachev]

I squashed my commits and made the following changes:

• found out that I am not setting consistent_snapshot to true in any of my tests and I am not actually testing downloading target names in the format HASH.FILENAME.EXT. Fixed that by modifying the root bytes, creating a new trusted metadata set and changing the updater internals. Not a great solution, but I think it's better than modifying the client directory used by the rest of the tests.
• reword my commits
• simplified a couple of places thanks to @sechkova suggestions.

[MVrachev]

Addressed the new comments made by @sechkova.

[sechkova]

You keep using this delegator to get the hashes, I keep disagreeing. We need a second opinion.

[MVrachev]

I am sorry it seemed that I disagree. I am just not sure how can I get information about hashes that needs to be calculated for a particular delegation if I don't access the delegator.
First, I thought you meant I didn't need to instantiate a Metadata instance in order to solve this problem and that's what I changed, but it seems like you meant something else.
It looks like I am missing something.
Can you share more how can I simplify this without accessing the delegator?

[MVrachev]

I removed the need of accessing the delegator file by swapping the places of get_one_valid_targetinfo() and _create_consistent_target().
After calling get_one_valid_targetinfo I have the information about the target hashes and I can pass it to _create_consistent_target() and that's what I did in my last commit.

[sechkova]

Looks fine to me now.

[jku]

Code part looks good. Could still have a look at variable use but these are nitpicks:

• why is target_fileinfo declared but then not used four lines later
• should filepath be renamed so it's clear that it's the path for local file

The test code looks a bit bad in that it's adding 80 lines of fairly complex code for a simple test that doesn't even check that the files it downloaded are the correct ones (half of the code is copy-paste as well)... but fixing this well probably needs a redesign of the whole test architecture so I'm not asking for changes here apart from the comment WRT modifying Updater internals.

[MVrachev]

Code part looks good. Could still have a look at variable use but these are nitpicks:

• why is target_fileinfo declared but then not used four lines later
• should filepath be renamed so it's clear that it's the path for local file

The test code looks a bit bad in that it's adding 80 lines of fairly complex code for a simple test that doesn't even check that the files it downloaded are the correct ones (half of the code is copy-paste as well)... but fixing this well probably needs a redesign of the whole test architecture so I'm not asking for changes here apart from the comment WRT modifying Updater internals.

I updated the pr so that:

• the test doesn't modify an internal state but instead creates a new root file with consistent_snapshot set to True
• changed the UpdaterConfig docstring
• used targetinfo where you pointed out
• renamed filepath to local_filepath
• rebased on top of develop

Maybe you can share what do you think could be simplified and we can submit an issue or directly the following pr?

[jku]

Maybe you can share what do you think could be simplified and we can submit an issue or directly the following pr?

I (and others) have made some notes here #1444 -- that one is about generated test data in general but there are comments wrt the other end of the spectrum (vast amounts of metadata that is at least mostly static), maybe no need to file new issues? We'll need to prototype some approaches to see if they make sense...

[MVrachev]

Yep, I agree with you we need prototypes and more discussions.
I added a test here even though it's not beautiful in order to make sure we don't forget to test the new use cases
we are supporting when implementing new features.

[jku]

Thanks. The UpdaterConfig docs should still be improved but lets not stop this PR with that either... the code looks fine to me.