Hi Donald,

On 17/09/2024 15:23, Donald Hunter wrote:

Antonio Quartulli [off-list ref] writes:

quoted

This commit introduces basic netlink support with family
registration/unregistration functionalities and stub pre/post-doit.

More importantly it introduces the YAML uAPI description along

Hi Antonio,

net-next is currently closed so my guess is that you'll have to resend
this when net-next reopens at the end of the month:

https://netdev.bots.linux.dev/net-next.html

I quickly discussed this point with Sabrina and the conclusion was that 
posting shouldn't hurt as this patchset is not truly "a new submission".
In any case, I'll see if more comments come through or not first.

I have read through the YAML spec and I have few comments (and nits)
below.

Thanks a lot. My replies are inline.

Thanks,
Donald.

quoted

diff --git a/Documentation/netlink/specs/ovpn.yaml b/Documentation/netlink/specs/ovpn.yaml
new file mode 100644
index 000000000000..456ac3747d27
--- /dev/null
+++ b/Documentation/netlink/specs/ovpn.yaml

@@ -0,0 +1,328 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+#
+# Author: Antonio Quartulli <antonio@openvpn.net>
+#
+# Copyright (c) 2024, OpenVPN Inc.
+#
+
+name: ovpn
+
+protocol: genetlink
+
+doc: Netlink protocol to control OpenVPN network devices
+
+definitions:
+  -
+    type: const
+    name: nonce-tail-size
+    value: 8
+  -
+    type: enum
+    name: cipher-alg
+    value-start: 0
+    entries: [ none, aes-gcm, chacha20_poly1305 ]

Nit: Is there any reason for the underscore in chacha20_poly1305 and the
mixed use of dash / underscore in various identifiers throughout the
spec? The YNL convention is to use dashes throughout.

No real reason, I must have not realized that I Was mixing the two.
I will convert them all to dashes.

quoted

+  -
+    type: enum
+    name: del-peer_reason
+    value-start: 0
+    entries: [ teardown, userspace, expired, transport-error, transport_disconnect ]
+  -
+    type: enum
+    name: key-slot
+    value-start: 0
+    entries: [ primary, secondary ]
+  -
+    type: enum
+    name: mode
+    value-start: 0
+    entries: [ p2p, mp ]
+
+attribute-sets:
+  -
+    name: peer
+    attributes:
+      -
+        name: id
+        type: u32
+        doc: |
+          The unique Id of the peer. To be used to identify peers during
+          operations
+        checks:
+          max: 0xFFFFFF
+      -
+        name: sockaddr-remote
+        type: binary
+        doc: |
+          The sockaddr_in/in6 object identifying the remote address/port of the
+          peer

The use of structs as attribute values is strongly discouraged. There
should be separate attributes for port and ipv[46]-address.

https://docs.kernel.org/userspace-api/netlink/intro.html#fixed-metadata-and-structures

Thanks a lot for the pointer! I didn't know that.
This means I have to change the netlink attributes as per your 
suggestion (address, port, scope_id) and recreate the sockaddr on the 
kernel side.

quoted

+      -
+        name: socket
+        type: u32
+        doc: The socket to be used to communicate with the peer
+      -
+        name: vpn-ipv4
+        type: u32
+        doc: The IPv4 assigned to the peer by the server

nit: IPv4 address

ACK

quoted

+        display-hint: ipv4
+      -
+        name: vpn-ipv6
+        type: binary
+        doc: The IPv6 assigned to the peer by the server

nit: IPv6 address

ACK

quoted

+        display-hint: ipv6
+        checks:
+          exact-len: 16
+      -
+        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?

quoted

+      -
+        name: local-port
+        type: u32
+        doc: The local port to be used to send packets to the peer (UDP only)
+        checks:
+          min: 1
+          max: u16-max
+      -
+        name: keepalive-interval
+        type: u32
+        doc: |
+          The number of seconds after which a keep alive message is sent to the
+          peer
+      -
+        name: keepalive-timeout
+        type: u32
+        doc: |
+          The number of seconds from the last activity after which the peer is
+          assumed dead
+      -
+        name: del-reason
+        type: u32
+        doc: The reason why a peer was deleted
+        enum: del-peer_reason
+      -
+        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?

quoted

+      -
+        name: vpn-rx_bytes
+        type: uint
+        doc: Number of bytes received over the tunnel
+      -
+        name: vpn-tx_bytes
+        type: uint
+        doc: Number of bytes transmitted over the tunnel
+      -
+        name: vpn-rx_packets
+        type: uint
+        doc: Number of packets received over the tunnel
+      -
+        name: vpn-tx_packets
+        type: uint
+        doc: Number of packets transmitted over the tunnel
+      -
+        name: link-rx_bytes
+        type: uint
+        doc: Number of bytes received at the transport level
+      -
+        name: link-tx_bytes
+        type: uint
+        doc: Number of bytes transmitted at the transport level
+      -
+        name: link-rx_packets
+        type: u32
+        doc: Number of packets received at the transport level
+      -
+        name: link-tx_packets
+        type: u32
+        doc: Number of packets transmitted at the transport level
+  -
+    name: keyconf
+    attributes:
+      -
+        name: slot
+        type: u32
+        doc: The slot where the key should be stored
+        enum: key-slot
+      -
+        name: key-id
+        doc: |
+          The unique ID for the key. Used to fetch the correct key upon
+          decryption
+        type: u32
+        checks:
+          max: 7
+      -
+        name: cipher-alg
+        type: u32
+        doc: The cipher to be used when communicating with the peer
+        enum: cipher-alg
+      -
+        name: encrypt-dir
+        type: nest
+        doc: Key material for encrypt direction
+        nested-attributes: keydir
+      -
+        name: decrypt-dir
+        type: nest
+        doc: Key material for decrypt direction
+        nested-attributes: keydir
+  -
+    name: keydir
+    attributes:
+      -
+        name: cipher-key
+        type: binary
+        doc: The actual key to be used by the cipher
+        checks:
+         max-len: 256
+      -
+        name: nonce-tail
+        type: binary
+        doc: |
+          Random nonce to be concatenated to the packet ID, in order to
+          obtain the actua cipher IV
+        checks:
+         exact-len: nonce-tail-size
+  -
+    name: ovpn
+    attributes:
+      -
+        name: ifindex
+        type: u32
+        doc: Index of the ovpn interface to operate on
+      -
+        name: ifname
+        type: string
+        doc: Name of the ovpn interface that is being created
+      -
+        name: mode
+        type: u32
+        enum: mode
+        doc: |
+          Oper mode instructing an interface to act as Point2Point or
+          MultiPoint
+      -
+        name: peer
+        type: nest
+        doc: |
+          The peer object containing the attributed of interest for the specific
+          operation
+        nested-attributes: peer
+
+operations:
+  list:
+    -
+      name: new-iface

This might be better called 'dev' or 'link' to be consistent with the
existing netlink UAPIs.

hm ok, then I think I will s/IFACE/DEV/

quoted

+      attribute-set: ovpn
+      flags: [ admin-perm ]
+      doc: Create a new interface
+      do:
+        request:
+          attributes:
+            - ifname
+            - mode
+        reply:
+          attributes:
+            - ifname
+            - ifindex
+    -
+      name: del-iface
+      attribute-set: ovpn
+      flags: [ admin-perm ]
+      doc: Delete existing interface
+      do:
+        pre: ovpn-nl-pre-doit
+        post: ovpn-nl-post-doit
+        request:
+          attributes:
+            - ifindex
+    -
+      name: set-peer
+      attribute-set: ovpn
+      flags: [ admin-perm ]
+      doc: Add or modify a remote peer
+      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

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?

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?

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 a lot for your comments, Donald!

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.