Add Cosign keyless mode required args for nerdctl pull and run #2185

ningziwen · 2023-04-15T08:44:09Z

These are some alternatives.

Simplify the X and X-regexp to one generic arg. Seems too complicated.
Just set wild regex as the two required args to Cosign without making it configurable. However the required args are more like user facing critical features. Users have the need to limit the issuer and certificate.

Tested locally.

Compose may also needs to be fixed. Will do in a separated PR to make the PR small.

AkihiroSuda · 2023-04-15T11:34:27Z

cmd/nerdctl/container_run.go

+	cmd.Flags().String("cosign-certificate-identity", "", "The identity expected in a valid Fulcio certificate for --verify=cosign. Valid values include email address, DNS names, IP addresses, and URIs. Either --cosign-certificate-identity or --cosign-certificate-identity-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-identity-regexp", "", "A regular expression alternative to --cosign-certificate-identity for --verify=cosign. Accepts the Go regular expression syntax described at https://golang.org/s/re2syntax. Either --cosign-certificate-identity or --cosign-certificate-identity-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-oidc-issuer", "", "The OIDC issuer expected in a valid Fulcio certificate for --verify=cosign, e.g. https://token.actions.githubusercontent.com or https://oauth2.sigstore.dev/auth. Either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-oidc-issuer-regexp", "", "A regular expression alternative to --certificate-oidc-issuer for --verify=cosign. Accepts the Go regular expression syntax described at https://golang.org/s/re2syntax. Either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp must be set for keyless flows")


Can we support compose too?
See #1508

I wanted to address in a separated PR because there is no integration test for keyless now. I feel it is heavy-handed for both author and reviewer to manually test too many things in one PR. #2185 (comment)

But I'm also ok to add it, so let me know if you really want them to be in same PR.

I wanted to address in a separated PR because there is no integration test for keyless now. I feel it is heavy-handed for both author and reviewer to manually test too many things in one PR. #2185 (comment)

But I'm also ok to add it, so let me know if you really want them to be in same PR.

LGTM

fahedouch · 2023-04-20T14:50:14Z

pkg/signutil/cosignutil.go

