Initial Security Model #693

pjbgf · 2021-11-24T20:17:22Z

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Initial security model markdown to inform users on permissions, and overall security related information.
In the future, this will contain also include compliance status (i.e. NIST, SLSA, etc.)

Which issue(s) this PR fixes:

Relates to #653

Does this PR have test?

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

NONE

pjbgf · 2021-11-24T20:18:39Z

@jhrozek @JAORMX @saschagrunert keen on hearing thoughts on how we can improve this. Also, need some other pairs of eyes to double check on the selinux front.

codecov-commenter · 2021-11-24T20:21:16Z

Codecov Report

Merging #693 (ac780d8) into master (568d383) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #693   +/-   ##
=======================================
  Coverage   46.41%   46.41%           
=======================================
  Files          35       35           
  Lines        2359     2359           
=======================================
  Hits         1095     1095           
  Misses       1211     1211           
  Partials       53       53

saschagrunert

I love it, it’s great to have these docs!

Do we plan in the future to only distinguish those two modes by configuration or do we wanna keep the operator configurable as is?

saschagrunert · 2021-11-25T08:09:43Z

/hold
For other reviews.

jhrozek

This is really great work. I left some comments inline.

Let's get this merged soon and then I'll add some OCP specific info on top of that (e.g. we are using the privileged SCC ...)

Btw does it make sense to mention that by default almost all the containers run with the spc_t SELinux type which pretty much means "unconfined"? (If you prefer, I can propose another PR atop this)

jhrozek · 2021-11-25T11:49:27Z

security-model.md

+
+|                                  | Seccomp | SELinux | AppArmor |
+|----------------------------------|---------|---------|----------|
+|               Requires root user |    No   |   Yes   |    Yes   |


Can you help me understand this column better? Does it mean "the operator is running as root" or "anything in the DS is running as root"? Because if SELinux is enabled, only selinuxd is running as root, not the operator. The operator just writes to /etc/selinux.d which is owned by the non-root user the operator is running as.

The idea here was to be more generic, meaning that for SELinux feature to be operational, something must be running at that given permission level to be able support it.

Initially I thought as marking with No* and adding a caveat soon after, but then decided to make it easier. But I agree, it is not completely clear.
Do you think it would be better if we broke down each of the components that support each enforcement technology (selinux, apparmor, selinux)?

I think your approach is better and we might want to just add a side-note or a sub-paragraph later.

jhrozek · 2021-11-25T11:51:12Z

security-model.md

+|                                  | Seccomp | SELinux | AppArmor |
+|----------------------------------|---------|---------|----------|
+|               Requires root user |    No   |   Yes   |    Yes   |
+|                 Requires HostPID |    No   |    ?    |    Yes   |


I don't see SELinux requiring hostPID, But then again, I must be looking at the wrong place, because I don't see AppArmor requiring hostpid either. Only the BPF recorder and the log enricher.

Sure, let me fix that - SELinux.

For AppArmor, it does currently as it needs to enter the host mnt namespace to gain access to the AppArmorFS on demand. There are other ways of doing it, but that's the current approach.

Ah, I know what confused me, the code that uses hostPIDs for AppArmor is not merged yet, but it is part of PR#680 right?

yes, that's correct. once that is merged and apparmor is enabled, spod will require HostPID.

jhrozek · 2021-11-25T11:52:56Z

security-model.md

+|----------------------------------|---------|---------|----------|
+|               Requires root user |    No   |   Yes   |    Yes   |
+|                 Requires HostPID |    No   |    ?    |    Yes   |
+|  Requires "privileged container" |    No   |   Yes   |    Yes   |


Same question here, I only see the log-enricher and the BPF recorder being privileged. selinuxd is running with its own SELinux policy.

jhrozek · 2021-11-25T11:53:08Z

security-model.md

+|               Requires root user |    No   |   Yes   |    Yes   |
+|                 Requires HostPID |    No   |    ?    |    Yes   |
+|  Requires "privileged container" |    No   |   Yes   |    Yes   |
+|     Requires SSH Access to nodes |    No   |    No   |    No    |


I'm not sure what this means?

Same as #693 (comment)

In the future we could potentially have a page to compare with other projects and help folks decide. But that may be hard to keep up to date.

jhrozek · 2021-11-25T11:54:42Z

security-model.md

+|                 Requires HostPID |    No   |    ?    |    Yes   |
+|  Requires "privileged container" |    No   |   Yes   |    Yes   |
+|     Requires SSH Access to nodes |    No   |    No   |    No    |
+| Access to host's mount namespace |    No   |    ?    | On Demand|


I'm not sure what exactly do you mean here. SELinux integration makes use of HostPath mounts, yes, but e.g. no mount propagation is used.

Sure, going through it again it does not come across as clear as I intented it to be.

