auc: preserve image's uci-defaults upon upgrade

[bam80]

uci-defaults scripts are essential for accessing device after firmware reset.

The device or user-facing PC might have no (easy accessible) Ethernet ports,
or wired LAN might not be accessible for any other reason.
In this case, uci-defaults script can contain code to setup WiFi AP on
the very first boot, thus granting the access.
Such uci-defaults script should survive the firmware upgrade with AUC,
if it was in the firmware image initially.
Otherwise, access to the device can be lost next time user performs firmware Reset.

This patch fixes it by passing the file
/rom/etc/uci-defaults/99-asu-defaults (the path used by ASU server)
for inclusion to a new firmware image using BuildRequest API's "defaults" member which allows specifying a content of that uci-defaults script:
https://sysupgrade.openwrt.org/ui/

fixes openwrt/asu#575

@hauke @ynezz @aparcar

[dangowrt]

The concept itself is ok and I understand the use-case for it.
The implementation needs to be improved regarding the current use-after-free problem, in style (variable declarations in the middle of the function should go to the top -- or the whole thing into a function of its own) and proper error handling has to be added.

[dangowrt]

You should use an allocated buffer (ie. grep for blobmsg_add_string_buffer to find example) because otherwise...

[bam80]

I believe we don't need any additional buffer here.
Fixed.

[dangowrt]

