serve sftp: Add support for public key with auth proxy - fixes #3572

cmd/serve/ftp/ftp.go

@@ -246,7 +246,7 @@ var connServeFunction = []byte("(*Conn).Serve(")
 func (s *server) CheckPasswd(user, pass string) (ok bool, err error) {
 	var VFS *vfs.VFS
 	if s.proxy != nil {
-		VFS, _, err = s.proxy.Call(user, pass)
+		VFS, _, err = s.proxy.Call(user, pass, false)
 		if err != nil {
 			fs.Infof(nil, "proxy login failed: %v", err)
 			return false, nil

cmd/serve/proxy/proxy.go

@@ -108,7 +108,7 @@ type Proxy struct {
 // cacheEntry is what is stored in the vfsCache
 type cacheEntry struct {
 	vfs    *vfs.VFS // stored VFS
-	pwHash []byte   // bcrypt hash of the password
+	pwHash []byte   // bcrypt hash of the password/publicKey
 }
 
 // New creates a new proxy with the Options passed in
@@ -162,12 +162,21 @@ func (p *Proxy) run(in map[string]string) (config configmap.Simple, err error) {
 }
 
 // call runs the auth proxy and returns a cacheEntry and an error
-func (p *Proxy) call(user, pass string, passwordBytes []byte) (value interface{}, err error) {
+func (p *Proxy) call(user, auth string, isPublicKey bool) (value interface{}, err error) {
+	var config configmap.Simple
 	// Contact the proxy
-	config, err := p.run(map[string]string{
+	if isPublicKey {
-		"user": user,
+		config, err = p.run(map[string]string{
-		"pass": pass,
+			"user":       user,
-	})
+			"public_key": auth,
+		})
+	} else {
+		config, err = p.run(map[string]string{
+			"user": user,
+			"pass": auth,
+		})
+	}
+
 	if err != nil {
 		return nil, err
 	}
@@ -208,10 +217,11 @@ func (p *Proxy) call(user, pass string, passwordBytes []byte) (value interface{}
 		if err != nil {
 			return nil, false, err
 		}
+
 		// The bcrypt cost is a compromise between security and speed. The password is looked up on every
 		// transaction for WebDAV so we store it lightly hashed. An attacker would find it easier to go after
 		// the unencrypted password in memory most likely.
-		pwHash, err := bcrypt.GenerateFromPassword(passwordBytes, bcrypt.MinCost)
+		pwHash, err := bcrypt.GenerateFromPassword([]byte(auth), bcrypt.MinCost)
 		if err != nil {
 			return nil, false, err
 		}
@@ -227,17 +237,15 @@ func (p *Proxy) call(user, pass string, passwordBytes []byte) (value interface{}
 	return value, nil
 }
 
-// Call runs the auth proxy with the given input, returning a *vfs.VFS
+// Call runs the auth proxy with the username and password/public key provided
-// and the key used in the VFS cache.
+// returning a *vfs.VFS and the key used in the VFS cache.
-func (p *Proxy) Call(user, pass string) (VFS *vfs.VFS, vfsKey string, err error) {
+func (p *Proxy) Call(user, auth string, isPublicKey bool) (VFS *vfs.VFS, vfsKey string, err error) {
-	var passwordBytes = []byte(pass)
-
 	// Look in the cache first
 	value, ok := p.vfsCache.GetMaybe(user)
 
 	// If not found then call the proxy for a fresh answer
 	if !ok {
-		value, err = p.call(user, pass, passwordBytes)
+		value, err = p.call(user, auth, isPublicKey)
 		if err != nil {
 			return nil, "", err
 		}
@@ -249,14 +257,14 @@ func (p *Proxy) Call(user, pass string) (VFS *vfs.VFS, vfsKey string, err error)
 		return nil, "", errors.Errorf("proxy: value is not cache entry: %#v", value)
 	}
 
-	// Check the password is correct in the cached entry.  This
+	// Check the password / public key is correct in the cached entry.  This
 	// prevents an attack where subsequent requests for the same
 	// user don't have their auth checked. It does mean that if
 	// the password is changed, the user will have to wait for
 	// cache expiry (5m) before trying again.
-	err = bcrypt.CompareHashAndPassword(entry.pwHash, passwordBytes)
+	err = bcrypt.CompareHashAndPassword(entry.pwHash, []byte(auth))
 	if err != nil {
-		return nil, "", errors.Wrap(err, "proxy: incorrect password")
+		return nil, "", errors.Wrap(err, "proxy: incorrect password / public key")
 	}
 
 	return entry.vfs, user, nil

cmd/serve/proxy/proxy_test.go

@@ -1,6 +1,10 @@
 package proxy
 
 import (
+	"crypto/rand"
+	"crypto/rsa"
+	"encoding/base64"
+	"log"
 	"strings"
 	"testing"
 
@@ -10,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/bcrypt"
+	"golang.org/x/crypto/ssh"
 )
 
 func TestRun(t *testing.T) {
@@ -68,13 +73,13 @@ func TestRun(t *testing.T) {
 	const testUser = "testUser"
 	const testPass = "testPass"
 
-	t.Run("call", func(t *testing.T) {
+	t.Run("call w/Password", func(t *testing.T) {
 		// check cache empty
 		assert.Equal(t, 0, p.vfsCache.Entries())
 		defer p.vfsCache.Clear()
 
 		passwordBytes := []byte(testPass)
-		value, err := p.call(testUser, testPass, passwordBytes)
+		value, err := p.call(testUser, testPass, false)
 		require.NoError(t, err)
 		entry, ok := value.(cacheEntry)
 		require.True(t, ok)
@@ -95,12 +100,12 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, value, cacheValue)
 	})
 
-	t.Run("Call", func(t *testing.T) {
+	t.Run("Call w/Password", func(t *testing.T) {
 		// check cache empty
 		assert.Equal(t, 0, p.vfsCache.Entries())
 		defer p.vfsCache.Clear()
 
-		vfs, vfsKey, err := p.Call(testUser, testPass)
+		vfs, vfsKey, err := p.Call(testUser, testPass, false)
 		require.NoError(t, err)
 		require.NotNil(t, vfs)
 		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
@@ -121,7 +126,7 @@ func TestRun(t *testing.T) {
 		})
 
 		// now try again from the cache
-		vfs, vfsKey, err = p.Call(testUser, testPass)
+		vfs, vfsKey, err = p.Call(testUser, testPass, false)
 		require.NoError(t, err)
 		require.NotNil(t, vfs)
 		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
@@ -131,7 +136,7 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, 1, p.vfsCache.Entries())
 
 		// now try again from the cache but with wrong password
-		vfs, vfsKey, err = p.Call(testUser, testPass+"wrong")
+		vfs, vfsKey, err = p.Call(testUser, testPass+"wrong", false)
 		require.Error(t, err)
 		require.Contains(t, err.Error(), "incorrect password")
 		require.Nil(t, vfs)
@@ -142,4 +147,89 @@ func TestRun(t *testing.T) {
 
 	})
 
+	privateKey, privateKeyErr := rsa.GenerateKey(rand.Reader, 2048)
+	if privateKeyErr != nil {
+		log.Fatal("error generating test private key " + privateKeyErr.Error())
+	}
+	publicKey, publicKeyError := ssh.NewPublicKey(&privateKey.PublicKey)
+	if privateKeyErr != nil {
+		log.Fatal("error generating test public key " + publicKeyError.Error())
+	}
+
+	publicKeyString := base64.StdEncoding.EncodeToString(publicKey.Marshal())
+
+	t.Run("Call w/PublicKey", func(t *testing.T) {
+		// check cache empty
+		assert.Equal(t, 0, p.vfsCache.Entries())
+		defer p.vfsCache.Clear()
+
+		value, err := p.call(testUser, publicKeyString, true)
+		require.NoError(t, err)
+		entry, ok := value.(cacheEntry)
+		require.True(t, ok)
+
+		// check publicKey is correct in entry
+		require.NoError(t, err)
+		require.NotNil(t, entry.vfs)
+		f := entry.vfs.Fs()
+		require.NotNil(t, f)
+		assert.Equal(t, "proxy-"+testUser, f.Name())
+		assert.True(t, strings.HasPrefix(f.String(), "Local file system"))
+
+		// check it is in the cache
+		assert.Equal(t, 1, p.vfsCache.Entries())
+		cacheValue, ok := p.vfsCache.GetMaybe(testUser)
+		assert.True(t, ok)
+		assert.Equal(t, value, cacheValue)
+	})
+
+	t.Run("call w/PublicKey", func(t *testing.T) {
+		// check cache empty
+		assert.Equal(t, 0, p.vfsCache.Entries())
+		defer p.vfsCache.Clear()
+
+		vfs, vfsKey, err := p.Call(
+			testUser,
+			publicKeyString,
+			true,
+		)
+		require.NoError(t, err)
+		require.NotNil(t, vfs)
+		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
+		assert.Equal(t, testUser, vfsKey)
+
+		// check it is in the cache
+		assert.Equal(t, 1, p.vfsCache.Entries())
+		cacheValue, ok := p.vfsCache.GetMaybe(testUser)
+		assert.True(t, ok)
+		cacheEntry, ok := cacheValue.(cacheEntry)
+		assert.True(t, ok)
+		assert.Equal(t, vfs, cacheEntry.vfs)
+
+		// Test Get works while we have something in the cache
+		t.Run("Get", func(t *testing.T) {
+			assert.Equal(t, vfs, p.Get(testUser))
+			assert.Nil(t, p.Get("unknown"))
+		})
+
+		// now try again from the cache
+		vfs, vfsKey, err = p.Call(testUser, publicKeyString, true)
+		require.NoError(t, err)
+		require.NotNil(t, vfs)
+		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
+		assert.Equal(t, testUser, vfsKey)
+
+		// check cache is at the same level
+		assert.Equal(t, 1, p.vfsCache.Entries())
+
+		// now try again from the cache but with wrong public key
+		vfs, vfsKey, err = p.Call(testUser, publicKeyString+"wrong", true)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "incorrect public key")
+		require.Nil(t, vfs)
+		require.Equal(t, "", vfsKey)
+
+		// check cache is at the same level
+		assert.Equal(t, 1, p.vfsCache.Entries())
+	})
 }

cmd/serve/sftp/server.go

@@ -8,10 +8,10 @@ import (
 	"crypto/rsa"
 	"crypto/subtle"
 	"crypto/x509"
+	"encoding/base64"
 	"encoding/pem"
 	"fmt"
 	"io/ioutil"
-	"log"
 	"net"
 	"os"
 	"path/filepath"
@@ -119,8 +119,13 @@ func (s *server) acceptConnections() {
 func (s *server) serve() (err error) {
 	var authorizedKeysMap map[string]struct{}
 
+	// ensure the user isn't trying to use conflicting flags
+	if proxyflags.Opt.AuthProxy != "" && s.opt.AuthorizedKeys != "" && s.opt.AuthorizedKeys != DefaultOpt.AuthorizedKeys {
+		return errors.New("--auth-proxy and --authorized-keys cannot be used at the same time")
+	}
+
 	// Load the authorized keys
-	if s.opt.AuthorizedKeys != "" {
+	if s.opt.AuthorizedKeys != "" && proxyflags.Opt.AuthProxy == "" {
 		authKeysFile := env.ShellExpand(s.opt.AuthorizedKeys)
 		authorizedKeysMap, err = loadAuthorizedKeys(authKeysFile)
 		// If user set the flag away from the default then report an error
@@ -142,7 +147,7 @@ func (s *server) serve() (err error) {
 			fs.Debugf(describeConn(c), "Password login attempt for %s", c.User())
 			if s.proxy != nil {
 				// query the proxy for the config
-				_, vfsKey, err := s.proxy.Call(c.User(), string(pass))
+				_, vfsKey, err := s.proxy.Call(c.User(), string(pass), false)
 				if err != nil {
 					return nil, err
 				}
@@ -164,7 +169,21 @@ func (s *server) serve() (err error) {
 		PublicKeyCallback: func(c ssh.ConnMetadata, pubKey ssh.PublicKey) (*ssh.Permissions, error) {
 			fs.Debugf(describeConn(c), "Public key login attempt for %s", c.User())
 			if s.proxy != nil {
-				return nil, errors.New("public key login not allowed when using auth proxy")
+				//query the proxy for the config
+				_, vfsKey, err := s.proxy.Call(
+					c.User(),
+					base64.StdEncoding.EncodeToString(pubKey.Marshal()),
+					true,
+				)
+				if err != nil {
+					return nil, err
+				}
+				// just return the Key so we can get it back from the cache
+				return &ssh.Permissions{
+					Extensions: map[string]string{
+						"_vfsKey": vfsKey,
+					},
+				}, nil
 			}
 			if _, ok := authorizedKeysMap[string(pubKey.Marshal())]; ok {
 				return &ssh.Permissions{
@@ -220,7 +239,7 @@ func (s *server) serve() (err error) {
 	// accepted.
 	s.listener, err = net.Listen("tcp", s.opt.ListenAddr)
 	if err != nil {
-		log.Fatal("failed to listen for connection", err)
+		return errors.Wrap(err, "failed to listen for connection")
 	}
 	fs.Logf(nil, "SFTP server listening on %v\n", s.listener.Addr())
 

cmd/serve/sftp/sftp_test.go

@@ -17,7 +17,7 @@ import (
 	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fs/config/configmap"
 	"github.com/rclone/rclone/fs/config/obscure"
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -45,7 +45,7 @@ func TestSftp(t *testing.T) {
 		opt.Pass = testPass
 
 		w := newServer(f, &opt)
-		assert.NoError(t, w.serve())
+		require.NoError(t, w.serve())
 
 		// Read the host and port we started on
 		addr := w.Addr()

cmd/serve/webdav/webdav.go

@@ -159,7 +159,7 @@ func (w *WebDAV) getVFS(ctx context.Context) (VFS *vfs.VFS, err error) {
 
 // auth does proxy authorization
 func (w *WebDAV) auth(user, pass string) (value interface{}, err error) {
-	VFS, _, err := w.proxy.Call(user, pass)
+	VFS, _, err := w.proxy.Call(user, pass, false)
 	if err != nil {
 		return nil, err
 	}

docs/content/commands/rclone_serve_sftp.md

@@ -22,9 +22,9 @@ The server will log errors.  Use -v to see access logs.
 --bwlimit will be respected for file transfers.  Use --stats to
 control the stats printing.
 
-You must provide some means of authentication, either with --user/--pass,
+You must provide some means of authentication, either with `--user`/`--pass`,
-an authorized keys file (specify location with --authorized-keys - the
+an authorized keys file (specify location with `--authorized-keys` - the
-default is the same as ssh) or set the --no-auth flag for no
+default is the same as ssh), an `--auth-proxy`, or set the --no-auth flag for no
 authentication when logging in.
 
 Note that this also implements a small number of shell commands so
@@ -183,11 +183,13 @@ rclone will use that program to generate backends on the fly which
 then are used to authenticate incoming requests.  This uses a simple
 JSON based protocl with input on STDIN and output on STDOUT.
 
+> **PLEASE NOTE:** `--auth-proxy` and `--authorized-keys` cannot be used together, if `--auth-proxy` is set the authorized keys option will be ignored.
+
 There is an example program
 [bin/test_proxy.py](https://github.com/rclone/rclone/blob/master/test_proxy.py)
 in the rclone source code.
 
-The program's job is to take a `user` and `pass` on the input and turn
+The program's job is to take a `user` and `pass` or `public_key` on the input and turn
 those into the config for a backend on STDOUT in JSON format.  This
 config will have any default parameters for the backend added, but it
 won't use configuration from environment variables or command line
@@ -200,7 +202,7 @@ This config generated must have this extra parameter
 And it may have this parameter
 - `_obscure` - comma separated strings for parameters to obscure
 
-For example the program might take this on STDIN
+If password authentication was used by the client, input to the proxy process (on STDIN) would look similar to this:
 
 ```
 {
@@ -209,7 +211,16 @@ For example the program might take this on STDIN
 }
 ```
 
-And return this on STDOUT
+If public-key authentication was used by the client, input to the proxy process (on STDIN) would look similar to this:
+
+```
+{
+    "user": "me",
+    "public_key": "AAAAB3NzaC1yc2EAAAADAQABAAABAQDuwESFdAe14hVS6omeyX7edc+4BlQz1s6tWT5VxBu1YlR9w39BUAom4qDKuH+uqLMDIaS5F7D6lNwOuPylvyV/LgMFsgJV4QZ52Kws7mNgdsCEDTvfLz5Pt9Qtp6Gnah3kA0cmbXcfQFaO50Ojnz/W1ozg2z5evKmGtyYMtywTXvH/KVh5WjhbpQ/ERgu+1pbgwWkpWNBM8TCO8D85PSpxtkdpEdkaiGtKA6U+6ZOtdCqd88EasyMEBWLVSx9bvqMVsD8plYstXOm5CCptGWWqckZBIqp0YBP6atw/ANRESD3cIJ4dOO+qlWkLR5npAZZTx2Qqh+hVw6qqTFB+JQdf"
+}
+```
+
+And as an example return this on STDOUT
 
 ```
 {
@@ -223,7 +234,7 @@ And return this on STDOUT
 ```
 
 This would mean that an SFTP backend would be created on the fly for
-the `user` and `pass` returned in the output to the host given.  Note
+the `user` and `pass`/`public_key` returned in the output to the host given.  Note
 that since `_obscure` is set to `pass`, rclone will obscure the `pass`
 parameter before creating the backend (which is required for sftp
 backends).
@@ -235,8 +246,8 @@ in the output and the user to `user`. For security you'd probably want
 to restrict the `host` to a limited list.
 
 Note that an internal cache is keyed on `user` so only use that for
-configuration, don't use `pass`.  This also means that if a user's
+configuration, don't use `pass` or `public_key`.  This also means that if a user's
-password is changed the cache will need to expire (which takes 5 mins)
+password or public-key is changed the cache will need to expire (which takes 5 mins)
 before it takes effect.
 
 This can be used to build general purpose proxies to any kind of