Message-ID: <99028055-f440-45e8-8fb1-ec4e19e0cafa@openvpn.net>
Date: Tue, 17 Sep 2024 23:28:41 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Donald Hunter <donald.hunter@...il.com>
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

Hi Donald,

On 17/09/2024 15:23, Donald Hunter wrote:
> 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 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.
> 
>> 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.

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


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

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

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

ACK

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

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

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

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

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

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

Powered by blists - more mailing lists