Re: [PATCH] base-apt: Use gpg keyid instead of yes

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

Am Mon, 30 Sep 2019 08:17:00 +0200
schrieb Jan Kiszka <jan.kiszka@siemens.com>:

> On 27.09.19 23:11, vijaikumar.kanagarajan@gmail.com wrote:
> > From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > 
> > When using "SignWith: yes", reprepro uses the default gpg key
> > of the system to sign the repo. The default gpg key might be
> > different from what is specified in BASE_REPO_KEY, resulting
> > in using a wrong key for signing.
> > 
> > Derive and use the keyid from the keyfile supplied instead of
> > a generic yes option.
> > 
> > Suggested-by: Amy Fong <Amy_Fong@mentor.com>
> > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > ---
> >   meta/recipes-devtools/base-apt/base-apt.bb | 22
> > +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3
> > deletions(-)
> > 
> > diff --git a/meta/recipes-devtools/base-apt/base-apt.bb
> > b/meta/recipes-devtools/base-apt/base-apt.bb index 74189f1..c74be86
> > 100644 --- a/meta/recipes-devtools/base-apt/base-apt.bb
> > +++ b/meta/recipes-devtools/base-apt/base-apt.bb
> > @@ -4,6 +4,7 @@
> >   SRC_URI = "file://distributions.in"
> >   
> >   BASE_REPO_KEY ?= ""
> > +KEYFILES ?= ""
> >   
> >   CACHE_CONF_DIR = "${REPO_BASE_DIR}/${BASE_DISTRO}/conf"
> >   do_cache_config[dirs] = "${CACHE_CONF_DIR}"
> > @@ -12,13 +13,18 @@ do_cache_config[lockfiles] =
> > "${REPO_BASE_DIR}/isar.lock" 
> >   # Generate reprepro config for current distro if it doesn't
> > exist. Once it's # generated, this task should do nothing.
> > -do_cache_config() {
> > +repo_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 [ -n "${KEYFILES}" ]; then
> > +            option=""
> > +            for key in ${KEYFILES}; do
> > +                keyid=$(cat ${key} | gpg --keyid-format 0xlong
> > --with-colons - 2>/dev/null |grep "^pub:" |awk -F':' '{print
> > $5;}')  
> 
> I hope this parsing is stable...

It looks ok, the format args used are meant for parsing.

But i would like to point out the useless use of cat.
Plus an inconsistent use of spaces around pipes.

Henning

> 
> > +                option="${option}${keyid} "
> > +            done
> >               # To generate Release.gpg
> > -            echo "SignWith: yes" >> ${CACHE_CONF_DIR}/distributions
> > +            echo "SignWith: ${option}" >>
> > ${CACHE_CONF_DIR}/distributions fi
> >       fi
> >   
> > @@ -35,4 +41,14 @@ do_cache_config() {
> >       fi
> >   }
> >   
> > +python do_cache_config() {
> > +    for key in d.getVar('BASE_REPO_KEY').split():
> > +        d.appendVar("SRC_URI", " %s" % key)
> > +        fetcher = bb.fetch2.Fetch([key], d)  
> 
> I wonder if that magically addresses the case that changing key file
> content should also trigger rebuilds. Similar to
> https://github.com/ilbers/isar/issues/60.
> 
> > +        filename = fetcher.localpath(key)
> > +        d.appendVar("KEYFILES", " %s" % filename)
> > +
> > +    bb.build.exec_func('repo_config', d)
> > +}
> > +
> >   addtask cache_config after do_build
> >   
> 
> Looks good - if the keyid extraction if actually robust.
> 
> Jan
>