fix: fix fetching updated targets from TUF root (based off of #1921) #1932

vaikas · 2022-05-30T08:17:55Z

Summary

This change refactors the cosign TUF client and hopefully aims to simplify the logic behind embedded TUF repository and targets, and the writeable on-disk/in-memory repository and targets. Roughly, I structured this so that the cosign TUF client contains (1) A client.LocalStore to hold TUF repository metadata updates and (2) A targetImpl to hold downloaded and cached target files.

Previously, the two were "out of sync" -- when starting with an embedded workflow, we would create an embedded root repository and an embedded targetImpl. However, the embedded targetImpl ONLY retrieved embedded targets, not the updated ones!

cosign/pkg/cosign/tuf/client.go

Line 548 in 5f09c42

b, err := embeddedRootRepo.ReadFile(path.Join("repository", "targets", p))

Embedded workflows never used their updated targets, despite writing them to the underlying storage, and failed to retrieve new targets referenced by updated metadata.

Conceptually now, there are 4 main objects in the TUF client:

An memoryCache targetImpl: A map that stores target files by name.
A diskCache targetImpl: Stores targets on disk.
An embeddedWrapper targetImpl: This wraps the underlying (memory, disk) cache. By default it Gets embedded targets. If any targets were downloaded and Set, then Get transfers to retrieving from the underlying cache.
An embeddedLocalStore that wraps either a MemoryLocalStore or FileLocalStore. Similar to above, by default it gets embedded repo metadata, until any new metadata needed to be downloaded and set. Then any GetMeta operations get the cached metadata.

Other:

To make testing easier, interfaced the default embedded repo and default remote mirror.
Tested the addition of a new target in an update. The same test at HEAD fails to find the target.
I tested this against a binary of cosign that pointed to the default mirror as brokenv3's GCS bucket and it successfully found the new target fulcio_interemediate_v1.crt.pem

If this design looks good, I will continue to add testing and clean-up test code. It's a pain to manually write out TUF updates, so I'd like to unify those functions and also add testing for consistent snapshotting (note brokenv3 enabled that so I'm 90% sure that works with this fix).

Ticket Link

Fixes #1899

Release Note

* bug fix: Fixes bug in retrieving newly added verification material from TUF root.

Signed-off-by: Asra Ali <asraa@google.com> add comment Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> possible fix windows Signed-off-by: Asra Ali <asraa@google.com> lint Signed-off-by: Asra Ali <asraa@google.com> fix windows maybe Signed-off-by: Asra Ali <asraa@google.com> fix close Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com> update fix Signed-off-by: Asra Ali <asraa@google.com> update and add some debug Signed-off-by: Asra Ali <asraa@google.com> add debug Signed-off-by: Asra Ali <asraa@google.com> no cache Signed-off-by: Asra Ali <asraa@google.com> remove debug Signed-off-by: Asra Ali <asraa@google.com>

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

codecov-commenter · 2022-05-30T08:20:26Z

Codecov Report

Merging #1932 (2805823) into main (5f09c42) will decrease coverage by 0.03%.
The diff coverage is 57.25%.

@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
- Coverage   34.01%   33.97%   -0.04%     
==========================================
  Files         153      153              
  Lines        9977     9974       -3     
==========================================
- Hits         3394     3389       -5     
- Misses       6202     6206       +4     
+ Partials      381      379       -2

Impacted Files	Coverage Δ
cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go	`36.36% <0.00%> (ø)`
pkg/cosign/tlog.go	`29.12% <18.18%> (-0.59%)`	⬇️
pkg/cosign/tuf/client.go	`62.07% <66.33%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f09c42...2805823. Read the comment docs.

cpanato · 2022-05-30T08:45:38Z

Makefile

@@ -118,7 +118,7 @@ cross:
 golangci-lint:
 	rm -f $(GOLANGCI_LINT_BIN) || :
 	set -e ;\
-	GOBIN=$(GOLANGCI_LINT_DIR) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 ;\
+	GOBIN=$(GOLANGCI_LINT_DIR) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.46.0 ;\


can bump to 1.46.2

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas · 2022-05-30T09:12:24Z

Looks like the Windows tests are unhappy with this:

--- FAIL: TestGetRekorPubKeys (0.21s)
    tlog_test.go:25: Unexpected error calling GetRekorPubs, expected nil: getting trusted root: open repository\root.json: file does not exist
    tlog_test.go:28: expected 1 or more keys, got 0
Repository initialized
Staged snapshot.json metadata with expiration date: 2022-06-06 08:58:35 +0000 UTC
Staged timestamp.json metadata with expiration date: 2022-05-31 08:58:35 +0000 UTC
Committed successfully

Maybe a path separator issue? Seems to work on everything else just fine.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas · 2022-05-30T10:25:41Z

pkg/cosign/tlog.go

@@ -380,6 +385,17 @@ func VerifyTLogEntry(ctx context.Context, rekorClient *client.Rekor, e *models.L
 		rekorPubKeys[keyID] = RekorPubKey{PubKey: pubFromAPI, Status: tuf.Active}
 	}

+	rekorPubKeysTuf, err := GetRekorPubs(ctx)


I think it might be cleaner to have all the override behaviour in one place, and move L371-386 to GetRekorPubs and have everything there. But, that would mean plumbing RekorClient through to GetRekorPubs, so wasn't sure if we wanted to do that. One thing that we might want to consider is plumbing the RekorClient into the ctx passed in and pull it from there so we wouldn't have to change the signature. I'd be happy to do that if we think that makes sense.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

hectorj2f

lgtm

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

dlorenc · 2022-05-31T12:08:10Z

Nice work @vaikas and @asraa!

When @asraa looks we can just merge this one.

vaikas · 2022-05-31T12:42:24Z

I cribbed the PR subject / description from #1921 here in case this is what we end up merging.

znewman01

This works for the time being, appears to fix the relevant bug.

I have some apprehensions about this whole chunk of code generally (that predate this PR), I'm going to file a cleanup bug to come back and address this because this code is pretty important and should be easy to understand/test.

znewman01 · 2022-05-31T13:50:15Z

pkg/cosign/tuf/client.go

-	// It should theoretically be safe to do this everywhere - but the files only seem to get mutated on Windows so
-	// let's only change them back there.
-	if runtime.GOOS == "windows" {
-		return bytes.ReplaceAll(b, []byte("\r\n"), []byte("\n")), nil


Where did this go?

Updated 410d6cf

znewman01 · 2022-05-31T14:44:15Z

Actually can we hold off a day on merging this? I think #1921 is pretty close

asraa

I'm worried that this doesn't solve the problem: Does GetRekorPubs still fail in the e2e test? It seems like the error in getting the TUF root Rekor public key is going "silent" now to allow the test to pass, so I just want to double check the logs and make sure that we could retrieve the TUF keys in that environment

asraa · 2022-05-31T14:50:02Z

Does GetRekorPubs still fail in the e2e test?

I don't see "unable to fetch" anymore, so I will assume it's stable for now -- but AI to track/add a test whether verification failure will cause a some path to not close the TUF object and cause the "resource unavailable" error.

haydentherapper · 2022-05-31T15:46:35Z

This PR does a lot, and I don't want to submit this without submitting #1921 first. We can't rush this large PR through to unblock tests, there's a risk with breaking something else accidentally.

asraa · 2022-05-31T15:56:32Z

This PR does a lot, and I don't want to submit this without submitting #1921 first. We can't rush this large PR through to unblock tests, there's a risk with breaking something else accidentally.

Hey! FWIW they're the same PR, so #1921 incorporates the CI fixes from @vaikas

dlorenc · 2022-05-31T16:00:07Z

This PR does a lot, and I don't want to submit this without submitting #1921 first. We can't rush this large PR through to unblock tests, there's a risk with breaking something else accidentally.

Hey! FWIW they're the same PR, so #1921 incorporates the CI fixes from @vaikas

Yep - we only need to merge one of them. We do need to get one of them merged though as this is holding up a pretty large release.

haydentherapper · 2022-05-31T16:01:06Z

Totally fair, but I also want to make sure we take our time reviewing the PR to not introduce other subtle bugs.

inferno-chromium · 2022-05-31T16:35:27Z

Some folks are concerned that this might be just hiding a test failure, let's take the time to review this in depth in the original PR #1921. @lukehinds @bobcallaway for another pair of eyes. What is the release timeline, we should be able to make that one.

dlorenc · 2022-05-31T22:26:27Z

We can close this one now, the commits got merged in with #1921.

FWIW they were always the same PR as @asraa noted, it was just done to expedite getting the tests fixed because this was blocking the release.

asraa added 3 commits May 25, 2022 13:23

try haydens comments

81a86c3

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

cpanato reviewed May 30, 2022

View reviewed changes

Use Rekor API for pubkeys before TUF if so specified.

527f95f

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas force-pushed the trust-rekor-pub-key branch from 605175c to 527f95f Compare May 30, 2022 08:48

vaikas added 2 commits May 30, 2022 12:12

Address PR feedback, bump golangci-lint from 1.46.0 to 1.46.2

a89a915

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Add comments for the env variables.

291fb20

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas commented May 30, 2022

View reviewed changes

Use path instead of filepath, basically revert to what it was before.

67e343b

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

hectorj2f previously approved these changes May 31, 2022

View reviewed changes

ho hum, really just use the path.

89bc9f9

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas dismissed hectorj2f’s stale review via 89bc9f9 May 31, 2022 09:08

When interacting with fs do not use OS specific separators.

2805823

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas changed the title ~~Trust rekor pub key if so specified.~~ fix: fix fetching updated targets from TUF root (based off of #1921) May 31, 2022

znewman01 approved these changes May 31, 2022

View reviewed changes

znewman01 mentioned this pull request May 31, 2022

Cleanup for TUF code #1935

Open

asraa reviewed May 31, 2022

View reviewed changes

dlorenc closed this May 31, 2022

vaikas mentioned this pull request Jan 5, 2023

Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired sigstore/sigstore#893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix fetching updated targets from TUF root (based off of #1921) #1932

fix: fix fetching updated targets from TUF root (based off of #1921) #1932

vaikas commented May 30, 2022 •

edited

Loading

codecov-commenter commented May 30, 2022 •

edited

Loading

cpanato May 30, 2022

vaikas commented May 30, 2022

vaikas May 30, 2022

hectorj2f left a comment

dlorenc commented May 31, 2022

vaikas commented May 31, 2022

znewman01 left a comment

znewman01 May 31, 2022

asraa May 31, 2022

znewman01 commented May 31, 2022

asraa left a comment

asraa commented May 31, 2022 •

edited

Loading

haydentherapper commented May 31, 2022

asraa commented May 31, 2022

dlorenc commented May 31, 2022

haydentherapper commented May 31, 2022

inferno-chromium commented May 31, 2022

dlorenc commented May 31, 2022

fix: fix fetching updated targets from TUF root (based off of #1921) #1932

fix: fix fetching updated targets from TUF root (based off of #1921) #1932

Conversation

vaikas commented May 30, 2022 • edited Loading

Summary

Ticket Link

Release Note

codecov-commenter commented May 30, 2022 • edited Loading

Codecov Report

cpanato May 30, 2022

Choose a reason for hiding this comment

vaikas commented May 30, 2022

vaikas May 30, 2022

Choose a reason for hiding this comment

hectorj2f left a comment

Choose a reason for hiding this comment

dlorenc commented May 31, 2022

vaikas commented May 31, 2022

znewman01 left a comment

Choose a reason for hiding this comment

znewman01 May 31, 2022

Choose a reason for hiding this comment

asraa May 31, 2022

Choose a reason for hiding this comment

znewman01 commented May 31, 2022

asraa left a comment

Choose a reason for hiding this comment

asraa commented May 31, 2022 • edited Loading

haydentherapper commented May 31, 2022

asraa commented May 31, 2022

dlorenc commented May 31, 2022

haydentherapper commented May 31, 2022

inferno-chromium commented May 31, 2022

dlorenc commented May 31, 2022

vaikas commented May 30, 2022 •

edited

Loading

codecov-commenter commented May 30, 2022 •

edited

Loading

asraa commented May 31, 2022 •

edited

Loading