... all access after this would be use-after-free (and blobmsg_add_string above doesn't duplicate the string!).

[bam80]

fixed

[dangowrt]

Moving the unmap further down may work, but the good solution would look more like this:
https://github.com/openwrt/netifd/blob/7a58b995fdbecd9beed57e4d66d42cb3cf66aee2/system-linux.c#L2614

[bam80]

Oh, please let me don't do that - it was a whole idea of using mmap() instead of fread() - to not allocate buffers manually and let OS do work for us in a best way possible!

[bam80]

Thanks for review Daniel!

[bam80]

@dangowrt could we proceed please?

[bam80]

Ping please?

[bam80]

@dangowrt ping please?

[efahl]

Excellent, I see no issues.

[bam80]

@efahl thank you for the review, could you merge it?

[efahl]

Not me, I'm just another person who submits PRs like you, without commit privileges.

[dangowrt]

Please explain the use-case of this new feature. I can imagine it, but it should be written in the commit description which currently sounds almost like if you were fixing a bug:

Without this, we can get inaccessible device after upgrade.

Apart from that the code looks acceptable, given that all of auc is not exactly beautiful and should be re-written in ucode once we got libuclient bindings for ucode, I would merge this once the commit description gives some information about what is being done and why and how one can use that feature and what it is typically used for.

[efahl]

@bam80, maybe you can get some ideas here to rewrite the commit message:

My use case is to define port mappings on my generic devices (x86), so I go to https://firmware-selector.openwrt.org/, add a script that does something like

lan="eth1 eth2 eth3"
wan="eth0"
ucidef_set_interfaces_lan_wan "$lan" "$wan"

If I do auc, that gets wiped out on upgrade, so I have to manually run the selector, paste that in again, download and so on... Would be very much more convenient if auc supported that defaults field in the asu server's API.

I do see others using the defaults mechanism to do file system resizing using the expand root script, here's a recent example: https://forum.openwrt.org/t/resize-emmc-partition-and-attended-sysupgrade/181712/4

[efahl]

How about?

Achieve feature parity with firmware selector, which allows inclusion of first boot uci-defaults files without rebuilding from scratch.

And now that I'm really thinking about it, it would be even nicer to expose the filename in a command line option, so that users could add or modify the existing 99-asu-defaults. Maybe something like this, which would read contents from the specified file:

auc -d ./my-uci-defaults ...

[bam80]

I think my commit message is good enough, if @dangowrt doesn't think so - I can make further adjustments.

Regarding the path, 99-asu-defaults is hard-coded here:
https://github.com/openwrt/asu/blob/3ef4757b9fcc401b3d18ebb6954276b0210f07fb/asu/build.py#L202
that is why it's essential to preserve exactly that path, no need to complicate this here.
Other expanded functionality beyond that could go to a separate PR.

[efahl]

I agree your commit message is fine, our paths crossed, I didn't see your new commit until after I had posted.

I think you misunderstand my -d file option. It would not replace 99-asu-defaults in the generated .img, but rather use a different source to read into the defaults blob when sending the request to the ASU build API. This would allow you to modify or even create 99-asu-defaults in the image without ever using the firmware selector in the first place.

But, yes, I agree, let's not complicate the issue now, leave things as-is and get the commit done then think about enhancements later.

[efahl]

I'm still trying to get all the pieces into my head, as there are a lot of different moving parts here. The reason I keep thinking this is best solved at the sysupgrade level is because that is the common point for both auc and LuCI Attended Sysupgrade, both of which suffer from the issue that @bam80 is trying to solve here. Fixing it in auc still leaves the bug in LuCI ASU.

And then there's the issue with ext4, since there's no /rom partition at all from which to carry over any custom uci-defaults script, so it seems to me that sysupgrade would again be the proper place to detect this (maybe just before it does first boot, stashing the custom scripts somewhere???) allowing it to restore them at the next sysupgrade???

And then there is the issue of carrying over other custom uci-defaults scripts that may not be using the ASU server's convention of 99-asu-defaults (maybe you used a full build system or image builder for that initial installation instead of Firmware Selector)...

[bam80]

@efahl no need to put all the issues into the one bin, that would just make things more complicated.
All off those you mentioned are not directly related with this one, and should be addressed separately.

Let's please keep this discussion dedicated to the issue here.
If you care, you can open related issues for each of them you mentioned, with the reference link to this one.
Thanks.

[bam80]

Those mentioned in my comment on 17.02. Just pushed to the master:

@rmilecki I see those commits were without PRs, does it mean you have the merge rights?
We could tweak the commit message if you want then, and get things done.

[bam80]

I would go with something like:

@rmilecki I adopted your suggestion for the description, please let me know if you see it satisfiable, I'll transfer it to the commit message then.
Thanks.

[rmilecki]

@efahl: quick summary:

1. We need images to contain 99-asu-defaults in their RO filesystem (squashfs)
2. There is no way for sysupgrade to modify downloaded image's RO filesystem (squashfs) and append 99-asu-defaults to it
3. The only solution is to have images generated with 99-asu-defaults already included which this Pull Request takes care of

@bam80: I mean to take care of this Pull Request, just give me few days to find time for it

[efahl]

no need to put all the issues into the one bin

On the contrary, there really is only one issue, namely "how do we maintain custom uci-defaults across sysupgrades." If there is a general solution to this in the infrastructure layer (i.e., sysupgrade) then various hacks to auc and LuCI Attended Sysupgrade are not needed.

After looking into how all of sysupgrade, firmware selector, LuCI ASU, auc, ASU server and the downloads site interact, I don't think this very specific patch to auc is appropriate, I think it should be addressed where all current and future clients of sysupgrade will benefit.

[rmilecki]

I can't imagine modifying squashfs of downloaded firmware to append uci-defaults script (99-asu-defaults) to it. There are many firmware formats: squashfs can be MTD partition or UBI volume to start with. You would need to extract squashfs, then unpack it, modify and repack. All of that on a target device with limited tools and memory. I don't see that happening.

Now I'm thinking about it there is probably another solution: preserving files during factory reset. The more I think of it the more sense it makes. It may be a bit counter-intuitive but I don't see why we couldn't keep a list of critical files and preserve them.

[efahl]

Now I'm thinking about it there is probably another solution: preserving files during factory reset. The more I think of it the more sense it makes. It may be a bit counter-intuitive but I don't see why we couldn't keep a list of critical files and preserve them.

Yes, this is the same route I've been thinking along. If there were an alternate mechanism that did something like 1) stashing these files somewhere rather than embedding the files in the .img; or 2) augmenting sysupgrade -b with maybe added syntax in /etc/sysupgrade.conf, then I think the clients could remove code and in general just work better.

On obvious advantage is that for ext4, this PR simply doesn't work, so something else needs to be done. (Simple test case: use firmware selector to build an ext4 image with a custom uci-defaults file; install that, then run auc with this PR's patch, see that your custom uci-defaults are silently gone.)

[bam80]