On a follow-up PR I think this can be replaced by breaking down the "host paths" for each main feature (SAS) and the access (rw) they require. So regardless of this is implemented, for Seccomp to work it requires rw access to {kubelet_path}/seccomp. And the same for selinux/apparmor.

Right, selinuxd needs to write to the hosts's selinux db. Thanks for the explanation.

jhrozek · 2021-11-25T11:56:36Z

security-model.md

+
+During the execution in profile generate mode, the observed applications may run less restricted than it 
+would otherwise, to allow for their operations to be observed and recorded. Keep this in mind when using it
+against workloads you may not trust.


I think we should expand on this section. e.g. for seccomp, you have a choice of using the log-enricher or the BPF built-in recorder which seem to be using hostPID or the seccomp OCI hook which doesn't (at least not in the operator context).

Fair, to be honest I think we need to add a lot more meat to this section. But was thinking on leaving as is and improve on follow-up PRs.

Overall, profile generation is tricky and generally can be done either with target workload being a) unrestricted or b) restricted with existing/template profile. The former may have security implications and the latter would yield a partial profile - assuming that those execution paths may not be fully materialised when restricted.

In my point of view, we can have here a disclaimer of potential risks, but then have a full on section on profiling in which we suggest how to do that correctly and securely. Would you be OK with that?

jhrozek · 2021-11-25T11:56:55Z

security-model.md

+|               Requires root user |   Yes   |   Yes   |    Yes   |
+|                 Requires HostPID |    No   |    ?    |    Yes   |
+|  Requires "privileged container" |    No   |   Yes   |    Yes   |
+|     Requires SSH Access to nodes |    No   |    No   |    No    |


I'm not sure what does SSH access to nodes mean here.

Fair point, this was just a reference to help end users to compare SPO with other projects that provide similar capabilities.

Some of those projects require SSH credentials in order to executec commands in the nodes to install/remove profiles.

Oh I didn't know that. Leave it in then, I like to show off where SPO is better!

jhrozek · 2021-11-25T11:57:13Z

security-model.md

+|                                  | Seccomp | SELinux | AppArmor |
+|----------------------------------|---------|---------|----------|
+|               Requires root user |   Yes   |   Yes   |    Yes   |
+|                 Requires HostPID |    No   |    ?    |    Yes   |


Since SELinux recording depend on the log enricher which uses hostPID, then the answer is yes.

Fixed it accordingly.

pjbgf · 2021-11-25T14:33:30Z

Btw does it make sense to mention that by default almost all the containers run with the spc_t SELinux type which pretty much means "unconfined"? (If you prefer, I can propose another PR atop this)

@jhrozek great point - yes. I think we should get out at a container level:

SELinux type
AppArmor profile
Seccomp profile
Capabilities

I added it here - as I had to make some changes anyway. But happy for you to improve it via a follow-up PR on that.

On the mid/long term, it would be great if we could think on ways to automatically extract such information (like we do for RBAC).

pjbgf · 2021-11-25T15:52:09Z

@jhrozek went through your poinds - thank you so much for the extensive feedback. PTAL

saschagrunert · 2021-11-25T15:56:17Z

security-model.md

+#### Host Paths
+
+Throughout their operation they require read and write permissions into host paths:
+
+- `/var/lib/kubelet/seccomp`
+- `/etc/selinux.d`
+- `/etc/apparmor.d`


There are a bunch of more host paths, should we cover them as well: https://cs.k8s.io/?q=HostPathVolumeSource&i=nope&files=&excludeFiles=vendor&repos=kubernetes-sigs/security-profiles-operator

@saschagrunert nice! I did not know about https://cs.k8s.io 👍

Yes, I think we should break it all down and make it explicit what we need and to what level.
This will also help when we create our AppArmor profiles - which could actually become our official documentation on host-level disk permissions.

pjbgf · 2021-11-25T16:00:40Z

I love it, it’s great to have these docs!

Do we plan in the future to only distinguish those two modes by configuration or do we wanna keep the operator configurable as is?

@saschagrunert thank you. In my opinion we can keep as is right now. Once we are a bit more settled in terms of features - specially around AppArmor and the great bits around eBPF profiling you are working on - we review it. What do you think?

saschagrunert · 2021-11-25T16:02:39Z

@saschagrunert thank you. In my opinion we can keep as is right now. Once we are a bit more settled in terms of features - specially around AppArmor and the great bits around eBPF profiling you are working on - we review it. What do you think?

Sounds great!

pjbgf · 2021-11-25T16:04:33Z

/test pull-security-profiles-operator-test-e2e

pjbgf · 2021-11-25T16:24:59Z

/test pull-security-profiles-operator-test-e2e

pjbgf · 2021-11-25T17:19:58Z

/test pull-security-profiles-operator-verify

jhrozek · 2021-11-25T19:30:27Z

I'm happy with the current state!
/lgtm

pjbgf · 2021-11-25T20:14:07Z

/retest

pjbgf · 2021-11-25T21:49:14Z

