I've already started some very initial work on [u]mount.crypt, I'm going to start to publish my changes in my fork, but it's quite early stage, so it can be buggy and I can sometimes do force pushes. I would really appreciate a review
I've made more patches. Some of them are just minor refactoring/fixes, but a few commits are rdconf updates. This works perfectly on my laptop, but I still have some questions:
Realpath. Shouldn't I get the header realpath here? Do we need it for mtab? What is the correct way to get it using HX library?
Memory management. I'm not very familiar with the project structure and the C language in general, so I could leave a leak somewhere. Foremost, I'm interested in this and this. It seems that the correct type of ehd_mount_info::header should be char and not hxmc_t, then we can invoke free(mt->container);, but I'm not sure about that.
The pmt-ehd is totally new to me. Should I add support for it?
A crypttab entry. Should we use crypttabhere? (I don't see crypttab any mention at all)
Remount support. I haven't tested the remount yet, but I'm pretty sure it's broken. I'll try to do it later...
If anyone wants to review (It would be wonderful!), commits have been created with an expectation --autosquash to be done before PR, so you can squash them before a review:
As I can see, the remount option works correctly even without any additional fixes due to the actual implementation doesn't really reopen the luks container but remounts filesystem only. It does not allow you to change such options as crypto_name or allow_discards, but I think it is out of scope of this PR
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I incorporated b6dbd44, with some WS and spellos addressed.
Realpath
When a path is fed into a system routine and later retrieved again. The equivalency between /tmp/../mnt and /mnt needs to be taken into account when comparing strings. This is generally limited to the source device and the mountpoint directory. The LUKS header path is never read from /etc/mtab|/proc/mounts again, is it..
(e.g. mount /dev/./sda1 /tmp/../mnt is likely to get resolved to /dev/sda1 and /mnt respectively by the mount(2) syscall).
@jengelh, thanks a lot! I've already added a header support to pmt-ehd (mainly because the previous changes broke it and I needed to do some research about how pmt-ehd works). And it seems, realpath is really needed... but in pmt-ehd (when a user passes the same path to header and container arguments)
I've already added realpath to mtcrypt.c to just make it more consistent with other paths and I will add it soon to ehd.c, remove mention of crypttab, add some documentation changes. And then, I'm going to make a PR
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think the PR is ready. The manual tests I made on each new commit:
creating an image without a detached header
mounting this image with mount.crypt
umounting
After the pmt-ehd: add support for detached headers, the following tests:
creating an image with a detached header
mounting this image with mount.crypt (manually and on login)
creating an image with exactly the same path to the image and header
mounting the previous image with -oheader=... and without it, both work correctly
umounting
If we create an image with lexicographically different paths to the image and header, but when both are the same file, before pmt-ehd: resolve absolute file names it expectedly creates an invalid image. For some reason, I've decided to commit the fix separate from the preceding commit, but, of course, I can squash them before the PR is merged
The key changes from b6dbd44, I've done:
updated documentation
allowed headers to be NULL in data structures (I think it's an easier way to manage memory)
Initial changes already can be found here: branch, diff
Last edit: oficsu 2022-06-13
I've made more patches. Some of them are just minor refactoring/fixes, but a few commits are rdconf updates. This works perfectly on my laptop, but I still have some questions:
Realpath. Shouldn't I get the header realpath here? Do we need it for
mtab? What is the correct way to get it using HX library?Memory management. I'm not very familiar with the project structure and the C language in general, so I could leave a leak somewhere. Foremost, I'm interested in this and this. It seems that the correct type of
ehd_mount_info::headershould becharand nothxmc_t, then we can invokefree(mt->container);, but I'm not sure about that.The
pmt-ehdis totally new to me. Should I add support for it?A
crypttabentry. Should we usecrypttabhere? (I don't see crypttab any mention at all)Remount support. I haven't tested the remount yet, but I'm pretty sure it's broken. I'll try to do it later...
If anyone wants to review (It would be wonderful!), commits have been created with an expectation
--autosquashto be done before PR, so you can squash them before a review:As I can see, the remount option works correctly even without any additional fixes due to the actual implementation doesn't really reopen the luks container but remounts filesystem only. It does not allow you to change such options as
crypto_nameorallow_discards, but I think it is out of scope of this PRI incorporated b6dbd44, with some WS and spellos addressed.
When a path is fed into a system routine and later retrieved again. The equivalency between
/tmp/../mntand/mntneeds to be taken into account when comparing strings. This is generally limited to the source device and the mountpoint directory. The LUKS header path is never read from /etc/mtab|/proc/mounts again, is it..(e.g.
mount /dev/./sda1 /tmp/../mntis likely to get resolved to/dev/sda1and/mntrespectively by the mount(2) syscall).Looks consistent.
char *: calloc / freehxmc_t *: HXmc_* / HXmc_freeLeaning towards no. crypttab is for boot-time volumes, or rather, static system volumes.
You can, but it probably isn't worth the time. The utility existed/exists to facilitate plain dm-crypt volumes when LUKS was new.
@jengelh, thanks a lot! I've already added a header support to pmt-ehd (mainly because the previous changes broke it and I needed to do some research about how pmt-ehd works). And it seems, realpath is really needed... but in pmt-ehd (when a user passes the same path to header and container arguments)
I've already added realpath to
mtcrypt.cto just make it more consistent with other paths and I will add it soon toehd.c, remove mention of crypttab, add some documentation changes. And then, I'm going to make a PRI think the PR is ready. The manual tests I made on each new commit:
mount.cryptAfter the
pmt-ehd: add support for detached headers, the following tests:-oheader=...and without it, both work correctlyIf we create an image with lexicographically different paths to the image and header, but when both are the same file, before
pmt-ehd: resolve absolute file namesit expectedly creates an invalid image. For some reason, I've decided to commit the fix separate from the preceding commit, but, of course, I can squash them before the PR is mergedThe key changes from b6dbd44, I've done:
ehd-related changes and fixesThe merge request
Last edit: oficsu 2022-06-29