Skip to content
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

Merged
merged 1 commit into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/nerdctl/container_run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

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

Copy link
Contributor Author

@ningziwen ningziwen Apr 16, 2023

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.

Copy link
Member

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.

LGTM

Comment on lines +280 to +283
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

@ningziwen ningziwen Apr 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// #endregion

cmd.Flags().String("ipfs-address", "", "multiaddr of IPFS API (default uses $IPFS_PATH env variable if defined or local directory ~/.ipfs)")
Expand Down
12 changes: 12 additions & 0 deletions cmd/nerdctl/flagutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ func processImageVerifyOptions(cmd *cobra.Command) (opt types.ImageVerifyOptions
if opt.CosignKey, err = cmd.Flags().GetString("cosign-key"); err != nil {
return
}
if opt.CosignCertificateIdentity, err = cmd.Flags().GetString("cosign-certificate-identity"); err != nil {
return
}
if opt.CosignCertificateIdentityRegexp, err = cmd.Flags().GetString("cosign-certificate-identity-regexp"); err != nil {
return
}
if opt.CosignCertificateOidcIssuer, err = cmd.Flags().GetString("cosign-certificate-oidc-issuer"); err != nil {
return
}
if opt.CosignCertificateOidcIssuerRegexp, err = cmd.Flags().GetString("cosign-certificate-oidc-issuer-regexp"); err != nil {
return
}
return
}

Expand Down
4 changes: 4 additions & 0 deletions cmd/nerdctl/image_pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ func newPullCommand() *cobra.Command {
return []string{"none", "cosign", "notation"}, cobra.ShellCompDirectiveNoFileComp
})
pullCommand.Flags().String("cosign-key", "", "Path to the public key file, KMS, URI or Kubernetes Secret for --verify=cosign")
pullCommand.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")
pullCommand.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")
pullCommand.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")
pullCommand.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")
// #endregion

pullCommand.Flags().BoolP("quiet", "q", false, "Suppress verbose output")
Expand Down
8 changes: 8 additions & 0 deletions docs/command-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,10 @@ Verify flags:

- :nerd_face: `--verify`: Verify the image (none|cosign|notation). See [`./cosign.md`](./cosign.md) and [`./notation.md`](./notation.md) for details.
- :nerd_face: `--cosign-key`: Path to the public key file, KMS, URI or Kubernetes Secret for `--verify=cosign`
- :nerd_face: `--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
- :nerd_face: `--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
- :nerd_face: `--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
- :nerd_face: `--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

IPFS flags:

Expand Down Expand Up @@ -710,6 +714,10 @@ Flags:
- :whale: `-q, --quiet`: Suppress verbose output
- :nerd_face: `--verify`: Verify the image (none|cosign|notation). See [`./cosign.md`](./cosign.md) and [`./notation.md`](./notation.md) for details.
- :nerd_face: `--cosign-key`: Path to the public key file, KMS, URI or Kubernetes Secret for `--verify=cosign`
- :nerd_face: `--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
- :nerd_face: `--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
- :nerd_face: `--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
- :nerd_face: `--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
- :nerd_face: `--ipfs-address`: Multiaddr of IPFS API (default uses `$IPFS_PATH` env variable if defined or local directory `~/.ipfs`)

Unimplemented `docker pull` flags: `--all-tags`, `--disable-content-trust` (default true)
Expand Down
4 changes: 3 additions & 1 deletion docs/cosign.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 expected in a valid Fulcio certificate for --verify=cosign, e.g. https://token.actions.githubusercontent.com or https://oauth2.sigstore.dev/auth.

```shell
# Verify the image with Keyless mode
$ nerdctl pull --verify=cosign devopps/hello-world
$ nerdctl pull --verify=cosign --certificate-identity=name@example.com --certificate-oidc-issuer=https://accounts.example.com devopps/hello-world
INFO[0004] cosign:
INFO[0004] cosign: [{"critical":{"identity":...}]
docker.io/devopps/nginx-new:latest: resolved |++++++++++++++++++++++++++++++++++++++|
Expand Down
8 changes: 8 additions & 0 deletions pkg/api/types/image_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,12 @@ type ImageVerifyOptions struct {
Provider string
// CosignKey Path to the public key file, KMS URI or Kubernetes Secret for --verify=cosign
CosignKey string
// CosignCertificateIdentity 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
CosignCertificateIdentity string
// CosignCertificateIdentityRegexp 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
CosignCertificateIdentityRegexp string
// CosignCertificateOidcIssuer 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
CosignCertificateOidcIssuer string
// CosignCertificateOidcIssuerRegexp 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
CosignCertificateOidcIssuerRegexp string
}
23 changes: 22 additions & 1 deletion pkg/signutil/cosignutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package signutil
import (
"bufio"
"context"
"errors"
"os"
"os/exec"
"strings"
Expand Down Expand Up @@ -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)
Expand All @@ -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 == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is contradictory with this statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 -> ||
and -> &&

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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."

Copy link
Member

Choose a reason for hiding this comment

The 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")
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/signutil/signutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func Verify(ctx context.Context, rawRef string, hostsDirs []string, experimental
return "", fmt.Errorf("cosign only work with enable experimental feature")
}

if ref, err = VerifyCosign(ctx, rawRef, options.CosignKey, hostsDirs); err != nil {
if ref, err = VerifyCosign(ctx, rawRef, options.CosignKey, hostsDirs, options.CosignCertificateIdentity, options.CosignCertificateIdentityRegexp, options.CosignCertificateOidcIssuer, options.CosignCertificateOidcIssuerRegexp); err != nil {
return "", err
}
case "notation":
Expand Down