verify: fetch rekor public keys before verification #2393

asraa · 2022-10-31T17:37:41Z

Signed-off-by: Asra Ali asraa@google.com

Removes calls to GetRekorPubs in pkg/. This fetches the rekor public keys before verification, making verification commands easier to test in other environments.

TODO: verify blob is currently all in cli/, so there are still some internal TUF calls there. TBD.

THIS SHOULD BE MERGED INTO A v2 BRANCH

@vaikas

Release note:

* feat!: Updates function signature for `cosign.GetRekorPubs` to remove unncessary `RekorClient` parameter.
* feat: Adds a field `RekorPubKeys` in `CheckOpts` that configured the trusted rekor public keys needed for verifying tlog entries. This may be populated with the public TUF instance using `cosign.GetRekorPubs`.
* breaking change: Updates function signatures for `VerifyTLogEntry` and `VerifyBundle` to take a map of trusted Rekor public keys to verify against. This may be populated with the public TUF instance using `cosign.GetRekorPubs`.

Summary

Release Note

Documentation

vaikas

Thank you so much!! Looking so much lovelier with this refactor :)

pkg/cosign/tlog.go

vaikas · 2022-10-31T22:32:55Z

pkg/cosign/tlog.go

@@ -138,6 +138,27 @@ func GetRekorPubs(ctx context.Context, _ *client.Rekor) (map[string]RekorPubKey,
 	return publicKeys, nil
 }

+// rekorPubsFromClient returns a RekorPubKey keyed by the log ID from the Rekor client.
+// NOTE: This should **not** be used in the verification path, but may be used in the


nit: 'should not' or 'must not'? I'm just curious with the emphasis on the not, if we should just make it a strong statement to avoid misuse?

must not, IMO. Yeah: I made this a private func on purpose so that users wouldn't be able to do this (but on the other hand, it would be useful for testing with a custom Rekor). updated the comment.

vaikas · 2022-10-31T22:38:18Z

pkg/cosign/tlog.go

+	if err != nil {
+		return nil, fmt.Errorf("unable to fetch rekor public key from rekor: %w", err)
+	}
+	pubFromAPI, err := PemToECDSAKey([]byte(pubOK.Payload))


this is just like L103-111, wondering if that could be hoisted out to a function that we can tell call from here (and hopefully remove the ENV stuff from GetRekorPubs but do the equivalent call to it from 'cli' portion instead). I assume at least that would be desirable?
We can do it in a follow up, but just curious if that even makes sense?

addressed in commit, but only to factor out the logic and make it easier to construct a map of trusted rekor pub keys outside TUF

pkg/cosign/verify.go

znewman01

+1 to Ville's comments, LGTM otherwise

pkg/cosign/tlog.go

pkg/cosign/verify.go

priyawadhwa · 2022-11-01T18:32:10Z

@asraa we decided to branch off 1.0 into the 1.0-fork branch, so you can merge this into main whenever you're ready!

asraa · 2022-11-01T20:02:02Z

Addressed comments, let me know if there's more

codecov-commenter · 2022-11-01T20:04:03Z

Codecov Report

Merging #2393 (d4550bd) into main (5d00b1f) will increase coverage by 0.18%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #2393      +/-   ##
==========================================
+ Coverage   29.98%   30.16%   +0.18%     
==========================================
  Files         139      139              
  Lines        8552     8583      +31     
==========================================
+ Hits         2564     2589      +25     
+ Misses       5633     5621      -12     
- Partials      355      373      +18

Impacted Files	Coverage Δ
cmd/cosign/cli/verify/verify.go	`19.36% <0.00%> (-0.38%)`	⬇️
cmd/cosign/cli/verify/verify_attestation.go	`0.00% <0.00%> (ø)`
pkg/cosign/tlog.go	`38.43% <34.14%> (-2.74%)`	⬇️
cmd/cosign/cli/verify/verify_blob.go	`49.56% <50.00%> (+0.01%)`	⬆️
pkg/cosign/verify.go	`41.65% <60.00%> (+2.92%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

vaikas

Awesomesauce! Just couple of lint warnings about names and from my pov it's good to go.

pkg/cosign/tlog.go

pkg/cosign/verify_test.go

pkg/cosign/tlog.go

vaikas

Thank you!!!

znewman01

This LGTM. Is there a code path we can hit for online verification?

asraa · 2022-11-01T23:16:03Z

Is there a code path we can hit for online verification?

What do you mean? I'm not sure it's relevant for this PR. We're discussing the design for the necessary flags/behavior here: https://docs.google.com/document/d/1E-cB0OdABTi4BXnRV9oBzdbUgRk3cJ-jw0E5KGE3Eiw/edit?resourcekey=0-TCQAQ1H_pAiRzC7avDt1LA#

znewman01 · 2022-11-01T23:23:15Z

Is there a code path we can hit for online verification?

What do you mean? I'm not sure it's relevant for this PR. We're discussing the design for the necessary flags/behavior here: https://docs.google.com/document/d/1E-cB0OdABTi4BXnRV9oBzdbUgRk3cJ-jw0E5KGE3Eiw/edit?resourcekey=0-TCQAQ1H_pAiRzC7avDt1LA#

I think what I mean is VerifyTlogEntry now doesn't take a RekorClient. What's my entry point in pkg/cosign/verify.go if I want to do online verification? I guess...FindTlogEntry? That feels very manual, I would like a Verify*() call instead.

But agreed it's not super relevant for this PR, I was just wondering because now there's no RekorClient getting passed in

asraa · 2022-11-02T12:40:19Z

I was just wondering because now there's no RekorClient getting passed in

You already have all the online data inside the tlog entry to do the validation. This is verifying the already fetched TLOG ENTRY, not performing a lookup. This is just the verification portion, which does internally validate the provided inclusion proof.

You're right though that the cosign API is unclear. :)

because now there's no RekorClient getting passed in

This was never used to fetch the inclusion proof. If anything it makes it more clear that the args supply the entry itself. This was INSECURELY used previously to fetch the public key.

asraa · 2022-11-02T13:37:11Z

I can't repro the failure locally :| Still trying

asraa · 2022-11-02T14:05:19Z

@priyawadhwa @vaikas Do either of you know if there's a difference in the environments running ./test/e2e_test.sh locally or on CI? https://github.com/sigstore/cosign/actions/runs/3373442220/workflow#L123

In CI we set up scaffolding first, but either way the tests run in test/e2e_test.go use the public Rekor: https://github.com/sigstore/cosign/blob/main/test/e2e_test.go#L73

Unless somehow they're not. If they're using the public Rekor (and it is on my laptop, since I printed the log ID of the failing bundle), then I wouldn't need to customize the trust root. However, on CI it fails saying the trust root has no matching Rekor keys.

asraa · 2022-11-02T16:25:35Z

However, on CI it fails saying the trust root has no matching Rekor keys.

I think @loosebazooka helped deduce that CI must be running with the local scaffolding .sigstore and root of trust. I have no idea how this test was working before then.

asraa · 2022-11-02T17:40:24Z

Resolved: the problem was that this test didn't add TUF root public keys because it's not a keyless verification. I added a TODO. We'll have this resolved when we have the online proof option for checking existing PKI log entries.

github-actions · 2022-12-03T01:58:31Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

vaikas · 2022-12-05T21:59:50Z

Looks like there was some flakiness checking out the repo, retrying.

znewman01

seems fine to me, couple final nits

znewman01 · 2022-12-05T21:56:29Z

cmd/cosign/cli/verify/verify.go

@@ -159,6 +159,11 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
 			return fmt.Errorf("getting Fulcio intermediates: %w", err)
 		}
 	}
+	// TODO: We only need this when we are doing online verification.


Hm, I would really prefer to turn this off by default before merge as this might break otherwise-offline flows.

Oh, updating the comment that was incorrect. This is needed for offline verification as well. You still need the rekor pubkeys to verify an offline bundle.

I can gate it with --skip-tlog-verify though.

cmd/cosign/cli/verify/verify_attestation.go

znewman01 · 2022-12-05T21:59:35Z

pkg/cosign/tlog.go

+// rekorPubsFromClient returns a RekorPubKey keyed by the log ID from the Rekor client.
+// NOTE: This **must not** be used in the verification path, but may be used in the
+// sign path to validate return responses are consistent from Rekor.
+func rekorPubsFromClient(rekorClient *client.Rekor) (*TrustedRekorPubKeys, error) {


Does the benefits of having this outweigh the risks that it'll be used inappropriately?

The alternative involves a real TUF root, which IMO we should want anyway even for signing. I think the post-sign verification should be identical to typical verification in every way.

It's a private function though, so I'm not sure it's that "risky". I think it's pretty clear it's retrieving the rekor pub from the rekor client.

Plumbing a TUF root into the sign path is somewhat difficult. I can file a TODO follow-up, but it really requires changing a lot more than what's here.

Great, TODO SGTM

It's also worth considering that this now doubles the amount of traffic to Rekor from Cosign. Is this needed, or can we add a follow up to plumb through TUF to verify the response?

There's a follow-up issue posted. I can get to it after the attestation support :/
it doesn't really double, but I get that it's an extra call on the sign path.

pkg/cosign/tlog.go

asraa · 2022-12-06T21:34:13Z

Looking at some e2e test failures:

2022-12-06T15:16:31.6270119Z     e2e_test.go:161: no matching signatures:
2022-12-06T15:16:31.6271235Z         searching log query: [POST /api/v1/log/entries/retrieve][404] searchLogQuery default  &{Code:404 Message:no responses found}

znewman01

lgtm other than test failures

asraa · 2022-12-06T22:10:33Z

lgtm other than test failures

Yep -- Hayden alerted me that it was the SkipTlog issues, they're in #2505 too, that I can rebase on

Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> lint Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com>

cmd/cosign/cli/verify/verify.go

Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the add-rekor-pubkey-opts branch from 74ef903 to 4e73c41 Compare October 31, 2022 17:44

znewman01 self-requested a review October 31, 2022 18:56

vaikas previously approved these changes Oct 31, 2022

View reviewed changes

znewman01 reviewed Nov 1, 2022

View reviewed changes

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

pkg/cosign/verify.go Show resolved Hide resolved

asraa dismissed vaikas’s stale review via 2da6c53 November 1, 2022 20:01

vaikas reviewed Nov 1, 2022

View reviewed changes

pkg/cosign/tlog.go Show resolved Hide resolved

pkg/cosign/verify_test.go Outdated Show resolved Hide resolved

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

pkg/cosign/tlog.go Outdated Show resolved Hide resolved

vaikas reviewed Nov 1, 2022

View reviewed changes

pkg/cosign/tlog.go Show resolved Hide resolved

vaikas previously approved these changes Nov 1, 2022

View reviewed changes

asraa dismissed vaikas’s stale review via 993f7f5 November 1, 2022 22:44

znewman01 previously approved these changes Nov 1, 2022

View reviewed changes

asraa dismissed znewman01’s stale review via e7d4046 November 2, 2022 17:40

asraa force-pushed the add-rekor-pubkey-opts branch from 993f7f5 to e7d4046 Compare November 2, 2022 17:40

github-actions bot added the no-pr-activity label Dec 3, 2022

asraa force-pushed the add-rekor-pubkey-opts branch from e7d4046 to 791183f Compare December 5, 2022 17:11

haydentherapper mentioned this pull request Dec 5, 2022

Cosign 2.0 Tracking #2376

Closed

3 tasks

znewman01 reviewed Dec 5, 2022

View reviewed changes

github-actions bot removed the no-pr-activity label Dec 6, 2022

asraa mentioned this pull request Dec 6, 2022

sign: Plumb TUF root into sign path for verification #2511

Open

asraa force-pushed the add-rekor-pubkey-opts branch from 013f9d1 to d3ee116 Compare December 6, 2022 14:51

asraa mentioned this pull request Dec 6, 2022

Breaking change: Respect tlog-upload=false, default to true #2505

Merged

znewman01 reviewed Dec 6, 2022

View reviewed changes

asraa added 5 commits December 7, 2022 09:37

add rekor pubkey checkopts

22302f5

Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> lint Signed-off-by: Asra Ali <asraa@google.com>

fix error message

235c94a

Signed-off-by: Asra Ali <asraa@google.com>

update

105d52c

Signed-off-by: Asra Ali <asraa@google.com>

update zack comments

a96a804

Signed-off-by: Asra Ali <asraa@google.com>

update tests

c60c2b5

Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the add-rekor-pubkey-opts branch from d3ee116 to c60c2b5 Compare December 7, 2022 15:40

haydentherapper reviewed Dec 7, 2022

View reviewed changes

cmd/cosign/cli/verify/verify.go Outdated Show resolved Hide resolved

update

d4550bd

Signed-off-by: Asra Ali <asraa@google.com>

znewman01 previously approved these changes Dec 7, 2022

View reviewed changes

fix merge conflict

0486dbf

Signed-off-by: Asra Ali <asraa@google.com>

asraa dismissed znewman01’s stale review via 0486dbf December 7, 2022 18:05

haydentherapper approved these changes Dec 7, 2022

View reviewed changes

asraa merged commit 7813599 into sigstore:main Dec 7, 2022

github-actions bot added this to the v1.14.0 milestone Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verify: fetch rekor public keys before verification #2393

verify: fetch rekor public keys before verification #2393

asraa commented Oct 31, 2022

vaikas left a comment

vaikas Oct 31, 2022

asraa Nov 1, 2022

vaikas Oct 31, 2022

asraa Nov 1, 2022

znewman01 left a comment

priyawadhwa commented Nov 1, 2022

asraa commented Nov 1, 2022

codecov-commenter commented Nov 1, 2022 •

edited

Loading

vaikas left a comment

vaikas left a comment

znewman01 left a comment

asraa commented Nov 1, 2022

znewman01 commented Nov 1, 2022

asraa commented Nov 2, 2022 •

edited

Loading

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

github-actions bot commented Dec 3, 2022

vaikas commented Dec 5, 2022

znewman01 left a comment

znewman01 Dec 5, 2022

asraa Dec 6, 2022

asraa Dec 6, 2022

znewman01 Dec 5, 2022

asraa Dec 6, 2022

znewman01 Dec 6, 2022

haydentherapper Dec 7, 2022

asraa Dec 7, 2022

asraa commented Dec 6, 2022

znewman01 left a comment

asraa commented Dec 6, 2022

verify: fetch rekor public keys before verification #2393

verify: fetch rekor public keys before verification #2393

Conversation

asraa commented Oct 31, 2022

Summary

Release Note

Documentation

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znewman01 left a comment

Choose a reason for hiding this comment

priyawadhwa commented Nov 1, 2022

asraa commented Nov 1, 2022

codecov-commenter commented Nov 1, 2022 • edited Loading

Codecov Report

vaikas left a comment

Choose a reason for hiding this comment

vaikas left a comment

Choose a reason for hiding this comment

znewman01 left a comment

Choose a reason for hiding this comment

asraa commented Nov 1, 2022

znewman01 commented Nov 1, 2022

asraa commented Nov 2, 2022 • edited Loading

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

asraa commented Nov 2, 2022

github-actions bot commented Dec 3, 2022

vaikas commented Dec 5, 2022

znewman01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Dec 6, 2022

znewman01 left a comment

Choose a reason for hiding this comment

asraa commented Dec 6, 2022

codecov-commenter commented Nov 1, 2022 •

edited

Loading

asraa commented Nov 2, 2022 •

edited

Loading