Re: [PATCH 09/45] vpn: Add VPN agent use callback for plugins

Hi Denis,

On 8/8/25 18:57, Denis Kenzior wrote:
> Hi Jussi,
> 
> On 8/8/25 7:28 AM, Jussi Laakkonen wrote:
>> Hi Denis,
>>
>> On 8/1/25 22:39, Denis Kenzior wrote:
>>> Hi Jussi,
>>>
>>> On 7/11/25 9:27 AM, Jussi Laakkonen wrote:
>>>> Add callback that can be used by the VPN plugins to tell the vpn_driver
>>>> whether it uses VPN agent or not. Default to using VPN agent if the
>>>> function is not defined.
>>>>
>>>> This is done to accommodate the state transition in vpn-provider when
>>>> the VPN does not utilize VPN agent.
>>>
>>> What about the case where previous credentials are cached?
>>
>> This does not have anything to do with caching of credentials? It just 
>> makes it possible to tell if the plugin uses or does not use agent. If 
>> there are issues with caching it should be addressed elsewhere and in 
>> separate patch I think. Sorry for not really understanding what you 
>> mean now.
> 
> So maybe I didn't fully follow the logic, so apologies if that's the 
> case.  By my reading we have two possibilities:
> 
> 1. driver has to ask for credentials.  So when .connect is invoked, core 
> automatically puts the state into 'Association' and lets the driver 
> report back when the credentials are obtained.
> 2. driver will never ask for credentials.  Core automatically puts the 
> state into 'Association' and then 'Connecting' or whatever.
> 
> Are there situations where the driver might cache the credentials and 
> not ask for them again on subsequent connects?  The core could then 
> treat this as case 2 above.

The configuration for the VPNs are read by vpn-provider.c and that does 
not update them when the content changes on the file that was read. That 
is why using VPN agent is a better option as that can be made to handle 
the caching cases well, and the errors are reported back to the agent so 
it can clear them when they're invalid. This is how it works on Sailfish 
OS, so an external storage is used for them by the agent to make sure 
caching is not an issue. Thus, the implementation here should not take a 
stance on it.

Since WireGuard does get the credentials through vpn-provider.c I think 
this goes beyond this patch set and would require further work to 
address any caching issues. It is already blowing out of proportions now 
I think.

> 
> <snip>
> 
>>>
>>> The only user of this would seem to be wireguard which blankly 
>>> returns false. Can this be made into a boolean of a set of flags 
>>> instead?
>>
>> I simply followed the approach that is used by the VPNs in general. 
>> Seemed simplest and most readable way to do it.
> 
> Hmm, but we already have:
> 
> #define VPN_FLAG_NO_TUN     1
> #define VPN_FLAG_NO_DAEMON  2
> 
> Wouldn't this fit the existing pattern?
> 

Um, no. A daemonless VPN can have a VPN agent in use. I was planning to 
implement this for WireGuard too. But for now this minor change is a 
clear distinction between the cases of agent and no-agent from my 
perspective.

Br,
  Jussi

P.S. I'll send a v2 of this whole set as some changes required changing 
the commits here and there and I lost track of them, easier to put a new 
complete one to avoid problems.