Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Unix sockets #1552

Merged
merged 1 commit into from
Jan 3, 2025
Merged

Conversation

fpagliughi
Copy link
Contributor

Adds support for UNIX-domain sockets on *nix platforms.
It implements issue #864.

This allows clients to communicate with an MQTT server over UNIX-domain sockets on *nix systems. (Not on Windows). This is opt-in with a CMake build option, PAHO_WITH_UNIX_SOCKETS.

This can increase performance and reduce latency since UNIX sockets have a much simpler network stack than TCP. But perhaps more importantly, it provides increased security with more fine-grained control when clients reside on the same host as the broker. Access to the broker can be done using file permissions (with groups, owners, etc).

The clients can request a socket of this type using a URI of unix:///path/to/socket/file. The socket file is created by the server when it binds and listens on the UNIX socket.

The implementation here is fairly trivial. The library must now recognize the unix:// scheme in the URI, then create and connect to a socket using a sockaddr_un socket address containing the path to the socket file.

After that all communications and closing the socket are handled by the same code that manages TCP connections. A socket is a socket.

This was tested on Linux and macOS with the mosquitto server. By definition it requires a broker running on the same host whereby it communicates with the clients using the local file system.

The mosquitto configuration file can be as simple as:

allow_anonymous true    
listener 0 /tmp/mosquitto.sock

This PR also updates several of the simple sample apps to take a URI at the command line, allowing them to be used to try this out, like:

MQTTAsync_publish unix:///tmp/mosquitto.sock

Copy link

@vjardin vjardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk @fpagliughi ; it'll be a nice security and performance improvement. I took the liberty to review your commits.

src/MQTTAsyncUtils.h Outdated Show resolved Hide resolved
src/MQTTClient.c Show resolved Hide resolved
src/MQTTProtocolOut.c Outdated Show resolved Hide resolved
src/MQTTProtocolOut.c Outdated Show resolved Hide resolved
@@ -329,6 +329,9 @@ int MQTTAsync_createWithOptions(MQTTAsync* handle, const char* serverURI, const
{
if (strncmp(URI_TCP, serverURI, strlen(URI_TCP)) != 0
&& strncmp(URI_MQTT, serverURI, strlen(URI_MQTT)) != 0
#if defined(UNIXSOCK)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should avoid such if defined() in order to avoid code complexity. The error cases can be handled for WIN32 or non UNIXSOCK into the function Socket_unix_new() that can be available for all the cases, but that should return a runtime ERROR when Unixsock is not available.

So the code will be easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree here. The conditional compilation ensures that unix:// is reported as an unsupported protocol if support is not compiled into the library - just the same way it's done with ssl://, mqtts://, and wss://.

@@ -34,7 +34,10 @@ int main(int argc, char* argv[])
MQTTClient_deliveryToken token;
int rc;

if ((rc = MQTTClient_create(&client, ADDRESS, CLIENTID,
const char* uri = (argc > 1) ? argv[1] : ADDRESS;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ident issue.

src/samples/MQTTClient_publish_async.c Outdated Show resolved Hide resolved
@@ -137,7 +137,10 @@ int main(int argc, char* argv[])
int rc;
int ch;

if ((rc = MQTTAsync_create(&client, ADDRESS, CLIENTID, MQTTCLIENT_PERSISTENCE_NONE, NULL))
const char* uri = (argc > 1) ? argv[1] : ADDRESS;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ident issue

src/samples/MQTTAsync_publish.c Show resolved Hide resolved
Comment on lines +1284 to +1291
if (1)
{
int optsend = 100; //2 * 1440;
printf("Setting optsend to %d\n", optsend);
if (setsockopt(*sock, SOL_SOCKET, SO_SNDBUF, (void*)&optsend, sizeof(optsend)) != 0)
Log(LOG_ERROR, -1, "Could not set SO_SNDBUF for socket %d", *sock);
}
#endif
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are not changing any code -> ident issue. The diff should be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree in the sense that the previous indenting was incorrect and misleading. Admittedly, this has nothing to do with the rest of the PR; it's a holdover from my initial attempt to modify Socket_new() for UNIX sockets, before deciding that adding a separate Socket_unix_new() would be more clear.

I'd prefer to keep it.

@fpagliughi
Copy link
Contributor Author

@vjardin Thanks so much for the review! So nice to have this quick feedback. I made some changes, and added my reasoning for not making other changes.

@vjardin
Copy link

vjardin commented Jan 2, 2025

OK, LGTM.

Regaarding NOSIGPIPE, if it is not defined, it means that you should handle the SIGPIPE, I did not check if it is properly done. But in 2025, it should not be a topic !

@fpagliughi
Copy link
Contributor Author

For years I made the argument to implement UNIX sockets for MQTT to reduce latency in local systems, but that never seemed to sway people. The security argument gets people's attention.

But I just ran my first test, compiling this branch into the Paho Rust library and using it on an MQTT v5 RPC system that I have for one of my customers. Each service exports a ping function to check that it's alive and responsive. The full round-trip is four publishes:

client [request] -> broker -> service [response] -> broker -> client

I just tried it to compare localhost TCP to Unix sockets:

$ svcctl ping  -U tcp://localhost:1883                                   
Service responded in 43.247ms

$ svcctl ping -U unix:///tmp/mosquitto.sock
Service responded in 2.530ms

Using TCP takes 43ms... using UNIX sockets takes 2.5ms.

That's a 94% reduction in round-trip latency!!!

@icraggs : I need this mergerd and released ASAP!!! :-)

@vjardin
Copy link

vjardin commented Jan 2, 2025

Same, this requirement of unix socket is for both security and performances.

@icraggs
Copy link
Contributor

icraggs commented Jan 2, 2025

Great! Does this change the external interfaces - as in could we have it in the next 1.3.n release?

I'm close to getting a 1.3.14 release out (I'm doing a proxy issue at the moment ) @fpagliughi could you take a look at this build issue to help me out? #1376

@fpagliughi
Copy link
Contributor Author

Hey @icraggs, no this doesn't change the external interfaces. When built in, it just allows the library to recognize a new URI scheme ("unix://") and handle it accordingly. I would love to get it into v1.3.x

I will squash the commits in this PR to make it easy to apply to both branches, and run some tests myself to merge and build in the develop branch.

@fpagliughi
Copy link
Contributor Author

Later today I can get a Windows machine running and test #1376

@fpagliughi
Copy link
Contributor Author

OK, I squashed this PR and then tried to cherry-pick the resulting commit into the develop branch. It didn't merge cleanly due to the choose-an-interface code from v1.4 in Socket.c

So I cleaned up the merge and created another pull request, #1553.

So both PRs can be landed into their target branches... this one into v1.4 and the other into develop.

@icraggs icraggs added this to the 1.3.14 milestone Jan 3, 2025
@icraggs
Copy link
Contributor

icraggs commented Jan 3, 2025

I use the paho_c_pub and paho_c_sub samples a lot. Their interface is similar to mosquitto_pub and sub. You can try the unix sockets without any changes, for instance:

build.paho/src/samples/paho_c_pub x --connection unix:///tmp/mosquitto.sock --trace protocol

@icraggs icraggs merged commit e470865 into eclipse-paho:1.4 Jan 3, 2025
5 checks passed
@fpagliughi fpagliughi deleted the unix-sockets-new branch January 6, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants