-
Notifications
You must be signed in to change notification settings - Fork 117
feat(scanner): Break out options for enabling libs and policies #1280
Conversation
c606912
to
d885472
Compare
@chen-keinan could you help me review this? I have made a PR in trivy to adapt to this change: aquasecurity/trivy#4070 - are there other users of defsec that you know that will need a change? |
d885472
to
7e92a22
Compare
regoScanner := rego.NewScanner(types.SourceCloud, s.scannerOptions...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
nit: maybe pass s
(Scanner) to LoadPolicies method as there are too many params
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.
Yeah that's a good idea, however s
could be any type of Scanner. I tried to implement this with generics but quickly ran into a dead end that a method on a receiver where the base (in this case rego scanner) isn't generic cannot accept generic parameters. https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#No-parameterized-methods
I will revisit this again. It will be a lot cleaner if we only passed s
since all function inputs (except srcFS) come from s
.
regoScanner := rego.NewScanner(types.SourceCloud, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.useEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
same as above comment
regoScanner := rego.NewScanner(types.SourceCloud, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
same as above
regoScanner := rego.NewScanner(types.SourceDockerfile, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
+1
policyFS = s.policyFS | ||
} | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, policyFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, policyFS, s.policyDirs, s.policyReaders); err != nil { |
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.
+1
regoScanner := rego.NewScanner(types.SourceKubernetes, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
+1
regoScanner := rego.NewScanner(types.SourceTOML, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
+1
regoScanner := rego.NewScanner(types.SourceYAML, s.options...) | ||
regoScanner.SetParentDebugLogger(s.debug) | ||
if err := regoScanner.LoadPolicies(s.loadEmbedded, srcFS, s.policyDirs, s.policyReaders); err != nil { | ||
if err := regoScanner.LoadPolicies(s.loadEmbeddedLibraries, s.loadEmbeddedPolicies, srcFS, s.policyDirs, s.policyReaders); err != nil { |
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.
+1
require.NoError(t, err) | ||
|
||
scanner := kubernetes.NewScanner(options.ScannerWithEmbeddedPolicies(true)) | ||
scanner := kubernetes.NewScanner(options.ScannerWithEmbeddedPolicies(true), options.ScannerWithEmbeddedLibraries(true), options.ScannerWithEmbeddedLibraries(true)) |
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.
+1
` | ||
|
||
results, err := NewScanner(options.ScannerWithEmbeddedPolicies(true)).ScanReader(context.TODO(), "k8s.yaml", strings.NewReader(file)) | ||
results, err := NewScanner(options.ScannerWithEmbeddedPolicies(true), options.ScannerWithEmbeddedLibraries(true), options.ScannerWithEmbeddedLibraries(true)).ScanReader(context.TODO(), "k8s.yaml", strings.NewReader(file)) |
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.
nit:
results, err := NewScanner(options.ScannerWithEmbeddedPolicies(true), options.ScannerWithEmbeddedLibraries(true), options.ScannerWithEmbeddedLibraries(true)).ScanReader(context.TODO(), "k8s.yaml", strings.NewReader(file)) | |
results, err := NewScanner( | |
options.ScannerWithEmbeddedPolicies(true), | |
options.ScannerWithEmbeddedLibraries(true), | |
options.ScannerWithEmbeddedLibraries(true)).ScanReader(context.TODO(), "k8s.yaml", strings.NewReader(file)) |
@simar7 look good, I have added few |
7e92a22
to
d0fadf8
Compare
Signed-off-by: Simar <[email protected]>
Signed-off-by: Simar <[email protected]>
Signed-off-by: Simar <[email protected]>
Signed-off-by: Simar <[email protected]>
Signed-off-by: Simar <[email protected]>
ba33c6f
to
220ede3
Compare
…d policies (#1280)" (#1298)" (#1357) * Revert "Revert "feat(scanner): Break out options for enabling libs and policies (#1280)" (#1298)" This reverts commit 63a8b4f. * add loadembedded for terraformplan Signed-off-by: Simar <[email protected]> * fix tests Signed-off-by: Simar <[email protected]> --------- Signed-off-by: Simar <[email protected]>
This adds the
SetUseEmbeddedLibraries
option for scanners. It is needed in cases where we only want to load libraries but not policies.Signed-off-by: Simar [email protected]