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

[ENGPROD-6507] filter secrets and licenses based on rego policy #4

Merged

Conversation

kristyko
Copy link

Description

Filter out Secrets and Licenses based on repo ignore policy just like Misconfigurations and Vulnerabilities.

Related issues

  • Close #XXX

Related PRs

  • #XXX
  • #YYY

Remove this section if you don't have 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).

@kristyko kristyko requested a review from carsongee December 14, 2023 08:44
func applyPolicy(ctx context.Context, vulns []types.DetectedVulnerability, misconfs []types.DetectedMisconfiguration,
policyFile string) ([]types.DetectedVulnerability, []types.DetectedMisconfiguration, int, error) {
func applyPolicy(ctx context.Context, vulns []types.DetectedVulnerability, misconfs []types.DetectedMisconfiguration, scrts []ftypes.SecretFinding, lics []types.DetectedLicense,
policyFile string) ([]types.DetectedVulnerability, []types.DetectedMisconfiguration, int, []ftypes.SecretFinding, []types.DetectedLicense, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll probably want to move these to before the int return. Or at least it makes the most logical sense to me that we should return the count of ignored things right before the error

Copy link
Member

Choose a reason for hiding this comment

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

I retract this...it is not what I thought it was 😢

Comment on lines 725 to 750
Secrets: []ftypes.SecretFinding{
{
RuleID: "generic-wanted-rule",
Severity: dbTypes.SeverityHigh.String(),
Title: "Secret that should pass filter on rule id",
StartLine: 1,
EndLine: 2,
Match: "*****",
},
{
RuleID: "generic-critical-rule",
Severity: dbTypes.SeverityHigh.String(),
Title: "Critical Secret shouldn't pass filter",
StartLine: 1,
EndLine: 2,
Match: "*****",
},
},
Licenses: []types.DetectedLicense{
{
Name: "GPL-3.0",
Severity: dbTypes.SeverityHigh.String(),
FilePath: "usr/share/gcc/python/libstdcxx/v6/printers.py",
Category: "restricted",
Confidence: 1,
},
Copy link
Member

Choose a reason for hiding this comment

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

Some classic tabs vs. spaces indent problems here

filteredLics = append(filteredLics, lic)
}

return filteredVulns, filteredMisconfs, ignoredMisconfs, filteredSecrets, filteredLics, nil
Copy link
Member

Choose a reason for hiding this comment

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

So, maybe I'm wrong. It seems really weird that we're using a blind int to "only" return the count of ignored misconfigurations. Probably a follow-up change, but it seems like it would be useful to make a struct with counts of all the ignores

Copy link
Author

Choose a reason for hiding this comment

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

@carsongee I just pushed a commit with a fix for the indentation issue. Should I open a follow-up ticket for the structure for ignores?

Copy link
Member

Choose a reason for hiding this comment

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

Let's start a "discussion" per their procedures and see if it is of interest for them

Copy link
Member

@carsongee carsongee left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Please also open a PR to the main repository with this change. Thank you for picking this up!

@carsongee carsongee merged commit bb6fa17 into main_datarobot Dec 27, 2023
6 of 11 checks passed
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.

2 participants