refactor: refactor pluginSigner to support new signature interface #131

chloeyin · 2022-09-08T09:11:06Z

What

Refactor notation-go to support multiple envelope types.
Background can be checked in notaryproject/notation#278
I wthe whole PR into two PRs to help review, this is the first PR. More unit test cases will be added in the next PR.
The whole picture is here #146

Major Changes

Use package github.com/notaryproject/notation-core-go/signature to sign and verify.
Combine runner and signer into a provider for pluginSigner to sign and remove the pluginSigProvider.
Add builtinProvider to support local signing and externalProvider to support signing by plugin.
Move the payload media type and its checks to signature package as mentioned in refactor: refactor envelope and signer to support cose notation-core-go#73
Support new keySpec and plugin contract.
Get verification plugin and version from extended attributes.
Add SpeculateSignatureEnvelopeFormat to inspect signature (This function may change later to better inspect a signature)
Add sign/verify from file test cases.
Signed-off-by: zaihaoyin [email protected]

codecov-commenter · 2022-09-21T06:07:50Z

Codecov Report

Merging #131 (3479183) into main (84d4de1) will decrease coverage by 2.16%.
The diff coverage is 49.08%.

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   72.79%   70.62%   -2.17%     
==========================================
  Files          36       39       +3     
  Lines        2503     2669     +166     
==========================================
+ Hits         1822     1885      +63     
- Misses        544      632      +88     
- Partials      137      152      +15

Impacted Files	Coverage Δ
plugin/algorithm.go	`0.00% <0.00%> (ø)`
plugin/plugin.go	`0.00% <ø> (ø)`
verification/verifier.go	`82.98% <50.00%> (-4.11%)`	⬇️
signature/verifier.go	`56.41% <57.14%> (+18.47%)`	⬆️
verification/verifier_helpers.go	`68.19% <63.01%> (-5.45%)`	⬇️
signature/provider.go	`71.64% <71.64%> (ø)`
signature/plugin.go	`79.25% <72.72%> (+10.75%)`	⬆️
signature/signer.go	`38.23% <80.00%> (-13.58%)`	⬇️
signature/envelope.go	`100.00% <100.00%> (ø)`
verification/helpers.go	`92.50% <100.00%> (ø)`
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

signature/envelope.go

signature/envelope_test.go

signature/provider.go

verification/verifier_helpers.go

notation.go

signature/envelope_test.go

plugin/plugin.go

signature/algorithm.go

signature/envelope.go

signature/algorithm.go

verification/verifier_helpers.go

signature/envelope.go

priteshbandi · 2022-09-22T06:14:59Z

plugin/plugin.go

