Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support
From: Donald Hunter <donald.hunter@gmail.com>
Date: 2024-09-17 13:24:06
Antonio Quartulli [off-list ref] writes:
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 have read through the YAML spec and I have few comments (and nits) below. Thanks, Donald.
quoted hunk ↗ jump to hunk
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.
+ - + 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
+ - + 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
+ display-hint: ipv4 + - + name: vpn-ipv6 + type: binary + doc: The IPv6 assigned to the peer by the server
nit: IPv6 address
+ 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
+ - + 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?
+ - + 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.
+ 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.
+ - + 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.
+ - + 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