unix socket support #378

cyberb · 2023-11-16T11:05:29Z

Allows using unix socket on both ends:

SYNCV3_BINDADDR=/var/snap/matrix/current/sliding-sync.socket
SYNCV3_SERVER=/var/snap/matrix/current/matrix.socket

Tested in Syncloud (https://github.com/syncloud/matrix) which allows me to use Element X now.

Pull Request Checklist

Pull request includes a sign off

Signed-off-by: Boris Rybalkin <ribalkin@gmail.com>

kegsay

Overall I'm happy with the idea, but there's some extra bits to do to get it over the line:

Factor out the unix socket detection to a helper function (aids readability and allows us to be more intelligent easily in the future)
Mention this new support in the docs (README and the big help message in ./cmd/syncv3/main.go)
Run E2E tests with unix sockets so we can make sure this works and won't bitrot. I'm also particularly concerned about size limits given sync v2 responses can be huge e.g https://stackoverflow.com/questions/21856517/whats-the-practical-limit-on-the-size-of-single-packet-transmitted-over-domain

Thanks for your contribution!

sync2/client.go

v3.go

cyberb · 2023-11-16T14:19:01Z

e2e test use real synapse server which I am not sure even supports unix sockets (dendrite does), I will need to check if there is an env, but even if there is do you want to run all e2e tests on each type (http and unix)?
I would just add one test to check that unix socket client can make requests.

kegsay · 2023-11-16T17:21:12Z

Hmm, good point. Synapse does support unix sockets via SYNAPSE_USE_UNIX_SOCKET=1 in the synapse service image we're using, but I'm not sure you can talk unix sockets to the GHA service container. Running a single test will do, though it isn't super ideal as I'm interested in testing large packet sizes / load.

Let me know how much work this will be, and if it's not really feasible we'll think of something else.

kegsay

I don't think HomeServerUrl makes things much clearer here, and it adds boilerplate where it is not necessary. Please:

revert it back and just add internal.IsUnixSocket(u string) bool and call that in places where you previously had strings.HasPrefix. That is just as easily unit testable should you wish to do so, but please do not add testify for basic equality checks.
add a function to map a unix socket string to a unix transport: internal.UnixTransport(u string) http.Transport
tie the two functions together in newClient:

func newClient(timeout time.Duration, destinationURL string) *http.Client {
    transport := http.DefaultTransport
    if internal.IsUnixSocket(destinationURL) (
        transport = internal.UnixTransport(destinationURL)
    }
    return &http.Client{
        Timeout: timeout,
        Transport: transport,
    }
}

kegsay · 2023-11-16T17:22:07Z

internal/errors.go

 	if !expr {
 		GetSentryHubFromContextOrDefault(ctx).CaptureException(fmt.Errorf("assertion failed: %s", msg))
 	}
 }

