diff mbox series

[OpenWrt-Devel,1/2] base-files: indicate initial setup by uci system config option

Message ID 20191108120520.1966-1-freifunk@adrianschmutzler.de
State Rejected
Delegated to: Adrian Schmutzler
Headers show
Series [OpenWrt-Devel,1/2] base-files: indicate initial setup by uci system config option | expand

Commit Message

Adrian Schmutzler Nov. 8, 2019, 12:05 p.m. UTC
This provides a uci system config setting that will be set only
during initial setup. This can be used by uci-defaults script to
determine whether they are run during initial setup or after a
sysupgrade.

Since the setting is removed again after uci-defaults have been
processed, it won't be recognized by the user on the running device,
but can be exploited also for downstream setup tasks.

Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
---
 package/base-files/files/bin/config_generate                  | 1 +
 .../base-files/files/etc/uci-defaults/90_end-initial-setup    | 4 ++++
 2 files changed, 5 insertions(+)
 create mode 100644 package/base-files/files/etc/uci-defaults/90_end-initial-setup

Comments

Piotr Dymacz Nov. 16, 2019, 3:50 p.m. UTC | #1
Hi Adrian,

On 08.11.2019 13:05, Adrian Schmutzler wrote:
> This provides a uci system config setting that will be set only
> during initial setup. This can be used by uci-defaults script to
> determine whether they are run during initial setup or after a
> sysupgrade.
> 
> Since the setting is removed again after uci-defaults have been
> processed, it won't be recognized by the user on the running device,
> but can be exploited also for downstream setup tasks.

This looks for me like a misuse of uci configuration and some kind of 
workaround for a missing feature, maybe in procd/ubus?

NAK on this one from me.
Adrian Schmutzler Nov. 16, 2019, 11:36 p.m. UTC | #2
Hi Piotr,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Piotr Dymacz
> Sent: Samstag, 16. November 2019 16:50
> To: Adrian Schmutzler <freifunk@adrianschmutzler.de>; openwrt-
> devel@lists.openwrt.org
> Subject: Re: [OpenWrt-Devel] [PATCH 1/2] base-files: indicate initial setup by
> uci system config option
> 
> Hi Adrian,
> 
> On 08.11.2019 13:05, Adrian Schmutzler wrote:
> > This provides a uci system config setting that will be set only during
> > initial setup. This can be used by uci-defaults script to determine
> > whether they are run during initial setup or after a sysupgrade.
> >
> > Since the setting is removed again after uci-defaults have been
> > processed, it won't be recognized by the user on the running device,
> > but can be exploited also for downstream setup tasks.
> 
> This looks for me like a misuse of uci configuration and some kind of
> workaround for a missing feature, maybe in procd/ubus?

Maybe I'm not familiar enough with it, but I currently can't think of a method indicating a fresh installation with procd/ubus.

> 
> NAK on this one from me.

Similar to the label MAC hostname/SSID stuff, this was just a nice-to-have thing for me. I will mark both patches as rejected.

Best

Adrian

