| 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
| ||
|
Message-ID: <cca86695-54ae-4784-a76b-c8c38031f79d@openvpn.net> Date: Wed, 25 Sep 2024 02:01:00 +0200 From: Antonio Quartulli <antonio@...nvpn.net> To: Sergey Ryazanov <ryazanov.s.a@...il.com> Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch, sd@...asysnail.net, donald.hunter@...il.com Subject: Re: [PATCH net-next v7 04/25] ovpn: add basic netlink support 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 -- Antonio Quartulli OpenVPN Inc.
Powered by blists - more mailing lists