-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(misconf): Add fallback support for trivy-checks #8062
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Simar <[email protected]>
oldart := c.artifact | ||
|
||
for i, repo := range c.checkBundleRepos { | ||
c.populateOCIArtifact(ctx, repo, registryOpts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work because the artifact is only initialized if it is missing, so the old one will be used. https://github.com/aquasecurity/trivy/blob/main/pkg/policy/policy.go#L92
dst := c.contentDir() | ||
if err := c.artifact.Download(ctx, dst, oci.DownloadOption{ | ||
MediaType: policyMediaType, | ||
Quiet: c.quiet, | ||
}, | ||
); err != nil { | ||
if i == len(c.checkBundleRepos)-1 { | ||
return xerrors.Errorf("download error: %w", err) | ||
} | ||
log.ErrorContext(ctx, "Failed to download checks bundle", log.String("repo", repo), log.Err(err)) | ||
c.artifact = oldart | ||
continue | ||
} | ||
|
||
digest, err := c.artifact.Digest(ctx) | ||
if err != nil { | ||
if i == len(c.checkBundleRepos)-1 { | ||
return xerrors.Errorf("digest error: %w", err) | ||
} | ||
log.ErrorContext(ctx, "Failed to get digest for check bundle", log.String("repo", repo), log.Err(err)) | ||
c.artifact = oldart | ||
continue | ||
} | ||
log.DebugContext(ctx, "Digest of the built-in checks", log.String("digest", digest)) | ||
|
||
// Update metadata.json with the new digest and the current date | ||
if err = c.updateMetadata(digest, c.clock.Now()); err != nil { | ||
if i == len(c.checkBundleRepos)-1 { | ||
return xerrors.Errorf("unable to update the check metadata: %w", err) | ||
} | ||
log.ErrorContext(ctx, "Failed to update metadata", log.String("digest", digest), log.Err(err)) | ||
c.artifact = oldart | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artifact download can be extracted into a separate function to avoid multiple conditions:
func (c *Client) downloadArtifact(ctx context.Context) error {
dst := c.contentDir()
if err := c.artifact.Download(ctx, dst, oci.DownloadOption{
MediaType: policyMediaType,
Quiet: c.quiet,
},
); err != nil {
return xerrors.Errorf("download error: %w", err)
}
digest, err := c.artifact.Digest(ctx)
if err != nil {
return xerrors.Errorf("digest error: %w", err)
}
log.DebugContext(ctx, "Digest of the built-in checks", log.String("digest", digest))
if err = c.updateMetadata(digest, c.clock.Now()); err != nil {
return xerrors.Errorf("unable to update the check metadata: %w", err)
}
log.DebugContext(ctx, "Successfully loaded checks bundle", log.String("digest", digest))
return nil
}
}, | ||
); err != nil { | ||
if i == len(c.checkBundleRepos)-1 { | ||
return xerrors.Errorf("download error: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we accumulate all the errors and in case the download fails from any source return it?
// Update metadata.json with the new digest and the current date | ||
if err = c.updateMetadata(digest, c.clock.Now()); err != nil { | ||
return xerrors.Errorf("unable to update the check metadata: %w", err) | ||
oldart := c.artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fall back to a previous artifact?
@@ -162,7 +193,7 @@ func (c *Client) NeedsUpdate(ctx context.Context, registryOpts types.RegistryOpt | |||
return false, nil | |||
} | |||
|
|||
c.populateOCIArtifact(ctx, registryOpts) | |||
c.populateOCIArtifact(ctx, "", registryOpts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The artifact for the default repository will be initialized here, even if the user has overridden the repositories.
@simar7 BTW, why do we need to check the digest match of the downloaded and remote bundles to determine if a download is needed, when we already use a timestamp to limit downloads to no more than once every 24 hours? |
The update verification process differs between vulnerability database and check bundles:
However, we may choose to remove the digest verification logic for check bundles if:
|
Then we need to improve digest checking for bundles from multiple repositories:
|
If I remember correctly, crane copy or oras copy keeps the digest. Can we see if we can use the same digsest in diffrent registries? |
Description
Trivy will now try to reach fallback sources before giving up and using embedded checks. Note if all sources fail to fetch, embedded checks are still present and will be fallen back on.
Before
After
Related issues
Checklist