From 3b402955c425a6dbf111cdad3ed54d20a2171f3b Mon Sep 17 00:00:00 2001 From: Oluwatobi Ogundimu Date: Sat, 1 Aug 2026 11:20:44 +0100 Subject: [PATCH] SC-17: wire rsync:// daemon URLs into the main sync command rsync://host/module is now a real destination for grsync SRC... DEST, via new daemon.DialClient wired into the same pipeline.Sender every destination uses. Credentials verified against real rsync's actual behavior: username from URL/USER/LOGNAME/nobody, password from --password-file/RSYNC_PASSWORD/interactive prompt - no --password flag, matching rsync's deliberate omission to avoid ps-visibility exposure. Password resolution is lazy, only triggered on AUTHREQD, proven by test. Two bugs found: ParseRemotePath silently mis-parsed rsync://host/module as SSH syntax (host=rsync) - fixed, rejecting any "://" with a regression test. DialGreeting/DialAuth/DialModule were built around an unexported type, unreachable despite being exported - fixed via DialClient. Scope gaps (documented in README, not separate tickets): sub-path syncing within a module, pulling from an rsync:// source, max connections enforcement. Clean on native Windows and cross-compiled Linux. --- README.md | 108 +++++++++-- go.mod | 2 + go.sum | 4 + internal/cli/credentials.go | 121 +++++++++++++ internal/cli/passwordfile_unix.go | 24 +++ internal/cli/passwordfile_windows.go | 13 ++ internal/cli/root.go | 45 +++-- internal/cli/rsync_url.go | 50 ++++++ internal/cli/rsync_url_test.go | 250 ++++++++++++++++++++++++++ internal/cli/sync.go | 59 +++++- internal/daemon/auth.go | 31 +++- internal/daemon/auth_test.go | 12 +- internal/daemon/client.go | 37 ++++ internal/daemon/client_test.go | 126 +++++++++++++ internal/daemon/server_test.go | 6 +- internal/transport/remotepath.go | 9 +- internal/transport/remotepath_test.go | 2 + 17 files changed, 839 insertions(+), 60 deletions(-) create mode 100644 internal/cli/credentials.go create mode 100644 internal/cli/passwordfile_unix.go create mode 100644 internal/cli/passwordfile_windows.go create mode 100644 internal/cli/rsync_url.go create mode 100644 internal/cli/rsync_url_test.go create mode 100644 internal/daemon/client.go create mode 100644 internal/daemon/client_test.go diff --git a/README.md b/README.md index 6d673c6..fe75adc 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,11 @@ special files) - this is real, working sync, not yet full feature parity. protocol - `rsyncd.conf` parsing, the `@RSYNCD` greeting/handshake, module listing, and real MD4 challenge-response authentication all match upstream rsync, verified against its actual source rather than assumed. -See [rsync Daemon Mode](#rsync-daemon-mode) below for exactly what that +**`rsync://host/module` is now a real destination argument to the main +`grsync SRC... DEST` command itself**, not just something exercised from +`internal/daemon`'s own tests - `grsync -a src rsync://host/module` works +today, credentials and all. See +[rsync Daemon Mode](#rsync-daemon-mode) below for exactly what that covers and where it hands off to grsync's own (non-rsync-wire-format) transfer protocol. @@ -68,6 +72,7 @@ argument is always the destination. | `--daemon` | | run as an rsync-protocol daemon, serving modules from `--config` (see [rsync Daemon Mode](#rsync-daemon-mode)) | | `--config PATH` | | path to the `rsyncd.conf` to serve (required with `--daemon`) | | `--port PORT` | | TCP port to listen on in `--daemon` mode (default `873`, matching rsync) | +| `--password-file FILE` | | read an `rsync://` daemon password from FILE instead of `RSYNC_PASSWORD` or an interactive prompt (see [rsync Daemon Mode](#rsync-daemon-mode)) | All five filter-related flags share one ordered rule list - their relative order on the command line is preserved, matching rsync's first-match-wins @@ -352,12 +357,58 @@ recognized parameter with a malformed value, is a hard parse error. `daemon.ParseURL` parses `rsync://[user@]host[:port]/module[/path]`, including the bare `rsync://host` and `rsync://host/` forms real rsync uses to mean "list this daemon's modules" rather than selecting one, and -IPv6 literals. This parser is implemented and tested, but **not yet wired -into the main `grsync SRC... DEST` sync command** - today it's only used -internally (and by `internal/daemon`'s own tests) to build a connection by -hand. Making `rsync://...` a valid source/destination argument the same -way an SSH `user@host:path` already is would be a natural, low-risk -follow-up. +IPv6 literals. + +**`rsync://host/module` is a real destination argument to the main +`grsync SRC... DEST` command**: `grsync -a src rsync://host/module` +dials the daemon over plain TCP, runs the real handshake/auth below, then +uploads through the same `pipeline.Sender` every other destination uses. +`internal/cli`'s `isRsyncURL` distinguishes this from a local path or an +SSH `user@host:path` by its `rsync://` prefix, checked before either of +those is - `transport.ParseRemotePath` itself now also refuses anything +containing `"://"`, so an `rsync://` URL is never valid SSH syntax by +construction (a real ambiguity found and fixed while wiring this up: it +used to parse as host `"rsync"`, path `"//host/module"`). + +Only pushing to a module (`grsync SRC rsync://host/module`) is supported, +matching the existing SSH-transport restriction rather than introducing a +new asymmetry - pulling *from* an `rsync://` source is rejected with a +clear error, the same as pulling from an SSH source already is. A +destination URL also can't yet target a sub-path within a module +(`rsync://host/module/subdir`) - the daemon protocol itself (see below) +only supports syncing an entire module, and that's explicitly out of +scope to change here; grsync rejects this case with a clear error rather +than silently ignoring the sub-path. + +**Credentials**: verified against real rsync's actual documented +behavior (`rsync.1`'s "RSYNC_PASSWORD" and "--password-file" sections) +rather than invented. The username is the URL's own `user@` part, else +`USER`, else `LOGNAME` (`USER` wins if both are set), else `"nobody"` - +real rsync's exact resolution order. The password comes from +`--password-file FILE` (or stdin, if `FILE` is `-`) if given, else the +`RSYNC_PASSWORD` environment variable, else an interactive, +non-echoing terminal prompt - also real rsync's exact precedence. +**There is deliberately no `--password` flag**: real rsync has never had +one either, precisely because a password given directly as a +command-line argument is visible to any other user on the same machine +via the process list (`ps`), and an environment variable can leak the +same way on some systems - which is exactly why `--password-file` exists +and why grsync (matching real rsync) refuses a world-readable +`--password-file` (POSIX only; see `checkPasswordFilePermissions`, split +`_unix.go`/`_windows.go` the same way `internal/sync`'s ownership and +hard-link handling already is). + +None of this is resolved eagerly: `daemon.PasswordFunc` is only ever +called if the daemon actually sends `AUTHREQD`, exactly matching real +rsync's own `auth_client()`, which is only ever invoked in response to +that same line. Connecting to a module that turns out not to require +authentication never prompts, never reads `--password-file`, and never +consults `RSYNC_PASSWORD` - `TestDialClient_PasswordFuncNotCalledForAnonymousModule` +in `internal/daemon/client_test.go` proves this directly, not just by +absence of a prompt in a passing test. A multi-source sync against the +same daemon destination resolves (and, if it comes to it, prompts) at +most once for the whole invocation, not once per source, even though +each source still gets its own connection (see `DialClient` below). ### Handshake and authentication @@ -422,6 +473,15 @@ follows is not.** Like the SSH transport (see grsync daemon interoperates with another grsync client, not with a real `rsync` binary, exactly the same boundary that already exists for SSH. +Server and client sides are each a single `net.Conn`-in/`error`-out entry +point: `daemon.ServeConn` (accepted connections, used by `Serve`'s accept +loop) and `daemon.DialClient` (outbound connections, used by +`internal/cli`'s `syncToRsyncDaemon`). `DialClient` exists specifically +because `DialGreeting`/`DialAuth`/`DialModule` are built around this +package's own unexported connection type - before `DialClient`, nothing +outside `internal/daemon` could actually call them, a gap found while +wiring the CLI up to this package rather than one planned in advance. + **Other scope boundaries:** - **`max connections` is parsed but not enforced** - nothing currently caps concurrent connections to a module at that number. @@ -433,19 +493,28 @@ grsync daemon interoperates with another grsync client, not with a real auth response) is capped at 8 KiB, so an unauthenticated client can't force unbounded memory growth by sending data with no newline. -Tested with `TestDaemon_RealTCP_*` in `internal/daemon/server_test.go` -over an actual loopback TCP connection - listen, dial, full handshake, -auth, and transfer - not just in-memory pipes standing in for a +Tested with `TestDaemon_RealTCP_*` and `TestDialClient_*` in +`internal/daemon`, and end to end through the real CLI command with +`TestE2E_LocalToRsyncDaemon_*` in `internal/cli/rsync_url_test.go` - all +over an actual loopback TCP connection (listen, dial, full handshake, +auth, and transfer), not just in-memory pipes standing in for a connection, since (unlike the SSH tests) nothing external is needed to -exercise this end to end. +exercise this end to end. `internal/cli`'s tests drive the real +`NewRootCmd()` command, the same way `TestE2E_LocalToLocal` does for a +local sync, against a real daemon started in-process - not +`internal/pipeline` or `internal/daemon` called directly. ## Architecture - `cmd/grsync` - CLI entrypoint. -- `internal/cli` - flag/argument parsing (built on cobra) and now the - real sync entry point (`sync.go`): local-to-local runs the pipeline - in-process over an `io.Pipe`, local-to-remote spawns and drives it over - an SSH `Session`. +- `internal/cli` - flag/argument parsing (built on cobra) and the real + sync entry point (`sync.go`): local-to-local runs the pipeline + in-process over an `io.Pipe`, local-to-remote (SSH) spawns and drives it + over an SSH `Session`, and local-to-`rsync://` (`rsync_url.go`) dials + the daemon over TCP and drives it through `internal/daemon`'s + `DialClient`; `credentials.go` resolves the username/password for the + latter, matching real rsync's own precedence (see + [rsync Daemon Mode](#rsync-daemon-mode) above). - `internal/pipeline` - wires `internal/sync` and `internal/transport` together into an actual sync; see [End-to-End Sync Pipeline](#end-to-end-sync-pipeline) above. @@ -454,11 +523,12 @@ exercise this end to end. - `internal/transport` - remote endpoint parsing, RSH command construction, frame protocol, subprocess session management, and the `--server` handshake. -- `internal/daemon` - the rsync daemon protocol: `rsyncd.conf` and - `rsync://` URL parsing, the `@RSYNCD` greeting/handshake and module +- `internal/daemon` - the rsync daemon protocol, both sides: `rsyncd.conf` + and `rsync://` URL parsing, the `@RSYNCD` greeting/handshake and module listing, MD4 challenge-response authentication, and per-module access - control, handing off to `internal/pipeline` for the actual transfer. - See [rsync Daemon Mode](#rsync-daemon-mode) above. + control, handing off to `internal/pipeline` for the actual transfer via + `ServeConn` (server) and `DialClient` (client). See + [rsync Daemon Mode](#rsync-daemon-mode) above. Goal: full feature parity with upstream rsync, including protocol/format interoperability where specified (e.g. batch mode's file format). diff --git a/go.mod b/go.mod index b52175b..7b1d487 100644 --- a/go.mod +++ b/go.mod @@ -10,4 +10,6 @@ require ( require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/spf13/pflag v1.0.9 // indirect + golang.org/x/sys v0.47.0 // indirect + golang.org/x/term v0.45.0 // indirect ) diff --git a/go.sum b/go.sum index 96ae128..2040be4 100644 --- a/go.sum +++ b/go.sum @@ -9,4 +9,8 @@ github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/cli/credentials.go b/internal/cli/credentials.go new file mode 100644 index 0000000..35063a5 --- /dev/null +++ b/internal/cli/credentials.go @@ -0,0 +1,121 @@ +package cli + +import ( + "bufio" + "fmt" + "io" + "os" + "sync" + + "golang.org/x/term" + + "github.com/syntaxroot-cc/grsync/internal/daemon" +) + +// resolveUser picks the username to authenticate an rsync:// connection +// as: the URL's own "user@" part if it had one, else the USER environment +// variable, else LOGNAME (USER wins if both are set) - matching real +// rsync's own documented resolution exactly (see rsync.1's "USER or +// LOGNAME" section). An empty result is valid: DialAuth itself defaults +// that to "nobody", the same final fallback real rsync uses. +func resolveUser(urlUser string) string { + if urlUser != "" { + return urlUser + } + if u := os.Getenv("USER"); u != "" { + return u + } + return os.Getenv("LOGNAME") +} + +// resolvePassword returns a daemon.PasswordFunc for an rsync:// daemon +// connection, matching real rsync's own precedence: --password-file (if +// given) beats the RSYNC_PASSWORD environment variable, which beats an +// interactive terminal prompt (see rsync.1's "RSYNC_PASSWORD" and +// "--password-file" sections). The result is memoized with sync.Once, so +// a multi-source sync against the same daemon destination resolves (and, +// if it comes to it, prompts) at most once - not once per source, even +// though each source gets its own connection (see syncToRsyncDaemon). The +// returned func still only actually runs any of this if DialAuth calls +// it, which only happens if the server challenges for a password at all. +func resolvePassword(passwordFile string, stdin io.Reader) daemon.PasswordFunc { + var once sync.Once + var password string + var resolveErr error + + return func() (string, error) { + once.Do(func() { + password, resolveErr = doResolvePassword(passwordFile, stdin) + }) + return password, resolveErr + } +} + +func doResolvePassword(passwordFile string, stdin io.Reader) (string, error) { + if passwordFile != "" { + return readPasswordFile(passwordFile, stdin) + } + if password, ok := os.LookupEnv("RSYNC_PASSWORD"); ok { + return password, nil + } + return promptForPassword(stdin) +} + +// readPasswordFile reads the first line of path as the password, matching +// real rsync's own --password-file behavior exactly: "-" means read from +// stdin instead of a named file, and the file's contents past the first +// line are ignored (real rsync documents this as deliberate, not a +// limitation - it means a trailing newline, or even a second unrelated +// line, is harmless). A named file is checked against +// checkPasswordFilePermissions first; "-" (stdin) is not, since there is +// no file to check permissions on. +func readPasswordFile(path string, stdin io.Reader) (string, error) { + var r io.Reader + if path == "-" { + r = stdin + } else { + if err := checkPasswordFilePermissions(path); err != nil { + return "", err + } + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("opening password file: %w", err) + } + defer func() { _ = f.Close() }() + r = f + } + + scanner := bufio.NewScanner(r) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return "", fmt.Errorf("reading password file: %w", err) + } + return "", fmt.Errorf("password file is empty") + } + return scanner.Text(), nil +} + +// promptForPassword shows an interactive, non-echoing "Password: " prompt +// and reads one line, matching real rsync's own getpass()-based fallback +// when neither --password-file nor RSYNC_PASSWORD is set. If stdin isn't +// an interactive terminal (piped input, a test harness, a non-interactive +// script), this returns an empty password rather than prompting or +// erroring - matching real rsync's own behavior when getpass() can't open +// a controlling terminal (it fails silently and rsync falls back to an +// empty password, which then simply fails authentication cleanly if the +// module actually needs one, rather than hanging forever waiting for +// input that will never come). +func promptForPassword(stdin io.Reader) (string, error) { + f, ok := stdin.(*os.File) + if !ok || !term.IsTerminal(int(f.Fd())) { + return "", nil + } + + fmt.Fprint(os.Stderr, "Password: ") + passwordBytes, err := term.ReadPassword(int(f.Fd())) + fmt.Fprintln(os.Stderr) + if err != nil { + return "", fmt.Errorf("reading password: %w", err) + } + return string(passwordBytes), nil +} diff --git a/internal/cli/passwordfile_unix.go b/internal/cli/passwordfile_unix.go new file mode 100644 index 0000000..0fb57c5 --- /dev/null +++ b/internal/cli/passwordfile_unix.go @@ -0,0 +1,24 @@ +//go:build !windows + +package cli + +import ( + "fmt" + "os" +) + +// checkPasswordFilePermissions refuses a world-readable --password-file, +// matching real rsync's own documented behavior: "Rsync will exit with +// an error if FILE is world readable." Real rsync also refuses a +// non-root-owned file when running as root; that narrower check is not +// implemented here. +func checkPasswordFilePermissions(path string) error { + info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("checking password file: %w", err) + } + if info.Mode().Perm()&0o004 != 0 { + return fmt.Errorf("password file %q must not be world readable", path) + } + return nil +} diff --git a/internal/cli/passwordfile_windows.go b/internal/cli/passwordfile_windows.go new file mode 100644 index 0000000..cba4f75 --- /dev/null +++ b/internal/cli/passwordfile_windows.go @@ -0,0 +1,13 @@ +//go:build windows + +package cli + +// checkPasswordFilePermissions is a no-op on Windows: os.FileInfo.Mode() +// there doesn't expose POSIX world-readable permission bits (Go's Windows +// port only reflects the read-only attribute), so there's nothing +// meaningful to check - the same platform split already established for +// ownership and hard-link handling in internal/sync. See +// passwordfile_unix.go for the real check. +func checkPasswordFilePermissions(_ string) error { + return nil +} diff --git a/internal/cli/root.go b/internal/cli/root.go index e9e9062..5f6010b 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -44,25 +44,26 @@ type FilterRule struct { // in one struct (rather than loose variables) makes it straightforward to // pass a single value into internal/sync once that package exists. type options struct { - archive bool - verbose bool - compress bool - recursive bool - dirs bool - dryRun bool - delete bool - progress bool - perms bool - times bool - owner bool - group bool - links bool - filterRules []FilterRule - rsh string - server bool - daemon bool - config string - port int + archive bool + verbose bool + compress bool + recursive bool + dirs bool + dryRun bool + delete bool + progress bool + perms bool + times bool + owner bool + group bool + links bool + filterRules []FilterRule + rsh string + server bool + daemon bool + config string + port int + passwordFile string } // filterRuleFlag implements pflag.Value. Each of --exclude/--include/ @@ -183,6 +184,12 @@ func NewRootCmd() *cobra.Command { flags.BoolVar(&opts.daemon, "daemon", false, "run as an rsync-protocol daemon, serving modules defined in --config") flags.StringVar(&opts.config, "config", "", "path to the rsyncd.conf file to serve (required with --daemon)") flags.IntVar(&opts.port, "port", daemon.DefaultPort, "TCP port to listen on in --daemon mode") + flags.StringVar(&opts.passwordFile, "password-file", "", + "read the rsync:// daemon password from FILE (or stdin, if FILE is \"-\") instead of the "+ + "RSYNC_PASSWORD environment variable or an interactive prompt; matches real rsync's own "+ + "--password-file, including refusing a world-readable FILE. There is deliberately no "+ + "--password flag: a password given directly as a command-line argument would be visible "+ + "to other users on the same machine via the process list, exactly why real rsync has never had one either") return cmd } diff --git a/internal/cli/rsync_url.go b/internal/cli/rsync_url.go new file mode 100644 index 0000000..a7cc531 --- /dev/null +++ b/internal/cli/rsync_url.go @@ -0,0 +1,50 @@ +package cli + +import ( + "fmt" + "net" + "strconv" + "strings" + "time" + + "github.com/syntaxroot-cc/grsync/internal/daemon" + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +// isRsyncURL reports whether s looks like an rsync:// daemon URL, as +// opposed to a local path or an SSH user@host:path. Checked by prefix +// (rather than by trying daemon.ParseURL and inspecting the error) so a +// malformed rsync:// URL is reported as a clear parse error from +// daemon.ParseURL itself, instead of silently falling through to be +// treated as some other kind of argument. +func isRsyncURL(s string) bool { + return strings.HasPrefix(s, "rsync://") +} + +// dialDaemonTimeout bounds how long connecting to an rsync:// daemon can +// take, so an unreachable or non-responding host fails with a clear error +// instead of hanging the whole command indefinitely. +const dialDaemonTimeout = 10 * time.Second + +// syncToRsyncDaemon uploads src to an rsync:// daemon destination u: +// dials the daemon over plain TCP, then hands the connection straight to +// daemon.DialClient, which runs the real handshake/authentication and +// then the same pipeline.Sender every other destination uses - this +// function's only job is to get from a URL to a net.Conn and supply the +// credentials, not to know anything about the transfer itself. +func syncToRsyncDaemon(src string, u daemon.URL, password daemon.PasswordFunc, walkOpts sync.WalkOptions, rules []sync.Rule) error { + port := u.Port + if port == 0 { + port = daemon.DefaultPort + } + addr := net.JoinHostPort(u.Host, strconv.Itoa(port)) + + nc, err := net.DialTimeout("tcp", addr, dialDaemonTimeout) + if err != nil { + return fmt.Errorf("connecting to %s: %w", addr, err) + } + defer func() { _ = nc.Close() }() + + user := resolveUser(u.User) + return daemon.DialClient(nc, u.Module, user, password, daemon.DirectionPut, src, rules, walkOpts, sync.AttrOptions{}) +} diff --git a/internal/cli/rsync_url_test.go b/internal/cli/rsync_url_test.go new file mode 100644 index 0000000..8dfb002 --- /dev/null +++ b/internal/cli/rsync_url_test.go @@ -0,0 +1,250 @@ +package cli + +import ( + "bytes" + "fmt" + "io" + "net" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/syntaxroot-cc/grsync/internal/daemon" +) + +// startTestDaemon listens on 127.0.0.1:0 (an OS-assigned free port) and +// serves cfg in the background until the test ends - a real TCP listener, +// not a stand-in, so this exercises the same connection code an actual +// `grsync ... rsync://host/module` invocation goes through end to end, +// the same way TestE2E_LocalToLocal drives the real CLI command rather +// than internal/pipeline directly. +func startTestDaemon(t *testing.T, cfg *daemon.Config) (port int, errLog *bytes.Buffer) { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listening on loopback: %v", err) + } + t.Cleanup(func() { _ = ln.Close() }) + + errLog = &bytes.Buffer{} + go func() { _ = daemon.Serve(ln, cfg, errLog) }() + + return ln.Addr().(*net.TCPAddr).Port, errLog +} + +// runGrsync executes the real root command with args, exactly the way a +// user's invocation would, with stdin fixed to an empty, non-terminal +// reader so credential resolution never depends on whether the test +// runner happens to have a real controlling terminal attached. +func runGrsync(t *testing.T, args ...string) error { + t.Helper() + cmd := NewRootCmd() + cmd.SetArgs(args) + cmd.SetOut(io.Discard) + cmd.SetIn(strings.NewReader("")) + return cmd.Execute() +} + +func TestE2E_LocalToRsyncDaemon_Anonymous(t *testing.T) { + modRoot := t.TempDir() + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "incoming": {Name: "incoming", Path: modRoot, ReadOnly: false}, + }} + port, errLog := startTestDaemon(t, cfg) + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "hello.txt"), "pushed to a real rsync:// daemon URL") + mustMkdirAll(t, filepath.Join(src, "sub")) + mustWriteFile(t, filepath.Join(src, "sub", "nested.txt"), "nested content") + + dest := fmt.Sprintf("rsync://127.0.0.1:%d/incoming", port) + if err := runGrsync(t, "-a", src, dest); err != nil { + t.Fatalf("grsync %s returned error: %v", dest, err) + } + + got, err := os.ReadFile(filepath.Join(modRoot, "hello.txt")) + if err != nil { + t.Fatalf("reading synced file: %v", err) + } + if string(got) != "pushed to a real rsync:// daemon URL" { + t.Errorf("content = %q, want %q", got, "pushed to a real rsync:// daemon URL") + } + if _, err := os.ReadFile(filepath.Join(modRoot, "sub", "nested.txt")); err != nil { + t.Errorf("reading synced nested file: %v", err) + } + if errLog.Len() != 0 { + t.Errorf("daemon error log = %q, want empty", errLog.String()) + } +} + +func TestE2E_LocalToRsyncDaemon_AuthenticatedViaEnvVar(t *testing.T) { + modRoot := t.TempDir() + secretsPath := writeTestSecretsFile(t, "alice:hunter2\n") + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "private": {Name: "private", Path: modRoot, ReadOnly: false, AuthUsers: []string{"alice"}, SecretsFile: secretsPath}, + }} + port, _ := startTestDaemon(t, cfg) + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "secret.txt"), "authenticated upload") + + t.Setenv("RSYNC_PASSWORD", "hunter2") + dest := fmt.Sprintf("rsync://alice@127.0.0.1:%d/private", port) + if err := runGrsync(t, "-a", src, dest); err != nil { + t.Fatalf("grsync %s returned error: %v", dest, err) + } + + got, err := os.ReadFile(filepath.Join(modRoot, "secret.txt")) + if err != nil { + t.Fatalf("reading synced file: %v", err) + } + if string(got) != "authenticated upload" { + t.Errorf("content = %q, want %q", got, "authenticated upload") + } +} + +func TestE2E_LocalToRsyncDaemon_WrongPasswordFailsClearly(t *testing.T) { + modRoot := t.TempDir() + secretsPath := writeTestSecretsFile(t, "alice:hunter2\n") + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "private": {Name: "private", Path: modRoot, ReadOnly: false, AuthUsers: []string{"alice"}, SecretsFile: secretsPath}, + }} + port, _ := startTestDaemon(t, cfg) + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "secret.txt"), "should never arrive") + + t.Setenv("RSYNC_PASSWORD", "wrong-password") + dest := fmt.Sprintf("rsync://alice@127.0.0.1:%d/private", port) + err := runGrsync(t, "-a", src, dest) + if err == nil { + t.Fatalf("grsync %s with a wrong password returned nil error, want an error", dest) + } + + if _, statErr := os.Stat(filepath.Join(modRoot, "secret.txt")); !os.IsNotExist(statErr) { + t.Errorf("secret.txt should not have been written after a failed auth, stat error = %v", statErr) + } +} + +func TestE2E_LocalToRsyncDaemon_ReadOnlyModuleRejectsUpload(t *testing.T) { + modRoot := t.TempDir() + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "public": {Name: "public", Path: modRoot, ReadOnly: true}, + }} + port, _ := startTestDaemon(t, cfg) + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "upload.txt"), "should be refused") + + dest := fmt.Sprintf("rsync://127.0.0.1:%d/public", port) + if err := runGrsync(t, "-a", src, dest); err == nil { + t.Fatalf("grsync %s against a read-only module returned nil error, want an error", dest) + } +} + +func TestE2E_PullingFromRsyncDaemonSourceIsRejected(t *testing.T) { + dest := t.TempDir() + err := runGrsync(t, "rsync://127.0.0.1:8730/whatever", dest) + if err == nil { + t.Fatalf("grsync with an rsync:// source returned nil error, want a clear \"not yet supported\" error") + } + if !strings.Contains(err.Error(), "not yet supported") { + t.Errorf("error = %q, want it to explain pulling isn't supported", err.Error()) + } +} + +func TestE2E_RsyncDaemonDestinationMissingModuleIsRejected(t *testing.T) { + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "f.txt"), "x") + + err := runGrsync(t, src, "rsync://127.0.0.1:8730") + if err == nil { + t.Fatalf("grsync with a moduleless rsync:// destination returned nil error, want an error") + } +} + +func TestE2E_RsyncDaemonDestinationSubPathIsRejected(t *testing.T) { + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "f.txt"), "x") + + err := runGrsync(t, src, "rsync://127.0.0.1:8730/module/subdir") + if err == nil { + t.Fatalf("grsync with a sub-path rsync:// destination returned nil error, want an error") + } +} + +func TestE2E_LocalToRsyncDaemon_PasswordFile(t *testing.T) { + // No platform skip here: unlike TestE2E_PasswordFileWorldReadableIsRejected, + // this doesn't depend on the world-readable check actually enforcing + // anything (checkPasswordFilePermissions is a no-op on Windows, but the + // --password-file flag and its happy path work identically everywhere). + modRoot := t.TempDir() + secretsPath := writeTestSecretsFile(t, "alice:hunter2\n") + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "private": {Name: "private", Path: modRoot, ReadOnly: false, AuthUsers: []string{"alice"}, SecretsFile: secretsPath}, + }} + port, _ := startTestDaemon(t, cfg) + + passwordFile := filepath.Join(t.TempDir(), "grsync.password") + if err := os.WriteFile(passwordFile, []byte("hunter2\n"), 0o600); err != nil { + t.Fatalf("writing password file: %v", err) + } + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "via-file.txt"), "authenticated via --password-file") + + dest := fmt.Sprintf("rsync://alice@127.0.0.1:%d/private", port) + if err := runGrsync(t, "-a", "--password-file", passwordFile, src, dest); err != nil { + t.Fatalf("grsync %s returned error: %v", dest, err) + } + + got, err := os.ReadFile(filepath.Join(modRoot, "via-file.txt")) + if err != nil { + t.Fatalf("reading synced file: %v", err) + } + if string(got) != "authenticated via --password-file" { + t.Errorf("content = %q, want %q", got, "authenticated via --password-file") + } +} + +func TestE2E_PasswordFileWorldReadableIsRejected(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("world-readable permission check is POSIX-only, see checkPasswordFilePermissions") + } + + modRoot := t.TempDir() + secretsPath := writeTestSecretsFile(t, "alice:hunter2\n") + cfg := &daemon.Config{Modules: map[string]daemon.Module{ + "private": {Name: "private", Path: modRoot, ReadOnly: false, AuthUsers: []string{"alice"}, SecretsFile: secretsPath}, + }} + port, _ := startTestDaemon(t, cfg) + + passwordFile := filepath.Join(t.TempDir(), "world-readable.password") + if err := os.WriteFile(passwordFile, []byte("hunter2\n"), 0o644); err != nil { + t.Fatalf("writing password file: %v", err) + } + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "f.txt"), "should never be sent") + + dest := fmt.Sprintf("rsync://alice@127.0.0.1:%d/private", port) + err := runGrsync(t, "--password-file", passwordFile, src, dest) + if err == nil { + t.Fatalf("grsync with a world-readable --password-file returned nil error, want an error") + } + if !strings.Contains(err.Error(), "world readable") { + t.Errorf("error = %q, want it to mention the file being world readable", err.Error()) + } +} + +func writeTestSecretsFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "rsyncd.secrets") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("writing secrets file: %v", err) + } + return path +} diff --git a/internal/cli/sync.go b/internal/cli/sync.go index 635e5a8..8b1b433 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -6,6 +6,7 @@ import ( "github.com/spf13/cobra" + "github.com/syntaxroot-cc/grsync/internal/daemon" "github.com/syntaxroot-cc/grsync/internal/pipeline" "github.com/syntaxroot-cc/grsync/internal/sync" "github.com/syntaxroot-cc/grsync/internal/transport" @@ -62,13 +63,18 @@ func toSyncRawRules(filterRules []FilterRule) []sync.RawRule { // runSync is the real sync entry point (as opposed to run, the // flag-echoing placeholder still used for --dry-run). For each source, it // syncs that source into destination - in-process for a local -// destination, or over an SSH-spawned connection for a remote one. +// destination, over an SSH-spawned connection for a remote user@host:path +// one, or over a plain TCP connection to an rsync:// daemon module. // -// Pulling FROM a remote source is not yet supported, only a local source -// to a local or remote destination - this ticket's scope is explicitly -// "local-to-local and local-to-remote," not pull mode. +// Pulling FROM a remote source (SSH or an rsync:// daemon) is not yet +// supported, only a local source to a local, SSH, or daemon destination - +// this scope is explicitly "push," not pull mode, matching the existing +// SSH-transport restriction rather than introducing a new asymmetry. func runSync(cmd *cobra.Command, sources []string, destination string, opts *options) error { for _, src := range sources { + if isRsyncURL(src) { + return fmt.Errorf("pulling from an rsync daemon source (%q) is not yet supported", src) + } if _, ok := transport.ParseRemotePath(src); ok { return fmt.Errorf("pulling from a remote source (%q) is not yet supported", src) } @@ -81,17 +87,52 @@ func runSync(cmd *cobra.Command, sources []string, destination string, opts *opt return fmt.Errorf("compiling filter rules: %w", err) } + // isRsyncURL is checked, and rsyncURL parsed, before + // transport.ParseRemotePath ever looks at destination: an rsync:// + // URL is never valid [user@]host:path syntax (ParseRemotePath itself + // now refuses anything containing "://"), but checking here first + // means that's true by construction, not just by the two parsers + // happening to agree. + var rsyncURL daemon.URL + isRsyncDaemon := isRsyncURL(destination) + if isRsyncDaemon { + rsyncURL, err = daemon.ParseURL(destination) + if err != nil { + return fmt.Errorf("parsing %q: %w", destination, err) + } + if rsyncURL.Module == "" { + return fmt.Errorf("%q has no module - an rsync:// sync destination must be rsync://host/module", destination) + } + if rsyncURL.Path != "" { + return fmt.Errorf("%q targets a sub-path within a module, which is not yet supported - "+ + "the daemon protocol only supports syncing an entire module", destination) + } + } remote, isRemote := transport.ParseRemotePath(destination) + // Resolved once, outside the per-source loop below, so a multi-source + // sync against the same daemon destination only ever prompts for (or + // reads) a password once - not once per source, even though each + // source gets its own connection, same as the SSH path already does. + var password daemon.PasswordFunc + if isRsyncDaemon { + password = resolvePassword(opts.passwordFile, cmd.InOrStdin()) + } + for _, src := range sources { - if isRemote { + switch { + case isRsyncDaemon: + if err := syncToRsyncDaemon(src, rsyncURL, password, walkOpts, rules); err != nil { + return fmt.Errorf("syncing %q to %q: %w", src, destination, err) + } + case isRemote: if err := syncToRemote(opts.rsh, src, remote, walkOpts, rules); err != nil { return fmt.Errorf("syncing %q to %q: %w", src, destination, err) } - continue - } - if err := syncLocal(src, destination, walkOpts, rules, attrOpts); err != nil { - return fmt.Errorf("syncing %q to %q: %w", src, destination, err) + default: + if err := syncLocal(src, destination, walkOpts, rules, attrOpts); err != nil { + return fmt.Errorf("syncing %q to %q: %w", src, destination, err) + } } } diff --git a/internal/daemon/auth.go b/internal/daemon/auth.go index 7c99754..b2f5fff 100644 --- a/internal/daemon/auth.go +++ b/internal/daemon/auth.go @@ -28,6 +28,25 @@ func (m Module) AuthRequired() bool { return len(m.AuthUsers) > 0 } +// PasswordFunc resolves the password to use for daemon authentication. +// DialAuth calls it at most once, and only if the server actually +// challenges for a password (an "@RSYNCD: AUTHREQD" line) - matching real +// rsync's own client behavior, where auth_client() is only ever invoked +// in response to that same line. Connecting to a module that turns out +// not to require authentication never calls this at all, so a caller +// backing it with an interactive terminal prompt or a --password-file +// read never triggers either one against an anonymous module - resolving +// eagerly, before knowing whether the server will ask, would be a real +// regression from real rsync's behavior here, not just a style choice. +type PasswordFunc func() (string, error) + +// StaticPassword wraps an already-known password (e.g. a test fixture, or +// a caller that has already decided eager resolution is fine) as a +// PasswordFunc. +func StaticPassword(password string) PasswordFunc { + return func() (string, error) { return password, nil } +} + // md4Hash returns the base64-encoded (standard alphabet, no padding - the // same encoding real rsync's own base64_encode(..., pad=0) produces) MD4 // digest of secret followed by challenge. This matches real rsync's @@ -168,8 +187,10 @@ func ServeAuth(c *conn, m Module) (user string, err error) { // itself is never sent, only this one-way hash of it, so ErrAuthFailed // and any wire capture of this exchange should never contain it. An empty // user is sent as "nobody", matching real rsync's own client behavior for -// anonymous-looking auth attempts. -func DialAuth(c *conn, user, password string) error { +// anonymous-looking auth attempts. password is only ever called if the +// server actually asks for one - see PasswordFunc's doc comment for why +// that laziness matters, not just how it works. +func DialAuth(c *conn, user string, password PasswordFunc) error { for { line, err := readLine(c.r) if err != nil { @@ -183,7 +204,11 @@ func DialAuth(c *conn, user, password string) error { if sendUser == "" { sendUser = "nobody" } - response := md4Hash(password, challenge) + pass, err := password() + if err != nil { + return fmt.Errorf("resolving password: %w", err) + } + response := md4Hash(pass, challenge) if err := writeLine(c.w, sendUser+" "+response); err != nil { return fmt.Errorf("writing auth response: %w", err) } diff --git a/internal/daemon/auth_test.go b/internal/daemon/auth_test.go index 8cc57f0..1f03a3d 100644 --- a/internal/daemon/auth_test.go +++ b/internal/daemon/auth_test.go @@ -42,7 +42,7 @@ func TestAuth_NoAuthRequiredSendsOK(t *testing.T) { errCh <- err }() - if err := DialAuth(client, "", ""); err != nil { + if err := DialAuth(client, "", StaticPassword("")); err != nil { t.Fatalf("DialAuth returned error: %v", err) } if err := <-errCh; err != nil { @@ -63,7 +63,7 @@ func TestAuth_CorrectPasswordSucceeds(t *testing.T) { errCh <- err }() - if err := DialAuth(client, "alice", "hunter2"); err != nil { + if err := DialAuth(client, "alice", StaticPassword("hunter2")); err != nil { t.Fatalf("DialAuth returned error: %v", err) } if err := <-errCh; err != nil { @@ -85,7 +85,7 @@ func TestAuth_WrongPasswordFails(t *testing.T) { errCh <- err }() - dialErr := DialAuth(client, "alice", "wrong-password") + dialErr := DialAuth(client, "alice", StaticPassword("wrong-password")) if dialErr == nil { t.Fatalf("DialAuth with a wrong password returned nil error, want an error") } @@ -108,7 +108,7 @@ func TestAuth_UnauthorizedUserFails(t *testing.T) { errCh <- err }() - dialErr := DialAuth(client, "eve", "hunter2") + dialErr := DialAuth(client, "eve", StaticPassword("hunter2")) if dialErr == nil { t.Fatalf("DialAuth for an unauthorized user returned nil error, want an error") } @@ -133,7 +133,7 @@ func TestAuth_NoPlaintextPasswordOnWire(t *testing.T) { errCh <- err }() - if err := DialAuth(client, "alice", password); err != nil { + if err := DialAuth(client, "alice", StaticPassword(password)); err != nil { t.Fatalf("DialAuth returned error: %v", err) } if err := <-errCh; err != nil { @@ -158,7 +158,7 @@ func TestAuth_MissingSecretsFileFails(t *testing.T) { errCh <- err }() - dialErr := DialAuth(client, "alice", "hunter2") + dialErr := DialAuth(client, "alice", StaticPassword("hunter2")) if dialErr == nil { t.Fatalf("DialAuth with a missing secrets file returned nil error, want an error") } diff --git a/internal/daemon/client.go b/internal/daemon/client.go new file mode 100644 index 0000000..830f41d --- /dev/null +++ b/internal/daemon/client.go @@ -0,0 +1,37 @@ +package daemon + +import ( + "fmt" + "net" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +// DialClient runs a full client-side daemon session over an already- +// connected nc: the greeting/module-selection handshake, authentication +// (only actually triggered if the server challenges for it - see +// PasswordFunc), and the resulting transfer against localPath. It is the +// client-side counterpart to ServeConn, and exists for the same reason: +// DialGreeting/DialAuth/DialModule are built around this package's own +// unexported conn type, so external callers (internal/cli, in +// particular) had no way to actually reach them until now - this is +// where net.Conn crosses into that internal representation. +// +// module must be non-empty: DialClient runs a transfer, not a listing - +// callers that want to list a daemon's modules should use DialGreeting +// directly with an empty module instead. +func DialClient(nc net.Conn, module, user string, password PasswordFunc, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions) error { + if module == "" { + return fmt.Errorf("DialClient requires a module name") + } + + c := newConn(nc) + + if _, err := DialGreeting(c, module); err != nil { + return err + } + if err := DialAuth(c, user, password); err != nil { + return err + } + return DialModule(c, direction, localPath, rules, walkOpts, attrOpts) +} diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go new file mode 100644 index 0000000..fedebe9 --- /dev/null +++ b/internal/daemon/client_test.go @@ -0,0 +1,126 @@ +package daemon + +import ( + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/syntaxroot-cc/grsync/internal/sync" +) + +func dialRawConn(t *testing.T, addr string) net.Conn { + t.Helper() + nc, err := net.DialTimeout("tcp", addr, 5*time.Second) + if err != nil { + t.Fatalf("dialing %s: %v", addr, err) + } + t.Cleanup(func() { _ = nc.Close() }) + return nc +} + +func TestDialClient_DownloadOverRealTCP(t *testing.T) { + modRoot := t.TempDir() + mustWriteFile(t, filepath.Join(modRoot, "hello.txt"), "hello via DialClient") + + cfg := &Config{Modules: map[string]Module{ + "public": {Name: "public", Path: modRoot, ReadOnly: true, List: true}, + }} + addr, errLog := startTestDaemon(t, cfg) + nc := dialRawConn(t, addr) + + dest := t.TempDir() + err := DialClient(nc, "public", "", StaticPassword(""), DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}) + if err != nil { + t.Fatalf("DialClient returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(dest, "hello.txt")) + if err != nil { + t.Fatalf("reading downloaded file: %v", err) + } + if string(got) != "hello via DialClient" { + t.Errorf("downloaded content = %q, want %q", got, "hello via DialClient") + } + if errLog.Len() != 0 { + t.Errorf("daemon error log = %q, want empty", errLog.String()) + } +} + +func TestDialClient_UploadOverRealTCP(t *testing.T) { + modRoot := t.TempDir() + secretsPath := writeSecretsFile(t, "alice:hunter2\n") + + cfg := &Config{Modules: map[string]Module{ + "incoming": {Name: "incoming", Path: modRoot, ReadOnly: false, AuthUsers: []string{"alice"}, SecretsFile: secretsPath}, + }} + addr, errLog := startTestDaemon(t, cfg) + nc := dialRawConn(t, addr) + + src := t.TempDir() + mustWriteFile(t, filepath.Join(src, "upload.txt"), "pushed via DialClient") + rules, err := sync.CompileRules(nil) + if err != nil { + t.Fatalf("compiling empty rule set: %v", err) + } + + err = DialClient(nc, "incoming", "alice", StaticPassword("hunter2"), DirectionPut, src, rules, sync.WalkOptions{}, sync.AttrOptions{}) + if err != nil { + t.Fatalf("DialClient returned error: %v", err) + } + + got, err := os.ReadFile(filepath.Join(modRoot, "upload.txt")) + if err != nil { + t.Fatalf("reading uploaded file: %v", err) + } + if string(got) != "pushed via DialClient" { + t.Errorf("uploaded content = %q, want %q", got, "pushed via DialClient") + } + if errLog.Len() != 0 { + t.Errorf("daemon error log = %q, want empty", errLog.String()) + } +} + +// TestDialClient_PasswordFuncNotCalledForAnonymousModule is the laziness +// guarantee made concrete: PasswordFunc must never be invoked when the +// server never challenges for a password, exactly matching real rsync's +// own auth_client(), which is only ever called in response to an +// AUTHREQD line. A PasswordFunc backed by an interactive terminal prompt +// or a --password-file read must not fire against an anonymous module. +func TestDialClient_PasswordFuncNotCalledForAnonymousModule(t *testing.T) { + modRoot := t.TempDir() + mustWriteFile(t, filepath.Join(modRoot, "open.txt"), "no auth needed") + + cfg := &Config{Modules: map[string]Module{ + "public": {Name: "public", Path: modRoot, ReadOnly: true, List: true}, + }} + addr, _ := startTestDaemon(t, cfg) + nc := dialRawConn(t, addr) + + called := false + poisonedPassword := PasswordFunc(func() (string, error) { + called = true + return "", nil + }) + + dest := t.TempDir() + err := DialClient(nc, "public", "", poisonedPassword, DirectionGet, dest, nil, sync.WalkOptions{}, sync.AttrOptions{}) + if err != nil { + t.Fatalf("DialClient returned error: %v", err) + } + if called { + t.Errorf("PasswordFunc was called for a module that never requested authentication") + } +} + +func TestDialClient_RejectsEmptyModule(t *testing.T) { + cfg := &Config{Modules: map[string]Module{}} + addr, _ := startTestDaemon(t, cfg) + nc := dialRawConn(t, addr) + + err := DialClient(nc, "", "", StaticPassword(""), DirectionGet, t.TempDir(), nil, sync.WalkOptions{}, sync.AttrOptions{}) + if err == nil { + t.Fatalf("DialClient with an empty module returned nil error, want an error") + } +} diff --git a/internal/daemon/server_test.go b/internal/daemon/server_test.go index 87d12f4..8fea8ae 100644 --- a/internal/daemon/server_test.go +++ b/internal/daemon/server_test.go @@ -55,7 +55,7 @@ func TestDaemon_RealTCP_AnonymousDownload(t *testing.T) { if _, err := DialGreeting(client, "public"); err != nil { t.Fatalf("DialGreeting: %v", err) } - if err := DialAuth(client, "", ""); err != nil { + if err := DialAuth(client, "", StaticPassword("")); err != nil { t.Fatalf("DialAuth: %v", err) } dest := t.TempDir() @@ -91,7 +91,7 @@ func TestDaemon_RealTCP_AuthenticatedUpload(t *testing.T) { if _, err := DialGreeting(client, "incoming"); err != nil { t.Fatalf("DialGreeting: %v", err) } - if err := DialAuth(client, "alice", "hunter2"); err != nil { + if err := DialAuth(client, "alice", StaticPassword("hunter2")); err != nil { t.Fatalf("DialAuth: %v", err) } @@ -134,7 +134,7 @@ func TestDaemon_RealTCP_WrongPasswordRejected(t *testing.T) { if _, err := DialGreeting(client, "private"); err != nil { t.Fatalf("DialGreeting: %v", err) } - if err := DialAuth(client, "alice", "wrong"); err == nil { + if err := DialAuth(client, "alice", StaticPassword("wrong")); err == nil { t.Fatalf("DialAuth with a wrong password returned nil error, want an error") } } diff --git a/internal/transport/remotepath.go b/internal/transport/remotepath.go index fd7556d..5662424 100644 --- a/internal/transport/remotepath.go +++ b/internal/transport/remotepath.go @@ -21,6 +21,13 @@ type RemotePath struct { // // Disambiguation rule, in order: // +// 0. A "://" anywhere in s (e.g. "rsync://host/module") is never this +// syntax at all - real [user@]host:path syntax never contains one, +// and without this check "rsync://host/module" would otherwise parse +// as host "rsync", path "//host/module", which is wrong in a way +// that's easy to miss (it "succeeds" instead of failing loudly). +// Checked alongside the Windows-drive-letter case below, before any +// of the numbered rules that follow ever run. // 1. A single ASCII letter immediately followed by ":" (e.g. "C:", // "C:\Users\...") is always a Windows drive letter, never a remote // host - real single-letter hostnames in this position are @@ -39,7 +46,7 @@ type RemotePath struct { // full of colons itself). // 4. Otherwise, the first ":" is the separator. func ParseRemotePath(s string) (RemotePath, bool) { - if s == "" || isWindowsDriveLetterPath(s) { + if s == "" || isWindowsDriveLetterPath(s) || strings.Contains(s, "://") { return RemotePath{}, false } diff --git a/internal/transport/remotepath_test.go b/internal/transport/remotepath_test.go index cd1dd1a..2860cc9 100644 --- a/internal/transport/remotepath_test.go +++ b/internal/transport/remotepath_test.go @@ -26,6 +26,8 @@ func TestParseRemotePath(t *testing.T) { {"empty string", "", RemotePath{}, false}, {"malformed ipv6 missing close bracket", "[::1:path", RemotePath{}, false}, {"malformed ipv6 no colon after bracket", "[::1]path", RemotePath{}, false}, + {"rsync daemon URL is not SSH syntax", "rsync://example.com/module", RemotePath{}, false}, + {"rsync daemon URL with user and port is not SSH syntax", "rsync://alice@example.com:8730/module", RemotePath{}, false}, } for _, tt := range tests {