fix: Improve USER instruction processing (#28)

* fix: Improve USER instruction processing

Signed-off-by: Jeff MAURY <[email protected]>

pkg/command/analyzer.go

@@ -11,6 +11,7 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -49,8 +50,10 @@ type Line struct {
 	Start int
 	End   int
 }
+
 type Command interface {
-	Analyze(*parser.Node, utils.Source, Line) []Result
+	Analyze(context.Context, *parser.Node, utils.Source, Line) context.Context
+	PostProcess(ctx context.Context) []Result
 }
 
 var commandHandlers = map[string]Command{
@@ -108,10 +111,12 @@ func AnalyzeImage(image string) []Result {
 			},
 		}
 	}
-	return AnalyzeNodeFromSource(node, utils.Source{
+	ctx := context.Background()
+	suggestions, _ := AnalyzeNodeFromSource(ctx, node, utils.Source{
 		Name: "",
 		Type: utils.Image,
 	})
+	return suggestions
 }
 
 func AnalyzeFile(file *os.File) []Result {
@@ -127,13 +132,16 @@ func AnalyzeFile(file *os.File) []Result {
 		}
 	}
 
-	return AnalyzeNodeFromSource(res.AST, utils.Source{
+	ctx := context.Background()
+
+	suggestions, _ := AnalyzeNodeFromSource(ctx, res.AST, utils.Source{
 		Name: "",
 		Type: utils.Image,
 	})
+	return suggestions
 }
 
-func AnalyzeNodeFromSource(node *parser.Node, source utils.Source) []Result {
+func AnalyzeNodeFromSource(ctx context.Context, node *parser.Node, source utils.Source) ([]Result, context.Context) {
 	suggestions := []Result{}
 	commands := []string{}
 	for _, child := range node.Children {
@@ -154,12 +162,17 @@ func AnalyzeNodeFromSource(node *parser.Node, source utils.Source) []Result {
 					})
 
 				} else {
-					suggestions = append(suggestions, handler.Analyze(n, source, line)...)
+					ctx = handler.Analyze(ctx, n, source, line)
 				}
 			}
 		}
 	}
