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 aliases to '--scanners' #5558

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Conversation

knqyf263
Copy link
Collaborator

Description

This PR updates the allowed values of --scanners from config to misconfig. It also adds aliases for backward compatibility and spelling discrepancies.

  • misconfig also allows
    • config
    • misconf
    • misconfiguration
  • vuln also allows
    • vulnerability

Before

$ trivy repo --scanners config .

NOTE: It remains working.

After

$ trivy repo --scanners misconfig .

Related Issues

Related PRs

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

Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 marked this pull request as ready for review November 13, 2023 02:04
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

I left 1 comment.
The rest look good.

But i am worried about different names of target (config) and scanner (misconfig).
We don't write that we support config, misconf and misconfiguration scanner words and different names can be confusing.
Is there any point in this renaming?

pkg/flag/scan_flags.go Show resolved Hide resolved
@knqyf263
Copy link
Collaborator Author

But i am worried about different names of target (config) and scanner (misconfig).

'Target' refers to what is being scanned, like container images, VM images, or configuration files. It's the object of our security analysis. 'Scanner', on the other hand, is the tool or method used to detect security issues within the 'Target'. For example, if we scan configuration files, the 'Target' is the configuration files themselves, and the 'Scanner' identifies any misconfigurations in them. Therefore, trivy config --scanners misconfig makes sense.

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@DmitriyLewen
Copy link
Contributor

hm... It make sense. You convinced me 😄
Thanks for explanation.

Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

LGTM

@knqyf263 knqyf263 self-assigned this Nov 13, 2023
Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263
Copy link
Collaborator Author

trivy config is just an alias of trivy repo/fs --scanners misconfig. We can remove trivy config if it is confusing.

@DmitriyLewen
Copy link
Contributor

I think Trivy config is convenient option for users who only use Trivy for misconfigurations.

After your explanation, I understand your logic and agree with it.
Also, if user wants to use scanners instead of target - we support config scanner (if he writes config by mistake) and this will not be a problem for the user.

I think we don't need to remove config.

@knqyf263
Copy link
Collaborator Author

OK, I'll wait for @simar7, then.

@itaysk
Copy link
Contributor

itaysk commented Nov 14, 2023

We can remove trivy config if it is confusing.

TBH I have always found trivy config to be weird command. I think it's fine to have special cases commands which are not targets, but just the name itself "config" is not great. I think it feels more about configuring Trivy rather than scanning something (e.g git config). If anything, I'd rename it to trivy iac instead of removing. I know this is less "accurate" name, but I believe most users are accustomed to "IaC scanning". not strongly advocating to make a change now, just sharing my thoughts.

@knqyf263
Copy link
Collaborator Author

I'd rename it to trivy iac instead of removing. I know this is less "accurate" name, but I believe most users are accustomed to "IaC scanning".

When I named it trivy config, we had planned to scan WordPress configuration files, and we anticipated detecting misconfigurations in middleware and applications such as Nginx and Ruby on Rails, leading me to believe that the name 'IaC scanning' might soon cause confusion. Then, I avoided using the word IaC.

However, in reality, WordPress configuration scanning has been added only to the product version, and we currently have no plans for supporting anything beyond IaC. Therefore, I think the name trivy iac is now more appropriate and reasonable, and renaming it would be a good choice.

@simar7
Copy link
Member

simar7 commented Nov 14, 2023

Keeping aliases for backwards compatibility makes sense to me. Moving forwards I think we can do trivy iac as @knqyf263 mentioned we only support IaC in OSS.

Therefore to summarize:

trivy iac will be the new command that runs IaC scanning
It can have the following aliases (as already handled here f6afc9d#diff-2e6004de7aba4f429611e2c3d2cf27da7a14031504e0ccd86dad13ddb5543aa3R44-R46)

  • trivy config
  • trivy misconf
  • trivy misconfiguration

@knqyf263
Copy link
Collaborator Author

@simar7 This PR is about scanners, not targets. We discussed targets a bit, but this PR renamed --scanners config to --scanners misconfig and kept --scanners config for backward compatibility.

The target (trivy iac or trivy config) is not a main topic of this PR. We can discuss it later.

I explained the differences between targets and scanners here.
#5558 (comment)

In short,

  • Target (off-topic)
    • config (now)
    • iac
  • Scanner (this PR)
    • config (old)
    • misconfig (new)

We used config for both the target and scanner, and it was confusing. The word "misconfiguration" is more appropriate for scanners, and the word "IaC" may be more appropriate for targets.

@simar7
Copy link
Member

simar7 commented Nov 15, 2023

@simar7 This PR is about scanners, not targets. We discussed targets a bit, but this PR renamed --scanners config to --scanners misconfig and kept --scanners config for backward compatibility.

The target (trivy iac or trivy config) is not a main topic of this PR. We can discuss it later.

I explained the differences between targets and scanners here. #5558 (comment)

In short,

  • Target (off-topic)

    • config (now)
    • iac
  • Scanner (this PR)

    • config (old)
    • misconfig (new)

We used config for both the target and scanner, and it was confusing. The word "misconfiguration" is more appropriate for scanners, and the word "IaC" may be more appropriate for targets.

Yeah that's fine by me.

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.

4 participants