Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 12/19/23 8:31 AM, Denis Kenzior wrote:
> Hi James,
>
>>>
>>> Ehh, I'm not so sure about this. handshake_state_set_8021x_config() 
>>> does a shallow copy of the l_settings object.  I think eap state 
>>> machine will make copies of the info it needs, but...
>> Ok, yes this wouldn't work as-is then, but we could clone it instead.
>>>
>>>>     to the profile if there is an ongoing connection. This is a much
>>>>     simpler approach (unless I'm missing something). Only questionable
>>>
>>> Also, netconfig_load_settings is called very early in 
>>> __station_connect_network, so even if you overwrite the settings 
>>> object here, it is probably too late to take any effect.
>>
>> For this case I'm not sure there is any difference. Keeping the 
>> existing settings or replacing won't change netconfig once 
>> __station_connect_network() is called. We would need some changes in 
>> station to load the settings again after the connection. I'm not sure 
>> trying to solve this specific case of settings 
>
> I guess I'm then confused on what you're trying to accomplish with 
> this patch. I thought you were trying to take care of the inherent 
> race between:
>     1. Connect
>     2. Write provisioning file

Things sort of evolved. I originally thought I could handle this race by 
just updating the settings but this is not really the case without 
additional changes to reapply the any netconfig changes after the 
connection. Yes this would also solve the DPP problem but I think its 
makes more sense to just sort out everything prior to issuing a 
connection rather than rely on the update to come in and quickly insert 
the missing config values. I'd hate to rely on this, its quite subtle.

>
> The result of that race is that any changes to the provisioning file 
> are not taken into consideration when connect proceeds.  In 
> particular, any networking related settings are ignored.  Note that 
> today we have a bunch of logic with several paths for syncing to disk 
> that take care that the settings are not actually lost (well, best 
> effort anyway) when the connection is successfully established:
>     - Special logic to update Autoconnect inside knownnetwork.c
>     - Special logic for updating just the PSK bits in 
> network_sync_settings()
>
> I assumed that by solving the above race, you'd be solving the problem 
> you're having with DPP?
It would yes, but not without re-applying the netconfig changes again.
>
>> changing _during_ a connection is really in the scope of this patch 
>> set. I'm 
>
> Okay, then this patch in particular is not needed?

Its at least partially needed, if a known network already exists prior 
to DPP and its overwritten there is no callback. So we may not need to 
actually touch network->settings at all as I thought. Just emit an 
UPDATED event when the profile changes. DPP can then call 
network_autoconnect and the settings _should_ be re-loaded unless I'm 
forgetting some instance where network->settings can persist, but I 
don't think it can.

>
>> also not really convinced trying to fix this race condition is all 
>> that important. Its not really a "race" per-se, but more of just how 
>> IWD works. The profile is loaded upon connecting and those settings 
>> are used. If you swap out/modify the profile in the middle of that I 
>> don't think anyone would/should expect those changes to be taken for 
>> that ongoing connection. That's my opinion on it anyways.
>
> That's fine as well.  In that case you just need to have DPP wait 
> until knownnetworks adds/updates its state.
>
>>
>> What we do need is the ability to update settings on disk if IWD is 
>> sitting idle or connected. Currently if network/network->settings 
>> already exists its not possible to update unless IWD disconnects.
>
> I'm not following.
>>
>> You mentioned copying over all settings except the security group, 
>> but this wouldn't handle the case of DPP (or anything) updating the 
>> profile with new credentials. I think we need the l_settings object 
>> to be 100% synced with whats on disk (but as you said, 
>> network->psk/network->passphrase shouldn't be altered). This then 
>> means the entire settings is copied, so its much more efficient to 
>> just replace it. Does this make sense?
>
> If the settings are opened, it is too late to alter anything related 
> to security since that is used by eapol, ft, netdev, etc. You might 
> have a chance to apply / re-apply the settings used by netconfig.

Yes, with additional changes. Not anything network/knownnetworks can do 
about it though is all I'm saying.


Thanks,

James

>
> Regards,
> -Denis