Re: [OE-core] [PATCH] kernel-fitimage: generate openssl RSA keys for signing fitimage

["Usama Arif" <usama.arif@arm.com>]

On 30/09/2020 11:22, Richard Purdie via lists.openembedded.org wrote:
> On Wed, 2020-09-30 at 11:14 +0100, Usama Arif wrote:
>>
>> On 21/09/2020 14:24, Usama Arif via lists.openembedded.org wrote:
>>> On 21/09/2020 14:03, Richard Purdie wrote:
>>>> On Tue, 2020-09-08 at 13:28 +0100, Usama Arif wrote:
>>>>> The keys are only generated if they dont exist. The key
>>>>> generation can be turned off by setting FIT_GENERATE_KEYS to
>>>>> "0".
>>>>> The default key length for private keys is 2048 and the default
>>>>> format for public key certificate is x.509.
>>>>>
>>>>> Signed-off-by: Usama Arif <usama.arif@arm.com>
>>>>> ---
>>>>>    meta/classes/kernel-fitimage.bbclass | 44
>>>>> ++++++++++++++++++++++++++++
>>>>>    1 file changed, 44 insertions(+)
>>>>
>>>> I'm worried about this as keys are generally something the user
>>>> needs
>>>> to handle carefully. Making it all "magic" means that a missing
>>>> key
>>>> might not throw an error when it should and also, someone might
>>>> not
>>>> save the keys when they might need to.
>>>>
>>> To make sure the keys exists, we could check in step 7 of
>>> fitimage_assemble that
>>> ${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key
>>> and ${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".crt exist if
>>> UBOOT_SIGN_ENABLE is set to 1?
>>>
>>>> Perhaps this code should need to be explicitly enabled?
>>>
>>> By explicitly enable do you mean change the ?= to = in the below
>>> line?
>>>
>>> FIT_GENERATE_KEYS ?= "${@bb.utils.contains('UBOOT_SIGN_ENABLE',
>>> '1',
>>> '1', '0', d)}"
>>>
>>> I actually think that keeping ?= is a good idea as users might want
>>> to
>>> use some other key not generated by oe-core, so they can choose to
>>> disable FIT_GENERATE_KEYS.
>>>
>>> Thanks for the review!
>>> Usama
>>>
>>
>> Hi,
>>
>> Just wanted to check if there were any more review comments or
>> anymore
>> comments on above, i.e. would you like me to add a check in step 7
>> to
>> make sure the keys exist and do you think its a good idea to use =
>> instead of ?= for setting FIT_GENERTATE_KEYS?
> 
> What I meant in my previous reply was setting:
> 
> FIT_GENERATE_KEYS ?= "0"
> 
> as the default and requiring the user to set it to something else to
> enable the key generation.
> 
> I'm worried that otherwise, users won't realise keys are being
> generated and they won't be managing them appropriately. Keys are
> probably something you should be preserving between builds for example?
> 
> Cheers,
> 
> Richard
> 
>

Hi,

Thanks for the reply. I have set FIT_GENERATE_KEYS ?= "0" in v2 of the 
patch.

In both v1 and v2 of the patch i have the following check before 
generating keys:

# Generate keys only if they don't already exist
if [ ! -f "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key ] || \
	[ ! -f "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".crt]; then

which will make sure that keys are not generated if they already exist 
and are therefore preserved between builds.

Thanks,
Usama

> 
> 
> 
> 
>