Implement generate signature plugin workflow #42

qmuntal · 2022-05-05T09:51:27Z

This PR contains the following changes:

Add support for describe-key command
Expand generate-signature request to include keySpec, hashAlgorithm and configMap
Implement plugin signer with generate-signature workflow support
Unit tests for everything
Edit: Create spec/v1/plugin and spec/v1/signature package. These contains the Go types as defined in the notary spec. Having this in its own package will help decoupling the spec itself with the crypto-related code and also plugin authors who want to implement the generate-envelope workflow.

This PR still doesn't implement:

Setting the right contract version in all plugin command
generate-signature workflow

@SteveLasker @shizhMSFT @gokarnm @jaredpar

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2 to 3. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v2...v3) --- updated-dependencies: - dependency-name: actions/setup-go dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Bumps [actions/cache](https://github.com/actions/cache) from 2 to 3.0.1. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v2...v3.0.1) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal · 2022-05-11T07:42:49Z

Plugin spec updated with latest changes in notaryproject/specifications#155

spec/v1/signature/jws.go

spec/v1/signature/types.go

shizhMSFT · 2022-05-11T11:49:19Z

notation.go

@@ -55,7 +33,7 @@ func (opts SignOptions) Validate() error {
 type Signer interface {
 	// Sign signs the artifact described by its descriptor,
 	// and returns the signature.
-	Sign(ctx context.Context, desc Descriptor, opts SignOptions) ([]byte, error)
+	Sign(ctx context.Context, desc signature.Descriptor, opts SignOptions) ([]byte, error)


I think Descriptor should be maintained at the notation level, regardless how plugin protocol works.

Any particular reason? The descriptor is part of the signing spec, so IMO it should be in the spec package.

Then we have the Signer interface to be spec specific, and probably we need to put the Signer into the specs/v1 folder. LoL.

Signer is not part of the spec, it's just an interface defined by notation-go, so I would put it in /spec. As we have removed the version subfolders from /spec, then we have no conflict here anymore.

There are two protocols here. One is at the Signer interface level, another is at the spec level. We probably need a design doc here.

IMO types which are inputs and outputs of Sign/Verify methods in top level package should also be in the same package, Descriptor should remain at notation level.

I think spec package is not required, Notation implements one major version of Notary V2 specifications, which can introduce non breaking changes. Breaking changes can be introduced in a new major version of specification and corresponding new major version of Notation.
An exception is the plugin contract spec versioning, which is independent of other versioning, but we need not reflect this in package naming, consumers do not directly interact with plugin contract version.
We should also put most types under internal package, and selectively move then out as publicly visible types as appropriate for Notation CLI and other consumers . Are we planning to use pkg folder for public library types? I was looking at Go lang project layout standard, but not sure how widely it's adopted.

There are two protocols here. One is at the Signer interface level, another is at the spec level. We probably need a design doc here.

Can you clarify @shizhMSFT? I didn't understand why two interfaces are required, are you referring to the low level Signer interface to be added in notation-core-go.

I think spec package is not required, Notation implements one major version of Notary V2 specifications, which can introduce non breaking changes. Breaking changes can be introduced in a new major version of specification and corresponding new major version of Notation.

I will not tire of repeating that mixing the notation-go module versioning with the Notary versioning is an artificial restriction that will limit notation-go capacity to improve its API. What if in a couple of months, or even years, we find that part of the API is too easy to misuse or has security vulnerabilities? What if we think there is a much better way to implement the Notary spec? We won't be able to react properly if we can't bump the major version of nottion-go.

About having the spec package or putting everything at the root of the module, I think there is value on having a clear separation for types which are defined in the Notary spec and types that are invented by notation-go. I'm not a maintainer of this library, so if you don't see it then I can remove it.

An exception is the plugin contract spec versioning, which is independent of other versioning, but we need not reflect this in package naming, consumers do not directly interact with plugin contract version.
We should also put most types under internal package, and selectively move then out as publicly visible types as appropriate for Notation CLI and other consumers

spec/plugin package is intended for plugin authors consumption, if we move those types into internal they will have to reimplement the whole plugin spec. I can assure you that all types and methods in spec are relevant for consumers, as I'm implementing the notation-azure-kv plugin and notation cli in parallel and I'm just exporting what's necessary.

Are we planning to use pkg folder for public library types? I was looking at Go lang project layout standard, but not sure how widely it's adopted.

Having a package named pkg is not idiomatic, it used to be long time ago, and the current best practice is to place packages at the module root, or stack them with care.

I will not tire of repeating that mixing the notation-go module versioning with the Notary versioning is an artificial restriction that will limit notation-go capacity to improve its API. What if in a couple of months, or even years, we find that part of the API is too easy to misuse or has security vulnerabilities?

I agree with the concern, I don't think we have thought enough about versioning of the multiple things (plugins, signature, trust policy) and which ones may reflect in package structure. For example, trust policy document has its own version property, so that policy language in trustPolicy.json can be independently revised without impacting anything else. We'll be making a round of refactoring to get it in line with intended separation between notation, notation-go and notation-core-go, we'll also wait for the rest of the specs to be completed. I'll track this as one of the items in a refactoring issue, and an independent issue to clarify versioning. Do you have few examples of projects that expose a spec and spec version to consumers as part of package structure, /image-spec was one, but that's quite stable.

Do you have few examples of projects that expose a spec and spec version to consumers as part of package structure, /image-spec was one, but that's quite stable.

To name a few big ones:

azure-go-sdk use it all around.

aws-sdk-go is more stable but also uses it.

google/go.

signature/jws/plugin.go

signature/jws/signer.go

crypto/jwsutil/envelope.go

signature/jws/spec.go

spec/v1/signature/jws.go

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

This reverts commit c9afb7c. Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm

Added some comments, will cover the rest of changes in another pass.

signature/jws/plugin.go

signature/jws/spec.go

signature/jws/plugin.go

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

shizhMSFT

LGTM

shizhMSFT · 2022-05-17T05:18:15Z

notation.go

@@ -55,7 +33,7 @@ func (opts SignOptions) Validate() error {
 type Signer interface {
 	// Sign signs the artifact described by its descriptor,
 	// and returns the signature.
-	Sign(ctx context.Context, desc Descriptor, opts SignOptions) ([]byte, error)
+	Sign(ctx context.Context, desc signature.Descriptor, opts SignOptions) ([]byte, error)


There are two protocols here. One is at the Signer interface level, another is at the spec level. We probably need a design doc here.

SteveLasker · 2022-05-17T05:56:15Z

@gokarnm , is your requested change resolved?

dtzar · 2022-05-17T15:57:56Z

@SteveLasker on the call yesterday @gokarnm said the changes he made are fine with the code he reviewed so far but wants a couple changes after the PR is merged and he will file issues to track. He only reviewed 1/2 of the PR and will review the rest today.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm

Mostly superficial comments around naming and error messages.

For organizing packages and types, IMO we should not have a spec top level package. I'd keep public methods and corresponding input/output types (e.g. Descriptor) in the same package. Any private types should be moved to internal package, any public plugin types should move to plugin package.

spec/signature/types.go

signature/jws/plugin.go

spec/signature/types.go

spec/plugin/plugin.go

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Co-authored-by: Milind Gokarn <milind81@gmail.com> Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal · 2022-05-18T08:12:53Z

Thanks everyone for the feedback. I've finally removed the spec package, as there is no consensus on how to manage the versioning of the spec itself and this discussion is out of the scope of this PR.

gokarnm

LGTM. Thanks @qmuntal for implementing this and improving the spec along the way!

SteveLasker

LGTM
Great work @qmuntal

qmuntal and others added 30 commits April 26, 2022 20:47

implement plugin manager

d0c0218

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

ignore symlinks

2943a57

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix Manager.Command

c33c354

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

export constants

5bfff33

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

pr feedback

c09b4a1

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

improve error message

e1f2d49

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

move manager to its own package

2cacb9f

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

change metadata error messages

55fc95a

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

improve the plugin manager interface

f0636e1

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

implement plugin.Run

b4e505d

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

add plugin signer

295d0bc

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

improve Manager.Run

e39e8ce

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

base64 encode payload

6307514

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix supported algs

d0874d9

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

create envelope

15d7713

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

pr feedback

d1f6564

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

remove Command.Capability() and Command.NewResponse()

74dc477

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

add DescribeKey command

f37ca5b

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

remove command validation

504ca16

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

plugin cleanup

7beb5cd

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix compilation

a3bdec6

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

base64 encode signature

04da321

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

ignore symlinked plugin directories

8f78a94

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

suuport plugin key spec

bd10cfe

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

pass signing payload as ascii string

ceb9b92

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix rsa method

26c6b91

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

define KeySpec

3470557

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

add signing tests

4d98f6d

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

shizhMSFT reviewed May 11, 2022

View reviewed changes

qmuntal added 2 commits May 11, 2022 15:59

PR feedback

92366fb

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

check runner response type

fc5592f

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

SteveLasker requested a review from gokarnm May 11, 2022 17:06

qmuntal added 4 commits May 12, 2022 08:43

remove spec/v1 directory

b2c02f5

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Revert "remove support for multiple signature envelope"

b9246e9

This reverts commit c9afb7c. Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

don't check timestamp certificate

cbc16f9

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

define SignatureAlgorithm in spec

5e2d8ca

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm requested changes May 13, 2022

View reviewed changes

Update plugin implementation with latest spec

a24ae78

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal requested a review from gokarnm May 13, 2022 09:41

SteveLasker mentioned this pull request May 13, 2022

Update notation plug-in support to the updated plug-in spec notaryproject/notation#162

Closed

5 tasks

improve plugin error report

35b4fac

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal mentioned this pull request May 16, 2022

Add plugin sign capabaility notaryproject/notation#176

Merged

shizhMSFT approved these changes May 17, 2022

View reviewed changes

add x5c comment

8177f1c

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm requested changes May 18, 2022

View reviewed changes

qmuntal and others added 5 commits May 18, 2022 09:42

simplify runner interface

979142f

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

remove spec package

6fe2523

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

pr feedback

ddf72d9

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

Apply suggestions from code review

cca70c9

Co-authored-by: Milind Gokarn <milind81@gmail.com> Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix build

07fe9b8

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal force-pushed the feat-plugins branch from c9b1afc to 07fe9b8 Compare May 18, 2022 08:09

gokarnm approved these changes May 18, 2022

View reviewed changes

SteveLasker approved these changes May 18, 2022

View reviewed changes

SteveLasker merged commit 4075375 into notaryproject:main May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement generate signature plugin workflow #42

Implement generate signature plugin workflow #42

qmuntal commented May 5, 2022 •

edited

Loading

qmuntal commented May 11, 2022

shizhMSFT May 11, 2022

qmuntal May 11, 2022

shizhMSFT May 12, 2022

qmuntal May 12, 2022

shizhMSFT May 17, 2022

gokarnm May 18, 2022 •

edited

Loading

gokarnm May 18, 2022

qmuntal May 18, 2022

gokarnm May 18, 2022

qmuntal May 18, 2022

gokarnm left a comment

shizhMSFT left a comment

shizhMSFT May 17, 2022

SteveLasker commented May 17, 2022

dtzar commented May 17, 2022 •

edited

Loading

gokarnm left a comment

qmuntal commented May 18, 2022

gokarnm left a comment

SteveLasker left a comment

Implement generate signature plugin workflow #42

Implement generate signature plugin workflow #42

Conversation

qmuntal commented May 5, 2022 • edited Loading

qmuntal commented May 11, 2022

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

gokarnm May 18, 2022 • edited Loading

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

gokarnm left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLasker commented May 17, 2022

dtzar commented May 17, 2022 • edited Loading

gokarnm left a comment

Choose a reason for hiding this comment

qmuntal commented May 18, 2022

gokarnm left a comment

Choose a reason for hiding this comment

SteveLasker left a comment

Choose a reason for hiding this comment

qmuntal commented May 5, 2022 •

edited

Loading

gokarnm May 18, 2022 •

edited

Loading

dtzar commented May 17, 2022 •

edited

Loading