feat: cosign sign #556

developer-guy · 2021-11-23T11:40:34Z

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Co-authored-by: Furkan Türkal furkan.turkal@trendyol.com

Fixes #423

I forgot that we can't have a digest before pushing the image, so, we have to do it right after pushing the image. 🙋🏻‍♂️

## Keyless Mode
$ COSIGN_EXPERIMENTAL=1 _output/nerdctl push --sign devopps/bar:latest

## Without Keyless Mode
$ cosign generate-key-pair
$ _output/nerdctl push --sign --cosign-key cosign.key devopps/bar:latest

cmd/nerdctl/push.go

go.mod

developer-guy · 2021-11-23T12:13:09Z

@AkihiroSuda would you mind testing this in your environment, we have a macOS laptop, so, didn't test it well, we tried on CentOS by leveraging your Vagrantfile, but I just want to be sure about the development, it'd be perfect if you can test this on your own. 🙋🏻‍♂️

developer-guy · 2021-11-23T12:54:05Z

seems there is a problem with the Go modules 👇

https://github.com/containerd/nerdctl/runs/4299324111?check_suite_focus=true#step:4:555

but couldn't make it work, I really don't understand why I'm getting this error, can you please help @AkihiroSuda, thanks in advance, but it is not a blocker thing to build binary itself which means that I can still run the following commands to build binary:

GOOS=linux make binaries && \
        GOOS=linux go test -c ./cmd/nerdctl

AkihiroSuda · 2021-11-23T14:36:35Z

  go: downloading go.opentelemetry.io v0.1.0
  github.com/containerd/nerdctl/cmd/nerdctl imports
  	github.com/sigstore/cosign/cmd/cosign/cli/sign imports
  	github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioverifier imports
  	github.com/google/certificate-transparency-go imports
  	go.etcd.io/etcd/v3 imports
  	go.etcd.io/etcd/tests/v3/integration imports
  	go.etcd.io/etcd/server/v3/embed imports
  	go.opentelemetry.io/otel/semconv: module go.opentelemetry.io/otel@latest found (v1.2.0), but does not contain package go.opentelemetry.io/otel/semconv
  github.com/containerd/nerdctl/cmd/nerdctl imports
  	github.com/sigstore/cosign/cmd/cosign/cli/sign imports
  	github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioverifier imports
  	github.com/google/certificate-transparency-go imports
  	go.etcd.io/etcd/v3 imports
  	go.etcd.io/etcd/tests/v3/integration imports
  	go.etcd.io/etcd/server/v3/embed imports
  	go.opentelemetry.io/otel/exporters/otlp imports
  	go.opentelemetry.io/otel/sdk/metric/controller/basic imports
  	go.opentelemetry.io/otel/metric/registry: module go.opentelemetry.io/otel/metric@latest found (v0.25.0), but does not contain package go.opentelemetry.io/otel/metric/registry

Probably you need some replace() lines in go.mod

AkihiroSuda · 2021-11-23T14:37:21Z

Depending on etcd and opentelemetry seems too much heavy.

Can we somehow reduce these deps?
e.g., by executing an external binary

developer-guy · 2021-11-29T14:49:14Z

developer-guy · 2021-11-29T20:52:39Z

cc: @Dentrax

AkihiroSuda · 2021-11-30T05:47:40Z

cmd/nerdctl/pull.go

@@ -48,6 +54,7 @@ func newPullCommand() *cobra.Command {
 	pullCommand.Flags().StringSlice("platform", nil, "Pull content for a specific platform")
 	pullCommand.RegisterFlagCompletionFunc("platform", shellCompletePlatforms)
 	pullCommand.Flags().Bool("all-platforms", false, "Pull content for all platforms")
+	pullCommand.Flags().Bool("verify", false, "Verify the image with cosign")


Maybe this should be a string flag --verify=(none|cosign) so that we can eventually add other verifier such as Notary (v2).

Also, can we have integration tests?
The tests should cover both good sign and bad sign.

String flag --verify=(none|cosign) would make sense to implement! However, what about adding this feature when someone wants to work with notary to implement in nerdctl? But we can do it anyway if you want, since it's a good long-term feature!

For the integration tests, we probably need to store cosign.key and cosign.pub files in the ./examples or smth folder. And we will be depended on cosign executable (must be installed) to run all integration tests, is it OK?

Notary support doesn't need to be added right now. We just have to design the CLI flag to be extensible to cover notary (or something else) in future.

For the integration tests, we probably need to store cosign.key and cosign.pub files in the ./examples or smth folder. And we will be depended on cosign executable (must be installed) to run all integration tests, is it OK?

The key should be generated during the tests by executing cosign. (as we generate ocicrypt keys in image_encrypt_linux_test.go)

(So, maybe we should change push --cosign to push --sign=(none|cosign) for consistency with pull --verify=(none|cosign). Sorry for going back and forth.)

Great, we've enhanced the flag according to your reviews

I think we wrote a simple integration test that creates a cosign-key-pair and run push&pull test cases, but not sure if it is effective. We couldn't able to run the tests in Vagrant since it could not find the testutil.go file. 🤔 What is the right way to run the all test?

docs/cosign.md

go.mod

AkihiroSuda · 2021-11-30T10:08:53Z

cmd/nerdctl/pull.go

+			cosignCmd.Env = append(cosignCmd.Env, "COSIGN_EXPERIMENTAL=true")
+		}
+
+		cosignCmd.Args = append(cosignCmd.Args, rawRef)


This is racy (sigstore/cosign#648), so at least we have to print a warning when rawRef does not contain the explicit digest value.

Ideally, we should resolve the digest before shelling out cosign (and print the validated digest).

that's so true, thanks for the warning, done.

The latest revision seems to have this race again

cmd/nerdctl/pull.go

cmd/nerdctl/push.go

cmd/nerdctl/pull.go

docs/cosign.md

AkihiroSuda

Thanks, but this race has to be either resolved or warned with logrus.Warn: https://github.com/containerd/nerdctl/pull/556/files#r759121329

Also please squash commits

cmd/nerdctl/pull.go

cmd/nerdctl/pull_linux_test.go

fahedouch · 2021-12-08T10:33:00Z

why docs/cosign.md is empty ?

fahedouch · 2021-12-08T11:26:11Z

root@a608af437e02:/home/github/developer-guy/nerdctl# _output/nerdctl push --cosign-key /home/github/cosign/cosign.key --sign=cosign fahedouch/alpine-cosign:test
INFO[0000] pushing as a reduced-platform image (application/vnd.docker.distribution.manifest.list.v2+json, sha256:00b7511e88b4d70718d5e3ca5fd2377cdaa7d27dd729214202dada4c8c57b287) 
index-sha256:00b7511e88b4d70718d5e3ca5fd2377cdaa7d27dd729214202dada4c8c57b287:    waiting        |--------------------------------------| 
manifest-sha256:e7d88de73db3d3fd9b2d63aa7f447a10fd0220b7cbf39803c803f2af9ba256b3: waiting        |--------------------------------------| 
layer-sha256:59bf1c3509f33515622619af21ed55bbe26d24913cedbca106468a5fb37a50c3:    waiting        |--------------------------------------| 
config-sha256:c059bfaa849c4d8e4aecaeb3a10c2d9b3d85f5165c66ad3a4d937758128c4d18:   waiting        |--------------------------------------| 
elapsed: 1.6 s                                                                    total:   0.0 B (0.0 B/s)                                         
INFO[0001] cosign: Enter password for private key:      
INFO[0001] cosign: Error: signing [fahedouch/alpine-cosign:test]: getting signer: reading key: inappropriate ioctl for device

looks like it prevents passing a password by redirected stdin when unset COSIGN_PASSWORD

developer-guy · 2021-12-08T11:29:48Z

why docs/cosign.md is empty ?

fixed, I don't why :/

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-authored-by: Furkan Türkal <furkan.turkal@trendyol.com> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

developer-guy · 2021-12-09T18:47:17Z

anything else do we need to fix or update? kindly ping @AkihiroSuda @fahedouch @ktock

AkihiroSuda · 2021-12-10T09:12:56Z

cmd/nerdctl/pull.go

+	}
+	return nil
+}
+


The race #556 (comment) seems introduced again in the latest revision.

imgutil.EnsureImage should be called with the verified digest value when verifier == "cosign"

developer-guy force-pushed the master branch from fe088fa to 50c15a1 Compare November 23, 2021 11:41

AkihiroSuda reviewed Nov 23, 2021

View reviewed changes

cmd/nerdctl/push.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 23, 2021

View reviewed changes

cmd/nerdctl/push.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Nov 23, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

AkihiroSuda added this to the v0.15.0 milestone Nov 23, 2021

AkihiroSuda added enhancement New feature or request impact/major labels Nov 23, 2021

developer-guy force-pushed the master branch 2 times, most recently from 7caa0b7 to c02db9c Compare November 23, 2021 12:49

developer-guy force-pushed the master branch from c02db9c to 2ad0c24 Compare November 29, 2021 14:28

Dentrax mentioned this pull request Nov 29, 2021

verifying the images by using cosign during the image pulling #577

Closed

AkihiroSuda added the status/needs-rebase label Nov 30, 2021