Add handler for ipk packages

---
 handlers/Config.in     |  6 ++++
 handlers/Makefile      |  1 +
 handlers/ipk_handler.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)
 create mode 100644 handlers/ipk_handler.c

zazid wrote on Tue, Mar 16, 2021 at 04:20:26AM +0100:
> +	snprintf(init_cmd, sizeof(init_cmd), "%s %s", INSTALL_CMD, pkg_name);
> +	ret = system(init_cmd);

This actually looks a lot like the pipe handler (except the data is not
piped but passed as argument) -- would it make sense to make appending
local filename as last argument an option of pipe handler (conflicting
with installed_directly)?

As far as I can see one would just need to set progress to 100% after
the copy in that case? (I seem to remember something about needing to
"read" the data even if it's not copied, but could not find that again
now... Calling copyfile() with skip=1 might be better?)



Note that for my usecase (podman), I wasn't thorough enough and reading
from stdin also stores a copy of the file so I no longer have much
reason of pushing for a pipe handler and a general "pass previously
extracted file to command" handler would probably make enough sense for
me.
I spent a while trying to figure a way to make podman efficient but they
need to read some file at the end of the archive to decide what to
extract so it's just not going to be great anyway :|
So in a sense that solves my fail-cmd/success-cmd problem and I'd rather
let swupdate do the extraction and check before running anything.

Hi Zazid,

On 16.03.21 04:20, zazid wrote:
> ---
>   handlers/Config.in     |  6 ++++
>   handlers/Makefile      |  1 +
>   handlers/ipk_handler.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 77 insertions(+)
>   create mode 100644 handlers/ipk_handler.c
> 
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 8571c0c..10dc173 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -290,4 +290,10 @@ config UCFW_OLD_LIBGPIOD
>   	  Rather there is no way to get this changes from the library
>   	  at build time.
>   
> +config IPKHANDLER
> +	bool "IPK package"
> +	default n
> +	help
> +	  This handler allows to install ipk packages using opkg.
> +
>   endmenu
> diff --git a/handlers/Makefile b/handlers/Makefile
> index 1449139..4d2c260 100644
> --- a/handlers/Makefile
> +++ b/handlers/Makefile
> @@ -24,3 +24,4 @@ obj-$(CONFIG_SSBLSWITCH) += ssbl_handler.o
>   obj-$(CONFIG_SWUFORWARDER_HANDLER) += swuforward_handler.o swuforward-ws.o
>   obj-$(CONFIG_UBIVOL)	+= ubivol_handler.o
>   obj-$(CONFIG_UCFWHANDLER)	+= ucfw_handler.o
> +obj-$(CONFIG_IPKHANDLER)       += ipk_handler.o
> diff --git a/handlers/ipk_handler.c b/handlers/ipk_handler.c
> new file mode 100644
> index 0000000..ce392ac
> --- /dev/null
> +++ b/handlers/ipk_handler.c
> @@ -0,0 +1,70 @@
> +/*
> + * Signed-off-by: Zakaria Zidouh <zazid@smile.fr>
> + *
> + *
> + * License:  GPL-2.0-or-later
> + */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include "swupdate.h"
> +#include "handler.h"
> +#include "util.h"
> +
> +#define INSTALL_CMD "opkg install"
> +
> +static int install_ipk_handler(struct img_type *img,
> +			void __attribute__ ((__unused__)) *data)
> +{
> +	int retVal = -1;
> +	const char* TMPDIR = get_tmpdir();
> +	int fdout;
> +	
> +	snprintf(img->extract_file, sizeof(img->extract_file), "%s%s",
> +			 TMPDIR , img->fname);
> +	
> +	fdout =  openfileoutput(img->extract_file);
> +	if (fdout < 0){
> +		ERROR("Cannot open %s file", img->extract_file);
> +		return fdout;
> +	}
> +	retVal = extract_next_file(img->fdin, fdout, img->offset, img->compressed,
> +					img->is_encrypted, img->ivt_ascii, img->sha256);


> +	if(!retVal){
> +		ERROR("Can't copy file");
> +	}
> +	if(strlen(img->path) != 0){
> +		TRACE("The path variable is not considred. The package will be installed in rootfs");
> +	}
> +	TRACE("Installing %s package", img->fname);
> +	retVal = install_package(img->extract_file);
> +	if (!retVal){
> +		ERROR("Can't install the package");
> +		return retVal;
> +	}
> +	return 0;
> +}

The implementation has some heavy issues. First, it is a wrapper for 
"opkg". Please read the recent thread :

https://groups.google.com/g/swupdate/c/C6tx1-ryKZ8/m/GWCJfxeQAgAJ

A reliable tool for updating must be as much as possible self contained. 
Just grep for system() calls in SWUpdate. Have you not found ? Right, 
there is a reason. Atacker can try to replace /bin/sh or something in 
the rootfs. The updater should not depend as much as possible from 
external tools. Not only, a call to system() is implicitely a fork of 
the whole swupdate process, and it increases the resource requirement of 
the updater.

Instead of it, you should use the libopkg library as part of the opkg 
project and link it to SWUpdate, and call it inside this handler.

The second heavy issue is that you drop streaming, that is the package 
is first extracted and saved temporarily on the device's filesystem. If 
this is a simple way to do, it is not what SWUpdate tries to reach. 
SWUpdate allows to stream the whole software without temporary copies, 
but it is not true with this patch.

> +
> +int install_package(char *pkg_name)
> +{
> +	int ret = -1;
> +	char init_cmd[MAX_IMAGE_FNAME];
> +	snprintf(init_cmd, sizeof(init_cmd), "%s %s", INSTALL_CMD, pkg_name);
> +	ret = system(init_cmd);

No way.

> +	if (ret != 0){
> +		ERROR("error occurred during package installation");
> +		return ret;
> +	}
> +	return EXIT_SUCCESS;
> +}
> +
> +__attribute__((constructor))
> +	void ipk_handler(void)
> +{
> +		register_handler("ipk", install_ipk_handler, FILE_HANDLER, NULL);
> +}
> 

Best regards,
Stefano Babic

Hi Dominique,

On 16.03.21 09:48, Dominique MARTINET wrote:
> zazid wrote on Tue, Mar 16, 2021 at 04:20:26AM +0100:
>> +	snprintf(init_cmd, sizeof(init_cmd), "%s %s", INSTALL_CMD, pkg_name);
>> +	ret = system(init_cmd);
> 
> This actually looks a lot like the pipe handler (except the data is not
> piped but passed as argument) -- would it make sense to make appending
> local filename as last argument an option of pipe handler (conflicting
> with installed_directly)?
> 
> As far as I can see one would just need to set progress to 100% after
> the copy in that case? (I seem to remember something about needing to
> "read" the data even if it's not copied, but could not find that again
> now... Calling copyfile() with skip=1 might be better?)
> 
> 
> 
> Note that for my usecase (podman), I wasn't thorough enough and reading
> from stdin also stores a copy of the file

Ouch.....

> so I no longer have much
> reason of pushing for a pipe handler and a general "pass previously
> extracted file to command" handler would probably make enough sense for
> me.

Got it. So I guess you won't repost your series, but outside the handler 
there are also a couple of generic fixes (for run_system_cmd). Do you 
plan to repost it ?

> I spent a while trying to figure a way to make podman efficient but they
> need to read some file at the end of the archive to decide what to
> extract so it's just not going to be great anyway :|

Aargh......

This forbids the streaming of containers.

> So in a sense that solves my fail-cmd/success-cmd problem and I'd rather
> let swupdate do the extraction and check before running anything.
> 

Best regards,
Stefano Babic

