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

verify command: support keyless verification using only a provided certificate chain with non-fulcio roots #2845

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

dmitris
Copy link
Contributor

@dmitris dmitris commented Mar 27, 2023

This is a continuation of the work started by @nsmith5 in #2631 and uses the code of that PR rebased on the current trunk.

Fixes #2630

Summary

This change adds support for verifying keyless signatures from the specified --certificate-chain instead of assuming Fulcio and loading its roots and without requiring to pass the certificate in addition to the certificate chain.

As @haydentherapper noted in the comment below, the keyless verification without the Fulcio certificate roots is already possible but requires passing both certificate-chain and certificate.
This would allow folks to more easily verify signatures from a custom CA (see #2630 for additional discussion).

Release Note

Allow verifying signatures from a custom CA without needing leaf certificate
Documentation

Will attach an PR for docs here if folks agree on the parent issue that its worth doing

TODO

The TODO list as this idea is still in discussion etc.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #2845 (ff1b5c1) into main (ef1b2a0) will increase coverage by 0.35%.
The diff coverage is 17.24%.

❗ Current head ff1b5c1 differs from pull request most recent head 6196278. Consider uploading reports for the commit 6196278 to get more accurate results

@@            Coverage Diff             @@
##             main    #2845      +/-   ##
==========================================
+ Coverage   30.32%   30.67%   +0.35%     
==========================================
  Files         151      151              
  Lines        9451     9469      +18     
==========================================
+ Hits         2866     2905      +39     
+ Misses       6140     6111      -29     
- Partials      445      453       +8     
Impacted Files Coverage Δ
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify.go 32.84% <20.00%> (+10.42%) ⬆️

@nsmith5
Copy link
Contributor

nsmith5 commented Mar 28, 2023

Oh lovely! Thanks for picking this up @dmitris

@dmitris dmitris force-pushed the keyless-without-fulcio branch from 86347cb to 4682b8c Compare March 30, 2023 10:25
@dmitris
Copy link
Contributor Author

dmitris commented Mar 31, 2023

(planning to work on this the weak after Easter - after April 11)

@dmitris dmitris force-pushed the keyless-without-fulcio branch from 4682b8c to c15ccc8 Compare April 10, 2023 11:16
@dmitris dmitris force-pushed the keyless-without-fulcio branch from c15ccc8 to 94efc78 Compare April 19, 2023 12:49
@haydentherapper
Copy link
Contributor

cc @znewman01 @woodruffw This relates to the conversation we've been having. If we go through with this, it would assume that the chain in the bundle contains the root. Note that Cosign already does this, with cosign sign --certificate cert.pem --certificate-chain chain.pem <artifact>, so this PR is mostly an optimization that would allow you to provide --bundle instead of --certificate & --certificate-chain. We could either say that this is no different than what we're doing currently, so it's fine, or argue that this behavior actually should be removed and we should be explicit about the root.

nit for the title, can we update to reflect this, something like "Support providing a bundle to verify with non-Fulcio roots", because identity-based verification is already suppported

@dmitris dmitris force-pushed the keyless-without-fulcio branch from 94efc78 to 8ebdf9c Compare April 20, 2023 16:10
@dmitris
Copy link
Contributor Author

dmitris commented Apr 20, 2023

so this PR is mostly an optimization that would allow you to provide --bundle instead of --certificate & --certificate-chain.

We are running for an internal PoC a version of cosign with this modification, and I think we do not change the command-line parameters - we call it like this:

./cosign verify --insecure-ignore-tlog --insecure-ignore-sct --check-claims=true --certificate-identity-regexp spiffe://<ci.internal.identity> --certificate-oidc-issuer-regexp '.*' --certificate-chain path/to/internal-cert-bundle.pem 

@haydentherapper - do you mean that we should introduce the --bundle parameter for verify and use it instead of the --certificate-chain one?

nit for the title, can we update to reflect this, something like "Support providing a bundle to verify with non-Fulcio roots", because identity-based verification is already suppported

identity-based verification is, but you cannot verify the image without the Fulcio roots, and I believe this Is the essence of this change (please correct me if I'm wrong!). I think with this we can fix the issue on the "Bad News" slide of @nsmith5's Seattle talk https://youtu.be/d5vROySf1I8?t=1726 28:45.
So I wonder if we could agree on the title like "verify command: support keyless verification using provided certificate chain with non-fulcio roots"?

@dmitris dmitris force-pushed the keyless-without-fulcio branch 2 times, most recently from e28a86c to e048e76 Compare April 20, 2023 20:41
@haydentherapper
Copy link
Contributor

Sorry @dmitris, I got my wires crossed, I was thinking this was about verify-blob and a bundle instead of verify. We don't need to add support for --bundle, as per your question on Slack.

Your PR title suggestion sounds good. For a small change, can we add "only", "verify command: support keyless verification using only a provided certificate chain with non-fulcio roots", because it is possible to do keyless verification with non-fulcio roots now, you just have to specify the leaf cert with --certificate in addition to the chain

@dmitris dmitris changed the title Support keyless verification without Fulcio roots verify command: support keyless verification using only a provided certificate chain with non-fulcio roots Apr 21, 2023
@dmitris dmitris force-pushed the keyless-without-fulcio branch from e048e76 to 6c192e5 Compare April 24, 2023 20:55
@woodruffw
Copy link
Member

Thanks for pinging me in here!

We could either say that this is no different than what we're doing currently, so it's fine, or argue that this behavior actually should be removed and we should be explicit about the root.

Making sure I understand: would the changes in this PR mean that including a root in the bundle's chain would be an intended use case? If that's correct, then I'd argue this should be removed entirely rather than expanded upon: including (and trusting) the bundled root is no different from accepting a self-signed signature.

(It's possible I completely misunderstood, in which case please ignore me!)

dmitris added a commit to dmitris/sigstore-docs that referenced this pull request Apr 24, 2023
Docs change for sigstore/cosign#2845.
For 'cosign verify', `--cert-chain` is sufficient,
an additional `--cert` parameter for the leaf certificate is
no longer required.

Signed-off-by: Dmitry S <dsavints@gmail.com>
dmitris added a commit to dmitris/sigstore-docs that referenced this pull request Apr 24, 2023
Docs change for sigstore/cosign#2845.
For 'cosign verify', `--cert-chain` is sufficient,
an additional `--cert` parameter for the leaf certificate is
no longer required.

Signed-off-by: Dmitry S <dsavints@gmail.com>
@dmitris dmitris force-pushed the keyless-without-fulcio branch from 8a987e7 to cd5d4a1 Compare April 24, 2023 21:56
@haydentherapper
Copy link
Contributor

The current state is that you can specify the certificate chain to verify the provided certificate. The root is not provided out of band - It's been noted on another issue that we need to correct this, I think we'll probably just tackle that when we implement support for the new bundle format.

This proposed change, for only cosign verify (containers) lets you specify only the certificate chain without specifying the leaf, because the leaf is in the OCI annotations.verify-blob is unaffected because you always have to specify the certificate (or the --bundle flag, which is more of an optimization). This is no worse than what we've got right now, but still has the issue of providing the root in-band.

So the question is, would we rather make the current, suboptimal behavior more uniform, or stop making any changes to the chain until we mandate provided roots out of band?

@woodruffw
Copy link
Member

Got it, thanks for explaining!

That sounds like a logistical choice for cosign, but my outsider's preference would be for unifying the different Sigstore clients on enforcing OOB trust materials 🙂

@znewman01
Copy link
Contributor

IMO this all goes back to #2472 — the "chain" concept is really confusing, and the --certificate-chain flag should be called something like --trusted-root-chain.

Given that, I think it's reasonable to make the behavior be (1) configure the trusted certificates, then (2) get the cert from wherever you can find it.

@dmitris dmitris force-pushed the keyless-without-fulcio branch from fdeb6a8 to d3a5b13 Compare April 25, 2023 08:39
@dmitris
Copy link
Contributor Author

dmitris commented Apr 25, 2023

for the first TODO in the description, "Add PR for docs with examples of this kind of verification", see sigstore/docs#153, preview https://deploy-preview-153--docssigstore.netlify.app/cosign/verify/#keyless-verification-using-only-a-provided-certificate-chain-with-non-fulcio-roots or the attached screenshots:
Screenshot 2023-04-25 at 10 43 10
Screenshot 2023-04-25 at 10 43 17

@dmitris dmitris marked this pull request as ready for review April 25, 2023 16:20
@dmitris
Copy link
Contributor Author

dmitris commented Apr 25, 2023

@haydentherapper would you (or other reviewers) insist on having an e2e test for this feature in this PR, or could I do it in a follow-up? I did test it manually and it works [for me] as expected. I'm not familiar with the e2e tests, and the related documentation I believe is still TODO (https://github.com/sigstore/cosign/blob/main/CONTRIBUTING.md) 😄 Trying to run test/e2e_test.sh, I'm getting a failure (below), both in this branch and in trunk - asked on Slack [1] but didn't get any replies (yet).

test/e2e_test.sh
[...]
2023/04/25 18:45:18 GET /v2/cosign-e2e/referrers/sha256:a43304747e8d511d77dcbab6ece8dafc57e8dab2eed5cf2afb50ce95a85262c7 404 METHOD_UNKNOWN We don't understand your method + url
2023/04/25 18:45:18 GET /v2/cosign-e2e/manifests/sha256-a43304747e8d511d77dcbab6ece8dafc57e8dab2eed5cf2afb50ce95a85262c7 404 MANIFEST_UNKNOWN Unknown manifest
2023/04/25 18:45:19 GET /v2/
2023/04/25 18:45:19 GET /v2/cosign-e2e/manifests/latest
2023/04/25 18:45:19 GET /v2/
2023/04/25 18:45:19 GET /v2/cosign-e2e/manifests/sha256-a43304747e8d511d77dcbab6ece8dafc57e8dab2eed5cf2afb50ce95a85262c7.sig
2023/04/25 18:45:19 GET /v2/cosign-e2e/blobs/sha256:8a3a2d50844ea1a43c3a308952e8b3b5b4ad08cacc36b10fc7555671c0c57c13
2023/04/25 18:45:20 GET /v2/
2023/04/25 18:45:20 DELETE /v2/cosign-e2e/manifests/latest
2023/04/25 18:45:21 GET /v2/
2023/04/25 18:45:21 DELETE /v2/cosign-e2e/manifests/sha256-a43304747e8d511d77dcbab6ece8dafc57e8dab2eed5cf2afb50ce95a85262c7.sig
FAIL
FAIL	github.com/sigstore/cosign/v2/test	542.888s
FAIL

Full output of bash -x test/e2e_test.sh: https://gist.github.com/dmitris/44b78d3de43c645fa55ca4c5ae324fb3

[1] https://sigstore.slack.com/archives/C01PZKDL4DP/p1681925607818049

@dmitris dmitris force-pushed the keyless-without-fulcio branch from d3a5b13 to 32a90b4 Compare April 26, 2023 07:57
@tuminoid
Copy link

tuminoid commented Apr 26, 2023

Full output of bash -x test/e2e_test.sh: https://gist.github.com/dmitris/44b78d3de43c645fa55ca4c5ae324fb3

[1] https://sigstore.slack.com/archives/C01PZKDL4DP/p1681925607818049

Yeah, this e2e is not really documented, but looking at the GH workflows .github/workflows/tests.yaml it seems to require kind cluster and GODEBUG env set up:

      - name: setup kind cluster
        run: |
          # Used to test: cosign generate-key-pair k8s://...
          go install sigs.k8s.io/kind@v0.17.0
          kind create cluster

      - name: Run end-to-end tests
        env:
          # See #2091 for the issue describing this temp workaround.
          GODEBUG: x509sha1=1
        run: ./test/e2e_test.sh

You also need ko installed: https://github.com/ko-build/ko/releases then after kind create cluster and export GODEBUG=x509sha1=1 e2e with ./test/e2e_test.sh works.

@dmitris dmitris force-pushed the keyless-without-fulcio branch from 32a90b4 to 6836ca7 Compare April 26, 2023 14:45
@dmitris dmitris force-pushed the keyless-without-fulcio branch from 6836ca7 to 830a122 Compare April 26, 2023 16:30
dmitris added a commit to dmitris/sigstore-docs that referenced this pull request Apr 26, 2023
Docs change for sigstore/cosign#2845.
For 'cosign verify', `--cert-chain` is sufficient,
an additional `--cert` parameter for the leaf certificate is
no longer required.

Signed-off-by: Dmitry S <dsavints@gmail.com>
@dmitris dmitris force-pushed the keyless-without-fulcio branch from 830a122 to 341d835 Compare April 27, 2023 14:22
@dmitris
Copy link
Contributor Author

dmitris commented Apr 27, 2023

I have put together a "one-shot" / automated script that goes through the keyless verification with non-fulcio root flow: https://github.com/dmitris/cosign-keyless - https://github.com/dmitris/cosign-keyless/blob/main/run.sh is the script,
https://github.com/dmitris/cosign-keyless/blob/main/sample.out contains sample output of a test run.

As expected, it fails with the trunk version of cosign and works OK using the branch of this PR. Maybe we could use it as the test evidence to merge? I would follow up with the unit tests afterwards.
/cc @haydentherapper

@dmitris dmitris force-pushed the keyless-without-fulcio branch 2 times, most recently from 25baec9 to ff1b5c1 Compare April 27, 2023 19:28
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Thanks for trying to get tests working, but I think it's clear that there's no great way to test this out besides a manual test. Given you've shown it works, I'm fine with this change. Can you just clean up the duplicated tests?

test/e2e_test.go Outdated Show resolved Hide resolved
cmd/cosign/cli/verify/verify_test.go Outdated Show resolved Hide resolved
nsmith5 and others added 4 commits April 28, 2023 07:33
Fixes sigstore#2630

Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
Signed-off-by: Dmitry S <dsavints@gmail.com>
Signed-off-by: Dmitry S <dsavints@gmail.com>
Signed-off-by: Dmitry S <dsavints@gmail.com>
@dmitris dmitris force-pushed the keyless-without-fulcio branch from ff1b5c1 to 9f53227 Compare April 28, 2023 05:36
@dmitris
Copy link
Contributor Author

dmitris commented Apr 28, 2023

@haydentherapper removed the TestVerifyKeylessVerification test and also squashed some commits. Thanks for your help! 👋

test/e2e_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

@znewman01 Any other thoughts? I'm fine with this since we already do support providing the trust root via --cert-chain, this is just an optimization to also not require --certificate.

@znewman01
Copy link
Contributor

@znewman01 Any other thoughts? I'm fine with this since we already do support providing the trust root via --cert-chain, this is just an optimization to also not require --certificate.

:shipit:

Signed-off-by: Dmitry S <dsavints@gmail.com>
@dmitris dmitris force-pushed the keyless-without-fulcio branch from 9f53227 to 6196278 Compare April 28, 2023 17:17
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@haydentherapper haydentherapper merged commit cbb8af6 into sigstore:main Apr 28, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Apr 28, 2023
@dmitris dmitris deleted the keyless-without-fulcio branch April 28, 2023 19:53
dmitris added a commit to dmitris/sigstore-docs that referenced this pull request May 2, 2023
Docs change for sigstore/cosign#2845.
For 'cosign verify', `--cert-chain` is sufficient,
an additional `--cert` parameter for the leaf certificate is
no longer required.

Signed-off-by: Dmitry S <dsavints@gmail.com>
ltagliaferri pushed a commit to sigstore/docs that referenced this pull request May 4, 2023
…rt-chain' without '--cert' (sigstore/cosign pr2845) (#153)

* cosign verify --cert-chain without --cert

Docs change for sigstore/cosign#2845.
For 'cosign verify', `--cert-chain` is sufficient,
an additional `--cert` parameter for the leaf certificate is
no longer required.

Signed-off-by: Dmitry S <dsavints@gmail.com>

* address PR feedback

Signed-off-by: Dmitry S <dsavints@gmail.com>

---------

Signed-off-by: Dmitry S <dsavints@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Support "keyless" verification non-Fulcio certificate authorities
6 participants