fix: use sigstore-go TUF client for verify to match initialize#763
fix: use sigstore-go TUF client for verify to match initialize#763SequeI wants to merge 2 commits intosigstore:mainfrom
Conversation
gitsign initialize writes the TUF cache in sigstore-go format, but verify was reading using the old sigstore/sigstore TUF client which expects a different cache layout. This caused verify to fall back to its expired embedded root. Switch all TUF reads to sigstore-go so initialize and verify use the same cache. Signed-off-by: SequeI <asiek@redhat.com>
|
Observed review from tommyd450+59835082 (@tommyd450) |
Signed-off-by: SequeI <asiek@redhat.com>
|
I don't think the gosec issues are related to anything I changed |
|
I applied this locally and this is the way! I think this needs priority because self hosted sigstore stack's are failing without this Thereis also this issue, where TSA fails validation because it's validating it incorrectly, let me know if im missing something; |
|
@noopduck I have not looked at this in a while since it got no attention, but I am really happy to hear it could help you out a little bit ! I'll rebase, fix linter, and fix TSA tomorrow; run full tests from a private instance and try to get the maintainers eyes |
wlynch
left a comment
wlynch
left a comment
There was a problem hiding this comment.
Sorry for missing this! Left some comments.
| // FromTUF loads certs from the TUF client. | ||
| func FromTUF(ctx context.Context) CertificateSource { | ||
| // FromTUF loads Fulcio certificates from the sigstore-go TUF cache. | ||
| func FromTUF(_ context.Context) CertificateSource { |
There was a problem hiding this comment.
Plumb through the context to the tuf.Options
| // FromTUF loads Fulcio certificates from the sigstore-go TUF cache. | ||
| func FromTUF(_ context.Context) CertificateSource { | ||
| return func() ([]*x509.Certificate, error) { | ||
| tufClient, err := tuf.NewFromEnv(ctx) |
There was a problem hiding this comment.
I think there's a regression here where we'd look for TUF_ROOT env var for a local root, though idk how documented this was in the broader sigstore clients. I don't think sigstore-go handles this natively though.
| } | ||
| if len(targets) == 0 { | ||
| return nil, errors.New("none of the Fulcio roots have been found") | ||
| return nil, fmt.Errorf("initializing tuf: %w", err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("initializing tuf: %w", err) | |
| return nil, fmt.Errorf("fetching fulcio certs: %w", err) |
| // limitations under the License. | ||
|
|
||
| package version | ||
| package version // nolint:revive |
There was a problem hiding this comment.
Don't blanket disable linting rules like this - prefer disabling per-line with a description (happy to resolve any of these in another PR).
| opts := tuf.DefaultOptions() | ||
| if mirror, err := readRemoteHint(opts.CachePath); err == nil && mirror != "" { | ||
| opts.RepositoryBaseURL = mirror | ||
| } | ||
| if opts.RepositoryBaseURL != tuf.DefaultMirror { | ||
| cachedRoot := filepath.Join(opts.CachePath, tuf.URLToPath(opts.RepositoryBaseURL), "root.json") | ||
| if rootBytes, err := os.ReadFile(cachedRoot); err == nil { | ||
| opts.Root = rootBytes | ||
| } | ||
| } | ||
| return opts |
There was a problem hiding this comment.
I assume this is to replicate cosign behavior? I wonder if there's something we can do to standardize that 🤔 (for now link to other similar behavior to explain why we're doing this / why sigstore-go doesn't handle it).
There was a problem hiding this comment.
I think this is more of a regression after 0.13, but it's possible that it makes sense, i haven't done a diff between 0.13 and 0.14... gitsign only started referencing that expired embedded root cert after gitsign version 0.13. So I rolled back and was testing with 0.13 and tufmirror defined and it would work fine until i referenced timestampserverurl (tsa) in the git config. All commits i did in mye testing repo was fine and tsa got correctly set inside the commits, however when i tried to verify them with gitsign verify, they always claimed to be invalid, even though their not before and not after parts where correct. This is where my proposal came in, i think that these fixups are very important for gitsign, since we are experiencing functionality breaking in 0.14 along with this, which was already an issue before 0.13 as well, at least it would seem:
diff --git a/internal/fork/ietf-cms/verify.go b/internal/fork/ietf-cms/verify.go
index fb5fb7e..ec3caf5 100644
--- a/internal/fork/ietf-cms/verify.go
+++ b/internal/fork/ietf-cms/verify.go
@@ -171,7 +171,7 @@ func (sd *SignedData) verify(econtent []byte, opts x509.VerifyOptions, tsOpts x5
// This check is slightly redundant, given that the cert validity times
// are checked by cert.Verify. We take the timestamp accuracy into account
// here though, whereas cert.Verify will not.
- if !tsti.Before(cert.NotAfter) || !tsti.After(cert.NotBefore) {
+ if tsti.After(cert.NotAfter) || tsti.Before(cert.NotBefore) {
return nil, x509.CertificateInvalidError{Cert: cert, Reason: x509.Expired, Detail: "timestamp authority verification failed"}
}
4 participants
Summary
gitsign initialize writes the TUF cache in sigstore-go format, but verify was reading using the old sigstore/sigstore TUF client which expects a different cache layout. This caused verify to fall back to its expired embedded root. Switch all TUF reads to sigstore-go so initialize and verify use the same cache.
Now, gitsign verify will work with gitsign/cosign initialize as it works off the same sigstore cache format.
Release Note
Documentation