[PATCH] imagebuilder: add package signature verification

[Paul Spooren]

Hi,

On Mon Sep 14, 2020 at 12:27 PM HST, Baptiste Jonglez wrote:
> Hi,
>
> Thanks for the patch, it looks good but comments below:
>
> On 25-08-20, Paul Spooren wrote:
> > The ImageBuilder downloads pre-built packages and adds them to images.
> > This process uses `opkg` which has the capability to verify package list
> > signatures, as enabled per default on running OpenWrt devices.
> >
> > Until now this was disabled for ImageBuilders because neither the OPKG
> > keys nor the `opkg-add` script was present during first packagelist
> > update.
> > 
> > To harden the ImageBuilder against *drive-by-download-attacks* both keys
> > and verification script are added to the ImageBuilder allowing OPKG to
> > verify downloaded package indices.
> > 
> > This commit adds `opkg-add` to the IB scripts folder, as it is just a
> > shell script. The keys folder is added to IBs TOPDIR to have an obvious
> > place for users to store their own keys. The `option check_signature` is
> > appended to the repositories.conf file.
>
> With this patch, the imagebuilder gives an error while trying to fetch a
> signature for the local package index:
>
> Downloading
> https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.gz
> Updated list of available packages in
> /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/openwrt_base
> Downloading
> https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.sig
> Signature check passed.
> Downloading file:packages/Packages
> Updated list of available packages in
> /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder
> Downloading file:packages/Packages.sig
> Signature file download failed.
> Remove wrong Signature file.
> Collected errors:
> * copy_file: packages/Packages.sig: No such file or directory.
> * file_copy: Failed to copy file packages/Packages.sig to
> /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder.sig.
>
> However, it works fine in the end: I think packages coming from a local
> repository are special-cased when it comes to verifying signature.

What do you think about the following patch disabling signature checks
for the local cache?

```diff
diff --git a/libopkg/opkg_cmd.c b/libopkg/opkg_cmd.c
index a329558..a6b5773 100644
--- a/libopkg/opkg_cmd.c
+++ b/libopkg/opkg_cmd.c
@@ -143,7 +143,7 @@ static int opkg_update_cmd(int argc, char **argv)
                }
                free(url);
 #if defined(HAVE_USIGN)
-               if (pkglist_dl_error == 0 && conf->check_signature) {
+               if (pkglist_dl_error == 0 && conf->check_signature && !strncmp("file:packages", src->value, 13)) {
                        /* download detached signitures to verify the package lists */
                        /* get the url for the sig file */
                        if (src->extra_data)    /* debian style? */
```

>
> It's not possible to sign this package file, because it is generated
> locally by the imagebuilder and we don't have access to any usign
> private
> key. Signing a locally-generated file doesn't make much sense anyway.
> So, it probably needs to be fixed in opkg.
>
> > All of the above only happens if the Buildbot runs with the
> > SIGNED_PACKAGES option.
>
> As far as I can tell, you don't actually rely on the package index
> signatures that are generated on the host? You are using the usign keys
> from the openwrt-keyring package as well as the locally-generated build
> key, both of which are enabled by SIGNED_PACKAGES. This is far from
> trivial and should be added to the commit message or as a comment.

True, it's not trivial and should be included in the commit message.
Sidenote, these keys are included in every image as well.

>
> I'm asking because my first idea was to depend on SIGNATURE_CHECK. It
> seems more logical but it's actually not relevant: SIGNATURE_CHECK
> enables
> signature checking in the target device opkg configuration, so it's
> completely unrelated to what should happen in the imagebuilder.
>

Paul