feat: Support delegated targets roles in repo writer

[mnm678]

This pr is a takeover of #171, which I don't seem to be able to edit directly.

cc @ethan-lowman-dd @asraa @trishankatdatadog @hosseinsia

[coveralls]

Pull Request Test Coverage Report for Build 2061709688

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

• 379 of 469 (80.81%) changed or added relevant lines in 4 files are covered.
• 87 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+1.3%) to 72.584%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
errors.go	0	3	0.0%
repo.go	377	464	81.25%

Files with Coverage Reduction	New Missed Lines	%
client/errors.go	33	7.84%
repo.go	54	78.6%

Totals
Change from base Build 2053262824:	1.3%
Covered Lines:	2539
Relevant Lines:	3498

---

💛 - Coveralls

[asraa]

Just started doing the review, just got caught up a little in the key generation not being compatible with delegations -- right now i think it is, except for the hashed bin delegations? (i think it should end up saving the keys in a role file, possibly duplicating them)

[ethan-lowman-dd]

I haven't looked at the parts of this I wrote in a while, so I'm unfortunately a bit rusty on what's unfinished, but this is the testing script I was using (haven't converted it to unit tests yet): https://gist.github.com/ethan-lowman-dd/cbc403cd0e68215703f903b7b8777c9c

Running this script on the current branch, regular delegated role signing seems to work. Key management for hash bin delegated signing is not working -- the leaf signer is not being fetched properly, so no signature is written.

There's also a potential issue with delegation traversal, but I need to read the spec closer to verify -- in the test, A.txt can not be signed since there is no explicit delegation for its path, but I think the top level targets role should sign in that case.

Additionally, I think it's still unaddressed how hash bin delegated keys would be loaded after the delegation is first written. In the test script, we generate the leaf key before we use it, so it's cached in memory by key ID. However, since the hash bins role name leaf doesn't appear anywhere in TUF metadata (its metadata is named bins_XX.json, not leaf.json), when we load the repo a second time, we have no handle to the leaf key on disk. One way to fix this would be to require clients to manually load delegated keys before signing with them, but that's perhaps not so friendly. I would lean towards something like storing keys on disk by their key ID rather than their role name, but that's a pretty big breaking change. Whatever the solution, it should ideally work for remote HSM signers too.

[ethan-lowman-dd]

Ah @asraa good point, another way to make the disk-backed key store work with delegated roles would be to just write it to disk under each of bins_00...bins_ff for example -- n copies of the key for n bins. That actually might be the easiest.

[mnm678]

Just started doing the review, just got caught up a little in the key generation not being compatible with delegations -- right now i think it is, except for the hashed bin delegations? (i think it should end up saving the keys in a role file, possibly duplicating them)

Yes, I was also running into this in tuf-notary. You can use the keys.GenerateEd25519Key(), etc, but not the helper functions. That would be a good addition here.

[trishankatdatadog]

Going to defer to the rest here. Great job, everyone!

[mnm678]

Thanks for the test script @ethan-lowman-dd. I think I fixed the AddTarget bug.

[mnm678]

Could I get another review @ethan-lowman-dd or @asraa? The hashed-bin key storage seems to be working as long as the keys are each stored (bins_0-1, etc). There might be a more elegant way to automate this, but I think we can leave that to a future pr.

[ethan-lowman-dd]

I pushed some failing tests -- currently, targets metadata that does not have any target files is not signed. I'm currently debugging.

Also, some notes for myself -- all of my printf debugging in repo.go needs to be removed still, and there's still some commented-out code. In fact, the commented out code in SnapshotWithExpires is the reason unsigned manifests are being successfully snapshotted. SetThreshold also does not support delegations yet.

[ethan-lowman-dd]

I fixed the issues I mentioned above. I think it may be okay if the SetThreshold modifier does not support delegation for now -- it can probably be a separate PR. I'll try to find some time today or tomorrow to work on more cleanup and tests.

[trishankatdatadog]

@lukpueh do you think you can help review?

[ethan-lowman-dd]

Any thoughts on whether delegated targets key revocation needs to go in this PR, or whether we can we leave those changes as TODOs? I wouldn't consider the code useable in production without APIs to revoke keys, but I also don't want to bloat this PR with mostly-independent changes.

[mnm678]

Any thoughts on whether delegated targets key revocation needs to go in this PR, or whether we can we leave those changes as TODOs? I wouldn't consider the code useable in production without APIs to revoke keys, but I also don't want to bloat this PR with mostly-independent changes.

I think we can update all of the add/revoke key functions in a separate pr to keep this one a bit smaller/easier to review. But I agree that we should get those done before a release that includes this pr.

[rdimitrov]

Just a general comment, but it would be really great for go-tuf if we can try to keep the PRs simpler and independent as much as possible so they are easy to review. For example, If there's some refactoring related to introducing a new feature, try to separate it in a different PR. Trying to make sense of a lot of changes put in a single PR is usually only feasible for people with very in-depth knowledge of the whole codebase. TUF, itself, is not an easy topic, so avoiding this would help in the direction of welcoming more people to review (and even contribute to) the project which would also limit the chance of introducing code that was not thought through well enough 👍

[ethan-lowman-dd]

@rdimitrov Since supporting delegated roles touches almost everywhere roles are used, it's been tricky to keep this change compact. However, I think at this point we're confident enough in the necessary refactors that it makes sense to separate them out as you suggested. I'll spend some time on this today.

[asraa]

Let's go! Really want to start getting this code in, and we can tackle the cleanups afterwards. If need be let's make a milestone tracking issue for all the tasks we need to do so we can sort out who can own each.

[hosseinsia]

LGTM