-
Notifications
You must be signed in to change notification settings - Fork 166
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
Conversation
After the ostree/rpm-ostree PRs land, we need another PR here to use |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
Let's hold this until we finalize the semantics in the OSTree patch? |
8781424
to
0196576
Compare
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.
0196576
to
368495f
Compare
This LGTM. OOC if you've got this already built locally, does this indeed plug the |
Although we don't need that one the same way we need |
Yep indeed. It's a trivial change in fedora-coreos-config. But...there are legitimate reasons for an admin or tools to e.g. I think I'd like to do it anyways, since |
There was a problem hiding this 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.
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. |
But I think we can probably just merge that now? |
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.
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.
It's not read by coreos-assembler, which has been turning on read-only `/sysroot` unconditionally for a while now: coreos/coreos-assembler#736
It's not read by coreos-assembler, which has been turning on read-only `/sysroot` unconditionally for a while now: coreos/coreos-assembler#736
It's not read by coreos-assembler, which has been turning on read-only `/sysroot` unconditionally for a while now: coreos/coreos-assembler#736
It's not read by coreos-assembler, which has been turning on read-only `/sysroot` unconditionally for a while now: coreos/coreos-assembler#736
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.