Now I'm thinking about it there is probably another solution: preserving files during factory reset.

That would diminish all the purpose of the factory reset:
you won't be able to restore the initial state of the firmware with it any more. So "fixing" it that way you would break the factory reset instead.

The patch here provides clear, non obtrusive way to fix the problem:
AUC already uses the ASU server BuildRequest API and it's "defaults" member is already used by the OpenWrt Firmware Selector, so here is the simplest in-parity adjustment. No need to invent something else here.

Other problems mentioned will be addressed in a separate PRs.

[ynezz]

I don't see why we couldn't keep a list of critical files and preserve them.

Wouldn't this make persistent malware much easier?

[ynezz]

In this case, uci-defaults script can contain code to setup WiFi AP on the very first boot, thus granting the access.

In other case, such uci-default script might contain malicious code, thus re-connect device to the botnet on the very first boot? Moreover I hope, that folks will enjoy this new feature and start uploading something really juicy, not just boring WiFi secrets :-)

[bam80]

In other case, such uci-default script might contain malicious code

This is out of scope of this patch.

What you have in the uci-default is solely up to you, only you control it and the ability to put random things in it is already given by the BuildRequest API and used by the OpenWrt Firmware Selector.
This patch just carries it over between the upgrades.

[rmilecki]

Wouldn't this make persistent malware much easier?

factoryreset is just a software, do we really treat it as a malware removing tool? What prevents malware from modifying factoryreset?

[bam80]

no need to put all the issues into the one bin

On the contrary, there really is only one issue, namely "how do we maintain custom uci-defaults across sysupgrades." If there is a general solution to this in the infrastructure layer (i.e., sysupgrade) then various hacks to auc and LuCI Attended Sysupgrade are not needed.

Once again, there is no single point where the problem could be solved once and for all.
Thus, it has to be addressed in different tools, independently.

We have two cases which need to be handled a bit differently:

• ro image based root filesystem (most common)
• rw root filesystem without stable image

For the ro image-based system, core principle to follow is:
if there was uci-default script in the image initially, it is there for a reason, and it should stay there between the upgrades.

That is best solved with help of the ASU server, considering it has an API to embed the uci-defaults script and it's already used by some tools.

There are 3 known clients currently talking to the ASU server:

• Web client The OpenWrt Firmware Selector: API's "defaults" member is already used there.
• CLI AUC tool: the "defaults" API support is implemented by this patch, so it is not a hack but precise fix.
• LuCI app Atttended SysUpgrade: the "defaults" API support is to be implemented

While the AUC and Atttended SysUpgrade clients use common sysupgrade tool under the hood, the patch can't be implemented there, as sysupgrade itself doesn't talk to the ASU server.

The rw rootfs-based systems without stable image have it's own specific:
the uci-defaults scripts are gone completely on the first boot, so you have to save them somewhere, probably in your router's configuration, for their consequent restoration.
Best place to save them would be by the same path they would found in the ro system, e.g.:
/rom/etc/uci-defaults/99-asu-defaults
So you just create this file in you rw rootfs and add it's path to the /etc/sysupgrade.conf, and you're all done - this patch for AUC and the future one for the LuCI Atttended SysUpgrade app would just work! Providing you restore your configuration before the next upgrade, of course.

One thing left to be care for the rw systems:
factoryreset
For sure, we shouldn't touch it's default behavior which is just wipe everything, and it should never be changed - otherwise, there would be no "reset" any more.
But for rw rootfs case specifically (and only for it) - we could implement some uci-defaults scripts restoration logic there.
That could probably be easily achievable just by symlinking from /rom/etc/uci-defaults/99-asu-defaults to /etc/uci-defaults/ upon the "reset".

And then there is the issue of carrying over other custom uci-defaults scripts that may not be using the ASU server's convention of 99-asu-defaults (maybe you used a full build system or image builder for that initial installation instead of Firmware Selector)...

I don't actually see an issue here on this stage: nothing prevents you from using the same ASU server's established 99-asu-defaults script name in your build, as it has special meaning anyway already. But we could extend this functionality in the future, if needed.

That concludes the all that have to be done here.
Note all the fixes do neither contradict nor cancel each others.

So I'm asking let's please finish with this PR and move on to the others.

