Text-based IPC for Userspace Implementations

Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

Hey guys,

Currently wg(8) talks to the kernel by passing structs through an
ioctl. Due to upstream demands, this is going to be changed to
netlink, and we'll ditch those structs. wg(8) previously tried to
re-use those same structs for userspace implementations, passing them
through a unix socket. Implementors did not like dealing with this.
Since the structs are going for the kernel stuff, we might as well
move to something better, too, for the userspace stuff. Therefore
wg(8) now has a very simple text-based IPC format over unix sockets
(or Windows named pipes) that can be easily implemented in nearly
every language, even bash.

I've written a small description of it here: https://www.wireguard.io/xplatform/

This will be part of the next snapshot.

Any questions?

Regards,
Jason

Re: Text-based IPC for Userspace Implementations

[Toke Høiland-Jørgensen]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hey guys,
>
> Currently wg(8) talks to the kernel by passing structs through an
> ioctl. Due to upstream demands, this is going to be changed to
> netlink, and we'll ditch those structs. wg(8) previously tried to
> re-use those same structs for userspace implementations, passing them
> through a unix socket. Implementors did not like dealing with this.
> Since the structs are going for the kernel stuff, we might as well
> move to something better, too, for the userspace stuff. Therefore
> wg(8) now has a very simple text-based IPC format over unix sockets
> (or Windows named pipes) that can be easily implemented in nearly
> every language, even bash.
>
> I've written a small description of it here: https://www.wireguard.io/xplatform/
>
> This will be part of the next snapshot.
>
> Any questions?

I applaud the general idea of moving to a text-based interface. But why
invent Yet Another Configuration Format?

I guess you could say that key=value pairs are fairly straight forward;
but from the examples it looks like there's an implicit nested
structure? I.e. a public_key=xxx line denotes the start of a new
endpoint, and the following keys are logically part of that endpoint?
Or? If this is the case, you'll need a stateful parser to parse it,
which is not immediately obvious from the description, and is bound to
trip people up at some point...

So why not avoid any possible confusion and just emit JSON? Or another
well-established serialisation format where the nesting can be made
explicit... :)

-Toke

Re: Text-based IPC for Userspace Implementations

[Ivan Labáth]

Hello,

does changing one peer affect settings of another
peer if they have common allowed_ips?

Regards,
Ivan Labáth

On Tue, May 16, 2017 at 02:36:00PM +0200, Jason A. Donenfeld wrote:
> Hey guys,
> 
> Currently wg(8) talks to the kernel by passing structs through an
> ioctl. Due to upstream demands, this is going to be changed to
> netlink, and we'll ditch those structs. wg(8) previously tried to
> re-use those same structs for userspace implementations, passing them
> through a unix socket. Implementors did not like dealing with this.
> Since the structs are going for the kernel stuff, we might as well
> move to something better, too, for the userspace stuff. Therefore
> wg(8) now has a very simple text-based IPC format over unix sockets
> (or Windows named pipes) that can be easily implemented in nearly
> every language, even bash.
> 
> I've written a small description of it here: https://www.wireguard.io/xplatform/
> 
> This will be part of the next snapshot.
> 
> Any questions?
> 
> Regards,
> Jason
> _______________________________________________
> WireGuard mailing list
> WireGuard@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: Text-based IPC for Userspace Implementations

[Jonathan Rudenberg]

> On May 16, 2017, at 09:12, Toke H=C3=B8iland-J=C3=B8rgensen =
<toke@toke.dk> wrote:
>=20
> So why not avoid any possible confusion and just emit JSON? Or another
> well-established serialisation format where the nesting can be made
> explicit... :)

+1 to this, requiring implementors to also implement a custom =
serialization format will lead to avoidable bugs.

I took a quick look and there is a bit of precedent for the use of JSON =
in the kernel tree: =
https://github.com/torvalds/linux/tree/3401e8d1e1300742ed41910b9338b9da526=
89a16/tools/perf/pmu-events

Jonathan

Re: Text-based IPC for Userspace Implementations