/test pull-security-profiles-operator-verify

pjbgf · 2021-11-25T22:18:20Z

/test pull-security-profiles-operator-verify

pjbgf · 2021-11-25T22:49:19Z

/test pull-security-profiles-operator-verify

JAORMX

Looks like a good start! Thanks for setting this up!

JAORMX · 2021-11-26T08:20:18Z

security-model.md

+| Access to host's mount namespace | No          | Yes         | On Demand   |
+|                 AppArmor Profile | `default`   | `default`   | `default`   |
+|                     SELinux type | `spc_t`     | `spc_t`     | `spc_t`     |
+|                  Seccomp Profile | `unconfined`| `unconfined`| `unconfined`|


one thing to note (maybe this can go in a separate PR) is that when SELinux is enabled, the SPOd has a sidecar container running selinuxd. selinuxd actually runs with a selinux.process type and is not a privileged container. Although it does run as root. Something we could start investigating is trying to run it with a non-root user and seeing if we can make do with enabling extra capabilities.... However, a lot of the attack surface of that container is mitigated by the custom policy we use for it.

JAORMX · 2021-11-26T08:21:45Z

security-model.md

+Throughout their operation they require read and write permissions into host paths:
+
+- `/var/lib/kubelet/seccomp`
+- `/etc/selinux.d`


we've removed this now #698

k8s-ci-robot · 2021-11-26T08:22:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, pjbgf, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JAORMX,pjbgf,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2021-11-26T11:33:25Z

/unhold

JAORMX · 2021-11-26T11:50:21Z

/retest

saschagrunert · 2021-11-26T12:03:35Z

/retest

pjbgf · 2021-11-26T12:07:00Z

/test pull-security-profiles-operator-verify

saschagrunert · 2021-11-26T12:07:16Z

/retest

saschagrunert · 2021-11-26T12:10:39Z

/retest

saschagrunert · 2021-11-26T12:11:00Z

/override pull-security-profiles-operator-verify

k8s-ci-robot · 2021-11-26T12:11:14Z

@saschagrunert: Overrode contexts on behalf of saschagrunert: pull-security-profiles-operator-verify

In response to this:

/override pull-security-profiles-operator-verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

saschagrunert · 2021-11-26T12:13:10Z

/override pull-security-profiles-operator-verify

k8s-ci-robot · 2021-11-26T12:13:23Z

@saschagrunert: Overrode contexts on behalf of saschagrunert: pull-security-profiles-operator-verify

In response to this:

/override pull-security-profiles-operator-verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 24, 2021

k8s-ci-robot requested review from cmurphy and jhrozek November 24, 2021 20:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2021

pjbgf mentioned this pull request Nov 24, 2021

Improve visibility of SPO's security posture #653

Closed

10 tasks

saschagrunert approved these changes Nov 25, 2021

View reviewed changes

k8s-ci-robot assigned saschagrunert Nov 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2021

jhrozek reviewed Nov 25, 2021

View reviewed changes

pjbgf force-pushed the secmodel branch from 79317eb to 4a0141f Compare November 25, 2021 15:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2021

saschagrunert reviewed Nov 25, 2021

View reviewed changes

Add security model

ac780d8

pjbgf force-pushed the secmodel branch from 4a0141f to ac780d8 Compare November 25, 2021 17:09

k8s-ci-robot assigned jhrozek Nov 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2021

saschagrunert approved these changes Nov 26, 2021

View reviewed changes

JAORMX approved these changes Nov 26, 2021

View reviewed changes

k8s-ci-robot assigned JAORMX Nov 26, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2021

k8s-ci-robot merged commit df29ce7 into kubernetes-sigs:main Nov 26, 2021

Initial Security Model #693

Initial Security Model #693

Conversation

pjbgf commented Nov 24, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

pjbgf commented Nov 24, 2021

codecov-commenter commented Nov 24, 2021 • edited Loading

Codecov Report

saschagrunert left a comment

Choose a reason for hiding this comment

saschagrunert commented Nov 25, 2021

jhrozek 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjbgf commented Nov 25, 2021 • edited Loading

pjbgf commented Nov 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjbgf commented Nov 25, 2021

saschagrunert commented Nov 25, 2021

pjbgf commented Nov 25, 2021

pjbgf commented Nov 25, 2021

pjbgf commented Nov 25, 2021

jhrozek commented Nov 25, 2021

pjbgf commented Nov 25, 2021

pjbgf commented Nov 25, 2021

pjbgf commented Nov 25, 2021

pjbgf commented Nov 25, 2021

JAORMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

JAORMX commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

pjbgf commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

k8s-ci-robot commented Nov 26, 2021

saschagrunert commented Nov 26, 2021

k8s-ci-robot commented Nov 26, 2021

codecov-commenter commented Nov 24, 2021 •

edited

Loading

pjbgf commented Nov 25, 2021 •

edited

Loading