From 740fcb9544ade6d006e77ad25987aab349645f32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reigota?= Date: Fri, 9 Apr 2021 16:13:07 +0100 Subject: [PATCH 1/4] Fix KICS not rendering Chart bug #2761 --- pkg/engine/provider/filesystem.go | 1 + pkg/engine/provider/filesystem_test.go | 75 +++++++++++++++++++++----- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/pkg/engine/provider/filesystem.go b/pkg/engine/provider/filesystem.go index 6202993119b..150939b36e5 100644 --- a/pkg/engine/provider/filesystem.go +++ b/pkg/engine/provider/filesystem.go @@ -146,6 +146,7 @@ func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions if err != nil { return true, nil } + return false, nil } if f, ok := s.excludes[info.Name()]; ok && containsFile(f, info) { diff --git a/pkg/engine/provider/filesystem_test.go b/pkg/engine/provider/filesystem_test.go index ea312ab5525..3d9daf5efa7 100644 --- a/pkg/engine/provider/filesystem_test.go +++ b/pkg/engine/provider/filesystem_test.go @@ -3,6 +3,7 @@ package provider import ( "context" "io" + "io/fs" "os" "path/filepath" "reflect" @@ -10,6 +11,7 @@ import ( "github.com/Checkmarx/kics/pkg/model" dockerParser "github.com/Checkmarx/kics/pkg/parser/docker" + "github.com/Checkmarx/kics/test" "github.com/pkg/errors" ) @@ -185,7 +187,16 @@ func TestFileSystemSourceProvider_GetSources(t *testing.T) { //nolint // TestFileSystemSourceProvider_checkConditions tests the functions [checkConditions()] and all the methods called by them func TestFileSystemSourceProvider_checkConditions(t *testing.T) { - infoFile, _ := os.Stat("../../../assets/queries") + if err := test.ChangeCurrentDir("kics"); err != nil { + t.Errorf("failed to change dir: %s", err) + } + infoFile, err := os.Stat(filepath.FromSlash("assets/queries")) + checkStatErr(t, err) + fileInfoSlice := []fs.FileInfo{ + infoFile, + } + infoHelm, errHelm := os.Stat(filepath.FromSlash("test/fixtures/test_helm")) + checkStatErr(t, errHelm) type fields struct { path string excludes map[string][]os.FileInfo @@ -195,19 +206,20 @@ func TestFileSystemSourceProvider_checkConditions(t *testing.T) { extensions model.Extensions path string } + type want struct { + got bool + err error + } tests := []struct { name string fields fields args args - want struct { - got bool - err error - } + want want }{ { name: "check_conditions", fields: fields{ - path: "../../assets/queries", + path: filepath.FromSlash("assets/queries"), excludes: nil, }, args: args{ @@ -215,16 +227,49 @@ func TestFileSystemSourceProvider_checkConditions(t *testing.T) { extensions: model.Extensions{ ".dockerfile": dockerParser.Parser{}, }, - path: "../../assets/queries", + path: filepath.FromSlash("assets/queries"), }, - want: struct { - got bool - err error - }{ + want: want{ got: true, err: nil, }, }, + { + name: "check_conditions_chart", + fields: fields{ + path: filepath.FromSlash("test/fixtures/test_helm"), + excludes: nil, + }, + args: args{ + info: infoHelm, + extensions: model.Extensions{}, + path: filepath.FromSlash("test/fixtures/test_helm"), + }, + want: want{ + got: false, + err: nil, + }, + }, + { + name: "should_skip_folder", + fields: fields{ + path: filepath.FromSlash("assets/queries"), + excludes: map[string][]fs.FileInfo{ + "queries": fileInfoSlice, + }, + }, + args: args{ + info: infoFile, + extensions: model.Extensions{ + ".dockerfile": dockerParser.Parser{}, + }, + path: filepath.FromSlash("assets/queries"), + }, + want: want{ + got: true, + err: filepath.SkipDir, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -233,7 +278,7 @@ func TestFileSystemSourceProvider_checkConditions(t *testing.T) { excludes: tt.fields.excludes, } if got, err := s.checkConditions(tt.args.info, tt.args.extensions, tt.args.path); got != tt.want.got || err != tt.want.err { - t.Errorf("FileSystemSourceProvider.checkConditions() = %v, want %v", got, tt.want) + t.Errorf("FileSystemSourceProvider.checkConditions() = %v, want %v", err, tt.want) } }) } @@ -254,3 +299,9 @@ var mockResolverSink = func(ctx context.Context, filename string) error { var mockErrResolverSink = func(ctx context.Context, filename string) error { return errors.New("") } + +func checkStatErr(t *testing.T, err error) { + if err != nil { + t.Errorf("failed to get info: %s", err) + } +} From e19b9949689deff0af1f67dfe3c7528a8f6079a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reigota?= Date: Fri, 9 Apr 2021 18:00:46 +0100 Subject: [PATCH 2/4] Fixed bug with Tracker counters #2735 --- pkg/engine/provider/filesystem.go | 60 +++++++++++++++++--------- pkg/engine/provider/filesystem_test.go | 8 ++-- pkg/engine/provider/provider.go | 2 +- pkg/kics/resolver_sink.go | 19 +++++--- pkg/kics/service.go | 3 +- pkg/kics/sink.go | 1 + 6 files changed, 58 insertions(+), 35 deletions(-) diff --git a/pkg/engine/provider/filesystem.go b/pkg/engine/provider/filesystem.go index 150939b36e5..0c932ca3982 100644 --- a/pkg/engine/provider/filesystem.go +++ b/pkg/engine/provider/filesystem.go @@ -26,30 +26,39 @@ var ErrNotSupportedFile = errors.New("invalid file format") func NewFileSystemSourceProvider(path string, excludes []string) (*FileSystemSourceProvider, error) { log.Debug().Msgf("provider.NewFileSystemSourceProvider()") ex := make(map[string][]os.FileInfo, len(excludes)) + fs := &FileSystemSourceProvider{ + path: filepath.FromSlash(path), + excludes: ex, + } for _, exclude := range excludes { excludePaths, err := getExcludePaths(exclude) if err != nil { return nil, err } - for _, excludePath := range excludePaths { - info, err := os.Stat(excludePath) - if err != nil { - if os.IsNotExist(err) { - continue - } - return nil, errors.Wrap(err, "failed to open excluded file") - } - if _, ok := ex[info.Name()]; !ok { - ex[info.Name()] = make([]os.FileInfo, 0) - } - ex[info.Name()] = append(ex[info.Name()], info) + if err := fs.AddExcluded(excludePaths); err != nil { + return nil, err } } - return &FileSystemSourceProvider{ - path: filepath.FromSlash(path), - excludes: ex, - }, nil + return fs, nil +} + +// TODO: NEDDS UNIT TESTS +func (s *FileSystemSourceProvider) AddExcluded(excludePaths []string) error { + for _, excludePath := range excludePaths { + info, err := os.Stat(excludePath) + if err != nil { + if os.IsNotExist(err) { + continue + } + return errors.Wrap(err, "failed to open excluded file") + } + if _, ok := s.excludes[info.Name()]; !ok { + s.excludes[info.Name()] = make([]os.FileInfo, 0) + } + s.excludes[info.Name()] = append(s.excludes[info.Name()], info) + } + return nil } func getExcludePaths(pathExpressions string) ([]string, error) { @@ -71,6 +80,7 @@ func (s *FileSystemSourceProvider) GetBasePath() string { // GetSources tries to open file or directory and execute sink function on it func (s *FileSystemSourceProvider) GetSources(ctx context.Context, extensions model.Extensions, sink Sink, resolverSink ResolverSink) error { + beenRendered := false fileInfo, err := os.Stat(s.path) if err != nil { return errors.Wrap(err, "failed to open path") @@ -99,12 +109,20 @@ func (s *FileSystemSourceProvider) GetSources(ctx context.Context, } // ------------------ Helm resolver -------------------------------- + // TODO: NEEDS REFACTORING if info.IsDir() { - err = resolverSink(ctx, strings.ReplaceAll(path, "\\", "/")) - if err != nil { - sentry.CaptureException(err) - log.Err(err). - Msgf("Filesystem files provider couldn't Resolve Directory, file=%s", info.Name()) + if !beenRendered { // This variable will block helm from re-rendering subcharts + excluded, errRes := resolverSink(ctx, strings.ReplaceAll(path, "\\", "/")) + if errRes != nil { + sentry.CaptureException(errRes) + log.Err(errRes). + Msgf("Filesystem files provider couldn't Resolve Directory, file=%s", info.Name()) + return nil // so beRendered flag isn't alterd + } + if errAdd := s.AddExcluded(excluded); errAdd != nil { + log.Err(errAdd).Msgf("got an error whooo") + } + beenRendered = true } return nil } diff --git a/pkg/engine/provider/filesystem_test.go b/pkg/engine/provider/filesystem_test.go index 3d9daf5efa7..e4e370d4d90 100644 --- a/pkg/engine/provider/filesystem_test.go +++ b/pkg/engine/provider/filesystem_test.go @@ -292,12 +292,12 @@ var mockErrSink = func(ctx context.Context, filename string, content io.ReadClos return errors.New("") } -var mockResolverSink = func(ctx context.Context, filename string) error { - return nil +var mockResolverSink = func(ctx context.Context, filename string) ([]string, error) { + return []string{}, nil } -var mockErrResolverSink = func(ctx context.Context, filename string) error { - return errors.New("") +var mockErrResolverSink = func(ctx context.Context, filename string) ([]string, error) { + return []string{}, errors.New("") } func checkStatErr(t *testing.T, err error) { diff --git a/pkg/engine/provider/provider.go b/pkg/engine/provider/provider.go index 398d1483abb..711930a64a4 100644 --- a/pkg/engine/provider/provider.go +++ b/pkg/engine/provider/provider.go @@ -12,7 +12,7 @@ import ( type Sink func(ctx context.Context, filename string, content io.ReadCloser) error // ResolverSink defines a sink function to be passed as reference to functions for resolved files/templates -type ResolverSink func(ctx context.Context, filename string) error +type ResolverSink func(ctx context.Context, filename string) ([]string, error) // SourceProvider is the interface that wraps the basic GetSources method. // GetBasePath returns base path of FileSystemSourceProvider diff --git a/pkg/kics/resolver_sink.go b/pkg/kics/resolver_sink.go index aac55228396..3bf7a5fc9c7 100644 --- a/pkg/kics/resolver_sink.go +++ b/pkg/kics/resolver_sink.go @@ -11,20 +11,24 @@ import ( "github.com/rs/zerolog/log" ) -func (s *Service) resolverSink(ctx context.Context, filename, scanID string) error { - s.Tracker.TrackFileFound() +func (s *Service) resolverSink(ctx context.Context, filename, scanID string) ([]string, error) { kind := s.Resolver.GetType(filename) if kind == model.KindCOMMON { - return nil + return []string{}, nil } resFiles, err := s.Resolver.Resolve(filename, kind) if err != nil { - return errors.Wrap(err, "failed to render file content") + return []string{}, errors.Wrap(err, "failed to render file content") } - for _, rfile := range resFiles.File { + + excluded := make([]string, len(resFiles.File)) + + for idx, rfile := range resFiles.File { + s.Tracker.TrackFileFound() + excluded[idx] = rfile.FileName documents, _, err := s.Parser.Parse(rfile.FileName, rfile.Content) if err != nil { - return errors.Wrap(err, "failed to parse file content") + return []string{}, errors.Wrap(err, "failed to parse file content") } for _, document := range documents { _, err = json.Marshal(document) @@ -47,6 +51,7 @@ func (s *Service) resolverSink(ctx context.Context, filename, scanID string) err } s.saveToFile(ctx, &file) } + s.Tracker.TrackFileParse() } - return nil + return excluded, nil } diff --git a/pkg/kics/service.go b/pkg/kics/service.go index b7fac6639c3..86a85dbef20 100644 --- a/pkg/kics/service.go +++ b/pkg/kics/service.go @@ -55,7 +55,7 @@ func (s *Service) StartScan(ctx context.Context, scanID string, hideProgress boo func(ctx context.Context, filename string, rc io.ReadCloser) error { return s.sink(ctx, filename, scanID, rc) }, - func(ctx context.Context, filename string) error { // Sink used for resolver files and templates + func(ctx context.Context, filename string) ([]string, error) { // Sink used for resolver files and templates return s.resolverSink(ctx, filename, scanID) }, ); err != nil { @@ -112,6 +112,5 @@ func (s *Service) saveToFile(ctx context.Context, file *model.FileMetadata) { err := s.Storage.SaveFile(ctx, file) if err == nil { s.files = append(s.files, *file) - s.Tracker.TrackFileParse() } } diff --git a/pkg/kics/sink.go b/pkg/kics/sink.go index 1bcf65624b4..ec8f19e16df 100644 --- a/pkg/kics/sink.go +++ b/pkg/kics/sink.go @@ -46,6 +46,7 @@ func (s *Service) sink(ctx context.Context, filename, scanID string, rc io.Reade } s.saveToFile(ctx, &file) } + s.Tracker.TrackFileParse() return errors.Wrap(err, "failed to save file content") } From 4857424448b82c241f9c5dfff1a75c262db5ded6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reigota?= Date: Mon, 12 Apr 2021 12:24:35 +0100 Subject: [PATCH 3/4] Added Unit Testing and Refactured Helm part of GetSources() #2735 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Reigota --- pkg/engine/provider/filesystem.go | 37 ++++---- pkg/engine/provider/filesystem_test.go | 119 ++++++++++++++++++++++++- pkg/engine/provider/provider.go | 2 +- 3 files changed, 138 insertions(+), 20 deletions(-) diff --git a/pkg/engine/provider/filesystem.go b/pkg/engine/provider/filesystem.go index 0c932ca3982..1825eab1f21 100644 --- a/pkg/engine/provider/filesystem.go +++ b/pkg/engine/provider/filesystem.go @@ -43,7 +43,7 @@ func NewFileSystemSourceProvider(path string, excludes []string) (*FileSystemSou return fs, nil } -// TODO: NEDDS UNIT TESTS +// AddExcluded add new excluded files to the File System Source Provider func (s *FileSystemSourceProvider) AddExcluded(excludePaths []string) error { for _, excludePath := range excludePaths { info, err := os.Stat(excludePath) @@ -80,7 +80,7 @@ func (s *FileSystemSourceProvider) GetBasePath() string { // GetSources tries to open file or directory and execute sink function on it func (s *FileSystemSourceProvider) GetSources(ctx context.Context, extensions model.Extensions, sink Sink, resolverSink ResolverSink) error { - beenRendered := false + resolved := false fileInfo, err := os.Stat(s.path) if err != nil { return errors.Wrap(err, "failed to open path") @@ -104,29 +104,26 @@ func (s *FileSystemSourceProvider) GetSources(ctx context.Context, return err } - if shouldSkip, skipFolder := s.checkConditions(info, extensions, path); shouldSkip { + if shouldSkip, skipFolder := s.checkConditions(info, extensions, path, resolved); shouldSkip { return skipFolder } // ------------------ Helm resolver -------------------------------- - // TODO: NEEDS REFACTORING if info.IsDir() { - if !beenRendered { // This variable will block helm from re-rendering subcharts - excluded, errRes := resolverSink(ctx, strings.ReplaceAll(path, "\\", "/")) - if errRes != nil { - sentry.CaptureException(errRes) - log.Err(errRes). - Msgf("Filesystem files provider couldn't Resolve Directory, file=%s", info.Name()) - return nil // so beRendered flag isn't alterd - } - if errAdd := s.AddExcluded(excluded); errAdd != nil { - log.Err(errAdd).Msgf("got an error whooo") - } - beenRendered = true + excluded, errRes := resolverSink(ctx, strings.ReplaceAll(path, "\\", "/")) + if errRes != nil { + sentry.CaptureException(errRes) + log.Err(errRes). + Msgf("Filesystem files provider couldn't Resolve Directory, file=%s", info.Name()) + return nil } + if errAdd := s.AddExcluded(excluded); errAdd != nil { + log.Err(errAdd).Msgf("Filesystem files provider couldn't exclude rendered Chart files, Chart=%s", info.Name()) + } + resolved = true return nil } - // ------------------------------------------------------------ + // ----------------------------------------------------------------- c, err := os.Open(filepath.Clean(path)) if err != nil { @@ -154,7 +151,8 @@ func closeFile(file *os.File, info os.FileInfo) { } } -func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions model.Extensions, path string) (bool, error) { +func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions model.Extensions, + path string, resolved bool) (bool, error) { if info.IsDir() { if f, ok := s.excludes[info.Name()]; ok && containsFile(f, info) { log.Info().Msgf("Directory ignored: %s", path) @@ -164,6 +162,9 @@ func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions if err != nil { return true, nil } + if resolved { + return true, nil + } return false, nil } diff --git a/pkg/engine/provider/filesystem_test.go b/pkg/engine/provider/filesystem_test.go index e4e370d4d90..d38bf948b76 100644 --- a/pkg/engine/provider/filesystem_test.go +++ b/pkg/engine/provider/filesystem_test.go @@ -171,6 +171,22 @@ func TestFileSystemSourceProvider_GetSources(t *testing.T) { //nolint }, wantErr: true, }, + { + name: "test_helm_source_provider", + fields: fields{ + path: "../../../test/fixtures/test_helm_subchart", + excludes: map[string][]os.FileInfo{}, + }, + args: args{ + ctx: nil, + extensions: model.Extensions{ + ".dockerfile": dockerParser.Parser{}, + }, + sink: mockSink, + resolverSink: mockResolverSink, + }, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -185,6 +201,41 @@ func TestFileSystemSourceProvider_GetSources(t *testing.T) { //nolint } } +func TestFileSystemSourceProvider_GetBasePath(t *testing.T) { + if err := test.ChangeCurrentDir("kics"); err != nil { + t.Errorf("failed to change dir: %s", err) + } + fsystem, err := initFs(filepath.FromSlash("test"), []string{}) + if err != nil { + t.Errorf("failed to initialize a new File System Source Provider") + } + type feilds struct { + fs *FileSystemSourceProvider + } + tests := []struct { + name string + feilds feilds + want string + }{ + { + name: "test_get_base_path", + feilds: feilds{ + fs: fsystem, + }, + want: "test", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.feilds.fs.GetBasePath() + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetBasePath() = %v, want = %v", got, tt.want) + } + }) + } +} + // TestFileSystemSourceProvider_checkConditions tests the functions [checkConditions()] and all the methods called by them func TestFileSystemSourceProvider_checkConditions(t *testing.T) { if err := test.ChangeCurrentDir("kics"); err != nil { @@ -277,13 +328,66 @@ func TestFileSystemSourceProvider_checkConditions(t *testing.T) { path: tt.fields.path, excludes: tt.fields.excludes, } - if got, err := s.checkConditions(tt.args.info, tt.args.extensions, tt.args.path); got != tt.want.got || err != tt.want.err { + if got, err := s.checkConditions(tt.args.info, tt.args.extensions, tt.args.path, false); got != tt.want.got || err != tt.want.err { t.Errorf("FileSystemSourceProvider.checkConditions() = %v, want %v", err, tt.want) } }) } } +// TestFileSystemSourceProvider_AddExcluded tests the functions [AddExcluded()] and all the methods called by them +func TestFileSystemSourceProvider_AddExcluded(t *testing.T) { + if err := test.ChangeCurrentDir("kics"); err != nil { + t.Errorf("failed to change dir: %s", err) + } + fsystem, err := initFs(filepath.FromSlash("test"), []string{}) + if err != nil { + t.Errorf("failed to initialize a new File System Source Provider") + } + type feilds struct { + fs *FileSystemSourceProvider + } + type args struct { + excludePaths []string + } + tests := []struct { + name string + feilds feilds + args args + want []string + wantErr bool + }{ + { + name: "test_add_excluded", + feilds: feilds{ + fs: fsystem, + }, + args: args{ + excludePaths: []string{ + "test/fixtures/config_test", + }, + }, + want: []string{ + "config_test", + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.feilds.fs.AddExcluded(tt.args.excludePaths) + if (err != nil) != tt.wantErr { + t.Errorf("AddExcluded() = %v, wantErr = %v", err, tt.wantErr) + } + got := getFSExcludes(tt.feilds.fs) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("AddExcluded() = %v, want = %v", got, tt.want) + } + }) + } +} + var mockSink = func(ctx context.Context, filename string, content io.ReadCloser) error { return nil } @@ -305,3 +409,16 @@ func checkStatErr(t *testing.T, err error) { t.Errorf("failed to get info: %s", err) } } + +// initFs creates a new instance of File System Source Provider +func initFs(path string, excluded []string) (*FileSystemSourceProvider, error) { + return NewFileSystemSourceProvider(path, excluded) +} + +func getFSExcludes(fsystem *FileSystemSourceProvider) []string { + excluded := make([]string, 0) + for key := range fsystem.excludes { + excluded = append(excluded, key) + } + return excluded +} diff --git a/pkg/engine/provider/provider.go b/pkg/engine/provider/provider.go index 711930a64a4..f27d4e794b4 100644 --- a/pkg/engine/provider/provider.go +++ b/pkg/engine/provider/provider.go @@ -20,5 +20,5 @@ type ResolverSink func(ctx context.Context, filename string) ([]string, error) type SourceProvider interface { GetBasePath() string GetSources(ctx context.Context, extensions model.Extensions, sink Sink, resolverSink ResolverSink) error - checkConditions(info os.FileInfo, extensions model.Extensions, path string) (bool, error) + checkConditions(info os.FileInfo, extensions model.Extensions, path string, resolved bool) (bool, error) } From bbe4732765a3f0e28907b91300722f8db79e50c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reigota?= Date: Tue, 13 Apr 2021 17:20:44 +0100 Subject: [PATCH 4/4] added sugestions #2735 --- pkg/engine/provider/filesystem.go | 5 +---- pkg/engine/provider/provider.go | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/engine/provider/filesystem.go b/pkg/engine/provider/filesystem.go index 1825eab1f21..276fe714fc0 100644 --- a/pkg/engine/provider/filesystem.go +++ b/pkg/engine/provider/filesystem.go @@ -159,10 +159,7 @@ func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions return true, filepath.SkipDir } _, err := os.Stat(filepath.Join(path, "Chart.yaml")) - if err != nil { - return true, nil - } - if resolved { + if err != nil || resolved { return true, nil } return false, nil diff --git a/pkg/engine/provider/provider.go b/pkg/engine/provider/provider.go index f27d4e794b4..23e942973b9 100644 --- a/pkg/engine/provider/provider.go +++ b/pkg/engine/provider/provider.go @@ -3,7 +3,6 @@ package provider import ( "context" "io" - "os" "github.com/Checkmarx/kics/pkg/model" ) @@ -20,5 +19,4 @@ type ResolverSink func(ctx context.Context, filename string) ([]string, error) type SourceProvider interface { GetBasePath() string GetSources(ctx context.Context, extensions model.Extensions, sink Sink, resolverSink ResolverSink) error - checkConditions(info os.FileInfo, extensions model.Extensions, path string, resolved bool) (bool, error) }