On 25/09/2024 00:10, Sergey Ryazanov wrote:

Hi Antonio,

On 23.09.2024 15:59, Antonio Quartulli wrote:

quoted

On 23/09/2024 01:20, Sergey Ryazanov wrote:

quoted

On 17.09.2024 04:07, Antonio Quartulli wrote:

quoted

+    -
+      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?

I don't think it is wrong, I think it's just a matter of preference.
In the ovpn context SET and NEW would kinda do the same thing, except 
that NEW should be called for a non existing peer-id, while SET needs it 
to exist.

Given the above, I just preferred to have one op only and avoid possible 
confusion.

 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.

quoted

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.

Consider that userspace and kernelspace must always be in sync, 
therefore keeping it easy allows to avoid desynchronization.

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?

I get your point, but I am not sure this can truly happen, since 
userspace will always issue a DEL_PEER if it believes the peer is 
gone/should go.

Assuming this can really be the case, the solution would either be to 
self-destroy everything or to try deleting and re-adding the peer.
That's what the SET would already achieve.
After all, the goal of SET is truly to tell ovpn to mirror what usespace 
knows about a certain peer-id.

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.

This is a leftover that I am going to change in v8.
I will use hlist_nulls to happily rehash peers upon IP change.
This is a must do because we'll want to support client IP change at runtime.

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

you mean supporting NLM_F_REPLACE in SET_PEER and returning -EEXIST if 
peer-id is known and the flag was not passed?

I am not sure it'd be truly helpful.
I am pretty sure userspace will just end up passing that flag all the 
time :-D "just to be safe, since it doesn't break anything"

Cheers,

quoted

quoted

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.

quoted

quoted

quoted

+      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

-- 
Antonio Quartulli
OpenVPN Inc.