Support for fetching Fulcio certs with self-managed key #2532

AnishShah · 2022-12-10T09:15:53Z

Summary

Added a new flag --issue-certificate to sign commands that allows users to fetch Fulcio certificate with self-managed key

Fixes #2398

Release Note

Added a new flag `--issue-certificate` in sign commands that allows users to fetch Fulcio certificate with a self-managed key.

Documentation

Yes

codecov-commenter · 2022-12-10T09:18:17Z

Codecov Report

Merging #2532 (340a823) into main (4797501) will increase coverage by 0.27%.
The diff coverage is 15.38%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2532      +/-   ##
==========================================
+ Coverage   30.00%   30.27%   +0.27%     
==========================================
  Files         146      146              
  Lines        9299     9323      +24     
==========================================
+ Hits         2790     2823      +33     
+ Misses       6077     6063      -14     
- Partials      432      437       +5

Impacted Files	Coverage Δ
cmd/cosign/cli/options/sign.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/signblob.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/sign.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/sign/sign.go	`14.88% <0.00%> (-0.91%)`	⬇️
cmd/cosign/cli/signblob.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/fulcio/fulcio.go	`45.00% <77.77%> (+28.46%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

haydentherapper

Thanks for working on this!

Can you also show that this has been tested in a PR comment? I'd try calling sign and verify with various key types

cmd/cosign/cli/options/key.go

cmd/cosign/cli/sign/sign.go

cmd/cosign/cli/options/signblob.go

cmd/cosign/cli/fulcio/fulcio.go

haydentherapper

This looks great! Can we just add some unit tests for NewSigner

cmd/cosign/cli/sign/sign.go

haydentherapper · 2023-01-10T23:36:21Z

I'm rebasing this PR and adding tests

AnishShah · 2023-01-11T00:33:48Z

Thanks @haydentherapper. I was planning to take this up this weekend. But I will appreciate the help as I'm not familiar with adding tests.

haydentherapper · 2023-01-11T22:28:51Z

@AnishShah, I added some tests for NewSigner, feel free to make any other updates! Thanks!

AnishShah · 2023-01-12T18:39:59Z

Thanks @haydentherapper. Tests LGTM. I had also added e2e test. Let me know if you have any feedback on that.

haydentherapper

It looks good, thanks!

haydentherapper · 2023-01-12T18:42:27Z

Oh huh, I didn't know I could approve a PR that I contributed to. That seems...risky.

znewman01

Thank you so much for this change! A few nits (docs, naming, refactoring). LMK if you think they make sense.

cmd/cosign/cli/options/sign.go

cmd/cosign/cli/fulcio/fulcio.go

cmd/cosign/cli/sign/sign.go

cmd/cosign/cli/options/signblob.go

cmd/cosign/cli/options/key.go

AnishShah · 2023-01-25T04:07:01Z

@znewman01 @haydentherapper Ready for another review. PTAL.

haydentherapper · 2023-01-26T01:52:42Z

@AnishShah, I don't see the changes that @znewman01 suggested.

AnishShah · 2023-01-26T01:55:30Z

Hmm not sure. You don't see last two commits - "Refactor sign.SignerFromKeyOpts"?

haydentherapper · 2023-01-26T02:03:14Z

Sorry, i do see that now! Will take a look

haydentherapper

Nice work on this! Thanks for making the changes.

Cc @znewman01 for approval and merge

haydentherapper · 2023-02-02T18:02:42Z

@AnishShah youll just need to merge or rebase from HEAD too

Added a new flag --issue-certificate to sign commands that allows users to fetch Fulcio certificate with self-managed key Signed-off-by: Anish Shah <anishshah@google.com>

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

AnishShah · 2023-02-02T18:51:50Z

@haydentherapper Done

haydentherapper · 2023-02-02T18:58:06Z

Just one issue, cmd/cosign/cli/sign/sign.go:536:6: undefined: ui.Info

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Signed-off-by: Anish Shah <anishshah@google.com>

znewman01

This change is great, thank you so much! Code looks better than when you started 😄

* Support for fetching Fulcio certs with self-managed key Added a new flag --issue-certificate to sign commands that allows users to fetch Fulcio certificate with self-managed key Signed-off-by: Anish Shah <anishshah@google.com> * add tests for newsigner Signed-off-by: Hayden Blauzvern <hblauzvern@google.com> * Fix lint and nit Signed-off-by: Hayden Blauzvern <hblauzvern@google.com> * remove commented out code Signed-off-by: Hayden Blauzvern <hblauzvern@google.com> * Refactor sign.SignerFromKeyOpts Signed-off-by: Anish Shah <anishshah@google.com> * Fix lint issues Signed-off-by: Anish Shah <anishshah@google.com> --------- Signed-off-by: Anish Shah <anishshah@google.com> Signed-off-by: Hayden Blauzvern <hblauzvern@google.com> Co-authored-by: Hayden Blauzvern <hblauzvern@google.com>

AnishShah force-pushed the issue-2398 branch 6 times, most recently from 3251570 to 0b3c6ea Compare December 11, 2022 02:58

haydentherapper requested changes Dec 12, 2022

View reviewed changes

haydentherapper reviewed Dec 12, 2022

View reviewed changes

cmd/cosign/cli/fulcio/fulcio.go Outdated Show resolved Hide resolved

AnishShah force-pushed the issue-2398 branch 4 times, most recently from 703b4d7 to 8c39c6b Compare December 13, 2022 07:42

haydentherapper reviewed Dec 13, 2022

View reviewed changes

cmd/cosign/cli/sign/sign.go Show resolved Hide resolved

haydentherapper force-pushed the issue-2398 branch from 8c39c6b to fd6b921 Compare January 10, 2023 23:35

haydentherapper requested review from asraa, znewman01 and priyawadhwa January 11, 2023 22:28

haydentherapper previously approved these changes Jan 12, 2023

View reviewed changes

znewman01 suggested changes Jan 14, 2023

View reviewed changes

AnishShah dismissed haydentherapper’s stale review via ea60952 January 22, 2023 22:45

AnishShah force-pushed the issue-2398 branch 3 times, most recently from a7aa5b7 to 89eac5a Compare January 22, 2023 23:17

haydentherapper previously approved these changes Feb 2, 2023

View reviewed changes

AnishShah and others added 2 commits February 2, 2023 18:49

Support for fetching Fulcio certs with self-managed key

ec694ed

Added a new flag --issue-certificate to sign commands that allows users to fetch Fulcio certificate with self-managed key Signed-off-by: Anish Shah <anishshah@google.com>

add tests for newsigner

8f78817

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

AnishShah dismissed haydentherapper’s stale review via 8f663d5 February 2, 2023 18:51

AnishShah force-pushed the issue-2398 branch from 89eac5a to 8f663d5 Compare February 2, 2023 18:51

haydentherapper and others added 4 commits February 2, 2023 18:58

Fix lint and nit

c64789d

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

remove commented out code

3e0e9c2

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Refactor sign.SignerFromKeyOpts

bbe307a

Signed-off-by: Anish Shah <anishshah@google.com>

Fix lint issues

340a823

Signed-off-by: Anish Shah <anishshah@google.com>

AnishShah force-pushed the issue-2398 branch from 8f663d5 to 340a823 Compare February 2, 2023 18:59

haydentherapper approved these changes Feb 2, 2023

View reviewed changes

znewman01 approved these changes Feb 8, 2023

View reviewed changes

znewman01 merged commit abb50cf into sigstore:main Feb 8, 2023

github-actions bot added this to the v1.14.0 milestone Feb 8, 2023

AnishShah deleted the issue-2398 branch February 9, 2023 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for fetching Fulcio certs with self-managed key #2532

Support for fetching Fulcio certs with self-managed key #2532

AnishShah commented Dec 10, 2022 •

edited

Loading

codecov-commenter commented Dec 10, 2022 •

edited

Loading

haydentherapper left a comment

haydentherapper left a comment

haydentherapper commented Jan 10, 2023

AnishShah commented Jan 11, 2023

haydentherapper commented Jan 11, 2023

AnishShah commented Jan 12, 2023

haydentherapper left a comment

haydentherapper commented Jan 12, 2023

znewman01 left a comment

AnishShah commented Jan 25, 2023

haydentherapper commented Jan 26, 2023

AnishShah commented Jan 26, 2023

haydentherapper commented Jan 26, 2023

haydentherapper left a comment

haydentherapper commented Feb 2, 2023

AnishShah commented Feb 2, 2023

haydentherapper commented Feb 2, 2023

znewman01 left a comment

Support for fetching Fulcio certs with self-managed key #2532

Support for fetching Fulcio certs with self-managed key #2532

Conversation

AnishShah commented Dec 10, 2022 • edited Loading

Summary

Release Note

Documentation

codecov-commenter commented Dec 10, 2022 • edited Loading

Codecov Report

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Jan 10, 2023

AnishShah commented Jan 11, 2023

haydentherapper commented Jan 11, 2023

AnishShah commented Jan 12, 2023

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Jan 12, 2023

znewman01 left a comment

Choose a reason for hiding this comment

AnishShah commented Jan 25, 2023

haydentherapper commented Jan 26, 2023

AnishShah commented Jan 26, 2023

haydentherapper commented Jan 26, 2023

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Feb 2, 2023

AnishShah commented Feb 2, 2023

haydentherapper commented Feb 2, 2023

znewman01 left a comment

Choose a reason for hiding this comment

AnishShah commented Dec 10, 2022 •

edited

Loading

codecov-commenter commented Dec 10, 2022 •

edited

Loading