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

switch-root: preserve the whole cred mount tree (/run/credentials/) #32800

Merged
merged 2 commits into from
May 16, 2024

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented May 14, 2024

Currently, during soft-reboot, some services may survive, but their associated credential mounts are dropped. Let's instead preserve them, as discussed.

@github-actions github-actions bot added util-lib please-review PR is ready for (re-)review by a maintainer labels May 14, 2024

This comment was marked as off-topic.

@yuwata
Copy link
Member

yuwata commented May 14, 2024

Could you add a simple testcase?

@yuwata yuwata added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels May 14, 2024
@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels May 14, 2024
@github-actions github-actions bot added the tests label May 14, 2024
@YHNdnzj YHNdnzj added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 14, 2024
@poettering
Copy link
Member

poettering commented May 14, 2024

hmm, so i think passing over the creds mounts that are associated with services that shall also survive the soft reboot is fine. But within the same hierarchy we also maintain a dir with the per-system creds, and I am not convinced we should pass that over always and unconditionally. i.e. I think it might be OK to pass over already acquired creds by default, but not unconditionally. It should be possible to pass a different set of creds to the next soft-reboot than we got on the current boot.

Specifically /run/credentials/@system and /run/credentials/@encrypted should really not be passed over via soft-reboot, however we should probably have something like the existing /run/credentials/@initrd/ that can be used to define creds to import on the subsequent soft reboot instance.

So, let's say we define /run/credentials/@soft-reboot and /run/credentials/@soft-reboot-encrypted as new dirs. If people do want to pass over creds in full, they could simply bind mount/copy/hardlink /run/credentials/@system and /run/credentials/@encrypted to these new dirs. But if they don't want to do that, they could also start a fresh set of dirs there, and just place in it whatever they like.

(i think these two dirs should be different from /run/credentials/@initrd though, i.e. treat switch root and soft-reboot different here)

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 14, 2024
@bluca
Copy link
Member

bluca commented May 14, 2024

So, let's say we define /run/credentials/@soft-reboot and /run/credentials/@soft-reboot-encrypted as new dirs. If people do want to pass over creds in full, they could simply bind mount/copy/hardlink /run/credentials/@System and /run/credentials/@Encrypted to these new dirs. But if they don't want to do that, they could also start a fresh set of dirs there, and just place in it whatever they like.

But why? This just makes it harder and harder to use. What's the point of jumping through those hoops? What advantage does it provide?

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 14, 2024

Specifically /run/credentials/@System and /run/credentials/@Encrypted should really not be passed over via soft-reboot

Well, if you look a bit closer, these aren't introduced by this PR, but in your original PR that introduced soft-reboot...

And there's even an accompanying comment: credentials passed to the system should survive.

Hence I'm not convinced. It seems like a completely different topic, and that contradicts your idea a year ago.

@poettering
Copy link
Member

But why? This just makes it harder and harder to use. What's the point of jumping through those hoops? What advantage does it provide?

our basic concepts should be generic, and cover more than one usecase. I think being able to correctly parameterize a subsequent soft-reboot is essential, just like we want to parameterize a subsequent regular reboot or any other boot.

As I already wrote: I am entirely fine if we by default copy the current creds into the future creds, I just don't think we should do so unconditionally, because that would severely restrict the flexibility on being able to parameterize boots, regardless if soft or other.

hence, i don't see your point regarding "harder to use", because it could by default feel the same.

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 14, 2024

I've been contemplating the "soft-reboot-fresh" mode I mentioned last year (#28545 (comment)). In particular, it shall ignore SurviveFinalKillSignal=yes, and no state is serialized (meaning the counter is dropped). Why? Because when you want to soft-reboot to a completely different installation/different OS, you don't want e.g. journald to pick up the previous soft reboot counter and such, but treat it as a fresh boot.

But still, that's a different topic, which has nothing to do with this PR.

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 14, 2024

In that mode, going with a fresh set of credentials make a ton of sense, but not really for usual soft-reboots, where you're just doing image-based updates or a fast reboot with the same /var/.

@bluca
Copy link
Member

bluca commented May 14, 2024

In that mode, going with a fresh set of credentials make a ton of sense, but not really for usual soft-reboots, where you're just doing image-based updates or a fast reboot with the same /var/.

