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

Jump to bottom

BZ 1956836: overlay: Add rhcos-usrlocal-selinux-fixup.service #551

Merged
merged 1 commit into from May 21, 2021
Merged

BZ 1956836: overlay: Add rhcos-usrlocal-selinux-fixup.service #551

merged 1 commit into from May 21, 2021

Conversation

cgwalters
Copy link
Member

A fix is inbound for policy, but we really should fixup existing
systems in place.

@openshift-ci openshift-ci bot requested review from jlebon and miabbott May 14, 2021 18:22
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2021
@jlebon
Copy link
Member

jlebon commented May 14, 2021

Hmm, I think we need this and also temporarily carry https://src.fedoraproject.org/rpms/selinux-policy/pull-request/24 until it makes it into RHEL8. Something like:

diff --git a/manifest.yaml b/manifest.yaml
index 4205ec4..30d74f9 100644
--- a/manifest.yaml
+++ b/manifest.yaml
@@ -163,6 +163,18 @@ postprocess:
      # NB: we don't use -f here so we break when this is no longer needed
      rm -v /etc/iscsi/initiatorname.iscsi

+  # Carry https://src.fedoraproject.org/rpms/selinux-policy/pull-request/24
+  # until it gets into RHEL8. Tracked at https://bugzilla.redhat.com/show_bug.cgi?id=1943381.
+  - |
+    #!/usr/bin/env bash
+    set -xeuo pipefail
+
+    f=/etc/selinux/targeted/contexts/files/file_contexts.subs_dist
+    if ! grep /var/usrlocal $f; then
+      echo "# https://src.fedoraproject.org/rpms/selinux-policy/pull-request/24" >> $f
+      echo "/var/usrlocal /usr/local" >> $f
+    fi
+
 etc-group-members:
   - wheel
   - sudo

?

This should also allow you to simplify the systemd service to just do restorecon -rv /var/usrlocal. Testing locally (after applying the change above live, not from a new compose):

[root@cosa-devsh ~]# echo foobar > /usr/local/sbin/foobar
[root@cosa-devsh ~]# chmod a+x /usr/local/sbin/foobar
[root@cosa-devsh ~]# restorecon -rv /var/usrlocal
Relabeled /var/usrlocal from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/etc from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/games from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/include from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/lib from system_u:object_r:var_t:s0 to system_u:object_r:lib_t:s0
Relabeled /var/usrlocal/man from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/sbin from system_u:object_r:var_t:s0 to system_u:object_r:bin_t:s0
Relabeled /var/usrlocal/sbin/foobar from unconfined_u:object_r:var_t:s0 to unconfined_u:object_r:bin_t:s0
Relabeled /var/usrlocal/share from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0
Relabeled /var/usrlocal/src from system_u:object_r:var_t:s0 to system_u:object_r:usr_t:s0

@travier
Copy link
Member

travier commented May 17, 2021 •
edited

This should also allow you to simplify the systemd service to just do restorecon -rv /var/usrlocal. Testing locally (after applying the change above live, not from a new compose):

Relabeled /var/usrlocal/sbin/foobar from unconfined_u:object_r:var_t:s0 to unconfined_u:object_r:bin_t:s0

To fix the (SELinux) user part too with restorecon you need -F.

@cgwalters
Copy link
Member Author

OK wow I had thought systemd-tmpfiles did nothing if the file exists, but it does seem to always reset the security context each boot, plus if e.g. I do chown bin:bin /usr/local/sbin && reboot then it gets reset back to root:root. Did you come across this before?

Mmmm. Locally changing policy feels riskier than just having this service run every time.

@cgwalters cgwalters marked this pull request as draft May 17, 2021 17:16
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
@cgwalters cgwalters marked this pull request as ready for review May 17, 2021 17:25
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
@cgwalters
Copy link
Member Author

OK reworked to always restorecon /var/usrlocal/sbin, but still only do the recursive relabel once.

@cgwalters
Copy link
Member Author

The unit test doesn't cover actually injecting binaries via Ignition right now, but I extensively tested this manually too.

jlebon
jlebon approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Minor comments, but LGTM generally!

overlay.d/05rhcos/usr/lib/systemd/system/rhcos-usrlocal-selinux-fixup.service Outdated Show resolved Hide resolved
tests/kola/misc-ro/misc-ro.sh Outdated Show resolved Hide resolved
tests/kola/misc-ro/misc-ro.sh Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member Author

Squashed the suggestions (thanks!) and updated Documentation=

@travier
Copy link
Member

travier commented May 18, 2021

OK wow I had thought systemd-tmpfiles did nothing if the file exists, but it does seem to always reset the security context each boot, plus if e.g. I do chown bin:bin /usr/local/sbin && reboot then it gets reset back to root:root. Did you come across this before?

I had a similar issue earlier: coreos/ignition#1156

travier
travier reviewed May 18, 2021
View reviewed changes
tests/kola/misc-ro/misc-ro.sh Outdated
@@ -133,6 +133,21 @@ echo "ok iSCSI initiator name"
systemctl is-enabled logrotate.timer
echo "ok logrotate"

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a broken merge 🙂

@cgwalters
BZ 1956836: overlay: Add rhcos-usrlocal-selinux-fixup.service
fb007ee 
A fix is inbound for policy, but we really should fixup existing
systems in place.
@jlebon
Copy link
Member

jlebon commented May 19, 2021

INFO[2021-05-19T16:25:48Z] error: Packages not found: audit, authselect, bash-completion, bind-utils, bsdtar, bzip2, chrony, cifs-utils, cloud-utils-growpart, compat-openssl10, conntrack-tools, coreutils, cryptsetup, cryptsetup-reencrypt, device-mapper-multipath, dhcp-client, dnsmasq, e2fsprogs, efibootmgr, git-core, glusterfs-fuse, gnupg2, grub2, grub2-efi-x64, gzip, hostname, iproute, iproute-tc, iptables, irqbalance, iscsi-initiator-utils, kernel, kernel-core, kernel-modules, kernel-modules-extra, kexec-tools, less, logrotate, lvm2, mdadm, microcode_ctl, nano, net-tools, nfs-utils, nftables, nmap-ncat, open-vm-tools, openssh-clients, openssh-server, passwd, perl-interpreter, qemu-guest-agent, rdma-core, rpm-ostree, rsync, selinux-policy-targeted, sg3_utils, shadow-utils, shim, socat, sssd, stalld, strace, subscription-manager-rhsm-certificates, sudo, systemd, systemd-journal-gateway, systemd-journal-remote, tar, teamd, tmux, tpm2-tools, vim-minimal, xfsprogs, xz

Hmm, something to do with the recent repo changes I suppose? /cc @travier

@jlebon
Copy link
Member

jlebon commented May 19, 2021

/lgtm
for PR itself!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

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

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@cgwalters
Copy link
Member Author

/hold
until repos are fixed

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2021
@cgwalters
Copy link
Member Author

/retest

@travier
Copy link
Member

travier commented May 20, 2021

Blocked by openshift/release#18691 🙁

@travier
Copy link
Member

travier commented May 20, 2021

Should be good to go now but maybe a full retest would be best

@miabbott
Copy link
Member

/retest

@miabbott
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2021
@openshift-merge-robot openshift-merge-robot merged commit a721b30 into openshift:master May 21, 2021
@cgwalters cgwalters mentioned this pull request Jun 14, 2021
Merged
cgwalters added a commit to cgwalters/machine-config-operator that referenced this pull request Jun 16, 2021
@cgwalters
templates: Rework node-valid-hostname to fix SELinux denial
783aa2c 
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1956836

Unfortunately right now, executing `bash` skips a domain transition
(see fedora-selinux/selinux-policy#778)
so the way we're sourcing the script means we stay in `initrc_t`
and end up triggering a SELinux policy denial.

(BTW this denial turns out to just delay the successful exit
 of the script, which will then end up just delaying kubelet start.
 it's otherwise harmless, but we also don't want SELinux policy denials
 in our product by default)

Fix this in two ways:

- First, just move the thing to `/usr/local/bin` to avoid issues
  with labeling of `/usr/local/sbin` that were fixed in openshift/os#551
- Second, rework it to be executed directly

While we're here:

- Clean the confusing+outdated comment about being a NM dispatcher
- Drop the `logger` bit which was only necessary as a NM dispatcher;
  since we're *always* running under systemd, this makes `journalctl -u node-valid-hostname`
  actually show the script output.
- Make it crystal clear that the "truncate hostname" is only run in GCP.
- Fix various typos
- Use the more precise term "non-localhost" in various places instead of the more ambiguous
  terms "real"/"valid"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travier travier travier left review comments

@jlebon jlebon jlebon approved these changes

@miabbott miabbott Awaiting requested review from miabbott

Assignees

@jlebon jlebon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@cgwalters @jlebon @travier @openshift-bot @miabbott @openshift-merge-robot