@@ -146,9 +145,9 @@ func (GenerateSignatureRequest) Command() Command {

 // GenerateSignatureResponse is the response of a generate-signature request.
 type GenerateSignatureResponse struct {
-	KeyID            string                    `json:"keyId"`
-	Signature        []byte                    `json:"signature"`
-	SigningAlgorithm signer.SignatureAlgorithm `json:"signingAlgorithm"`


Can we use algorithm type instead of string?

https://github.com/notaryproject/notaryproject/blob/main/specs/plugin-extensibility.md
I think spec requires a string value as the protocol to communicate with notation plugin.
E.g.

RSASSA-PSS-SHA-256: RSASSA-PSS with SHA-256 RSASSA-PSS-SHA-384: RSASSA-PSS with SHA-384

@shizhMSFT Can we reuse algorithm defined in notation-core-go?

priteshbandi · 2022-09-22T06:15:51Z

signature/algorithm.go

+const (
+	ECDSA_SHA_256      = "ECDSA-SHA-256"
+	ECDSA_SHA_384      = "ECDSA-SHA-384"
+	ECDSA_SHA_512      = "ECDSA-SHA-512"
+	RSASSA_PSS_SHA_256 = "RSASSA-PSS-SHA-256"
+	RSASSA_PSS_SHA_384 = "RSASSA-PSS-SHA-384"
+	RSASSA_PSS_SHA_512 = "RSASSA-PSS-SHA-512"
+)


same here, spec describe it as a string

priteshbandi · 2022-09-22T06:18:36Z

signature/algorithm.go

+var InvalidKeySpec = signature.KeySpec{}
+
+// KeySpecName returns the name of a keySpec according to the spec.
+func KeySpecName(k signature.KeySpec) string {


Can we reuse https://github.com/notaryproject/notation-core-go/blob/f14510cf21c244182574a71f26b920057a0d5bf3/signature/algorithm.go#L29-L32 ?

priteshbandi · 2022-09-22T06:22:05Z

signature/algorithm.go

+	case signature.AlgorithmES256:
+		return ECDSA_SHA_256
+	case signature.AlgorithmES384:
+		return ECDSA_SHA_384
+	case signature.AlgorithmES512:
+		return ECDSA_SHA_512
+	case signature.AlgorithmPS256:
+		return RSASSA_PSS_SHA_256
+	case signature.AlgorithmPS384:
+		return RSASSA_PSS_SHA_384
+	case signature.AlgorithmPS512:
+		return RSASSA_PSS_SHA_512


Why do we have different algo for core-go and for notation-go? Can we use one for both packages?

keySpec : One of following supported key types - RSA-2048, RSA-3072, RSA-4096, EC-256, EC-384, EC-521. I think plugin spec requires them to be a string.

signature/plugin.go

notation.go

plugin/plugin.go

signature/plugin.go

signature/provider.go

shizhMSFT

LGTM with suggestions.

notation.go

shizhMSFT · 2022-09-23T14:18:04Z

signature/plugin.go

+	// TODO: pass media type as a parameter.
+	envelopeMediaType := jws.MediaTypeEnvelope


@priteshbandi Let's resolve this TODO in this PR. Although it will be an API change, it will be a small change in notation and will not block the alpha.4 release.

Of course, we can defer it to post-alpha.4. It is just weird to have hard-coded envelopeMediaType in the code.

Looks like @chloeyin has updated the pr with to pass envelopeMediaType as parameter.

signature/provider.go

shizhMSFT · 2022-09-23T14:24:20Z

signature/signer.go

+	// TODO: pass media type as a parameter
+	envelopeMediaType := jws.MediaTypeEnvelope


@priteshbandi same here

priteshbandi

The subsequent updates in PR are difficult to review because we cannot view diff after the comments/feedbacks were resolved as commits were force pushed the commits which overrides the history.

priteshbandi · 2022-09-26T02:20:17Z

plugin/algorithm.go

+func ParseSigningAlgorithm(raw string) (signature.Algorithm, error) {
+	switch raw {
+	case ECDSA_SHA_256:
+		return signature.AlgorithmES256, nil
+	case ECDSA_SHA_384:
+		return signature.AlgorithmES384, nil
+	case ECDSA_SHA_512:
+		return signature.AlgorithmES512, nil
+	case RSASSA_PSS_SHA_256:
+		return signature.AlgorithmPS256, nil
+	case RSASSA_PSS_SHA_384:
+		return signature.AlgorithmPS384, nil
+	case RSASSA_PSS_SHA_512:
+		return signature.AlgorithmPS512, nil
+	}
+	return 0, errors.New("unknown signing algorithm")
+}


Can we reuse mapping in SigningAlgorithmString function ?

chloeyin · 2022-09-26T02:44:57Z

The subsequent updates in PR are difficult to review because we cannot view diff after the comments/feedbacks were resolved as commits were force pushed the commits which overrides the history.

The force push is rebasing the main branch. The latest change is here. 3479183

priteshbandi

Added some nitpicks.
LGTM

dtzar · 2022-09-26T19:29:08Z

@chloeyin @shizhMSFT @yizha1 - this appears ready to merge after updating latest changes from main. I'd click to update it, but then it likely wouldn't be signed and it would then fail DCO.

As this is blocking testing for Alpha 4 release, can we update/merge ASAP? @vaninrao10 @iamsamirzon
Then update notation to use new go library notaryproject/notation#347 and then perhaps
Release new out-of-band "weekly" dev build to test against for alpha.4 release.

Signed-off-by: zaihaoyin <[email protected]>

…provider Signed-off-by: zaihaoyin <[email protected]>

Signed-off-by: zaihaoyin <[email protected]>

chloeyin · 2022-09-27T02:34:03Z

@chloeyin @shizhMSFT @yizha1 - this appears ready to merge after updating latest changes from main. I'd click to update it, but then it likely wouldn't be signed and it would then fail DCO.

As this is blocking testing for Alpha 4 release, can we update/merge ASAP? @vaninrao10 @iamsamirzon Then update notation to use new go library notaryproject/notation#347 and then perhaps Release new out-of-band "weekly" dev build to test against for alpha.4 release.

Merged. notaryproject/notation#357 This pr will update dependency.

shizhMSFT requested review from a team September 9, 2022 01:59

shizhMSFT mentioned this pull request Sep 9, 2022

refactor:support cose signature format #127

Closed

chloeyin force-pushed the cose-pr1 branch from 8f5d387 to edc1978 Compare September 13, 2022 10:03

chloeyin force-pushed the cose-pr1 branch 2 times, most recently from 44c2e39 to e2c3b75 Compare September 21, 2022 06:04

patrickzheng200 reviewed Sep 21, 2022

View reviewed changes

signature/envelope.go Outdated Show resolved Hide resolved

chloeyin force-pushed the cose-pr1 branch 2 times, most recently from d232af2 to 2b26389 Compare September 21, 2022 06:32

chloeyin changed the title ~~refactor:refactor pluginSigner to support cose envelope~~ refactor:refactor pluginSigner to support new signature interface Sep 21, 2022

chloeyin force-pushed the cose-pr1 branch from 2b26389 to ecf992b Compare September 21, 2022 07:34

chloeyin mentioned this pull request Sep 21, 2022

refactor: support cose envelope #146

Merged

chloeyin force-pushed the cose-pr1 branch 2 times, most recently from db95979 to bfccb79 Compare September 22, 2022 07:40

binbin-li reviewed Sep 22, 2022

View reviewed changes

yizha1 added this to the RC-1 milestone Sep 22, 2022

yizha1 assigned chloeyin Sep 22, 2022

shizhMSFT requested changes Sep 22, 2022

View reviewed changes

dtzar modified the milestones: RC-1, alpha-4 Sep 22, 2022

chloeyin force-pushed the cose-pr1 branch 2 times, most recently from d8a85f0 to a2f21d6 Compare September 23, 2022 02:44

priteshbandi reviewed Sep 23, 2022

View reviewed changes

shizhMSFT changed the title ~~refactor:refactor pluginSigner to support new signature interface~~ refactor: refactor pluginSigner to support new signature interface Sep 23, 2022

shizhMSFT approved these changes Sep 23, 2022

View reviewed changes

yizha1 mentioned this pull request Sep 24, 2022

Failed to sign images using self-signed certificates created from Key vault notaryproject/notation#320

Closed

chloeyin force-pushed the cose-pr1 branch from dc1439c to e1a6b46 Compare September 26, 2022 02:03

priteshbandi reviewed Sep 26, 2022

View reviewed changes

priteshbandi approved these changes Sep 26, 2022

View reviewed changes

chloeyin mentioned this pull request Sep 26, 2022

Adding Notary v2 payload validation #154

Closed

zaihaoyin added 5 commits September 27, 2022 10:00

refactor:refactor pluginSigner to support cose envelope

dc49c96

Signed-off-by: zaihaoyin <[email protected]>

refactor:support new signer interface

983442c

Signed-off-by: zaihaoyin <[email protected]>

refactor:clean code

e7b109e

Signed-off-by: zaihaoyin <[email protected]>

refactor:keep sign interface unchanged. pass keyspec as parameter to …

33b4d2e

…provider Signed-off-by: zaihaoyin <[email protected]>

refactor:add envelope type as sign/verify parameter

2fa3bef

Signed-off-by: zaihaoyin <[email protected]>

chloeyin force-pushed the cose-pr1 branch from 3479183 to 2fa3bef Compare September 27, 2022 02:00

chloeyin merged commit 2bcfd34 into notaryproject:main Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refactor pluginSigner to support new signature interface #131

refactor: refactor pluginSigner to support new signature interface #131

chloeyin commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 21, 2022 •

edited

Loading

priteshbandi Sep 22, 2022

chloeyin Sep 23, 2022

priteshbandi Sep 22, 2022

chloeyin Sep 23, 2022

priteshbandi Sep 22, 2022

chloeyin Sep 23, 2022

priteshbandi Sep 22, 2022

chloeyin Sep 23, 2022

shizhMSFT left a comment

shizhMSFT Sep 23, 2022

shizhMSFT Sep 23, 2022

priteshbandi Sep 26, 2022 •

edited

Loading

shizhMSFT Sep 23, 2022

priteshbandi left a comment

priteshbandi Sep 26, 2022

chloeyin commented Sep 26, 2022

priteshbandi left a comment

dtzar commented Sep 26, 2022 •

edited

Loading

chloeyin commented Sep 27, 2022

		// TODO: pass media type as a parameter.
		envelopeMediaType := jws.MediaTypeEnvelope

		// TODO: pass media type as a parameter
		envelopeMediaType := jws.MediaTypeEnvelope

refactor: refactor pluginSigner to support new signature interface #131

refactor: refactor pluginSigner to support new signature interface #131

Conversation

chloeyin commented Sep 8, 2022 • edited Loading

What

Major Changes

codecov-commenter commented Sep 21, 2022 • edited Loading

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

shizhMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chloeyin commented Sep 26, 2022

priteshbandi left a comment

Choose a reason for hiding this comment

dtzar commented Sep 26, 2022 • edited Loading

chloeyin commented Sep 27, 2022

chloeyin commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 21, 2022 •

edited

Loading

priteshbandi Sep 26, 2022 •

edited

Loading

dtzar commented Sep 26, 2022 •

edited

Loading