From 86d49b1b1a6017a5a28994de544648b2a4b006b4 Mon Sep 17 00:00:00 2001 From: Liz Rice Date: Tue, 15 Aug 2017 16:01:27 +0100 Subject: [PATCH 1/4] =?UTF-8?q?We=20don=E2=80=99t=20care=20whether=20the?= =?UTF-8?q?=20binaries=20are=20in=20our=20path=20or=20not,=20just=20whethe?= =?UTF-8?q?r=20they=20are=20running?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/util.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/util.go b/cmd/util.go index 937e3e0..c3d82cb 100644 --- a/cmd/util.go +++ b/cmd/util.go @@ -113,11 +113,6 @@ func verifyBin(binPath ...string) { } } - if len(missing) > 0 { - missing = strings.Trim(missing, ", ") - printlnWarn(fmt.Sprintf("Missing kubernetes binaries: %s", missing)) - } - if len(notRunning) > 0 { notRunning = strings.Trim(notRunning, ", ") printlnWarn(fmt.Sprintf("Kubernetes binaries not running: %s", notRunning)) From 34f8b8e980a438c1db8dbb7c0ec63037f86feed6 Mon Sep 17 00:00:00 2001 From: Liz Rice Date: Tue, 15 Aug 2017 16:44:40 +0100 Subject: [PATCH 2/4] Simplify verifying binaries and config files --- cmd/common.go | 31 ++++++++++++++++++---- cmd/util.go | 67 +++++++++++++++--------------------------------- cmd/util_test.go | 40 ++++++++++++++++++++++++++--- 3 files changed, 82 insertions(+), 56 deletions(-) diff --git a/cmd/common.go b/cmd/common.go index d27dc1f..fcacd8b 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -17,6 +17,7 @@ package cmd import ( "fmt" "io/ioutil" + "os" "strings" "github.com/aquasecurity/kube-bench/check" @@ -150,15 +151,35 @@ func runChecks(t check.NodeType) { // verifyNodeType checks the executables and config files are as expected // for the specified tests (master, node or federated). func verifyNodeType(t check.NodeType) { + var bins []string + var confs []string + switch t { case check.MASTER: - verifyBin(apiserverBin, schedulerBin, controllerManagerBin) - verifyConf(apiserverConf, schedulerConf, controllerManagerConf) + bins = []string{apiserverBin, schedulerBin, controllerManagerBin} + confs = []string{apiserverConf, schedulerConf, controllerManagerConf} case check.NODE: - verifyBin(kubeletBin, proxyBin) - verifyConf(kubeletConf, proxyConf) + bins = []string{kubeletBin, proxyBin} + confs = []string{kubeletConf, proxyConf} case check.FEDERATED: - verifyBin(fedApiserverBin, fedControllerManagerBin) + bins = []string{fedApiserverBin, fedControllerManagerBin} + } + + for _, bin := range bins { + if !verifyBin(bin, ps) { + printlnWarn(fmt.Sprintf("%s is not running", bin)) + } + } + + for _, conf := range confs { + _, err := os.Stat(conf) + if err != nil { + if os.IsNotExist(err) { + printlnWarn(fmt.Sprintf("Missing kubernetes config file: %s", conf)) + } else { + exitWithError(fmt.Errorf("error looking for file %s: %v", conf, err)) + } + } } } diff --git a/cmd/util.go b/cmd/util.go index c3d82cb..4967cbe 100644 --- a/cmd/util.go +++ b/cmd/util.go @@ -64,59 +64,30 @@ func cleanIDs(list string) []string { return ids } -func verifyConf(confPath ...string) { - var missing string - - for _, c := range confPath { - if _, err := os.Stat(c); err != nil && os.IsNotExist(err) { - continueWithError(err, "") - missing += c + ", " - } - } - - if len(missing) > 0 { - missing = strings.Trim(missing, ", ") - printlnWarn(fmt.Sprintf("Missing kubernetes config files: %s", missing)) - } - -} - -func verifyBin(binPath ...string) { - var binSlice []string - var bin string - var missing string - var notRunning string - - // Construct proc name for ps(1) - for _, b := range binPath { - _, err := exec.LookPath(b) - bin = bin + "," + b - binSlice = append(binSlice, b) - if err != nil { - missing += b + ", " - continueWithError(err, "") - } - } - bin = strings.Trim(bin, ",") - - cmd := exec.Command("ps", "-C", bin, "-o", "cmd", "--no-headers") +// ps execs out to the ps command; it's separated into a function so we can write tests +func ps(proc string) string { + cmd := exec.Command("ps", "-C", proc, "-o", "cmd", "--no-headers") out, err := cmd.Output() if err != nil { continueWithError(fmt.Errorf("%s: %s", cmd.Args, err), "") } - for _, b := range binSlice { - matched := strings.Contains(string(out), b) + return string(out) +} - if !matched { - notRunning += b + ", " - } - } +// verifyBin checks that the binary specified is running +func verifyBin(bin string, psFunc func(string) string) bool { - if len(notRunning) > 0 { - notRunning = strings.Trim(notRunning, ", ") - printlnWarn(fmt.Sprintf("Kubernetes binaries not running: %s", notRunning)) - } + // Strip any quotes + bin = strings.Trim(bin, "'\"") + + // bin could consist of more than one word + // We'll search for running processes with the first word, and then check the whole + // proc as supplied is included in the results + proc := strings.Fields(bin)[0] + out := psFunc(proc) + + return strings.Contains(out, bin) } func verifyKubeVersion(major string, minor string) { @@ -133,7 +104,9 @@ func verifyKubeVersion(major string, minor string) { if err != nil { s := fmt.Sprintf("Kubernetes version check skipped with error %v", err) continueWithError(err, sprintlnWarn(s)) - return + if len(out) == 0 { + return + } } msg := checkVersion("Client", string(out), major, minor) diff --git a/cmd/util_test.go b/cmd/util_test.go index 74d955f..fe9e0b9 100644 --- a/cmd/util_test.go +++ b/cmd/util_test.go @@ -16,6 +16,7 @@ package cmd import ( "regexp" + "strconv" "testing" ) @@ -36,8 +37,8 @@ func TestCheckVersion(t *testing.T) { {t: "Server", s: "something unexpected", major: "2", minor: "0", exp: "Couldn't find Server version from kubectl output 'something unexpected'"}, } - for _, c := range cases { - t.Run("", func(t *testing.T) { + for id, c := range cases { + t.Run(strconv.Itoa(id), func(t *testing.T) { m := checkVersion(c.t, c.s, c.major, c.minor) if m != c.exp { t.Fatalf("Got: %s, expected: %s", m, c.exp) @@ -66,8 +67,8 @@ func TestVersionMatch(t *testing.T) { {r: minor}, // Checking that we don't fall over if the string is empty } - for _, c := range cases { - t.Run("", func(t *testing.T) { + for id, c := range cases { + t.Run(strconv.Itoa(id), func(t *testing.T) { m := versionMatch(c.r, c.s) if m != c.exp { t.Fatalf("Got %s expected %s", m, c.exp) @@ -75,3 +76,34 @@ func TestVersionMatch(t *testing.T) { }) } } + +var g string + +func fakeps(proc string) string { + return g +} +func TestVerifyBin(t *testing.T) { + cases := []struct { + proc string + psOut string + exp bool + }{ + {proc: "single", psOut: "single", exp: true}, + {proc: "single", psOut: "", exp: false}, + {proc: "two words", psOut: "two words", exp: true}, + {proc: "two words", psOut: "", exp: false}, + {proc: "cmd", psOut: "cmd param1 param2", exp: true}, + {proc: "cmd param", psOut: "cmd param1 param2", exp: true}, + {proc: "cmd param", psOut: "cmd", exp: false}, + } + + for id, c := range cases { + t.Run(strconv.Itoa(id), func(t *testing.T) { + g = c.psOut + v := verifyBin(c.proc, fakeps) + if v != c.exp { + t.Fatalf("Expected %v got %v", c.exp, v) + } + }) + } +} From 6b9f117f879f06d6d2b99f9e64ea897264623159 Mon Sep 17 00:00:00 2001 From: Liz Rice Date: Tue, 15 Aug 2017 17:00:35 +0100 Subject: [PATCH 3/4] Allow for multiple words in executable names --- cmd/common.go | 41 ++++++++++++++++++++--------------------- cmd/util.go | 9 +++++++++ cmd/util_test.go | 22 ++++++++++++++++++++++ 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/cmd/common.go b/cmd/common.go index fcacd8b..89f45bc 100644 --- a/cmd/common.go +++ b/cmd/common.go @@ -18,7 +18,6 @@ import ( "fmt" "io/ioutil" "os" - "strings" "github.com/aquasecurity/kube-bench/check" "github.com/spf13/viper" @@ -97,26 +96,26 @@ func runChecks(t check.NodeType) { } // Variable substitutions. Replace all occurrences of variables in controls files. - s := strings.Replace(string(in), "$apiserverbin", apiserverBin, -1) - s = strings.Replace(s, "$apiserverconf", apiserverConf, -1) - s = strings.Replace(s, "$schedulerbin", schedulerBin, -1) - s = strings.Replace(s, "$schedulerconf", schedulerConf, -1) - s = strings.Replace(s, "$controllermanagerbin", controllerManagerBin, -1) - s = strings.Replace(s, "$controllermanagerconf", controllerManagerConf, -1) - s = strings.Replace(s, "$config", config, -1) - - s = strings.Replace(s, "$etcdbin", etcdBin, -1) - s = strings.Replace(s, "$etcdconf", etcdConf, -1) - s = strings.Replace(s, "$flanneldbin", flanneldBin, -1) - s = strings.Replace(s, "$flanneldconf", flanneldConf, -1) - - s = strings.Replace(s, "$kubeletbin", kubeletBin, -1) - s = strings.Replace(s, "$kubeletconf", kubeletConf, -1) - s = strings.Replace(s, "$proxybin", proxyBin, -1) - s = strings.Replace(s, "$proxyconf", proxyConf, -1) - - s = strings.Replace(s, "$fedapiserverbin", fedApiserverBin, -1) - s = strings.Replace(s, "$fedcontrollermanagerbin", fedControllerManagerBin, -1) + s := multiWordReplace(string(in), "$apiserverbin", apiserverBin) + s = multiWordReplace(s, "$apiserverconf", apiserverConf) + s = multiWordReplace(s, "$schedulerbin", schedulerBin) + s = multiWordReplace(s, "$schedulerconf", schedulerConf) + s = multiWordReplace(s, "$controllermanagerbin", controllerManagerBin) + s = multiWordReplace(s, "$controllermanagerconf", controllerManagerConf) + s = multiWordReplace(s, "$config", config) + + s = multiWordReplace(s, "$etcdbin", etcdBin) + s = multiWordReplace(s, "$etcdconf", etcdConf) + s = multiWordReplace(s, "$flanneldbin", flanneldBin) + s = multiWordReplace(s, "$flanneldconf", flanneldConf) + + s = multiWordReplace(s, "$kubeletbin", kubeletBin) + s = multiWordReplace(s, "$kubeletconf", kubeletConf) + s = multiWordReplace(s, "$proxybin", proxyBin) + s = multiWordReplace(s, "$proxyconf", proxyConf) + + s = multiWordReplace(s, "$fedapiserverbin", fedApiserverBin) + s = multiWordReplace(s, "$fedcontrollermanagerbin", fedControllerManagerBin) controls, err := check.NewControls(t, []byte(s)) if err != nil { diff --git a/cmd/util.go b/cmd/util.go index 4967cbe..478ae21 100644 --- a/cmd/util.go +++ b/cmd/util.go @@ -150,3 +150,12 @@ func versionMatch(r *regexp.Regexp, s string) string { } return match[1] } + +func multiWordReplace(s string, subname string, sub string) string { + f := strings.Fields(sub) + if len(f) > 1 { + sub = "'" + sub + "'" + } + + return strings.Replace(s, subname, sub, -1) +} diff --git a/cmd/util_test.go b/cmd/util_test.go index fe9e0b9..dbd434b 100644 --- a/cmd/util_test.go +++ b/cmd/util_test.go @@ -107,3 +107,25 @@ func TestVerifyBin(t *testing.T) { }) } } + +func TestMultiWordReplace(t *testing.T) { + cases := []struct { + input string + sub string + subname string + output string + }{ + {input: "Here's a file with no substitutions", sub: "blah", subname: "blah", output: "Here's a file with no substitutions"}, + {input: "Here's a file with a substitution", sub: "blah", subname: "substitution", output: "Here's a file with a blah"}, + {input: "Here's a file with multi-word substitutions", sub: "multi word", subname: "multi-word", output: "Here's a file with 'multi word' substitutions"}, + {input: "Here's a file with several several substitutions several", sub: "blah", subname: "several", output: "Here's a file with blah blah substitutions blah"}, + } + for id, c := range cases { + t.Run(strconv.Itoa(id), func(t *testing.T) { + s := multiWordReplace(c.input, c.subname, c.sub) + if s != c.output { + t.Fatalf("Expected %s got %s", c.output, s) + } + }) + } +} From af0eadc792db314478cda46be1ba3a10da8d0f07 Mon Sep 17 00:00:00 2001 From: Liz Rice Date: Tue, 15 Aug 2017 18:34:07 +0100 Subject: [PATCH 4/4] Add a couple more tests for file permission checks --- check/data | 22 ++++++++++++++++++++++ check/test_test.go | 8 ++++++++ 2 files changed, 30 insertions(+) diff --git a/check/data b/check/data index 73575b3..1e88841 100644 --- a/check/data +++ b/check/data @@ -94,3 +94,25 @@ groups: op: eq value: "644" set: true + + - id: 9 + text: "test permissions" + audit: "/bin/sh -c 'if test -e $config; then stat -c %a $config; fi'" + tests: + bin_op: or + test_items: + - flag: "644" + compare: + op: eq + value: "644" + set: true + - flag: "640" + compare: + op: eq + value: "640" + set: true + - flag: "600" + compare: + op: eq + value: "600" + set: true diff --git a/check/test_test.go b/check/test_test.go index f605309..a0228c2 100644 --- a/check/test_test.go +++ b/check/test_test.go @@ -86,6 +86,14 @@ func TestTestExecute(t *testing.T) { controls.Groups[0].Checks[8], "644", }, + { + controls.Groups[0].Checks[9], + "640", + }, + { + controls.Groups[0].Checks[9], + "600", + }, } for _, c := range cases {