Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support delegated targets roles in repo writer #175

Merged
merged 48 commits into from
Apr 19, 2022

Conversation

mnm678
Copy link
Collaborator

@mnm678 mnm678 commented Nov 16, 2021

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
Copy link

coveralls commented Nov 16, 2021

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.

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 Coverage Status
Change from base Build 2053262824: 1.3%
Covered Lines: 2539
Relevant Lines: 3498

💛 - Coveralls

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

data/types.go Outdated Show resolved Hide resolved
local_store.go Outdated Show resolved Hide resolved
local_store.go Outdated Show resolved Hide resolved
@ethan-lowman-dd
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

mnm678 commented Nov 17, 2021

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
Copy link
Member

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

@mnm678
Copy link
Collaborator Author

mnm678 commented Nov 19, 2021

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

repo.go Outdated Show resolved Hide resolved
@mnm678
Copy link
Collaborator Author

mnm678 commented Nov 29, 2021

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
Copy link
Contributor

ethan-lowman-dd commented Dec 1, 2021

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.

@trishankatdatadog trishankatdatadog mentioned this pull request Dec 2, 2021
@ethan-lowman-dd
Copy link
Contributor

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
Copy link
Member

@lukpueh do you think you can help review?

@ethan-lowman-dd
Copy link
Contributor

ethan-lowman-dd commented Dec 3, 2021

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
Copy link
Collaborator Author

mnm678 commented Dec 6, 2021

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.

local_store.go Outdated Show resolved Hide resolved
repo.go Outdated Show resolved Hide resolved
repo.go Show resolved Hide resolved
@rdimitrov
Copy link
Contributor

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
Copy link
Contributor

@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.

ethan-lowman-dd added a commit that referenced this pull request Dec 10, 2021
ethan-lowman-dd added a commit that referenced this pull request Dec 10, 2021
ethan-lowman-dd added a commit that referenced this pull request Dec 10, 2021
ethan-lowman-dd added a commit that referenced this pull request Dec 10, 2021
ethan-lowman-dd added a commit that referenced this pull request Dec 10, 2021
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

repo.go Show resolved Hide resolved
Copy link
Contributor

@hosseinsia hosseinsia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ethan-lowman-dd ethan-lowman-dd merged commit fd8ac04 into theupdateframework:master Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants