diff --git a/cmd/serve/ftp/ftp.go b/cmd/serve/ftp/ftp.go index 9399aa989..73faf8f81 100644 --- a/cmd/serve/ftp/ftp.go +++ b/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 diff --git a/cmd/serve/proxy/proxy.go b/cmd/serve/proxy/proxy.go index 1c55588e7..dc4fce848 100644 --- a/cmd/serve/proxy/proxy.go +++ b/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{ - "user": user, - "pass": pass, - }) + if isPublicKey { + config, err = p.run(map[string]string{ + "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 -// and the key used in the VFS cache. -func (p *Proxy) Call(user, pass string) (VFS *vfs.VFS, vfsKey string, err error) { - var passwordBytes = []byte(pass) - +// Call runs the auth proxy with the username and password/public key provided +// returning a *vfs.VFS and the key used in the VFS cache. +func (p *Proxy) Call(user, auth string, isPublicKey bool) (VFS *vfs.VFS, vfsKey string, err error) { // 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 diff --git a/cmd/serve/proxy/proxy_test.go b/cmd/serve/proxy/proxy_test.go index 135ed6aab..5dd417e89 100644 --- a/cmd/serve/proxy/proxy_test.go +++ b/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()) + }) } diff --git a/cmd/serve/sftp/server.go b/cmd/serve/sftp/server.go index ca507882e..bd87483c5 100644 --- a/cmd/serve/sftp/server.go +++ b/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()) diff --git a/cmd/serve/sftp/sftp_test.go b/cmd/serve/sftp/sftp_test.go index f9fa92795..baa81c196 100644 --- a/cmd/serve/sftp/sftp_test.go +++ b/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() diff --git a/cmd/serve/webdav/webdav.go b/cmd/serve/webdav/webdav.go index 16610768f..c6cdf244e 100644 --- a/cmd/serve/webdav/webdav.go +++ b/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 } diff --git a/docs/content/commands/rclone_serve_sftp.md b/docs/content/commands/rclone_serve_sftp.md index d4b9164c2..845a64418 100644 --- a/docs/content/commands/rclone_serve_sftp.md +++ b/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, -an authorized keys file (specify location with --authorized-keys - the -default is the same as ssh) or set the --no-auth flag for no +You must provide some means of authentication, either with `--user`/`--pass`, +an authorized keys file (specify location with `--authorized-keys` - the +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 -password is changed the cache will need to expire (which takes 5 mins) +configuration, don't use `pass` or `public_key`. This also means that if a user's +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