Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Tristan, I'm back on this topic (I had not forgotten it). On Sat, Mar 09, 2024 at 07:02:34PM +, Tristan wrote: > > > On 09/03/2024 18:09, Tristan wrote: > > Hi Willy, > > > > On 09/03/2024 16:51, Willy Tarreau wrote: > > > Hi Tristan, > > > > > > On Sat, Mar 09, 2024 at 04:20:21PM +, Tristan wrote: > > > > To be honest, I don't think this is unfixable. It's just a matter of how > > > > much code change we think is acceptable for it. > > > > > > I don't mind about the amount of changes. "we've always done it like > > > this" > > > is never a valid excuse to keep code as it is, especially when it's wrong > > > and causes difficulties to add new features. Of course I prefer invasive > > > changes early in the dev cycle than late but we're still OK and I don't > > > expect that many changes for this anyway. > > > > Agreed on not shying away from amount of changes. > > > > I tried to follow that route, being generous in what I was willing to > > touch, but ran into the issue of protocols and socket families being a > > 1:1 binding. > > > > So adding a new socket family *requires* adding a new protocol too, > > which isn't too hard but spirals a bit out of control: > > - protocol_by_family is a misnomer, and it is actually protocols *by > > socket domain* (which is still AF_UNIX, even with family AF_CUST_ABNS2, > > and ends up overriding standard socket/abns protocol) > > - but you can't just change that, because then other things stop working > > as some places rely on socket domain != socket family > > Actually, following up on this, I'm more and more convinced that it actually > makes things somewhat worse to take this approach... > > At least I don't see a good way to make it work without looking very bad, > because in a few places we lookup a protocol based on a socket_storage > reference, so we have truly only the socket domain to work with if no > connection is established yet. > > While it sounds like shying away from deeper changes, it seems to me that a > bind/server option is truly the right way to go here. I addressed these shortcomings of protocol lookups in a branch here: https://github.com/haproxy/haproxy/tree/20240809-abnsz-4 There was an API bug dating from 2.3 that was making everything more complicated than necessary, the code was assigning the custom family to sock_domain (the socket() argument) instead of sock_family (the one we're supposed to store into the address itself). I fixed that and added the minimal needed logic to properly perform the lookups and also resolve custom families into their real ones, because previously we had to perform random protocol lookups to figure them (we didn't notice since other custom families don't map to real ones). It even simplified a few places where some values were hard-coded in certain calls, or some logic enforced late (e.g. log.c). I now created abns as a custom proto, separate from uxst, so that it has its own family. It was not strictly necessary but it will allow us to get rid of plenty of \0 tests at many places by just having functions dedicated to unix or abns. Even the addrcmp() function should become simpler and I thought I'd specialize them. I've just added a last patch to create abnsz (the zero-terminated abns, since William rightfully suggested that the codename abns2 could make one think it's version 2), but for now it's a perfect copy of the other one, I have not modified the address length checks. I remember that you had a working PoC so I guess you've already spotted all locations and what to change, so restarting from your patch set will be much faster now. Please let me know if you want to give it a try, or if you think you won't have time to look into it and you prefer me to try to merge your work into this, it's as you prefer. I feel like we're finally seeing the light at the end of this long tunnel! I'm done for today and probably for the week-end as well I think. Have a nice week-end! Wily
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Willy,
Since you mentioned this topic in the 3.1-dev2 release notes, let's
start the discussion again I suppose.
To summarize what I was saying at the end last time:
1. Duplicating abns into abns2 introduces a non-trivial amount of cruft
and awkward bits and pieces
2. I still don't see the abns/abns2 split as worth it, as opposed to a
`pad { on | off }` (or similar) bind argument being added to the abns spec
Tristan
On 09/03/2024 14:02, Tristan wrote:
On 09/03/2024 18:09, Tristan wrote:
Hi Willy,
On 09/03/2024 16:51, Willy Tarreau wrote:
Hi Tristan,
On Sat, Mar 09, 2024 at 04:20:21PM +, Tristan wrote:
To be honest, I don't think this is unfixable. It's just a matter of
how
much code change we think is acceptable for it.
I don't mind about the amount of changes. "we've always done it like
this"
is never a valid excuse to keep code as it is, especially when it's
wrong
and causes difficulties to add new features. Of course I prefer invasive
changes early in the dev cycle than late but we're still OK and I don't
expect that many changes for this anyway.
Agreed on not shying away from amount of changes.
I tried to follow that route, being generous in what I was willing to
touch, but ran into the issue of protocols and socket families being a
1:1 binding.
So adding a new socket family *requires* adding a new protocol too,
which isn't too hard but spirals a bit out of control:
- protocol_by_family is a misnomer, and it is actually protocols *by
socket domain* (which is still AF_UNIX, even with family
AF_CUST_ABNS2, and ends up overriding standard socket/abns protocol)
- but you can't just change that, because then other things stop
working as some places rely on socket domain != socket family
Actually, following up on this, I'm more and more convinced that it
actually makes things somewhat worse to take this approach...
At least I don't see a good way to make it work without looking very
bad, because in a few places we lookup a protocol based on a
socket_storage reference, so we have truly only the socket domain to
work with if no connection is established yet.
While it sounds like shying away from deeper changes, it seems to me
that a bind/server option is truly the right way to go here.
Tristan
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
On 09/03/2024 18:09, Tristan wrote: Hi Willy, On 09/03/2024 16:51, Willy Tarreau wrote: Hi Tristan, On Sat, Mar 09, 2024 at 04:20:21PM +, Tristan wrote: To be honest, I don't think this is unfixable. It's just a matter of how much code change we think is acceptable for it. I don't mind about the amount of changes. "we've always done it like this" is never a valid excuse to keep code as it is, especially when it's wrong and causes difficulties to add new features. Of course I prefer invasive changes early in the dev cycle than late but we're still OK and I don't expect that many changes for this anyway. Agreed on not shying away from amount of changes. I tried to follow that route, being generous in what I was willing to touch, but ran into the issue of protocols and socket families being a 1:1 binding. So adding a new socket family *requires* adding a new protocol too, which isn't too hard but spirals a bit out of control: - protocol_by_family is a misnomer, and it is actually protocols *by socket domain* (which is still AF_UNIX, even with family AF_CUST_ABNS2, and ends up overriding standard socket/abns protocol) - but you can't just change that, because then other things stop working as some places rely on socket domain != socket family Actually, following up on this, I'm more and more convinced that it actually makes things somewhat worse to take this approach... At least I don't see a good way to make it work without looking very bad, because in a few places we lookup a protocol based on a socket_storage reference, so we have truly only the socket domain to work with if no connection is established yet. While it sounds like shying away from deeper changes, it seems to me that a bind/server option is truly the right way to go here. Tristan
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Willy, On 09/03/2024 16:51, Willy Tarreau wrote: Hi Tristan, On Sat, Mar 09, 2024 at 04:20:21PM +, Tristan wrote: To be honest, I don't think this is unfixable. It's just a matter of how much code change we think is acceptable for it. I don't mind about the amount of changes. "we've always done it like this" is never a valid excuse to keep code as it is, especially when it's wrong and causes difficulties to add new features. Of course I prefer invasive changes early in the dev cycle than late but we're still OK and I don't expect that many changes for this anyway. Agreed on not shying away from amount of changes. I tried to follow that route, being generous in what I was willing to touch, but ran into the issue of protocols and socket families being a 1:1 binding. So adding a new socket family *requires* adding a new protocol too, which isn't too hard but spirals a bit out of control: - protocol_by_family is a misnomer, and it is actually protocols *by socket domain* (which is still AF_UNIX, even with family AF_CUST_ABNS2, and ends up overriding standard socket/abns protocol) - but you can't just change that, because then other things stop working as some places rely on socket domain != socket family If push comes to shove, one can always add an extra field somewhere, or an extra arg in get_addr_len, even if it'd be a little ugly. Actually you're totally right and you make me realize that there's an addrcmp field in the address families, and it's inconsistent to have get_addr_len() being family-agnostic. So most likely we should first change get_addr_len() to be per-family and appear in the proto_fam struct. The few places where get_addr_len() is used should instead rely on the protocol family for this. And due to this new address having a variable length, we should support passing a pointer to the address (like the current get_addr_len() does) in addition to the protocol pointer. I tried to do that yes, changing sock_addrlen to be a function pointer. But it's actually less convenient, surprisingly, since then you end up with constructs like: listener->rx/tx->ss->proto->fam->get_addrlen(listener->rx/tx->ss) (paraphrasing from memory; point is that it becomes somewhat unpleasant to look at) Still might end up having to do that given how tricky things become otherwise... Doing so would cleanly unlock all the problem. The new abns2 family would just bring its own get_addr_len() variant that uses strlen(). Agreed on that OOP-ish approach being in principle a bit nicer though (sorry for the swear word ;-) ) Tristan
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Tristan, On Sat, Mar 09, 2024 at 04:20:21PM +, Tristan wrote: > To be honest, I don't think this is unfixable. It's just a matter of how > much code change we think is acceptable for it. I don't mind about the amount of changes. "we've always done it like this" is never a valid excuse to keep code as it is, especially when it's wrong and causes difficulties to add new features. Of course I prefer invasive changes early in the dev cycle than late but we're still OK and I don't expect that many changes for this anyway. > If push comes to shove, one > can always add an extra field somewhere, or an extra arg in get_addr_len, > even if it'd be a little ugly. Actually you're totally right and you make me realize that there's an addrcmp field in the address families, and it's inconsistent to have get_addr_len() being family-agnostic. So most likely we should first change get_addr_len() to be per-family and appear in the proto_fam struct. The few places where get_addr_len() is used should instead rely on the protocol family for this. And due to this new address having a variable length, we should support passing a pointer to the address (like the current get_addr_len() does) in addition to the protocol pointer. Doing so would cleanly unlock all the problem. The new abns2 family would just bring its own get_addr_len() variant that uses strlen(). This just shows how important it is to discuss about design ;-) Cheers, Willy
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Willy, On 08/03/2024 18:01, Willy Tarreau wrote: The problem with default values is that a single behavior cannot be deduced from reading a single config statement. That's quite painful for lots of people (including those who copy config blocks from stackoverflow), and for API tools. And it works half of the time because internally both modes work but they can't interoperate with other tools. Here we're really indicating a new address format. There's nothing wrong with that and we do have the tools to handle that. I think that the plan should be: - add AF_CUST_ABNS2 to protocol-t.h before AF_CUST_MAX - add to str2sa_range() the case for "abns2" just after "abns" with address family "AF_CUST_ABNS2", and duplicate the test for ss.ss_family == AF_UNIX later for AF_CUST_ABNS2 removing all the stuff related to !abstract, or instead, just extend the "if" condition to the new family and add that case there as well (might be even easier). - duplicate "proto_fam_unix" to "proto_fam_abns2" in sock_unix.c, referencing "sock_abns2_addrcmp" for the address comparison - duplicate sock_unix_addrcmp() to sock_abns2_addrcmp() and implement only the abns case, basically: if (a->ss_family != b->ss_family) return -1; if (a->ss_family != AF_CUST_ABNS2) return -1; if (au->sun_path[0]) return -1; return memcmp(au->sun_path, bu->sun_path, sizeof(au->sun_path)); - in sock_unix_bind_receiver(), add a case for this address family in the bind() size argument, replacing sizeof(addr) with the string length when running AF_CUST_ABNS2. - for get_addr_len() I need to think :-/ I actually don't know how that one works with socketpairs already, so there might be a trick I'm overlooking. Alright. I will look at how that looks and report back shortly. I need to leave now, but I continue to think that if there is any internal shortcoming, we should not try to work around it at the expense of trickier long-term maintenance, instead we should address it. And don't worry, I'm not going to assign you the task of dealing with limited stuff. We may end up with the workaround in the worst case if we don't find better, but that would mean declaring a failure and having to break that stuff sometime in the future, which is always a pain. To be honest, I don't think this is unfixable. It's just a matter of how much code change we think is acceptable for it. If push comes to shove, one can always add an extra field somewhere, or an extra arg in get_addr_len, even if it'd be a little ugly. Tristan
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
On Fri, Mar 08, 2024 at 05:38:32PM +, Tristan wrote: > Hi Willy, > > On 08/03/2024 17:05, Willy Tarreau wrote: > > We could just have "abns2" and declare that it's the second version of the > > abns format and know that this one is interoperable with a number of other > > components. > > Having abns@ and abns2@ strictly for that one difference seems like a lot to > me, to be honest, but I get the argument that they are otherwise > fundamentally incompatible... Hmm... Believe me, it's not much (in theory, we can always detect a long tail and later decide to change our minds). I wouldn't recommend you a painful option. > > It would even be easier to deal with for those who document > > such components because their integration docs would just indicate that > > the address has to be placed after "abns2@" in haproxy, instead of saying > > "and don't forget to adjust this global setting in your config, unless you > > already use abns anywhere else". > > Considering there's barely any documentation on the internet as a whole > about abstract unix sockets, I doubt we should worry too much about that > bit. > Also the target audience for those is unlikely to be too deterred. Maybe. One of the reasons might also be the failure from some to make it interoperate with their tools, and they give up. > > Indeed, right now it seems that your patch > > does nothing for the receivers, bind() still uses sizeof(), and the > > sock_unix_addrcmp() function continues to compare the full name, making > > such sockets non-transferable across reloads. So it's unclear to me how > > this can work in your test :-/ > > Maybe I'm missing some place/case, but at least for trivial binds it > certainly works; which explains why the test passes :-) Yes but based on the code I don't see how this is possible. bind() uses sizeof() there, not get_addr_len(). > // without tightsocklen > $ netstat -l | grep @haproxy > unix 2 [ ACC ] STREAM LISTENING 934838 > @haproxy-f1@ > > // with tightsocklen > $ netstat -l | grep @haproxy > unix 2 [ ACC ] STREAM LISTENING 956878 @haproxy-f1 OK thanks for the capture. I just fail to see what code this passes through and that worries me a bit more. > > Thus I think it would be preferable to go that route rather than having > > to deal with all exceptions everywhere based on a global-only option. > > We can help on this, of course. > > Well I do want it so I'm fine with doing it. But I wonder if it's not a bit > of a: > - abns@ eventually not used by anyone as everything keeps on moving away > from padding > - abns2@ eventually the default, which is quite jarring I suppose We don't care. It's like the proxy protocol, one is legacy and still quite deployed, but all improvements come on the new version. If there's no cost in keeping legacy stuff for those who already use it, let's keep it running and don't worry about it. The only configs which will need to be adapted are those which cannot interoperate with external tools anyway, so it's not as if it would require more changes. > If it's easier that way, I'd prefer making it a proper bind/server option, > which allows for easier transition, and can just change default value down > the line if necessary? The problem with default values is that a single behavior cannot be deduced from reading a single config statement. That's quite painful for lots of people (including those who copy config blocks from stackoverflow), and for API tools. And it works half of the time because internally both modes work but they can't interoperate with other tools. Here we're really indicating a new address format. There's nothing wrong with that and we do have the tools to handle that. I think that the plan should be: - add AF_CUST_ABNS2 to protocol-t.h before AF_CUST_MAX - add to str2sa_range() the case for "abns2" just after "abns" with address family "AF_CUST_ABNS2", and duplicate the test for ss.ss_family == AF_UNIX later for AF_CUST_ABNS2 removing all the stuff related to !abstract, or instead, just extend the "if" condition to the new family and add that case there as well (might be even easier). - duplicate "proto_fam_unix" to "proto_fam_abns2" in sock_unix.c, referencing "sock_abns2_addrcmp" for the address comparison - duplicate sock_unix_addrcmp() to sock_abns2_addrcmp() and implement only the abns case, basically: if (a->ss_family != b->ss_family) return -1; if (a->ss_family != AF_CUST_ABNS2) return -1; if (au->sun_path[0]) return -1; return memcmp(au->sun_path, bu->sun_path, sizeof(au->sun_path)); - in sock_unix_bind_receiver(), add a case for this address family in the bind() size argument, replacing sizeof(addr) with the string length when running AF_CUST_ABNS2. - for get_addr_len() I ne
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Willy, On 08/03/2024 17:05, Willy Tarreau wrote: We could just have "abns2" and declare that it's the second version of the abns format and know that this one is interoperable with a number of other components. Having abns@ and abns2@ strictly for that one difference seems like a lot to me, to be honest, but I get the argument that they are otherwise fundamentally incompatible... Hmm... It would even be easier to deal with for those who document such components because their integration docs would just indicate that the address has to be placed after "abns2@" in haproxy, instead of saying "and don't forget to adjust this global setting in your config, unless you already use abns anywhere else". Considering there's barely any documentation on the internet as a whole about abstract unix sockets, I doubt we should worry too much about that bit. Also the target audience for those is unlikely to be too deterred. Indeed, right now it seems that your patch does nothing for the receivers, bind() still uses sizeof(), and the sock_unix_addrcmp() function continues to compare the full name, making such sockets non-transferable across reloads. So it's unclear to me how this can work in your test :-/ Maybe I'm missing some place/case, but at least for trivial binds it certainly works; which explains why the test passes :-) // without tightsocklen $ netstat -l | grep @haproxy unix 2 [ ACC ] STREAM LISTENING 934838 @haproxy-f1@ // with tightsocklen $ netstat -l | grep @haproxy unix 2 [ ACC ] STREAM LISTENING 956878 @haproxy-f1 Thus I think it would be preferable to go that route rather than having to deal with all exceptions everywhere based on a global-only option. We can help on this, of course. Well I do want it so I'm fine with doing it. But I wonder if it's not a bit of a: - abns@ eventually not used by anyone as everything keeps on moving away from padding - abns2@ eventually the default, which is quite jarring I suppose If it's easier that way, I'd prefer making it a proper bind/server option, which allows for easier transition, and can just change default value down the line if necessary? Thanks, Tristan
Re: [RFC] Allow disabling abstract unix socket paths NUL-padding
Hi Tristan,
On Wed, Mar 06, 2024 at 07:32:55AM +, Tristan wrote:
> Hello,
>
> Earlier, I ran into the issue outlined in
> https://github.com/haproxy/haproxy/issues/977
>
> Namely, that HAProxy will NUL-pad (as suffix) abstract unix socket paths,
> causing interop issues with other programs.
> I raised a new feature request to make this behaviour tunable
> (https://github.com/haproxy/haproxy/issues/2479).
Yep, thanks for this.
> Since I do quite want this, I also came up with a patch for it, attached to
> this email.
>
> Marked as RFC for now because there hasn't really been a broad discussion on
> the topic yet.
Sure. There's one thing that bothers me in the way the option is configured,
which is that "unix-bind" takes options from the "bind" line, and here we're
adding a special case where this option is only valid for this line, and it
experiences special treatment. If we do it the "unix-bind" way, it means the
option will also be valid for each "bind" line. I understand the problem it
could cause when parsing it (i.e. was the global setting already set or not)
but that also enlights one of the difficulties related to interpreting the
configuration based on settings in the global section.
I'm still tempted to think that this should be seen as a different address
family since that's basically what we're doing, changing the way addresses
are used. Indeed, except for socket names that are exactly 107-chars long,
there's no way any address of the previous family can interact with one of
the new family.
We could just have "abns2" and declare that it's the second version of the
abns format and know that this one is interoperable with a number of other
components. It would even be easier to deal with for those who document
such components because their integration docs would just indicate that
the address has to be placed after "abns2@" in haproxy, instead of saying
"and don't forget to adjust this global setting in your config, unless you
already use abns anywhere else".
I had a look, and I think everything begins in str2sa_range() in this
block:
else if (ss.ss_family == AF_UNIX) {
...
}
By adding a new AF_CUST_ABNS2 family I think we can extend sock_unix
to properly deal with that. Indeed, right now it seems that your patch
does nothing for the receivers, bind() still uses sizeof(), and the
sock_unix_addrcmp() function continues to compare the full name, making
such sockets non-transferable across reloads. So it's unclear to me how
this can work in your test :-/
Having an explicit family would allow to manage all of this at once:
mostly everywhere we have a test for AF_UNIX we could add || AF_CUST_ABNS2
and make the special case of the address length there. I don't *think*
the socket transfer mechanism in sock.c needs to be touched because
retrieving the socket's address should give us its original length.
Thus I think it would be preferable to go that route rather than having
to deal with all exceptions everywhere based on a global-only option.
We can help on this, of course.
Thanks!
Willy