Stefano Babic wrote on Tue, Mar 16, 2021 at 12:38:08PM +0100:
> > Note that for my usecase (podman), I wasn't thorough enough and reading
> > from stdin also stores a copy of the file
> 
> Ouch.....
> 
> > so I no longer have much
> > reason of pushing for a pipe handler and a general "pass previously
> > extracted file to command" handler would probably make enough sense for
> > me.
> 
> Got it. So I guess you won't repost your series,

While the idea was nice I cannot think of a way to make piping work for
me right now, so either the handler could do both piping/executing a
script with argument (which is a bit klumsy in my opinion) or it will
probably be best to drop it for now.

I'll start a new thread for this discussion.

> but outside the handler
> there are also a couple of generic fixes (for run_system_cmd). Do you plan
> to repost it ?

Yes for the other fixes, these won't hurt whatever we decide for the
handler itself.

On 17.03.21 01:17, Dominique MARTINET wrote:
> Stefano Babic wrote on Tue, Mar 16, 2021 at 12:38:08PM +0100:
>>> Note that for my usecase (podman), I wasn't thorough enough and reading
>>> from stdin also stores a copy of the file
>>
>> Ouch.....
>>
>>> so I no longer have much
>>> reason of pushing for a pipe handler and a general "pass previously
>>> extracted file to command" handler would probably make enough sense for
>>> me.
>>
>> Got it. So I guess you won't repost your series,
> 
> While the idea was nice I cannot think of a way to make piping work for
> me right now, so either the handler could do both piping/executing a
> script with argument (which is a bit klumsy in my opinion)

Agree, I am surprised as well. Due t othe size of the containers, a 
stream solution is more as desired. But if data at the end of the 
archive are read to decide what should be extracted, the format is not 
streamable and there is no solution.

> or it will
> probably be best to drop it for now.
> 

Ok

> I'll start a new thread for this discussion.
> 
>> but outside the handler
>> there are also a couple of generic fixes (for run_system_cmd). Do you plan
>> to repost it ?
> 
> Yes for the other fixes, these won't hurt whatever we decide for the
> handler itself.
> 

Thanks !

Best regards,
Stefano Babic

Hi,

> > [...]
> > Note that for my usecase (podman), I wasn't thorough enough and reading
> > from stdin also stores a copy of the file
> 
> Ouch.....
> 
> > so I no longer have much
> > reason of pushing for a pipe handler and a general "pass previously
> > extracted file to command" handler would probably make enough sense for
> > me.
> 
> Got it. So I guess you won't repost your series, but outside the handler there
> are also a couple of generic fixes (for run_system_cmd). Do you plan to repost
> it ?
> 
> > I spent a while trying to figure a way to make podman efficient but they
> > need to read some file at the end of the archive to decide what to
> > extract so it's just not going to be great anyway :|
> 
> Aargh......
> 
> This forbids the streaming of containers.

Have you tried to address this upstream at podman?
I mean this is not a corner use case these days to update containers in
streaming mode via a firmware update mechamism....


Kind regards,
   Christian

Christian Storm wrote on Wed, Mar 17, 2021 at 11:05:18AM +0100:
> Have you tried to address this upstream at podman?
> I mean this is not a corner use case these days to update containers in
> streaming mode via a firmware update mechamism....

I will probably write a ticket for that but that will require some
intensive rework so I don't want to rely on it at this point.


I suspect the bulk of their users has direct internet access in which
case a simple "podman pull" command will do the right thing™ and only
download what is strictly required, and this is also going to be our
recommended way of upgrading for OTA updates.
(In that mode, container images can be signed, so integrity is not a
problem)

For bundled updates we basically have the choice of just putting
container update tarballs flat next to the swu which would be efficient
or bundle them in and pay for an extra copy -- and for me having an
extra copy is more practical than fumbling around looking for data,
validating it ourselves (because for some reason you can't sign local
images?!), etc...
Ideally a later version would have efficient streaming, but I gave up
on it short term.