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.

Sign up for GitHub

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

Jump to bottom

Fix verify logs repetition #344

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+32 −46
Draft

Fix verify logs repetition #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db5d67c
removing repeated log lines and unnecessary searches (memory source s…
ChaosInTheCRD Aug 1, 2024
9dbe8c5
Merge branch 'main' of github.com:in-toto/go-witness into fix-verify-…
ChaosInTheCRD Sep 4, 2024
3c3f4af
fixing issues with search depth
ChaosInTheCRD Sep 4, 2024
cf7190c
removing redundant boolean
ChaosInTheCRD Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 14 additions & 21 deletions policy/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,30 +239,30 @@ func (p Policy) Verify(ctx context.Context, opts ...VerifyOption) (bool, map[str
resultsByStep := make(map[string]StepResult)
for depth := 0; depth < vo.searchDepth; depth++ {
for stepName, step := range p.Steps {
// Use search to get all the attestations that match the supplied step name and subjects
collections, err := vo.verifiedSource.Search(ctx, stepName, vo.subjectDigests, attestationsByStep[stepName])
// initialize the result for this step if it hasn't been already
if _, ok := resultsByStep[stepName]; !ok {
resultsByStep[stepName] = StepResult{Step: stepName}
}

collections, err := vo.verifiedSource.Search(ctx, depth, stepName, vo.subjectDigests, attestationsByStep[stepName])
if err != nil {
return false, nil, err
}

if len(collections) == 0 {
collections = append(collections, source.CollectionVerificationResult{Errors: []error{ErrNoCollections{Step: stepName}}})
continue
}

// Verify the functionaries
// Verify the functionaries and validate attestations
collections = step.checkFunctionaries(collections, trustBundles)

stepResult := step.validateAttestations(collections)

// We perform many searches against the same step, so we need to merge the relevant fields
if resultsByStep[stepName].Step == "" {
resultsByStep[stepName] = stepResult
} else {
if result, ok := resultsByStep[stepName]; ok {
result.Passed = append(result.Passed, stepResult.Passed...)
result.Rejected = append(result.Rejected, stepResult.Rejected...)
resultsByStep[stepName] = result
}
// Merge the results
if result, ok := resultsByStep[stepName]; ok {
result.Passed = append(result.Passed, stepResult.Passed...)
result.Rejected = append(result.Rejected, stepResult.Rejected...)

resultsByStep[stepName] = result
}

for _, coll := range stepResult.Passed {
Expand Down Expand Up @@ -325,13 +325,6 @@ func (p Policy) verifyArtifacts(resultsByStep map[string]StepResult) (map[string
for _, step := range p.Steps {
accepted := false
if len(resultsByStep[step.Name].Passed) == 0 {
if result, ok := resultsByStep[step.Name]; ok {
result.Rejected = append(result.Rejected, RejectedCollection{Reason: fmt.Errorf("failed to verify artifacts for step %s: no passed collections present", step.Name)})
resultsByStep[step.Name] = result
} else {
return nil, fmt.Errorf("failed to find step %s in step results map", step.Name)
}

continue
}

Expand Down
2 changes: 1 addition & 1 deletion policy/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func newDummyVerifiedSourcer(verifiedCollections []source.CollectionVerification
return &dummyVerifiedSourcer{verifiedCollections}
}

func (s *dummyVerifiedSourcer) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]source.CollectionVerificationResult, error) {
func (s *dummyVerifiedSourcer) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]source.CollectionVerificationResult, error) {
return s.verifiedCollections, nil
}

Expand Down
2 changes: 1 addition & 1 deletion policy/step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func (s Step) validateAttestations(collectionResults []source.CollectionVerifica
if passed {
result.Passed = append(result.Passed, collection)
} else {
r := strings.Join(reasons, ",\n - ")
r := strings.Join(reasons, "\n - ")
reason := fmt.Sprintf("collection validation failed:\n - %s", r)
result.Rejected = append(result.Rejected, RejectedCollection{
Collection: collection,
Expand Down
2 changes: 1 addition & 1 deletion source/archivista.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func NewArchvistSource(client *archivista.Client) *ArchivistaSource {
}
}

func (s *ArchivistaSource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
func (s *ArchivistaSource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
gitoids, err := s.client.SearchGitoids(ctx, archivista.SearchGitoidVariables{
CollectionName: collectionName,
SubjectDigests: subjectDigests,
Expand Down
23 changes: 8 additions & 15 deletions source/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"

"github.com/in-toto/go-witness/dsse"
"github.com/in-toto/go-witness/log"
)

type ErrDuplicateReference string
Expand Down Expand Up @@ -103,7 +104,13 @@ func (s *MemorySource) LoadEnvelope(reference string, env dsse.Envelope) error {
return nil
}

func (s *MemorySource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
func (s *MemorySource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mikhailswift I found the issue which was producing the false negative results and it was the limiting of the memory source searches. I have now passed in the search dept integer to the Sourcer interface, which feels slightly wrong as only the memory source search needs it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the memory source needs the depth -- and I don't think we can actually skip over subsequent memory searches where depth > 0.

If we do one round of searches, and find some attestations that match, and others that don't, but then find some back ref subjects on one of the matched attestations, we may discover more that match on the next iteration of searches.

Copy link
Member

Choose a reason for hiding this comment

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

The current tests are passing even with this change, but I suspect it is not correct. Let me try to write a test that can demonstrate.

if depth > 0 {
fmt.Println("skipping memory source search")
log.Debug("skipping memory source search: already performed")
return []CollectionEnvelope{}, nil
}

matches := make([]CollectionEnvelope, 0)
for _, potentialMatchReference := range s.referencesByCollectionName[collectionName] {
env, ok := s.envelopesByReference[potentialMatchReference]
Expand All @@ -125,20 +132,6 @@ func (s *MemorySource) Search(ctx context.Context, collectionName string, subjec
continue
}

// make sure all the expected attestations appear in the collection
attestationsMatched := true
indexAttestations := s.attestationsByReference[potentialMatchReference]
for _, checkAttestation := range attestations {
if _, ok := indexAttestations[checkAttestation]; !ok {
attestationsMatched = false
break
}
}

if !attestationsMatched {
continue
}

matches = append(matches, env)
}

Expand Down
2 changes: 1 addition & 1 deletion source/memory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func TestSearch(t *testing.T) {
}

// Run the search query on the MemorySource
got, err := s.Search(tt.searchQuery.ctx, tt.searchQuery.collectionName, tt.searchQuery.subDigest, tt.searchQuery.attestations)
got, err := s.Search(tt.searchQuery.ctx, 0, tt.searchQuery.collectionName, tt.searchQuery.subDigest, tt.searchQuery.attestations)
if (err != nil) != tt.wantErr {
t.Fatalf("MemorySource.Search() error = %v, wantErr %v", err, tt.wantErr)
}
Expand Down
4 changes: 2 additions & 2 deletions source/multi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func NewMultiSource(sources ...Sourcer) *MultiSource {
}

// Search concurrently queries all sources and returns the combined results.
func (s *MultiSource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
func (s *MultiSource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) {
results := []CollectionEnvelope{}
errors := []error{}

Expand Down Expand Up @@ -61,7 +61,7 @@ func (s *MultiSource) Search(ctx context.Context, collectionName string, subject
// Goroutine for querying a source and collecting the results or error
go func(src Sourcer) {
defer wg.Done()
res, err := src.Search(ctx, collectionName, subjectDigests, attestations)
res, err := src.Search(ctx, depth, collectionName, subjectDigests, attestations)
if err != nil {
errs <- err
} else {
Expand Down
2 changes: 1 addition & 1 deletion source/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type CollectionEnvelope struct {
}

type Sourcer interface {
Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error)
Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error)
}

func envelopeToCollectionEnvelope(reference string, env dsse.Envelope) (CollectionEnvelope, error) {
Expand Down
6 changes: 3 additions & 3 deletions source/verified.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type CollectionVerificationResult struct {
}

type VerifiedSourcer interface {
Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionVerificationResult, error)
Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionVerificationResult, error)
}

type VerifiedSource struct {
Expand All @@ -43,8 +43,8 @@ func NewVerifiedSource(source Sourcer, verifyOpts ...dsse.VerificationOption) *V
return &VerifiedSource{source, verifyOpts}
}

func (s *VerifiedSource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionVerificationResult, error) {
unverified, err := s.source.Search(ctx, collectionName, subjectDigests, attestations)
func (s *VerifiedSource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionVerificationResult, error) {
unverified, err := s.source.Search(ctx, depth, collectionName, subjectDigests, attestations)
if err != nil {
return nil, err
}
Expand Down