[dangowrt]

While the AUC and Atttended SysUpgrade clients use common sysupgrade tool under the hood, the patch can't be implemented there

I disagree. The feature could be implemented in sysupgrade in such a way the it will include /rom/etc/uci-defaults/99-asu-defaults in the config backup tarball, and move it to /etc/uci-defaults/ there. Sure, that won't survive a "factory reset" (via factoryreset package? Or just via firstboot on cmdline?), but should it really? Isn't that a different use-case to begin with?

@ynezz comment hits the point here:

Moreover I hope, that folks will enjoy this new feature and start uploading something really juicy, not just boring WiFi secrets :-)

Imho configuration (which usually includes some secret or confidential parts such as passwords or private keys) should always stay local. Uploading it and processing on a server isn't a responsible thing to do imho and we should try to avoid that.

That being said, I'd rather like to see more ways to append such information to the image locally than having the server do that.

[bam80]

Sure, that won't survive a "factory reset" (via factoryreset package? Or just via firstboot on cmdline?), but should it really? Isn't that a different use-case to begin with?

Of course it should, it must!
I wrote in the description why it's important, it was a whole point of this patch.

Imho configuration (which usually includes some secret or confidential parts such as passwords or private keys) should always stay local. Uploading it and processing on a server isn't a responsible thing to do imho and we should try to avoid that.

This patch is about 99-asu-defaults only which is a reserved name and the file comes from ASU server, that means if it exists you have already passed it's content to the server once, so you know what you are doing.
Nothing more is passed.

[dangowrt]

so you know what you are doing.
...
Nothing more is passed.

Imho this is problematic to begin with as people will happily submit their WiFi credentials, PPPoE passwords or even Wireguard private keys there, and that make the asu server an ever more lucrative target. The fact that people decided to have this in asu and the firmware selector gives my cold shivers and I really would not want to be quoted having said "ok" to this.

[bam80]

@dangowrt I understand your concern but the point is the ASU server's uci-defaults API is already exposed and used so this patch doesn't actually impose any more security risk than we have already, and people will continue using the API with or without this patch.

Regarding the introduction of the API itself, I think it was maybe a reasonable tradeoff as otherwise how else would you get an access to a newly flashed device if there is no LAN?

We also have to consider there is no alternative solution implemented at the moment, while the problem itself is quite significant.

So could we come to a compromise maybe?
How about this plan:

