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 <[email protected]> --- 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 [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