> 
> --
> Cheers,
> Piotr
> 
> >
> > Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
> > ---
> >   package/base-files/files/bin/config_generate                  | 1 +
> >   .../base-files/files/etc/uci-defaults/90_end-initial-setup    | 4 ++++
> >   2 files changed, 5 insertions(+)
> >   create mode 100644
> > package/base-files/files/etc/uci-defaults/90_end-initial-setup
> >
> > diff --git a/package/base-files/files/bin/config_generate
> > b/package/base-files/files/bin/config_generate
> > index b473eba9e9..273561229a 100755
> > --- a/package/base-files/files/bin/config_generate
> > +++ b/package/base-files/files/bin/config_generate
> > @@ -243,6 +243,7 @@ generate_static_system() {
> >   		set system.@system[-1].ttylogin='0'
> >   		set system.@system[-1].log_size='64'
> >   		set system.@system[-1].urandom_seed='0'
> > +		set system.@system[-1].initial_setup='1'
> >
> >   		delete system.ntp
> >   		set system.ntp='timeserver'
> > diff --git
> > a/package/base-files/files/etc/uci-defaults/90_end-initial-setup
> > b/package/base-files/files/etc/uci-defaults/90_end-initial-setup
> > new file mode 100644
> > index 0000000000..779d858d5f
> > --- /dev/null
> > +++ b/package/base-files/files/etc/uci-defaults/90_end-initial-setup
> > @@ -0,0 +1,4 @@
> > +uci -q delete system.@system[0].initial_setup uci commit system
> > +
> > +exit 0
> >
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Piotr Dymacz Nov. 17, 2019, 12:55 p.m. UTC | #3
Hi Adrian,

On 17.11.2019 00:36, mail@adrianschmutzler.de wrote:
> Hi Piotr,
> 
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
>> On Behalf Of Piotr Dymacz
>> Sent: Samstag, 16. November 2019 16:50
>> To: Adrian Schmutzler <freifunk@adrianschmutzler.de>; openwrt-
>> devel@lists.openwrt.org
>> Subject: Re: [OpenWrt-Devel] [PATCH 1/2] base-files: indicate initial setup by
>> uci system config option
>> 
>> Hi Adrian,
>> 
>> On 08.11.2019 13:05, Adrian Schmutzler wrote:
>> > This provides a uci system config setting that will be set only during
>> > initial setup. This can be used by uci-defaults script to determine
>> > whether they are run during initial setup or after a sysupgrade.
>> >
>> > Since the setting is removed again after uci-defaults have been
>> > processed, it won't be recognized by the user on the running device,
>> > but can be exploited also for downstream setup tasks.
>> 
>> This looks for me like a misuse of uci configuration and some kind of
>> workaround for a missing feature, maybe in procd/ubus?
> 
> Maybe I'm not familiar enough with it, but I currently can't think of a method indicating a fresh installation with procd/ubus.

I just don't think uci is the right way for this job. It's used for 
configuration, not for storing and clearing some flags about system 
state. Your way of using uci for that could introduce false positive 
results.

I would look at preinit stage, maybe checking if the writable filesystem 
(rootfs_data) is created/ready could be used for detecting the 'initial 
setup' state (I'm just not sure if that could work across all targets).

Either way, maybe an env variable, like we have for initramfs images 
(INITRAMFS set to 1), would be better?
Petr Štetiar Nov. 17, 2019, 9:20 p.m. UTC | #4
Adrian Schmutzler <freifunk@adrianschmutzler.de> [2019-11-08 13:05:19]:

> This provides a uci system config setting that will be set only
> during initial setup. This can be used by uci-defaults script to
> determine whether they are run during initial setup or after a
> sysupgrade.

I use similar stuff in my config scripts, just differently.

I do `set system.@system[-1].firstboot_done='1'` in order to know, that
the system was already first time booted.

So if the user performs sysupgrade but prefers to keep settings, I don't run
uci-default scripts which performs one time setup stuff, like setting
hostname, generating unique auth token etc. basically it boils down to
`is_firstboot || exit 0` in those one time uci-default scripts.

-- ynezz
diff mbox series

Patch

diff --git a/package/base-files/files/bin/config_generate b/package/base-files/files/bin/config_generate
index b473eba9e9..273561229a 100755
--- a/package/base-files/files/bin/config_generate
+++ b/package/base-files/files/bin/config_generate
@@ -243,6 +243,7 @@  generate_static_system() {
 		set system.@system[-1].ttylogin='0'
 		set system.@system[-1].log_size='64'
 		set system.@system[-1].urandom_seed='0'
+		set system.@system[-1].initial_setup='1'
 
 		delete system.ntp
 		set system.ntp='timeserver'
diff --git a/package/base-files/files/etc/uci-defaults/90_end-initial-setup b/package/base-files/files/etc/uci-defaults/90_end-initial-setup
new file mode 100644
index 0000000000..779d858d5f
--- /dev/null
+++ b/package/base-files/files/etc/uci-defaults/90_end-initial-setup
@@ -0,0 +1,4 @@ 
+uci -q delete system.@system[0].initial_setup
+uci commit system
+
+exit 0