Re: [PATCH v2] dm verity: Add support for signature verification with 2nd keyring

[Mickaël Salaün]

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.
> 
>>
>> The current userspace configuration through veritysetup does not need
>> any patches for your patch, correct?
> 
> Right, it's only different from the kernel point of view.
> 
>>
>> Thanks,
>> Milan
>>