On 18/09/2024 12:07, Donald Hunter wrote:

Antonio Quartulli [off-list ref] writes:

quoted

quoted

quoted

+      -
+        name: local-ip
+        type: binary
+        doc: The local IP to be used to send packets to the peer (UDP only)
+        checks:
+          max-len: 16

It might be better to have separate attrs fopr local-ipv4 and
local-ipv6, to be consistent with vpn-ipv4 / vpn-ipv6

while it is possible for a peer to be dual stack and have both an IPv4 and IPv6 address assigned
to the VPN tunnel, the local transport endpoint can only be one (either v4 or v6).
This is why we have only one local_ip.
Does it make sense?

I was thinking that the two attributes would be mutually exclusive. You
could accept local-ipv4 OR local-ipv6. If both are provided then you can
report an extack error.

Ok then, I'll split the local-ip in two attrs.

It also gets cleaner as we have an explicit type definition, while right 
now we infer the type from the data length.

quoted

quoted

quoted

+      -
+        name: keyconf
+        type: nest
+        doc: Peer specific cipher configuration
+        nested-attributes: keyconf

Perhaps keyconf should just be used as a top-level attribute-set. The
only attr you'd need to duplicate would be peer-id? There are separate
ops for setting peers and for key configuration, right?

This is indeed a good point.
Yes, SET_PEER and SET_KEY are separate ops.

I could go with SET_PEER only, and let the user specify a keyconf within a peer (like now).

Or I could keep to SET_KEY, but then do as you suggest and move KEYCONF to the root level.

Is there any preferred approach?

I liked the separate ops for key management because the sematics are
explicit and it is very obvious that there is no op for reading keys. If
you also keep keyconf attrs separate from the peer attrs then it would be
obvious that the peer ops would never expose any keyconf attrs.

Ok, will move KEYCONF to the root level and will duplicate the PEER_ID.

quoted

quoted

quoted

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

I think you need to add an op for 'del-peer-notify' to specify the
notification, not reuse the 'del-peer' command.

my idea was to use CMD_DEL_PEER and then send back a very very short PEER object.
I took inspiration from nl80211 that sends CMD_NEW_STATION and CMD_DEL_STATION when a wifi host
connects or disconnect. In that case the full STATION object is also delivered (maybe I should
do the same?)

Or is there some other technical reason for not reusing CMD_DEL_PEER?

nl80211 is maybe not a good example to follow because it predates the
ynl specs and code generation. The netdev.yaml spec is a good example of
a modern genetlink spec. It specifies ops for 'dev-add-ntf' and
'dev-del-ntf' that both reuse the definition from 'dev-get' with the
'notify: dev-get' attribute:

     -
       name: dev-get
       doc: Get / dump information about a netdev.
       attribute-set: dev
       do:
         request:
           attributes:
             - ifindex
         reply: &dev-all
           attributes:
             - ifindex
             - xdp-features
             - xdp-zc-max-segs
             - xdp-rx-metadata-features
             - xsk-features
       dump:
         reply: *dev-all
     -
       name: dev-add-ntf
       doc: Notification about device appearing.
       notify: dev-get
       mcgrp: mgmt
     -
       name: dev-del-ntf
       doc: Notification about device disappearing.
       notify: dev-get
       mcgrp: mgmt

The notify ops get distinct ids so they should never be confused with
normal command responses.

Interesting. I will do the same then.

quoted

quoted

quoted

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

Same for swap-keys notifications.

Yeah, here I can understand. My rationale was: tell userspace that now we truly need a
SWAP_KEYS. Do you think this can create problems/confusion?

Right, so this is a notification to user space that it is time to swap
keys, not that a swap-keys operation has happened?

Correct. It is delivered when the current key cannot be used anymore and 
we need userspace to inject a new one.

If the payload is
unique to this notification then you should probably use the 'event' op
format. For example:

     -
       name: swap-keys-ntf
       doc: Notify user space that a swap-keys op is due.
       attribute-set: ovpn
       event:
         attributes:
           - ifindex
           - peer
       mcgrp: peers

make sense. Will create the new op.
Since we're moving the KEYCONF to the root level, we can just send that 
instead of the PEER.

quoted

quoted

quoted

+    -
+      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
+
+mcast-groups:
+  list:
+    -
+      name: peers

Thanks!

-- 
Antonio Quartulli
OpenVPN Inc.