Fix encrypted mount support for ecryptfs v2 wrapping (Issue #3947) #3978
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
|
Thanks for submitting a fix! I wonder if it would be better to use that first line to store the size of the keys, so that we don't have to try to guess it. We can default to "80 80" if nothing is specified. The downside is that people who have encrypted new chroots will have to re-create them (this is ok; they never worked), but people who recently encrypted unencrypted chroots would still be up a creek without a paddle (this is not ok). Also I'm not actually sure what people do with the first line in KEYFILE currently. In theory, if you're storing keyfiles separately, one could use the line to add whatever comments you want, which could be useful. Using it for metadata could mess things up. Really what I should have done from the get-go is store the wrapped keys base64-encoded on separate lines (maybe even with variable names), instead of dumping the binary directly into the file, but too late for that. All considered, I think the current approach is fine, and we should merge it once the syntax is fixed. |
host-bin/mount-chroot
Outdated
| tail -c 80 "$KEYFILE" > "$wrappedfnek" | ||
| wrappedtotal="`expr $(cat "$KEYFILE" | wc -c) - $(head -n 1 "$KEYFILE" | wc -c)`" | ||
| wrappedsize="`expr $wrappedtotal / 2`" | ||
| tail -c $wrappedtotal "$KEYFILE" | head -c $wrappedsize > "$wrappedkey" |
There was a problem hiding this comment.
"$wrappedtotal", "$wrappedsize", etc
host-bin/mount-chroot
Outdated
| tail -c 160 "$KEYFILE" | head -c 80 > "$wrappedkey" | ||
| tail -c 80 "$KEYFILE" > "$wrappedfnek" | ||
| wrappedtotal="`expr $(cat "$KEYFILE" | wc -c) - $(head -n 1 "$KEYFILE" | wc -c)`" | ||
| wrappedsize="`expr $wrappedtotal / 2`" |
host-bin/mount-chroot
Outdated
| @@ -237,8 +237,10 @@ $passphrase" | ecryptfs-wrap-passphrase "$wrappedfnek" - | |||
| addtrap "rm -f '$wrappedkey' '$wrappedfnek'" | |||
|
|
|||
| # Extract wrapped keys from keyfile | |||
| tail -c 160 "$KEYFILE" | head -c 80 > "$wrappedkey" | |||
| tail -c 80 "$KEYFILE" > "$wrappedfnek" | |||
| wrappedtotal="`expr $(cat "$KEYFILE" | wc -c) - $(head -n 1 "$KEYFILE" | wc -c)`" | |||
There was a problem hiding this comment.
$((...)) is preferred over `expr ...`
You can also skip the cat line without causing wc to output the file name by redirecting stdin.
Combined:
"$(($(wc -c < "$KEYFILE") - ...))"
|
Cool, I've pushed those style changes. I think supporting existing .ecryptfs files is important -- I first encountered this when adding encryption to an existing chroot, so having it work following an update would be nice. |
|
Thanks; now we just need to run the encryption test suite on the change. |
|
Apologies for the delay. Thanks for the fix! |
No description provided.