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

refactor: define a new struct for scan targets #5397

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

knqyf263
Copy link
Collaborator

@knqyf263 knqyf263 commented Oct 18, 2023

Description

This PR exports Scanner.ScanTarget().

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@knqyf263 knqyf263 self-assigned this Oct 18, 2023
@knqyf263
Copy link
Collaborator Author

knqyf263 commented Oct 18, 2023

@chen-keinan I've refactored and exported ScanTarget() so you can implement k8s component vulnerability scanning. You can collect k8s component information, build types.Application, and call ScanTarget(). What do you think?

@knqyf263 knqyf263 marked this pull request as ready for review October 18, 2023 05:42
@chen-keinan
Copy link
Contributor

@chen-keinan I've refactored and exported ScanTarget() so you can implement k8s component vulnerability scanning. You can collect k8s component information, build types.Application, and call ScanTarget(). What do you think?

@knqyf263 Thanks will review it

@DmitriyLewen
Copy link
Contributor

I don't see problem with these changes, but i try to understand how you want to use ScanTarget() for k8s.

We initialize all related Scanners using wire. Do you want create separately (without using wire) one more scanners for k8s in github.com/aquasecurity/trivy/pkg/k8s package?

Something like that:

ospkgScanner := ospkg.NewScanner()
langpkgScanner := langpkg.NewScanner()
config := db.Config{}
client := vulnerability.NewClient(config)
k8sScanner := local.NewScanner(nil, ospkgScanner, langpkgScanner, client)
so := types.ScanOptions{} // convert s.opts (flag options) to ScanOptions
target := types.ScanTarget{
	Applications: []ftypes.Application{}, // convert found k8s components to Applications
}
k8sScanner.ScanTarget(ctx, target, so)

@knqyf263
Copy link
Collaborator Author

knqyf263 commented Oct 18, 2023

We initialize all related Scanners using wire. Do you want create separately (without using wire) one more scanners for k8s in github.com/aquasecurity/trivy/pkg/k8s package?

Exactly. I'm surprised you quickly understand my idea 😄. But we can use wire to initialize the scanner. We can create inject.go under k8s/.

We scan container images for vulnerabilities, misconfigurations and secrets here.

onItem := func(ctx context.Context, artifact *artifacts.Artifact) (scanResult, error) {
scanResults := scanResult{}
if s.opts.Scanners.AnyEnabled(types.VulnerabilityScanner, types.SecretScanner) {
opts := s.opts
opts.Credentials = make([]ftypes.Credential, len(s.opts.Credentials))
copy(opts.Credentials, s.opts.Credentials)
// add image private registry credential auto detected from workload imagePullsecret / serviceAccount
if len(artifact.Credentials) > 0 {
for _, cred := range artifact.Credentials {
opts.RegistryOptions.Credentials = append(opts.RegistryOptions.Credentials,
ftypes.Credential{
Username: cred.Username,
Password: cred.Password,
},
)
}
}
vulns, err := s.scanVulns(ctx, artifact, opts)
if err != nil {
return scanResult{}, xerrors.Errorf("scanning vulnerabilities error: %w", err)
}
scanResults.vulns = vulns
}
if local.ShouldScanMisconfigOrRbac(s.opts.Scanners) {
misconfig, err := s.scanMisconfigs(ctx, artifact)
if err != nil {
return scanResult{}, xerrors.Errorf("scanning misconfigurations error: %w", err)
}
scanResults.misconfig = misconfig
}
return scanResults, nil
}

However, the component information is not a container image. I was thinking of converting artifactsData []*artifacts.Artifact into Application somehow. Then, pass it to ScanTarget like @DmitriyLewen wrote in addition to scanning container images.

Another idea in my mind is we treat it as an artifact like a container image, filesystem, etc. We can extend the existing SBOM artifact.

type Artifact struct {
filePath string
cache cache.ArtifactCache
analyzer analyzer.AnalyzerGroup
handlerManager handler.Manager
artifactOption artifact.Option
}

Neither idea is ideal. Do you guys have any other ideas?

@DmitriyLewen
Copy link
Contributor

Another idea in my mind is we treat it as an artifact like a container image, filesystem, etc. We can extend the existing SBOM artifact.

Some of my thoughts on this:
1 - I am not sure that we need to relate k8s with SBOM.
2- I think we can create new artifact (as you said). But in this case, it is better to move logic to detect k8s components to Inspect function. But then we will split k8s logic to 2 packages (k8s for images and artifact for components). I am not sure that it is comfortable for work and support.
3 - on the other hand - we don't need to change other logic - Inspect will return ArtifactReference with k8s components which will be handled as language package.

I was thinking of converting artifactsData []*artifacts.Artifact into Application somehow

If it is easy to implement, I think that is better way. This solution is less confusing than new Artifact.

@chen-keinan
Copy link
Contributor

I was thinking of converting artifactsData []*artifacts.Artifact into Application somehow

If it is easy to implement, I think that is better way. This solution is less confusing than new Artifact.

I agree, the easy and right way will be to convert artifactsData []*artifacts.Artifact into Application call ScanTarget directly and append it results to usual resource scan results to be output together with vulnerability report

@knqyf263 knqyf263 added this pull request to the merge queue Oct 20, 2023
Merged via the queue into aquasecurity:main with commit f2a12f5 Oct 20, 2023
12 checks passed
@knqyf263 knqyf263 deleted the refactor/scanner branch October 20, 2023 02:15
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.

None yet

3 participants