• we won't implement uci-defaults API for the Atttended SysUpgrade LuCI app so far
• we merge the AUC patch here for now, to have at least something for those who really need it. It can even remain as undocumented feature.
• meanwhile, we try to implement the needed support in factoryreset/firstboot (don't remember which is what) for both ro/rw systems.
• if that succeeds, we can consider to revert this AUC patch.

Would that work?

[bam80]

@bam80: I mean to take care of this Pull Request, just give me few days to find time for it

@rmilecki Did you have a chance?

[stangri]

The feature could be implemented in sysupgrade in such a way the it will include /rom/etc/uci-defaults/99-asu-defaults in the config backup tarball, and move it to /etc/uci-defaults/ there.

What's the plan for the ext4-based file systems which do not have the /rom?

Sorry I'm very late to the party, but it looks like an issue which is easily solved by making a custom upgrade image which has the necessary customizations (uci-defaults script) baked in, rather than creating a code which works differently for for different platforms, which tries to preserve an initial customization in the image flashed.

IMHO if your customizations cannot be preserved thru "keep settings", you should really be making a new custom image when upgrading.

Also, you're only considering the use-case where it's desirable that the changes from the previously created image persist. What about users who want a clean slate upon upgrade (or if the reset is required) and you're forcing them to carry some customizations into the new image?

Sorry, I realize some work and testing went into this PR, however I'd NACK it for the reasons above and reasons @ynezz brought up.

[bam80]

What's the plan for the ext4-based file systems which do not have the /rom?

Sorry I'm very late to the party, but it looks like an issue which is easily solved by making a custom upgrade image which has the necessary customizations (uci-defaults script) baked in, rather than creating a code which works differently for for different platforms, which tries to preserve an initial customization in the image flashed.

IMHO if your customizations cannot be preserved thru "keep settings", you should really be making a new custom image when upgrading.

@stangri
I wrote thorough description of my vision above, please see if you missed it.
Here is the excerpt regarding the rw filesystems:

The rw rootfs-based systems without stable image have it's own specific:
the uci-defaults scripts are gone completely on the first boot, so you have to save them somewhere, probably in your router's configuration, for their consequent restoration.
Best place to save them would be by the same path they would found in the ro system, e.g.:
/rom/etc/uci-defaults/99-asu-defaults
So you just create this file in you rw rootfs and add it's path to the /etc/sysupgrade.conf, and you're all done - this patch for AUC and the future one for the LuCI Atttended SysUpgrade app would just work!

The solution would allow to not even touch sysupgrade code.
Yes I agree making a custom upgrade image with uci-defaults script is a way to go, and this patch is all about it - it just passes the uci-defaults script to ASU server to make the custom image.

Also, you're only considering the use-case where it's desirable that the changes from the previously created image persist. What about users who want a clean slate upon upgrade (or if the reset is required) and you're forcing them to carry some customizations into the new image?

Good catch, the patch doesn't take care about wiping the current uci-defaults script or changing it in any way, indeed.
But this could be easily achieved by re-generating the image with OpenWrt Firmware Selector Web app, without the uci-defaults script at all or by copy-pasting the current version of the script and correcting it there.
Considering it shall not be needed often, I intentionally skipped the functionality for now, to keep the patch as simple as possible. The features can be added later.

If you nevertheless think it needs to be done to accept this patch, please let me know and I'll add the feature.

[CTCD]

Add an input box in ASU's advanced mode (similar to Firmware Selector Web).
The content of the input box is /etc/99-asu-defaults.

[CTCD]

Add cp /etc/uci-defaults/99-asu-defaults /etc/99-asu-defaults at the end of the /etc/uci-defaults/99-asu-defaults script.

[bam80]

Add cp /etc/uci-defaults/99-asu-defaults /etc/99-asu-defaults at the end of the /etc/uci-defaults/99-asu-defaults script.

I don't see the point - this won't work for the reset case when all the configuration is wiped.
For that, you still need your config be in ROM.

And for the other cases, the config changes that have been made by 99-asu-defaults are preserved anyway as part of normal upgrade process.

[CTCD]

Add at the end of the script.cp /etc/uci-defaults/99-asu-defaults /etc/99-asu-defaults``/etc/uci-defaults/99-asu-defaults

I don't see the point - this won't work for the reset case when all the configuration is wiped. For that, you still need your config be in ROM.

And for the other cases, the config changes that have been made by are preserved anyway as part of normal upgrade process.99-asu-defaults

The content in the input box is based on /etc/99-asu-defaults, which is the script executed during the initial boot and is sent to the server. It is definitely kept in ROM.

When you upgrade again, you can modify the content in the input box to your preferred configuration and send it to the server again. However, you must keep "cp /etc/uci-defaults/99-asu-defaults /etc/99-asu-defaults" at the end of /etc/uci-defaults/99-asu-defaults to prepare for the next upgrade.

[bam80]

Add an input box in ASU's advanced mode (similar to Firmware Selector Web).
The content of the input box is /etc/99-asu-defaults.

You mean, make the input file name/content to send to the server configurable?
Where - in the LuCI app, auc CLI?

I would suggest other scheme:
If the /etc/uci-defaults/99-asu-defaults file present in the rw filesystem, use it.
Otherwise, fallback to the ROM's /rom/etc/uci-defaults/99-asu-defaults.
Simple and effective - it would also allow us to remove the file entirely by placing an empty file on the rw path.

Anyway, I think it's not necessary here as the patch includes bare minimum.
We can extend it later as appropriate.

[CTCD]

This script is for upgrading OpenWrt firmware and supports preserving user configuration and installed packages after resetting the device:
wget -O /etc/99-asu-defaults "https://raw.githubusercontent.com/CTCD/99-asu-defaults/main/99-asu-defaults" && sed -n '/Install upasu/,$p' /etc/99-asu-defaults | sh

[bam80]

@CTCD I see you pass defaults to the server in there, what is the fundamental difference from the solution suggested here then?
You just made your own auc with the script?