cliplugin: add VerifySignature by ramonpetgrave64 · Pull Request #1944 · sigstore/sigstore

ramonpetgrave64 · 2025-01-24T22:46:34Z

Summary

Adds the VerifySignature method.

Release Note

Adds the VerifySignature method.

Documentation

No new documentation.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 · 2025-01-24T22:55:22Z

@haydentherapper @loosebazooka

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

pkg/signature/kms/cliplugin/common/interface.go

Hayden-IO · 2025-01-27T17:55:56Z

pkg/signature/kms/cliplugin/common/options.go

 }

+// VerifyOptions contains the values for signature.VerifyOption.
+type VerifyOptions struct {


What is the motivation for defining another structure? Can we just have an Options structure with common options?

We could, but then the JSON schema would also change, making things a bit more complicated for plugin authors of other languages that would need to parse the json objects directly.

In case that we add or remove any options, some options might no longer be common, but belong in other opts structs. keeping the opts separate minimizes that breakage, and I think keeps things simpler for sigstore maintainers.

OK, I see you're mirroring the existing APIs as well.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Hayden-IO · 2025-01-27T18:39:57Z

pkg/signature/kms/cliplugin/common/options.go

 }

+// VerifyOptions contains the values for signature.VerifyOption.
+type VerifyOptions struct {


OK, I see you're mirroring the existing APIs as well.

Hayden-IO · 2025-01-27T18:42:52Z

pkg/signature/kms/cliplugin/common/options.go

+// VerifyOptions contains the values for signature.VerifyOption.
+type VerifyOptions struct {
+	RPCOptions     RPCOptions     `json:"rpcOptions"`
+	MessageOptions MessageOptions `json:"messageOptions"`


To confirm, if I want to verify an object without passing its digest, this is passed over stdin?

Yes. The message is the object to be verified, and it's sent to invokePlugin() as stdin.

https://github.com/sigstore/sigstore/pull/1944/files#diff-ca0732ab1082ecf7f774ea34224fd6317cb38133fdb1852715834c27954431deR174

loosebazooka · 2025-01-28T19:17:11Z

pkg/signature/kms/cliplugin/common/interface_test.go

-		DefaultAlgorithm: &DefaultAlgorithmResp{DefaultAlgorithm: "any-algo"},
-		CreateKey:        &CreateKeyResp{PublicKeyPEM: []byte("mypem")},
-		SignMessage:      &SignMessageResp{Signature: []byte("any-signature")},
+		DefaultAlgorithm: &DefaultAlgorithmResp{DefaultAlgorithm: testAlgorithm},


I feel like this test should test closer to what's expected rather than one big chunk? Can we split it in to parameterized testcases within this test?

test/cliplugin/localkms/implementation.go

loosebazooka · 2025-01-28T19:23:41Z

test/cliplugin/localkms/implementation.go

+		opt.ApplyCryptoSignerOpts(&signerOpts)
+	}
+
+	if len(digest) == 0 {


it's a bit confusing that the user can provide message or digest here. I would think it's one or the other?

more specifically, I don't think the user should be able to specify both (because it could be error prone?)

I think I agree, but since the interface allows both, we have to respect that for now.

alright, can we leave an issue open about this? I feel like if we're gonna muck with the API, now would be the time.

created #1949

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

pkg/signature/kms/cliplugin/handler/methods.go

pkg/signature/kms/cliplugin/signer.go

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 added 21 commits January 23, 2025 12:06

squash from ramonpetgrave64-kms-plugin-cli-signmessage

476fa0b

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

remove parallels for tests that try to createKey

5888e88

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

fix parallelism

ae5cb4d

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

readmne update

4490691

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

cleanup

7ff16f1

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add serialization tests

b5efb76

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

move pack and unpack to new encoding package

66b3260

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

384b5a4

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add hint about json shape

a7b41c5

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

clenaup

87c3c86

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

4ba7d98

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

better encoding for options

cd95101

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

3d99449

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

discard cancel

27f4ba6

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

f0f6813

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

undo

c18e0a3

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

cleanup log

0545f64

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

base. todo: implementation program

c19c06a

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add signer program test

3fecb08

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Merge branch 'main' into ramonpetgrave64-kms-plugin-cli-verifysignature

7016e90

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

typo

6e7aedc

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 marked this pull request as ready for review January 24, 2025 22:54

ramonpetgrave64 added 3 commits January 27, 2025 09:46

interface_test for verifySignature, doc fixes

04c6604

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

do not run TestCreateKey in parallel, pem file readonly

4562202

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

add missing pluginResp test for verifySignature

4746973

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

Hayden-IO reviewed Jan 27, 2025

View reviewed changes

typo

8e899b0

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 requested a review from Hayden-IO January 27, 2025 18:05

Hayden-IO previously approved these changes Jan 27, 2025

View reviewed changes

Hayden-IO requested a review from loosebazooka January 27, 2025 18:43

This was referenced Jan 27, 2025

cliplugin: add SupportedAlgorithms(), PublicKey(), and CryptoSigner(). #1946

Merged

add initial plugin support for KMSs #1901

Merged

loosebazooka reviewed Jan 28, 2025

View reviewed changes

ramonpetgrave64 added 2 commits January 28, 2025 14:43

typo

8d3ab7e

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

separate test cases for TestPluginArgsJSON

e7889b7

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

ramonpetgrave64 dismissed Hayden-IO’s stale review via e7889b7 January 28, 2025 20:13

ramonpetgrave64 requested a review from loosebazooka January 28, 2025 20:16

loosebazooka approved these changes Jan 29, 2025

View reviewed changes

ramonpetgrave64 mentioned this pull request Jan 29, 2025

SignerVerifier: providing either a message or a digest #1949

Open

bobcallaway reviewed Jan 29, 2025

View reviewed changes

pkg/signature/kms/cliplugin/handler/methods.go Outdated Show resolved Hide resolved

pkg/signature/kms/cliplugin/signer.go Outdated Show resolved Hide resolved

ramonpetgrave64 added 2 commits January 29, 2025 12:35

rename stdin to message, discard message when not needed

004e595

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

tidy err return

3f09001

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

bobcallaway approved these changes Jan 29, 2025

View reviewed changes

bobcallaway merged commit dac6ce0 into sigstore:main Jan 29, 2025
9 checks passed

Conversation

ramonpetgrave64 commented Jan 24, 2025

Summary

Release Note

Documentation

Uh oh!

ramonpetgrave64 commented Jan 24, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramonpetgrave64 Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramonpetgrave64 Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ramonpetgrave64 Jan 27, 2025 •

edited

Loading

ramonpetgrave64 Jan 27, 2025 •

edited

Loading