-func assert(msg string, expr bool) {
+func assertCustom(msg string, expr bool) {


Why the rename?

Please revert.

kegsay · 2023-11-16T17:22:57Z

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/pressly/goose/v3 v3.14.0
 	github.com/prometheus/client_golang v1.13.0
 	github.com/rs/zerolog v1.29.0
+	github.com/stretchr/testify v1.8.4


Please don't add an entire test package just for basic equality checks.

go mod tidy please.

internal/home_server_url.go

kegsay · 2023-11-16T17:34:52Z

v3.go

+	if err != nil {
+		logger.Fatal().Err(err).Msg("failed to serve unix socket")
+	}
+	err = os.Chmod(bindAddr, 0755)


Please document the respective r/w/x permissions you wish to give to this socket as a comment.

kegsay

Thanks, just a few final minor things:

go mod tidy to remove the testify dep.
Please document the respective r/w/x permissions you wish to give to this socket as a comment.
Revert the change from assert to assertCustom.

Then LGTM, thanks!

cyberb · 2023-11-17T18:16:27Z

Thank you!
Probably this issue can be closed as well: #216

emilylange · 2023-11-20T19:14:56Z

v3.go

+	// TODO: safe default for now (rwxr-xr-x), could be extracted as env variable if needed
+	err = os.Chmod(bindAddr, 0755)


Not sure what is meant by "safe default" here.
Unix sockets don't need the write permissions.

To connect to and use a unix socket as a client, all you need is the read permissions bit.

0755, which translates to u=rwx,go=rw, renders this unix socket world readable, thus making it world-connectable.

I was under impression that 755 will only allow the owner to use it.
After googling looks like eXecute is not need but read and write are needed to use it.
Ideally we pass it as a parameter, if not then probably 600 is what I wanted.

According to unix(7), write permission is required to connect to a unix domain socket.

On Linux, connecting to a stream socket object requires write permission on that socket; sending a datagram to a datagram socket likewise requires write permission on that socket.

I'd suggest to make it at least user and group connectable, that is 0660 (execution bit has no effect).

First and foremost: Sorry for my original comment.
I was in a bit of a rush when I wrote it, and should just have waited a bit longer before commenting.

Multiple /bit flips/ happened in my original comment, which I want to point out:

write perms are needed to connect and talk over a unix socket, not read, as @n3vu0r correctly pointed out.

0755 does not translate to u=rwx,go=rw but rather u=rwx,go=rx.

Which in turn makes, as intended, the socket not world-connectable but rather only by the owner itself.

I just was very happy to see unix socket support implemented so soon, but then saw some unusual permission bits.

Given I worked on unix socket permission bits as part of caddyserver/caddy#4741, I figured I should comment. Which then lead to that hastily, and more importantly, incorrect comment. Sorry for that.

According to the current unix(7), only write perms are needed.
There are, however, still a lot of old unix(7) versions hosted on the web, which still mention read and write being needed.

For more details on this, see caddyserver/caddy#4741 (comment).

I'd suggest to make it at least user and group connectable, that is 0660 (execution bit has no effect).

I would suggest the same, but without read, so 0220.

unix socket support

ae73ace

Signed-off-by: Boris Rybalkin <ribalkin@gmail.com>

kegsay requested changes Nov 16, 2023

View reviewed changes

sync2/client.go Outdated Show resolved Hide resolved

sync2/client.go Outdated Show resolved Hide resolved

v3.go Outdated Show resolved Hide resolved

cyberb added 3 commits November 16, 2023 13:57

extract HomeServerUrl struct

b055d27

fix docs

d04a0a7

fix more docs

4c858fe

kegsay requested changes Nov 16, 2023

View reviewed changes

back to util functions

8d38785

kegsay reviewed Nov 17, 2023

View reviewed changes

cyberb added 4 commits November 17, 2023 12:12

go mod tidy

2f2501e

revert assertCustom

c1dca54

chmod comment

06fcbb1

fix one more IsUnixSocket check

bf477c1

kegsay approved these changes Nov 17, 2023

View reviewed changes

kegsay merged commit 62d3798 into matrix-org:main Nov 17, 2023
7 checks passed

emilylange reviewed Nov 20, 2023

View reviewed changes

DMRobertson mentioned this pull request Dec 4, 2023

SYNCV3_BINDADDR variable for unix socket #383

Closed

nisbet-hubbard mentioned this pull request Dec 9, 2023

Fix unix socket permissions #385

Merged

spantaleev mentioned this pull request Dec 13, 2023

Add SYNCV3_SERVER_PUB environment variable pointing to the public homeserver URL #386

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix socket support #378

unix socket support #378

cyberb commented Nov 16, 2023

kegsay left a comment •

edited

Loading

cyberb commented Nov 16, 2023

kegsay commented Nov 16, 2023

kegsay left a comment

kegsay Nov 16, 2023

kegsay Nov 17, 2023

kegsay Nov 16, 2023

kegsay Nov 17, 2023

kegsay Nov 16, 2023

kegsay left a comment

cyberb commented Nov 17, 2023

emilylange Nov 20, 2023

cyberb Nov 20, 2023 •

edited

Loading

n3vu0r Nov 20, 2023

emilylange Nov 21, 2023

		// TODO: safe default for now (rwxr-xr-x), could be extracted as env variable if needed
		err = os.Chmod(bindAddr, 0755)

unix socket support #378

unix socket support #378

Conversation

cyberb commented Nov 16, 2023

Pull Request Checklist

kegsay left a comment • edited Loading

Choose a reason for hiding this comment

cyberb commented Nov 16, 2023

kegsay commented Nov 16, 2023

kegsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

cyberb commented Nov 17, 2023

Choose a reason for hiding this comment

cyberb Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment •

edited

Loading

cyberb Nov 20, 2023 •

edited

Loading