Fix tlog entry matching #584
Conversation
adityasaky
force-pushed
the
fix-entry-matching
branch
from
3b53ebc
to
ca42682
Compare
pkg/rekor/rekor.go
Outdated
| // TODO: Refactor this into a shared lib. | ||
| func extractCerts(e *models.LogEntryAnon) ([]*x509.Certificate, error) { | ||
| func extractCertsForHash(e *models.LogEntryAnon, hash string) ([]*x509.Certificate, error) { |
There was a problem hiding this comment.
| func extractCertsForHash(e *models.LogEntryAnon, hash string) ([]*x509.Certificate, error) { | |
| // This function extracts the data hash and certs from a given LogEntryAnon. | |
| func extractData(e *models.LogEntryAnon) (string, []*x509.Certificate, error) { |
The original intent for this func was to be a generic "get certs from log entry", and make the policy decisions above where there is more context. We should follow a similar pattern here instead of plumbing through the hash and splitting where the hash vs pk policy checks are being done.
| params := index.NewSearchIndexParamsWithContext(ctx) | ||
| params.Query = &models.SearchIndex{} | ||
|
|
||
| h := sha256.New() | ||
| h.Write(payload) | ||
| params.Query.Hash = fmt.Sprintf("sha256:%s", strings.ToLower(hex.EncodeToString(h.Sum(nil)))) |
There was a problem hiding this comment.
(optional): we may want to consider switching to cosign.FindTLogEntriesByPayload if we think there are going to be many cases of the same Pk being used.
There was a problem hiding this comment.
i can take a look tomorrow, I'm in a bit of a brain freeze atm and I'm not sure what we'd do to counter potential sha-1 collisions here.
There was a problem hiding this comment.
I don't think we have to worry about that too much. we'll still filter by PK+hash, and collisions would need to be generated by the same key within the inclusion window. It's also questionable if you distrust sha1 altogether if you could even trust the signature content being served to you in a sha1 commit. 😅
I think the long term answer is move away from the dual signing and only support offline verification, though we'll always need to support verification in some form.
Signed-off-by: Aditya Sirish A Yelgundhalli <[email protected]>
adityasaky
force-pushed
the
fix-entry-matching
branch
from
ca42682
to
13e1cd6
Compare
Summary
When gitsign-credential-cache is used, multiple entries share the same public key / signing cert. The rekor search flow was matching with the payload hash and the public key as parameters, but the results are entries which matches either condition, meaning tlog entries that used the same key (from the cache) but for other commits may be used during verification.
Release Note
Fixed matching of tlog entries during verification
Documentation
NONE