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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <782b53e2-58ef-47a7-b392-c18d807d3e72@gmail.com>
Date: Mon, 23 Sep 2024 01:24:32 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>,
 Donald Hunter <donald.hunter@...il.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, andrew@...n.ch, sd@...asysnail.net
Subject: Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support

Hello Antonio, Donald,

On 18.09.2024 14:16, Antonio Quartulli wrote:
> 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.

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