Re: base-apt signing interface could be improved

[Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>]

On Fri, Jun 28, 2019 at 10:14:48AM +0200, Henning Schild wrote:
Hi Henning,

Thanks for info. Sorry for the late reply. Was a little busy testing
some kas changes. I have some observations wrt ISAR and since all
of these is in the package feed creation part I will use this thread
to sum it up.

Hi All,

I have been using ISAR for quite some time and have observed the following
limitations/issues with it, mostly in the package feed creation part.


1. ISAR is not ready for handling password protected keys. Reprepro still 
uses password prompt(default pinentry by the system). i.e we cannot automate
the password input to reprepro.

Description:
If we were to generate base-apt with a password protected gpg key then ISAR 
build would fail unless the password is saved or entered in the pinentry
prompt.

Possible Solution:
We could possibly have a variable called BASE_REPO_PASSPHRASE which would 
store the password for reprepro to use when signing. One issue here is that
reprepro uses the pinentry used by gpg agent. We would need to configure gpg 
to loopback pinentry and pass in the password.
BASE_REPO_PASSPHRASE is mostly inspired by the RPM_GPG_PASSPHRASE in oe.
https://git.yoctoproject.org/cgit.cgi/poky/plain/meta/classes/sign_rpm.bbclass


2. Reprepro implementation in ISAR doesnot take into consideration the 
GNUPGHOME env.

Description:
Lets say the user has the .gnupg directory in a custom location 
(ex: /opt/.gnupg) in his server and has set GNUPGHOME to /opt/.gnupg. 
If you trigger a base-apt generation with ISAR in that server, then reprepro 
command would fail(if there is no ~/.gnupg). reprepro in bitbake environment 
would not know about the GNUPGHOME varible.

Possible solution:
One could add GNUPGHOME to BB_ENV_EXTRAWHITE and if is not empty can export it
to the environment where reprepro is called.
+        if [ ! -z ${GNUPGHOME} ]; then
+            export GNUPGHOME=${GNUPGHOME}
+        fi

3. ISAR repo signing would always use the default GPG-key of the system. One 
cannot specify the key to use.

Description:
This is particularly tedious in case of CI. The user might have a server which
has n GPG keys in it and may wanted to sign the repo using the nth key. So one
would probably export the public key and set BASE_REPO_KEY=<path to the key>. 
But reprepro would just use the default key in the system which might be a 
completely different key.

Possible solution:
We could use the key ids instead of the key file itself. Something like 
BASE_REPO_KEYID="6FA1B2F0416E3C24".
And the reprepro distributions file can be as below.
Codename: stretch
Architectures: i386 armhf arm64 amd64 source
Components: main
SignWith: "6FA1B2F0416E3C24"

Please let me know your thoughts on this. Meanwhile I will test some of my
changes for the above issues and post the patchset for review.

Thanks,
Vijai Kumar K

> Hi Vijai,
> 
> you are assuming kas and the agent implementation that mentor proposed
> there.
> 
> two things to keep in mind:
> - kas is not just for Isar, but also yocto/oe
> - Isar can be used without kas
> - kas can be used without docker
> 
> The current implementation does not work in so many places because it
> was never applied on a real use-case. You now find the limitations
> because you have that use-case.
> 
> Again, please implement the full story, including documentation, and
> propose patches that enable such a use-case.
> 
> These small intermediate steps just bring isar and kas into the next
> intermediate state between early-feature and working. And what might
> seem correct today may become a revert and an API breakage tomorrow.
> 
> Henning
> 
> Am Thu, 27 Jun 2019 23:30:31 -0700
> schrieb <vijaikumar.kanagarajan@gmail.com>:
> 
> > 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:> 
> > >  
> > 
>