Message-ID: <m2o74lb7hu.fsf@gmail.com>
Date: Wed, 18 Sep 2024 11:07:09 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org,  kuba@...nel.org,  pabeni@...hat.com,
  ryazanov.s.a@...il.com,  edumazet@...gle.com,  andrew@...n.ch,
  sd@...asysnail.net
Subject: Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support

Antonio Quartulli <antonio@...nvpn.net> writes:
>>> +      -
>>> +        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,

Powered by blists - more mailing lists