Adds a new 'mounts' command to audit sensitive host paths mounts #322

jcbbc · 2021-02-03T17:00:58Z

Description

Adds a new command mounts to audit sensitive host paths that are mounted in contains. The auditor will trigger an alert when one of the specified paths is mounted by a container. If no paths are specified in arguments, a default paths list is used (I use the one used by Falco).

Alerts can be overridden using a label either at the pod level or the container level. Labels are per mount, so you can only override alerts for legit mounts.

Here's a sample result:

kubeaudit mounts -f /Users/j.courtial/dev/go/src/github.com/kubeaudit/auditors/mounts/fixtures/proc-mounted.yml

---------------- Results for ---------------

  apiVersion: v1
  kind: Pod
  metadata:
    name: pod
    namespace: proc-mounted

--------------------------------------------

-- [error] SensitivePathsMounted
   Message: Sensitive path mounted as volume: proc-volume (/proc -> /host/proc, readOnly: false). It should be removed from the container's mounts list.
   Metadata:
      Container: container
      Mount: proc-volume

This auditor makes the mountds kind of redundant so we can remove/deprecate it if you prefer.

Type of change

New feature ✨
This change requires a documentation update 📖

How Has This Been Tested?

Automated tests
Manual tests (cluster - local mode, manifest files)

Checklist:

I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The test coverage did not decrease
I have signed the appropriate Contributor License Agreement

The audit will trigger an alert if one a the specified host paths is mounted inside a container. Alerts for legit use cases can be overriden using a dedicated label per mount.

genevieveluyt · 2021-02-04T15:50:18Z

Hey @jcbbc , thanks for opening another PR! The team will triage this on Monday. Cheers!

genevieveluyt

This is really awesome, thank you! Much better implemented than the mountds auditor too 😄

I think you're right and we should deprecate mountds in favour of this one. There's not really a proper deprecation flow but I think we can just remove it from the all command and update the docs to say it's deprecated.

Could you also update the kubeaudit config docs in the main README and add examples using the config file and flag (for example)

Would you mind modifying one of the tests (or adding a test) to test at least one other host path (other than /proc) to prove it doesn't only work for the first path in the array.

auditors/all/all.go

auditors/mounts/mounts.go

docs/auditors/mounts.md

auditors/mounts/config.go

genevieveluyt

Docs are looking good!

cmd/commands/all.go

auditors/mounts/mounts_test.go

docs/auditors/mounts.md

jcbbc · 2021-02-12T09:20:38Z

Should we address the deprecation in this PR or another one?

genevieveluyt

Just a few small things and then I think it should be good to merge! Let's do the deprecation separately.

genevieveluyt · 2021-02-16T15:51:50Z

auditors/mounts/mounts.go

+}
+
+func getOverrideLabel(mountName string) string {
+	return overrideLabelPrefix + strings.Replace(strings.ToLower(mountName), "_", "-", -1)


Suggested change

return overrideLabelPrefix + strings.Replace(strings.ToLower(mountName), "_", "-", -1)

return overrideLabelPrefix + mountName

genevieveluyt · 2021-02-16T15:52:40Z

cmd/commands/mounts.go

+A WARN result is generated when a container mounts one or more paths specified with the '--paths' argument.
+
+Example usage:
+kubeaudit mount --paths "%s"`, formatPathsList(), strings.Join(mounts.DefaultSensitivePaths[:3], ",")),


Suggested change

kubeaudit mount --paths "%s"`, formatPathsList(), strings.Join(mounts.DefaultSensitivePaths[:3], ",")),

