-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Cosign keyless mode required args for nerdctl pull and run #2185
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,6 +277,10 @@ func setCreateFlags(cmd *cobra.Command) { | |
return []string{"none", "cosign", "notation"}, cobra.ShellCompDirectiveNoFileComp | ||
}) | ||
cmd.Flags().String("cosign-key", "", "Path to the public key file, KMS, URI or Kubernetes Secret for --verify=cosign") | ||
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") | ||
Comment on lines
+280
to
+283
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the doc. |
||
// #endregion | ||
|
||
cmd.Flags().String("ipfs-address", "", "multiaddr of IPFS API (default uses $IPFS_PATH env variable if defined or local directory ~/.ipfs)") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ package signutil | |
import ( | ||
"bufio" | ||
"context" | ||
"errors" | ||
"os" | ||
"os/exec" | ||
"strings" | ||
|
@@ -65,7 +66,9 @@ func SignCosign(rawRef string, keyRef string) error { | |
|
||
// VerifyCosign verifies an image(`rawRef`) with a cosign public key(`keyRef`) | ||
// `hostsDirs` are used to resolve image `rawRef` | ||
func VerifyCosign(ctx context.Context, rawRef string, keyRef string, hostsDirs []string) (string, error) { | ||
// 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. | ||
func VerifyCosign(ctx context.Context, rawRef string, keyRef string, hostsDirs []string, | ||
certIdentity string, certIdentityRegexp string, certOidcIssuer string, certOidcIssuerRegexp string) (string, error) { | ||
digest, err := imgutil.ResolveDigest(ctx, rawRef, false, hostsDirs) | ||
if err != nil { | ||
logrus.WithError(err).Errorf("unable to resolve digest for an image %s: %v", rawRef, err) | ||
|
@@ -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 == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this shouldn't block if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is blocked in Cosign. https://github.com/sigstore/cosign/blob/v2.0.1/cmd/cosign/cli/options/certificate.go#L94 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is contradictory with this statement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 -> || There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK could you please make the sentence simpler may be by splitting it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks |
||
return ref, errors.New("--cosign-certificate-identity or --cosign-certificate-identity-regexp is required for Cosign verification in keyless mode") | ||
} | ||
if certIdentity != "" { | ||
cosignCmd.Args = append(cosignCmd.Args, "--certificate-identity", certIdentity) | ||
} | ||
if certIdentityRegexp != "" { | ||
cosignCmd.Args = append(cosignCmd.Args, "--certificate-identity-regexp", certIdentityRegexp) | ||
} | ||
if certOidcIssuer == "" && certOidcIssuerRegexp == "" { | ||
return ref, errors.New("--cosign-certificate-oidc-issuer or --cosign-certificate-oidc-issuer-regexp is required for Cosign verification in keyless mode") | ||
} | ||
if certOidcIssuer != "" { | ||
cosignCmd.Args = append(cosignCmd.Args, "--certificate-oidc-issuer", certOidcIssuer) | ||
} | ||
if certOidcIssuerRegexp != "" { | ||
cosignCmd.Args = append(cosignCmd.Args, "--certificate-oidc-issuer-regexp", certOidcIssuerRegexp) | ||
} | ||
cosignCmd.Env = append(cosignCmd.Env, "COSIGN_EXPERIMENTAL=true") | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we support compose too?
See #1508
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM