feat: add sign blob commands #856

rrsemlani · 2023-12-26T22:31:04Z

Adding sign blob commands.
Spec Pr used for ref: #811

Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

codecov-commenter · 2023-12-26T23:34:15Z

Codecov Report

Attention: Patch coverage is 88.54167% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 64.45%. Comparing base (31c9e84) to head (2ed55c6).
Report is 74 commits behind head on main.

❗ Current head 2ed55c6 differs from pull request most recent head 696d4a4. Consider uploading reports for the commit 696d4a4 to get more accurate results

Files	Patch %	Lines
cmd/notation/blob.go	88.29%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #856      +/-   ##
==========================================
+ Coverage   64.09%   64.45%   +0.35%     
==========================================
  Files          40       46       +6     
  Lines        2284     2788     +504     
==========================================
+ Hits         1464     1797     +333     
- Misses        698      835     +137     
- Partials      122      156      +34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rgnote · 2023-12-28T21:37:34Z

cmd/notation/main.go

@@ -35,6 +35,7 @@ func main() {
 	}
 	cmd.AddCommand(
 		signCommand(nil),
+		blobSignCommand(nil),


nit: put all blob commends at one place for readability

rgnote · 2023-12-28T21:41:35Z

cmd/notation/blob.go

+	"github.com/spf13/cobra"
+)
+
+func blobSignCommand(opts *signOpts) *cobra.Command {


signOpts defined in sign.go has more flags than what is needed for notation blob sign command. Can we have a separate signOpts for blob signing?

rgnote · 2023-12-28T21:47:10Z

cmd/notation/blob.go

+func blobSignCommand(opts *signOpts) *cobra.Command {
+	if opts == nil {
+		opts = &signOpts{
+			inputType: inputTypeRegistry, // remote registry by default


Registries do not apply to blobs. Lets remove this.

rgnote · 2023-12-28T21:50:11Z

cmd/notation/blob.go

+			inputType: inputTypeRegistry, // remote registry by default
+		}
+	}
+	longMessage := `Sign artifacts


Can you use the log message as is from notation blob sign section in https://github.com/notaryproject/notation/pull/811/files?diff=unified&w=0?

rgnote · 2023-12-28T21:50:51Z

cmd/notation/blob.go

+`
+
+	command := &cobra.Command{
+		Use:   "blob sign [flags] <reference>",


rgnote · 2023-12-28T21:51:10Z

cmd/notation/blob.go

+	longMessage := `Inspect all signatures associated with the signed artifact.
+
+Example - Inspect signatures on an BLOB artifact identified by a digest:
+  notation blob inspect <registry>/<repository>@<digest>


Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

priteshbandi · 2024-01-16T19:03:32Z

cmd/notation/blob.go

@@ -0,0 +1,232 @@
+// Copyright The Notary Project Authors.


Please move this inside blob directory similar to cert, plugin etc. Also, rename file to sign.go.

priteshbandi · 2024-01-16T19:27:06Z

cmd/notation/blob.go

+type blobOpts struct {
+	cmd.LoggingFlagOpts
+	cmd.SignerFlagOpts
+	SecureFlagOpts


Do we need SecureFlagOpts ?

priteshbandi · 2024-01-16T19:28:48Z

cmd/notation/blob.go

+	pluginConfig  []string
+	userMetadata  []string
+	blobPath      string
+	signaturePath string


Suggested change

signaturePath string

signatureDirectory string

priteshbandi · 2024-01-16T19:30:24Z

cmd/notation/blob.go

+		Long:  longMessage,
+		Args: func(cmd *cobra.Command, args []string) error {
+			if len(args) == 0 {
+				return errors.New("missing blob_path")


What is blob_path ? How will user interpret it?

priteshbandi · 2024-01-16T19:31:42Z

cmd/notation/blob.go

+	desc          ocispec.Descriptor
+	sigRepo       registry.Repository


Why do we need this ?

priteshbandi · 2024-01-16T19:33:35Z

cmd/notation/blob.go

+	userMetadata  []string
+	blobPath      string
+	signaturePath string
+	outputFormat  string


are we missing option for some flags like --force ?

priteshbandi · 2024-01-16T19:34:04Z

cmd/notation/blob.go

+
+func blobSignCommand(opts *blobOpts) *cobra.Command {
+	if opts == nil {
+		opts = &blobOpts{}


Is this correct ?

priteshbandi · 2024-01-16T19:35:04Z

cmd/notation/blob.go

+	return blobOpts, nil
+}
+
+func blobInspectCommand(opts *blobOpts) *cobra.Command {


Please create new .go file for inspect.

priteshbandi · 2024-01-16T19:36:19Z

cmd/notation/blob.go

+
+	output := inspectOutput{MediaType: opts.desc.MediaType, Signatures: []signatureOutput{}}
+
+	sigBlob, _, _ := opts.sigRepo.FetchSignatureBlob(ctx, opts.desc)


we dont have sigRepo in case of blob signing.

Two-Hearts · 2024-01-30T08:26:27Z

cmd/notation/notation

nit:
We should not include the go build notation binary in the PR.

Two-Hearts · 2024-01-31T01:56:49Z

cmd/notation/blob.go

+		var errorPushSignatureFailed notation.ErrorPushSignatureFailed
+		if errors.As(err, &errorPushSignatureFailed) && strings.Contains(err.Error(), referrersTagSchemaDeleteError) {


For blob signing, there's no more notation.ErrorPushSignatureFailed and referrersTagSchemaDeleteError because they are OCI specific errors.

cmd/notation/blob/cmd.go

priteshbandi · 2024-02-12T18:42:31Z

cmd/notation/notation

Please remove binary

Will remove in new rev

priteshbandi · 2024-02-12T18:45:21Z

cmd/notation/main.go

@@ -45,6 +46,7 @@ func main() {
 		logoutCommand(nil),
 		versionCommand(),
 		inspectCommand(nil),
+		blob.SignCommand(nil),


Please follow pattern likecert.Cmd, policy.Cmd().

Will update in new rev

cmd/notation/blob/sign.go

Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

priteshbandi · 2024-03-27T23:12:59Z

cmd/notation/blob/sign.go

+		Long:  longMessage,
+		Args: func(cmd *cobra.Command, args []string) error {
+			if len(args) == 0 {
+				return errors.New("missing blob file path to the artifact: use `notation blob sign --help` to see what parameters are required")


Suggested change

return errors.New("missing blob file path to the artifact: use `notation blob sign --help` to see what parameters are required")

return errors.New("missing file path use `notation blob sign --help` to see what parameters are required")

priteshbandi · 2024-03-27T23:17:20Z

cmd/notation/blob/sign.go

+	// Todo: we will need to replace signer with actual blob signer implementation in notation-go
+	// initialize


As discussed on Monday, for signing we dont need a TODO as code has already been merged on notation-go.

priteshbandi · 2024-03-28T00:24:04Z

cmd/notation/blob/sign.go

+
+	// Todo: we will need to replace signer with actual blob signer implementation in notation-go
+	// initialize
+	signer, err := cmd.GetSigner(ctx, &cmdOpts.SignerFlagOpts)


cmd.GetSigner returns notation.Signer which doesnt supports for BlobSigning.

priteshbandi · 2024-03-28T00:24:43Z

cmd/notation/blob/sign.go

+	blobOpts := notation.SignOptions{
+		SignerSignOptions: notation.SignerSignOptions{
+			SignatureMediaType: mediaType,
+			ExpiryDuration:     opts.expiry,
+			PluginConfig:       pluginConfig,
+		},
+		UserMetadata: userMetadata,


Please use blob related opts

priteshbandi · 2024-03-28T00:27:43Z

cmd/notation/blob/sign.go

+	if err != nil {
+		return err
+	}
+	contents, err := os.ReadFile(cmdOpts.blobPath)


Dont read file; pass the reader to SignBlob.

priteshbandi · 2024-03-28T00:28:09Z

cmd/notation/blob/sign.go

+		return err
+	}
+	// core process
+	_, _, err = notation.SignBlob(ctx, signer, strings.NewReader(string(contents)), blobOpts) //PlaceHolder


Can we please add implementation to write signature file?

github-actions · 2024-05-12T01:49:07Z

This PR is stale because it has been opened for 45 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days.

rrsemlani requested review from gokarnm, JeyJeyGao, justincormack, NiazFK, priteshbandi, rgnote, shizhMSFT, SteveLasker and Two-Hearts as code owners December 26, 2023 22:31

Adding sign and inspect blob commands

2ed55c6

Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

rrsemlani force-pushed the rsemlani_notation branch from ad53a80 to 2ed55c6 Compare December 26, 2023 23:20

rgnote reviewed Dec 28, 2023

View reviewed changes

Adding sign and inspect blob commands

a0fff85

Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

rrsemlani requested a review from toddysm as a code owner January 16, 2024 17:02

priteshbandi reviewed Jan 16, 2024

View reviewed changes

Two-Hearts reviewed Jan 30, 2024

View reviewed changes

cmd/notation/notation Outdated

Copy link

Contributor

Two-Hearts Jan 30, 2024

Choose a reason for hiding this comment

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

nit:
We should not include the go build notation binary in the PR.

Two-Hearts changed the title ~~Adding sign and inspect blob commands~~ feat: add sign and inspect blob commands Jan 30, 2024

Two-Hearts reviewed Jan 31, 2024

View reviewed changes

priteshbandi reviewed Feb 12, 2024

View reviewed changes

cmd/notation/blob/sign.go Show resolved Hide resolved

rrsemlani force-pushed the rsemlani_notation branch from 1c81aa3 to 211d186 Compare February 12, 2024 19:30

rrsemlani changed the title ~~feat: add sign and inspect blob commands~~ feat: add sign blob commands Feb 13, 2024

rrsemlani force-pushed the rsemlani_notation branch from 211d186 to 48102ef Compare February 13, 2024 20:29

priteshbandi reviewed Feb 21, 2024

View reviewed changes

cmd/notation/blob/sign.go Show resolved Hide resolved

rrsemlani force-pushed the rsemlani_notation branch from 48102ef to 440cfd1 Compare March 26, 2024 18:46

Adding blob sign command

696d4a4

Signed-off-by: Rishab Semlani <rrsemlani@gmail.com>

rrsemlani force-pushed the rsemlani_notation branch from 440cfd1 to 696d4a4 Compare March 26, 2024 18:49

priteshbandi reviewed Mar 28, 2024

View reviewed changes

github-actions bot added Stale and removed Stale labels May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add sign blob commands #856

feat: add sign blob commands #856

rrsemlani commented Dec 26, 2023 •

edited

codecov-commenter commented Dec 26, 2023 •

edited by codecov bot

rgnote Dec 28, 2023

rgnote Dec 28, 2023

rgnote Dec 28, 2023

rgnote Dec 28, 2023

rgnote Dec 28, 2023

rgnote Dec 28, 2023

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

priteshbandi Jan 16, 2024

Two-Hearts Jan 30, 2024

Two-Hearts Jan 31, 2024

priteshbandi Feb 12, 2024

rrsemlani Feb 12, 2024

priteshbandi Feb 12, 2024

rrsemlani Feb 12, 2024

priteshbandi Mar 27, 2024

priteshbandi Mar 27, 2024

priteshbandi Mar 28, 2024

priteshbandi Mar 28, 2024

priteshbandi Mar 28, 2024

priteshbandi Mar 28, 2024

github-actions bot commented May 12, 2024


		output := inspectOutput{MediaType: opts.desc.MediaType, Signatures: []signatureOutput{}}

		sigBlob, _, _ := opts.sigRepo.FetchSignatureBlob(ctx, opts.desc)

		var errorPushSignatureFailed notation.ErrorPushSignatureFailed
		if errors.As(err, &errorPushSignatureFailed) && strings.Contains(err.Error(), referrersTagSchemaDeleteError) {

	return errors.New("missing blob file path to the artifact: use `notation blob sign --help` to see what parameters are required")
	return errors.New("missing file path use `notation blob sign --help` to see what parameters are required")

		// Todo: we will need to replace signer with actual blob signer implementation in notation-go
		// initialize

feat: add sign blob commands #856

Are you sure you want to change the base?

feat: add sign blob commands #856

Conversation

rrsemlani commented Dec 26, 2023 • edited

codecov-commenter commented Dec 26, 2023 • edited by codecov bot

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2024

rrsemlani commented Dec 26, 2023 •

edited

codecov-commenter commented Dec 26, 2023 •

edited by codecov bot