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

create_disk: Enable read-only /sysroot #736

Merged
merged 1 commit into from
Dec 11, 2019
Merged

Conversation

cgwalters
Copy link
Member

Let's opt-in to this by default.
See ostreedev/ostree#1265

This currently is a no-op if the required ostree support hasn't
landed yet, so I think we can safely merge this PR first.

@cgwalters
Copy link
Member Author

After the ostree/rpm-ostree PRs land, we need another PR here to use Options=ro for boot.mount and boot-efi.mount.

cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Sep 5, 2019
Add the immutable bit to the physical root; we don't
expect people to be creating anything there.  A use case for
OSTree in general is to support installing *inside* the existing
root of a deployed OS, so OSTree doesn't do this by default, but
we have no reason not to enable it here.  Administrators should
generally expect that state data is in /etc and /var; if anything
else is in /sysroot it's probably by accident.

I just happened to think of this while working on
coreos#736
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM

cgwalters added a commit that referenced this pull request Sep 5, 2019
Add the immutable bit to the physical root; we don't
expect people to be creating anything there.  A use case for
OSTree in general is to support installing *inside* the existing
root of a deployed OS, so OSTree doesn't do this by default, but
we have no reason not to enable it here.  Administrators should
generally expect that state data is in /etc and /var; if anything
else is in /sysroot it's probably by accident.

I just happened to think of this while working on
#736
@jlebon
Copy link
Member

jlebon commented Sep 9, 2019

Let's hold this until we finalize the semantics in the OSTree patch?

Let's opt-in to this by default.
See ostreedev/ostree#1265

This currently is a no-op if the required ostree support hasn't
landed yet, so I think we can safely merge this PR first.
@jlebon
Copy link
Member

jlebon commented Dec 11, 2019

This LGTM. OOC if you've got this already built locally, does this indeed plug the restorecon -R / footgun? Hmm, I guess a follow-up here is having /boot be mounted read-only too, right?

@jlebon
Copy link
Member

jlebon commented Dec 11, 2019

Hmm, I guess a follow-up here is having /boot be mounted read-only too, right?

Although we don't need that one the same way we need /sysroot read-only which protects us from object corruption. We should probably discuss /boot separately.

@cgwalters
Copy link
Member Author

We should probably discuss /boot separately.

Yep indeed. It's a trivial change in fedora-coreos-config. But...there are legitimate reasons for an admin or tools to e.g. vi /boot/loader/foo.conf.

I think I'd like to do it anyways, since rpm-ostree kargs will continue to work, but it's a separate thing for sure, and we need to double-check that at least our tools are OK with it.

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.

Nice work on this!

Note we need to make sure the ostree and rpm-ostree patches land in FCOS at the same time.

@jlebon jlebon merged commit 26645d8 into coreos:master Dec 11, 2019
@jlebon jlebon removed the hold waiting on something label Dec 11, 2019
@cgwalters
Copy link
Member Author

Hm sorry I should have clarified my idea was to land the rpm-ostree change before this one so we didn't need to do it all at once.

@cgwalters
Copy link
Member Author

But I think we can probably just merge that now?

cgwalters added a commit to cgwalters/fedora-coreos-config that referenced this pull request Apr 17, 2020
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.
cgwalters added a commit to cgwalters/fedora-coreos-config that referenced this pull request Apr 17, 2020
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.
jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Aug 6, 2021
It's not read by coreos-assembler, which has been turning on read-only
`/sysroot` unconditionally for a while now:

coreos/coreos-assembler#736
dustymabe pushed a commit to coreos/fedora-coreos-config that referenced this pull request Aug 6, 2021
It's not read by coreos-assembler, which has been turning on read-only
`/sysroot` unconditionally for a while now:

coreos/coreos-assembler#736
HuijingHei pushed a commit to HuijingHei/fedora-coreos-config that referenced this pull request Oct 10, 2023
It's not read by coreos-assembler, which has been turning on read-only
`/sysroot` unconditionally for a while now:

coreos/coreos-assembler#736
HuijingHei pushed a commit to HuijingHei/fedora-coreos-config that referenced this pull request Oct 10, 2023
It's not read by coreos-assembler, which has been turning on read-only
`/sysroot` unconditionally for a while now:

coreos/coreos-assembler#736
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.

3 participants