feat: Cosign verifier based on Cosign CLI by junczhu · Pull Request #10 · notaryproject/ratify-verifier-go

junczhu · 2025-02-13T08:46:56Z

What this PR does / why we need it:

Added design document

Implement cosign verifier

verify cosign signature including both with key and keyless verification
truststore mapping, including keys, certificates, and certchains for verification
using VerfierOptions, is for verifier creatation and VerifyOption and VerifyContext for verification

Which issue(s) this PR resolves

Resolves #39

Please check the following list:

Does the affected code have corresponding tests, e.g. unit test?
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have an appropriate license header?

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

codecov-commenter · 2025-02-13T10:11:20Z

Codecov Report

Attention: Patch coverage is 38.24701% with 155 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cosign/verifier.go	29.03%	145 Missing and 9 partials ⚠️
...osign/verifycontextoptions/verifycontextoptions.go	90.00%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (38.24%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (48.53%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Files with missing lines	Coverage Δ
cosign/truststore/truststore.go	`100.00% <100.00%> (ø)`
...osign/verifycontextoptions/verifycontextoptions.go	`90.00% <90.00%> (ø)`
cosign/verifier.go	`29.03% <29.03%> (ø)`

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

binbin-li · 2025-02-17T04:39:14Z

cosign/truststore.go

+	certChains map[string][]*x509.Certificate
+}
+
+func NewWithOpts(opts *VerifierOptions) TrustStore {


NewTrustStore?

does opts get used?

A1: Shall we rename or create a new package for this structure?
A2: I would read input from opts

use NewTrustStore

cosign/truststore.go

binbin-li · 2025-02-17T04:44:12Z

cosign/truststore.go

+}
+
+func (t *TrustStoreImp) GetVerifyOpts(subjectRef string) (*VOptions, error) {
+	return t.optsMap[subjectRef], nil


should we return err if it's not existing.

cosign/verifier.go

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu · 2025-02-18T06:42:51Z

Gonna update the remote branch to apply those newly merged changes.

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

binbin-li

could you briefly explain how to support distinguish cert/keys for different repos/registries.

cosign/truststore/truststore.go

cosign/verifycontextoptions/verifycontextoptions.go

binbin-li · 2025-02-20T07:52:44Z

cosign/verifycontextoptions/verifycontextoptions.go

+}
+
+type verifyContextOptions struct {
+	optsMap map[string]*VerifyContext


the map seems mapping a digested reference to a context, which means users need to configure the context for each artifact that will be validated. It would be a huge work for users. Does all options in this VerifyContext vary for each artifact?

Would it be better to have a mapping function

binbin-li · 2025-02-20T08:19:14Z

cosign/verifier.go

+		return nil, v1.Hash{}, fmt.Errorf("unable to locate reference with artifactType %s", artifactTypeCosign)
+	}
+
+	signatureDesc := signatureDescriptors[numResults-1]


seems it's an experimental feature for cosign, probably we still need to support the main user scenario. cc: @shizhMSFT

cosign/verifier.go

binbin-li · 2025-02-20T08:30:12Z

cosign/verifycontextoptions/verifycontextoptions.go

+	GetVerifyOpts(subjectRef string) (*VerifyContext, error)
+}
+
+type verifyContextOptions struct {


we should either expose it or have a constructor to create it with values.

binbin-li · 2025-02-20T08:31:24Z

cosign/verifycontextoptions/verifycontextoptions.go

+)
+
+// VerifyContext holds the options for verifying a context.
+type VerifyContext struct {


we could remove some options if they are not required in this PR

Those ones are filtered and indeed used in this PR.
I would keep an eye on the changes based on the change of PR

cosign/truststore/truststore.go

shizhMSFT

I would request a markdown of the design. Otherwise, it is too difficult to understand. Please provide an outline first, and then fill in the details.

cosign/go.mod

cosign/truststore/truststore.go

junczhu · 2025-02-17T05:54:32Z

cosign/go.mod

+
+go 1.23.4
+
+toolchain go1.23.6


need to remove

cosign/truststore.go

junczhu · 2025-02-17T05:59:32Z

cosign/truststore.go

+	IgnoreSCT     bool
+}
+
+type TrustStoreImp struct {


junczhu · 2025-02-17T06:00:13Z

cosign/truststore.go

+	certChains map[string][]*x509.Certificate
+}
+
+func NewWithOpts(opts *VerifierOptions) TrustStore {


use NewTrustStore

junczhu · 2025-02-17T06:01:00Z

cosign/truststore.go

+}
+
+func (t *TrustStoreImp) GetVerifyOpts(subjectRef string) (*VOptions, error) {
+	return t.optsMap[subjectRef], nil


junczhu · 2025-02-20T09:00:16Z

cosign/verifycontextoptions/verifycontextoptions.go

+}
+
+type verifyContextOptions struct {
+	optsMap map[string]*VerifyContext


Would it be better to have a mapping function

junczhu · 2025-02-20T09:09:11Z

cosign/verifier.go

+		Verifier: v,
+	}
+	// TODO: update verify result
+	_, err = cosign.VerifyImageSignature(ctx, sig, signatureDescHash, checkOpts)