kubeaudit mounts --paths "%s"`, formatPathsList(), strings.Join(mounts.DefaultSensitivePaths[:3], ",")),

genevieveluyt · 2021-02-16T16:13:00Z

auditors/mounts/mounts.go

+					"Container":              container.Name,
+					MountNameMetadataKey:     mount.Name,
+					MountPathMetadataKey:     mount.MountPath,
+					MountReadOnlyMetadataKey: fmt.Sprintf("%t", mount.ReadOnly),


Actually can we add the volume name and host path as metadata as well? It might be useful for JSON output

genevieveluyt · 2021-02-16T16:13:48Z

docs/auditors/mounts.md

+   Message: Sensitive path mounted as volume: etc-volume (/etc -> /host/etc, readOnly: false). It should be removed from the container's mounts list.
+   Metadata:
+      Container: container
+      Mount: etc-volume


Do you mind adding the other metadata fields here?

genevieveluyt

Awesome, thanks so much!

docs/auditors/mounts.md

dani-santos-code · 2021-02-16T22:27:35Z

docs/auditors/mounts.md

+```yaml
+---
+auditors:
+  mounts:
+    paths: ["/etc", "/var/run/docker.sock"]
+```


When I tested locally, I also had to turn this auditor on by setting mountsto true, like so:

enabledAuditors: mounts: true

I see it in the config.yaml example, but it might be worth making it explicit

Another suggestion, to make it more explicit (completely optional). Instead of paths, how about pathsToWatch?

I went for the simple paths to be more aligned with other auditors that also use simple key (add, image, cpu) that are also the same as the CLI options. But we can totally switch to pathsToWatch if you find it better.

I see. For the capabilities auditor, however, we add capabilities to an allowAddList so kubeaudit won't raise errors for those. So it made me think that paths would have a similar effect - listed paths would be the ones kubeaudit would ignore. However it's the opposite - kubeaudit will only watch for those specific paths, everything else will be ignored. That's why I thought of a more explicit name... what do you think?

Ok understood. I find the phrasing a bit weird however :) I was about to suggest sensitivesPaths but I guess the issue is the same, it may not be quite clear that it work as a deny-list and not as an allow-list. So if you prefer, yes we can go for pathsToWatch

yeah... let's leave it like that, then. And hopefully the documentation will make it clear! 😁

or maybe denyPathsList? :P

@jcbbc can you please change it to denyPathsList? with that, we're good to go! thanks! 🙏

Sorry, I missed your previous comment. I've made the change.

Thanks a lot, @jcbbc!! :)

docs/auditors/mounts.md

dani-santos-code · 2021-02-16T22:56:10Z

cmd/commands/mounts.go

+}
+
+func setPathsFlags(cmd *cobra.Command) {
+	cmd.Flags().StringSliceVarP(&mountsConfig.SensitivePaths, sensitivePathsFlagName, "s", mounts.DefaultSensitivePaths,


Another tiny thing, and more of a question. How about p rather than s for the shorthand?

Suggested change

cmd.Flags().StringSliceVarP(&mountsConfig.SensitivePaths, sensitivePathsFlagName, "s", mounts.DefaultSensitivePaths,

cmd.Flags().StringSliceVarP(&mountsConfig.SensitivePaths, sensitivePathsFlagName, "p", mounts.DefaultSensitivePaths,

Yes, that was my initial idea but -p is already used as a shorthand for the --format argument (I guess it comes from pretty?)

-p, --format string The output format to use (one of "pretty", "logrus", "json") (default "pretty")

So I fall back on s for "Sensitive paths" but it's indeed not really obvious

oh, yeah! you're right! perhaps sp? not sure...it's optional. We can always revisit that later if someone gives us feedback saying it's confusing.

dani-santos-code · 2021-02-16T22:59:21Z

This is looking great, @jcbbc! I agree with @genevieveluyt that this is a more comprehensive implementation, if compared to our current mountds auditor! Thank you!!! I left some comments/questions. So it makes sense to deprecate/remove the old auditor. 🎉🎉🎉

docs/auditors/mounts.md

jcbbc · 2021-02-24T10:35:50Z

Should be good now 👌

dani-santos-code · 2021-02-24T12:57:11Z

Thank you @jcbbc !!!

jcbbc added 2 commits February 3, 2021 17:54

Add a new 'mounts' command to audit sensitive host paths mounts

6a42bf1

The audit will trigger an alert if one a the specified host paths is mounted inside a container. Alerts for legit use cases can be overriden using a dedicated label per mount.

Remove unused code

230e1a6

ghost added the core label Feb 3, 2021

jcbbc mentioned this pull request Feb 4, 2021

Add a new command to audit runAsUser fields #316

Merged

10 tasks

genevieveluyt reviewed Feb 10, 2021

View reviewed changes

Various improvements following feedbacks

fa88f5e

ghost added the readme label Feb 11, 2021

jcbbc added 2 commits February 11, 2021 16:51

Merge branch 'master' into mount-audit

ebad8dd

Move mount info to metadata

26cc11a

genevieveluyt reviewed Feb 11, 2021

View reviewed changes

cmd/commands/all.go Outdated Show resolved Hide resolved

auditors/mounts/mounts_test.go Outdated Show resolved Hide resolved

docs/auditors/mounts.md Outdated Show resolved Hide resolved

Another batch of small improvements

cf59458

genevieveluyt reviewed Feb 16, 2021

View reviewed changes

More improvements following feedbacks

57c7453

genevieveluyt previously approved these changes Feb 16, 2021

View reviewed changes

dani-santos-code reviewed Feb 16, 2021

View reviewed changes

docs/auditors/mounts.md Outdated Show resolved Hide resolved

dani-santos-code reviewed Feb 16, 2021

View reviewed changes

docs/auditors/mounts.md Outdated Show resolved Hide resolved

dani-santos-code reviewed Feb 16, 2021

View reviewed changes

docs/auditors/mounts.md Outdated Show resolved Hide resolved

dani-santos-code reviewed Feb 16, 2021

View reviewed changes

Documentation improvements

f2782d0

jcbbc dismissed genevieveluyt’s stale review via f2782d0 February 17, 2021 08:28

dani-santos-code previously approved these changes Feb 18, 2021

View reviewed changes

Rename --path argument to --denyPathsList

7eb28a2

jcbbc dismissed dani-santos-code’s stale review via 7eb28a2 February 23, 2021 16:17

dani-santos-code reviewed Feb 23, 2021

View reviewed changes

docs/auditors/mounts.md Outdated Show resolved Hide resolved

dani-santos-code reviewed Feb 23, 2021

View reviewed changes

docs/auditors/mounts.md Show resolved Hide resolved

jcbbc added 2 commits February 24, 2021 10:44

Change -s shorthand to -d. Also fix docs

0452090

More doc fixes

d0fb510

dani-santos-code approved these changes Feb 24, 2021

View reviewed changes

dani-santos-code merged commit fb6003a into Shopify:master Feb 24, 2021

dani-santos-code mentioned this pull request Feb 26, 2021

Deprecate mountds #328

Closed

3 tasks

dani-santos-code mentioned this pull request Mar 23, 2021

Removes mountds in favour of mounts #330

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a new 'mounts' command to audit sensitive host paths mounts #322

Adds a new 'mounts' command to audit sensitive host paths mounts #322

jcbbc commented Feb 3, 2021 •

edited

Loading

genevieveluyt commented Feb 4, 2021

genevieveluyt left a comment

genevieveluyt left a comment

jcbbc commented Feb 12, 2021

genevieveluyt left a comment

genevieveluyt Feb 16, 2021

genevieveluyt Feb 16, 2021

genevieveluyt Feb 16, 2021

genevieveluyt Feb 16, 2021

genevieveluyt left a comment

dani-santos-code Feb 16, 2021 •

edited

Loading

dani-santos-code Feb 16, 2021

jcbbc Feb 17, 2021

dani-santos-code Feb 17, 2021

jcbbc Feb 18, 2021

dani-santos-code Feb 18, 2021 •

edited

Loading

dani-santos-code Feb 18, 2021

dani-santos-code Feb 23, 2021

jcbbc Feb 23, 2021

dani-santos-code Feb 23, 2021

dani-santos-code Feb 16, 2021

jcbbc Feb 17, 2021

dani-santos-code Feb 17, 2021 •

edited

Loading

dani-santos-code commented Feb 16, 2021 •

edited

Loading

jcbbc commented Feb 24, 2021

dani-santos-code commented Feb 24, 2021

	return overrideLabelPrefix + strings.Replace(strings.ToLower(mountName), "_", "-", -1)
	return overrideLabelPrefix + mountName

	kubeaudit mount --paths "%s"`, formatPathsList(), strings.Join(mounts.DefaultSensitivePaths[:3], ",")),
	kubeaudit mounts --paths "%s"`, formatPathsList(), strings.Join(mounts.DefaultSensitivePaths[:3], ",")),

	cmd.Flags().StringSliceVarP(&mountsConfig.SensitivePaths, sensitivePathsFlagName, "s", mounts.DefaultSensitivePaths,
	cmd.Flags().StringSliceVarP(&mountsConfig.SensitivePaths, sensitivePathsFlagName, "p", mounts.DefaultSensitivePaths,

Adds a new 'mounts' command to audit sensitive host paths mounts #322

Adds a new 'mounts' command to audit sensitive host paths mounts #322

Conversation

jcbbc commented Feb 3, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

genevieveluyt commented Feb 4, 2021

genevieveluyt left a comment

Choose a reason for hiding this comment

genevieveluyt left a comment

Choose a reason for hiding this comment

jcbbc commented Feb 12, 2021

genevieveluyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genevieveluyt left a comment

Choose a reason for hiding this comment

dani-santos-code Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-santos-code Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-santos-code Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

dani-santos-code commented Feb 16, 2021 • edited Loading

jcbbc commented Feb 24, 2021

dani-santos-code commented Feb 24, 2021

jcbbc commented Feb 3, 2021 •

edited

Loading

dani-santos-code Feb 16, 2021 •

edited

Loading

dani-santos-code Feb 18, 2021 •

edited

Loading

dani-santos-code Feb 17, 2021 •

edited

Loading

dani-santos-code commented Feb 16, 2021 •

edited

Loading