Re: base-apt signing interface could be improved

[Henning Schild <henning.schild@siemens.com>]

Am Thu, 27 Jun 2019 10:04:05 -0700
schrieb <vijaikumar.kanagarajan@gmail.com>:

> On Monday, June 17, 2019 at 4:49:58 PM UTC+5:30, 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. 
> >  
> 
> From reprepro manual:
> 
> SignWith
> When this field is present, a Release.gpg file will be generated. If
> the value is "yes" or "default", the default key of gpg is used.
> 
> Reference:
> https://manpages.debian.org/stretch/reprepro/reprepro.1.en.html 
> 
> So I guess it would not be signed by all the keys instead just with
> the default key.

Thanks for looking this up. That actually means that the current code
is incorrect.
 - the variable is an array but the name is singular
 - every step, except reprepro deals with that correctly

So the idea of one of your patches is correct, and a fixed version of
it should be merged.

Henning

> Thanks,
> Vijai Kumar K
> 
> 
> > 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. 
> >
> > 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 
> > >   
> > >   
> >
> >  
>