Cleanup for TUF code

[znewman01]

To fix #1899 @asraa submitted #1921. @vaikas stepped in to help with tests and adapted it to #1932. This appears to fix the bug, and I approved it because this has been blocking the Cosign release for more than a week (the new release contains #1894 which we should get out ASAP).

But #1921 contains some good suggestions for cleaning up that code; we should definitely do a bunch of them. It's important that this code be clear and testable.

CC @haydentherapper

[vaikas]

FYI, just to make sure you're aware, but there's this PR moving much of the code sigstore/sigstore:
sigstore/sigstore#435

[asraa]

One AI for tracking: Clean up the GetRekorPub overrides
https://github.com/sigstore/cosign/pull/1932/files/81a86c3c4b79545d0a009ebb4d4ec77a8c1b76e8..2805823eaa317d931f5b6cb3d196aab3f52a5ed2#r884674685

[asraa]

Another one: testing verification failure and TUF object closure #1932 (comment)

https://github.com/sigstore/cosign/runs/6643745610?check_suite_focus=true

  error: failed to create job: admission webhook "policy.sigstore.dev" denied the request: validation failed: failed policy: image-policy-keyless-with-identities: spec.template.spec.containers[0].image
  registry.local:5000/policy-controller/demo@sha256:903d83d6d129a07eda4aa5e39bc59471de598b9e5930acdc217c4184e94fddbd validate signatures with fulcio: no matching signatures:
  unable to fetch Rekor public keys from TUF repository: creating cached local store: resource temporarily unavailable
  + echo Failed to create Job in namespace with matching 'issuer/subject!'
  Failed to create Job in namespace with matching issuer/subject!

This seems to happen after the job rejection, and now currently passes after the ordering was switched, and retrieving the alt key happens before GetRekorPubs. Even if GetRekorPubs fails (as it does in the snippet above), the test passes because the alt key is at least fetched:

cosign/pkg/cosign/tlog.go

Line 393 in e7bcb69

fmt.Fprintf(os.Stderr, "**Warning** Failed to fetch Rekor public keys from TUF, using the public key from Rekor API because %s was specified", addRekorPublicKeyFromRekor)

DONE: #1953

[asraa]

Cleanup GetEmbedded usage, but still keep easy-to-test

#1921 (comment)

[haydentherapper]

I’m going to review 1921 today.

i would prefer we not submit 1932 without more discussion.

[asraa]

Another one: Cleanup the configurable remote https://github.com/sigstore/cosign/pull/1921/files#r882887941

[asraa]

Load all targets into memory at initialize to save time reading/writing to disk: #1921 (comment)

DONE: #1953

[asraa]

TUF Object concurrency fix: #1941

The leveldb underlying store cannot be used by multiple threads, causing failures where the local cache cannot be accessed. Fix by only creating a writeable DB store when needing to update, and otherwise read-only threadsafe

DONE: #1953

[asraa]

One more: Initialize CheckOpts for verification material with TUF, do not invoke our TUF client inside library functions. This will make it infinitely easier to test.

[znewman01]

Asra, thanks for your hard work here, it's looking good 😄

Is this actually closed?

[asraa]

Re-opening, the remaining items are:

• Cleanup the GetEmbedded and GetRemoteRoot
• Consolidate the Rekor pub overrides
• Set RekorPubs in CheckOpts as we do for Fulcio roots so we aren't calling the TUF pkg from other internal packages. Makes cosign as a library more rovust.

[vaikas]

I think the consolidate Rekor pub overrides has been done as well as 'Set RekorPubs in CheckOpts'.

I'm curious however about the need for this to be a singleton, especially since it is a library, it being a singleton makes some things more interesting. I know there's work going on with TAP-4 in the upstream, but in the meantime, if this were not a singleton, a user could create two instances of the TUF client, and now that the checkopts can be passed the keys independent of the TUF (TUF fetches, they are then passed to CheckOpts), I can't think of a reason for this being a singleton, but wouldn't be the first time I'm wrong :)