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.

Sign up for GitHub

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

Jump to bottom

feat: add payload and add-signature commands. #214

Merged
merged 10 commits into from May 9, 2022
Merged

feat: add payload and add-signature commands. #214

merged 10 commits into from May 9, 2022

Conversation

znewman01
Copy link
Contributor

Fixes #205.

@coveralls
Copy link

coveralls commented Feb 1, 2022 •
edited

Pull Request Test Coverage Report for Build 2049224869

  • 26 of 32 (81.25%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 71.384%
Changes Missing Coverage Covered Lines Changed/Added Lines %
errors.go 0 1 0.0%
repo.go 26 31 83.87%
Files with Coverage Reduction New Missed Lines %
repo.go 1 76.09%
Totals Coverage Status
Change from base Build 2029695489: 0.1%
Covered Lines: 2295
Relevant Lines: 3215
💛 - Coveralls

@znewman01 znewman01 force-pushed the offline branch 2 times, most recently from 6e19e18 to b15910d Compare February 1, 2022 16:35
@znewman01
Copy link
Contributor Author

ping @asraa (not to nag/rush, just want to make sure it's on your radar)

@znewman01
Copy link
Contributor Author

Oh also CC @haydentherapper

rdimitrov
rdimitrov reviewed Feb 15, 2022
View reviewed changes
repo.go Show resolved Hide resolved
@trishankatdatadog
Copy link
Member

Let's try to review/merge/close this soon. What are we blocking on?

@znewman01
Copy link
Contributor Author

I've pushed a couple commits to fix the issues mentioned; of note, I add a sign-payload command which is the "offline tool" we've been dancing around.

@znewman01 znewman01 force-pushed the offline branch 2 times, most recently from 07df8d8 to 3cde0d7 Compare March 27, 2022 22:54
rdimitrov
rdimitrov reviewed Mar 30, 2022
View reviewed changes
Copy link
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

I'd also suggest updating the list of commands in the README.md file by adding their descriptions along with examples of how to use them 👍

repo.go Outdated Show resolved Hide resolved
repo.go Show resolved Hide resolved
repo.go Show resolved Hide resolved
@znewman01
Copy link
Contributor Author

I'd also suggest updating the list of commands in the README.md file by adding their descriptions along with examples of how to use them 👍

Great idea. I've added them to the "Commands" section, along with an example of using these commands together in the "Examples" section.

@znewman01 znewman01 mentioned this pull request Mar 31, 2022
Merged
5 tasks
asraa
asraa previously approved these changes Apr 1, 2022
View reviewed changes
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 some minor comments!

README.md Show resolved Hide resolved
repo.go Show resolved Hide resolved
@znewman01 znewman01 mentioned this pull request Apr 5, 2022
Closed
rdimitrov
rdimitrov previously approved these changes Apr 5, 2022
View reviewed changes
repo.go Outdated Show resolved Hide resolved
@znewman01 znewman01 requested a review from asraa April 11, 2022 16:26
@znewman01
Copy link
Contributor Author

No substantial changes but looks like I need a re-review, @asraa

Otherwise I think this is good to go except I need one more approval from a maintainer—@trishankatdatadog any chance you could give a quick review?

@trishankatdatadog
Copy link
Member

Otherwise I think this is good to go except I need one more approval from a maintainer—@trishankatdatadog any chance you could give a quick review?

Sorry, no time 😕 Could someone else from DD take over (@ethan-lowman-dd @hosseinsia)?

asraa
asraa reviewed Apr 13, 2022
View reviewed changes
README.md Show resolved Hide resolved
asraa
asraa previously approved these changes Apr 13, 2022
View reviewed changes
@znewman01 znewman01 dismissed stale reviews from asraa and rdimitrov via 2ef1544 April 17, 2022 23:39
@znewman01
Copy link
Contributor Author

Need quick re-reviews again

rdimitrov
rdimitrov previously approved these changes Apr 19, 2022
View reviewed changes
@trishankatdatadog
Copy link
Member

Need quick re-reviews again

@cedricvanrompay-datadog would you pls mind reviewing?

On that note, I think it's worth nominating @znewman01 and @cedricvanrompay-datadog as additional maintainers. What do the @theupdateframework/go-tuf-maintainers think?

asraa
asraa previously approved these changes Apr 19, 2022
View reviewed changes
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.

Thanks for the test that Payload is canonicalized!

@asraa
Copy link
Contributor

asraa commented Apr 19, 2022

On that note, I think it's worth nominating @znewman01 and @cedricvanrompay-datadog as additional maintainers. What do the https://github.com/orgs/theupdateframework/teams/go-tuf-maintainers think?

Def +1 to Zack, solid reviews, issue filing, and PRs.

@znewman01
feat: Add payload and add-signature commands.
d3c12fc 
Fixes theupdateframework#205.
@znewman01
docs: Clarify payload and add-signature args.
132ef82 
Specifically, they expect a metadata file name, *not* a role name.

Added a test for each.
@znewman01
feat: Add sign-payload command.
4a2f608 
This completes the offline flow:

```shell
tuf payload root.json > /tmp/root.json.payload
tuf sign-payload --role=root /tmp/root.json.payload > /tmp/root.json.sigs
tuf add-signatures --signatures /tmp/root.json.sigs root.json
```

Additional changes:
- rename `add-signature` to `add-signatures`
- `add-signatures` expects JSON (from `sign-payload`) rather than hex bytes
@znewman01
docs: Beef up documentation for offline signature flow.
2197d3b 
- move CLI commands to matching file names
- add examples to README.md
- more details for `repo.SignPayload` docs
@znewman01
docs: Point out where keys are stored in sign-payload docs
23cbb2e
@znewman01
fix: ensure that output is canonicalized
3854498
@znewman01
style: rename ErrInsufficientKeys to ErrNoKeys
5610ecd
@znewman01
doc: minor tuf sign-payload clarifiation
4ec9a54
@znewman01
test: add client test for offline flow
a0bb510
@znewman01
test: fix tests after rebase
1748ea5
@znewman01 znewman01 dismissed stale reviews from asraa and rdimitrov via 1748ea5 April 25, 2022 13:33
@znewman01
Copy link
Contributor Author

I had to rebase on top of the delegations changes, so need new review. Very little difference from before.

@znewman01
Copy link
Contributor Author

@cedricvanrompay-datadog it won't let me re-request a review for you for some reason so tagging you in manually

@znewman01
Copy link
Contributor Author

Gentle ping @cedricvanrompay-datadog @asraa

@znewman01 znewman01 merged commit 4bf58eb into theupdateframework:master May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haydentherapper haydentherapper haydentherapper left review comments

@cedricvanrompay-datadog cedricvanrompay-datadog cedricvanrompay-datadog left review comments

@asraa asraa asraa approved these changes

@rdimitrov rdimitrov rdimitrov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TUF CLI: Improve experience by allowing offline signature generation
7 participants
@znewman01 @coveralls @trishankatdatadog @cedricvanrompay-datadog @asraa @haydentherapper @rdimitrov