Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SELinux Overhaul #33663

Merged
merged 9 commits into from
Nov 1, 2016
Merged

SELinux Overhaul #33663

merged 9 commits into from
Nov 1, 2016

Conversation

pmorie
Copy link
Member

@pmorie pmorie commented Sep 28, 2016

Overhauls handling of SELinux in Kubernetes. TLDR: Kubelet dir no longer has to be labeled svirt_sandbox_file_t.

Fixes #33351 and #33510. Implements #33951.


This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 28, 2016
@pmorie pmorie assigned yifan-gu and unassigned ixdy Sep 28, 2016
@pmorie pmorie added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 28, 2016
@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/design Categorizes issue or PR as related to design. labels Sep 28, 2016
@yifan-gu
Copy link
Contributor

cc @kubernetes/sig-node @kubernetes/sig-rktnetes

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2016
@pmorie pmorie force-pushed the selinux-fixes branch 3 times, most recently from 591246b to 698fca6 Compare October 24, 2016 22:51
@pmorie pmorie changed the title WIP: SELinux Overhaul SELinux Overhaul Oct 24, 2016
@pmorie
Copy link
Member Author

pmorie commented Oct 24, 2016

@yifan-gu ready for review, PTAL.

@@ -510,7 +498,7 @@ function start_kubelet {
--volume=/var/run:/var/run:rw \
--volume=/sys:/sys:ro \
--volume=/var/lib/docker/:/var/lib/docker:ro \
--volume=/var/lib/kubelet/:/var/lib/kubelet:rw,z \
--volume=/var/lib/kubelet/:/var/lib/kubelet:rw \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, the relabel to a shared SELinux context should no longer be necessary, because the Kubelet will be running with spc_t due to running in a privileged container, which is unconfined.

@pmorie pmorie added area/kubelet area/security and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Oct 24, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit aa855b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit aa855b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit aa855b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@feiskyer
Copy link
Member

@k8s-bot test this please, issue #IGNORE.

@feiskyer
Copy link
Member

lgtm

@feiskyer
Copy link
Member

@euank @squeed would you like to take another look?

@pmorie
Copy link
Member Author

pmorie commented Oct 27, 2016

cc @aveshagarwal @ingvagabund


// Have docker relabel the termination log path if SELinux is
// enabled.
if selinux.SELinuxEnabled() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why this was not here before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't as smart before :) It was a bug.

ContainerPath: etcHostsPath,
HostPath: hostsFilePath,
ReadOnly: false,
SELinuxRelabel: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe comment it with Isolate pods on each host from one another by default? Thought it is already captured in the docs (I liked reading it), the comment here could help to make the correct associations sooner. Just nit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ingvagabund I'm not sure I understand what is being asked for here. Elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to point out the SELinuxRelabel is set to true to isolate the pods from each other by default.

@pmorie pmorie added this to the v1.5 milestone Oct 28, 2016
@feiskyer
Copy link
Member

feiskyer commented Nov 1, 2016

ping @yifan-gu @pmorie. Any updates to this?

@yifan-gu yifan-gu assigned ingvagabund and unassigned yifan-gu Nov 1, 2016
@yifan-gu
Copy link
Contributor

yifan-gu commented Nov 1, 2016

LGTM
@ingvagabund I reassigned to you, feel free to apply lgtm label :)

@ingvagabund ingvagabund added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 44b684a into kubernetes:master Nov 1, 2016
k8s-github-robot pushed a commit that referenced this pull request Nov 6, 2016
Automatic merge from submit-queue

Remove GetRootContext method from VolumeHost interface

Remove the `GetRootContext` call from the `VolumeHost` interface, since Kubernetes no longer needs to know the SELinux context of the Kubelet directory.

Per #33951 and #35127.

Depends on #33663; only the last commit is relevant to this PR.
euank added a commit to euank/kubernetes that referenced this pull request Jul 20, 2017
The original change that added the unconfined label included a comment
indicating it won't be needed in the future.
See: kubernetes#33555 (comment)

That time is now. kubernetes#33663
has landed and means we no longer have to go out of our way to make that
work.

Removing the label also increases security since there wasn't really a
good reason for etcd to be run with such broad selinux privileges.

This also will allow kubeadm to avoid errors on distros without an spc_t
type, such as Gentoo and Container Linux (at the time of writing at
least).

Fixes kubernetes/kubeadm#269
k8s-github-robot pushed a commit that referenced this pull request Jul 21, 2017
Automatic merge from submit-queue (batch tested with PRs 49328, 49285, 49307, 49127, 49163)

kubeadm: don't customize etcd selinux label

The original change that added the unconfined label included a comment
indicating it won't be needed in the future.
See: #33555 (comment)

That time is now. #33663
has landed and means we no longer have to go out of our way to make that
work.

Removing the label also increases security since there wasn't really a
good reason for etcd to be run with such broad selinux privileges.

This also will allow kubeadm to avoid errors on distros without an spc_t
type, such as Gentoo and Container Linux (at the time of writing at
least).

Fixes kubernetes/kubeadm#269

**Release note**:
```release-note
NONE
```
luxas pushed a commit to luxas/kubernetes that referenced this pull request Jul 25, 2017
The original change that added the unconfined label included a comment
indicating it won't be needed in the future.
See: kubernetes#33555 (comment)

That time is now. kubernetes#33663
has landed and means we no longer have to go out of our way to make that
work.

Removing the label also increases security since there wasn't really a
good reason for etcd to be run with such broad selinux privileges.

This also will allow kubeadm to avoid errors on distros without an spc_t
type, such as Gentoo and Container Linux (at the time of writing at
least).

Fixes kubernetes/kubeadm#269
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue

Remove GetRootContext method from VolumeHost interface

Remove the `GetRootContext` call from the `VolumeHost` interface, since Kubernetes no longer needs to know the SELinux context of the Kubelet directory.

Per kubernetes#33951 and kubernetes#35127.

Depends on kubernetes#33663; only the last commit is relevant to this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubelet area/security lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants