WIP: rc / rcd over unix socket

devnode · October 20, 2022, 3:48am

Hey,

I'm currently working on an rclone integration that would benefit from having the ability to interact with the rcd over unix socket. This would simplify things by removing the need for HTTP authentication, removing the need for CORS and it would also enhance security in multi-user environments by not listening on localhost. I made a quick implementation to get an idea of how much work would be involved but wanted to start a conversation here to get some feedback before spending more time on it:

$ rm -f /tmp/rclone.sock && go run rclone.go rcd --rc-net=unix --rc-addr=/tmp/rclone.sock
2022/10/19 23:26:27 NOTICE: Serving remote control on unix:/tmp/rclone.sock

$ go run rclone.go rc --unix=/tmp/rclone.sock core/pid
{
        "pid": 3227748
}

Known issues:

There's still an issue with detecting SIGINT and performing cleanup of the socket on shutdown.
It does make changes to the shared httplib code so additional testing would be required to ensure all code importing it would also support unix sockets.
Add additional check to prevent Open the files in the browser from returning an error in rcserver.go

Feedback required:

OS Compatibility: I think it's fine just to make a note of it in the documentation as it would be an opt in feature. If someone did happen to enable it on an unsupported system, net.Listen would ultimately throw an error.
Do we refactor to reuse the unix socket code already implemented in cmd/serve/docker/unix.go?
Where to store the socket? My first thought was to keep it simple, just piggyback off the default config dir and put it next to rclone.conf.
Then ideally, we would add a few global config options / flags / env vars so you'd configure it once and not have to pass flags to every rc / rcd command call:
1. Enable the functionality: rcd_unix = true
2. Change the default socket path: rcd_unix_path = /new/path/to/rclone.sock
3. Maybe add the other rc fields and call it a quality of life improvement? lol

Cheers,
Tom

Ole · October 20, 2022, 6:48am

Hey Tom,

Interesting, do you (plan to) support Windows 10+ too?

If so, then I suggest the flag (--unix) and description (path to unix socket) are worded more generally e.g. --socket and "path to socket".

I don't understand the need for CORS when serving and requesting from the same system/user, but that may well be my lack of in-depth CORS knowledge.

devnode · October 20, 2022, 10:40am

It should work with Windows 10+ since it's just using the Go standard library which added support back in 2018.

As for the name of the flag, I simply added another one as it was faster than reworking the logic for --url. But ultimately, I think not adding an additional flag would be the way to go since there'd be no confusion on what takes precedence --url or --unix / --socket and you can easily swap one config value / environment variable to go from local socket to remote host:

--url :1234
--url 127.0.0.1:1234
--url http://domain.tld
--url https://domain.tld
--url /path/to/rclone.sock
--url C:\Users\tom\rclone.sock

And for CORS, yea it's not needed with a unix socket, I just worded it badly, haha. Definitely still want it if you're listening on localhost with javscript enabled in your browser!

-Tom

Ole · October 20, 2022, 12:45pm

Thanks, sounds great to me!

I don't have enough knowledge to answer your issues and well thought questions, so I will leave them open until somebody more knowledgeable drops by.

ncw · October 20, 2022, 5:18pm

Rclone runs rclone serve restic over a stdin/stdout pipe. This runs http2 over that pipe so some code might be re-usable from there.

Your patch looks much less scary than I thought it would be!

I suggest open a new issue on Github with the above in it then we can work on a PR.

lib/atexit is the solution here

Note that there is actually two bits to httplib (refactor that never got finished grrr) so make sure both bits get done!

Fine.

I'd say probably not as that code is quite specialized to docker stuff. But then I don't know that code very well.

Isn't the socket location something the user provides? Or we make a default location in a flag?

You can already pass every flag as an env var - would this do?

devnode · October 21, 2022, 8:01am

yea, it's really not that much to get it working, it's just a matter of the smaller details that i want to get right.

after thinking about it some more, this is what i want to implement:

$HOME/.config/rclone/rclone.conf
$HOME/.config/rclone/rcd.conf
$HOME/.config/rclone/rcd.sock

this would allow for an easier guided setup of rcd, similar to configuring a remote. just generate a simple config, place it in a well known directory, don't have to modify any .bashrc or .profile files on behalf of the user and maybe even use a premade systemd unit.

then rclone rcd and rclone rc would check if rcd.conf exists, if it does, parse it so that:

flags > env vars > rcd.conf > hard coded defaults

this way, it's fully opt in. then i could add something like rclone rcinfo to display all the connection details in a nice json format and maybe even do a healthcheck on rcd to see if it's running. it'd be one command to gather all the state required to interact with rcd and end users wouldn't have to maintain two copies of connection details, one via flags / env vars and one in the gui.

also, down the road, if there's interest, it could be improved further:

$ vim $HOME/.config/rclone/rcd.conf
// quickly enable unix socket on localhost
network = unix

[server1]
url = https://1.2.3.4:7752
user = me
pass = 1234

