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.

Sign up for GitHub

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

Jump to bottom

Fedora and RHEL use etc_t and the convention is <type_name>_t #7891

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Fedora and RHEL use etc_t and the convention is <type_name>_t #7891

merged 3 commits into from
Aug 27, 2021

Conversation

cristicalin
Copy link
Contributor

@cristicalin cristicalin commented Aug 19, 2021 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:
In #7791 the SELinux file context type for /etc/kubernetes/kubelet.env was incorrectly set to t_etc. The convention and implementation of the type in Fedora, RHEL and derivatives is etc_t. This PR sets the proper file type so that systemd can load the environment file.

Which issue(s) this PR fixes:

Fixes #7790

Special notes for your reviewer:
This PR adds a CI job with preinstall_selinux_state set to enforcing on latest CentOS/Alma/Fedora to catch SELinux errors. Our default is permissive which is not something most people use in production.

Does this PR introduce a user-facing change?:

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 19, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @cristicalin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 19, 2021
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 19, 2021
@cristicalin cristicalin force-pushed the kubelet_env_selinux_type branch from 7a55b51 to 6a4ffd7 Compare August 19, 2021 08:11
champtar
champtar reviewed Aug 19, 2021
View reviewed changes
roles/kubernetes/node/tasks/kubelet.yml Outdated
@@ -17,7 +17,7 @@
template:
src: "kubelet.env.{{ kubeletConfig_api_version }}.j2"
dest: "{{ kube_config_dir }}/kubelet.env"
setype: "{{ (preinstall_selinux_state == 'enforcing') | ternary('t_etc', omit) }}"
setype: "{{ (preinstall_selinux_state == 'enforcing') | ternary('etc_t', omit) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also set the correct type in permissive mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I changed the condition to != 'disabled'.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is now needed to fix current deployments, but I'm wondering why it was introduced in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://bugzilla.redhat.com/show_bug.cgi?id=1957840 may shed some light.

systemd runs as init_t domain and it is not allowed to read kubernetes_files_t which was recently introduced in Fedora 33/RHEL8.4. The fix was to allow init_t access to kubernetes_files_t but apparently the fix is still not working properly. Because of this we proactively change the context of /etc/kubernetes/kubelet.env to etc_t to allow systemd to read it and apply the environment to the kubelet service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/cc @spaced since you authored the initial change can you help with some extra context?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the != disabled is much better.
and sorry, fucked up with etc_t (our fork has the right value and i did no retest with upstream, shame on me)
I introduced the change because we use enforcing forehand even kubespray does not really support it:

preinstall_selinux_state - Set selinux state, permitted values are permissive and disabled.

however we do not have any other issues except this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@spaced what OS are you using ? And what does your stack look like (containerd / calico / rook / ... ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

@champtar: fedora CoreOS, cri-o and calico

@cristicalin cristicalin force-pushed the kubelet_env_selinux_type branch 2 times, most recently from 74a9f2a to be8626a Compare August 19, 2021 08:35
@cristicalin cristicalin force-pushed the kubelet_env_selinux_type branch from be8626a to 3e85606 Compare August 26, 2021 18:34
@k8s-ci-robot
Copy link
Contributor

@cristicalin: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

floryut
floryut approved these changes Aug 27, 2021
View reviewed changes
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut, spaced

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2021
@oomichi
Copy link
Contributor

oomichi commented Aug 27, 2021

This seems valid based on https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/selinux_users_and_administrators_guide/sect-security-enhanced_linux-working_with_selinux-selinux_contexts_labeling_files

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1afdb05 into kubernetes-sigs:master Aug 27, 2021
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 23, 2021
@cristicalin @LuckySB
Fedora and RHEL use etc_t and the convention is <type_name>_t (kubern…
229208a 
…etes-sigs#7891)

* Fedora and RHEL use etc_t and the convention is <type_name>_t

* Docs: specify all values for preinstall_selinux_state

* CI: Add Fedora 34 with SELinux in enforcing mode
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
@cristicalin @sakuraiyuta
Fedora and RHEL use etc_t and the convention is <type_name>_t (kubern…
e4a4849 
…etes-sigs#7891)

* Fedora and RHEL use etc_t and the convention is <type_name>_t

* Docs: specify all values for preinstall_selinux_state

* CI: Add Fedora 34 with SELinux in enforcing mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@champtar champtar champtar left review comments

@spaced spaced spaced approved these changes

@floryut floryut floryut approved these changes

@holmsten holmsten Awaiting requested review from holmsten

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@oomichi oomichi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubelet can not read kubelet.env if selinux is enforcing
6 participants
@cristicalin @k8s-ci-robot @oomichi @spaced @champtar @floryut