calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.46k stars 235 forks source link

Drop privileges timeout conflicts with service socket #862

Open Xaekai opened 4 years ago

Xaekai commented 4 years ago

The timeout in the config defaults to 30 giving the server admin the implication that the value is provided in seconds. However we are in Javascript land here. Time is milliseconds.

Creating and binding a service socket apparently requires root privs? Not sure, I will have to do solo small case test examples, but either way these two systems fight each other with the given defaults.

Solutions: Either of these: 1.) Surface the fact timeout should be provided in ms to the user in the comments of the template 2.) Presume the admin provided the timeout in seconds based on simple sanity math and multiple it by 1000 for them.

Determine everything the server does that requires privs. Use event emitters to emit announcement frame when each has bound the resources they need. Have a listener and when all are fulfilled or errored, drop privs and eitoar discard the timeout or use it as a fallback

calzoneman commented 4 years ago

The timeout in the config defaults to 30

I see 15 in the template, not sure where you copied 30 from. Regardless I guess the specific number doesn't really detract from the point.

1.) Surface the fact timeout should be provided in ms to the user in the comments of the template

It looks like this is documented:

https://github.com/calzoneman/sync/blob/3.0/config.template.yaml#L197-L198

2.) Presume the admin provided the timeout in seconds based on simple sanity math and multiple it by 1000 for them.

This would be a backwards-incompatible change. We would need to change the name of the config key and migrate old configs or something similar.

I agree that the timeout approach is sort of a hack that was added by the original contributor of the PR and it would be better to explicitly signal that the appropriate sockets have been bound and then drop root.

The whole problem can also be avoided entirely by just running the service as a non-root user and either setcaping node to allow binding low ports, adding a few iptables rules to forward 443 to 8443 on loopback, or throwing nginx in front for HTTPS and using a high port for the socket connection.

calzoneman commented 4 years ago

Creating and binding a service socket apparently requires root privs?

Looks like the servsock code uses the filename from config.yaml to delete (if exists), create, and bind the socket. If that file ends up owned by root (e.g. because the servsock is initialized before dropping privs), that's probably what breaks it. I've never had an issue with non-root users not being able to bind that socket.

Xaekai commented 4 years ago

Maybe the 30ms was just enough to cause race condition between root and non-root state when the service socket code was initiated.