// connect to remote url over an ssh tunnel
[server2]
ssh_host = server2.domain.tld
ssh_port = 22
ssh_user = me
url = /home/me/.config/rclone/rcd.sock

// now it's only one flag, not --url, --user and --pass
rclone rc --host server1 core/pid

// or write a wrapper over rc
rclone rce server2 core/pid

anyways, i'll create an issue as suggested in the near future so we can get the ball rolling on this.

ncw · October 21, 2022, 3:03pm

I don't want to make another config file...

I'd rather use the profiles idea to put config into the existing config file: Storing defaults for global flags in the config file (profiles) · Issue #2697 · rclone/rclone · GitHub

However placing the socket in $HOME/.config/rclone/rcd.sock sounds like a sensible choice. Or possibly in ~/.cache - not an LSB expert!

Note that people quite often run several copies of rclone so we can't be to magic about it.

Grand.

Lets do the smallest possible change for the socket stuff then see how that works out!

devnode · November 3, 2022, 4:39pm

well it's not exactly "the smallest possible change" but i think this is make things better moving forward. also, i can break it up into smaller sections, per command or whatever, just let me know. i got some more contract work to take care of but i'll check back in around sunday.

httplib is no more, lib/http takes its place.
updated rc, rcd, serve http, serve restic & serve webdav
swapped deprecated ioutil functions for their new os / io versions
then some general housekeeping, nothing too drastic, just more about consistency between commands

Multiple listeners, unix w/ auth & cors disabled, http and https

go run rclone.go rcd \
    --rc-addr /tmp/rclone.sock \
    --rc-addr 127.0.0.1:1234 \
    --rc-addr tls://127.0.0.1:1235 \
    --rc-user admin \
    --rc-pass admin \
    --rc-cert ~/.certs/local.crt \
    --rc-key ~/.certs/local.key

Single TLS listener, no prefix required:

go run rclone.go rcd \
    --rc-addr 127.0.0.1:1235 \
    --rc-user admin \
    --rc-pass admin \
    --rc-cert ~/.certs/local.crt \
    --rc-key ~/.certs/local.key

rc with unix socket, alternative flag

go run rclone.go rc \
    --rc-addr /tmp/rclone.sock \
    operations/list fs=testremote1: remote=data

also works fine

go run rclone.go serve http testremote1:data \
    --addr /tmp/rclone.sock \
    --addr 127.0.0.1:1234 \
    --addr tls://127.0.0.1:1235 \
    --user admin \
    --pass admin \
    --cert ~/.certs/local.crt \
    --key ~/.certs/local.key

Tests passing:

go test -v ./cmd/serve/http/...
go test -v ./cmd/serve/webdav/...
go test -v ./fs/rc/...
go test -v ./lib/http/...

Passes but skips restic source outside of repo:

#   restic_test.go:28: Skipping test as restic source not found: stat ../../../../../restic/restic: no such file or directory
go test -v ./cmd/serve/restic/...
cd cmd/serve/restic && ./restic-test.sh

Few TODOs:

- compare help text and flags, before and after, probably just script it
- TLS test for certificate auth / mutual TLS
- figure out that skipped restic test
- double check libhttp.Shutdown logic, may have duplicated it on rcd

ncw · November 3, 2022, 4:48pm

Wow, nice - well done! That's a bit piece of work.

Nice demo!

Breaking it up would be helpful, but TBH this is going to be very difficult to review, so don't sweat it too much.

Commits like this would be ideal

one per command would be nice as that will help with bisect. If not it is no big deal.
one removing httplib at the end!
ioutil tidy up
houskeeping (maybe more commits).

What I'll probably do is skim the code looking for oddities, make sure the tests pass then merge. We are at the start of the dev cycle so now is a good time to merge code which might break stuff.

Great work

albertony · November 3, 2022, 4:58pm

I already have a PR for the ioutil part, I assume the changes are very similar: Replace deprecated ioutil by albertony · Pull Request #6389 · rclone/rclone · GitHub

devnode · November 4, 2022, 11:16pm

it was a bit more than i intended to write but i wanted to make sure that it would fit well with the rest of the code, so down the rabbit hole i went, haha...

sounds good. i believe there's a test i wrote for BaseURL that needs adjusting and a few more minor tweaks i wanted to make after replying but the rest of it will probably be just minor naming things and other oddities. i tried to be cognizant of the current style but as the day dragged on, i shifted more into "just making it work" mode.

anyways, i'll break it out into the following PRs so it's easier deal with:

lib/http & cmd/serve/http since they're linked
rc & rcd
cmd/serve/restic
cmd/serve/webdav

yea, let's get that merged first since it's way more inclusive. ioutil was deprecated in 1.16 and the go.mod is already set to 1.17.

ncw · November 7, 2022, 11:34am

Sounds perfect.

Great.Will finish revewing @albertony 's PR then get to work on @devnode 's !

system · January 6, 2023, 11:34am

This topic was automatically closed 60 days after the last reply. New replies are no longer allowed.