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

feat: add a flag for selectively enabling config types #4990

Closed
wants to merge 1 commit into from

Conversation

nikpivkin
Copy link
Contributor

Description

  • add tests
  • update doc

Related issues

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).

@simar7
Copy link
Member

simar7 commented Aug 14, 2023

hi @nikpivkin please hold off on this as we have not planned it for this milestone.

@knqyf263
Copy link
Collaborator

This enhancement is similar to this proposal. Do we want to provide a consolidated flag across all scanners, like --sub-scanners ruby,terraform? Or we can expand the existing flag --scanners vuln:ruby config:terraform. Also, we need to think about enabling or disabling. What if people just want to disable Azure ARM scanning with --config-type?

@simar7
Copy link
Member

simar7 commented Aug 17, 2023

This enhancement is similar to this proposal. Do we want to provide a consolidated flag across all scanners, like --sub-scanners ruby,terraform? Or we can expand the existing flag --scanners vuln:ruby config:terraform. Also, we need to think about enabling or disabling. What if people just want to disable Azure ARM scanning with --config-type?

This issue only focuses only misconfiguration scanning. Today it's a single target with no way to disable/enable different kinds of IaC.

As for consolidation across other scanners, yes I think making them more granular makes sense. Ideally, this control knob should exist at every scan type level. So for e.g. misconf can enable/disable: terraform, cloudformation, ARM etc. and vulnerability scanning can enable/disable: packages, library etc.

@github-actions
Copy link

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Oct 17, 2023
@github-actions github-actions bot closed this Nov 7, 2023
@knqyf263 knqyf263 reopened this Nov 7, 2023
@knqyf263
Copy link
Collaborator

knqyf263 commented Nov 7, 2023

@itaysk Do you have any thoughts? We renamed --security-checks to --scanners according to your suggestion. Do you prefer --config-scanners rather than --config-type?

If we also want to provide a capability of disabling config types, we may need --enable-config-type and --disable-config-type.

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Nov 8, 2023
@itaysk
Copy link
Contributor

itaysk commented Nov 9, 2023

Do you prefer --config-scanners rather than --config-type

Between these I'd go with --scanner-config
But I'm not sure we have to make it generic like that, as opposed to every scanner can have scanner-specific flags. I'm afraid in the future it would evolve to more than a list of scanner "components" to enable/disable, but how to configure them, and then we end up with crazy syntaxes like --scanner-config vulnerability:ruby:some-ruby-thing=something. If we are not sure, it's better to start with specific flags and later consolidate.

@knqyf263
Copy link
Collaborator

Hmm, the name is probably confusing. I think --config-scanners and --config-type means misconfiguration scanners/types like terraform. @simar7 Am I correct?

@itaysk You suggested --scanner-config because you thought the flag was for all scanners, right? We can forget about it. Another flag looks better now.

We might want to replace --scanner config with --scanner misconfig so that we will not be confused with the word config. Then, the new flag should be --misconfig-scanners or --misconfig-types. What do you think?

@simar7
Copy link
Member

simar7 commented Nov 10, 2023

Hmm, the name is probably confusing. I think --config-scanners and --config-type means misconfiguration scanners/types like terraform. @simar7 Am I correct?

That's right. I had proposed trivy config --config-type=dockerfile,helm.

We might want to replace --scanner config with --scanner misconfig so that we will not be confused with the word config. Then, the new flag should be --misconfig-scanners or --misconfig-types. What do you think?

Yeah this is good, I think it will reduce confusion as we call reference misconfig mostly everywhere.

@itaysk
Copy link
Contributor

itaysk commented Nov 10, 2023

yes. sorry I read this too fast. SGTM

@knqyf263
Copy link
Collaborator

@simar7 I opened a PR.
#5558

@knqyf263
Copy link
Collaborator

#5558 got merged. We can now use something like --misconfig-type. I'd vote for --misconfig-scanner terraform,dockerfile as --vuln-type os is more higher-level. We will probably add --vuln-scanners php,python, so --misconfig-scanners is more aligned with that. What do you think?

@simar7
Copy link
Member

simar7 commented Nov 28, 2023

#5558 got merged. We can now use something like --misconfig-type. I'd vote for --misconfig-scanner terraform,dockerfile as --vuln-type os is more higher-level. We will probably add --vuln-scanners php,python, so --misconfig-scanners is more aligned with that. What do you think?

Yeah --misconfig-scanners=dockerfile,terraform makes sense to me. We can close this PR for now and start afresh. I can take care of it.

@simar7 simar7 closed this Nov 28, 2023
@nikpivkin nikpivkin deleted the feat/config-types branch January 22, 2024 07:45
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.

feat(misconf): Selectively enable misconfiguration scanners
4 participants