feat: cert-extensions verify #1626

developer-guy · 2022-03-17T09:27:09Z

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Co-authored-by: Christian Kotzbauer <@ckotzbauer1>

Summary

This PR will add support for verifying additional claims within the signature cert.

$ COSIGN_EXPERIMENTAL=1 cosign verify aquasec/trivy:0.24.3 --cert-extensions githubWorkflowRepository=aquasecurity/trivy

Ticket Link

Fixes #1625 and #1989

Release Note

feat: cert-extensions verify

🚨 Include some breaking changes, I moved some logic into a different package which might cause breaking changes issues.

cc: @ckotzbauer1

codecov-commenter · 2022-03-17T09:47:59Z

Codecov Report

Merging #1626 (fd446ed) into main (1832b4c) will decrease coverage by 0.31%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1626      +/-   ##
==========================================
- Coverage   27.97%   27.66%   -0.32%     
==========================================
  Files         137      140       +3     
  Lines        7820     7872      +52     
==========================================
- Hits         2188     2178      -10     
- Misses       5403     5465      +62     
  Partials      229      229

Impacted Files	Coverage Δ
cmd/cosign/cli/options/certextensions.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/verify.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/policy_init.go	`1.43% <0.00%> (ø)`
cmd/cosign/cli/verify.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/verify/verify.go	`0.00% <0.00%> (ø)`
pkg/cosign/certextensions.go	`0.00% <0.00%> (ø)`
pkg/cosign/verifiers.go	`0.00% <0.00%> (ø)`
pkg/cosign/verify.go	`11.46% <0.00%> (-0.21%)`	⬇️
pkg/signature/certextensions.go	`0.00% <0.00%> (ø)`
pkg/signature/keys.go	`14.68% <ø> (-4.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1832b4c...fd446ed. Read the comment docs.

developer-guy · 2022-03-17T10:24:08Z

PTAL @ckotzbauer

ckotzbauer

Thanks @developer-guy for adopting this logic! I think the code overall looks good and solid. It should not break the logic from Kyverno on the next update as the extensions-parameter is not used there (apart from the new location of some functions)

ckotzbauer · 2022-03-17T11:35:24Z

pkg/cosign/verify.go

+		}
+	}
+	return true
+}


I'm not sure about this function. Kyverno ignores empty-string values (same as the extension would not be required) and respects shortened values or wildcards. However, it is of course not mandatory that cosign will do the same here. WDYT?

It is the function that makes the matching between the actual one and the expected one to decide whether claims are equal, I couldn't understand your comment :/

Yes, correct. What I mean is, that this function checks for strict equality, wildcards and shortened values are not supported.
E.g. cosign verify aquasec/trivy:0.24.3 --cert-extensions githubWorkflowRepository=aquasecurity/* won't work (with or without asterisk). This is how Kyverno would validate: https://github.com/kyverno/kyverno/blob/main/pkg/cosign/cosign.go#L419-L421

It is not required to make the validation equal to the Kyverno logic, just want to mention the difference 😉

haydentherapper

I'd like to think more about the UX for this. How does the user discover the names of the supported custom OIDs? githubWorkflowTrigger is an internal name in code, I don't think it should become a flag name as-is.

haydentherapper · 2022-03-17T20:55:43Z

cmd/cosign/cli/options/verify.go

@@ -34,6 +34,7 @@ type VerifyOptions struct {
 	Registry        RegistryOptions
 	SignatureDigest SignatureDigestOptions
 	AnnotationOptions
+	CertExtensionOptions


Since this is for verifying certificates, this should be under the existing CertVerifyOptions.

Bumping this comment, we should group this under CertVerifyOptions so it gets pulled into all of the verify commands automatically

haydentherapper · 2022-03-17T20:56:11Z

pkg/cosign/certextensions.go

+
+var (
+	// Fulcio cert-extensions, documented here: https://github.com/sigstore/fulcio/blob/main/docs/oid-info.md
+	CertExtensionOIDCIssuer               = "1.3.6.1.4.1.57264.1.1"


This is already covered -

cosign/cmd/cosign/cli/options/certificate.go

Line 25 in 079e28d

CertOidcIssuer string

See #1308 where it was added.

Bumping this comment too - We shouldn't have two ways to verify the issuer

haydentherapper · 2022-03-17T21:04:26Z

cc @asraa
There was related conversation on this topic in #1313, but focused on subject verification.

developer-guy · 2022-03-18T07:32:09Z

I'd like to think more about the UX for this. How does the user discover the names of the supported custom OIDs? githubWorkflowTrigger is an internal name in code, I don't think it should become a flag name as-is.

We'll be creating documentation about this to help people to discover these certain extensions about what we are adding to certificates, does it make sense?

haydentherapper · 2022-03-18T18:25:22Z

I'm wondering if this feature makes sense as flags or if policy should be a part of a file, since this is meant to be repeated for every command, and I'd assume users would want to check in their policy to audit.

dlorenc · 2022-03-18T22:58:52Z

I'm wondering if this feature makes sense as flags or if policy should be a part of a file, since this is meant to be repeated for every command, and I'd assume users would want to check in their policy to audit.

I don't see an issue with flags and a policy file. Both make sense to me.

dlorenc · 2022-03-18T23:01:19Z

We'll be creating documentation about this to help people to discover these certain extensions about what we are adding to certificates, does it make sense?

I think we should document the canonical strings/field names somewhere, probably near where the OIDs themselves live, then use those here. I agree the variable names themselves are probably not the best ux.

developer-guy · 2022-05-12T13:06:53Z

kindly ping @dlorenc

haydentherapper · 2022-05-25T16:35:21Z

pkg/cosign/certextensions.go

+
+var (
+	// Fulcio cert-extensions, documented here: https://github.com/sigstore/fulcio/blob/main/docs/oid-info.md
+	CertExtensionOIDCIssuer               = "1.3.6.1.4.1.57264.1.1"


Bumping this comment too - We shouldn't have two ways to verify the issuer

haydentherapper · 2022-05-25T16:35:49Z

cmd/cosign/cli/options/verify.go

@@ -34,6 +34,7 @@ type VerifyOptions struct {
 	Registry        RegistryOptions
 	SignatureDigest SignatureDigestOptions
 	AnnotationOptions
+	CertExtensionOptions


Bumping this comment, we should group this under CertVerifyOptions so it gets pulled into all of the verify commands automatically

haydentherapper · 2022-05-25T16:39:20Z

pkg/cosign/verifiers.go

@@ -51,11 +51,18 @@ func SimpleClaimVerifier(sig oci.Signature, imageDigest v1.Hash, annotations map
 			return errors.New("missing or incorrect annotation")
 		}
 	}
+


I'm not sure what the purpose of this function is, but I'd strongly prefer we continue to group certificate verification under this function -

cosign/pkg/cosign/verify.go

Line 203 in 03e66aa

func CheckCertificatePolicy(cert *x509.Certificate, co *CheckOpts) error {

This goes back to the earlier point too about the existing flag for OIDC issuer.

haydentherapper · 2022-05-25T16:42:12Z

cmd/cosign/cli/options/certextensions.go

+
+// AddFlags implements Interface
+func (o *CertExtensionOptions) AddFlags(cmd *cobra.Command) {
+	cmd.Flags().StringSliceVarP(&o.CertExtensions, "cert-extensions", "", nil,


I think we discussed this on the comments, but it'd be preferrable to match the existing style of a flag per extension instead of have a map.

See https://github.com/sigstore/cosign/blob/aad7c5fcd6e5f854ae48a57e42919039e6d219c0/cmd/cosign/cli/options/certificate.go for existing flags. We should add flags like certificate-github-ref, etc

haydentherapper · 2022-06-02T20:17:18Z

pkg/cosign/verify.go

@@ -71,8 +71,10 @@ type CheckOpts struct {

 	// Annotations optionally specifies image signature annotations to verify.
 	Annotations map[string]interface{}
+	// CertExtensions optionally specifies image signature cert extensions to verify.
+	CertExtensions map[string]string


In the same vein, moving away from a map, I think we should have a field per extension - CertGitHubSha, CertGitHubRef, etc - If this is used a library function, it's not as clean of an interface to take a map, because there's no way to know what is a valid string.

haydentherapper

Two high-level comments:

We need to make sure that the verify* commands do not diverge. This means that if we add a verify flag for cosign verify, it should work for verify, verify attestation, verify blob, verify manifest, and verify dockerfile.
Most of these changes lack tests. For all of the changes to pkg, please add tests.

haydentherapper · 2022-06-06T22:46:45Z

pkg/cosign/verify.go

@@ -90,6 +90,14 @@ type CheckOpts struct {
 	CertEmail string
 	// CertOidcIssuer is the OIDC issuer expected for a certificate to be valid. The empty string means any certificate can be valid.
 	CertOidcIssuer string
+
+	// CertGithubWorkflowTrigger is the GitHub Workflow Trigger name expected for a certificate to be valid. The empty string means any certificate can be valid.
+	CertGithubWorkflowTrigger    string


nit: Comments for all

yep, thank you

haydentherapper · 2022-06-06T22:50:17Z

cmd/cosign/cli/options/certextensions.go

+	sigs "github.com/sigstore/cosign/pkg/signature"
+)
+
+// CertExtensionOptions is the top level wrapper for the annotations.


Looks like this file is not used, delete?

Agree, thank you good catch.

haydentherapper · 2022-06-06T22:55:23Z

pkg/cosign/certextensions.go

@@ -0,0 +1,84 @@
+//


We already have tests in the keys_test.go file called TestCertExtensions

Can we copy over the tests to a new file? Otherwise, it's not clear that these are tested.

haydentherapper · 2022-06-06T22:57:04Z

pkg/cosign/verify.go

-			return errors.New("expected oidc issuer not found in certificate")
-		}
+
+	if err := validateCertExtensions(ce, co); err != nil {


Tests? See

cosign/pkg/cosign/verify_test.go

Line 515 in 802f0a2

func TestValidateAndUnpackCertInvalidOidcIssuer(t *testing.T) {

for examples

GenerateLeafCert in the test utils will need to be updated to support adding all extensions

haydentherapper · 2022-06-06T22:57:20Z

pkg/signature/certextensions.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package signature


File unused, delete?

bump, can this be deleted?

yep, deleted, thank you.

haydentherapper · 2022-06-06T23:01:02Z

cmd/cosign/cli/verify/verify.go

@@ -48,22 +48,27 @@ import (
 // nolint


Also need to update:

https://github.com/sigstore/cosign/blob/f2c360eb97e52fa7766ecde370f1a48b910d7404/cmd/cosign/cli/dockerfile.go

https://github.com/sigstore/cosign/blob/f2c360eb97e52fa7766ecde370f1a48b910d7404/cmd/cosign/cli/manifest.go

https://github.com/sigstore/cosign/blob/03e66aad02cf7c987ea489cdeda1fd580b6b1fc6/cmd/cosign/cli/verify/verify_attestation.go

https://github.com/sigstore/cosign/blob/03e66aad02cf7c987ea489cdeda1fd580b6b1fc6/cmd/cosign/cli/verify/verify_blob.go

And also update where o.CertVerify.X is added to the command, like line 179 in cmd/cosign/cli/verify.go and in Verify Blob (

cosign/cmd/cosign/cli/verify.go

Line 260 in 03e66aa

o.CertVerify.CertEmail, o.CertVerify.CertOidcIssuer, o.CertVerify.CertChain,

)

thank you, I missed those parts.

thanks for updating these!

haydentherapper · 2022-06-13T01:24:09Z

pkg/signature/certextensions.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package signature


bump, can this be deleted?

haydentherapper · 2022-06-13T01:24:53Z

cmd/cosign/cli/verify/verify.go

@@ -48,22 +48,27 @@ import (
 // nolint


thanks for updating these!

haydentherapper · 2022-06-13T01:24:57Z

pkg/cosign/certextensions.go

@@ -0,0 +1,84 @@
+//


Can we copy over the tests to a new file? Otherwise, it's not clear that these are tested.

haydentherapper · 2022-06-13T01:28:30Z

test/cert_utils.go

@@ -131,7 +132,12 @@ func GenerateLeafCert(subject string, oidcIssuer string, parentTemplate *x509.Ce
 			Id:       asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 1},
 			Critical: false,
 			Value:    []byte(oidcIssuer),
-		}},
+		},


nit: To avoid having to update all places where GenerateLeafCert is called, can we create a new function like GenerateLeafCertWithGitHubOIDs or something?

that makes a lot of sense, thank you, updated.

haydentherapper

Just one thing to update with verify-attestation, looks good otherwise!

haydentherapper · 2022-06-13T17:16:31Z

cmd/cosign/cli/verify.go

+				Annotations:                  annotations,
+				HashAlgorithm:                hashAlgorithm,
+				SignatureRef:                 o.SignatureRef,
+				LocalImage:                   o.LocalImage,


Also need to update VerifyAttestation with the new verify options, on line 185

good catch @haydentherapper as always

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-authored-by: Christian Kotzbauer <@ckotzbauer1> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

haydentherapper

Thanks for the updates, this looks great!

developer-guy requested review from dlorenc, bobcallaway and cpanato March 17, 2022 09:27

developer-guy self-assigned this Mar 17, 2022

developer-guy force-pushed the feature/1625 branch 2 times, most recently from c93432d to fd446ed Compare March 17, 2022 09:45

ckotzbauer reviewed Mar 17, 2022

View reviewed changes

haydentherapper requested changes Mar 17, 2022

View reviewed changes

developer-guy force-pushed the feature/1625 branch 2 times, most recently from 8f38160 to 3f7c561 Compare May 25, 2022 08:32

developer-guy requested a review from haydentherapper May 25, 2022 08:56

haydentherapper requested changes May 25, 2022

View reviewed changes

developer-guy force-pushed the feature/1625 branch from 3f7c561 to 517c33c Compare June 1, 2022 21:25

haydentherapper requested changes Jun 2, 2022

View reviewed changes

developer-guy changed the title ~~feat: cert-extensions verify~~ [W.I.P] feat: cert-extensions verify Jun 3, 2022

developer-guy force-pushed the feature/1625 branch 3 times, most recently from 0788d39 to fbb5437 Compare June 3, 2022 18:37

developer-guy changed the title ~~[W.I.P] feat: cert-extensions verify~~ feat: cert-extensions verify Jun 3, 2022

developer-guy requested a review from haydentherapper June 3, 2022 18:37

developer-guy force-pushed the feature/1625 branch 2 times, most recently from 7225580 to 802f0a2 Compare June 3, 2022 18:43

haydentherapper requested changes Jun 6, 2022

View reviewed changes

developer-guy force-pushed the feature/1625 branch 5 times, most recently from 1f1bbea to 777965f Compare June 12, 2022 19:59

developer-guy requested a review from haydentherapper June 12, 2022 20:01

developer-guy force-pushed the feature/1625 branch 2 times, most recently from 218ed2b to 6a25723 Compare June 12, 2022 21:42

haydentherapper requested changes Jun 13, 2022

View reviewed changes

developer-guy force-pushed the feature/1625 branch from 6a25723 to e4b9e30 Compare June 13, 2022 06:42

developer-guy requested a review from haydentherapper June 13, 2022 06:43

developer-guy force-pushed the feature/1625 branch from e4b9e30 to 3b6e20a Compare June 13, 2022 06:44

haydentherapper requested changes Jun 13, 2022

View reviewed changes

feat: cert-extensions verify

3c5a6f3

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-authored-by: Christian Kotzbauer <@ckotzbauer1> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

developer-guy force-pushed the feature/1625 branch from 3b6e20a to 3c5a6f3 Compare June 13, 2022 18:25

developer-guy requested a review from haydentherapper June 13, 2022 19:09

haydentherapper approved these changes Jun 13, 2022

View reviewed changes

thepwagner mentioned this pull request Jun 28, 2022

dockerfile-verify: inline certificate verification options #2031

Closed

dlorenc approved these changes Jun 29, 2022

View reviewed changes

dlorenc merged commit 890cec1 into sigstore:main Jun 29, 2022

github-actions bot added this to the v1.10.0 milestone Jun 29, 2022

chipzoller mentioned this pull request Jun 29, 2022

print cert-extensions if any in verification message #1989

Closed

haydentherapper mentioned this pull request Jul 14, 2022

Discourage (disallow?) naked cosign verify invocations #2056

Closed

jeff-mccoy mentioned this pull request Jul 28, 2022

Update module github.com/sigstore/cosign to v1.10.0 defenseunicorns/zarf#638

Merged

1 task

haydentherapper mentioned this pull request Aug 12, 2022

Verifying GitHub specific claims sigstore/sigstore-python#157

Closed

haydentherapper mentioned this pull request Aug 30, 2022

Verify certificate against job_workflow_ref string #1313

Closed

feat: cert-extensions verify #1626

feat: cert-extensions verify #1626

Conversation

developer-guy commented Mar 17, 2022 • edited Loading

Summary

Ticket Link

Release Note

codecov-commenter commented Mar 17, 2022 • edited Loading

Codecov Report

developer-guy commented Mar 17, 2022

ckotzbauer left a 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

haydentherapper left a 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

haydentherapper commented Mar 17, 2022

developer-guy commented Mar 18, 2022

haydentherapper commented Mar 18, 2022

dlorenc commented Mar 18, 2022

dlorenc commented Mar 18, 2022

developer-guy commented May 12, 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

haydentherapper left a 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

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

haydentherapper Jun 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

developer-guy commented Mar 17, 2022 •

edited

Loading

codecov-commenter commented Mar 17, 2022 •

edited

Loading

haydentherapper Jun 6, 2022 •

edited

Loading