From 94aea42afbf1a6014d883a9f46da6ce2638d2b73 Mon Sep 17 00:00:00 2001 From: Erez Freiberger <efreiber@redhat.com> Date: Mon, 20 Nov 2017 13:51:52 +0200 Subject: [PATCH 1/5] serve the status of image aquiring Factoring out the whole image acquiring to a function and abstracting docker client with an interface to ease testing --- pkg/api/types.go | 2 + pkg/imageserver/webdav.go | 25 +-- pkg/inspector/image-inspector.go | 241 ++++++++++++++------------ pkg/inspector/image-inspector_test.go | 43 ++--- 4 files changed, 157 insertions(+), 154 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index cb9e513..9dbeb75 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -93,6 +93,8 @@ var ( // InspectorMetadata is the metadata type with information about image-inspector's operation type InspectorMetadata struct { + ImageAcquireError string // error from aquiring the image + docker.Image // Metadata about the inspected image // OpenSCAP describes the state of the OpenSCAP scan diff --git a/pkg/imageserver/webdav.go b/pkg/imageserver/webdav.go index 01bfbdc..6cf1420 100644 --- a/pkg/imageserver/webdav.go +++ b/pkg/imageserver/webdav.go @@ -65,18 +65,21 @@ func (s *webdavImageServer) GetHandler(meta *iiapi.InspectorMetadata, ) (http.Handler, error) { mux := http.NewServeMux() servePath := ImageServeURL - if s.opts.Chroot { - if err := syscall.Chroot(ImageServeURL); err != nil { - return nil, fmt.Errorf("Unable to chroot into %s: %v\n", ImageServeURL, err) - } - if err := syscall.Chdir("/"); err != nil { - return nil, fmt.Errorf("Unable to change directory into new root: %v\n", err) + + if len(servePath) > 0 && len(meta.ImageAcquireError) == 0 { + if s.opts.Chroot { + if err := syscall.Chroot(ImageServeURL); err != nil { + return nil, fmt.Errorf("unable to chroot into %s: %v\n", ImageServeURL, err) + } + if err := syscall.Chdir("/"); err != nil { + return nil, fmt.Errorf("unable to change directory into new root: %v\n", err) + } + servePath = chrootServePath + } else { + log.Printf("!!!WARNING!!! It is insecure to serve the image content without changing") + log.Printf("root (--chroot). Absolute-path symlinks in the image can lead to disclose") + log.Printf("information of the hosting system.") } - servePath = chrootServePath - } else { - log.Printf("!!!WARNING!!! It is insecure to serve the image content without changing") - log.Printf("root (--chroot). Absolute-path symlinks in the image can lead to disclose") - log.Printf("information of the hosting system.") } mux.HandleFunc(s.opts.HealthzURL, func(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/inspector/image-inspector.go b/pkg/inspector/image-inspector.go index 0fa8eaa..09717a5 100644 --- a/pkg/inspector/image-inspector.go +++ b/pkg/inspector/image-inspector.go @@ -110,6 +110,16 @@ func NewDefaultImageInspector(opts iicmd.ImageInspectorOptions) ImageInspector { return inspector } +type DockerRuntimeClient interface { + InspectImage(name string) (*docker.Image, error) + ContainerChanges(id string) ([]docker.Change, error) + PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error + CreateContainer(opts docker.CreateContainerOptions) (*docker.Container, error) + RemoveContainer(opts docker.RemoveContainerOptions) error + InspectContainer(id string) (*docker.Container, error) + DownloadFromContainer(id string, opts docker.DownloadFromContainerOptions) error +} + // Inspect inspects and serves the image based on the ImageInspectorOptions. func (i *defaultImageInspector) Inspect() error { var ( @@ -126,126 +136,60 @@ func (i *defaultImageInspector) Inspect() error { Results: []iiapi.Result{}, } + ctx := context.Background() + client, err := docker.NewClient(i.opts.URI) + if err == nil { + err, i.meta.Image, scanResults.ImageID, scanResults.ContainerID, filterFn = i.acquireImage(client) + } if err != nil { - return fmt.Errorf("Unable to connect to docker daemon: %v\n", err) + i.meta.ImageAcquireError = err.Error() } - ctx := context.Background() - - if len(i.opts.Container) == 0 { - imageMetaBefore, inspectErrBefore := client.InspectImage(i.opts.Image) - if i.opts.PullPolicy == iiapi.PullNever && inspectErrBefore != nil { - return fmt.Errorf("Image %s is not available and pull-policy %s doesn't allow pulling", - i.opts.Image, i.opts.PullPolicy) - } - - if i.opts.PullPolicy == iiapi.PullAlways || - (i.opts.PullPolicy == iiapi.PullIfNotPresent && inspectErrBefore != nil) { - if err = i.pullImage(client); err != nil { + if err == nil { + switch i.opts.ScanType { + case "openscap": + if i.opts.ScanResultsDir, err = createOutputDir(i.opts.ScanResultsDir, "image-inspector-scan-results-"); err != nil { return err } - } - - imageMetaAfter, inspectErrAfter := client.InspectImage(i.opts.Image) - if inspectErrBefore == nil && inspectErrAfter == nil && - imageMetaBefore.ID == imageMetaAfter.ID { - log.Printf("Image %s was already available", i.opts.Image) - } - - randomName, err := generateRandomName() - if err != nil { - return err - } - - imageMetadata, err := i.createAndExtractImage(client, randomName) - if err != nil { - return err - } - i.meta.Image = *imageMetadata - scanResults.ImageID = i.meta.Image.ID - } else { - meta, err := i.getContainerMeta(client) - if err != nil { - return err - } - - i.meta.Image = *meta.Image - scanResults.ImageID = meta.Image.ID - scanResults.ContainerID = meta.Container.ID - - var filterInclude map[string]struct{} - - if i.opts.ScanContainerChanges { - filterInclude, err = i.getContainerChanges(client, meta) - } - - i.opts.DstPath = fmt.Sprintf("/proc/%d/root/", meta.Container.State.Pid) - - excludePrefixes := []string{ - i.opts.DstPath + "proc", - i.opts.DstPath + "sys", - } - - filterFn = func(path string, fileInfo os.FileInfo) bool { - if filterInclude != nil { - if _, ok := filterInclude[path]; !ok { - return false - } + var ( + results []iiapi.Result + reportObj interface{} + ) + scanner = openscap.NewDefaultScanner(OSCAP_CVE_DIR, i.opts.ScanResultsDir, i.opts.CVEUrlPath, i.opts.OpenScapHTML) + results, reportObj, err = scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) + if err != nil { + i.meta.OpenSCAP.SetError(err) + log.Printf("DEBUG: Unable to scan image %q with OpenSCAP: %v", i.opts.Image, err) + } else { + i.meta.OpenSCAP.Status = iiapi.StatusSuccess + report := reportObj.(openscap.OpenSCAPReport) + scanReport = report.ArfBytes + htmlScanReport = report.HTMLBytes + scanResults.Results = append(scanResults.Results, results...) } - for _, prefix := range excludePrefixes { - if strings.HasPrefix(path, prefix) { - return false - } + case "clamav": + scanner, err = clamav.NewScanner(i.opts.ClamSocket) + if err != nil { + return fmt.Errorf("failed to initialize clamav scanner: %v", err) + } + results, _, err := scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) + if err != nil { + log.Printf("DEBUG: Unable to scan image %q with ClamAV: %v", i.opts.Image, err) + return err } - - return true - } - } - - switch i.opts.ScanType { - case "openscap": - if i.opts.ScanResultsDir, err = createOutputDir(i.opts.ScanResultsDir, "image-inspector-scan-results-"); err != nil { - return err - } - var ( - results []iiapi.Result - reportObj interface{} - ) - scanner = openscap.NewDefaultScanner(OSCAP_CVE_DIR, i.opts.ScanResultsDir, i.opts.CVEUrlPath, i.opts.OpenScapHTML) - results, reportObj, err = scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) - if err != nil { - i.meta.OpenSCAP.SetError(err) - log.Printf("DEBUG: Unable to scan image %q with OpenSCAP: %v", i.opts.Image, err) - } else { - i.meta.OpenSCAP.Status = iiapi.StatusSuccess - report := reportObj.(openscap.OpenSCAPReport) - scanReport = report.ArfBytes - htmlScanReport = report.HTMLBytes scanResults.Results = append(scanResults.Results, results...) - } - case "clamav": - scanner, err = clamav.NewScanner(i.opts.ClamSocket) - if err != nil { - return fmt.Errorf("failed to initialize clamav scanner: %v", err) - } - results, _, err := scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) - if err != nil { - log.Printf("DEBUG: Unable to scan image %q with ClamAV: %v", i.opts.Image, err) - return err + default: + return fmt.Errorf("unsupported scan type: %s", i.opts.ScanType) } - scanResults.Results = append(scanResults.Results, results...) - default: - return fmt.Errorf("unsupported scan type: %s", i.opts.ScanType) - } - - if len(i.opts.PostResultURL) > 0 { - if err := i.postResults(scanResults); err != nil { - log.Printf("Error posting results: %v", err) - return nil + if len(i.opts.PostResultURL) > 0 { + if err := i.postResults(scanResults); err != nil { + log.Printf("Error posting results: %v", err) + return nil + } } } @@ -361,7 +305,7 @@ func decodeDockerResponse(parsedErrors chan error, reader io.Reader) { } } -func (i *defaultImageInspector) getContainerMeta(client *docker.Client) (*containerMeta, error) { +func (i *defaultImageInspector) getContainerMeta(client DockerRuntimeClient) (*containerMeta, error) { var err error result := &containerMeta{} @@ -372,13 +316,13 @@ func (i *defaultImageInspector) getContainerMeta(client *docker.Client) (*contai result.Image, err = client.InspectImage(result.Container.Image) if err != nil { - return nil,fmt.Errorf("Unable to get docker image information: %v", err) + return nil, fmt.Errorf("Unable to get docker image information: %v", err) } return result, nil } -func (i *defaultImageInspector) getContainerChanges(client *docker.Client, meta *containerMeta) (map[string]struct{}, error) { +func (i *defaultImageInspector) getContainerChanges(client DockerRuntimeClient, meta *containerMeta) (map[string]struct{}, error) { rootPath := i.opts.DstPath containerChanges, err := client.ContainerChanges(i.opts.Container) @@ -402,7 +346,7 @@ func (i *defaultImageInspector) getContainerChanges(client *docker.Client, meta // pullImage pulls the inspected image using the given client. // It will try to use all the given authentication methods and will fail // only if all of them failed. -func (i *defaultImageInspector) pullImage(client *docker.Client) error { +func (i *defaultImageInspector) pullImage(client DockerRuntimeClient) error { log.Printf("Pulling image %s", i.opts.Image) var imagePullAuths *docker.AuthConfigurations @@ -447,7 +391,7 @@ func (i *defaultImageInspector) pullImage(client *docker.Client) error { // option's destination path. If the destination path is empty it will write to a temp directory // and update the option's destination path with a /var/tmp directory. /var/tmp is used to // try and ensure it is a non-in-memory tmpfs. -func (i *defaultImageInspector) createAndExtractImage(client *docker.Client, containerName string) (*docker.Image, error) { +func (i *defaultImageInspector) createAndExtractImage(client DockerRuntimeClient, containerName string) (*docker.Image, error) { container, err := client.CreateContainer(docker.CreateContainerOptions{ Name: containerName, Config: &docker.Config{ @@ -649,3 +593,76 @@ func createOutputDir(dirName string, tempName string) (string, error) { } return dirName, nil } + +// acquireImage returns error, the docker image that was acquired, the image ID, +// the container ID that the image was acquired from, and iiapi.FilesFilter for this image. +func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, docker.Image, string, string, iiapi.FilesFilter) { + var err error + if len(i.opts.Container) == 0 { + imageMetaBefore, inspectErrBefore := client.InspectImage(i.opts.Image) + if i.opts.PullPolicy == iiapi.PullNever && inspectErrBefore != nil { + return fmt.Errorf("Image %s is not available and pull-policy %s doesn't allow pulling", + i.opts.Image, i.opts.PullPolicy), docker.Image{}, "", "", nil + } + + if i.opts.PullPolicy == iiapi.PullAlways || + (i.opts.PullPolicy == iiapi.PullIfNotPresent && inspectErrBefore != nil) { + if err = i.pullImage(client); err != nil { + return err, docker.Image{}, "", "", nil + } + } + + imageMetaAfter, inspectErrAfter := client.InspectImage(i.opts.Image) + if inspectErrBefore == nil && inspectErrAfter == nil && + imageMetaBefore.ID == imageMetaAfter.ID { + log.Printf("Image %s was already available", i.opts.Image) + } + + randomName, err := generateRandomName() + if err != nil { + return err, docker.Image{}, "", "", nil + } + + imageMetadata, err := i.createAndExtractImage(client, randomName) + if err != nil { + return err, docker.Image{}, "", "", nil + } + + return nil, *imageMetadata, imageMetadata.ID, "", nil + } else { + meta, err := i.getContainerMeta(client) + if err != nil { + return err, docker.Image{}, "", "", nil + } + + var filterInclude map[string]struct{} + + if i.opts.ScanContainerChanges { + filterInclude, err = i.getContainerChanges(client, meta) + } + + i.opts.DstPath = fmt.Sprintf("/proc/%d/root/", meta.Container.State.Pid) + + excludePrefixes := []string{ + i.opts.DstPath + "proc", + i.opts.DstPath + "sys", + } + + filterFn := func(path string, fileInfo os.FileInfo) bool { + if filterInclude != nil { + if _, ok := filterInclude[path]; !ok { + return false + } + } + + for _, prefix := range excludePrefixes { + if strings.HasPrefix(path, prefix) { + return false + } + } + + return true + } + return nil, *meta.Image, meta.Image.ID, meta.Container.ID, filterFn + } +} diff --git a/pkg/inspector/image-inspector_test.go b/pkg/inspector/image-inspector_test.go index 55954ef..5597d2f 100644 --- a/pkg/inspector/image-inspector_test.go +++ b/pkg/inspector/image-inspector_test.go @@ -1,7 +1,6 @@ package inspector import ( - "context" "fmt" "io" "io/ioutil" @@ -13,39 +12,18 @@ import ( iicmd "github.com/openshift/image-inspector/pkg/cmd" ) -type FailMockScanner struct{} -type SuccMockScanner struct { - FailMockScanner -} -type NoResMockScanner struct { - SuccMockScanner -} -type SuccWithReportMockScanner struct { - SuccMockScanner -} - -func (ms *FailMockScanner) Scan(context.Context, string, *docker.Image, iiapi.FilesFilter) ([]iiapi.Result, interface{}, error) { - return nil, nil, fmt.Errorf("FAIL SCANNER!") -} -func (ms *FailMockScanner) Name() string { - return "MockScanner" -} -func (ms *SuccMockScanner) Scan(context.Context, string, *docker.Image, iiapi.FilesFilter) ([]iiapi.Result, interface{}, error) { - return []iiapi.Result{}, nil, nil -} - -func TestScanImage(t *testing.T) { - ctx := context.Background() +func TestAcquiringInInspect(t *testing.T) { for k, v := range map[string]struct { - ii defaultImageInspector - s iiapi.Scanner - shouldFail bool + ii defaultImageInspector + shouldFail bool + expectedAcqErr string }{ - "Scanner fails on scan": {ii: defaultImageInspector{}, s: &FailMockScanner{}, shouldFail: true}, - "Happy Flow": {ii: defaultImageInspector{}, s: &SuccMockScanner{}, shouldFail: false}, + "Scanner fails on scan": {ii: defaultImageInspector{opts: iicmd.ImageInspectorOptions{URI: "No such file", Serve: ""}}, + shouldFail: false, + expectedAcqErr: "invalid endpoint", + }, } { - v.ii.opts.DstPath = "here" - _, _, err := v.s.Scan(ctx, v.ii.opts.DstPath, nil, nil) + err := v.ii.Inspect() if v.shouldFail && err == nil { t.Errorf("%s should have failed but it didn't!", k) } @@ -54,6 +32,9 @@ func TestScanImage(t *testing.T) { t.Errorf("%s should have succeeded but failed with %v", k, err) } } + if v.ii.meta.ImageAcquireError != v.expectedAcqErr { + t.Errorf("%s acquire error is not matching.\nExtected: %v\nReceived: %v\n", k, v.expectedAcqErr, v.ii.meta.ImageAcquireError) + } } } From c842dd0743800e519f1ac0c2eb2bca3810647d9b Mon Sep 17 00:00:00 2001 From: Erez Freiberger <efreiber@redhat.com> Date: Sun, 14 Jan 2018 17:07:52 +0200 Subject: [PATCH 2/5] add finish channel to decodeDockerResponse to ensure it finishes --- pkg/inspector/image-inspector.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/inspector/image-inspector.go b/pkg/inspector/image-inspector.go index 09717a5..ad21f43 100644 --- a/pkg/inspector/image-inspector.go +++ b/pkg/inspector/image-inspector.go @@ -257,8 +257,8 @@ func aggregateBytesAndReport(bytesChan chan int) { // from reader. It will start aggregateBytesAndReport with bytesChan // and will push the difference of bytes downloaded to bytesChan. // Errors encountered during parsing are reported to parsedErrors channel. -// After reader is closed it will send nil on parsedErrors, close bytesChan and exit. -func decodeDockerResponse(parsedErrors chan error, reader io.Reader) { +// After reader is closed it will send nil on parsedErrors, close bytesChan and send true on finished. +func decodeDockerResponse(parsedErrors chan error, reader io.Reader, finished chan bool) { type progressDetailType struct { Current, Total int } @@ -303,6 +303,8 @@ func decodeDockerResponse(parsedErrors chan error, reader io.Reader) { bytesChan <- (bytes - last) } } + + finished <- true } func (i *defaultImageInspector) getContainerMeta(client DockerRuntimeClient) (*containerMeta, error) { @@ -359,7 +361,13 @@ func (i *defaultImageInspector) pullImage(client DockerRuntimeClient) error { var err error for name, auth := range imagePullAuths.Configs { parsedErrors := make(chan error, 100) - defer func() { close(parsedErrors) }() + finished := make(chan bool, 1) + + defer func() { + <-finished + close(finished) + close(parsedErrors) + }() go func() { reader, writer := io.Pipe() @@ -370,7 +378,7 @@ func (i *defaultImageInspector) pullImage(client DockerRuntimeClient) error { OutputStream: writer, RawJSONStream: true, } - go decodeDockerResponse(parsedErrors, reader) + go decodeDockerResponse(parsedErrors, reader, finished) if err = client.PullImage(imagePullOption, auth); err != nil { parsedErrors <- err @@ -378,7 +386,7 @@ func (i *defaultImageInspector) pullImage(client DockerRuntimeClient) error { }() if parsedError := <-parsedErrors; parsedError != nil { - log.Printf("Authentication with %s failed: %v", name, parsedError) + log.Printf("Pulling image with authentication %s failed: %v", name, parsedError) } else { return nil } From d7cfc31eb5b74a310428c21e21211778a2910d41 Mon Sep 17 00:00:00 2001 From: Erez Freiberger <efreiber@redhat.com> Date: Sun, 14 Jan 2018 17:49:56 +0200 Subject: [PATCH 3/5] adding test for acquireImage and pullImage also checking return value of getContainerChanges --- pkg/inspector/image-inspector.go | 5 +- pkg/inspector/image-inspector_test.go | 161 +++++++++++++++++++++++++- 2 files changed, 163 insertions(+), 3 deletions(-) diff --git a/pkg/inspector/image-inspector.go b/pkg/inspector/image-inspector.go index ad21f43..1df8408 100644 --- a/pkg/inspector/image-inspector.go +++ b/pkg/inspector/image-inspector.go @@ -391,7 +391,7 @@ func (i *defaultImageInspector) pullImage(client DockerRuntimeClient) error { return nil } } - return fmt.Errorf("Unable to pull docker image: %v\n", err) + return fmt.Errorf("Unable to pull docker image: %v", err) } // createAndExtractImage creates a docker container based on the option's image with containerName. @@ -647,6 +647,9 @@ func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, if i.opts.ScanContainerChanges { filterInclude, err = i.getContainerChanges(client, meta) + if err != nil { + return err, docker.Image{}, "", "", nil + } } i.opts.DstPath = fmt.Sprintf("/proc/%d/root/", meta.Container.State.Pid) diff --git a/pkg/inspector/image-inspector_test.go b/pkg/inspector/image-inspector_test.go index 5597d2f..f87f9ba 100644 --- a/pkg/inspector/image-inspector_test.go +++ b/pkg/inspector/image-inspector_test.go @@ -115,14 +115,19 @@ func Test_decodeDockerResponse(t *testing.T) { for test_name, test_params := range tests { parsedErrors := make(chan error, 100) - defer func() { close(parsedErrors) }() + finished := make(chan bool, 1) + defer func() { + <-finished // wait for decodeDockerResponse to finish + close(finished) + close(parsedErrors) + }() go func() { reader, writer := io.Pipe() // handle closing the reader/writer in the method that creates them defer reader.Close() defer writer.Close() - go decodeDockerResponse(parsedErrors, reader) + go decodeDockerResponse(parsedErrors, reader, finished) writer.Write([]byte(test_params.readerInput)) }() @@ -194,3 +199,155 @@ func TestCreateOutputDir(t *testing.T) { } } } + +type mockDockerRuntimeClient struct{} + +func (c mockDockerRuntimeClient) InspectImage(name string) (*docker.Image, error) { + return nil, fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) ContainerChanges(id string) ([]docker.Change, error) { + return nil, fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error { + return fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) CreateContainer(opts docker.CreateContainerOptions) (*docker.Container, error) { + return nil, fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) RemoveContainer(opts docker.RemoveContainerOptions) error { + return fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) InspectContainer(id string) (*docker.Container, error) { + return nil, fmt.Errorf("mockDockerRuntimeClient FAIL") +} +func (c mockDockerRuntimeClient) DownloadFromContainer(id string, opts docker.DownloadFromContainerOptions) error { + return fmt.Errorf("mockDockerRuntimeClient FAIL") +} + +type mockRuntimeClientPullSuccess struct { + mockDockerRuntimeClient +} + +func (c mockRuntimeClientPullSuccess) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error { + return nil +} + +type mockRuntimeClientInspectSuccess struct { + mockDockerRuntimeClient +} + +func (c mockRuntimeClientInspectSuccess) InspectImage(name string) (*docker.Image, error) { + return &docker.Image{}, nil +} + +type mockDockerRuntimeClientAllSuccess struct{} + +func (c mockDockerRuntimeClientAllSuccess) InspectImage(name string) (*docker.Image, error) { + return &docker.Image{}, nil +} +func (c mockDockerRuntimeClientAllSuccess) ContainerChanges(id string) ([]docker.Change, error) { + return []docker.Change{}, nil +} +func (c mockDockerRuntimeClientAllSuccess) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error { + return nil +} +func (c mockDockerRuntimeClientAllSuccess) CreateContainer(opts docker.CreateContainerOptions) (*docker.Container, error) { + return &docker.Container{}, nil +} +func (c mockDockerRuntimeClientAllSuccess) RemoveContainer(opts docker.RemoveContainerOptions) error { + return nil +} +func (c mockDockerRuntimeClientAllSuccess) InspectContainer(id string) (*docker.Container, error) { + return &docker.Container{}, nil +} +func (c mockDockerRuntimeClientAllSuccess) DownloadFromContainer(id string, opts docker.DownloadFromContainerOptions) error { + return nil +} + +type mockRuntimeClientAllSuccessButContainerChanges struct { + mockDockerRuntimeClientAllSuccess +} + +func (c mockRuntimeClientAllSuccessButContainerChanges) ContainerChanges(id string) ([]docker.Change, error) { + return []docker.Change{}, fmt.Errorf("mockDockerRuntimeClient FAIL") +} + +func TestPullImage(t *testing.T) { + for k, v := range map[string]struct { + client DockerRuntimeClient + shouldFail bool + expectedErr string + }{ + "With instant pull failing client": {shouldFail: true, + client: mockDockerRuntimeClient{}, + expectedErr: "Unable to pull docker image: mockDockerRuntimeClient FAIL"}, + "With instant pull success client": {shouldFail: false, + client: mockRuntimeClientPullSuccess{}}, + } { + ii := &defaultImageInspector{iicmd.ImageInspectorOptions{Image: "NoSuchImage!"}, iiapi.InspectorMetadata{}, nil} + err := ii.pullImage(v.client) + if v.shouldFail { + if err == nil { + t.Errorf("%s should have failed but it didn't", k) + } else { + if err.Error() != v.expectedErr { + t.Errorf("Wrong error message for %s.\nExpected: %s\nReceived: %s\n", k, v.expectedErr, err.Error()) + } + } + } else { + if err != nil { + t.Errorf("%s should not have failed with: %s", k, err.Error()) + } + } + } +} + +func TestAcquireImage(t *testing.T) { + noContainerPullNever := iicmd.ImageInspectorOptions{Image: "noSuchImage", Container: "", PullPolicy: iiapi.PullNever} + noContainerPullAlways := iicmd.ImageInspectorOptions{Image: "noSuchImage", Container: "", PullPolicy: iiapi.PullAlways} + noContainerPullNotPresent := iicmd.ImageInspectorOptions{Image: "noSuchImage", Container: "", PullPolicy: iiapi.PullIfNotPresent} + + fromContainer := iicmd.ImageInspectorOptions{Container: "I am a container", ScanContainerChanges: true} + + for k, v := range map[string]struct { + opts iicmd.ImageInspectorOptions + client DockerRuntimeClient + shouldFail bool + expectedErr string + }{ + "When unable to inspect image and also never pull": {opts: noContainerPullNever, shouldFail: true, + client: mockDockerRuntimeClient{}, + expectedErr: fmt.Sprintf("Image %s is not available and pull-policy %s doesn't allow pulling", + noContainerPullNever.Image, noContainerPullNever.PullPolicy)}, + "When unable to inspect or pull image and also always pull": {opts: noContainerPullAlways, shouldFail: true, + client: mockDockerRuntimeClient{}, + expectedErr: "Unable to pull docker image: mockDockerRuntimeClient FAIL"}, + "When unable to inspect or pull image and also pull if no present": {opts: noContainerPullNotPresent, shouldFail: true, + client: mockDockerRuntimeClient{}, + expectedErr: "Unable to pull docker image: mockDockerRuntimeClient FAIL"}, + "Unable to inspect running container": {opts: fromContainer, shouldFail: true, + client: mockDockerRuntimeClient{}, + expectedErr: "Unable to get docker container information: mockDockerRuntimeClient FAIL"}, + "Cannot get container changes": {opts: fromContainer, shouldFail: true, + client: mockRuntimeClientAllSuccessButContainerChanges{}, + expectedErr: "Unable to get docker container changes: mockDockerRuntimeClient FAIL"}, + "Success with running Container": {opts: fromContainer, shouldFail: false, + client: mockDockerRuntimeClientAllSuccess{}}, + } { + ii := &defaultImageInspector{v.opts, iiapi.InspectorMetadata{}, nil} + err, _, _, _, _ := ii.acquireImage(v.client) + if v.shouldFail { + if err == nil { + t.Errorf("%s should have failed but it didn't", k) + } else { + if err.Error() != v.expectedErr { + t.Errorf("Wrong error message for %s.\nExpected: %s\nReceived: %s\n", k, v.expectedErr, err.Error()) + } + } + } else { + if err != nil { + t.Errorf("%s should not have failed with: %s", k, err.Error()) + } + } + } +} From 18075e4692aa25b17e16481de31acf1450110d4b Mon Sep 17 00:00:00 2001 From: Erez Freiberger <efreiber@redhat.com> Date: Mon, 15 Jan 2018 18:24:21 +0200 Subject: [PATCH 4/5] removing unnecessary variables from acquireImage function --- pkg/inspector/image-inspector.go | 30 +++++++++++++++------------ pkg/inspector/image-inspector_test.go | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/inspector/image-inspector.go b/pkg/inspector/image-inspector.go index 1df8408..0976102 100644 --- a/pkg/inspector/image-inspector.go +++ b/pkg/inspector/image-inspector.go @@ -139,11 +139,13 @@ func (i *defaultImageInspector) Inspect() error { ctx := context.Background() client, err := docker.NewClient(i.opts.URI) - if err == nil { - err, i.meta.Image, scanResults.ImageID, scanResults.ContainerID, filterFn = i.acquireImage(client) - } if err != nil { i.meta.ImageAcquireError = err.Error() + } else { + err, scanResults.ContainerID, filterFn = i.acquireImage(client) + if err != nil { + i.meta.ImageAcquireError = err.Error() + } } if err == nil { @@ -602,21 +604,21 @@ func createOutputDir(dirName string, tempName string) (string, error) { return dirName, nil } -// acquireImage returns error, the docker image that was acquired, the image ID, +// acquireImage returns error, // the container ID that the image was acquired from, and iiapi.FilesFilter for this image. -func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, docker.Image, string, string, iiapi.FilesFilter) { +func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, string, iiapi.FilesFilter) { var err error if len(i.opts.Container) == 0 { imageMetaBefore, inspectErrBefore := client.InspectImage(i.opts.Image) if i.opts.PullPolicy == iiapi.PullNever && inspectErrBefore != nil { return fmt.Errorf("Image %s is not available and pull-policy %s doesn't allow pulling", - i.opts.Image, i.opts.PullPolicy), docker.Image{}, "", "", nil + i.opts.Image, i.opts.PullPolicy), "", nil } if i.opts.PullPolicy == iiapi.PullAlways || (i.opts.PullPolicy == iiapi.PullIfNotPresent && inspectErrBefore != nil) { if err = i.pullImage(client); err != nil { - return err, docker.Image{}, "", "", nil + return err, "", nil } } @@ -628,27 +630,29 @@ func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, randomName, err := generateRandomName() if err != nil { - return err, docker.Image{}, "", "", nil + return err, "", nil } imageMetadata, err := i.createAndExtractImage(client, randomName) if err != nil { - return err, docker.Image{}, "", "", nil + return err, "", nil } + i.meta.Image = *imageMetadata - return nil, *imageMetadata, imageMetadata.ID, "", nil + return nil, "", nil } else { meta, err := i.getContainerMeta(client) if err != nil { - return err, docker.Image{}, "", "", nil + return err, "", nil } + i.meta.Image = *meta.Image var filterInclude map[string]struct{} if i.opts.ScanContainerChanges { filterInclude, err = i.getContainerChanges(client, meta) if err != nil { - return err, docker.Image{}, "", "", nil + return err, "", nil } } @@ -674,6 +678,6 @@ func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, return true } - return nil, *meta.Image, meta.Image.ID, meta.Container.ID, filterFn + return nil, meta.Container.ID, filterFn } } diff --git a/pkg/inspector/image-inspector_test.go b/pkg/inspector/image-inspector_test.go index f87f9ba..c8b607d 100644 --- a/pkg/inspector/image-inspector_test.go +++ b/pkg/inspector/image-inspector_test.go @@ -335,7 +335,7 @@ func TestAcquireImage(t *testing.T) { client: mockDockerRuntimeClientAllSuccess{}}, } { ii := &defaultImageInspector{v.opts, iiapi.InspectorMetadata{}, nil} - err, _, _, _, _ := ii.acquireImage(v.client) + err, _, _ := ii.acquireImage(v.client) if v.shouldFail { if err == nil { t.Errorf("%s should have failed but it didn't", k) From 710bead15d545cec178ffa4e94e2b8e780f2b561 Mon Sep 17 00:00:00 2001 From: Erez Freiberger <efreiber@redhat.com> Date: Mon, 15 Jan 2018 18:49:07 +0200 Subject: [PATCH 5/5] factoring out the serving from Inspect function --- pkg/inspector/image-inspector.go | 147 +++++++++++++++----------- pkg/inspector/image-inspector_test.go | 10 +- 2 files changed, 88 insertions(+), 69 deletions(-) diff --git a/pkg/inspector/image-inspector.go b/pkg/inspector/image-inspector.go index 0976102..b877b06 100644 --- a/pkg/inspector/image-inspector.go +++ b/pkg/inspector/image-inspector.go @@ -60,12 +60,21 @@ type ImageInspector interface { Inspect() error } +// scanOutputs is a struct to hold all the scan outputs that needs to be served +type scanOutputs struct { + ScanReport []byte + HtmlScanReport []byte + ScanResults iiapi.ScanResult +} + // defaultImageInspector is the default implementation of ImageInspector. type defaultImageInspector struct { opts iicmd.ImageInspectorOptions meta iiapi.InspectorMetadata // an optional image server that will server content for inspection. imageServer apiserver.ImageServer + + scanOutputs scanOutputs } // NewInspectorMetadata returns a new InspectorMetadata out of *docker.Image @@ -107,6 +116,13 @@ func NewDefaultImageInspector(opts iicmd.ImageInspectorOptions) ImageInspector { } inspector.imageServer = apiserver.NewWebdavImageServer(imageServerOpts) } + + inspector.scanOutputs.ScanResults = iiapi.ScanResult{ + APIVersion: iiapi.DefaultResultsAPIVersion, + ImageName: inspector.opts.Image, + Results: []iiapi.Result{}, + } + return inspector } @@ -122,81 +138,84 @@ type DockerRuntimeClient interface { // Inspect inspects and serves the image based on the ImageInspectorOptions. func (i *defaultImageInspector) Inspect() error { + err := i.acquireAndScan() + if err != nil { + i.meta.ImageAcquireError = err.Error() + } + + if i.imageServer != nil { + return i.imageServer.ServeImage(&i.meta, i.opts.DstPath, i.scanOutputs.ScanResults, i.scanOutputs.ScanReport, i.scanOutputs.HtmlScanReport) + } + + return err +} + +// AcquireAndScan acquires and scans the image based on the ImageInspectorOptions. +func (i *defaultImageInspector) acquireAndScan() error { var ( scanner iiapi.Scanner err error - scanReport, htmlScanReport []byte - filterFn iiapi.FilesFilter + filterFn iiapi.FilesFilter ) - scanResults := iiapi.ScanResult{ - APIVersion: iiapi.DefaultResultsAPIVersion, - ImageName: i.opts.Image, - Results: []iiapi.Result{}, - } - ctx := context.Background() client, err := docker.NewClient(i.opts.URI) if err != nil { i.meta.ImageAcquireError = err.Error() + return err } else { - err, scanResults.ContainerID, filterFn = i.acquireImage(client) + err, filterFn = i.acquireImage(client) if err != nil { i.meta.ImageAcquireError = err.Error() + return err } } - if err == nil { - switch i.opts.ScanType { - case "openscap": - if i.opts.ScanResultsDir, err = createOutputDir(i.opts.ScanResultsDir, "image-inspector-scan-results-"); err != nil { - return err - } - var ( - results []iiapi.Result - reportObj interface{} - ) - scanner = openscap.NewDefaultScanner(OSCAP_CVE_DIR, i.opts.ScanResultsDir, i.opts.CVEUrlPath, i.opts.OpenScapHTML) - results, reportObj, err = scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) - if err != nil { - i.meta.OpenSCAP.SetError(err) - log.Printf("DEBUG: Unable to scan image %q with OpenSCAP: %v", i.opts.Image, err) - } else { - i.meta.OpenSCAP.Status = iiapi.StatusSuccess - report := reportObj.(openscap.OpenSCAPReport) - scanReport = report.ArfBytes - htmlScanReport = report.HTMLBytes - scanResults.Results = append(scanResults.Results, results...) - } - - case "clamav": - scanner, err = clamav.NewScanner(i.opts.ClamSocket) - if err != nil { - return fmt.Errorf("failed to initialize clamav scanner: %v", err) - } - results, _, err := scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) - if err != nil { - log.Printf("DEBUG: Unable to scan image %q with ClamAV: %v", i.opts.Image, err) - return err - } - scanResults.Results = append(scanResults.Results, results...) - - default: - return fmt.Errorf("unsupported scan type: %s", i.opts.ScanType) + switch i.opts.ScanType { + case "openscap": + if i.opts.ScanResultsDir, err = createOutputDir(i.opts.ScanResultsDir, "image-inspector-scan-results-"); err != nil { + return err + } + var ( + results []iiapi.Result + reportObj interface{} + ) + scanner = openscap.NewDefaultScanner(OSCAP_CVE_DIR, i.opts.ScanResultsDir, i.opts.CVEUrlPath, i.opts.OpenScapHTML) + results, reportObj, err = scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) + if err != nil { + i.meta.OpenSCAP.SetError(err) + log.Printf("DEBUG: Unable to scan image %q with OpenSCAP: %v", i.opts.Image, err) + } else { + i.meta.OpenSCAP.Status = iiapi.StatusSuccess + report := reportObj.(openscap.OpenSCAPReport) + i.scanOutputs.ScanReport = report.ArfBytes + i.scanOutputs.HtmlScanReport = report.HTMLBytes + i.scanOutputs.ScanResults.Results = append(i.scanOutputs.ScanResults.Results, results...) } - if len(i.opts.PostResultURL) > 0 { - if err := i.postResults(scanResults); err != nil { - log.Printf("Error posting results: %v", err) - return nil - } + case "clamav": + scanner, err = clamav.NewScanner(i.opts.ClamSocket) + if err != nil { + return fmt.Errorf("failed to initialize clamav scanner: %v", err) } + results, _, err := scanner.Scan(ctx, i.opts.DstPath, &i.meta.Image, filterFn) + if err != nil { + log.Printf("DEBUG: Unable to scan image %q with ClamAV: %v", i.opts.Image, err) + return err + } + i.scanOutputs.ScanResults.Results = append(i.scanOutputs.ScanResults.Results, results...) + + default: + return fmt.Errorf("unsupported scan type: %s", i.opts.ScanType) } - if i.imageServer != nil { - return i.imageServer.ServeImage(&i.meta, i.opts.DstPath, scanResults, scanReport, htmlScanReport) + if len(i.opts.PostResultURL) > 0 { + if err := i.postResults(i.scanOutputs.ScanResults); err != nil { + log.Printf("Error posting results: %v", err) + return err + } } return nil @@ -604,21 +623,20 @@ func createOutputDir(dirName string, tempName string) (string, error) { return dirName, nil } -// acquireImage returns error, -// the container ID that the image was acquired from, and iiapi.FilesFilter for this image. -func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, string, iiapi.FilesFilter) { +// acquireImage returns error and iiapi.FilesFilter for this image. +func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, iiapi.FilesFilter) { var err error if len(i.opts.Container) == 0 { imageMetaBefore, inspectErrBefore := client.InspectImage(i.opts.Image) if i.opts.PullPolicy == iiapi.PullNever && inspectErrBefore != nil { return fmt.Errorf("Image %s is not available and pull-policy %s doesn't allow pulling", - i.opts.Image, i.opts.PullPolicy), "", nil + i.opts.Image, i.opts.PullPolicy), nil } if i.opts.PullPolicy == iiapi.PullAlways || (i.opts.PullPolicy == iiapi.PullIfNotPresent && inspectErrBefore != nil) { if err = i.pullImage(client); err != nil { - return err, "", nil + return err, nil } } @@ -630,29 +648,30 @@ func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, randomName, err := generateRandomName() if err != nil { - return err, "", nil + return err, nil } imageMetadata, err := i.createAndExtractImage(client, randomName) if err != nil { - return err, "", nil + return err, nil } i.meta.Image = *imageMetadata - return nil, "", nil + return nil, nil } else { meta, err := i.getContainerMeta(client) if err != nil { - return err, "", nil + return err, nil } i.meta.Image = *meta.Image + i.scanOutputs.ScanResults.ContainerID = meta.Container.ID var filterInclude map[string]struct{} if i.opts.ScanContainerChanges { filterInclude, err = i.getContainerChanges(client, meta) if err != nil { - return err, "", nil + return err, nil } } @@ -678,6 +697,6 @@ func (i *defaultImageInspector) acquireImage(client DockerRuntimeClient) (error, return true } - return nil, meta.Container.ID, filterFn + return nil, filterFn } } diff --git a/pkg/inspector/image-inspector_test.go b/pkg/inspector/image-inspector_test.go index c8b607d..b40a59f 100644 --- a/pkg/inspector/image-inspector_test.go +++ b/pkg/inspector/image-inspector_test.go @@ -19,7 +19,7 @@ func TestAcquiringInInspect(t *testing.T) { expectedAcqErr string }{ "Scanner fails on scan": {ii: defaultImageInspector{opts: iicmd.ImageInspectorOptions{URI: "No such file", Serve: ""}}, - shouldFail: false, + shouldFail: true, expectedAcqErr: "invalid endpoint", }, } { @@ -76,7 +76,7 @@ func TestGetAuthConfigs(t *testing.T) { } for k, v := range tests { - ii := &defaultImageInspector{*v.opts, iiapi.InspectorMetadata{}, nil} + ii := &defaultImageInspector{*v.opts, iiapi.InspectorMetadata{}, nil, scanOutputs{}} auths, err := ii.getAuthConfigs() if !v.shouldFail { if err != nil { @@ -284,7 +284,7 @@ func TestPullImage(t *testing.T) { "With instant pull success client": {shouldFail: false, client: mockRuntimeClientPullSuccess{}}, } { - ii := &defaultImageInspector{iicmd.ImageInspectorOptions{Image: "NoSuchImage!"}, iiapi.InspectorMetadata{}, nil} + ii := &defaultImageInspector{iicmd.ImageInspectorOptions{Image: "NoSuchImage!"}, iiapi.InspectorMetadata{}, nil, scanOutputs{}} err := ii.pullImage(v.client) if v.shouldFail { if err == nil { @@ -334,8 +334,8 @@ func TestAcquireImage(t *testing.T) { "Success with running Container": {opts: fromContainer, shouldFail: false, client: mockDockerRuntimeClientAllSuccess{}}, } { - ii := &defaultImageInspector{v.opts, iiapi.InspectorMetadata{}, nil} - err, _, _ := ii.acquireImage(v.client) + ii := &defaultImageInspector{v.opts, iiapi.InspectorMetadata{}, nil, scanOutputs{}} + err, _ := ii.acquireImage(v.client) if v.shouldFail { if err == nil { t.Errorf("%s should have failed but it didn't", k)