On Fri, Oct 23 2020 at 6:20am -0400, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > It seems that there is no more question. Mike, Alasdair, could you > please consider to merge this into the tree? > > On 16/10/2020 14:19, Mickaël Salaün wrote: > > > > On 16/10/2020 13:08, Milan Broz wrote: > >> On 16/10/2020 10:49, Mickaël Salaün wrote: > >>> On 16/10/2020 10:29, Mickaël Salaün wrote: > >>>> > >>>> On 15/10/2020 18:52, Mike Snitzer wrote: > >>>>> Can you please explain why you've decided to make this a Kconfig CONFIG > >>>>> knob? Why not either add: a dm-verity table argument? A dm-verity > >>>>> kernel module parameter? or both (to allow a particular default but > >>>>> then > >>>>> per-device override)? > >>>> > >>>> The purpose of signed dm-verity images is to authenticate files, or said > >>>> in another way, to enable the kernel to trust disk images in a flexible > >>>> way (i.e. thanks to certificate's chain of trust). Being able to update > >>>> such chain at run time requires to use the second trusted keyring. This > >>>> keyring automatically includes the certificate authorities from the > >>>> builtin trusted keyring, which are required to dynamically populate the > >>>> secondary trusted keyring with certificates signed by an already trusted > >>>> authority. The roots of trust must then be included at build time in the > >>>> builtin trusted keyring. > >>>> > >>>> To be meaningful, using dm-verity signatures implies to have a > >>>> restricted user space, i.e. even the root user has limited power over > >>>> the kernel and the rest of the system. Blindly trusting data provided by > >>>> user space (e.g. dm-verity table argument, kernel module parameter) > >>>> defeat the purpose of (mandatory) authenticated images. > >>>> > >>>>> > >>>>> Otherwise, _all_ DM verity devices will be configured to use secondary > >>>>> keyring fallback. Is that really desirable? > >>>> > >>>> That is already the current state (on purpose). > >>> > >>> I meant that when DM_VERITY_VERIFY_ROOTHASH_SIG is set, dm-verity > >>> signature becomes mandatory. This new configuration > >>> DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING extend this trust to the > >>> secondary trusted keyring, which contains certificates signed (directly > >>> or indirectly) by CA from the builtin trusted keyring. > >>> > >>> So yes, this new (optional) configuration *extends* the source of trust > >>> for all dm-verity devices, and yes, it is desirable. I think it should > >>> have been this way from the beginning (as for other authentication > >>> mechanisms) but it wasn't necessary at that time. > >> > >> Well, I understand why you need a config option here. > >> And using the secondary keyring actually makes much more sense to me than > >> the original approach. > >> > >> But please do not forget that dm-verity is sometimes used in different > >> contexts where such strict in-kernel certificate trust is unnecessary. > >> With your configure options set, you deliberately remove the possibility > >> to configure such devices. > > It doesn't make sense to set DM_VERITY_VERIFY_ROOTHASH_SIG in generic > > distro because such policy is configured at build time in the kernel > > with hardcoded CAs. If the new option is not set then nothing change. I > > don't see why it could be an issue for use cases we previously defined > > (with DM_VERITY_VERIFY_ROOTHASH_SIG). > > > >> I understand that it is needed for "trusted" systems, but we should be > >> clear > >> in the documentation. > >> Maybe also add note to > >> /Documentation/admin-guide/device-mapper/verity.rst ? > >> We already mention DM_VERITY_VERIFY_ROOTHASH_SIG there. > > > > The current documentation remains true. > > DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING depends on > > DM_VERITY_VERIFY_ROOTHASH_SIG. Yes, while true that doesn't change the fact that documenting DM_VERITY_VERIFY_ROOTHASH_SIG_SECONDARY_KEYRING is useful to potential consumers of baseline DM_VERITY_VERIFY_ROOTHASH_SIG. Please update Documentation and post v3, I'll get it merged for 5.11. Thanks, Mike
