[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <m2wmjabehc.fsf@gmail.com>
Date: Tue, 17 Sep 2024 14:23:59 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch,
sd@...asysnail.net
Subject: Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support
Antonio Quartulli <antonio@...nvpn.net> 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.
> 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@...nvpn.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
Powered by blists - more mailing lists