netdev - Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20c73bae-0c79-4a8a-af60-6dbc6a88e953@gmail.com>
Date: Wed, 25 Sep 2024 01:10:46 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, andrew@...n.ch, sd@...asysnail.net,
 donald.hunter@...il.com
Subject: Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support

Hi Antonio,

On 23.09.2024 15:59, Antonio Quartulli wrote:
> On 23/09/2024 01:20, Sergey Ryazanov wrote:
>> On 17.09.2024 04:07, Antonio Quartulli wrote:
>>> +    -
>>> +      name: set-peer
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Add or modify a remote peer
>>
>> As Donald already mentioned, the typical approach to manage objects 
>> via Netlink is to provide an interface with four commands: New, Set, 
>> Get, Del. Here, peer created implicitely using the "set" comand. Out 
>> of curiosity, what the reason to create peers in the such way?
> 
> To be honest, I just wanted to keep the API as concise as possible and 
> having ADD and SET looked like duplicating methods, from a conceptual 
> perspective.

Could you elaborate, what is wrong with separated NEW and SET method 
conceptually?

 From the implementation point of view I can see that both methods can 
setup a same set of object properties. What can be resolved using a 
shared (between NEW and SET) peer configuration method.

> What userspace wants is "ensure we have a peer with ID X and these 
> attributes". If this ID was already known is not extremely important.
> 
> I can understand in other contexts knowing if an object already exists 
> can be crucial.

Looks like you want a "self synchronizing" API that automatically 
recovers synchronization between userspace and kernel.

On one hand this approach can mask potential bug. E.g. management 
application assumes that a peer was not configured and trying to 
configure it and kernel quietly reconfigure earlier known peer. Shall we 
in that case loudly inform everyone that something already went wrong?

On another hand, I see that current implementation does not do this. The 
SET method handler works differently depending on prior peer existence. 
The SET method will not allow an existing peer reconfiguration since it 
will trigger error due to inability to update "VPN" IPv4/IPv6 address. 
So looks like we have two different methods merged into the single 
function with complex behaviour.

BTW, if you want an option to recreate a peer, did you consider the 
NLM_F_REPLACE flag support in the NEW method?

>> Is the reason to create keys also implicitly same?
> 
> basically yes: userspace tells kernelspace "this is what I have 
> configured in my slots - make sure to have the same"
> (this statement also goes back to the other reply I have sent regarding 
> changing the KEY APIs)

If we save the current conception of slots, then yes it make sense.

>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +    -
>>> +      name: get-peer
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Retrieve data about existing remote peers (or a specific 
>>> one)
>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +        reply:
>>> +          attributes:
>>> +            - peer
>>> +      dump:
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +        reply:
>>> +          attributes:
>>> +            - peer
>>> +    -
>>> +      name: del-peer
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Delete existing remote peer
>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +    -
>>> +      name: set-key
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Add or modify a cipher key for a specific peer
>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +    -
>>> +      name: swap-keys
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Swap primary and secondary session keys for a specific peer
>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +    -
>>> +      name: del-key
>>> +      attribute-set: ovpn
>>> +      flags: [ admin-perm ]
>>> +      doc: Delete cipher key for a specific peer
>>> +      do:
>>> +        pre: ovpn-nl-pre-doit
>>> +        post: ovpn-nl-post-doit
>>> +        request:
>>> +          attributes:
>>> +            - ifindex
>>> +            - peer
>>> +

--
Sergey