[elbe-devel] [PATCH v3 3/8] debinstaller: implement download of vmlinuz and initrd.gz

[Manuel Traut]

On Mon, Oct 15, 2018 at 03:18:01PM +0200, Torben Hohn wrote:
> the debinstaller module shall replace elbe-bootstrap.
> it downloads debian installer linux kernel and initrd.gz from
> a debian mirror. It does that in a secure manner, iE validating
> Release.gpg and SHA256SUMS on the way.
> 
> filenames on the mirror:
> /debian/dists/jessie/main/installer-amd64/current/images/netboot/debian-installer/amd64/linux
> /debian/dists/jessie/main/installer-amd64/current/images/netboot/debian-installer/amd64/initrd.gz
> /debian/dists/jessie/main/installer-amd64/current/images/cdrom/initrd.gz
> 
> Although this functionality is also provided by apt,
> we implement it here in pure python, because outside
> of the initvm, we can not rely on apt being available.
> 
> The initrd and vmlinuz are stored in the initvm in /var/cache/elbe/installer.
> Also put them on bin-cdrom.iso, and reuse them, when an elbe build
> is run from cdrom.
> 
> since elbe uses elbepack.debinstaller outside of the initvm now,
> the non-virtap fallback code is not needed anymore.
> Remove it and make pkgutils.py pylint clean.
> Also rename the, now moved to debinstaller, NoKinitrdException
> to NoPackageException.
> 
> Also remove code dealing with elbe bootstrap in packagelists and
> xmldefaults.
> 
> Signed-off-by: Torben Hohn <torben.hohn at linutronix.de>
>
> ---

[..]

> diff --git a/elbepack/debinstaller.py b/elbepack/debinstaller.py
> new file mode 100644
> index 00000000..d251f202
> --- /dev/null
> +++ b/elbepack/debinstaller.py

[..]

> +def download(url, local_fname):
> +    try:
> +        rf = urlopen(url, None, 10)
> +        with open(local_fname, "w") as wf:
> +            copyfileobj(rf, wf)
> +    finally:
> +        rf.close()

error handling is not correct, if an invalid URL was specified,
i get a backtrace:

--8<--
Traceback (most recent call last):
  File "/home/manut/Projects/elbe/elbe/elbe", line 55, in <module>
    cmdmod.run_command(sys.argv[2:])
  File "/home/manut/Projects/elbe/elbe/elbepack/commands/init.py", line 168, in run_command
    copy_kinitrd(xml.node("/initvm"), out_path)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 231, in copy_kinitrd
    download_kinitrd(tmp, suite, mirror)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 169, in download_kinitrd
    download_release(tmp, base_url)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 135, in download_release
    download(base_url + "Release", tmp.fname('Release'))
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 123, in download
    rf.close()
UnboundLocalError: local variable 'rf' referenced before assignment
--8<--

Also the user should get some output during download. URL, filename is mandatory
for analyzing problems. Sth. like a progress bar would be nice to have if easy
to implement.

Why not just use os.system(wget..) ?

> +def download_release(tmp, base_url):
> +
> +    # setup gpg context, for verifying
> +    # the Release.gpg signature.
> +    os.environ['GNUPGHOME'] = tmp.fname('/')
> +    ctx = Context()
> +
> +    # download the Relase file to a tmp file,
> +    # because we need it 2 times
> +    download(base_url + "Release", tmp.fname('Release'))
> +
> +    # validate signature.
> +    # open downloaded plaintext file, and
> +    # use the urlopen object of the Release.gpg
> +    # directtly.
> +    try:
> +        sig = urlopen(base_url + 'Release.gpg', None, 10)
> +        with tmp.open("Release", "r") as signed:
> +
> +            overall_status = OverallStatus()
> +
> +            # verify detached signature
> +            sigs = ctx.verify(sig, signed, None)
> +
> +            for s in sigs:
> +                status = check_signature(ctx, s)
> +                overall_status.add(status)
> +
> +            if overall_status.to_exitcode():
> +                raise InvalidSignature('Failed to verify Release file')
> +    finally:
> +        sig.close()

incorrect error handling

If Release.gpg is not on the mirror, i get:

  File "/home/manut/Projects/elbe/elbe/elbe", line 55, in <module>
    cmdmod.run_command(sys.argv[2:])
  File "/home/manut/Projects/elbe/elbe/elbepack/commands/init.py", line 168, in run_command
    copy_kinitrd(xml.node("/initvm"), out_path)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 231, in copy_kinitrd
    download_kinitrd(tmp, suite, mirror)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 169, in download_kinitrd
    download_release(tmp, base_url)
  File "/home/manut/Projects/elbe/elbe/elbepack/debinstaller.py", line 157, in download_release
    sig.close()
UnboundLocalError: local variable 'sig' referenced before assignment

I just found the same issue in hashes.py..

> +def download_kinitrd(tmp, suite, mirror):
> +    base_url = "%s/dists/%s/" % (
> +        mirror.replace("LOCALMACHINE", "localhost"), suite)
> +    installer_path = "main/installer-amd64/current/images/"
> +
> +    setup_apt_keyring(tmp.fname('/'), 'pubring.gpg')
> +
> +    # download release file and check
> +    # signature
> +    download_release(tmp, base_url)
> +
> +    # parse Release file, and remember hashvalues
> +    # we are interested in
> +    interesting = [installer_path + 'SHA256SUMS']
> +    release_file = ReleaseFile(base_url, tmp.fname('Release'), interesting)
> +
> +    # now download and validate SHA256SUMS
> +    release_file.download_and_validate_file(
> +            installer_path + 'SHA256SUMS',
> +            tmp.fname('SHA256SUMS'))
> +
> +    # now we have a valid SHA256SUMS file
> +    # parse it
> +    interesting = ['./cdrom/initrd.gz',
> +                   './cdrom/vmlinuz',
> +                   './netboot/debian-installer/amd64/initrd.gz',
> +                   './netboot/debian-installer/amd64/linux']
> +    sha256_sums = SHA256SUMSFile(
> +            base_url + installer_path,
> +            tmp.fname('SHA256SUMS'),
> +            interesting)
> +
> +    # and then download the files we actually want
> +    for p, ln in zip(interesting, ['initrd-cdrom.gz',
> +                                   'linux-cdrom',
> +                                   'initrd.gz',
> +                                   'vmlinuz']):
> +        sha256_sums.download_and_validate_file(
> +                p,
> +                tmp.fname(ln))
> +
> +
> +def get_primary_mirror(prj):
> +    if prj.has("mirror/primary_host"):
> +        m = prj.node("mirror")
> +
> +        mirror = m.text("primary_proto") + "://"
> +        mirror += m.text("primary_host") + "/"
> +        mirror += m.text("primary_path")
> +    else:
> +        raise NoKinitrdException("Broken xml file: "
> +                                 "no cdrom and no primary host")

no cdrom?? this is not checked.

Why not use elbexml.py / get_primary_mirror ?

> +    return mirror
> +
> +
> +def copy_kinitrd(prj, target_dir):
> +
> +    suite = prj.text("suite")
> +
> +    try:
> +        tmp = TmpdirFilesystem()
> +        if prj.has("mirror/cdrom"):
> +            system('7z x -o%s "%s" initrd-cdrom.gz vmlinuz' %
> +                   (tmp.fname('/'), prj.text("mirror/cdrom")))

we should use check_call instead for new code