Re: base-apt signing interface could be improved

[vijaikumar.kanagarajan@gmail.com]

[-- Attachment #1.1: Type: text/plain, Size: 7979 bytes --]

Hi Claudius,

Just wondering, why cant the BASE_REPO_KEY be a list of key ids instead of 
the keyfile itself. Since we are using the host gpg agent anyways for 
signing.

Later if this key needs to be added to apt sources key ring, it can be 
exported.

The one advantage is that it eliminates the need to maintain the keyfiles 
in host. It is redundant. Anyway one would need to have the keys as part of 
gpg keyring to successfully sign the repo.

Thanks,
Vijai Kumar K

On Monday, June 17, 2019 at 5:06:29 PM UTC+5:30, Claudius Heine wrote:
>
> Hi, 
>
> On 17/06/2019 13.19, [ext] Henning Schild wrote: 
> > Am Fri, 14 Jun 2019 06:50:58 -0700 
> > schrieb "Amy_...@mentor.com <javascript:>" <amy.f...@gmail.com 
> <javascript:>>: 
> > 
> >> 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_...@mentor.com <javascript:>> 
> >> 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_...@mentor.com <javascript:>> 
> >> 
> >> 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: c...@denx.de 
> <javascript:> 
>

[-- Attachment #1.2: Type: text/html, Size: 16357 bytes --]