search

NetworkBlockDevice / nbd

Network Block Device
GNU General Public License v2.0
450 stars 116 forks source link

Don't listen on TCP if we are using a UNIX socket. #90

Closed Ferroin closed 5 years ago

Ferroin commented 5 years ago

Currently, nbd-server will open a listening TCP socket even if it's been asked to use a UNIX domain socket. This is inconsistent with typical behavior of applications which can use UNIX sockets, and may present a security risk on some systems.

This commit modifies this behavior so that use of a UNIX socket and other types of socket is mutually exclusive. You can only listen on a regular (TCP or SDP) socket or a UNIX socket, but not both, with the presence of the unixsock option taking precedence over any other listener configuration.

This has a small potential to break some existing installations, though they are arguably depending on broken behavior to begin with.

The manual page is also updated to reflect this change.

Minimally tested on Gentoo using a custom patch and the live ebuild (so, tested by applying this on top of the master branch and building the result), because I couldn't get autoconf to behave for some reason.

yoe commented 5 years ago

On Fri, Nov 09, 2018 at 07:22:31AM -0800, Austin S. Hemmelgarn wrote:

Currently, nbd-server will open a listening TCP socket even if it's been asked to use a UNIX domain socket. This is inconsistent with typical behavior of applications which can use UNIX sockets, and may present a security risk on some systems.

This commit modifies this behavior so that use of a UNIX socket and other types of socket is mutually exclusive. You can only listen on a regular (TCP or SDP) socket or a UNIX socket, but not both, with the presence of the unixsock option taking precedence over any other listener configuration.

This has a small potential to break some existing installations, though they are arguably depending on broken behavior to begin with.

No. I can agree that if unix domain sockets are live, by default we probably don't want to do TCP. However, if someone explicitly configures both unix and TCP listeners, it seems wrong to disable the TCP listeners.

The manual page is also updated to reflect this change.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Minimally tested on Gentoo using a custom patch and the live ebuild (so, tested by applying this on top of the master branch and building the result), because I couldn't get autoconf to behave for some reason.

The test suite tests the unix domain sockets, and it doesn't fail, so fine by me :-)

What is the autoconf issue you're fighting with? If it's something in the nbd source, I want to fix it ;-)

-- To the thief who stole my anti-depressants: I hope you're happy

-- seen somewhere on the Internet on a photo of a billboard

Ferroin commented 5 years ago

No. I can agree that if unix domain sockets are live, by default we probably don't want to do TCP. However, if someone explicitly configures both unix and TCP listeners, it seems wrong to disable the TCP listeners.

I'll look at updating it to just use what is explicitly configured then, though it may take a while (I'm still not certain that I understand what all of the config code is doing).

What is the autoconf issue you're fighting with? If it's something in the nbd source, I want to fix it ;-)

I'm pretty sure the problem is me, I somehow completely missed the autogen.sh script and tried to run autoconf manually as a result. Actually using the autogen.sh script works correctly.

yoe commented 5 years ago

No. I can agree that if unix domain sockets are live, by default we probably don't want to do TCP. However, if someone explicitly configures both unix and TCP listeners, it seems wrong to disable the TCP listeners.

I'll look at updating it to just use what is explicitly configured then, though it may take a while (I'm still not certain that I understand what all of the config code is doing).

If you have any questions, shoot (although the mailinglist might be more appropriate -- https://lists.debian.org/nbd)

Ferroin commented 5 years ago

Updated with a new config option to listen on both TCP/SDP and UDS. Sorry it took so long (I've got very little experience debugging C code, so most of the time was spent fighting with GDB trying to figure out why things weren't working as I expected them to).

The commit contains an update to the DocBook version of the manpages, but I wasn't sure how to translate that to the pre-rendered copy. I'll be happy to update the docs the rest of the way if I could get some guidance on how to do so (I'm pretty sure I have all the tools, I just don't know for certain how to do it).

Ferroin commented 5 years ago

OK, this is weird, tests are failing only on macOS built with Clang...

I'm not seeing from the Travis logs what's wrong, and I don't have a macOS system I can test on, any advice @yoe?

yoe commented 5 years ago

The travis builds sometimes fail for state-of-the-moon reasons. I don't know why, really, but often if you just restart it, it works.

I've just retried the build, we'll see what the result is of that.

Ferroin commented 5 years ago

Looks like it all passed this time. Unless you've got any further comments on the code, i think this is probably ready to merge.