-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
darwin: encrypt nix volume if filevault is enabled #4289
Conversation
Thanks once again for this work and the attention to detail. Does anyone know: on T2 Macs, if you encrypt a user-created APFS volume, is the encryption/decryption for that volume still done on the T2, as with the system volumes; or is it done in software on the CPU? |
I think it's on the T2, but I won't swear I personally know this as a fact. For example, see the last line below:
If you encrypt a volume on a pre-T2 mac, that line will say |
That's also how I understood the T2 documentation from apple https://www.apple.com/jp/mac/docs/Apple_T2_Security_Chip_Overview.pdf. The chip handles all the encryption and is actually always enabled, FileVault only changes the conditions for which the chip unlocks the data. Meaning that the data also doesn't have to change when updating credentials. Which the behaviour you noticed seems to confirm is the case. |
In case anyone's curious, I added screenshots of the ~interactive UI/X elements in a comment alongside the code for each (and immediately resolved them, so they don't take up gobs of space in the main thread). |
For future reference, you can use <details><summary>Description of screenshot</summary>
// drag image into this blank line, if it's surrounded by blank lines the markdown ![image](url) syntax will work just fine.
</details> This will show up as Collapsed summary hereOnce expanded the image will be visible. |
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.
This is only a partial review, I’ll have to come back to this later.
I've pushed commits addressing one round of comments from @lilyball and @SuperSandro2000 (also thanks to @samueldr and @jtojnar for additional discussion). Going to go ahead and convert this from a draft. I've started a build of this locally, and will update to confirm its status (status: my install checks are working as of 6bb3bc9, but the Nix repo checks failed at this commit--I'd be surprised if this caused that, so I assume it's a transient failure or flakiness?) For anyone champing at the bit, I suspect we'll be holding up just a little bit (if it's a short wait) for the chance I'll be able to help @domenkozar test a new GitHub Actions integration per #4047 that should make it easier to test PRs that touch the installer. In any case, reviews still welcome in the meantime. :) |
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.
Another partial review. Apologies for not managing to do a full one.
@@ -1,33 +1,235 @@ | |||
#!/bin/sh | |||
set -e | |||
#!/usr/bin/env bash |
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.
I'm not sure if this really matters, but maybe we should stick with /bin/bash
just to ensure we're using the exact same version of bash everywhere?
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.
Good question.
I just adopted the shebang from install-multi-user.sh
for consistency.
I'm not sure how to weight consistency with install-multi-user against being explicit here. Everything install-multi-user.sh
sources has to be 3.2 compatible for this reason (and I did a lot of manual 3.2 testing across all of the scripts as I went).
I assumed/inferred from the #!/usr/bin/env bash
shebang on install-multi-user that at least some of the platforms we might be run on don't have a /bin/bash, but if that's not true I suppose they could all change.
What do you think?
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.
I suppose consistency is worthwhile. If we do need to run install-multi-user.sh
on a platform that doesn't have /bin/bash
then the current shebang makes sense.
ca42caf
to
bf5290d
Compare
bf5290d fixes an oversight caught by lilyball regarding curing/checking for the presence of Nix as a symlink in synthetic.conf. You can see sample output in this CI run: The stitched jobs fail because once we've stitched, we need a reboot to remove the symlink. |
@abathur Thanks a lot for your work on this! Is there an ETA for when this will be made publicly available? |
Not sure. We're on the crowded side of a bottleneck. :) |
I've seen good success with this PR and I think it is time to give our users a better experience, so I'm going to go ahead and merge it :) |
@abathur Thank you very much for your work! Am I right that these changes only affect nixUnstable and upcoming releases? If so, is there a chance for a 2.3.x backport? |
@jgeerds Yep. Backport decisions are above my paygrade, but unless 2.4 is impending I think it spares enough pain to be worth a backport and release. |
This may also be a good place to note: I think the merge of this PR lights a path to full uninstall/reinstall. I can't pursue it any time soon, but I'm happy to discuss as needed if someone else can. I wrote a proof-of-concept in Oct. that should be reheatable. Validation is also much easier since domen added installer tests. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Holy smokes; what a great install experience. This worked for me on a macOS early 2013 running |
@abathur Sorry to bother: how will we know when this will become the default installer for Darwin? This is what I'm telling coworkers currently: sh <(curl https://abathur-nix-install-tests.cachix.org/serve/yihf8zbs0jwph2rs9qfh80dnilijxdi2/install) --tarball-url-prefix https://abathur-nix-install-tests.cachix.org/serve |
@rpearce No worries. Subscribing to this thread is probably the best bet. I deferred a sprawling, intricate refactor of another project for a few months to focus on this PR. For my own sanity, I had to stick a fork in this a couple months back so that I could shift enough of my focus back to that effort to make any headway. I guess we're waiting for either:
It can be backported (and I agree with releasing it), but there are a few caveats that probably make it more complicated than just bugging domen:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/anyone-up-for-picking-at-some-nix-onboarding-improvements/13152/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-install-nix-on-macos-with-m1/13791/4 |
An issue was just reported to IHP with the following error running this installer:
Was anything changed on the installer? Did it already get released and @abathur's installer is deprecated? Linked issue: digitallyinduced/ihp#960 |
Most likely a transient network error, the hash is correct when I test it locally. |
Thanks for the quick help Domen! :) The user reported back that the problem still occurs. Notable it's happening on a Macbook Air with a M1. Maybe this could be causing the problems? |
The installer that's used by IHP doesn't support M1, I'd suggest to use the newest one from Nix master or use the default installer that ships M1 support as of yesterday. |
Thanks, using the default installer solved the problem :) |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/what-is-the-recommended-installer-to-use-macos/15138/1 |
This refactors WIP from #4181 to address things that became obvious:
The history had grown unruly even before this refactor, so for now I'm flattening this into an omnibus commit to start fresh. Since this touches on a lot, I've tried to better-document the high-altitude view by making three outlines:
the general process/logic in pseudocode (as-is, and for the uninstaller I'm imagining)removed this for mergeHigh-level functionality changes
--darwin-use-unencrypted-nix-store-volume
flag is now a no-op warning[nN]ix.*
(see [OSX catalina] install script fails if a volume starting with [nN]ix.* already exists #4150)Lower-level implementation changes
/nix
already exists,chown -R root:nixbld
itinstall -dv -m
instead ofmkdir -pv -m
-g
(group) flag/usr/bin/mktemp -d -t tmp.XXXXXXXXXX --> /.../tmp.XXXXXXXXXX.2RMrVwkx
/usr/bin/mktemp -d "${TMPDIR:-/tmp/}tmp.XXXXXXXXXX" --> /.../tmp.JwmZDxN1kf
<run very slow diskutil command> | <parse a single bit of info out with an xpath query>
, which is OK in isolation but was becoming a bit too common@LnL7 @lilyball @matthewbauer @dhess @mroi @domenkozar (others I'm forgetting are inevitably also interested; feel free to add people)
Try it out
I have been hosting my own installers for much of the history of this PR. Nix now has a better installer-testing process, so I'm going to see if we can lean on it here. I'll leave the old process note here for a bit in case there are problems with the new approach. If you can't try it out, you can see output from the CI run.
The new approach requires an extra flag, so it'll be easiest to just use in interactive mode:
sh <(curl https://abathur-nix-install-tests.cachix.org/serve/yihf8zbs0jwph2rs9qfh80dnilijxdi2/install) --tarball-url-prefix https://abathur-nix-install-tests.cachix.org/serve
Here's a history of the versions it has pointed at, with the current at the bottom (note: I'll only update this for substantive changes or breakage):
6132132(equivalent to 401afa0, but without the reference darwin_volume_reference.py)6bb3bc90bd108386dbee797269f6c235bedold privately-hosted installer (out of date--use only if above doesn't work)
I have a temporary installer up at https://files.t-ravis.com/install-multi, so
curl https://files.t-ravis.com/install-multi | sh
(headless) andsh <(curl https://files.t-ravis.com/install-multi)
(interactive) should work.Other issues
I believe this PR: