Re: [PATCH v8 1/7] Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS"

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

Am Tue, 16 Apr 2019 06:54:51 +0200
schrieb "Maxim Yu. Osipov" <mosipov@ilbers.de>:

> On 4/15/19 1:11 PM, Andreas Reichel wrote:
> > On Mon, Mar 25, 2019 at 12:20:10PM +0100, Maxim Yu. Osipov wrote:  
> >> Hi Andreas,
> >>
> >> On 3/21/19 4:15 PM, Andreas J. Reichel wrote:  
> >>> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> >>>
> >>> This reverts commit af983a13b6f4cee5d4af5e5cf6318231e02775c9.
> >>>
> >>> This commit broke usage of remote keys, where they usually come
> >>> from. If fetching "http://example.com/dir1/dir2/key", the file is
> >>> fetched into the subdir WORKDIR/dir1/dir2/, which breaks with
> >>> this code. However it succeeds with absolute paths.
> >>> We do not want to guess where the downloaded file will be. This
> >>> does not work anymore if the key is downloaded from remote with a
> >>> URL. Furthermore, a user could specify "subdir" as fetcher
> >>> parameter or other things, which break this.  
> >>
> >> In general it's better to avoid reverting the patches.  
> > 
> > Okay.
> >   
> >>
> >> I disagree with the statement that "commit af983a13  broke usage
> >> of remote keys" - I wrote you a month ago (see forwarded email
> >> below) that this patch doesn't break Raspbian build  - the key is
> >> put also under downloads (DL_DIR)  
> > 
> > in DL_DIR - Yes, where you can get name collisions with several
> > keys since you cannot rename the files with your code via bitbake
> > URL parameter.  
> 
> Well...And how this problem is related with my patch?
> My patch fixes the problem with locally stored keys, as
> Local fetcher module puts the file under
> ${WORKDIR}/<absolute_path_to_key_file>, so
> gpg in do_generate_keyring() task can't find it.
> 
> 
> >> directory:
> >>
> >> I've build the current 'master' for target
> >> multiconfig:rpi-jessie:isar-image-base.  
> > 
> > I gave you an example where it broke for me. Raspbian is not one of
> > these.
> > 
> >   https://archive.raspbian.org/raspbian.public.key
> > 
> >   Is there a subdir after the domain? - No ! Does it fit to my
> > example? => No. Does your argument with raspbian make sense now? =>
> > No.  
> 
> Again, my patch fixes the problem with local keys.
> It does nothing about fetching remote keys - it just doesn't break 
> existing functionality.

While i do agree that a revert looks like blaming a patch and an author
for doing something wrong, i fail to get why this comes up so late in
the review. Yes Andreas can rebase yet again and turn the revert into
something else, in the end the code will be the same.

The revert question was already discussed in v3. Let us talk content
and that series is in very good shape.

I would suggest to merge that and accept a test-case / example on top.

Henning

> 
> >>
> >> Sorry for copy-paste ;):
> >>
> >> <<<<
> >> isar/build$ find -name raspbian.public.key
> >> ./downloads/raspbian.public.key
> >> ./tmp/work/raspbian-jessie-armhf/isar-bootstrap-target/raspbian.public.key  
> >>>>>  
> >>
> >> So I don't accept this patch.
> >>  
> > Then I have to rebase - without revert - , which may take some
> > time...  
> 
> OK.
> 
> Regards,
> Maxim.
> 
> > Regards,
> > Andreas
> >   
> >> Regards,
> >> Maxim.
> >>
> >>
> >> -------- Forwarded Message --------
> >> Subject: Re: [PATCH 0/1] Fix remote key fetching apt keyring
> >> Date: Wed, 20 Feb 2019 12:58:09 +0100
> >> From: Maxim Yu. Osipov <mosipov@ilbers.de>
> >> Organization: ilbers GmbH
> >> To: Jan Kiszka <jan.kiszka@siemens.com>, [ext] Andreas J. Reichel
> >> <andreas.reichel.ext@siemens.com>, isar-users@googlegroups.com,
> >> Baurzhan Ismagulov <ibr@ilbers.de>
> >>
> >> On 2/20/19 12:27 PM, Jan Kiszka wrote:  
> >>> On 20.02.19 12:21, [ext] Andreas J. Reichel wrote:  
> >>>> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> >>>>
> >>>> Since my last mail was not answered, but this is an important
> >>>> topic, here is a patch that shows what the problem is.
> >>>>
> >>>> If we fetch the user apt key from remote, we need the basename,
> >>>> if we fetch it locally we need the absolute path...
> >>>>
> >>>> While this might not be the best way to fix this, it works as
> >>>> good as the rest of this code...
> >>>>
> >>>> At least it fixes Isar again up to adding the key to the keyring.
> >>>>
> >>>> But this still does not fix the next problem with the docker-ce
> >>>> key:
> >>>>
> >>>> | I: Running command: debootstrap --arch arm64 --foreign
> >>>> --verbose --variant=minbase --include=locales
> >>>> --components=main,contrib,non-free --keyring
> >>>> /build/build/tmp/work/debian-stretch-arm64/isar-bootstrap-target/apt-keyring.gpg  
> >>  
> >>>> stretch
> >>>> /build/build/tmp/work/debian-stretch-arm64/isar-bootstrap-target/rootfs  
> >> http://ftp.debian.org/debian  
> >>>>
> >>>> | I: Retrieving InRelease
> >>>> | I: Retrieving Release
> >>>> | I: Retrieving Release.gpg
> >>>> | I: Checking Release signature
> >>>> | E: Release signed by unknown key (key id EF0F382A1A7B6500)
> >>>>
> >>>> So something additionally must be done. Since I am not an expert
> >>>> on debian keyring/debootstrap and dpkg signing I will try to
> >>>> find a solution but maybe somebody has a good idea already?
> >>>>  
> >>>
> >>> Baurzhan, Maxim, any idea?  
> >>
> >> Strange...I thought that commit af983a13 fixes the reported problem
> >> When testing my patch signing base-apt I've tried both - remote
> >> keys (used by Raspberry Pi target) and local key.
> >>
> >>
> >>  
> >>>
> >>> This is really fixed in a follow-up commit.
> >>>
> >>> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> >>> ---
> >>>    meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 5 +++--
> >>>    1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> >>> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> >>> c1b571a..2910eea 100644 ---
> >>> a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> >>> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -40,8
> >>> +40,9 @@ python () { d.setVar("DEBOOTSTRAP_KEYRING", "--keyring
> >>> ${APTKEYRING}") for key in distro_apt_keys.split():
> >>>                url = urlparse(key)
> >>> -            d.appendVar("SRC_URI", " " + key)
> >>> -            d.appendVar("APTKEYFILES", " " + wd + url.path)
> >>> +            filename = os.path.basename(url.path)
> >>> +            d.appendVar("SRC_URI", " %s" % key)
> >>> +            d.appendVar("APTKEYFILES", " %s" % filename)
> >>>        if
> >>> bb.utils.to_boolean(d.getVar('ISAR_USE_CACHED_BASE_REPO')):
> >>> own_pub_key = d.getVar("BASE_REPO_KEY", False) if own_pub_key:
> >>>  
> >>
> >>
> >> -- 
> >> Maxim Osipov
> >> ilbers GmbH
> >> Maria-Merian-Str. 8
> >> 85521 Ottobrunn
> >> Germany
> >> +49 (151) 6517 6917
> >> mosipov@ilbers.de
> >> http://ilbers.de/
> >> Commercial register Munich, HRB 214197
> >> General Manager: Baurzhan Ismagulov  
> >   
> 
>