You are right, We should also consider cases that fail to verify but no error.

cosign/verifier.go

cosign/truststore/truststore.go

cosign/go.mod

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu · 2025-02-26T06:46:06Z

Convert to draft as a PoC

feat: cosign verifier

b51ac18

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch from dce27bf to b51ac18 Compare February 13, 2025 09:53

feat: cosign verifier fix end of line

e9eab8e

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu added 2 commits February 14, 2025 00:19

feat: update keyless verify

062bfe8

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

feat: update keyless verify 2

414a753

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch 5 times, most recently from 05d3557 to 59240a2 Compare February 14, 2025 07:18

feat: update unit test

f0aaace

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch from 59240a2 to f0aaace Compare February 14, 2025 07:29

junczhu added 2 commits February 16, 2025 22:34

feat: refactor 1

5f90eff

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

feat: refactor 2

8671266

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch 2 times, most recently from 513e55b to db55766 Compare February 17, 2025 04:35

feat: refactor 3

7a13f1c

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch from db55766 to 7a13f1c Compare February 17, 2025 04:36

binbin-li reviewed Feb 17, 2025

View reviewed changes

junczhu force-pushed the cosign-verifier branch 2 times, most recently from 49149f1 to 9c25283 Compare February 18, 2025 06:34

refactor: update structs and add todos

fd733b2

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch from 9c25283 to fd733b2 Compare February 18, 2025 06:35

junczhu added 2 commits February 18, 2025 17:43

Merge branch 'ratify-project:main' into cosign-verifier

9d01ab0

chore: update Store API

1538753

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu marked this pull request as ready for review February 18, 2025 07:29

junczhu added 2 commits February 19, 2025 03:50

chore: update comments for readability

497463a

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

chore: update verify context

50309df

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

chore: update verify context 2

af9542c

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

binbin-li reviewed Feb 20, 2025

View reviewed changes

shizhMSFT requested changes Feb 20, 2025

View reviewed changes

cosign/go.mod Outdated Show resolved Hide resolved

cosign/truststore/truststore.go Outdated Show resolved Hide resolved

junczhu commented Feb 21, 2025

View reviewed changes

junczhu added 2 commits February 21, 2025 02:37

chore: resolve comments

d23da5d

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

chore: draft design doc

a7cef57

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu marked this pull request as draft February 21, 2025 06:02

junczhu added 5 commits February 21, 2025 07:19

docs: update design doc

bd4c4fe

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

docs: update design doc

7d8342c

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

docs: update design doc

daffb32

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

feat: update the verifier metadata

d0e3b85

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

feat: resolve comments

da7e072

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu force-pushed the cosign-verifier branch from c3897f5 to da7e072 Compare February 23, 2025 19:28

test: added unit tests

93d765b

Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>

junczhu marked this pull request as ready for review February 24, 2025 00:59

junczhu marked this pull request as draft February 26, 2025 06:45

junczhu changed the title ~~feat: cosign verifier~~ feat: Cosign verifier based on Cosign CLI Mar 12, 2025

junczhu closed this Mar 17, 2025

Conversation

junczhu commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Uh oh!

codecov-commenter commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junczhu commented Feb 18, 2025

Uh oh!

binbin-li left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junczhu commented Feb 26, 2025

junczhu commented Feb 13, 2025 •

edited

Loading

codecov-commenter commented Feb 13, 2025 •

edited

Loading