Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support

[Donald Hunter <donald.hunter@gmail.com>]

Antonio Quartulli <antonio@openvpn.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@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