Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers

[Glenn Washburn]

On Wed, 5 Jan 2022 02:50:36 +0300
Dmitry <reagentoo@gmail.com> wrote:

> ср, 5 янв. 2022 г. в 02:30, Dmitry <reagentoo@gmail.com>:
> 
> >
> >
> > ср, 5 янв. 2022 г. в 01:57, Dmitry <reagentoo@gmail.com>:
> >
> >>
> >>
> >> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn <development@efficientek.com>:
> >>
> >>> On Tue, 4 Jan 2022 15:42:22 -0600
> >>> Glenn Washburn <development@efficientek.com> wrote:
> >>>
> >>> I'm generally very pro-flexibility, but I'm not sure I like this from a
> >>> user perspective. For the common case, which is detached headers in a
> >>> file, this will cause the user to do more work (create a loopback
> >>> device of the file). What's a reasonable scenario where a user would
> >>> want the detached header on a device as opposed to a file system? Am I
> >>> correct in thinking that you use such functionality?
> >>>
> >>
> >> Actually no, I only use a file for the external header, not a disk.
> >> I have now looked at the patches again and will try to state my point of
> >> view in
> >> more detail:
> >>
> >> I don't think the hdr_file field as it stands in the patch set is
> >> relevant. I mean
> >> the hdr_file field of type grub_file_t in the grub_cryptomount_args
> >> structure.
> >> Even the grub_disk_t type may not be relevant here. You could only pass
> >> a header file name or a disk name (as char*) through this structure. This
> >> would
> >>
> >
> So, please ignore these statements. Looks like it's not valid.

I still like the idea of not having to conditionally choose to use the
disk vs. file api for reading the header. I think it would be nice for
the -H argument to be either a file or a device. It seems to me the most
logical place for this to be handled is in the cryptomount arg
handling. If a file is passed, setup a loopback device and pass that as
the header, otherwise if its a device, just open it and pass it along.
This would make cryptodisk module dependent on the loopback module,
which I don't particularly like and may not be acceptable to others.
Dnaiel do you have an opinion about this?

Also, I've looked over the code again and I don't think the benefit of
having detached headers be disk internally is that great at this point
either. If more cryptodisk backends get added that support detached
headers, then the benefit will increase.

I definitely don't want to require that a disk device be passed in as
the detached header argument to cryptomount. So I think the current
approach is acceptable and further down the road someone can propose a
patch to go in the direction you suggest. If you have an alternative
proposal that I'm not thinking of, I'm more than willing to hear it out
and modify these patches if it sounds good.

Glenn

> > reflect the essence of this structure, but further implementation the code
> >> will
> >> not be pretty in this case.
> >>
> >> I still suggest expanding the number of parameters for the recover_key
> >> function
> >> and use grub_disk_t to pass the header from the user directly.
> >>
> >
> > Although in general I'm quite satisfied with the current patch set. It
> > suits my
> > requirements. Maybe disk may be really useless and I overdid it.. It will
> > only
> > remain to add the master key parameter in the future.
> >
> >