[OpenWrt-Devel] [PATCH RFC] base-files: add /sbin/check_image

Mon Aug 19 16:52:02 EDT 2019

On 19.08.2019 10:40, Rafał Miłecki wrote:
> This new executable allows validating firmware file. Its main advantage
> is JSON output which should allow all kind of UIs to provide a
> meaningful feedback on possible validation issues. Used design allows
> checking functions to mark firmware as totally unsupported (FORCEABLE=0)
> and prevent user from forcing its installation.
> 
> This commit updates /sbin/sysupgrade to use that new validation method
> so no code is duplicated. Further plans for this feature are:
> 1) Add ubus method calling /sbin/check_image
> 2) Introduce platform checks extending output JSON
> 3) Extend "sysupgrade" ubus method to use check_image so it's possible
>     and safe to upgrade without using /sbin/sysupgrade
> 
> Output example:
> {
>          "tests": {
>                  "fwtool_signature": true,
>                  "fwtool_device_match": true
>          },
>          "valid": true,
>          "forceable": true
> }
> 
> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> ---
> I'm not sure how to implement platform checks that may set extra JSON
> object fields. I'd e.g. Broadcom targets to allow something like:
> "tests": {
> 	"trx_checksum": true,
> 	"platform_device_match": true,
> 	"fwtool_signature": true,
> 	"fwtool_device_match": true
> }
> 
> Any ideas?

IRC discussion on this topic:

