openwall-com-au / BootUnlock

A helper script that unlocks macOS'es encrypted APFS volumes before login
GNU General Public License v3.0
51 stars 7 forks source link

Repeat unlock 60 times #5

Closed netj closed 5 years ago

netj commented 5 years ago

I noticed not all my (encrypted) external drives show up at the first diskutil apfs list and therefore most of my reboots failed to unlock and mount home drive. This PR addresses the issue in a (pretty ugly but) simple way by trying the unlock 60 times waiting for a second after each attempt.

Also due to set -o pipefail when grep does not find any encrypted drives the script was aborting which has been suppressed by || true

(bash nit: ending a line with | makes the backslashes at the end of every line unnecessary. But I didn't want to make this PR's diff appear complicated.)

galaxy4public commented 5 years ago

Thanks for submitting the PR. I am evaluating it. I am not sure that I like the approach since basically it is still a race condition. Most likely, I will leverage launchd condition for detecting volumes - this way the unlock will work even if you attach a volume much later.

Re: bash long lines, yes, that is a conscious choice to use backslashes over there.

Re: || true, ||: is shorter and does not execute a binary :). Anyway, I think that instead of suppressing the script should be launched when there is work to do.

Give me a couple of days and I will fix it one way or another.

netj commented 5 years ago

I’m totally with you that a more proper solution is needed. Meanwhile I just needed something working right now for the drive I happened to have just encrypted for my new Mac and this works for me.

I think I know what you mean but I have to say this won’t create any race b/c nothing really runs in parallel but only create limited functionality that works for drives found for the first minute after boot up compared to launchd that can work very cleanly throughout the entire uptime.

And I still would strongly recommend against backslash followed by newline then pipe. You can simply put pipes before breaking the lines. For wrapping a long command I think backslashes are okay but there are also ways to avoid it:

longArgs=(
a b c
New lines
And white space 
Delimited
d e f
)
printf '%s\n' some args here "${longArgs[@]}" there

(coming from writing many dozen thousands of lines of bash if not hundred thousand over a decade or two and from watching how everyone agonize with backslashes... but you may have good reasons I'd love to learn about.)

You have a great point about ||: not executing file though. I'll have to switch my liberal use of true/false if experiments show there's a significant difference. I know bash does some optimizations for many pseudo builtin posix commands. I hope true/false are also included. Thanks for the tip!

And really appreciate it for putting all this together in a readily usable form.

galaxy4public commented 5 years ago

@netj, instead of your changes to the helper script, could you please try to replace KeepAlive with StartOnMount in the .plist (https://github.com/openwall-com-au/BootUnlock/blob/master/scripts/postinstall#L36)?

My tests are showing that it should address the issue you were facing, but I just wanted to confirm that it solves it for you too.

galaxy4public commented 5 years ago

@netj, I have just released a version that should address the issue and extends the use case even further. Please try the new version and I would appreciate any feedback whether it helped on not.

netj commented 5 years ago

@galaxy4public I tried two reboots with 1.1.0 StartOnMount and both attempt so far appears to work smoothly. Glad it's now working in the right way!