[Toke Høiland-Jørgensen]

Jonathan Rudenberg <jonathan@titanous.com> writes:

>> On May 16, 2017, at 09:12, Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.d=
k> wrote:
>>=20
>> So why not avoid any possible confusion and just emit JSON? Or another
>> well-established serialisation format where the nesting can be made
>> explicit... :)
>
> +1 to this, requiring implementors to also implement a custom
> serialization format will lead to avoidable bugs.
>
> I took a quick look and there is a bit of precedent for the use of
> JSON in the kernel tree:
> https://github.com/torvalds/linux/tree/3401e8d1e1300742ed41910b9338b9da52=
689a16/tools/perf/pmu-events

Yeah, JSON is definitely gaining ground as the go-to format. See also
NetJSON: http://netjson.org/docs/what.html

-Toke

Re: Text-based IPC for Userspace Implementations

[Guanhao Yin]

Hi,

在 2017年05月16日 21:12, Toke Høiland-Jørgensen 写道:
> "Jason A. Donenfeld" <Jason@zx2c4.com> writes:
>
>> Hey guys,
>>
>> Currently wg(8) talks to the kernel by passing structs through an
>> ioctl. Due to upstream demands, this is going to be changed to
>> netlink, and we'll ditch those structs. wg(8) previously tried to
>> re-use those same structs for userspace implementations, passing them
>> through a unix socket. Implementors did not like dealing with this.
>> Since the structs are going for the kernel stuff, we might as well
>> move to something better, too, for the userspace stuff. Therefore
>> wg(8) now has a very simple text-based IPC format over unix sockets
>> (or Windows named pipes) that can be easily implemented in nearly
>> every language, even bash.
>>
>> I've written a small description of it here:
https://www.wireguard.io/xplatform/
>>
>> This will be part of the next snapshot.
>>
>> Any questions?

Good! 😄.

In the “configuration protocol” section, I think “wg(8) responds to
...” should be “An userspace implementation respondes to ...”?

A text based format is certainly easier to deal with than C
structs. Hope I can find some time to finally implement it in
WireGuard.rs.

That said, I still like the idea that an userspace implementation
being usable without wg(8): it just takes a configuration and runs it,
simple and straightforward.

>
> I applaud the general idea of moving to a text-based interface. But why
> invent Yet Another Configuration Format?
>
> I guess you could say that key=value pairs are fairly straight forward;
> but from the examples it looks like there's an implicit nested
> structure? I.e. a public_key=xxx line denotes the start of a new
> endpoint, and the following keys are logically part of that endpoint?
> Or? If this is the case, you'll need a stateful parser to parse it,
> which is not immediately obvious from the description, and is bound to
> trip people up at some point...
>
> So why not avoid any possible confusion and just emit JSON? Or another
> well-established serialisation format where the nesting can be made
> explicit... :)

+1 for well-established serialisation format.


Guanhao Yin

Re: Text-based IPC for Userspace Implementations

[Jörg Thalheim]

On 2017-05-16 17:01, Jonathan Rudenberg wrote:
>> On May 16, 2017, at 09:12, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> So why not avoid any possible confusion and just emit JSON? Or another
>> well-established serialisation format where the nesting can be made
>> explicit... :)
> +1 to this, requiring implementors to also implement a custom serialization format will lead to avoidable bugs.
>
> I took a quick look and there is a bit of precedent for the use of JSON in the kernel tree: https://github.com/torvalds/linux/tree/3401e8d1e1300742ed41910b9338b9da52689a16/tools/perf/pmu-events
>
> Jonathan

For the kernel, netlink will be used, which is already does its own serialization format. The use of JSON here, would not make sense here.

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

On Tue, May 16, 2017 at 6:33 PM, Guanhao Yin <sopium@mysterious.site> wrote=
:
> In the =E2=80=9Cconfiguration protocol=E2=80=9D section, I think =E2=80=
=9Cwg(8) responds to
> ...=E2=80=9D should be =E2=80=9CAn userspace implementation respondes to =
...=E2=80=9D?

Nice find, thanks.

> A text based format is certainly easier to deal with than C
> structs. Hope I can find some time to finally implement it in
> WireGuard.rs.

Looking forward to that.

Regards,
Jason

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

Hello list,

Re:JSON --

JSON is difficult to parse and requires large libraries, especially
for something like JSON-RPC. I'd rather not have these dependencies or
complications.

On the contrary, a simple key=value newline scheme can be parsed
trivially by anyone in a safe way, and allows for very quick in basic
implementations in nearly all environments, even bash. Even given a
JSON library, a stream of key=value is considerably easier and more
flexible in the parsing stage.

JSON is nice but it is not ideal for all environments.

Jason

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

On Tue, May 16, 2017 at 9:26 PM, J=C3=B6rg Thalheim <joerg@higgsboson.tk> w=
rote:
> For the kernel, netlink will be used, which is already does its own seria=
lization format. The use of JSON here, would not make sense here.

Right. I'd indeed be very amused to see a JSON parser land in the Linux ker=
nel.

Jason

Re: Text-based IPC for Userspace Implementations

[Manuel Schölling]

Hi Jason,

Great to hear that wireguard gets a nice IPC interface.
Do you already know when this is about to land in the wireguard
releases/repos?

Bye,

Manuel


On Wed, 2017-05-17 at 16:01 +0200, Jason A. Donenfeld wrote:
> On Tue, May 16, 2017 at 9:26 PM, J=C3=B6rg Thalheim <joerg@higgsboson.tk>
> wrote:
> > For the kernel, netlink will be used, which is already does its own
> > serialization format. The use of JSON here, would not make sense
> > here.
>=20
> Right. I'd indeed be very amused to see a JSON parser land in the
> Linux kernel.
>=20
> Jason
> _______________________________________________
> WireGuard mailing list
> WireGuard@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

Hi Manuel,

On Wed, May 17, 2017 at 4:04 PM, Manuel Sch=C3=B6lling
<manuel.schoelling@gmx.de> wrote:
> Great to hear that wireguard gets a nice IPC interface.
> Do you already know when this is about to land in the wireguard
> releases/repos?

When Trevor wakes up today and publishes rev32b of Noise, I'm planning
on releasing a snapshot of WireGuard seconds after. Currently 7am on
the pacific coast...

Jason

Re: Text-based IPC for Userspace Implementations

[Bzzzz]

On Wed, 17 May 2017 16:01:16 +0200
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> 
> Right. I'd indeed be very amused to see a JSON parser land in the
> Linux kernel.

Perhaps, this could do the trick: https://github.com/martinh/libconfuse

Jean-Yves

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

On Wed, May 17, 2017 at 4:14 PM, Bzzzz <lazyvirus@gmx.com> wrote:
> On Wed, 17 May 2017 16:01:16 +0200
> "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>> Right. I'd indeed be very amused to see a JSON parser land in the
>> Linux kernel.
>
> Perhaps, this could do the trick: https://github.com/martinh/libconfuse

If you'd like to add a JSON parser to the Linux kernel, please direct
your questions to linux-kernel@vger.kernel.org instead. You'll be
asking solely on your own accord; please do _not_ mention WireGuard.

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

[-- Attachment #1.1: Type: text/plain, Size: 17 bytes --]

Ta da!


​

[-- Attachment #1.2: Type: text/html, Size: 163 bytes --]

[-- Attachment #2: wg8-on-freebsd.png --]
[-- Type: image/png, Size: 428778 bytes --]

Re: Text-based IPC for Userspace Implementations

[Toke Høiland-Jørgensen]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hello list,
>
> Re:JSON --
>
> JSON is difficult to parse and requires large libraries, especially
> for something like JSON-RPC. I'd rather not have these dependencies or
> complications.
>
> On the contrary, a simple key=value newline scheme can be parsed
> trivially by anyone in a safe way, and allows for very quick in basic
> implementations in nearly all environments, even bash. Even given a
> JSON library, a stream of key=value is considerably easier and more
> flexible in the parsing stage.

My point was not so much "just use JSON". My point was "you are
disguising something that requires stateful parsing as a simple
key/value scheme, which is bound to lead to bugs eventually". I.e. if
someone were to write a parser that would (e.g.) just grep for a public
key, they'd end up with ambiguous results at best.

-Toke

Re: Text-based IPC for Userspace Implementations

[Jason A. Donenfeld]

On Wed, May 17, 2017 at 8:07 PM, Toke H=C3=B8iland-J=C3=B8rgensen <toke@tok=
e.dk> wrote:
> My point was not so much "just use JSON". My point was "you are
> disguising something that requires stateful parsing as a simple
> key/value scheme, which is bound to lead to bugs eventually". I.e. if
> someone were to write a parser that would (e.g.) just grep for a public
> key, they'd end up with ambiguous results at best.

If your random 256-bit public key is accidentally the prefix or suffix
of something else somewhere else, you have a problem with your RNG.

Anyway, JSON is not regular, though it is context-free, which I think
is your argument. If I had to choose between the two, I'd easily
choose something that's regular to parse, since it can be implemented
in a bug free fashion trivially anywhere.

allowed_ips move semantics?

[Ivan Labáth]

Hi,

I was asking, because I think automatically
moving allowed_ips is prone to cause more
trouble than the small savings of one or few
commands, both via IPC and the wg tool. I would
suggest having an error semantics instead.


I believe there are two cases where moving
ranges is relevant:

1) You know the range is a duplicate, in which
case it shouln't be that hard to remove it
from the relevant peer first.
If it is hard, then it is a good area
of improvement.

2) You don't know the range is a duplicate,
in which case you have probably made a mistake.
One wich you might not notice until you see
something is broken, so an error instead would
be welcome.


Semantics of (permanently) moving a range
from another peer is not obvious unless you
know it or actually consider the effects.

IMO it is a bad default. It will cause pains
to fat-fingered sysadmins, it is prone to
race conditions and even more importantly
if in API, it will lead to development of
slightly broken tools.


Case in point:

The wg tool itself (0.0.20170517) will happily
accept this configuration

[Interface]
ListenPort = 51820
PrivateKey = NNNNooootttt++rrreeeaaalll+++kkkkkeeeeyyyy0=

[Peer]
PublicKey = 5c/Fuf2V7tgcxNRfMvuyCsZ+/5xXZm1pxewmvpY0n1k=
AllowedIPs = 172.16.0.1/32

[Peer]
PublicKey = 6yztQEsu3vCsKz3WrCgqXfTjizHAtTylqAQzrTwjIA0=
AllowedIPs = 172.16.0.1/32

After load, the first peer will have no allowed ips,
which was probably not intended and in large configurations
it would be easy to miss.


Example of prior art:

# ip route add 172.16.0.1/32 dev lo
# ip route add 172.16.0.1/32 dev wg0
RTNETLINK answers: File exists


I would suggest changing allowed_ips moves to be errors,
and possibly improving the wg tool to make removing unwanted
allowed_ips easier. Perhaps something ip route add/delete style
would be appropriate.

Regards,
Ivan


On Wed, May 17, 2017 at 03:47:51PM +0200, Jason A. Donenfeld wrote:
> Hi Ivan,
> 
> On Tue, May 16, 2017 at 5:42 PM, Ivan Labáth <labawi-wg@matrix-dream.net> wrote:
> > does changing one peer affect settings of another
> > peer if they have common allowed_ips?
> 
> Great question. I've improved the documentation to note this. The
> answer is that: if you have a 100% identical allowed_ips entry in a
> first peer and in a second peer, the entry moves from the first to the
> second.
> 
> Jason

Re: allowed_ips move semantics?

[Jason A. Donenfeld]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

Hi Ivan,

You make a lot of compelling points there, and I'm inclined to follow your
advice. The (untested) patch to change this to the behavior you'd like is
actually this trivial two line change:

diff --git a/src/routingtable.c b/src/routingtable.c
index f9c3eff..0ad4d3c 100644
--- a/src/routingtable.c
+++ b/src/routingtable.c
@@ -194,6 +194,8 @@ static int add(struct routing_table_node __rcu **trie,
u8 bits, const u8 *key, u
  return 0;
  }
  if (node_placement(*trie, key, cidr, bits, &node, lock)) {
+ if (node->peer)
+ return -EEXIST;
  node->peer = peer;
  return 0;
  }

I'm especially compelled by your point that this would mirror the semantics
of the routing table.

However, I was a bit confused by your mention of a race condition, because
I believe that my current solution _avoids_ a race condition. The routing
table has a concept of metrics, which allows for avoiding a race condition
when gracefully handing over routes between different interfaces, like this:

# ip route add 10.0.0.0/24 dev eth0 metric 1000
# ip route add 10.0.0.0/24 dev wlan0 metric 1001
# ip route del 10.0.0.0/24 dev eth0 metric 1000

(It also has `ip route change`, but I don't know if this is actually
implemented in a race free way in the kernel or how it works.)

If I added the -EEXIST semantic to WireGuard's allowed-ips, if you wanted
to change traffic from one peer to the other, you could not do so without
dropping a bunch of packets during the interim. Thus, a race condition.
This was the original reason for designing it as I did -- I liked the idea
of race-free handovers.

Do you think maintaining race-free handover is important enough to warrant
not using -EEXIST? I sort of think it is important. Of course, I could add
an explicit "change" flag, but that makes things a bit complicated. And, in
the case of your hypothetical fat-fingered sysadmin, the error condition is
that this "fails closed", rather than "fails open", which probably makes it
not such a big deal.

What do you think of that argumentation?

Jason

[-- Attachment #2: Type: text/html, Size: 2882 bytes --]

Re: allowed_ips move semantics?

[Ivan Labáth]

Hi,

I didn't think it would be such a small change :)

You mention changing routes. It is indeed a valid
use for move instead of remove + add, as it should
not drop packets, but I do not think it is a compelling
reason. Executing remove + add shouldn't take more than
a few milliseconds and networks should be capable
of handling transient errors. Unless you start responding
with resets or something, but it seems like a niche
use where users would be capable of handling it.


The race condition I mentioned was that of adding peers,
or adding new ips to existing peers.
With move semantics, if you use an interface with dynamic
ips, there is no way to be sure you didn't remove some
peer's ip, unless you use some form of external locking
and/or centralized database.

On top of a move semantics API you cannot implement a race-free
just_add_peer_with_ips_but_dont_snatch_from_others operation.
Because of that you also cannot implement an operation
that would safely add a peer and then remove it without
possibly affecting other peers even after it was removed.

If I compare functionality
a) add_ips_and_possibly_remove_from_others
b) add_ips_but_fail_if_others_use_it
I believe users will mostly want to use (b). Also (b)
is safer as it is less likely to have unintended consequences.
A good API is one that is hard to misuse. API (a) is
easy to misuse, (b) is not. As wireguard is a security
product it should provide (b).

If you wish to provide an API with move semantics,
I would even consider to make the API be
c) move_ips_X_from_peer_A_to_peer_B,
rather than the (a) version
a) add_ips_X_to_peer_B_and_remove_from_others
just so it wouldn't be easier to do the wrong
thing and mindlessly stick a move flag rather
than the right thing and find out where the ip
is used and handle it accordingly.


I think preventing the unintended consequences of removing
ips from peers when doing e.g.
wg add peer B ; wg remove peer B
which will happen, is more important than small temporary
packet loss from
wg peer A remove ips && wg peer B add ips


Regards,
ivan


On Thu, May 18, 2017 at 12:08:37PM +0200, Jason A. Donenfeld wrote:
> Hi Ivan,
> 
> You make a lot of compelling points there, and I'm inclined to follow your
> advice. The (untested) patch to change this to the behavior you'd like is
> actually this trivial two line change:
> 
> diff --git a/src/routingtable.c b/src/routingtable.c
> index f9c3eff..0ad4d3c 100644
> --- a/src/routingtable.c
> +++ b/src/routingtable.c
> @@ -194,6 +194,8 @@ static int add(struct routing_table_node __rcu **trie,
> u8 bits, const u8 *key, u
>   return 0;
>   }
>   if (node_placement(*trie, key, cidr, bits, &node, lock)) {
> + if (node->peer)
> + return -EEXIST;
>   node->peer = peer;
>   return 0;
>   }
> 
> I'm especially compelled by your point that this would mirror the semantics
> of the routing table.
> 
> However, I was a bit confused by your mention of a race condition, because
> I believe that my current solution _avoids_ a race condition. The routing
> table has a concept of metrics, which allows for avoiding a race condition
> when gracefully handing over routes between different interfaces, like this:
> 
> # ip route add 10.0.0.0/24 dev eth0 metric 1000
> # ip route add 10.0.0.0/24 dev wlan0 metric 1001
> # ip route del 10.0.0.0/24 dev eth0 metric 1000
> 
> (It also has `ip route change`, but I don't know if this is actually
> implemented in a race free way in the kernel or how it works.)
> 
> If I added the -EEXIST semantic to WireGuard's allowed-ips, if you wanted
> to change traffic from one peer to the other, you could not do so without
> dropping a bunch of packets during the interim. Thus, a race condition.
> This was the original reason for designing it as I did -- I liked the idea
> of race-free handovers.
> 
> Do you think maintaining race-free handover is important enough to warrant
> not using -EEXIST? I sort of think it is important. Of course, I could add
> an explicit "change" flag, but that makes things a bit complicated. And, in
> the case of your hypothetical fat-fingered sysadmin, the error condition is
> that this "fails closed", rather than "fails open", which probably makes it
> not such a big deal.
> 
> What do you think of that argumentation?
> 
> Jason

Re: Text-based IPC for Userspace Implementations

[Manuel Schölling]

On Wed, 2017-05-17 at 16:08 +0200, Jason A. Donenfeld wrote:
> Hi Manuel,
>=20
> On Wed, May 17, 2017 at 4:04 PM, Manuel Sch=C3=B6lling
> <manuel.schoelling@gmx.de> wrote:
> > Great to hear that wireguard gets a nice IPC interface.
> > Do you already know when this is about to land in the wireguard
> > releases/repos?
>=20
> When Trevor wakes up today and publishes rev32b of Noise, I'm
> planning
> on releasing a snapshot of WireGuard seconds after. Currently 7am on
> the pacific coast...

I am a bit confused: I updated to 0.0.20170517 on Debian, but no
/run/wireguard/ or /var/run/wireguard/ exists.
Then I checked the source code and apparently only the wireguard-tools
support the new socket IPC interface but the kernel module does not.
Is that observation correct?

Thanks!

Manuel

Re: Text-based IPC for Userspace Implementations

[Daniel Kahn Gillmor]

On Thu 2017-05-18 18:46:42 +0200, Manuel Schölling wrote:
> On Wed, 2017-05-17 at 16:08 +0200, Jason A. Donenfeld wrote:
>> Hi Manuel,
>> 
>> On Wed, May 17, 2017 at 4:04 PM, Manuel Schölling
>> <manuel.schoelling@gmx.de> wrote:
>> > Great to hear that wireguard gets a nice IPC interface.
>> > Do you already know when this is about to land in the wireguard
>> > releases/repos?
>> 
>> When Trevor wakes up today and publishes rev32b of Noise, I'm
>> planning
>> on releasing a snapshot of WireGuard seconds after. Currently 7am on
>> the pacific coast...
>
> I am a bit confused: I updated to 0.0.20170517 on Debian, but no
> /run/wireguard/ or /var/run/wireguard/ exists.
> Then I checked the source code and apparently only the wireguard-tools
> support the new socket IPC interface but the kernel module does not.
> Is that observation correct?

yes!  the subject line is "for Userspace Implementations" , so i think
they're not relevant for kernel implementations :)

      --dkg