serve ftp: Use new facilities in goftp to fix and simplify auth proxy #4394

- Use Driver.CheckPasswd instead of server.CheckPasswd
- Make server.CheckPasswd return an error
- Remove awful findID to find parent function hack
- Remove Driver.Init as it is no longer called
- Fix backwards incompatible PublicIp -> PublicIP change

See: https://gitea.com/goftp/server/issues/117

cmd/serve/ftp/ftp.go

@@ -5,13 +5,11 @@
 package ftp
 
 import (
-	"bytes"
 	"fmt"
 	"io"
 	"net"
 	"os"
 	"os/user"
-	"runtime"
 	"strconv"
 	"sync"
 
@@ -114,13 +112,11 @@ You can set a single username and password with the --user and --pass flags.
 
 // server contains everything to run the server
 type server struct {
-	f         fs.Fs
+	f     fs.Fs
-	srv       *ftp.Server
+	srv   *ftp.Server
-	opt       Options
+	opt   Options
-	vfs       *vfs.VFS
+	vfs   *vfs.VFS
-	proxy     *proxy.Proxy
+	proxy *proxy.Proxy
-	pendingMu sync.Mutex
-	pending   map[string]*Driver // pending Driver~s that haven't got their VFS
 }
 
 // Make a new FTP to serve the remote
@@ -135,9 +131,8 @@ func newServer(f fs.Fs, opt *Options) (*server, error) {
 	}
 
 	s := &server{
-		f:       f,
+		f:   f,
-		opt:     *opt,
+		opt: *opt,
-		pending: make(map[string]*Driver),
 	}
 	if proxyflags.Opt.AuthProxy != "" {
 		s.proxy = proxy.New(&proxyflags.Opt)
@@ -151,7 +146,7 @@ func newServer(f fs.Fs, opt *Options) (*server, error) {
 		Factory:        s, // implemented by NewDriver method
 		Hostname:       host,
 		Port:           portNum,
-		PublicIp:       opt.PublicIP,
+		PublicIP:       opt.PublicIP,
 		PassivePorts:   opt.PassivePorts,
 		Auth:           s, // implemented by CheckPasswd method
 		Logger:         &Logger{},
@@ -200,80 +195,13 @@ func (l *Logger) PrintResponse(sessionID string, code int, message string) {
 	fs.Infof(sessionID, "< %d %s", code, message)
 }
 
-// findID finds the connection ID of the calling program.  It does
-// this in an incredibly hacky way by looking in the stack trace.
-//
-// callerName should be the name of the function that we are looking
-// for with a trailing '('
-//
-// What is really needed is a change of calling protocol so
-// CheckPassword is called with the connection.
-func findID(callerName []byte) (string, error) {
-	// Dump the stack in this format
-	// github.com/rclone/rclone/vendor/goftp.io/server.(*Conn).Serve(0xc0000b2680)
-	// 	/home/ncw/go/src/github.com/rclone/rclone/vendor/goftp.io/server/conn.go:116 +0x11d
-	buf := make([]byte, 4096)
-	n := runtime.Stack(buf, false)
-	buf = buf[:n]
-
-	// look for callerName first
-	i := bytes.Index(buf, callerName)
-	if i < 0 {
-		return "", errors.Errorf("findID: caller name not found in:\n%s", buf)
-	}
-	buf = buf[i+len(callerName):]
-
-	// find next ')'
-	i = bytes.IndexByte(buf, ')')
-	if i < 0 {
-		return "", errors.Errorf("findID: end of args not found in:\n%s", buf)
-	}
-	buf = buf[:i]
-
-	// trim off first argument
-	// find next ','
-	i = bytes.IndexByte(buf, ',')
-	if i >= 0 {
-		buf = buf[:i]
-	}
-
-	return string(buf), nil
-}
-
-var connServeFunction = []byte("(*Conn).Serve(")
-
 // CheckPasswd handle auth based on configuration
+//
+// This is not used - the one in Driver should be called instead
 func (s *server) CheckPasswd(user, pass string) (ok bool, err error) {
-	var VFS *vfs.VFS
+	err = errors.New("internal error: server.CheckPasswd should never be called")
-	if s.proxy != nil {
+	fs.Errorf(nil, "Error: %v", err)
-		VFS, _, err = s.proxy.Call(user, pass, false)
+	return false, err
-		if err != nil {
-			fs.Infof(nil, "proxy login failed: %v", err)
-			return false, nil
-		}
-		id, err := findID(connServeFunction)
-		if err != nil {
-			fs.Infof(nil, "proxy login failed: failed to read ID from stack: %v", err)
-			return false, nil
-		}
-		s.pendingMu.Lock()
-		d := s.pending[id]
-		delete(s.pending, id)
-		s.pendingMu.Unlock()
-		if d == nil {
-			err := errors.Errorf("proxy login failed: failed to find pending Driver under ID %q", id)
-			fs.Errorf(nil, "Error %v", err)
-			return false, err
-		}
-		d.vfs = VFS
-	} else {
-		ok = s.opt.BasicUser == user && (s.opt.BasicPass == "" || s.opt.BasicPass == pass)
-		if !ok {
-			fs.Infof(nil, "login failed: bad credentials")
-			return false, nil
-		}
-	}
-	return true, nil
 }
 
 // NewDriver starts a new session for each client connection
@@ -293,15 +221,25 @@ type Driver struct {
 	lock sync.Mutex
 }
 
-//Init a connection
+// CheckPasswd handle auth based on configuration
-func (d *Driver) Init(c *ftp.Conn) {
+func (d *Driver) CheckPasswd(user, pass string) (ok bool, err error) {
-	defer log.Trace("", "Init session")("")
+	s := d.s
-	if d.s.proxy != nil {
+	if s.proxy != nil {
-		id := fmt.Sprintf("%p", c)
+		var VFS *vfs.VFS
-		d.s.pendingMu.Lock()
+		VFS, _, err = s.proxy.Call(user, pass, false)
-		d.s.pending[id] = d
+		if err != nil {
-		d.s.pendingMu.Unlock()
+			fs.Infof(nil, "proxy login failed: %v", err)
+			return false, nil
+		}
+		d.vfs = VFS
+	} else {
+		ok = s.opt.BasicUser == user && (s.opt.BasicPass == "" || s.opt.BasicPass == pass)
+		if !ok {
+			fs.Infof(nil, "login failed: bad credentials")
+			return false, nil
+		}
 	}
+	return true, nil
 }
 
 //Stat get information on file or folder

cmd/serve/ftp/ftp_test.go

@@ -8,7 +8,6 @@
 package ftp
 
 import (
-	"fmt"
 	"testing"
 
 	_ "github.com/rclone/rclone/backend/local"
@@ -17,7 +16,6 @@ import (
 	"github.com/rclone/rclone/fs/config/configmap"
 	"github.com/rclone/rclone/fs/config/obscure"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	ftp "goftp.io/server"
 )
 
@@ -70,10 +68,3 @@ func TestFTP(t *testing.T) {
 
 	servetest.Run(t, "ftp", start)
 }
-
-func TestFindID(t *testing.T) {
-	id, err := findID([]byte("TestFindID("))
-	require.NoError(t, err)
-	// id should be the argument to this function
-	assert.Equal(t, fmt.Sprintf("%p", t), id)
-}