Exactly, and as you noted already, it's how it currently works and is documented. A flag to clear state can be discussed separately, but the usefulness here is to transition state across, adding breaking changes there now, after it has already shipped, doesn't seem desirable to me.

Currently, during soft-reboot, some services may survive,
but their associated credential mounts are dropped.
Let's instead preserve them, as discussed.
@github-actions github-actions bot removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks needs-rebase ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels May 15, 2024
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label May 15, 2024
@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 15, 2024

As discussed, let's move the discussion on /run/credentials/@soft-reboot/ to another PR, potentially the one that will introduce the soft-reboot-fresh mode.

@poettering
Copy link
Member

i still think we should allow to parameterize soft-reboot iterations properly, and thus differently from the current invocation. Consider this usecase: a mechanism for doing classic package based system upgrades in a clean way: we soft reboot with some parameterization that ensures that we issue something like "dnf distro-sync", and then when done it either soft reboots back into the system, or (if it detects that the kernel got upgraded) does a hard reboot.

Hence, I think soft-reboot is great if you have an image based system, but we shouldn't limit ourselves to that usecase, and instead keep other scenarios in mind as well: parameterizing soft reboots so that they can have slightly different execution environments does make sense to me.

@poettering
Copy link
Member

Or another example: allow me to parameterize a soft-reboot of the system so that we only do storagetm to expose local disks with everything else shut down. and then do another soft-reboot to get back a working system. this would also require some for of parameterization.

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 15, 2024

Hence, I think soft-reboot is great if you have an image based system, but we shouldn't limit ourselves to that usecase, and instead keep other scenarios in mind as well: parameterizing soft reboots so that they can have slightly different execution environments does make sense to me.

I don't disagree. The main argument is that it does not belong to this specific PR, though.

But now that we're targeting v257 anyway, I can probably start working on the fresh mode I mentioned, which additionally allows the user to choose what to clear/keep.

@bluca
Copy link
Member

bluca commented May 15, 2024

Don't we need this bug fix for 256?

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 15, 2024

Don't we need this bug fix for 256?

In that case we really should merge this as-is. But it's up to @poettering I guess.

@bluca
Copy link
Member

bluca commented May 15, 2024

Right now it's broken and we need a fix. We might want to add other modes as you have said, with new options and so on, but that would be material for the next release. But this bugfix should go in.

@bluca bluca added this to the v256 milestone May 15, 2024
@keszybz keszybz added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 16, 2024
@keszybz
Copy link
Member

keszybz commented May 16, 2024

CI is still unhappy. In ci (fedora, rawhide), TEST-82-SOFTREBOOT fails.

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 16, 2024

CI is still unhappy. In ci (fedora, rawhide), TEST-82-SOFTREBOOT fails.

The failure should be #32834, and looking at the journal it happens after the assert_eq added in this PR.

@YHNdnzj YHNdnzj removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 16, 2024
@keszybz
Copy link
Member

keszybz commented May 16, 2024

It seems that the patch is straightforward and fixes the issue that needs to be fixed.

We might want to have additional mode where some credentials are cleared, but that's a much more complicated issue and needs to be discussed separately. Let's merge this.

@keszybz keszybz merged commit b3aa88a into systemd:main May 16, 2024
41 of 44 checks passed
@keszybz keszybz removed the please-review PR is ready for (re-)review by a maintainer label May 16, 2024
@YHNdnzj YHNdnzj deleted the preserve-cred-mounts branch May 16, 2024 10:46
@poettering
Copy link
Member

I still think this is a mistake to do like this because in particular with encrypted creds we want to re-encrypt during the soft reboot transition, since otherwise the keys are not going to be accessible (under the assumption they are locked to boot phase)

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 21, 2024

I still think this is a mistake to do like this because in particular with encrypted creds we want to re-encrypt during the soft reboot transition, since otherwise the keys are not going to be accessible (under the assumption they are locked to boot phase)

Well, I still think you misread this PR - the @system and @encrypted cred dirs are not additionally preserved in this PR, but in the original PR that introduced soft-reboot. I'd be happy to allow users to control what's preserved and what not, but that's a different topic for v257.

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

Successfully merging this pull request may close these issues.

None yet

5 participants