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

feat: refactor: use sigstore-go for fetching TrustedRoot #791

Merged
merged 23 commits into from
Aug 2, 2024

Conversation

ramonpetgrave64
Copy link
Contributor

@ramonpetgrave64 ramonpetgrave64 commented Jul 18, 2024

Uses the sigstore-go library for fetching the TrustedRoot, which contains the Sigstore infrastructure certificates needed to validate the leaf ephemeral certificates used to sign artifacts.

Refactors:

  • replace TrustedRootSingleton() with getDefaultCosignCheckOpts(), since only VerifyImage() will now need that data.
  • replace cosign.ValidateAndUnpackCert withsigstoreVerify.VerifyLeafCertificate()
  • use sync.Once for sigstore and rekor clients, and the TrustedRoot

Testing

  • existing tests continue to pass
  • manual invocations of verify-artifact.

ramonpetgrave64 and others added 15 commits July 9, 2024 00:02
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64 ramonpetgrave64 marked this pull request as ready for review July 19, 2024 18:24
@ramonpetgrave64
Copy link
Contributor Author

@ramonpetgrave64 ramonpetgrave64 changed the title feat: use sigstore-go for fetching TrustedRoot feat: refactor: use sigstore-go for fetching TrustedRoot Jul 22, 2024
@ramonpetgrave64
Copy link
Contributor Author

@loosebazooka

@haydentherapper haydentherapper self-requested a review July 23, 2024 20:41
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.

Nice work on this! Just one remark about long-running processes.

go.mod Outdated Show resolved Hide resolved
trustedRootOnce = new(sync.Once)
return
}
trustedRoot, err = sigstoreRoot.GetTrustedRoot(client)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle slsa-verifier used in any long-running (> 1 day) processes? If so, then we should look at using LiveTrustedRoot to refresh the root periodically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to do that, but I feel we need to change the NewLiveTrustedRoot to accept an existing TUFClient, rather than make a new one. wdyt?

from

func NewLiveTrustedRoot(opts *tuf.Options) (*LiveTrustedRoot, error)

to

func NewLiveTrustedRoot(c *tuf.Client) (*LiveTrustedRoot, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, we don't need to explicitly cache TrustedRoot, because the TUFClient will already cache it with GetTarget(). Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

following up in sigstore/sigstore-go#249

Copy link
Contributor

Choose a reason for hiding this comment

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

GetTarget caches the target file to prevent redownloading it. Separately, the TUF client will still check if the local TUF repository metadata needs to be updated. If caching is configured (https://github.com/sigstore/sigstore-go/blob/main/pkg/tuf/options.go#L47), then the client will only fetch new targets once the local timestamp has expired or the cache validity window has passed.

For an API to accept a TUF client, one issue is that the underlying Updater can only have Refresh called once (https://github.com/theupdateframework/go-tuf/blob/f95222bdd22d2ac4e5b8ed6fe912b645e213c3b5/metadata/updater/updater.go#L105) (I'm not sure why that is the decision, we can ask Fredrik on the issue). If the live updater accepts an existing TUF client, it will need to reinitialize the Updater, which will need the options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the current implementation just supports verifying from the public instance, WDYT about proceeding with using NewLiveTrustedRoot as-is, since a custom TUF client shouldn't be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

It seems counter to our other plans for allowing slsa-verifier users to pass their own TUF client. But slsa-verifier needs to do live lookups against the public Sigstore infra, yes, I would be okay with not using the user-provided TUF client.

verifiers/internal/gha/rekor.go Outdated Show resolved Hide resolved
Signed-off-by: Ramon Petgrave <[email protected]>
@ramonpetgrave64 ramonpetgrave64 requested a review from a team as a code owner July 26, 2024 20:33
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, looks great! I’ll follow up about the API and TUF clients on the other PR.

@ramonpetgrave64 ramonpetgrave64 enabled auto-merge (squash) August 2, 2024 21:32
@ramonpetgrave64 ramonpetgrave64 merged commit c789437 into slsa-framework:main Aug 2, 2024
15 checks passed
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.

2 participants