Fix and improve interface binding

[mosterdt]

Thank you for contributing your time to the Mosquitto project!

Before you go any further, please note that we cannot accept contributions if
you haven't signed the Eclipse Contributor Agreement.
If you aren't able to do that, or just don't want to, please describe your bug
fix/feature change in an issue. For simple bug fixes it is can be just as easy
for us to be told about the problem and then go fix it directly.

Then please check the following list of things we ask for in your pull request:

• Have you signed the Eclipse Contributor Agreement, using the same email address as you used in your commits?
• Do each of your commits have a "Signed-off-by" line, with the correct email address? Use "git commit -s" to generate this line for you.
• (N/A) If you are contributing a new feature, is your work based off the develop branch?
• If you are contributing a bugfix, is your work based off the fixes branch?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you successfully run make test with your changes locally?

---

Hi Mosquitto developers,

While upstepping this component, we noticed a regression in the interface binding of the broker.

More specifically: the commit 886ee6c changed the interface binding behavior from using SO_BINDTODEVICE to a method using getifaddrs().

Overstrict binding

While this approach works fine in most use-cases, it can fail when (1) no host is specified, (2) the specified interface does not support ipv4 (or ipv6) and (3) there are other interfaces that do support the missing sa_family. In this use-case, the call to getaddrinfo() will give 2 options to bind to: an ipv4 and an ipv6 version. In the loop, the broker will try to get the address for both families, but it will fail on one of the two, resulting in a failed startup. (It's all or nothing)

Steps to reproduce:

ip link add eth_dummy type dummy
ip addr add 192.168.111.2/24 dev eth_dummy
# (you can remove the interface with `ip link del eth_dummy`)
printf "listener 31337\nbind_interface eth_dummy\n" > broker.conf
mosquitto -v -c broker.conf

will result in:

1612816913: mosquitto version 2.0.7 starting                                                 
1612816913: Config loaded from broker.conf.
1612816913: Opening ipv4 listen socket on port 31337.
1612816913: Opening ipv6 listen socket on port 31337.
1612816913: Error: Interface eth_dummy not found.

The first commit fixes this issue:

1612817207: mosquitto version 2.0.7 starting
1612817207: Config loaded from broker.conf.
1612817207: Opening ipv4 listen socket on port 31337.
1612817207: Opening ipv6 listen socket on port 31337.
1612817207: Warning: Interface eth_dummy does not support ipv6.
1612817207: mosquitto version 2.0.7 running
...

Bind address overwrite

A second issue occurs when specifying both a host to a listener config line, and a bind_interface:

listener 31337 192.168.66.1
bind_interface eth_dummy

Because there is no check in the current net__bind_interface(), the specified listener address is overwritten by the first valid address returned from getifaddrs(). The second commit addresses this issue by checking if the interface address matches the host. If it does, the address is accepted. The function now seems somewhat complex, so you might just want to disallow this configuration combination altogether. The current solution results in mosquitto listening to the intersection between (the listen address/host) and (the interface addresses).

With the same setup as before, but with the changed configuration, the original code gives:

1612822113: mosquitto version 2.0.7 starting
1612822113: Config loaded from broker.conf.
1612822113: Opening ipv4 listen socket on port 31337.
1612822113: mosquitto version 2.0.7 running

but ultimately on the "wrong" address:

~ $ netstat -tlpn | grep mosquit 
tcp        0      0 192.168.111.2:31337     0.0.0.0:*               LISTEN      2824914/src/mosquit 

With the fix, mosquitto refuses to launch as no valid listeners could be created:

1612822661: mosquitto version 2.0.7 starting
1612822661: Config loaded from broker.conf.
1612822661: Opening ipv4 listen socket on port 31337.
1612822661: Warning: Interface address does not match specified listener host.
1612822661: Warning: Interface eth_dummy does not support ipv4 configuration.

Incomplete binding

For this issue, I don't have an immediate fix. The problem here is that one interface can have multiple IP addresses. One might expect the bind_interface config option to bind to each and every one of those, instead of only the first (valid) one returned by getifaddrs(). The current code is written with the expectation that there is only one IP address that would match a family. A possible, proper fix would be to first enumerate the addresses from an interface, and only then as a next step create a listener for each of those addresses. It would be nice to reuse the net__socket_listen_tcp() call from some net__socket_listen_tcp_interface(), but then you need to convert structures back to strings first. I do want to take a throw at implementing it, but I would love to hear your opinion first.

---

If there are any other issues with this MR, please don't hold back. I would be glad to fix them.

[ralight]

Thanks very much for the nice explanation and changes. The move from using SO_BINDTODEVICE was due mostly to wanting to get rid of all privileged operations.

I agree with what you've proposed, I'll take a look at testing it properly in a little while.

I'm not sure what to say with regards the incomplete binding. If it is possible to implement with a single listener then it makes complete sense we should do so. If we effectively have to create new listeners to be able to bind to multiple IP addresses, then this is potentially a problem.

[mosterdt]

Looking at it again, and getting a bit more familiar with the direction you're going (deprecating bind_address and port), I think it's a matter of taste.
Like bind_address and port, bind_interface is a global option, not tied to any specific listener. I see multiple ways in which someone could interpret a configuration with a bind_interface defined:

listener 1883
bind_interface eth0

Could mean:

• Listen on each IP address defined on the specified interface on port 1883 (filter)
• Listen on all available addresses and ignore the bind_interface (ignored like port and bind_address)
• Listen on all available addresses and on the addresses of the specified interface (union)

listener 1883 192.0.2.3
bind_interface eth0

Could mean:

• Listen on 192.0.2.3, but only if this address is bound to eth0 (filter)
• Listen on 192.0.2.3 and ignore the bind_interface (ignored like port and bind_address)
• Listen on 192.0.2.3 and on the addresses of the specified interface (union)

Then there are also the cases where multiple listeners are defined, ..
In general, it looks like it would be a good idea to define an interface as an additional argument for a listener:

listener port [bind address/host/unix socket path/interface]

The issue here is recognizing the difference between an interface or a host. Cups solves this by putting @IF() around an interface (see cupsd.conf(5)), but that feels somewhat hacky.

I don't think the filter case of the second config is realistic, you could just omit the bind_interface config option. Same for the union case in the first example. Writing this out, the missing feature here seems to really being able to define a listener that listens on an interface and certain port. To avoid the strange syntax, you might want to keep/extend bind_interface as a form of listener_dev port interface, or create a second listener keyword for this usecase. (backwards compatibility etc.)

This also doesn't answer on how the issue should be fixed in this version, but it's probably important to notice that there is some expressiveness missing in the current configuration options.

[ralight]

Like bind_address and port, bind_interface is a global option, not tied to any specific listener.

That is not the case, bind_interface is specific to a listener, which I think changes all of your comments below.

I see multiple ways in which someone could interpret a configuration with a bind_interface defined:

listener 1883
bind_interface eth0

Could mean:

* Listen on each IP address defined on the specified interface on port 1883 (filter)
* Listen on all available addresses and ignore the bind_interface (ignored like port and bind_address)
* Listen on all available addresses and on the addresses of the specified interface (union)

"Listen on all available addresses" doesn't apply, because bind_interface is specific to the listener. At the moment this only binds to a single address from eth0, as we've discussed.

listener 1883 192.0.2.3
bind_interface eth0

Could mean:

* Listen on 192.0.2.3, but only if this address is bound to eth0 (filter)

This is the only one that can apply.

In general, it looks like it would be a good idea to define an interface as an additional argument for a listener:

listener port [bind address/host/unix socket path/interface]

The issue here is recognizing the difference between an interface or a host. Cups solves this by putting @IF() around an interface (see cupsd.conf(5)), but that feels somewhat hacky.

Agreed on both points. This approach would prevent the awkward edge cases around having both an interface and an IP address specified, which I think is the crux of your argument.

[mosterdt]

Oh, I see, I was a bit off the mark then. Maybe let's ignore this last issue for now, as it seems like it's something that would be best implemented by people with better knowledge of the codebase. I hope my current contributions in this merge request are still helpful, and at least they fix my usecase. Thank you for maintaining this project!