-	return suggestions
+	for key, _ := range commandHandlers {
+		handler := commandHandlers[key]
+
+		suggestions = append(suggestions, handler.PostProcess(ctx)...)
+	}
+	return suggestions, ctx
 }
 
 func IsCommand(text string, command string) bool {

pkg/command/analyzer_test.go

@@ -22,7 +22,13 @@ func TestCheckNginx(t *testing.T) {
 
 func TestFromScratch(t *testing.T) {
 	errors := AnalyzePath("resources/Containerfile.fromscratch")
-	if len(errors) > 0 {
+	if len(errors) != 1 {
 		t.Error("Image with FROM scratch returns errors")
 	}
 }
+func TestFromNginxWithUser(t *testing.T) {
+	errors := AnalyzePath("resources/Containerfile.fromnginxwithuser")
+	if len(errors) != 1 {
+		t.Error("Image with FROM nginx with USER returns errors")
+	}
+}

pkg/command/expose.go

@@ -11,6 +11,7 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 	"strings"
@@ -22,13 +23,17 @@ import (
 type Expose struct {
 }
 
-func (e Expose) Analyze(node *parser.Node, source utils.Source, line Line) []Result {
-	results := []Result{}
+type exposeResultKeyType struct{}
+
+var exposeResultKey exposeResultKeyType
+
+func (e Expose) Analyze(ctx context.Context, node *parser.Node, source utils.Source, line Line) context.Context {
 	str := node.Value
 	index := strings.IndexByte(node.Value, '/')
 	if index >= 0 {
 		str = node.Value[0:index]
 	}
+	var results []Result
 	port, err := strconv.Atoi(str)
 	if err != nil {
 		results = append(results, Result{
@@ -46,5 +51,13 @@ func (e Expose) Analyze(node *parser.Node, source utils.Source, line Line) []Res
 			Description: fmt.Sprintf(`port %d exposed %s could be wrong. TCP/IP port numbers below 1024 are privileged port numbers`, port, GenerateErrorLocation(source, line)),
 		})
 	}
-	return results
+	return context.WithValue(ctx, exposeResultKey, results)
+}
+
+func (e Expose) PostProcess(ctx context.Context) []Result {
+	result := ctx.Value(exposeResultKey)
+	if result == nil {
+		return nil
+	}
+	return result.([]Result)
 }

pkg/command/from.go

@@ -11,8 +11,8 @@
 package command
 
 import (
+	"context"
 	"fmt"
-
 	"github.com/moby/buildkit/frontend/dockerfile/parser"
 	"github.com/redhat-developer/docker-openshift-analyzer/pkg/decompiler"
 	"github.com/redhat-developer/docker-openshift-analyzer/pkg/utils"
@@ -21,30 +21,39 @@ import (
 type From struct {
 }
 
+type fromResultKeyType struct{}
+
+var fromResultKey fromResultKeyType
+
 const SCRATCH_IMAGE_NAME = "scratch"
 
-func (f From) Analyze(node *parser.Node, source utils.Source, line Line) []Result {
+func (f From) Analyze(ctx context.Context, node *parser.Node, source utils.Source, line Line) context.Context {
 	if node.Value == SCRATCH_IMAGE_NAME {
-		return nil
+		return ctx
 	}
-	results := []Result{}
 	decompiledNode, err := decompiler.Decompile(node.Value)
 	if err != nil {
 		// unable to decompile base image
-		results = append(results, Result{
-			Name:        "Analyze error",
-			Status:      StatusFailed,
-			Severity:    SeverityLow,
-			Description: fmt.Sprintf("unable to analyze the base image %s", node.Value),
+		ctx = context.WithValue(ctx, fromResultKey, []Result{
+			Result{
+				Name:        "Analyze error",
+				Status:      StatusFailed,
+				Severity:    SeverityLow,
+				Description: fmt.Sprintf("unable to analyze the base image %s", node.Value),
+			},
 		})
-		return results
 	}
-	errsFromBaseImage := AnalyzeNodeFromSource(decompiledNode, utils.Source{
+	_, ctx = AnalyzeNodeFromSource(ctx, decompiledNode, utils.Source{
 		Name: node.Value,
 		Type: utils.Parent,
 	})
-	if len(errsFromBaseImage) > 0 {
-		results = append(results, errsFromBaseImage...)
+	return ctx
+}
+
+func (f From) PostProcess(ctx context.Context) []Result {
+	result := ctx.Value(fromResultKey)
+	if result == nil {
+		return nil
 	}
-	return results
+	return result.([]Result)
 }

pkg/command/resources/Containerfile.fromnginxwithuser

@@ -0,0 +1,2 @@
+FROM nginx:1.25.3
+USER nginx

pkg/command/run.go

@@ -11,6 +11,7 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"regexp"
 	"strings"
@@ -21,31 +22,42 @@ import (
 
 type Run struct{}
 
-func (r Run) Analyze(node *parser.Node, source utils.Source, line Line) []Result {
-	results := []Result{}
+type runResultKeyType struct{}
+
+var runResultKey runResultKeyType
+
+func (r Run) Analyze(ctx context.Context, node *parser.Node, source utils.Source, line Line) context.Context {
 
 	// let's split the run command by &&. E.g chmod 070 /app && chmod 070 /app/routes && chmod 070 /app/bin
 	splittedCommands := strings.Split(node.Value, "&&")
+	var results []Result
 	for _, command := range splittedCommands {
 		if r.isChmodCommand(command) {
-			err := r.analyzeChmodCommand(command, source, line)
-			if err != nil {
-				results = append(results, *err)
+			result := r.analyzeChmodCommand(command, source, line)
+			if result != nil {
+				results = append(results, *result)
 			}
 		} else if r.isChownCommand(command) {
-			err := r.analyzeChownCommand(command, source, line)
-			if err != nil {
-				results = append(results, *err)
+			result := r.analyzeChownCommand(command, source, line)
+			if result != nil {
+				results = append(results, *result)
 			}
 		} else if r.isSudoOrSuCommand(command) {
-			err := r.analyzeSudoAndSuCommand(command, source, line)
-			if err != nil {
-				results = append(results, *err)
+			result := r.analyzeSudoAndSuCommand(command, source, line)
+			if result != nil {
+				results = append(results, *result)
 			}
 		}
 	}
+	return context.WithValue(ctx, runResultKey, results)
+}
 
-	return results
+func (r Run) PostProcess(ctx context.Context) []Result {
+	result := ctx.Value(runResultKey)
+	if result == nil {
+		return nil
+	}
+	return result.([]Result)
 }
 
 func (r Run) isSudoOrSuCommand(s string) bool {

test/command/run_test.go → pkg/command/run_test.go

@@ -11,11 +11,11 @@
 package command
 
 import (
+	"context"
 	"strings"
 	"testing"
 
 	"github.com/moby/buildkit/frontend/dockerfile/parser"
-	"github.com/redhat-developer/docker-openshift-analyzer/pkg/command"
 	"github.com/redhat-developer/docker-openshift-analyzer/pkg/utils"
 )
 
@@ -100,19 +100,21 @@ func TestFailChmodCommandWithInvalidPermissionCode(t *testing.T) {
 	}
 }
 
-func verifyParsingCommand(t *testing.T, cmd string, numberExpectedErrors int) []command.Result {
-	run := command.Run{}
-	suggestions := run.Analyze(&parser.Node{
+func verifyParsingCommand(t *testing.T, cmd string, numberExpectedErrors int) []Result {
+	run := Run{}
+	ctx := context.Background()
+	ctx = run.Analyze(ctx, &parser.Node{
 		Value: cmd,
 	},
 		utils.Source{
 			Name: "test",
 			Type: utils.Image,
 		},
-		command.Line{
+		Line{
 			Start: 1,
 			End:   1,
 		})
+	suggestions := run.PostProcess(ctx)
 	if len(suggestions) != numberExpectedErrors {
 		t.Errorf("Expected %d suggestions but they were %d", numberExpectedErrors, len(suggestions))
 	}

pkg/command/user.go

@@ -11,6 +11,7 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"strings"
 
@@ -20,8 +21,14 @@ import (
 
 type User struct{}
 
-func (u User) Analyze(node *parser.Node, source utils.Source, line Line) []Result {
-	results := []Result{}
+type userResultKeyType struct{}
+type userProcessedKeyType struct{}
+
+var userResultKey userResultKeyType
+var userProcessedKey userProcessedKeyType
+
+func (u User) Analyze(ctx context.Context, node *parser.Node, source utils.Source, line Line) context.Context {
+	var results []Result
 	if strings.EqualFold(node.Value, "root") {
 		results = append(results, Result{
 			Name:        "User set to root",
@@ -30,5 +37,25 @@ func (u User) Analyze(node *parser.Node, source utils.Source, line Line) []Resul
 			Description: fmt.Sprintf(`USER directive set to root %s could cause an unexpected behavior. In OpenShift, containers are run using arbitrarily assigned user ID`, GenerateErrorLocation(source, line)),
 		})
 	}
+	ctx = context.WithValue(ctx, userResultKey, results)
+	return context.WithValue(ctx, userProcessedKey, true)
+}
+
+func (u User) PostProcess(ctx context.Context) []Result {
+	processed := ctx.Value(userProcessedKey)
+	res := ctx.Value(userResultKey)
+	var results []Result
+	if res != nil {
+		results = res.([]Result)
+	}
+	if processed == nil {
+		results = append(results, Result{
+			Name:        "User set to root",
+			Status:      StatusFailed,
+			Severity:    SeverityMedium,
+			Description: fmt.Sprintf("USER directive implicitely set to root could cause an unexpected behavior. In OpenShift, containers are run using arbitrarily assigned user ID"),
+		})
+	}
 	return results
+
 }