install-multi-user: chown bootstrap store contents #7442
Conversation
winterqt
force-pushed
the
chown-bootstrap-store
branch
from
dea1cdc
to
a4ade45
Compare
winterqt
force-pushed
the
chown-bootstrap-store
branch
from
a4ade45
to
3058222
Compare
winterqt
requested review from
abathur
and removed request for
edolstra and
thufschmitt
|
Oh, so that... removes code owner review requests when that usually isn't possible... 🙃 |
|
I don't feel like I have a handle on the high-level correctness of the change, but we can start by setting up testing as a backstop. If you follow the steps in https://nixos.org/manual/nix/stable/contributing/hacking.html#installer-tests it'll enable the installer test job in your fork's GA workflow. It's not a full test suite, but it at least confirms that the happy-path isn't broken. It'll also be possible to use the x86_64-darwin installer it generates outside of the workflow to confirm. |
|
I manually tested the change before I submitted this PR, and it worked as expected. I'll setup the tests in a few hours, though, and we can go from there. |
This isn't a Darwin specific issue, I was also able to reproduce this on Linux. |
|
Sorry--jumping to conclusions since most of my installer work is for macOS fixes. It'll generate an |
|
Yup, was just making sure you were aware that this affects both Darwin and Linux. I've tested that this works on aarch64-darwin (thanks to Virtualization.framework), and I'll generate an installer for aarch64-linux and test there. (Maybe we can even add this as a case for installer_test, that is, making sure the store permissions are setup correctly?) (Side note, but we should probably enable aarch64-linux cross builds for the installer workflow... I'll test + do that in another PR :) |
winterqt
force-pushed
the
chown-bootstrap-store
branch
from
3058222
to
958efd4
Compare
|
@abathur Installer tests succeeded: https://github.com/winterqt/nix/actions/runs/3671329069 |
| @@ -810,6 +810,24 @@ install_from_extracted_nix() { | |||
| _sudo "to make the new store non-writable at $NIX_ROOT/store" \ | |||
| chmod -R ugo-w "$NIX_ROOT/store/" | |||
|
|
|||
| # This is copied from create_directories, see it for why we do all this stuff just to find chown. | |||
There was a problem hiding this comment.
Instead of copying, this could be factored out into a function? Or just moved up in the script so it's done only once?
There was a problem hiding this comment.
See #7442 (comment) for @abathur and I's previous discussion on this.
Before this change, the bootstrap store contents would be owned by the user who ran the script. This leads to inconsistencies in store path permissions, and can cause confusing issues for users (an example being zsh's compaudit being fired when attempting to use the completions from the bootstrap copy of Nix).
winterqt
force-pushed
the
chown-bootstrap-store
branch
from
958efd4
to
cb768c5
Compare
| EOF | ||
| else | ||
| _sudo "to change ownership of Nix store files" \ | ||
| "$get_chr_own" -R "root:$NIX_BUILD_GROUP_NAME" "$NIX_ROOT/store" || true |
There was a problem hiding this comment.
Should this use $NIX_ROOT/store/ like the commands above, does it really make a difference in this case? (I think we e.g. still want /nix/store to be owned by root:nixbld, so I don't think any special casing surrounding trailing versus no trailing slash should matter practically...)
|
@abathur Do you have any opinion on this? Maybe instead of doing a |
|
I said I wasn't sure about high-level correctness before, and I guess that's because I'm a little perplexed about why it's only showing up now if this has been happening on normal installs in the wild. It seems like anything running later under the installing user being able to fiddle with the bootstrapped files is probably Bad, so I'd be surprised if it went unnoticed for years and years, which makes me feel like there's an unknown variable somewhere. I guess an explanation that would make sense is if this shifted as a result of #5150 Idiomatically, this addition mirrors the chmod to disable write permissions above it. If the rsync -> cp change above did cause ownership to shift, I guess the ownership of the unpacked files may not get preserved during the recursive copy anyways? I can't fiddle around with the commands to test that thesis just now. Maybe this evening. |
|
Though the @iFreilicht did you happen to notice the installing user owning any store files while working through #7603? |
|
Got out a spare laptop to test. I do see this behavior on modern installers. Can verify with something like The first release with the rsync -> cp change from 5150 was 2.4, and the behavior is present there. I do not see the behavior on 2.3.16. I think this is because the |
I did not, but I didn't look particularly closely at that. The issues I fixed were not caused by an ownership issue.
That would be a great addition to the installer-test to prevent a regression in this matter. |
Before this change, the bootstrap store contents would be owned by the user who ran the script.
This is not ideal, and leads to confusing issues like zsh's compaudit being fired.