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

Implement generate signature plugin workflow #42

Merged
merged 59 commits into from
May 18, 2022
Merged

Implement generate signature plugin workflow #42

Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
d0c0218
implement plugin manager
qmuntal Apr 26, 2022
2943a57
ignore symlinks
qmuntal Apr 26, 2022
c33c354
fix Manager.Command
qmuntal Apr 27, 2022
5bfff33
export constants
qmuntal Apr 27, 2022
c09b4a1
pr feedback
qmuntal Apr 28, 2022
e1f2d49
improve error message
qmuntal Apr 28, 2022
2cacb9f
move manager to its own package
qmuntal Apr 28, 2022
55fc95a
change metadata error messages
qmuntal Apr 28, 2022
f0636e1
improve the plugin manager interface
qmuntal Apr 29, 2022
b4e505d
implement plugin.Run
qmuntal Apr 30, 2022
295d0bc
add plugin signer
qmuntal May 2, 2022
e39e8ce
improve Manager.Run
qmuntal May 2, 2022
6307514
base64 encode payload
qmuntal May 2, 2022
d0874d9
fix supported algs
qmuntal May 2, 2022
15d7713
create envelope
qmuntal May 3, 2022
d1f6564
pr feedback
qmuntal May 3, 2022
74dc477
remove Command.Capability() and Command.NewResponse()
qmuntal May 3, 2022
f37ca5b
add DescribeKey command
qmuntal May 3, 2022
504ca16
remove command validation
qmuntal May 3, 2022
7beb5cd
plugin cleanup
qmuntal May 3, 2022
a3bdec6
fix compilation
qmuntal May 3, 2022
04da321
base64 encode signature
qmuntal May 3, 2022
8f78a94
ignore symlinked plugin directories
qmuntal May 4, 2022
bd10cfe
suuport plugin key spec
qmuntal May 4, 2022
ceb9b92
pass signing payload as ascii string
qmuntal May 4, 2022
26c6b91
fix rsa method
qmuntal May 4, 2022
3470557
define KeySpec
qmuntal May 4, 2022
4d98f6d
add signing tests
qmuntal May 5, 2022
01a45db
Bump actions/setup-go from 2 to 3 (#32)
dependabot[bot] Apr 27, 2022
8ea7512
Bump actions/cache from 2 to 3.0.1 (#31)
dependabot[bot] Apr 27, 2022
c6553f8
Bump github.com/golang-jwt/jwt/v4 from 4.3.0 to 4.4.1 (#29)
dependabot[bot] Apr 27, 2022
7891053
Merge branch 'main' of https://github.com/notaryproject/notation-go i…
qmuntal May 5, 2022
03b1a43
check extension ID length and add tests
qmuntal May 5, 2022
7bbec7e
fail on empty certificate chain
qmuntal May 5, 2022
c3265c3
create and use spec package
qmuntal May 5, 2022
f50b242
dedup notaryClaim
qmuntal May 5, 2022
c9afb7c
remove support for multiple signature envelope
qmuntal May 6, 2022
70877b2
move payload creation to where it's needed
qmuntal May 6, 2022
a4981c0
add missing json tags
qmuntal May 6, 2022
a2fff48
use correct header encoding
qmuntal May 6, 2022
eb16900
rename NewManager to New and add root param
qmuntal May 9, 2022
2a6e597
remove artifact type from descriptor
qmuntal May 9, 2022
a453d9e
dedup jwt.Token creation
qmuntal May 9, 2022
a284a1e
simpligy running plugin command
qmuntal May 9, 2022
c55f36f
update plugin spec
qmuntal May 11, 2022
92366fb
PR feedback
qmuntal May 11, 2022
fc5592f
check runner response type
qmuntal May 11, 2022
b2c02f5
remove spec/v1 directory
qmuntal May 12, 2022
b9246e9
Revert "remove support for multiple signature envelope"
qmuntal May 12, 2022
cbc16f9
don't check timestamp certificate
qmuntal May 12, 2022
5e2d8ca
define SignatureAlgorithm in spec
qmuntal May 12, 2022
a24ae78
Update plugin implementation with latest spec
qmuntal May 13, 2022
35b4fac
improve plugin error report
qmuntal May 14, 2022
8177f1c
add x5c comment
qmuntal May 18, 2022
979142f
simplify runner interface
qmuntal May 18, 2022
6fe2523
remove spec package
qmuntal May 18, 2022
ddf72d9
pr feedback
qmuntal May 18, 2022
cca70c9
Apply suggestions from code review
qmuntal May 18, 2022
07fe9b8
fix build
qmuntal May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 0 additions & 53 deletions crypto/jwsutil/envelope.go
View file
Open in desktop

This file was deleted.

102 changes: 0 additions & 102 deletions crypto/jwsutil/envelope_test.go
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions crypto/jwsutil/errors.go
View file
Open in desktop

This file was deleted.

56 changes: 0 additions & 56 deletions crypto/jwsutil/signature.go
View file
Open in desktop

This file was deleted.

31 changes: 3 additions & 28 deletions notation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,9 @@ import (
"time"

"github.com/notaryproject/notation-go/crypto/timestamp"
"github.com/opencontainers/go-digest"
"github.com/notaryproject/notation-go/spec/v1/signature"
)

// Descriptor describes the content signed or to be signed.
type Descriptor struct {
// MediaType is the media type of the targeted content.
MediaType string `json:"mediaType"`

// Digest is the digest of the targeted content.
Digest digest.Digest `json:"digest"`

// Size specifies the size in bytes of the blob.
Size int64 `json:"size"`

// Annotations contains optional user defined attributes.
Annotations map[string]string `json:"annotations,omitempty"`
}

// Equal reports whether d and t points to the same content.
func (d Descriptor) Equal(t Descriptor) bool {
return d.MediaType == t.MediaType && d.Digest == t.Digest && d.Size == t.Size
}

// SignOptions contains parameters for Signer.Sign.
type SignOptions struct {
// Expiry identifies the expiration time of the resulted signature.
Expand All @@ -44,18 +24,13 @@ type SignOptions struct {
TSAVerifyOptions x509.VerifyOptions
}

// Validate does basic validation on SignOptions.
func (opts SignOptions) Validate() error {
return nil
}

// Signer is a generic interface for signing an artifact.
// The interface allows signing with local or remote keys,
// and packing in various signature formats.
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

@gokarnm gokarnm May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

}

// VerifyOptions contains parameters for Verifier.Verify.
Expand All @@ -70,7 +45,7 @@ func (opts VerifyOptions) Validate() error {
type Verifier interface {
// Verify verifies the signature and returns the verified descriptor and
// metadata of the signed artifact.
Verify(ctx context.Context, signature []byte, opts VerifyOptions) (Descriptor, error)
Verify(ctx context.Context, signature []byte, opts VerifyOptions) (signature.Descriptor, error)
}

// Service combines the signing and verification services.
Expand Down
2 changes: 1 addition & 1 deletion plugin/manager/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"reflect"
"testing"

"github.com/notaryproject/notation-go/plugin"
"github.com/notaryproject/notation-go/spec/v1/plugin"
)

func preparePlugin(t *testing.T) string {
Expand Down
4 changes: 3 additions & 1 deletion plugin/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"path/filepath"
"runtime"

"github.com/notaryproject/notation-go/plugin"
"github.com/notaryproject/notation-go/spec/v1/plugin"
)

// Plugin represents a potential plugin with all it's metadata.
Expand Down Expand Up @@ -199,6 +199,8 @@ func run(ctx context.Context, cmder commander, pluginPath string, cmd plugin.Com
resp = new(plugin.GenerateSignatureResponse)
case plugin.CommandGenerateEnvelope:
resp = new(plugin.GenerateEnvelopeResponse)
case plugin.CommandDescribeKey:
resp = new(plugin.DescribeKeyResponse)
default:
return nil, fmt.Errorf("unsupported command: %s", cmd)
}
Expand Down
2 changes: 1 addition & 1 deletion plugin/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"testing"
"testing/fstest"

"github.com/notaryproject/notation-go/plugin"
"github.com/notaryproject/notation-go/spec/v1/plugin"
)

type testCommander struct {
Expand Down
Loading