@@ -92,6 +95,24 @@ func VerifyCosign(ctx context.Context, rawRef string, keyRef string, hostsDirs [
 	if keyRef != "" {
 		cosignCmd.Args = append(cosignCmd.Args, "--key", keyRef)
 	} else {
+		if certIdentity == "" && certIdentityRegexp == "" {


this shouldn't block if certOidcIssuer != "" || certOidcIssuerRegexp != ""

It is blocked in Cosign. https://github.com/sigstore/cosign/blob/v2.0.1/cmd/cosign/cli/options/certificate.go#L94

this is contradictory with this statement

I wanted to say

(Either --cosign-certificate-identity or --cosign-certificate-identity-regexp) and (either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp) must be set for keyless flows.

either... or -> ||
and -> &&

OK could you please make the sentence simpler may be by splitting it

Reworded to "For keyless flows to work, you need to set either --cosign-certificate-identity or --cosign-certificate-identity-regexp, and either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp."

fahedouch

Thanks

ktock · 2023-04-22T05:38:23Z

cmd/nerdctl/container_run.go

+	cmd.Flags().String("cosign-certificate-identity", "", "The identity expected in a valid Fulcio certificate for --verify=cosign. Valid values include email address, DNS names, IP addresses, and URIs. Either --cosign-certificate-identity or --cosign-certificate-identity-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-identity-regexp", "", "A regular expression alternative to --cosign-certificate-identity for --verify=cosign. Accepts the Go regular expression syntax described at https://golang.org/s/re2syntax. Either --cosign-certificate-identity or --cosign-certificate-identity-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-oidc-issuer", "", "The OIDC issuer expected in a valid Fulcio certificate for --verify=cosign, e.g. https://token.actions.githubusercontent.com or https://oauth2.sigstore.dev/auth. Either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp must be set for keyless flows")
+	cmd.Flags().String("cosign-certificate-oidc-issuer-regexp", "", "A regular expression alternative to --certificate-oidc-issuer for --verify=cosign. Accepts the Go regular expression syntax described at https://golang.org/s/re2syntax. Either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp must be set for keyless flows")


Could you update docs/command-reference.md as well?
It would be great if we can update tests in /cmd/nerdctl/container_run_verify_linux_test.go. (Is #2184 testable in CI?)

Updated the doc.
Keyless requires complicated prompt, which includes the third party authentication in website so it is not testable in CI now.

ktock · 2023-04-22T05:38:25Z

docs/cosign.md

-$ nerdctl pull --verify=cosign devopps/hello-world
+$ nerdctl pull --verify=cosign devopps/hello-world --certificate-identity=name@example.com --certificate-oidc-issuer=https://accounts.example.com


Flag order should be nerdctl pull [flags] imagename

ktock · 2023-04-25T14:39:24Z

docs/cosign.md

@@ -70,9 +70,11 @@ Verify the container image while pulling:

 > REMINDER: Image won't be pulled if there are no matching signatures in case you passed `--verify` flag.

+> REMINDER: For keyless flows to work, you need to set either --cosign-certificate-identity or --cosign-certificate-identity-regexp, and either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp. The oidc-issuer for Google is https://accounts.google.com, Microsoft is https://login.microsoftonline.com, and GitHub is https://github.com/login/oauth.


Can we have the same description as the flag description? e.g. The OIDC issuer expected in a valid Fulcio certificate, e.g. https://token.actions.githubusercontent.com or https://oauth2.sigstore.dev/auth.

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>

ningziwen force-pushed the keyless branch from 28db1fd to 98383fd Compare April 15, 2023 08:53

ningziwen changed the title ~~Add Cosign keyless mode required args~~ Add Cosign keyless mode required args for nerdctl pull and run Apr 15, 2023

fahedouch added this to the v1.4.0 (tentative) milestone Apr 15, 2023

fahedouch added the area/cosign cosign label Apr 15, 2023

AkihiroSuda reviewed Apr 15, 2023

View reviewed changes

fahedouch self-requested a review April 19, 2023 16:36

fahedouch reviewed Apr 20, 2023

View reviewed changes

ningziwen force-pushed the keyless branch from 98383fd to b4b10d7 Compare April 20, 2023 23:38

ningziwen requested a review from fahedouch April 20, 2023 23:38

fahedouch approved these changes Apr 21, 2023

View reviewed changes

AkihiroSuda requested a review from ktock April 21, 2023 15:28

ktock reviewed Apr 22, 2023

View reviewed changes

ningziwen force-pushed the keyless branch 2 times, most recently from d0d107b to 3c300b7 Compare April 22, 2023 06:39

fahedouch requested a review from ktock April 25, 2023 13:37

ktock reviewed Apr 25, 2023

View reviewed changes

Add Cosign keyless mode required args for nerdctl pull and run

d292f7b

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>

ningziwen force-pushed the keyless branch from 3c300b7 to d292f7b Compare April 25, 2023 19:56

ningziwen requested a review from ktock April 25, 2023 19:56

ktock approved these changes Apr 26, 2023

View reviewed changes

fahedouch merged commit 6fafcbf into containerd:main Apr 26, 2023

mzaian mentioned this pull request May 22, 2023

[nerdctl] upgrade to version 1.4.0 kubernetes-sigs/kubespray#10119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cosign keyless mode required args for nerdctl pull and run #2185

Add Cosign keyless mode required args for nerdctl pull and run #2185

ningziwen commented Apr 15, 2023 •

edited

Loading

AkihiroSuda Apr 15, 2023

ningziwen Apr 16, 2023 •

edited

Loading

fahedouch Apr 19, 2023

fahedouch Apr 20, 2023

ningziwen Apr 20, 2023

fahedouch Apr 20, 2023

ningziwen Apr 20, 2023

fahedouch Apr 20, 2023

ningziwen Apr 20, 2023

fahedouch Apr 21, 2023

fahedouch left a comment

ktock Apr 22, 2023

ningziwen Apr 22, 2023 •

edited

Loading

ktock Apr 22, 2023

ktock Apr 25, 2023

ningziwen Apr 25, 2023

		$ nerdctl pull --verify=cosign devopps/hello-world
		$ nerdctl pull --verify=cosign devopps/hello-world --certificate-identity=name@example.com --certificate-oidc-issuer=https://accounts.example.com

		@@ -70,9 +70,11 @@ Verify the container image while pulling:

		> REMINDER: Image won't be pulled if there are no matching signatures in case you passed `--verify` flag.

		> REMINDER: For keyless flows to work, you need to set either --cosign-certificate-identity or --cosign-certificate-identity-regexp, and either --cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp. The oidc-issuer for Google is https://accounts.google.com, Microsoft is https://login.microsoftonline.com, and GitHub is https://github.com/login/oauth.

Add Cosign keyless mode required args for nerdctl pull and run #2185

Add Cosign keyless mode required args for nerdctl pull and run #2185

Conversation

ningziwen commented Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

ningziwen Apr 16, 2023 • 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

Choose a reason for hiding this comment

fahedouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningziwen Apr 22, 2023 • 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

ningziwen commented Apr 15, 2023 •

edited

Loading

ningziwen Apr 16, 2023 •

edited

Loading

ningziwen Apr 22, 2023 •

edited

Loading