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

overlay/mounts: Mount /boot and /boot/efi ro,nodev,nosuid #356

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

Ironically ostree has had support for a ro boot for a long time,
and only more recently did we land the sysroot readonly.

But we never actually went and made /boot ro for FCOS, so let's
do it now.

This was actually motivated by someone wanting to "security harden" RHCOS
running through a checklist saying certain mounts should be nodev.
Let's add nosuid while we're here.

Ironically ostree has had support for a `ro` boot for a long time,
and only more recently did we land the [sysroot readonly](coreos/coreos-assembler#736).

But we never actually went and made `/boot` `ro` for FCOS, so let's
do it now.

This was actually motivated by someone wanting to "security harden" RHCOS
running through a checklist saying certain mounts should be `nodev`.
Let's add `nosuid` while we're here.
Comment on lines +2 to +20
set -euo pipefail
systemctl is-enabled logrotate.service
echo "ok logrotate"

validate_not_writable_mount() {
local mnt=$1
shift
findmnt "${mnt}" -o OPTIONS | grep -q ro
if test -w "${mnt}"; then
echo "writable ${mnt}"
exit 1
fi
echo "ok not writable ${mnt}"
}

validate_not_writable_mount /boot
if test -d /boot/efi; then
validate_not_writable_mount /boot/efi
fi
Copy link
Member

Choose a reason for hiding this comment

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

should this go in a separate file?

Copy link
Member

Choose a reason for hiding this comment

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

i.e. a different test and maybe add some text about why we have this test

Copy link
Member Author

Choose a reason for hiding this comment

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

But then we end up booting a whole other VM for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

(What we really want are nondestructive tests and teach kola to reuse existing VMs for them)

Copy link
Member

Choose a reason for hiding this comment

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

yeah ok that's a good reason to keep it in the same file. Can we add some text at the top about an RFE for nondestructive tests (open issue somewhere?) and then we can break these out once that is in place,

It would still be nice to have a comment at the top of this particular test you are adding about
# in coreos-boot-mount-generator we mount boot readonly, verify it is readonly.

@dustymabe
Copy link
Member

If we mount boot read only we probably need to delete at least:

# If we make it to the realroot (successfully ran ignition) then
# clean up the files in the firstboot network dir
echo "R ${realroot_firstboot_network_dir} - - - - -" > \
/run/tmpfiles.d/15-coreos-firstboot-network.conf

@cgwalters
Copy link
Member Author

If we mount boot read only we probably need to delete at least:

Hmm; and have the files leak? We already have ignition-firstboot-complete.service which is remounting writable (privately/temporarily) too from the real root. Maybe we should coalesce the "clean up stuff from /boot" there.

@dustymabe
Copy link
Member

Maybe we should coalesce the "clean up stuff from /boot" there.

SGTM

@cgwalters
Copy link
Member Author

Closing in favor of #659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants