Message-ID: <63c452f7-041e-4a28-96ba-c37ea8170dfd@openvpn.net>
Date: Wed, 18 Sep 2024 13:16:48 +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

On 18/09/2024 12:07, Donald Hunter wrote:
> 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.

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!

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists