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

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

[Usama Arif]

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(+)

diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
index fa4ea6feef..1fa8c8f05c 100644
--- a/meta/classes/kernel-fitimage.bbclass
+++ b/meta/classes/kernel-fitimage.bbclass
@@ -56,6 +56,22 @@ FIT_HASH_ALG ?= "sha256"
 # fitImage Signature Algo
 FIT_SIGN_ALG ?= "rsa2048"
 
+# Generate keys for signing fitImage
+FIT_GENERATE_KEYS ?= "${@bb.utils.contains('UBOOT_SIGN_ENABLE', '1', '1', '0', d)}"
+
+# Size of private key in number of bits
+FIT_SIGN_NUMBITS ?= "2048"
+
+# args to openssl genrsa (Default is just the public exponent)
+FIT_KEY_GENRSA_ARGS ?= "-F4"
+
+# args to openssl req (Default is -batch for non interactive mode and
+# -new for new certificate)
+FIT_KEY_REQ_ARGS ?= "-batch -new"
+
+# Standard format for public key certificate
+FIT_KEY_SIGN_PKCS ?= "-x509"
+
 #
 # Emit the fitImage ITS header
 #
@@ -522,6 +538,34 @@ do_assemble_fitimage_initramfs() {
 
 addtask assemble_fitimage_initramfs before do_deploy after do_bundle_initramfs
 
+do_generate_rsa_keys() {
+	if [ "${UBOOT_SIGN_ENABLE}" = "0" ] && [ "${FIT_GENERATE_KEYS}" = "1" ]; then
+		bbwarn "FIT_GENERATE_KEYS is set to 1 eventhough UBOOT_SIGN_ENABLE is set to 0. The keys will not be generated as they won't be used."
+	fi
+
+	if [ "${UBOOT_SIGN_ENABLE}" = "1" ] && [ "${FIT_GENERATE_KEYS}" = "1" ]; then
+
+		# 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
+
+			# make directory if it does not already exist
+			mkdir -p "${UBOOT_SIGN_KEYDIR}"
+
+			echo "Generating RSA private key for signing fitImage"
+			openssl genrsa ${FIT_KEY_GENRSA_ARGS} -out \
+				"${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
+				"${FIT_SIGN_NUMBITS}"
+
+			echo "Generating certificate for signing fitImage"
+			openssl req ${FIT_KEY_REQ_ARGS} "${FIT_KEY_SIGN_PKCS}" \
+				-key "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
+				-out "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".crt
+		fi
+	fi
+}
+
+addtask generate_rsa_keys before do_assemble_fitimage after do_compile
 
 kernel_do_deploy[vardepsexclude] = "DATETIME"
 kernel_do_deploy_append() {
-- 
2.17.1

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

[Usama Arif]

On 08/09/2020 13:28, 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(+)
> 
> diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
> index fa4ea6feef..1fa8c8f05c 100644
> --- a/meta/classes/kernel-fitimage.bbclass
> +++ b/meta/classes/kernel-fitimage.bbclass
> @@ -56,6 +56,22 @@ FIT_HASH_ALG ?= "sha256"
>   # fitImage Signature Algo
>   FIT_SIGN_ALG ?= "rsa2048"
>   
> +# Generate keys for signing fitImage
> +FIT_GENERATE_KEYS ?= "${@bb.utils.contains('UBOOT_SIGN_ENABLE', '1', '1', '0', d)}"
> +
> +# Size of private key in number of bits
> +FIT_SIGN_NUMBITS ?= "2048"
> +
> +# args to openssl genrsa (Default is just the public exponent)
> +FIT_KEY_GENRSA_ARGS ?= "-F4"
> +
> +# args to openssl req (Default is -batch for non interactive mode and
> +# -new for new certificate)
> +FIT_KEY_REQ_ARGS ?= "-batch -new"
> +
> +# Standard format for public key certificate
> +FIT_KEY_SIGN_PKCS ?= "-x509"
> +
>   #
>   # Emit the fitImage ITS header
>   #
> @@ -522,6 +538,34 @@ do_assemble_fitimage_initramfs() {
>   
>   addtask assemble_fitimage_initramfs before do_deploy after do_bundle_initramfs
>   
> +do_generate_rsa_keys() {
> +	if [ "${UBOOT_SIGN_ENABLE}" = "0" ] && [ "${FIT_GENERATE_KEYS}" = "1" ]; then
> +		bbwarn "FIT_GENERATE_KEYS is set to 1 eventhough UBOOT_SIGN_ENABLE is set to 0. The keys will not be generated as they won't be used."
> +	fi
> +
> +	if [ "${UBOOT_SIGN_ENABLE}" = "1" ] && [ "${FIT_GENERATE_KEYS}" = "1" ]; then
> +
> +		# 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
> +
> +			# make directory if it does not already exist
> +			mkdir -p "${UBOOT_SIGN_KEYDIR}"
> +
> +			echo "Generating RSA private key for signing fitImage"
> +			openssl genrsa ${FIT_KEY_GENRSA_ARGS} -out \
> +				"${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
> +				"${FIT_SIGN_NUMBITS}"
> +
> +			echo "Generating certificate for signing fitImage"
> +			openssl req ${FIT_KEY_REQ_ARGS} "${FIT_KEY_SIGN_PKCS}" \
> +				-key "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
> +				-out "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".crt
> +		fi
> +	fi
> +}
> +
> +addtask generate_rsa_keys before do_assemble_fitimage after do_compile
>   
>   kernel_do_deploy[vardepsexclude] = "DATETIME"
>   kernel_do_deploy_append() {
> 

The relevant yocto-docs changes for this patch are in 
https://lists.yoctoproject.org/g/docs/message/340

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

[Usama Arif]

Hi,

Just wanted to check if there were any review comments for this patch?

Thanks,
Usama

On 08/09/2020 13:43, Usama Arif via lists.openembedded.org wrote:
> 
> 
> On 08/09/2020 13:28, 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(+)
>>
>> diff --git a/meta/classes/kernel-fitimage.bbclass 
>> b/meta/classes/kernel-fitimage.bbclass
>> index fa4ea6feef..1fa8c8f05c 100644
>> --- a/meta/classes/kernel-fitimage.bbclass
>> +++ b/meta/classes/kernel-fitimage.bbclass
>> @@ -56,6 +56,22 @@ FIT_HASH_ALG ?= "sha256"
>>   # fitImage Signature Algo
>>   FIT_SIGN_ALG ?= "rsa2048"
>> +# Generate keys for signing fitImage
>> +FIT_GENERATE_KEYS ?= "${@bb.utils.contains('UBOOT_SIGN_ENABLE', '1', 
>> '1', '0', d)}"
>> +
>> +# Size of private key in number of bits
>> +FIT_SIGN_NUMBITS ?= "2048"
>> +
>> +# args to openssl genrsa (Default is just the public exponent)
>> +FIT_KEY_GENRSA_ARGS ?= "-F4"
>> +
>> +# args to openssl req (Default is -batch for non interactive mode and
>> +# -new for new certificate)
>> +FIT_KEY_REQ_ARGS ?= "-batch -new"
>> +
>> +# Standard format for public key certificate
>> +FIT_KEY_SIGN_PKCS ?= "-x509"
>> +
>>   #
>>   # Emit the fitImage ITS header
>>   #
>> @@ -522,6 +538,34 @@ do_assemble_fitimage_initramfs() {
>>   addtask assemble_fitimage_initramfs before do_deploy after 
>> do_bundle_initramfs
>> +do_generate_rsa_keys() {
>> +    if [ "${UBOOT_SIGN_ENABLE}" = "0" ] && [ "${FIT_GENERATE_KEYS}" = 
>> "1" ]; then
>> +        bbwarn "FIT_GENERATE_KEYS is set to 1 eventhough 
>> UBOOT_SIGN_ENABLE is set to 0. The keys will not be generated as they 
>> won't be used."
>> +    fi
>> +
>> +    if [ "${UBOOT_SIGN_ENABLE}" = "1" ] && [ "${FIT_GENERATE_KEYS}" = 
>> "1" ]; then
>> +
>> +        # 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
>> +
>> +            # make directory if it does not already exist
>> +            mkdir -p "${UBOOT_SIGN_KEYDIR}"
>> +
>> +            echo "Generating RSA private key for signing fitImage"
>> +            openssl genrsa ${FIT_KEY_GENRSA_ARGS} -out \
>> +                "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
>> +                "${FIT_SIGN_NUMBITS}"
>> +
>> +            echo "Generating certificate for signing fitImage"
>> +            openssl req ${FIT_KEY_REQ_ARGS} "${FIT_KEY_SIGN_PKCS}" \
>> +                -key "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".key \
>> +                -out "${UBOOT_SIGN_KEYDIR}/${UBOOT_SIGN_KEYNAME}".crt
>> +        fi
>> +    fi
>> +}
>> +
>> +addtask generate_rsa_keys before do_assemble_fitimage after do_compile
>>   kernel_do_deploy[vardepsexclude] = "DATETIME"
>>   kernel_do_deploy_append() {
>>
> 
> The relevant yocto-docs changes for this patch are in 
> https://lists.yoctoproject.org/g/docs/message/340
> 
> 
> 

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

[Richard Purdie]

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.

Perhaps this code should need to be explicitly enabled?

Cheers,

Richard

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

[Usama Arif]

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

> 
> Cheers,
> 
> Richard
> 

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

[Usama Arif]

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?

Thanks,
Usama

>>
>> Cheers,
>>
>> Richard
>>
> 
> 
> 
> 

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

[Richard Purdie]

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

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

[Usama Arif]

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

> 
> 
> 
> 
>