serve: fix auth proxy using stale config parameters when making a backend

Before this change, if the user changed their password or public-key
and the auth proxy script returned updated config parameters for the
backend (eg a rotated api_key) rclone would continue to re-use the old
backend with the old config parameters out of the fscache.

This was because both the VFS cache and the fs/cache key were derived
from the user name only, so a change in the user's password or
public-key did not invalidate the cached backend.

Fix this by deriving the cache key from the user plus a hash of the
password/public-key, so a credential change forces a fresh backend.
The hash uses a per-process random HMAC key so the fragment that
appears in logs cannot be brute-forced offline.

cmd/serve/proxy/proxy.go

@@ -4,8 +4,11 @@ package proxy
 import (
 	"bytes"
 	"context"
+	"crypto/hmac"
+	"crypto/rand"
 	"crypto/sha256"
 	"crypto/subtle"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -98,10 +101,11 @@ to make proxy to many different sftp backends, you could make the
 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
+An internal cache of backends is keyed on the |user| and a hash of the
-configuration, don't use |pass| or |public_key|.  This also means that if a user's
+|pass| or |public_key|.  This means that if a user's password or
-password or public-key is changed the cache will need to expire (which takes 5 mins)
+public-key changes, or the proxy returns different config parameters
-before it takes effect.
+(eg a rotated |api_key|), a fresh backend will be created on the next
+request rather than the cached one being reused.
 
 This can be used to build general purpose proxies to any kind of
 backend that rclone supports.
@@ -196,6 +200,25 @@ func (p *Proxy) run(in map[string]string) (config configmap.Simple, err error) {
 	return config, nil
 }
 
+// cacheKeyHMACKey is a per-process random key used to derive cache keys
+// from auth credentials. Using a keyed hash (HMAC) rather than a bare
+// SHA256 means the hash fragment that appears in logs and backend names
+// cannot be used to brute-force the underlying password offline.
+var cacheKeyHMACKey = func() []byte {
+	key := make([]byte, 32)
+	if _, err := rand.Read(key); err != nil {
+		panic(fmt.Sprintf("proxy: failed to generate cache key: %v", err))
+	}
+	return key
+}()
+
+// generateCacheKey creates a composite cache key from user and auth credentials
+func generateCacheKey(user, auth string) string {
+	mac := hmac.New(sha256.New, cacheKeyHMACKey)
+	mac.Write([]byte(auth))
+	return user + "-" + hex.EncodeToString(mac.Sum(nil)[:8])
+}
+
 // call runs the auth proxy and returns a cacheEntry and an error
 func (p *Proxy) call(user, auth string, isPublicKey bool) (value any, err error) {
 	var config configmap.Simple
@@ -232,12 +255,17 @@ func (p *Proxy) call(user, auth string, isPublicKey bool) (value any, err error)
 		return nil, fmt.Errorf("proxy: couldn't find backend for %q: %w", fsName, err)
 	}
 
-	// base name of config on user name.  This may appear in logs
+	// Make the cache key include the auth so that changes to the
-	name := "proxy-" + user
+	// auth (eg the proxy returning new config) create a fresh
+	// backend rather than reusing the cached one.
+	cacheKey := generateCacheKey(user, auth)
+
+	// base name of config on user name and auth hash.  This may appear in logs
+	name := "proxy-" + cacheKey
 	fsString := name + ":" + root
 
 	// Look for fs in the VFS cache
-	value, err = p.vfsCache.Get(user, func(key string) (value any, ok bool, err error) {
+	value, err = p.vfsCache.Get(cacheKey, func(key string) (value any, ok bool, err error) {
 		// Create the Fs from the cache
 		f, err := cache.GetFn(p.ctx, fsString, func(ctx context.Context, fsString string) (fs.Fs, error) {
 			// Update the config with the default values
@@ -271,8 +299,11 @@ func (p *Proxy) call(user, auth string, isPublicKey bool) (value any, err error)
 // 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
+	// Cache key includes the auth so credential changes don't hit a stale entry
-	value, ok := p.vfsCache.GetMaybe(user)
+	cacheKey := generateCacheKey(user, auth)
+
+	// Look in the cache first with the credential-aware key
+	value, ok := p.vfsCache.GetMaybe(cacheKey)
 
 	// If not found then call the proxy for a fresh answer
 	if !ok {
@@ -288,11 +319,11 @@ func (p *Proxy) Call(user, auth string, isPublicKey bool) (VFS *vfs.VFS, vfsKey
 		return nil, "", fmt.Errorf("proxy: value is not cache entry: %#v", value)
 	}
 
-	// Check the password / public key is correct in the cached entry.  This
+	// Check the password / public key matches the cached entry. The
-	// prevents an attack where subsequent requests for the same
+	// cache key already includes a hash of the auth, so a changed
-	// user don't have their auth checked. It does mean that if
+	// credential lands on a fresh key rather than this entry; this
-	// the password is changed, the user will have to wait for
+	// check is a final guard against a hash collision on the key
-	// cache expiry (5m) before trying again.
+	// returning a backend created with different auth.
 	authHash := sha256.Sum256([]byte(auth))
 	if subtle.ConstantTimeCompare(authHash[:], entry.pwHash[:]) != 1 {
 		if isPublicKey {
@@ -301,7 +332,7 @@ func (p *Proxy) Call(user, auth string, isPublicKey bool) (VFS *vfs.VFS, vfsKey
 		return nil, "", errors.New("proxy: incorrect password")
 	}
 
-	return entry.vfs, user, nil
+	return entry.vfs, cacheKey, nil
 }
 
 // Get VFS from the cache using key - returns nil if not found

cmd/serve/proxy/proxy_test.go

@@ -91,12 +91,13 @@ func TestRun(t *testing.T) {
 		require.NotNil(t, entry.vfs)
 		f := entry.vfs.Fs()
 		require.NotNil(t, f)
-		assert.Equal(t, "proxy-"+testUser, f.Name())
+		cacheKey := generateCacheKey(testUser, testPass)
+		assert.Equal(t, "proxy-"+cacheKey, 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)
+		cacheValue, ok := p.vfsCache.GetMaybe(cacheKey)
 		assert.True(t, ok)
 		assert.Equal(t, value, cacheValue)
 	})
@@ -106,23 +107,24 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, 0, p.vfsCache.Entries())
 		defer p.vfsCache.Clear()
 
+		cacheKey := generateCacheKey(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())
+		assert.Equal(t, "proxy-"+cacheKey, vfs.Fs().Name())
-		assert.Equal(t, testUser, vfsKey)
+		assert.Equal(t, cacheKey, vfsKey)
 
 		// check it is in the cache
 		assert.Equal(t, 1, p.vfsCache.Entries())
-		cacheValue, ok := p.vfsCache.GetMaybe(testUser)
+		cacheValue, ok := p.vfsCache.GetMaybe(cacheKey)
 		assert.True(t, ok)
-		cacheEntry, ok := cacheValue.(cacheEntry)
+		cached, ok := cacheValue.(cacheEntry)
 		assert.True(t, ok)
-		assert.Equal(t, vfs, cacheEntry.vfs)
+		assert.Equal(t, vfs, cached.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.Equal(t, vfs, p.Get(cacheKey))
 			assert.Nil(t, p.Get("unknown"))
 		})
 
@@ -130,22 +132,36 @@ func TestRun(t *testing.T) {
 		vfs, vfsKey, err = p.Call(testUser, testPass, false)
 		require.NoError(t, err)
 		require.NotNil(t, vfs)
-		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
+		assert.Equal(t, "proxy-"+cacheKey, vfs.Fs().Name())
-		assert.Equal(t, testUser, vfsKey)
+		assert.Equal(t, cacheKey, vfsKey)
 
 		// check cache is at the same level
 		assert.Equal(t, 1, p.vfsCache.Entries())
 
-		// now try again from the cache but with wrong password
+		// A different password produces a different cache key, so it
-		vfs, vfsKey, err = p.Call(testUser, testPass+"wrong", false)
+		// creates a fresh cache entry rather than hitting the existing
+		// one. Authentication itself is the proxy script's job.
+		vfs2, vfsKey2, err := p.Call(testUser, testPass+"different", false)
+		require.NoError(t, err)
+		require.NotNil(t, vfs2)
+		assert.NotEqual(t, cacheKey, vfsKey2)
+		assert.Equal(t, 2, p.vfsCache.Entries())
+
+		// The underlying fs.Fs must also be a fresh instance from fs/cache
+		if vfs.Fs() == vfs2.Fs() {
+			t.Error("fs/cache returned the stale backend after auth change")
+		}
+
+		// If a cached entry's pwHash somehow doesn't match the supplied
+		// auth (eg a hash collision on the cache key), Call must reject
+		// it. Simulate by corrupting the cached pwHash.
+		entry := cacheEntry{vfs: vfs, pwHash: sha256.Sum256([]byte("tampered"))}
+		p.vfsCache.Put(cacheKey, entry)
+		vfs, vfsKey, err = p.Call(testUser, testPass, false)
 		require.Error(t, err)
 		require.Contains(t, err.Error(), "incorrect password")
 		require.Nil(t, vfs)
 		require.Equal(t, "", vfsKey)
-
-		// check cache is at the same level
-		assert.Equal(t, 1, p.vfsCache.Entries())
-
 	})
 
 	privateKey, privateKeyErr := rsa.GenerateKey(rand.Reader, 2048)
@@ -174,12 +190,13 @@ func TestRun(t *testing.T) {
 		require.NotNil(t, entry.vfs)
 		f := entry.vfs.Fs()
 		require.NotNil(t, f)
-		assert.Equal(t, "proxy-"+testUser, f.Name())
+		cacheKey := generateCacheKey(testUser, publicKeyString)
+		assert.Equal(t, "proxy-"+cacheKey, 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)
+		cacheValue, ok := p.vfsCache.GetMaybe(cacheKey)
 		assert.True(t, ok)
 		assert.Equal(t, value, cacheValue)
 	})
@@ -189,6 +206,7 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, 0, p.vfsCache.Entries())
 		defer p.vfsCache.Clear()
 
+		cacheKey := generateCacheKey(testUser, publicKeyString)
 		vfs, vfsKey, err := p.Call(
 			testUser,
 			publicKeyString,
@@ -196,20 +214,20 @@ func TestRun(t *testing.T) {
 		)
 		require.NoError(t, err)
 		require.NotNil(t, vfs)
-		assert.Equal(t, "proxy-"+testUser, vfs.Fs().Name())
+		assert.Equal(t, "proxy-"+cacheKey, vfs.Fs().Name())
-		assert.Equal(t, testUser, vfsKey)
+		assert.Equal(t, cacheKey, vfsKey)
 
 		// check it is in the cache
 		assert.Equal(t, 1, p.vfsCache.Entries())
-		cacheValue, ok := p.vfsCache.GetMaybe(testUser)
+		cacheValue, ok := p.vfsCache.GetMaybe(cacheKey)
 		assert.True(t, ok)
-		cacheEntry, ok := cacheValue.(cacheEntry)
+		cached, ok := cacheValue.(cacheEntry)
 		assert.True(t, ok)
-		assert.Equal(t, vfs, cacheEntry.vfs)
+		assert.Equal(t, vfs, cached.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.Equal(t, vfs, p.Get(cacheKey))
 			assert.Nil(t, p.Get("unknown"))
 		})
 
@@ -217,20 +235,35 @@ func TestRun(t *testing.T) {
 		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, "proxy-"+cacheKey, vfs.Fs().Name())
-		assert.Equal(t, testUser, vfsKey)
+		assert.Equal(t, cacheKey, 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
+		// A different public key produces a different cache key, so it
-		vfs, vfsKey, err = p.Call(testUser, publicKeyString+"wrong", true)
+		// creates a fresh cache entry rather than hitting the existing
+		// one. Authentication itself is the proxy script's job.
+		vfs2, vfsKey2, err := p.Call(testUser, publicKeyString+"different", true)
+		require.NoError(t, err)
+		require.NotNil(t, vfs2)
+		assert.NotEqual(t, cacheKey, vfsKey2)
+		assert.Equal(t, 2, p.vfsCache.Entries())
+
+		// The underlying fs.Fs must be a fresh instance from fs/cache
+		if vfs.Fs() == vfs2.Fs() {
+			t.Error("fs/cache returned the stale backend after public key change")
+		}
+
+		// If a cached entry's pwHash somehow doesn't match the supplied
+		// auth (eg a hash collision on the cache key), Call must reject
+		// it. Simulate by corrupting the cached pwHash.
+		entry := cacheEntry{vfs: vfs, pwHash: sha256.Sum256([]byte("tampered"))}
+		p.vfsCache.Put(cacheKey, entry)
+		vfs, vfsKey, err = p.Call(testUser, publicKeyString, 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())
 	})
 }