Re: base-apt signing interface could be improved

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi,

On 17/06/2019 13.19, [ext] Henning Schild wrote:
> Am Fri, 14 Jun 2019 06:50:58 -0700
> schrieb "Amy_Fong@mentor.com" <amy.fong.3142@gmail.com>:
> 
>> On Friday, 14 June 2019 04:23:00 UTC-4, Henning Schild wrote:
>>>
>>> Am Thu, 13 Jun 2019 09:55:29 -0700
>>> schrieb "Amy_...@mentor.com <javascript:>" <amy.f...@gmail.com
>>> <javascript:>>:
>>>   
>>>> On Thursday, 6 June 2019 09:46:02 UTC-4, Henning Schild wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> i just had a quick look at the implementation of the base-apt
>>>>> signing for the first time. The interface is not ideal and has
>>>>> potential for the signing key and the checking key not actually
>>>>> belonging together.
>>>>>
>>>>> As far as i understand the code i read, Isar will start signing
>>>>> base-apt if BASE_REPO_KEY is set to anything. The private key
>>>>> it will use to sign the repo is not specified at all, it will
>>>>> be whatever gnupg defaults to, given its configuration.
>>>>>
>>>>> I would suggest to switch from "SignWith yes" to "SignWith
>>>>> <keyid>", and derive the id from BASE_REPO_KEY.
>>>>>
>>>>> Further improvements would be to actually configure gnupg
>>>>> inside Isar and not rely on an outside configuration. Relying
>>>>> on the outside config means that all (multi)configs will have
>>>>> to use the same keypair. So we would add
>>>>>
>>>>> BASE_REPO_KEY_PRIVATE and ..._PASSPHRASE
>>>>>
>>>>> Now we would create a new gpg homedir next to where we store
>>>>> base-apt. We would import that one key there and potentially
>>>>> unlock it with its passphrase. If we clean and rebuild we get a
>>>>> working gpghome for sure.
>>>>>
>>>>> Henning
>>>>>      
>>>>
>>>> Hi,
>>>>
>>>> Perhaps something like the following ...
>>>>
>>>> Of course, since BASE_REPO_KEY permits specifying
>>>> multiple keys, this raises a question of which keyid?
>>>
>>> Oh that is a nice hidden feature, indeed one can specify multiple
>>> keys there. So that variable should be called BASE_REPO_KEYS
>>> instead.
>>>
>>> And yes reprepro also supports multiple values. So i guess your
>>> patch is correct and it would probably sign the repo with all the
>>> keys specified.
>>>
>>> Whether that is what we want is another question, and i am not sure
>>> whether "yes" will also use all keys or just the default one.
>>>   
>>>> Amy
>>>>
>>>>  From 5ceb4a2ef97bc7fa6c44cd9ce6f73f9a831773f3 Mon Sep 17 00:00:00
>>>> 2001 From: Amy Fong <Amy_...@mentor.com <javascript:>>
>>>> Date: Thu, 13 Jun 2019 12:52:06 -0400
>>>> Subject: [PATCH] base-apt: Use BASE_REPO_KEY for signing
>>>>
>>>> Extract keyid from BASE_REPO_KEY for signing
>>>>
>>>> Signed-off-by: Amy Fong <Amy_...@mentor.com <javascript:>>
>>>> ---
>>>>   meta/recipes-devtools/base-apt/base-apt.bb | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/meta/recipes-devtools/base-apt/base-apt.bb
>>>> b/meta/recipes-devtools/base-apt/base-apt.bb
>>>> index 1c0b4c6..81245f7 100644
>>>> --- a/meta/recipes-devtools/base-apt/base-apt.bb
>>>> +++ b/meta/recipes-devtools/base-apt/base-apt.bb
>>>> @@ -19,8 +19,15 @@ do_cache_config() {
>>>>           sed -e "s#{CODENAME}#"${BASE_DISTRO_CODENAME}"#g" \
>>>>               ${WORKDIR}/distributions.in >
>>>> ${CACHE_CONF_DIR}/distributions if [ "${BASE_REPO_KEY}" ] ; then
>>>> +            option="yes"
>>>
>>> maybe there is a better name for the variable?
>>>
>>> Henning
>>>   
>>>> +            for key in ${BASE_REPO_KEY}; do
>>>> +                keyid=$(wget -qO - $key | gpg --keyid-format
>>>> 0xlong --with-colons - 2>/dev/null |grep "^pub:" |awk -F':'
>>>> '{print $5;}')
>>>> +                if [ -n "$keyid" ]; then
>>>> +                    option="$keyid"
>>>> +                fi
>>>> +            done
>>>>               # To generate Release.gpg
>>>> -            echo "SignWith: yes" >>
>>>> ${CACHE_CONF_DIR}/distributions
>>>> +            echo "SignWith: $option" >>
>>>> ${CACHE_CONF_DIR}/distributions fi
>>>>       fi
>>>>      
>>>   
>>
>> How about BASE_REPO_SIGN_KEY?
> 
> I do not understand what you are trying to solve with changing that
> name and going back to one-key-only, after you have found that
> BASE_REPO_KEY is indeed an array and reprepro also accepts an array.
> 
> Now we need to know what "yes", compared to the array.
> 
> And any tiny patch like this one, without a proper commit message and
> description, is not going to lead anywhere good.
> 
> You guys are doing the full story. kas, signed base-apt, multiple keys,
> agent-forwarding ...
> After you are done you should have a clear picture of what currently
> does not work as expected, and how it can be fixes (your initial
> implementation).
> We can then discuss that implementation and incorporate a full patch
> series including docs into kas and Isar.
> 
>> commit 42ee1139e8383fc27e7d98be522cb4d306fd170c (HEAD -> apt_sign)
>> Author: Amy Fong <Amy_Fong@mentor.com>
>> Date:   Thu Jun 13 12:52:06 2019 -0400
>>
>>      base-apt: Use BASE_REPO_SIGN_KEY for signing
>>      
>>      Extract keyid from BASE_REPO_SIGN_KEY for signing
>>      
>>      Signed-off-by: Amy Fong <Amy_Fong@mentor.com>
>>
>> diff --git a/meta/recipes-devtools/base-apt/base-apt.bb
>> b/meta/recipes-devtools/base-apt/base-apt.bb
>> index 1c0b4c6..c896add 100644
>> --- a/meta/recipes-devtools/base-apt/base-apt.bb
>> +++ b/meta/recipes-devtools/base-apt/base-apt.bb
>> @@ -18,9 +18,14 @@ do_cache_config() {
>>       if [ ! -e "${CACHE_CONF_DIR}/distributions" ]; then
>>           sed -e "s#{CODENAME}#"${BASE_DISTRO_CODENAME}"#g" \
>>               ${WORKDIR}/distributions.in >
>> ${CACHE_CONF_DIR}/distributions
>> -        if [ "${BASE_REPO_KEY}" ] ; then
>> +        if [ "${BASE_REPO_SIGN_KEY}" ] ; then
>> +            option="yes"
>> +            keyid=$(wget -qO - "${BASE_REPO_SIGN_KEY}" | gpg
> 
> Using wget, but that is most likely a "file:///" URI. And whenever you
> do networking in a task, you need to take care of proxies.

Fetching should not be done like this anyway. If something needs to be 
fetched then it should be part of the SRC_URI and be fetched by the 
do_fetch task. The reasons for this are offline reproducibility among 
others.

regards,
Claudius

> 
> Henning
> 
>> --keyid-format 0xlong --with-colons - 2>/dev/null |grep "^pub:" |awk
>> -F':' '{print $5;}')
>> +            if [ -n "$keyid" ]; then
>> +                option="$keyid"
>> +            fi
>>               # To generate Release.gpg
>> -            echo "SignWith: yes" >> ${CACHE_CONF_DIR}/distributions
>> +            echo "SignWith: $option" >>
>> ${CACHE_CONF_DIR}/distributions fi
>>       fi
>>   
>>
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de