[10:42] <rmilecki> jow: i'd like you give you some idea how I'd like LuCI to behave when doing sysupgrade and see if that sounds good to you
[10:42] <rmilecki> i'd like LuCI to  1) upload firmware  2) validate firmware using /sbin/check_image  3) if firmware is not valid but can be forced, display a proper warning + info why image validation has failed
[10:43] <rmilecki> jow: see [PATCH RFC] base-files: add /sbin/check_image
[10:53] <jow> rmilecki: why a new one-off script with an overly generic name instead of "sysupgrade -c" ?
[10:53] <jow> (didn't see the RFC mail yet, will read it now)
[10:55] <rmilecki> 1) I find /sbin/sysupgrade too big/messy already
[10:55] <rmilecki> 2) it's meant to be somehow independent as I want to expose it as a separated ubus method in the future
[10:55] <rmilecki> 3) it's not meant to mess with /sbin/sysupgrade variables
[10:56] <rmilecki> i'd like sysupgrade to be fully exposed using ubus at some point
[11:51] <jow> rmilecki: I do not like the idea at all to have firmware image checks outside of sysupgrade
[11:51] <russell--> hmm. i'm getting a kernel panic on boot of a meraki mr24 (which had been working okay a little while ago), did a dirclean, panic persists. and yet another recent image boots. https://paste.debian.net/1096534/
[11:52] <jow> and I see no difference in difficulty to either expose "/sbin/check_image %s" or "/sbin/sysupgrade -c %s"
[11:53] <russell--> i wonder if the rootfs is too big?
[11:53] <jow> rmilecki: before exposing sysupgrade to ubus, you need to think about large file/blob handling first
[11:54] <jow> luci right now uses out-of-band facilities for that (upload the image via HTTP POST + CGI program to fixed location, thne have ubus methods to operate on that)
[11:54] <rmilecki> jow: i didn't mean to change that part
[11:55] <jow> we also have an "fwtool" already
[11:55] <rmilecki> what about it?
[11:55] <jow> so we'll end up with three idependant, wildly different names tools all somehow dealing with firmware images
[11:56] <jow> fwtool, check_image and sysupgrade
[11:56] <jow> *named
[11:56] <jow> intuitively I'd expect fwtool to deal with all things related to firmware images, I'd expect check_image to belong to imagemagick or something and sysupgrade to be something like apt-get upgrade
[11:57] <jow> or apt-get dist-upgrade rather
[11:57] <rmilecki> so basically move my changes into fwtool?
[11:57] <jow> but thats not your fault obviously, just an illustration of the current mess
[11:58] <rmilecki> that /sbin/sysupgrade is mess I've hard time to deal with
[11:58] <jow> why? because its shell?
[11:58] <rmilecki> no, just file design
[11:58] <rmilecki> amount of variables, options, calls
[11:58] <rmilecki> i can clean it up, see if you like it, but after all, i'd like to simplify it to the minimum anywya
[11:58] <jow> we should refactor it then instead of adding yet another half-finished tool in parallel
[11:58] <rmilecki> so I don't want to waste time cleaning it up
[11:59] <jow> but maybe I should clarfiy
[11:59] <rmilecki> i want /sbin/sysupgrade to be just a command line interface for ubus call system sysupgrade
[11:59] <jow> correct
[11:59] <rmilecki> that means firmware validation won't belong there
[11:59] <jow> so if you put your check_image into /usr/libexec/ and have sysupgrade call that internally to verify its images, then fine for me
[12:00] <jow> but please do not make it a user-facing utility in $PATH
[12:00] <rmilecki> jow: fine, that sounds good to me
[12:00] <jow> we already have too many wildly inconsistent badly designed CLI tools
[12:00] <rmilecki> i have no problem with that
[12:00] <rmilecki> i never meant /sbin/check_image to be called directly
[12:00] <jow> okay, the /sbin/ somehow implied that
[12:00] <rmilecki> so I probaly should have used /usr/libexec/ since the beginning
[12:00] <rmilecki> my bad
[12:01] <rmilecki> jow: do you have any idea for extenging that check_image with platform checks & JSON output?
[12:01] <rmilecki> *extending
[12:02] <jow> well first I'd refactor sysugprade (or /lib/upgrade/* code rather) to not use "echo" or "printf" for outputting messages but a log function, something like msg_info(), msg_warn(), msg_err()
[12:03] <jow> then you can add a global switch (e.g. env var) which is used by these functins to decide whether to print the message or to append it to some internal json stack
[12:04] <jow> the platform_* checks are invoked anyway from a list iirc (something like `for fn in $platform_checks; do $fn ...; done) so you can simply fgather your json in the same place
[12:05] <rmilecki> jow: i may need some example... so should I have something like "platform_check_image" or "platform_check_image2" that woudl do... what?
[12:05] <rmilecki> msg_err "trx_checksum" "Firmware has wrong checksum"
[12:05] <rmilecki> and that msg_err should either print or call json_add_string?
[12:06] <rmilecki> or json_add_boolean rather
[12:06] <jow> e.g.  json_add_object; for fn in $platform_checks; do $fn ...; test $? = 0 && json_add_bool "$fn" 1 || json_add_bool "$fn" 0; done; json_close_object
[12:06] <rmilecki> ah, so a new variable with a list of platform functiosn perorming validation checks
[12:07] <jow> iirc sysupgrade already is structured this way
[12:07] <rmilecki> yes
[12:08] <rmilecki> what if I have one platform function that could and should perform few validation checks?
[12:08] <rmilecki> i may have e.g. "platform_validate_asus_trx" that may want to perform "trx_checksum" validation AND "asus_device_match" validation
[12:10] <jow> I'd say maintain a list variable holding the check functions to call
[12:11] <jow> instead of declare platform_check_image() { }   do something like   append PLATFORM_CHECK_FUNCTIONS trx_checksum; append PLATFORM_CHECK_FUNCTIONS asus_device_match
[12:11] <jow> then have a global platform_check_image() implementation that loops $PLATFORM_CHECK_FUNCTIONS, records the success state for each and finally returns true or false depending on whether all checks were true
[12:11] <rmilecki> i understand that
[12:11] <rmilecki> it just may not be fully optimal
[12:12] <rmilecki> if I can do few validation checks with a single image read, i'd like to have one platform check function report few validation results
[12:13] <rmilecki> e.g. i don't want to have "asus_device_match", "asus_checksum" and "asus_version_check" read the some firmware header
[12:13] <jow> well then have a single procedure and give it some notification mechanism callback
[12:13] <rmilecki> is using callback functions in bash OK?
[12:13] <rmilecki> just asking
[12:13] <jow> notify_check_result asus_device_match 1; notify_check_result asus_checksum 1; notify_check_result asus_version_check 0
[12:14] <rmilecki> jow: that would solve my problem
[12:14] <rmilecki> jow: ok, thanks for all the tips, i'll be back working on that in next days
[12:14] * rmilecki is going to eat sth
[12:16] <KanjiMonster> splitting the platform_image_check into subchecks would make it easier to add a generic way of ignoring certain checks (by e.g. adding something like "-F asus_device_match" you would ignore the result of the asus_device_match check, but would still fail if the checksum is wrong etc)
[12:22] <rmilecki> KanjiMonster: great, more features; )
[12:22] <rmilecki> ;)

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel