unix socket support

[cyberb]

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

[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!

[cyberb]

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]

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]

Why the rename?

[kegsay]

Please revert.

[kegsay]

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

[kegsay]

go mod tidy please.

[kegsay]

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]

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

[emilylange]

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.

[cyberb]

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.

[n3vu0r]

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).

[emilylange]

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:

1. write perms are needed to connect and talk over a unix socket, not read, as @n3vu0r correctly pointed out.
2. 0755 does not translate to u=rwx,go=rw but rather u=rwx,go=rx.
3. 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.