Accept a slice of remote.Option for cosign verification #916

souleb · 2022-09-26T16:18:26Z

fixes #915

If implemented this enable passing a keychain, an authenticator and a custom transport as remote.Option to the verifier. It enables contextual login and self-signed certificates as well insecure registries.

Signed-off-by: Soule BA soule@weave.works

stefanprodan · 2022-09-27T13:46:47Z

Tested d5854dc with a local registry and it fails to verify:

$ cosign verify --key=$HOME/.cosign/cosign.pub localhost:5050/podinfo-deploy:latest
Verification for localhost:5050/podinfo-deploy:latest --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - The signatures were verified against the specified public key

[{"critical":{"identity":{"docker-reference":"localhost:5050/podinfo-deploy"},"image":{"docker-manifest-digest":"sha256:df41ceaea12823eb049ce7e4b80915bb59b8503b9a197accc93ae81b42b5962b"},"type":"cosign container image signature"},"optional":null}]

$ flux -n apps reconcile source oci podinfo-signed 
► annotating OCIRepository podinfo-signed in apps namespace
✔ OCIRepository annotated
◎ waiting for OCIRepository reconciliation
✗ OCIRepository reconciliation failed: 'failed to verify the signature using provider 'cosign': no matching signatures were found for 'kind-registry:5000/podinfo-deploy:latest''

If I disable verification, it works:

$ flux -n apps reconcile source oci podinfo-signed 
► annotating OCIRepository podinfo-signed in apps namespace
✔ OCIRepository annotated
◎ waiting for OCIRepository reconciliation
✔ fetched revision latest/df41ceaea12823eb049ce7e4b80915bb59b8503b9a197accc93ae81b42b5962b

Tested with:

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: OCIRepository
metadata:
  name: podinfo-signed
  namespace: apps
spec:
  interval: 5m
  insecure: true
  url: oci://kind-registry:5000/podinfo-deploy #oci://ghcr.io/stefanprodan/podinfo-deploy
  ref:
    tag: latest
  verify:
    provider: cosign
    secretRef:
      name: podinfo-key
---
apiVersion: v1
kind: Secret
metadata:
  namespace: apps
  name: podinfo-key
stringData:
  key.pub: |
    -----BEGIN PUBLIC KEY-----
    MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEST+BqQ1XZhhVYx0YWQjdUJYIG5Lt
    iz2+UxRIqmKBqNmce2T+l45qyqOs99qfD7gLNGmkVZ4vtJ9bM7FxChFczg==
    -----END PUBLIC KEY-----

darkowlzz · 2022-09-27T13:51:54Z

Key based verification works on azure with the latest commit (12e8230).

spec:
  interval: 1m0s
  provider: azure
  ref:
    tag: "1"
  timeout: 60s
  url: oci://xxxx.azurecr.io/hello-world
  verify:
    provider: cosign
    secretRef:
      name: cosign-public-keys
status:
  artifact:
    checksum: a159aec65b18430c32e9986deadbafc891e08f991974b9e2a07bd4700236d0de
    lastUpdateTime: "2022-09-27T13:45:36Z"
    path: ocirepository/default/hello-world/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4.tar.gz
    revision: 1/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4
    size: 2495
    url: http://source-controller.flux-system.svc.cluster.local./ocirepository/default/hello-world/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4.tar.gz
  conditions:
  - lastTransitionTime: "2022-09-27T13:45:36Z"
    message: stored artifact for digest '1/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2022-09-27T13:45:36Z"
    message: stored artifact for digest '1/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  - lastTransitionTime: "2022-09-27T13:45:36Z"
    message: verified signature of revision 1/f54a58bc1aac5ea1a25d796ae155dc228b3f0e11d046ae276b39c4bf2f13d8c4
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: SourceVerified
  observedGeneration: 1
  url: http://source-controller.flux-system.svc.cluster.local./ocirepository/default/hello-world/latest.tar.gz

Will try on GCP for keyless verification.

darkowlzz · 2022-09-27T14:04:32Z

Tested d5854dc with a local registry and it fails to verify:

@stefanprodan I can also confirm that with the latest change, verification against local registry works. That commit had some issues. Please try the latest commit.
Update: My local registry is secured. Not sure about insecure registries.

stefanprodan · 2022-09-27T14:05:54Z

@darkowlzz no luck with the latest commit, note that my registry has no auth at all, it's just a plain HTTP registry.

darkowlzz · 2022-09-27T15:58:26Z

Did more testing on GCP.
Key based verification just works, similar to the azure test posted above.
Keyless verification also works.

Followed the instructions from https://github.com/sigstore/cosign/blob/main/KEYLESS.md#on-gcp for signing.

Without signing the artifact, the following happens:

failed to verify the signature using provider 'cosign keyless': no matching signatures:

Signing the artifact without uploading the transparency log to rekor results in:

failed to verify the signature using provider 'cosign keyless': no matching signatures: signature not found in transparency log

Uploading the transparency log to rekor makes it work:

spec:
  interval: 1m0s
  provider: gcp
  ref:
    tag: "3"
  timeout: 60s
  url: oci://us-central1-docker.pkg.dev/xxxx/flux-test-moving-glowworm/hello-world
  verify:
    provider: cosign
status:
  artifact:
    checksum: 9f3bc0f341d4ecf2bab460cc59320a2a9ea292f01d7b96e32740a9abfd341088
    lastUpdateTime: "2022-09-27T15:20:48Z"
    path: ocirepository/default/hello-world/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9.tar.gz
    revision: 3/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9
    size: 1105
    url: http://source-controller.flux-system.svc.cluster.local/./ocirepository/default/hello-world/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9.tar.gz
  conditions:
  - lastTransitionTime: "2022-09-27T15:20:48Z"
    message: stored artifact for digest '3/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2022-09-27T15:20:48Z"
    message: stored artifact for digest '3/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9'
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  - lastTransitionTime: "2022-09-27T15:20:48Z"
    message: verified signature of revision 3/4c929c017d159e65154ddb38dae8d3edf54f911345d1453d1491b6a7bc1b28e9
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: SourceVerified
  observedGeneration: 1
  url: http://source-controller.flux-system.svc.cluster.local/./ocirepository/default/hello-world/latest.tar.gz

stefanprodan · 2022-09-27T16:05:36Z

I suspect the http transport gets overwritten by cosign and that's the reason spec.insecure doesn't work. @darkowlzz do you have a registry with a self-signed cert to test verify?

darkowlzz · 2022-09-27T16:09:28Z

I suspect the whole transport gets overwritten by cosgin and that's the reason spec.insecure doesn't work. @darkowlzz do you have a registry with a self-signed cert to test verify?

@stefanprodan The latest commit may help https://github.com/fluxcd/source-controller/compare/12e8230b8e15131c28293951495e1e2122f8bd09..c8017244d28a8d8ea27b1d6a3fbb6187f44f761c .
My registry uses letsencrypt certs. But I think we use self-signed certs in our controller tests that need a local registry. Maybe we can make use of it?

stefanprodan · 2022-09-27T16:11:23Z

The latest commit may help

@darkowlzz I've tested the last commit and it doesn't work with an insecure registry.

But I think we use self-signed certs in our controller tests that need a local registry. Maybe we can make use of it?

Yeap good idea, we need to add a separate test for client cert + cosign.

stefanprodan · 2022-09-27T16:14:23Z

What's really annoying about all of this is that both crane and cosign are changing the transport when localhost is used, the tests are passing because of that, once you replace localhost with a Kubernetes service name, things fall apart.

stefanprodan · 2022-09-27T16:40:04Z

Ok so the conclusion after many hours of head banging is that cosign doesn't propagate the insecure flag from their RegistryOptions to the name.Registry scheme, so it stays on HTTPS (when the host is not localhost), no relevant errors just no matching signatures were found 🤬

controllers/ocirepository_controller.go

souleb · 2022-09-28T12:46:07Z

I have tested this on aws with contextual login and a given secret. The verification works as expected. Also tested with contextual login on gcp using a key and keyless verification.

stefanprodan · 2022-09-28T13:36:37Z

@souleb let's fail the reconciliation if both verify and insecure are set with cosign does not support insecure registries.

hiddeco · 2022-09-28T13:49:23Z

controllers/ocirepository_controller.go

-		options = append(options, crane.WithTransport(transport))
-	}
+
+	opts := r.makeOptions(ctx, obj, withTransport(transport), withKeychainOrAuth(keychain, auth))


Wouldn't it be better to simply make the signature here makeOptions(ctx, obj, transport, keychain, auth) to prevent another options interface, and just have flat out conditional logic in makeOptions?

We will likely keep adding options (at least for insecure).

I have added a commit that complies with this.

souleb · 2022-09-29T08:52:49Z

While adding a test for cosign + registry with TLS, I discovered that the sign.SignCmd does not support passing custom transport... So I won't include that test case for now.

stefanprodan

LGTM

Thanks @souleb 🏅

hiddeco · 2022-09-29T12:16:06Z

Think you can squash the last commit together with a866866.

hiddeco

Other than the nit, this looks good to me too but @darkowlzz might want to have another look.

Thanks @souleb 🙇

If implemented this enable passing a keychain, an authenticator and a custom transport as remote.Option to the verifier. It enables contextual login, self-signed certificates and insecure registries. Signed-off-by: Soule BA <soule@weave.works> refactor makeOptions Reduce complexity by replacing the functional options with a flat out conditional logic in makeOptions. Signed-off-by: Soule BA <soule@weave.works>

If implemented we fails when trying to verify with insecure set. This will likely change once cosign add support for insecure registries. Signed-off-by: Soule BA <soule@weave.works>

darkowlzz

LGTM!

souleb marked this pull request as draft September 26, 2022 16:19

souleb force-pushed the fix-915 branch from f47823c to fc55787 Compare September 27, 2022 13:11

souleb marked this pull request as ready for review September 27, 2022 13:12

souleb requested a review from stefanprodan September 27, 2022 13:12

souleb marked this pull request as draft September 27, 2022 13:23

souleb force-pushed the fix-915 branch 3 times, most recently from d5854dc to 12e8230 Compare September 27, 2022 13:38

stefanprodan added the area/oci OCI related issues and pull requests label Sep 27, 2022

souleb force-pushed the fix-915 branch from 12e8230 to c801724 Compare September 27, 2022 15:10

hiddeco reviewed Sep 27, 2022

View reviewed changes

controllers/ocirepository_controller.go Outdated Show resolved Hide resolved

souleb force-pushed the fix-915 branch from c801724 to 64c0e70 Compare September 28, 2022 12:43

hiddeco reviewed Sep 28, 2022

View reviewed changes

stefanprodan mentioned this pull request Sep 28, 2022

Prepare for v0.35 release fluxcd/flux2#3155

Closed

11 tasks

souleb force-pushed the fix-915 branch from 64c0e70 to 6c590b6 Compare September 29, 2022 10:05

souleb marked this pull request as ready for review September 29, 2022 10:22

souleb force-pushed the fix-915 branch from a4f5fb4 to 468aecb Compare September 29, 2022 11:45

stefanprodan approved these changes Sep 29, 2022

View reviewed changes

hiddeco approved these changes Sep 29, 2022

View reviewed changes

souleb force-pushed the fix-915 branch from 468aecb to 0a9f86b Compare September 29, 2022 12:42

souleb added 2 commits September 29, 2022 14:45

Fail when verifying with insecure

f51c98e

If implemented we fails when trying to verify with insecure set. This will likely change once cosign add support for insecure registries. Signed-off-by: Soule BA <soule@weave.works>

souleb force-pushed the fix-915 branch from 0a9f86b to f51c98e Compare September 29, 2022 12:45

darkowlzz approved these changes Sep 29, 2022

View reviewed changes

stefanprodan merged commit 2a2b525 into fluxcd:main Sep 29, 2022

souleb deleted the fix-915 branch September 29, 2022 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept a slice of remote.Option for cosign verification #916

Accept a slice of remote.Option for cosign verification #916

souleb commented Sep 26, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022

stefanprodan commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022

stefanprodan commented Sep 27, 2022

souleb commented Sep 28, 2022

stefanprodan commented Sep 28, 2022

hiddeco Sep 28, 2022

souleb Sep 29, 2022

souleb Sep 29, 2022

souleb commented Sep 29, 2022

stefanprodan left a comment

hiddeco commented Sep 29, 2022

hiddeco left a comment •

edited

Loading

darkowlzz left a comment

Accept a slice of remote.Option for cosign verification #916

Accept a slice of remote.Option for cosign verification #916

Conversation

souleb commented Sep 26, 2022 • edited Loading

stefanprodan commented Sep 27, 2022 • edited Loading

darkowlzz commented Sep 27, 2022 • edited Loading

darkowlzz commented Sep 27, 2022 • edited Loading

stefanprodan commented Sep 27, 2022 • edited Loading

darkowlzz commented Sep 27, 2022 • edited Loading

stefanprodan commented Sep 27, 2022 • edited Loading

darkowlzz commented Sep 27, 2022

stefanprodan commented Sep 27, 2022 • edited Loading

stefanprodan commented Sep 27, 2022

stefanprodan commented Sep 27, 2022

souleb commented Sep 28, 2022

stefanprodan commented Sep 28, 2022

hiddeco Sep 28, 2022

Choose a reason for hiding this comment

souleb Sep 29, 2022

Choose a reason for hiding this comment

souleb Sep 29, 2022

Choose a reason for hiding this comment

souleb commented Sep 29, 2022

stefanprodan left a comment

Choose a reason for hiding this comment

hiddeco commented Sep 29, 2022

hiddeco left a comment • edited Loading

Choose a reason for hiding this comment

darkowlzz left a comment

Choose a reason for hiding this comment

souleb commented Sep 26, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

darkowlzz commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

stefanprodan commented Sep 27, 2022 •

edited

Loading

hiddeco left a comment •

edited

Loading