| Message ID | 20240917010734.1905-5-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
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
Hi Donald, On 17/09/2024 15:23, Donald Hunter wrote: > 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 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@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. > >> + - >> + 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 <antonio@openvpn.net> writes: >>> + - >>> + 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? I was thinking that the two attributes would be mutually exclusive. You could accept local-ipv4 OR local-ipv6. If both are provided then you can report an extack error. >> >>> + - >>> + 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? I liked the separate ops for key management because the sematics are explicit and it is very obvious that there is no op for reading keys. If you also keep keyconf attrs separate from the peer attrs then it would be obvious that the peer ops would never expose any keyconf attrs. >> >>> + - >>> + 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? nl80211 is maybe not a good example to follow because it predates the ynl specs and code generation. The netdev.yaml spec is a good example of a modern genetlink spec. It specifies ops for 'dev-add-ntf' and 'dev-del-ntf' that both reuse the definition from 'dev-get' with the 'notify: dev-get' attribute: - name: dev-get doc: Get / dump information about a netdev. attribute-set: dev do: request: attributes: - ifindex reply: &dev-all attributes: - ifindex - xdp-features - xdp-zc-max-segs - xdp-rx-metadata-features - xsk-features dump: reply: *dev-all - name: dev-add-ntf doc: Notification about device appearing. notify: dev-get mcgrp: mgmt - name: dev-del-ntf doc: Notification about device disappearing. notify: dev-get mcgrp: mgmt The notify ops get distinct ids so they should never be confused with normal command responses. >> >>> + - >>> + 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? Right, so this is a notification to user space that it is time to swap keys, not that a swap-keys operation has happened? If the payload is unique to this notification then you should probably use the 'event' op format. For example: - name: swap-keys-ntf doc: Notify user space that a swap-keys op is due. attribute-set: ovpn event: attributes: - ifindex - peer mcgrp: peers >> >>> + - >>> + 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,
On 18/09/2024 12:07, Donald Hunter wrote: > Antonio Quartulli <antonio@openvpn.net> writes: >>>> + - >>>> + 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? > > I was thinking that the two attributes would be mutually exclusive. You > could accept local-ipv4 OR local-ipv6. If both are provided then you can > report an extack error. Ok then, I'll split the local-ip in two attrs. It also gets cleaner as we have an explicit type definition, while right now we infer the type from the data length. > >>> >>>> + - >>>> + 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? > > I liked the separate ops for key management because the sematics are > explicit and it is very obvious that there is no op for reading keys. If > you also keep keyconf attrs separate from the peer attrs then it would be > obvious that the peer ops would never expose any keyconf attrs. Ok, will move KEYCONF to the root level and will duplicate the PEER_ID. > >>> >>>> + - >>>> + 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? > > nl80211 is maybe not a good example to follow because it predates the > ynl specs and code generation. The netdev.yaml spec is a good example of > a modern genetlink spec. It specifies ops for 'dev-add-ntf' and > 'dev-del-ntf' that both reuse the definition from 'dev-get' with the > 'notify: dev-get' attribute: > > - > name: dev-get > doc: Get / dump information about a netdev. > attribute-set: dev > do: > request: > attributes: > - ifindex > reply: &dev-all > attributes: > - ifindex > - xdp-features > - xdp-zc-max-segs > - xdp-rx-metadata-features > - xsk-features > dump: > reply: *dev-all > - > name: dev-add-ntf > doc: Notification about device appearing. > notify: dev-get > mcgrp: mgmt > - > name: dev-del-ntf > doc: Notification about device disappearing. > notify: dev-get > mcgrp: mgmt > > The notify ops get distinct ids so they should never be confused with > normal command responses. Interesting. I will do the same then. > >>> >>>> + - >>>> + 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? > > Right, so this is a notification to user space that it is time to swap > keys, not that a swap-keys operation has happened? Correct. It is delivered when the current key cannot be used anymore and we need userspace to inject a new one. > If the payload is > unique to this notification then you should probably use the 'event' op > format. For example: > > - > name: swap-keys-ntf > doc: Notify user space that a swap-keys op is due. > attribute-set: ovpn > event: > attributes: > - ifindex > - peer > mcgrp: peers make sense. Will create the new op. Since we're moving the KEYCONF to the root level, we can just send that instead of the PEER. > >>> >>>> + - >>>> + 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!
Hello Antonio, Donald, On 18.09.2024 14:16, Antonio Quartulli wrote: > On 18/09/2024 12:07, Donald Hunter wrote: >> Antonio Quartulli <antonio@openvpn.net> writes: >>>>> + - >>>>> + 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? >> >> I was thinking that the two attributes would be mutually exclusive. You >> could accept local-ipv4 OR local-ipv6. If both are provided then you can >> report an extack error. > > Ok then, I'll split the local-ip in two attrs. > > It also gets cleaner as we have an explicit type definition, while right > now we infer the type from the data length. > >> >>>> >>>>> + - >>>>> + 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? >> >> I liked the separate ops for key management because the sematics are >> explicit and it is very obvious that there is no op for reading keys. If >> you also keep keyconf attrs separate from the peer attrs then it would be >> obvious that the peer ops would never expose any keyconf attrs. > > Ok, will move KEYCONF to the root level and will duplicate the PEER_ID. Nice idea! A was about to suggest the same. Besides making semantic simple, what somehow subjective, it should make parsing way simple. Two nested attributes parsing calls will be saved. -- Sergey
On 17.09.2024 04:07, Antonio Quartulli wrote: > 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 > with its auto-generated files: > - include/uapi/linux/ovpn.h > - drivers/net/ovpn/netlink-gen.c > - drivers/net/ovpn/netlink-gen.h > > Cc: donald.hunter@gmail.com > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > Documentation/netlink/specs/ovpn.yaml | 328 ++++++++++++++++++++++++++ > MAINTAINERS | 1 + > drivers/net/ovpn/Makefile | 2 + > drivers/net/ovpn/main.c | 14 ++ > drivers/net/ovpn/netlink-gen.c | 206 ++++++++++++++++ > drivers/net/ovpn/netlink-gen.h | 41 ++++ > drivers/net/ovpn/netlink.c | 153 ++++++++++++ > drivers/net/ovpn/netlink.h | 15 ++ > drivers/net/ovpn/ovpnstruct.h | 21 ++ > include/uapi/linux/ovpn.h | 108 +++++++++ > 10 files changed, 889 insertions(+) > create mode 100644 Documentation/netlink/specs/ovpn.yaml > create mode 100644 drivers/net/ovpn/netlink-gen.c > create mode 100644 drivers/net/ovpn/netlink-gen.h > create mode 100644 drivers/net/ovpn/netlink.c > create mode 100644 drivers/net/ovpn/netlink.h > create mode 100644 drivers/net/ovpn/ovpnstruct.h > create mode 100644 include/uapi/linux/ovpn.h > > 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 ] > + - > + 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 > + - > + 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 > + display-hint: ipv4 > + - > + name: vpn-ipv6 > + type: binary > + doc: The IPv6 assigned to the peer by the server > + 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 > + - > + 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 > + - > + 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 > + 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 As Donald already mentioned, the typical approach to manage objects via Netlink is to provide an interface with four commands: New, Set, Get, Del. Here, peer created implicitely using the "set" comand. Out of curiosity, what the reason to create peers in the such way? Is the reason to create keys also implicitly same? > + 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 > + - > + 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 > + - > + 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 > + -- Sergey
On 23/09/2024 01:20, Sergey Ryazanov wrote: > On 17.09.2024 04:07, Antonio Quartulli wrote: >> + - >> + name: set-peer >> + attribute-set: ovpn >> + flags: [ admin-perm ] >> + doc: Add or modify a remote peer > > As Donald already mentioned, the typical approach to manage objects via > Netlink is to provide an interface with four commands: New, Set, Get, > Del. Here, peer created implicitely using the "set" comand. Out of > curiosity, what the reason to create peers in the such way? To be honest, I just wanted to keep the API as concise as possible and having ADD and SET looked like duplicating methods, from a conceptual perspective. What userspace wants is "ensure we have a peer with ID X and these attributes". If this ID was already known is not extremely important. I can understand in other contexts knowing if an object already exists can be crucial. > > Is the reason to create keys also implicitly same? basically yes: userspace tells kernelspace "this is what I have configured in my slots - make sure to have the same" (this statement also goes back to the other reply I have sent regarding changing the KEY APIs) Cheers, > >> + 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 >> + - >> + 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 >> + - >> + 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 >> + > > -- > Sergey >
Hi Antonio, On 23.09.2024 15:59, Antonio Quartulli wrote: > On 23/09/2024 01:20, Sergey Ryazanov wrote: >> On 17.09.2024 04:07, Antonio Quartulli wrote: >>> + - >>> + name: set-peer >>> + attribute-set: ovpn >>> + flags: [ admin-perm ] >>> + doc: Add or modify a remote peer >> >> As Donald already mentioned, the typical approach to manage objects >> via Netlink is to provide an interface with four commands: New, Set, >> Get, Del. Here, peer created implicitely using the "set" comand. Out >> of curiosity, what the reason to create peers in the such way? > > To be honest, I just wanted to keep the API as concise as possible and > having ADD and SET looked like duplicating methods, from a conceptual > perspective. Could you elaborate, what is wrong with separated NEW and SET method conceptually? From the implementation point of view I can see that both methods can setup a same set of object properties. What can be resolved using a shared (between NEW and SET) peer configuration method. > What userspace wants is "ensure we have a peer with ID X and these > attributes". If this ID was already known is not extremely important. > > I can understand in other contexts knowing if an object already exists > can be crucial. Looks like you want a "self synchronizing" API that automatically recovers synchronization between userspace and kernel. On one hand this approach can mask potential bug. E.g. management application assumes that a peer was not configured and trying to configure it and kernel quietly reconfigure earlier known peer. Shall we in that case loudly inform everyone that something already went wrong? On another hand, I see that current implementation does not do this. The SET method handler works differently depending on prior peer existence. The SET method will not allow an existing peer reconfiguration since it will trigger error due to inability to update "VPN" IPv4/IPv6 address. So looks like we have two different methods merged into the single function with complex behaviour. BTW, if you want an option to recreate a peer, did you consider the NLM_F_REPLACE flag support in the NEW method? >> Is the reason to create keys also implicitly same? > > basically yes: userspace tells kernelspace "this is what I have > configured in my slots - make sure to have the same" > (this statement also goes back to the other reply I have sent regarding > changing the KEY APIs) If we save the current conception of slots, then yes it make sense. >>> + 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 >>> + - >>> + 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 >>> + - >>> + 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 >>> + -- Sergey
On 25/09/2024 00:10, Sergey Ryazanov wrote: > Hi Antonio, > > On 23.09.2024 15:59, Antonio Quartulli wrote: >> On 23/09/2024 01:20, Sergey Ryazanov wrote: >>> On 17.09.2024 04:07, Antonio Quartulli wrote: >>>> + - >>>> + name: set-peer >>>> + attribute-set: ovpn >>>> + flags: [ admin-perm ] >>>> + doc: Add or modify a remote peer >>> >>> As Donald already mentioned, the typical approach to manage objects >>> via Netlink is to provide an interface with four commands: New, Set, >>> Get, Del. Here, peer created implicitely using the "set" comand. Out >>> of curiosity, what the reason to create peers in the such way? >> >> To be honest, I just wanted to keep the API as concise as possible and >> having ADD and SET looked like duplicating methods, from a conceptual >> perspective. > > Could you elaborate, what is wrong with separated NEW and SET method > conceptually? I don't think it is wrong, I think it's just a matter of preference. In the ovpn context SET and NEW would kinda do the same thing, except that NEW should be called for a non existing peer-id, while SET needs it to exist. Given the above, I just preferred to have one op only and avoid possible confusion. > > From the implementation point of view I can see that both methods can > setup a same set of object properties. What can be resolved using a > shared (between NEW and SET) peer configuration method. > >> What userspace wants is "ensure we have a peer with ID X and these >> attributes". If this ID was already known is not extremely important. >> >> I can understand in other contexts knowing if an object already exists >> can be crucial. > > Looks like you want a "self synchronizing" API that automatically > recovers synchronization between userspace and kernel. Consider that userspace and kernelspace must always be in sync, therefore keeping it easy allows to avoid desynchronization. > > On one hand this approach can mask potential bug. E.g. management > application assumes that a peer was not configured and trying to > configure it and kernel quietly reconfigure earlier known peer. Shall we > in that case loudly inform everyone that something already went wrong? I get your point, but I am not sure this can truly happen, since userspace will always issue a DEL_PEER if it believes the peer is gone/should go. Assuming this can really be the case, the solution would either be to self-destroy everything or to try deleting and re-adding the peer. That's what the SET would already achieve. After all, the goal of SET is truly to tell ovpn to mirror what usespace knows about a certain peer-id. > > On another hand, I see that current implementation does not do this. The > SET method handler works differently depending on prior peer existence. > The SET method will not allow an existing peer reconfiguration since it > will trigger error due to inability to update "VPN" IPv4/IPv6 address. > So looks like we have two different methods merged into the single > function with complex behaviour. This is a leftover that I am going to change in v8. I will use hlist_nulls to happily rehash peers upon IP change. This is a must do because we'll want to support client IP change at runtime. > > BTW, if you want an option to recreate a peer, did you consider the > NLM_F_REPLACE flag support in the NEW method? you mean supporting NLM_F_REPLACE in SET_PEER and returning -EEXIST if peer-id is known and the flag was not passed? I am not sure it'd be truly helpful. I am pretty sure userspace will just end up passing that flag all the time :-D "just to be safe, since it doesn't break anything" Cheers, > >>> Is the reason to create keys also implicitly same? >> >> basically yes: userspace tells kernelspace "this is what I have >> configured in my slots - make sure to have the same" >> (this statement also goes back to the other reply I have sent >> regarding changing the KEY APIs) > > If we save the current conception of slots, then yes it make sense. > >>>> + 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 >>>> + - >>>> + 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 >>>> + - >>>> + 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 >>>> + > > -- > Sergey
Donald, On 18/09/2024 12:07, Donald Hunter wrote: [...] > nl80211 is maybe not a good example to follow because it predates the > ynl specs and code generation. The netdev.yaml spec is a good example of > a modern genetlink spec. It specifies ops for 'dev-add-ntf' and > 'dev-del-ntf' that both reuse the definition from 'dev-get' with the > 'notify: dev-get' attribute: > > - > name: dev-get > doc: Get / dump information about a netdev. > attribute-set: dev > do: > request: > attributes: > - ifindex > reply: &dev-all > attributes: > - ifindex > - xdp-features > - xdp-zc-max-segs > - xdp-rx-metadata-features > - xsk-features > dump: > reply: *dev-all > - > name: dev-add-ntf > doc: Notification about device appearing. > notify: dev-get > mcgrp: mgmt > - > name: dev-del-ntf > doc: Notification about device disappearing. > notify: dev-get > mcgrp: mgmt > > The notify ops get distinct ids so they should never be confused with > normal command responses. I see most (if not all) modules have named ops dev-del/add/get, while in ovpn I am going with new/del-dev (action and object are inverted). Do you think it'd make sense to change all the op names to follow the convention used by the other modules? Cheers,
On Wed, 25 Sept 2024 at 12:36, Antonio Quartulli <antonio@openvpn.net> wrote: > > Donald, > > I see most (if not all) modules have named ops dev-del/add/get, while in > ovpn I am going with new/del-dev (action and object are inverted). > > Do you think it'd make sense to change all the op names to follow the > convention used by the other modules? It's a good question. I'm not sure there's much consistency for either format: Total ops: 231 Starts with (new|get|del): 51 Ends with (new|get|del): 63 Exactly (new|get|del): 11 For the legacy and raw specs that I have written, I followed whatever convention was used for the enums in the UAPI, e.g. getroute from RTM_GETROUTE. The newer genetlink specs like netdev.yaml mostly favour the dev-get form so maybe that's the convention we should try to establish going forward? Cheers, Donald.
On 26/09/2024 17:06, Donald Hunter wrote: > On Wed, 25 Sept 2024 at 12:36, Antonio Quartulli <antonio@openvpn.net> wrote: >> >> Donald, >> >> I see most (if not all) modules have named ops dev-del/add/get, while in >> ovpn I am going with new/del-dev (action and object are inverted). >> >> Do you think it'd make sense to change all the op names to follow the >> convention used by the other modules? > > It's a good question. I'm not sure there's much consistency for either format: > > Total ops: 231 > Starts with (new|get|del): 51 > Ends with (new|get|del): 63 > Exactly (new|get|del): 11 > > For the legacy and raw specs that I have written, I followed whatever > convention was used for the enums in the UAPI, e.g. getroute from > RTM_GETROUTE. The newer genetlink specs like netdev.yaml mostly favour > the dev-get form so maybe that's the convention we should try to > establish going forward? If netdev went with that format, I presume that's where the preference is, therefore I'll follow that one. I'll modify the op names then. Thanks a lot. Cheers, > > Cheers, > 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 ] + - + 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 + - + 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 + display-hint: ipv4 + - + name: vpn-ipv6 + type: binary + doc: The IPv6 assigned to the peer by the server + 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 + - + 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 + - + 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 + 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 + - + 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 + - + 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 diff --git a/MAINTAINERS b/MAINTAINERS index 53b6350d95be..e6ac09ef5f6e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17269,6 +17269,7 @@ L: openvpn-devel@lists.sourceforge.net (moderated for non-subscribers) L: netdev@vger.kernel.org S: Maintained F: drivers/net/ovpn/ +F: include/uapi/linux/ovpn.h P54 WIRELESS DRIVER M: Christian Lamparter <chunkeey@googlemail.com> diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile index 53fb197027d7..201dc001419f 100644 --- a/drivers/net/ovpn/Makefile +++ b/drivers/net/ovpn/Makefile @@ -9,3 +9,5 @@ obj-$(CONFIG_OVPN) := ovpn.o ovpn-y += main.o ovpn-y += io.o +ovpn-y += netlink.o +ovpn-y += netlink-gen.o diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c index 8a90319e4600..7c35697cb596 100644 --- a/drivers/net/ovpn/main.c +++ b/drivers/net/ovpn/main.c @@ -7,12 +7,16 @@ * James Yonan <james@openvpn.net> */ +#include <linux/genetlink.h> #include <linux/module.h> #include <linux/netdevice.h> #include <linux/version.h> #include <net/rtnetlink.h> +#include <uapi/linux/ovpn.h> +#include "ovpnstruct.h" #include "main.h" +#include "netlink.h" #include "io.h" /* Driver info */ @@ -34,6 +38,7 @@ bool ovpn_dev_is_valid(const struct net_device *dev) * therefore ifaces should be destroyed when exiting a netns */ static struct rtnl_link_ops ovpn_link_ops = { + .kind = OVPN_FAMILY_NAME, }; static int ovpn_netdev_notifier_call(struct notifier_block *nb, @@ -86,8 +91,16 @@ static int __init ovpn_init(void) goto unreg_netdev; } + err = ovpn_nl_register(); + if (err) { + pr_err("ovpn: can't register netlink family: %d\n", err); + goto unreg_rtnl; + } + return 0; +unreg_rtnl: + rtnl_link_unregister(&ovpn_link_ops); unreg_netdev: unregister_netdevice_notifier(&ovpn_netdev_notifier); return err; @@ -95,6 +108,7 @@ static int __init ovpn_init(void) static __exit void ovpn_cleanup(void) { + ovpn_nl_unregister(); rtnl_link_unregister(&ovpn_link_ops); unregister_netdevice_notifier(&ovpn_netdev_notifier); diff --git a/drivers/net/ovpn/netlink-gen.c b/drivers/net/ovpn/netlink-gen.c new file mode 100644 index 000000000000..f7c4c448b263 --- /dev/null +++ b/drivers/net/ovpn/netlink-gen.c @@ -0,0 +1,206 @@ +// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) +/* Do not edit directly, auto-generated from: */ +/* Documentation/netlink/specs/ovpn.yaml */ +/* YNL-GEN kernel source */ + +#include <net/netlink.h> +#include <net/genetlink.h> + +#include "netlink-gen.h" + +#include <uapi/linux/ovpn.h> + +/* Integer value ranges */ +static const struct netlink_range_validation ovpn_a_peer_id_range = { + .max = 16777215ULL, +}; + +static const struct netlink_range_validation ovpn_a_peer_local_port_range = { + .min = 1ULL, + .max = 65535ULL, +}; + +/* Common nested types */ +const struct nla_policy ovpn_keyconf_nl_policy[OVPN_A_KEYCONF_DECRYPT_DIR + 1] = { + [OVPN_A_KEYCONF_SLOT] = NLA_POLICY_MAX(NLA_U32, 1), + [OVPN_A_KEYCONF_KEY_ID] = NLA_POLICY_MAX(NLA_U32, 7), + [OVPN_A_KEYCONF_CIPHER_ALG] = NLA_POLICY_MAX(NLA_U32, 2), + [OVPN_A_KEYCONF_ENCRYPT_DIR] = NLA_POLICY_NESTED(ovpn_keydir_nl_policy), + [OVPN_A_KEYCONF_DECRYPT_DIR] = NLA_POLICY_NESTED(ovpn_keydir_nl_policy), +}; + +const struct nla_policy ovpn_keydir_nl_policy[OVPN_A_KEYDIR_NONCE_TAIL + 1] = { + [OVPN_A_KEYDIR_CIPHER_KEY] = NLA_POLICY_MAX_LEN(256), + [OVPN_A_KEYDIR_NONCE_TAIL] = NLA_POLICY_EXACT_LEN(OVPN_NONCE_TAIL_SIZE), +}; + +const struct nla_policy ovpn_peer_nl_policy[OVPN_A_PEER_LINK_TX_PACKETS + 1] = { + [OVPN_A_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_peer_id_range), + [OVPN_A_PEER_SOCKADDR_REMOTE] = { .type = NLA_BINARY, }, + [OVPN_A_PEER_SOCKET] = { .type = NLA_U32, }, + [OVPN_A_PEER_VPN_IPV4] = { .type = NLA_U32, }, + [OVPN_A_PEER_VPN_IPV6] = NLA_POLICY_EXACT_LEN(16), + [OVPN_A_PEER_LOCAL_IP] = NLA_POLICY_MAX_LEN(16), + [OVPN_A_PEER_LOCAL_PORT] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_peer_local_port_range), + [OVPN_A_PEER_KEEPALIVE_INTERVAL] = { .type = NLA_U32, }, + [OVPN_A_PEER_KEEPALIVE_TIMEOUT] = { .type = NLA_U32, }, + [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4), + [OVPN_A_PEER_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_nl_policy), + [OVPN_A_PEER_VPN_RX_BYTES] = { .type = NLA_UINT, }, + [OVPN_A_PEER_VPN_TX_BYTES] = { .type = NLA_UINT, }, + [OVPN_A_PEER_VPN_RX_PACKETS] = { .type = NLA_UINT, }, + [OVPN_A_PEER_VPN_TX_PACKETS] = { .type = NLA_UINT, }, + [OVPN_A_PEER_LINK_RX_BYTES] = { .type = NLA_UINT, }, + [OVPN_A_PEER_LINK_TX_BYTES] = { .type = NLA_UINT, }, + [OVPN_A_PEER_LINK_RX_PACKETS] = { .type = NLA_U32, }, + [OVPN_A_PEER_LINK_TX_PACKETS] = { .type = NLA_U32, }, +}; + +/* OVPN_CMD_NEW_IFACE - do */ +static const struct nla_policy ovpn_new_iface_nl_policy[OVPN_A_MODE + 1] = { + [OVPN_A_IFNAME] = { .type = NLA_NUL_STRING, }, + [OVPN_A_MODE] = NLA_POLICY_MAX(NLA_U32, 1), +}; + +/* OVPN_CMD_DEL_IFACE - do */ +static const struct nla_policy ovpn_del_iface_nl_policy[OVPN_A_IFINDEX + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, +}; + +/* OVPN_CMD_SET_PEER - do */ +static const struct nla_policy ovpn_set_peer_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* OVPN_CMD_GET_PEER - do */ +static const struct nla_policy ovpn_get_peer_do_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* OVPN_CMD_GET_PEER - dump */ +static const struct nla_policy ovpn_get_peer_dump_nl_policy[OVPN_A_IFINDEX + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, +}; + +/* OVPN_CMD_DEL_PEER - do */ +static const struct nla_policy ovpn_del_peer_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* OVPN_CMD_SET_KEY - do */ +static const struct nla_policy ovpn_set_key_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* OVPN_CMD_SWAP_KEYS - do */ +static const struct nla_policy ovpn_swap_keys_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* OVPN_CMD_DEL_KEY - do */ +static const struct nla_policy ovpn_del_key_nl_policy[OVPN_A_PEER + 1] = { + [OVPN_A_IFINDEX] = { .type = NLA_U32, }, + [OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy), +}; + +/* Ops table for ovpn */ +static const struct genl_split_ops ovpn_nl_ops[] = { + { + .cmd = OVPN_CMD_NEW_IFACE, + .doit = ovpn_nl_new_iface_doit, + .policy = ovpn_new_iface_nl_policy, + .maxattr = OVPN_A_MODE, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_DEL_IFACE, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_del_iface_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_del_iface_nl_policy, + .maxattr = OVPN_A_IFINDEX, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_SET_PEER, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_set_peer_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_set_peer_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_GET_PEER, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_get_peer_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_get_peer_do_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_GET_PEER, + .dumpit = ovpn_nl_get_peer_dumpit, + .policy = ovpn_get_peer_dump_nl_policy, + .maxattr = OVPN_A_IFINDEX, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP, + }, + { + .cmd = OVPN_CMD_DEL_PEER, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_del_peer_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_del_peer_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_SET_KEY, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_set_key_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_set_key_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_SWAP_KEYS, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_swap_keys_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_swap_keys_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, + { + .cmd = OVPN_CMD_DEL_KEY, + .pre_doit = ovpn_nl_pre_doit, + .doit = ovpn_nl_del_key_doit, + .post_doit = ovpn_nl_post_doit, + .policy = ovpn_del_key_nl_policy, + .maxattr = OVPN_A_PEER, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, +}; + +static const struct genl_multicast_group ovpn_nl_mcgrps[] = { + [OVPN_NLGRP_PEERS] = { "peers", }, +}; + +struct genl_family ovpn_nl_family __ro_after_init = { + .name = OVPN_FAMILY_NAME, + .version = OVPN_FAMILY_VERSION, + .netnsok = true, + .parallel_ops = true, + .module = THIS_MODULE, + .split_ops = ovpn_nl_ops, + .n_split_ops = ARRAY_SIZE(ovpn_nl_ops), + .mcgrps = ovpn_nl_mcgrps, + .n_mcgrps = ARRAY_SIZE(ovpn_nl_mcgrps), +}; diff --git a/drivers/net/ovpn/netlink-gen.h b/drivers/net/ovpn/netlink-gen.h new file mode 100644 index 000000000000..ce11f74e1b56 --- /dev/null +++ b/drivers/net/ovpn/netlink-gen.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */ +/* Do not edit directly, auto-generated from: */ +/* Documentation/netlink/specs/ovpn.yaml */ +/* YNL-GEN kernel header */ + +#ifndef _LINUX_OVPN_GEN_H +#define _LINUX_OVPN_GEN_H + +#include <net/netlink.h> +#include <net/genetlink.h> + +#include <uapi/linux/ovpn.h> + +/* Common nested types */ +extern const struct nla_policy ovpn_keyconf_nl_policy[OVPN_A_KEYCONF_DECRYPT_DIR + 1]; +extern const struct nla_policy ovpn_keydir_nl_policy[OVPN_A_KEYDIR_NONCE_TAIL + 1]; +extern const struct nla_policy ovpn_peer_nl_policy[OVPN_A_PEER_LINK_TX_PACKETS + 1]; + +int ovpn_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, + struct genl_info *info); +void +ovpn_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, + struct genl_info *info); + +int ovpn_nl_new_iface_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_del_iface_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_get_peer_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_get_peer_dumpit(struct sk_buff *skb, struct netlink_callback *cb); +int ovpn_nl_del_peer_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_set_key_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_swap_keys_doit(struct sk_buff *skb, struct genl_info *info); +int ovpn_nl_del_key_doit(struct sk_buff *skb, struct genl_info *info); + +enum { + OVPN_NLGRP_PEERS, +}; + +extern struct genl_family ovpn_nl_family; + +#endif /* _LINUX_OVPN_GEN_H */ diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c new file mode 100644 index 000000000000..b5b53c06d64a --- /dev/null +++ b/drivers/net/ovpn/netlink.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: Antonio Quartulli <antonio@openvpn.net> + */ + +#include <linux/netdevice.h> +#include <net/genetlink.h> + +#include <uapi/linux/ovpn.h> + +#include "ovpnstruct.h" +#include "main.h" +#include "io.h" +#include "netlink.h" +#include "netlink-gen.h" + +MODULE_ALIAS_GENL_FAMILY(OVPN_FAMILY_NAME); + +/** + * ovpn_get_dev_from_attrs - retrieve the netdevice a netlink message is + * targeting + * @net: network namespace where to look for the interface + * @info: generic netlink info from the user request + * + * Return: the netdevice, if found, or an error otherwise + */ +static struct net_device * +ovpn_get_dev_from_attrs(struct net *net, const struct genl_info *info) +{ + struct net_device *dev; + int ifindex; + + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_IFINDEX)) + return ERR_PTR(-EINVAL); + + ifindex = nla_get_u32(info->attrs[OVPN_A_IFINDEX]); + + dev = dev_get_by_index(net, ifindex); + if (!dev) { + NL_SET_ERR_MSG_MOD(info->extack, + "ifindex does not match any interface"); + return ERR_PTR(-ENODEV); + } + + if (!ovpn_dev_is_valid(dev)) + goto err_put_dev; + + return dev; + +err_put_dev: + netdev_put(dev, NULL); + + NL_SET_ERR_MSG_MOD(info->extack, "specified interface is not ovpn"); + NL_SET_BAD_ATTR(info->extack, info->attrs[OVPN_A_IFINDEX]); + + return ERR_PTR(-EINVAL); +} + +int ovpn_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, + struct genl_info *info) +{ + struct net *net = genl_info_net(info); + struct net_device *dev = ovpn_get_dev_from_attrs(net, info); + + if (IS_ERR(dev)) + return PTR_ERR(dev); + + info->user_ptr[0] = netdev_priv(dev); + + return 0; +} + +void ovpn_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, + struct genl_info *info) +{ + struct ovpn_struct *ovpn = info->user_ptr[0]; + + if (ovpn) + netdev_put(ovpn->dev, NULL); +} + +int ovpn_nl_new_iface_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_del_iface_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_get_peer_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_get_peer_dumpit(struct sk_buff *skb, struct netlink_callback *cb) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_del_peer_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_set_key_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_swap_keys_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +int ovpn_nl_del_key_doit(struct sk_buff *skb, struct genl_info *info) +{ + return -EOPNOTSUPP; +} + +/** + * ovpn_nl_register - perform any needed registration in the NL subsustem + * + * Return: 0 on success, a negative error code otherwise + */ +int __init ovpn_nl_register(void) +{ + int ret = genl_register_family(&ovpn_nl_family); + + if (ret) { + pr_err("ovpn: genl_register_family failed: %d\n", ret); + return ret; + } + + return 0; +} + +/** + * ovpn_nl_unregister - undo any module wide netlink registration + */ +void ovpn_nl_unregister(void) +{ + genl_unregister_family(&ovpn_nl_family); +} diff --git a/drivers/net/ovpn/netlink.h b/drivers/net/ovpn/netlink.h new file mode 100644 index 000000000000..9e87cf11d1e9 --- /dev/null +++ b/drivers/net/ovpn/netlink.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_NETLINK_H_ +#define _NET_OVPN_NETLINK_H_ + +int ovpn_nl_register(void); +void ovpn_nl_unregister(void); + +#endif /* _NET_OVPN_NETLINK_H_ */ diff --git a/drivers/net/ovpn/ovpnstruct.h b/drivers/net/ovpn/ovpnstruct.h new file mode 100644 index 000000000000..ff248cad1401 --- /dev/null +++ b/drivers/net/ovpn/ovpnstruct.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2019-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#ifndef _NET_OVPN_OVPNSTRUCT_H_ +#define _NET_OVPN_OVPNSTRUCT_H_ + +/** + * struct ovpn_struct - per ovpn interface state + * @dev: the actual netdev representing the tunnel + */ +struct ovpn_struct { + struct net_device *dev; +}; + +#endif /* _NET_OVPN_OVPNSTRUCT_H_ */ diff --git a/include/uapi/linux/ovpn.h b/include/uapi/linux/ovpn.h new file mode 100644 index 000000000000..677a8105b61b --- /dev/null +++ b/include/uapi/linux/ovpn.h @@ -0,0 +1,108 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */ +/* Do not edit directly, auto-generated from: */ +/* Documentation/netlink/specs/ovpn.yaml */ +/* YNL-GEN uapi header */ + +#ifndef _UAPI_LINUX_OVPN_H +#define _UAPI_LINUX_OVPN_H + +#define OVPN_FAMILY_NAME "ovpn" +#define OVPN_FAMILY_VERSION 1 + +#define OVPN_NONCE_TAIL_SIZE 8 + +enum ovpn_cipher_alg { + OVPN_CIPHER_ALG_NONE, + OVPN_CIPHER_ALG_AES_GCM, + OVPN_CIPHER_ALG_CHACHA20_POLY1305, +}; + +enum ovpn_del_peer_reason { + OVPN_DEL_PEER_REASON_TEARDOWN, + OVPN_DEL_PEER_REASON_USERSPACE, + OVPN_DEL_PEER_REASON_EXPIRED, + OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, +}; + +enum ovpn_key_slot { + OVPN_KEY_SLOT_PRIMARY, + OVPN_KEY_SLOT_SECONDARY, +}; + +enum ovpn_mode { + OVPN_MODE_P2P, + OVPN_MODE_MP, +}; + +enum { + OVPN_A_PEER_ID = 1, + OVPN_A_PEER_SOCKADDR_REMOTE, + OVPN_A_PEER_SOCKET, + OVPN_A_PEER_VPN_IPV4, + OVPN_A_PEER_VPN_IPV6, + OVPN_A_PEER_LOCAL_IP, + OVPN_A_PEER_LOCAL_PORT, + OVPN_A_PEER_KEEPALIVE_INTERVAL, + OVPN_A_PEER_KEEPALIVE_TIMEOUT, + OVPN_A_PEER_DEL_REASON, + OVPN_A_PEER_KEYCONF, + OVPN_A_PEER_VPN_RX_BYTES, + OVPN_A_PEER_VPN_TX_BYTES, + OVPN_A_PEER_VPN_RX_PACKETS, + OVPN_A_PEER_VPN_TX_PACKETS, + OVPN_A_PEER_LINK_RX_BYTES, + OVPN_A_PEER_LINK_TX_BYTES, + OVPN_A_PEER_LINK_RX_PACKETS, + OVPN_A_PEER_LINK_TX_PACKETS, + + __OVPN_A_PEER_MAX, + OVPN_A_PEER_MAX = (__OVPN_A_PEER_MAX - 1) +}; + +enum { + OVPN_A_KEYCONF_SLOT = 1, + OVPN_A_KEYCONF_KEY_ID, + OVPN_A_KEYCONF_CIPHER_ALG, + OVPN_A_KEYCONF_ENCRYPT_DIR, + OVPN_A_KEYCONF_DECRYPT_DIR, + + __OVPN_A_KEYCONF_MAX, + OVPN_A_KEYCONF_MAX = (__OVPN_A_KEYCONF_MAX - 1) +}; + +enum { + OVPN_A_KEYDIR_CIPHER_KEY = 1, + OVPN_A_KEYDIR_NONCE_TAIL, + + __OVPN_A_KEYDIR_MAX, + OVPN_A_KEYDIR_MAX = (__OVPN_A_KEYDIR_MAX - 1) +}; + +enum { + OVPN_A_IFINDEX = 1, + OVPN_A_IFNAME, + OVPN_A_MODE, + OVPN_A_PEER, + + __OVPN_A_MAX, + OVPN_A_MAX = (__OVPN_A_MAX - 1) +}; + +enum { + OVPN_CMD_NEW_IFACE = 1, + OVPN_CMD_DEL_IFACE, + OVPN_CMD_SET_PEER, + OVPN_CMD_GET_PEER, + OVPN_CMD_DEL_PEER, + OVPN_CMD_SET_KEY, + OVPN_CMD_SWAP_KEYS, + OVPN_CMD_DEL_KEY, + + __OVPN_CMD_MAX, + OVPN_CMD_MAX = (__OVPN_CMD_MAX - 1) +}; + +#define OVPN_MCGRP_PEERS "peers" + +#endif /* _UAPI_LINUX_OVPN_H */
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 with its auto-generated files: - include/uapi/linux/ovpn.h - drivers/net/ovpn/netlink-gen.c - drivers/net/ovpn/netlink-gen.h Cc: donald.hunter@gmail.com Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- Documentation/netlink/specs/ovpn.yaml | 328 ++++++++++++++++++++++++++ MAINTAINERS | 1 + drivers/net/ovpn/Makefile | 2 + drivers/net/ovpn/main.c | 14 ++ drivers/net/ovpn/netlink-gen.c | 206 ++++++++++++++++ drivers/net/ovpn/netlink-gen.h | 41 ++++ drivers/net/ovpn/netlink.c | 153 ++++++++++++ drivers/net/ovpn/netlink.h | 15 ++ drivers/net/ovpn/ovpnstruct.h | 21 ++ include/uapi/linux/ovpn.h | 108 +++++++++ 10 files changed, 889 insertions(+) create mode 100644 Documentation/netlink/specs/ovpn.yaml create mode 100644 drivers/net/ovpn/netlink-gen.c create mode 100644 drivers/net/ovpn/netlink-gen.h create mode 100644 drivers/net/ovpn/netlink.c create mode 100644 drivers/net/ovpn/netlink.h create mode 100644 drivers/net/ovpn/ovpnstruct.h create mode 100644 include/uapi/linux/ovpn.h