Menu

#131 add detached headers support

pam_mount
open
nobody
None
5
2022-06-25
2022-06-13
oficsu
No

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

Discussion

  • oficsu

    oficsu - 2022-06-13

    Initial changes already can be found here: branch, diff

     

    Last edit: oficsu 2022-06-13
  • oficsu

    oficsu - 2022-06-14

    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 crypttab here? (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:

    $ git switch wip/detached-headers
    $ GIT_SEQUENCE_EDITOR=: git rebase --autosquash -i ef314d5
    
     
  • oficsu

    oficsu - 2022-06-17

    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

     
  • Jan Engelhardt

    Jan Engelhardt - 2022-06-20

    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).

    Memory management

    Looks consistent.

    char *: calloc / free
    hxmc_t *: HXmc_* / HXmc_free

     
  • Jan Engelhardt

    Jan Engelhardt - 2022-06-20

    Should we use crypttab

    Leaning towards no. crypttab is for boot-time volumes, or rather, static system volumes.

    The pmt-ehd is totally new to me. Should I add support for it?

    You can, but it probably isn't worth the time. The utility existed/exists to facilitate plain dm-crypt volumes when LUKS was new.

     
  • oficsu

    oficsu - 2022-06-20

    @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

     
  • oficsu

    oficsu - 2022-06-25

    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)
    • ehd-related changes and fixes
    • some code style fixes

    The merge request

     

    Last edit: oficsu 2022-06-29

Log in to post a comment.

MongoDB Logo MongoDB