serve s3: fix auth proxy accepting any request signed with an empty secret GHSA-xwwr-4h3p-r22c CVE-PENDING

With --auth-proxy set and --auth-key unset, serve s3 registered every client
supplied access key ID with an empty secret and verified the SigV4 signature
against that, so anyone could sign a request for an arbitrary access key ID with
an empty secret and be let in. The proxy program was only ever given the access
key ID (as both user and pass) so it had nothing with which to authenticate the
client either.

An S3 client never sends its secret, only a signature made with it, so the
server has to know the secret to check the request. The auth proxy protocol as
been changed to handle this. For serve s3 the proxy program is given just the
access key ID as the user (no pass or public_key) and must return the matching
secret as _secret_access_key in its output. rclone verifies the request's
signature against that secret, refusing the request if the proxy rejects the
access key ID, doesn't return a secret or returns an empty one, or the signature
doesn't match. The secret is only used for this server's own verification and is
never registered with gofakes3, so other serve s3 instances in the same process
don't honour it.

The proxy's answers are cached. If a signature fails against a cached secret the
proxy is consulted again so a rotated secret takes effect immediately - but only
for a signature mismatch, and at most once every 10 seconds per access key ID
and client IP, so a stream of bad signatures can't make the proxy program run
for every request. A rotation never shuts down the cached backend under requests
still using it. A cached answer is checked with the proxy again once it is 5
minutes old even if in constant use, so revoking an access key ID takes effect
within 5 minutes.

This means --auth-key is no longer needed with --auth-proxy: it is ignored and a
warning is given at startup if both are set. The proxy is the source of truth
for both the credentials and the backend they map to. Presigned URLs (credential
in the query string) are now recognised by the proxy middleware too. The auth
proxy docs are added to serve s3.

Note that the serve s3 auth proxy protocol has changed. The proxy program is now
given the access key ID as "user" (it was previously given an MD5 hash of it,
with the access key ID as "pass") and must return the matching secret as
"_secret_access_key".

This needs gofakes3 v0.0.9 for signature.V4SignVerifyWithSecret.

cmd/serve/proxy/proxy.go

@@ -15,6 +15,8 @@ import (
 	"net/netip"
 	"os/exec"
 	"strings"
+	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/rclone/rclone/fs"
@@ -54,9 +56,10 @@ This config generated must have this extra parameter
 
 - |_root| - root to use for the backend
 
-And it may have this parameter
+And it may have these parameters
 
 - |_obscure| - comma separated strings for parameters to obscure
+- |_secret_access_key| - the secret for S3 access key auth (see below)
 
 If password authentication was used by the client, input to the proxy
 process (on STDIN) would look similar to this:
@@ -80,6 +83,32 @@ proxy process (on STDIN) would look similar to this:
 }
 |||
 
+If the client authenticated with an S3 access key (|rclone serve s3|),
+the client never sends its secret, only a signature made with it, so
+the input contains just the access key ID as the |user| with no |pass|
+or |public_key|:
+
+|||json
+{
+  "user": "AKIAIOSFODNN7EXAMPLE",
+  "client_ip": "192.168.1.1"
+}
+|||
+
+In this case the program must look up the secret access key for that
+access key ID and return it in the |_secret_access_key| field of the
+output. Rclone then uses that secret to verify the signature on the
+request, refusing the request if it does not match. This means the
+proxy program is the source of truth for both the credentials and the
+backend they map to. If the program does not return
+|_secret_access_key| or returns it empty the request is refused.
+
+The program's answer for an access key ID is cached (see below) but
+is checked with the program again after 5 minutes even if the access
+key ID is in constant use, so revoking an access key ID in the
+program takes effect within 5 minutes. A rotated secret takes effect
+on the first request signed with it.
+
 The |client_ip| key holds the IP address the client connected from,
 without a port number.  It can be used to restrict logins to certain
 networks, or to log authentication attempts centrally.  It is omitted if
@@ -145,19 +174,48 @@ func init() {
 
 // Proxy represents a proxy to turn auth requests into a VFS
 type Proxy struct {
-	cmdLine  []string // broken down command line
-	vfsCache *libcache.Cache
-	ctx      context.Context // for global config
-	Opt      Options
-	vfsOpt   vfscommon.Options
+	cmdLine     []string // broken down command line
+	vfsCache    *libcache.Cache
+	ctx         context.Context // for global config
+	Opt         Options
+	vfsOpt      vfscommon.Options
+	accessKeyMu sync.Mutex // serialises replacing a cached access key entry
 }
 
 // cacheEntry is what is stored in the vfsCache
 type cacheEntry struct {
-	vfs    *vfs.VFS          // stored VFS
-	pwHash [sha256.Size]byte // sha256 hash of the password/publicKey
+	vfs       *vfs.VFS          // stored VFS
+	pwHash    [sha256.Size]byte // sha256 hash of the password/publicKey
+	secret    string            // secret access key returned by the proxy for access key auth
+	refreshed *atomic.Int64     // unix nanoseconds when the proxy last confirmed this entry
 }
 
+// accessKeyRefreshInterval is the minimum time between consulting the
+// proxy again for a cached access key ID whose secret failed to
+// verify a request, so a stream of bad signatures for a known access
+// key ID can't make the proxy run for every request.
+//
+// A variable so tests can adjust it.
+var accessKeyRefreshInterval = 10 * time.Second
+
+// accessKeyRevalidateInterval is how long a cached access key ID is
+// trusted before the proxy is asked about it again even though its
+// signatures are still verifying, so a revoked access key ID stops
+// working within this time rather than for as long as it stays in
+// use.
+//
+// A variable so tests can adjust it.
+var accessKeyRevalidateInterval = 5 * time.Minute
+
+// authKind is the kind of credential the client presented
+type authKind int
+
+const (
+	authPassword  authKind = iota // auth is a password
+	authPublicKey                 // auth is a public key
+	authAccessKey                 // user is an S3 access key ID and auth is empty
+)
+
 // New creates a new proxy with the Options passed in
 //
 // Any VFS are created with the vfsOpt passed in.
@@ -251,14 +309,15 @@ func generateCacheKey(user, auth, clientIP string) string {
 }
 
 // call runs the auth proxy and returns a cacheEntry and an error
-func (p *Proxy) call(user, auth string, isPublicKey bool, clientIP string) (value any, err error) {
+func (p *Proxy) call(user, auth string, kind authKind, clientIP string) (value any, err error) {
 	// Contact the proxy
 	in := map[string]string{
 		"user": user,
 	}
-	if isPublicKey {
+	switch kind {
+	case authPublicKey:
 		in["public_key"] = auth
-	} else {
+	case authPassword:
 		in["pass"] = auth
 	}
 	if clientIP != "" {
@@ -278,6 +337,19 @@ func (p *Proxy) call(user, auth string, isPublicKey bool, clientIP string) (valu
 	if !ok {
 		return nil, errors.New("proxy: _root not set in result")
 	}
+	var secret string
+	if kind == authAccessKey {
+		secret, ok = config.Get("_secret_access_key")
+		if !ok {
+			return nil, errors.New("proxy: _secret_access_key not set in result")
+		}
+		// An empty secret would let anyone sign for this access key ID
+		if secret == "" {
+			return nil, errors.New("proxy: _secret_access_key is empty in result")
+		}
+		// Keep the secret out of the backend config
+		delete(config, "_secret_access_key")
+	}
 
 	// Find the backend
 	fsInfo, err := fs.Find(fsName)
@@ -290,6 +362,28 @@ func (p *Proxy) call(user, auth string, isPublicKey bool, clientIP string) (valu
 	// fresh backend rather than reusing the cached one.
 	cacheKey := generateCacheKey(user, auth, clientIP)
 
+	// For access key auth the secret isn't part of the cache key as
+	// it isn't known until the proxy has been run, so retire any
+	// cached entry whose secret the proxy has since changed.
+	//
+	// The stale entry is moved to a key including its old secret
+	// rather than deleted so that its VFS isn't shut down under
+	// requests still using it - it expires with the rest of the cache
+	// once it has been unused for long enough.
+	//
+	// The check, rename and the creation below are done under a lock
+	// so that two concurrent refreshes during a rotation can't have
+	// the second retire the backend the first has just created.
+	if kind == authAccessKey {
+		p.accessKeyMu.Lock()
+		defer p.accessKeyMu.Unlock()
+		if old, ok := p.vfsCache.GetMaybe(cacheKey); ok {
+			if entry, ok := old.(cacheEntry); ok && entry.secret != secret {
+				p.vfsCache.Rename(cacheKey, generateCacheKey(user, entry.secret, clientIP))
+			}
+		}
+	}
+
 	// base name of config on user name and auth hash.  This may appear in logs
 	name := "proxy-" + cacheKey
 	fsString := name + ":" + root
@@ -315,14 +409,21 @@ func (p *Proxy) call(user, auth string, isPublicKey bool, clientIP string) (valu
 		// need to in memory. An attacker would find it easier to go
 		// after the unencrypted password in memory most likely.
 		entry := cacheEntry{
-			vfs:    vfs.New(p.ctx, f, &p.vfsOpt),
-			pwHash: sha256.Sum256([]byte(auth)),
+			vfs:       vfs.New(p.ctx, f, &p.vfsOpt),
+			pwHash:    sha256.Sum256([]byte(auth)),
+			secret:    secret,
+			refreshed: new(atomic.Int64),
 		}
 		return entry, true, nil
 	})
 	if err != nil {
 		return nil, fmt.Errorf("proxy: failed to create backend: %w", err)
 	}
+	// The proxy has just confirmed this entry whether it was created
+	// or reused
+	if entry, ok := value.(cacheEntry); ok {
+		entry.refreshed.Store(time.Now().UnixNano())
+	}
 	return value, nil
 }
 
@@ -344,7 +445,11 @@ func (p *Proxy) Call(user, auth string, isPublicKey bool, remoteAddr string) (VF
 
 	// If not found then call the proxy for a fresh answer
 	if !ok {
-		value, err = p.call(user, auth, isPublicKey, clientIP)
+		kind := authPassword
+		if isPublicKey {
+			kind = authPublicKey
+		}
+		value, err = p.call(user, auth, kind, clientIP)
 		if err != nil {
 			return nil, "", err
 		}
@@ -372,6 +477,53 @@ func (p *Proxy) Call(user, auth string, isPublicKey bool, remoteAddr string) (VF
 	return entry.vfs, cacheKey, nil
 }
 
+// CallAccessKey runs the auth proxy for an S3 access key ID returning
+// a *vfs.VFS and the secret access key the proxy supplied for it.
+//
+// The caller must verify the request's signature against the returned
+// secret - the proxy only maps the access key ID to a backend and
+// secret, it cannot authenticate the client itself.
+//
+// If refresh is true the proxy is consulted rather than using a cached
+// answer, unless it was consulted for this access key ID less than
+// accessKeyRefreshInterval ago. A refresh never shuts down a cached
+// backend, even if the proxy returns a different secret, so this is
+// safe to do when a signature fails to verify in case the secret has
+// been rotated.
+//
+// A cached answer older than accessKeyRevalidateInterval is always
+// checked with the proxy so a revoked access key ID is refused even
+// if it is in constant use.
+//
+// remoteAddr is the address of the client as returned by net.Addr.String().
+func (p *Proxy) CallAccessKey(accessKeyID, remoteAddr string, refresh bool) (VFS *vfs.VFS, secret string, err error) {
+	clientIP := ipFromAddr(remoteAddr)
+	cacheKey := generateCacheKey(accessKeyID, "", clientIP)
+	value, ok := p.vfsCache.GetMaybe(cacheKey)
+	if ok {
+		if entry, isEntry := value.(cacheEntry); isEntry {
+			age := time.Since(time.Unix(0, entry.refreshed.Load()))
+			switch {
+			case age >= accessKeyRevalidateInterval:
+				refresh = true
+			case age < accessKeyRefreshInterval:
+				refresh = false
+			}
+		}
+	}
+	if !ok || refresh {
+		value, err = p.call(accessKeyID, "", authAccessKey, clientIP)
+		if err != nil {
+			return nil, "", err
+		}
+	}
+	entry, ok := value.(cacheEntry)
+	if !ok {
+		return nil, "", fmt.Errorf("proxy: value is not cache entry: %#v", value)
+	}
+	return entry.vfs, entry.secret, nil
+}
+
 // Get VFS from the cache using key - returns nil if not found
 func (p *Proxy) Get(key string) *vfs.VFS {
 	value, ok := p.vfsCache.GetMaybe(key)

cmd/serve/proxy/proxy_code.go

@@ -34,6 +34,25 @@ func main() {
 	if out["_root"] == "" {
 		out["_root"] = ""
 	}
+	// S3 access key auth has neither pass nor public_key and needs
+	// the secret returned, unless the user asks for it to be omitted
+	// or empty. The secret's suffix can be changed to simulate a
+	// rotation and an access key ID can be revoked.
+	_, havePass := in["pass"]
+	_, havePublicKey := in["public_key"]
+	switch {
+	case havePass || havePublicKey || in["user"] == "nosecret":
+	case in["user"] == os.Getenv("RCLONE_TEST_PROXY_REVOKED"):
+		log.Fatalf("access key ID %q revoked", in["user"])
+	case in["user"] == "emptysecret":
+		out["_secret_access_key"] = ""
+	default:
+		suffix := os.Getenv("RCLONE_TEST_PROXY_SECRET_SUFFIX")
+		if suffix == "" {
+			suffix = "-secret"
+		}
+		out["_secret_access_key"] = in["user"] + suffix
+	}
 	json.NewEncoder(os.Stdout).Encode(&out)
 	if err != nil {
 		log.Fatal(err)

cmd/serve/proxy/proxy_test.go

@@ -7,12 +7,15 @@ import (
 	"crypto/sha256"
 	"encoding/base64"
 	"strings"
+	"sync"
 	"testing"
+	"time"
 
 	_ "github.com/rclone/rclone/backend/local"
 	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fs/config/configmap"
 	"github.com/rclone/rclone/fs/config/obscure"
+	"github.com/rclone/rclone/vfs"
 	"github.com/rclone/rclone/vfs/vfscommon"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -118,7 +121,7 @@ func TestRun(t *testing.T) {
 		defer p.vfsCache.Clear()
 
 		passwordBytes := []byte(testPass)
-		value, err := p.call(testUser, testPass, false, testIP)
+		value, err := p.call(testUser, testPass, authPassword, testIP)
 		require.NoError(t, err)
 		entry, ok := value.(cacheEntry)
 		require.True(t, ok)
@@ -239,7 +242,7 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, 0, p.vfsCache.Entries())
 		defer p.vfsCache.Clear()
 
-		value, err := p.call(testUser, publicKeyString, true, testIP)
+		value, err := p.call(testUser, publicKeyString, authPublicKey, testIP)
 		require.NoError(t, err)
 		entry, ok := value.(cacheEntry)
 		require.True(t, ok)
@@ -328,6 +331,47 @@ func TestRun(t *testing.T) {
 	})
 }
 
+// TestCallAccessKeyConcurrentRefresh checks that concurrent refreshes
+// which all see a rotated secret end up sharing one backend and one
+// cache entry rather than the later ones retiring the entry an
+// earlier one created and returned.
+func TestCallAccessKeyConcurrentRefresh(t *testing.T) {
+	opt := Opt
+	opt.AuthProxy = "go run proxy_code.go"
+	p := New(context.Background(), &opt, &vfscommon.Opt)
+	defer p.Shutdown()
+	const remoteAddr = "192.0.2.1:1234"
+
+	oldInterval := accessKeyRefreshInterval
+	accessKeyRefreshInterval = 0
+	defer func() { accessKeyRefreshInterval = oldInterval }()
+
+	VFS, _, err := p.CallAccessKey("CONCURRENT", remoteAddr, false)
+	require.NoError(t, err)
+
+	// Rotate the secret then refresh from many goroutines at once
+	t.Setenv("RCLONE_TEST_PROXY_SECRET_SUFFIX", "-rotated")
+	const n = 8
+	results := make([]*vfs.VFS, n)
+	var wg sync.WaitGroup
+	for i := range n {
+		wg.Go(func() {
+			newVFS, secret, err := p.CallAccessKey("CONCURRENT", remoteAddr, true)
+			assert.NoError(t, err)
+			assert.Equal(t, "CONCURRENT-rotated", secret)
+			results[i] = newVFS
+		})
+	}
+	wg.Wait()
+
+	// Every caller must have got the same backend and only one
+	// entry was retired
+	for i := range n {
+		assert.Same(t, VFS, results[i], "goroutine %d got a different backend", i)
+	}
+	assert.Equal(t, 2, p.vfsCache.Entries())
+}
+
 func TestIPFromAddr(t *testing.T) {
 	for _, test := range []struct {
 		in   string
@@ -347,3 +391,87 @@ func TestIPFromAddr(t *testing.T) {
 		assert.Equal(t, test.want, ipFromAddr(test.in), test.in)
 	}
 }
+
+func TestCallAccessKey(t *testing.T) {
+	opt := Opt
+	opt.AuthProxy = "go run proxy_code.go"
+	p := New(context.Background(), &opt, &vfscommon.Opt)
+	defer p.Shutdown()
+	const remoteAddr = "192.0.2.1:1234"
+
+	// Disable refresh rate limiting for this test
+	oldInterval := accessKeyRefreshInterval
+	accessKeyRefreshInterval = 0
+	defer func() { accessKeyRefreshInterval = oldInterval }()
+
+	VFS, secret, err := p.CallAccessKey("AKID", remoteAddr, false)
+	require.NoError(t, err)
+	require.NotNil(t, VFS)
+	assert.Equal(t, "AKID-secret", secret)
+
+	// Check the cached entry is returned on the next call
+	VFS2, secret2, err := p.CallAccessKey("AKID", remoteAddr, false)
+	require.NoError(t, err)
+	assert.Same(t, VFS, VFS2)
+	assert.Equal(t, secret, secret2)
+
+	// Check a different access key ID gets a different backend
+	VFS3, secret3, err := p.CallAccessKey("OTHER", remoteAddr, false)
+	require.NoError(t, err)
+	assert.NotSame(t, VFS, VFS3)
+	assert.Equal(t, "OTHER-secret", secret3)
+
+	// Check a refresh with an unchanged secret keeps the cached backend
+	VFS4, secret4, err := p.CallAccessKey("AKID", remoteAddr, true)
+	require.NoError(t, err)
+	assert.Same(t, VFS, VFS4)
+	assert.Equal(t, secret, secret4)
+
+	// Check a refresh with a changed secret returns the new secret.
+	// The VFS is the same object as vfs.New shares a live VFS for the
+	// same backend and options, which keeps requests in flight under
+	// the old secret working.
+	t.Setenv("RCLONE_TEST_PROXY_SECRET_SUFFIX", "-rotated")
+	entries := p.vfsCache.Entries()
+	VFS5, secret5, err := p.CallAccessKey("AKID", remoteAddr, true)
+	require.NoError(t, err)
+	assert.Same(t, VFS, VFS5)
+	assert.Equal(t, "AKID-rotated", secret5)
+	// The old entry is retired rather than dropped
+	assert.Equal(t, entries+1, p.vfsCache.Entries())
+
+	// Check a proxy which doesn't return the secret is an error
+	_, _, err = p.CallAccessKey("nosecret", remoteAddr, false)
+	require.ErrorContains(t, err, "_secret_access_key not set")
+
+	// Check a proxy which returns an empty secret is an error
+	_, _, err = p.CallAccessKey("emptysecret", remoteAddr, false)
+	require.ErrorContains(t, err, "_secret_access_key is empty")
+
+	// Check refreshes are rate limited: with a long interval a
+	// refresh returns the cached secret without running the proxy
+	accessKeyRefreshInterval = time.Hour
+	t.Setenv("RCLONE_TEST_PROXY_SECRET_SUFFIX", "-rotated-again")
+	VFS6, secret6, err := p.CallAccessKey("AKID", remoteAddr, true)
+	require.NoError(t, err)
+	assert.Same(t, VFS5, VFS6)
+	assert.Equal(t, "AKID-rotated", secret6)
+
+	// And with no interval the proxy is run and the rotation seen
+	accessKeyRefreshInterval = 0
+	_, secret7, err := p.CallAccessKey("AKID", remoteAddr, true)
+	require.NoError(t, err)
+	assert.Equal(t, "AKID-rotated-again", secret7)
+
+	// Check a cached entry is revalidated with the proxy once it is
+	// old enough even without a refresh being asked for
+	oldRevalidate := accessKeyRevalidateInterval
+	defer func() { accessKeyRevalidateInterval = oldRevalidate }()
+	t.Setenv("RCLONE_TEST_PROXY_REVOKED", "AKID")
+	_, secret8, err := p.CallAccessKey("AKID", remoteAddr, false)
+	require.NoError(t, err, "cached entry should still be trusted")
+	assert.Equal(t, "AKID-rotated-again", secret8)
+	accessKeyRevalidateInterval = 0
+	_, _, err = p.CallAccessKey("AKID", remoteAddr, false)
+	require.ErrorContains(t, err, "revoked")
+}

cmd/serve/s3/s3.go

@@ -120,7 +120,7 @@ var Command = &cobra.Command{
 	},
 	Use:   "s3 remote:path",
 	Short: `Serve remote:path over s3.`,
-	Long:  help() + strings.TrimSpace(httplib.AuthHelp(flagPrefix)+httplib.Help(flagPrefix)+vfs.Help()),
+	Long:  help() + strings.TrimSpace(httplib.AuthHelp(flagPrefix)+httplib.Help(flagPrefix)+vfs.Help()+proxy.Help),
 	RunE: func(command *cobra.Command, args []string) error {
 		var f fs.Fs
 		if proxy.Opt.AuthProxy == "" {

cmd/serve/s3/s3_test.go

@@ -20,6 +20,7 @@ import (
 	v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4"
 	"github.com/minio/minio-go/v7"
 	"github.com/minio/minio-go/v7/pkg/credentials"
+	"github.com/rclone/gofakes3/signature"
 	_ "github.com/rclone/rclone/backend/local"
 	_ "github.com/rclone/rclone/backend/s3" // for TestS3Minio backing remote
 	"github.com/rclone/rclone/cmd/serve/proxy"
@@ -42,11 +43,19 @@ const (
 )
 
 // Configure and serve the server
+//
+// If f is nil the server is expected to be using an auth proxy, in
+// which case the credentials are handed to the test proxy via the
+// environment rather than --auth-key.
 func serveS3(t *testing.T, f fs.Fs) (testURL string, keyid string, keysec string, w *Server) {
 	keyid = random.String(16)
 	keysec = random.String(16)
 	opt := Opt // copy default options
-	opt.AuthKey = []string{fmt.Sprintf("%s,%s", keyid, keysec)}
+	if f == nil {
+		t.Setenv("RCLONE_TEST_PROXY_AUTH_KEY", fmt.Sprintf("%s,%s", keyid, keysec))
+	} else {
+		opt.AuthKey = []string{fmt.Sprintf("%s,%s", keyid, keysec)}
+	}
 	opt.HTTP.ListenAddr = []string{endpoint}
 	w, _ = newServer(context.Background(), f, &opt, &vfscommon.Opt, &proxy.Opt)
 	go func() {
@@ -286,9 +295,6 @@ func TestListBucketsAuthProxy(t *testing.T) {
 		{
 			description: "list buckets",
 			bucket:      "mybucket",
-			// request with random keyid
-			// instead of what was set in 'authPair'
-			keyID: random.String(16),
 			files: []FileStuct{
 				{
 					path:     "",
@@ -300,6 +306,12 @@ func TestListBucketsAuthProxy(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "list buckets: unknown s3 key",
+			bucket:      "mybucket",
+			keyID:       random.String(16),
+			shouldFail:  true,
+		},
 		{
 			description: "list buckets: wrong s3 secret",
 			bucket:      "mybucket",
@@ -339,6 +351,85 @@ func TestNewServerPerServerAuthProxy(t *testing.T) {
 	assert.Nil(t, w.provider.VFS(), "expected no fixed VFS when auth proxy is in use")
 }
 
+// TestAuthProxyEmptySecret checks that a request for an arbitrary
+// access key ID signed with an empty secret is refused when using an
+// auth proxy without --auth-key.
+func TestAuthProxyEmptySecret(t *testing.T) {
+	fstest.Initialise()
+
+	prog, err := filepath.Abs("../servetest/proxy_code.go")
+	require.NoError(t, err)
+	files, err := filepath.Abs("testdata")
+	require.NoError(t, err)
+	proxy.Opt.AuthProxy = "go run " + prog + " " + files
+	defer func() {
+		proxy.Opt.AuthProxy = ""
+	}()
+
+	endpoint, keyid, _, s := serveS3(t, nil)
+	defer func() {
+		assert.NoError(t, s.server.Shutdown())
+	}()
+
+	for _, accessKeyID := range []string{keyid, random.String(16)} {
+		req, err := http.NewRequest("GET", endpoint+"/", nil)
+		require.NoError(t, err)
+		req.Header.Set("X-Amz-Content-Sha256", "UNSIGNED-PAYLOAD")
+		signer := v4.NewSigner()
+		err = signer.SignHTTP(context.Background(), aws.Credentials{AccessKeyID: accessKeyID, SecretAccessKey: ""}, req, "UNSIGNED-PAYLOAD", "s3", "us-east-1", time.Now())
+		require.NoError(t, err)
+
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		body, _ := io.ReadAll(resp.Body)
+		_ = resp.Body.Close()
+		assert.Equal(t, http.StatusForbidden, resp.StatusCode, string(body))
+		assert.NotContains(t, string(body), "ListAllMyBucketsResult")
+	}
+}
+
+// TestAuthProxyKeysNotRegistered checks that secrets supplied by the
+// auth proxy are never registered in gofakes3's process wide key
+// store where any other serve s3 instance in the process would honour
+// them.
+func TestAuthProxyKeysNotRegistered(t *testing.T) {
+	fstest.Initialise()
+
+	prog, err := filepath.Abs("../servetest/proxy_code.go")
+	require.NoError(t, err)
+	files, err := filepath.Abs("testdata")
+	require.NoError(t, err)
+	proxy.Opt.AuthProxy = "go run " + prog + " " + files
+	defer func() {
+		proxy.Opt.AuthProxy = ""
+	}()
+
+	endpoint, keyid, keysec, s := serveS3(t, nil)
+	defer func() {
+		assert.NoError(t, s.server.Shutdown())
+	}()
+
+	sign := func() *http.Request {
+		req, err := http.NewRequest("GET", endpoint+"/", nil)
+		require.NoError(t, err)
+		req.Header.Set("X-Amz-Content-Sha256", "UNSIGNED-PAYLOAD")
+		signer := v4.NewSigner()
+		err = signer.SignHTTP(context.Background(), aws.Credentials{AccessKeyID: keyid, SecretAccessKey: keysec}, req, "UNSIGNED-PAYLOAD", "s3", "us-east-1", time.Now())
+		require.NoError(t, err)
+		return req
+	}
+
+	// A correctly signed request is accepted by the server
+	resp, err := http.DefaultClient.Do(sign())
+	require.NoError(t, err)
+	body, _ := io.ReadAll(resp.Body)
+	_ = resp.Body.Close()
+	assert.Equal(t, http.StatusOK, resp.StatusCode, string(body))
+
+	// But the key it used must not be in the global key store
+	assert.NotEqual(t, signature.ErrNone, signature.V4SignVerify(sign()), "proxy secret was registered in the gofakes3 key store")
+}
+
 // TestAuthKeyPerServer checks that two servers in the same process
 // with different --auth-key pairs only accept their own credentials.
 func TestAuthKeyPerServer(t *testing.T) {

cmd/serve/s3/serve_s3.md

@@ -13,6 +13,12 @@ docs](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html)).
 `--auth-key` is not provided then `serve s3` will allow anonymous
 access.
 
+Alternatively `--auth-proxy` can be used to look up the secret for each
+access key ID and choose the backend it maps to (see [Auth
+Proxy](#auth-proxy) below). When an auth proxy is in use `--auth-key`
+is ignored and every request must be signed with the secret the proxy
+returns for its access key ID.
+
 Like all rclone flags `--auth-key` can be set via environment
 variables, in this case `RCLONE_AUTH_KEY`. Since this flag can be
 repeated, the input to `RCLONE_AUTH_KEY` is CSV encoded. Because the

cmd/serve/s3/server.go

@@ -3,14 +3,11 @@ package s3
 
 import (
 	"context"
-	"crypto/md5"
-	"encoding/hex"
 	"errors"
 	"fmt"
 	"math/rand"
 	"net"
 	"net/http"
-	"strings"
 	"time"
 
 	"github.com/go-chi/chi/v5"
@@ -40,7 +37,6 @@ type Server struct {
 	backend      *s3Backend
 	handler      http.Handler
 	ctx          context.Context // for global config
-	s3Secret     string
 	etagHashType hash.Type
 }
 
@@ -76,14 +72,20 @@ func newServer(ctx context.Context, f fs.Fs, opt *Options, vfsOpt *vfscommon.Opt
 
 	if len(opt.AuthKey) == 0 && !w.provider.IsProxy() {
 		fs.Logf("serve s3", "No auth provided so allowing anonymous access")
-	} else if len(opt.AuthKey) > 0 {
-		w.s3Secret = getAuthSecret(opt.AuthKey)
 	}
 
 	authList, err := authlistResolver(opt.AuthKey)
 	if err != nil {
 		return nil, fmt.Errorf("parsing auth list failed: %q", err)
 	}
+	if w.provider.IsProxy() {
+		// The proxy middleware authenticates every request itself so
+		// gofakes3's own auth must be left empty.
+		if len(authList) > 0 {
+			fs.Logf("serve s3", "--auth-key is ignored when --auth-proxy is set - the proxy must supply the secret for each access key ID")
+		}
+		authList = nil
+	}
 
 	w.backend = newBackend(w)
 	if w.opt.MultipartExpiry > 0 {
@@ -104,9 +106,7 @@ func newServer(ctx context.Context, f fs.Fs, opt *Options, vfsOpt *vfscommon.Opt
 	w.handler = w.faker.Server()
 
 	if w.provider.IsProxy() {
-		// proxy auth middleware
 		w.handler = proxyAuthMiddleware(w.handler, w)
-		w.handler = authPairMiddleware(w.handler, w)
 	} else if len(opt.AuthKey) > 0 {
 		w.faker.AddAuthKeys(authList)
 	}
@@ -142,13 +142,41 @@ func (w *Server) getVFS(ctx context.Context) (VFS *vfs.VFS, err error) {
 	return VFS, nil
 }
 
-// auth does proxy authorization
-func (w *Server) auth(r *http.Request, accessKeyID string) (value any, err error) {
-	VFS, _, err := w.provider.Proxy().Call(stringToMd5Hash(accessKeyID), accessKeyID, false, r.RemoteAddr)
+// auth authenticates the request via the auth proxy.
+//
+// The proxy maps the access key ID to a VFS and a secret access key
+// and the request's signature is verified against that secret. If it
+// fails against a cached secret the proxy is consulted again in case
+// the secret has been rotated.
+//
+// The secret is only ever used here and is never registered with
+// gofakes3, whose key store is shared by every instance in the
+// process.
+func (w *Server) auth(r *http.Request, accessKeyID string) (VFS *vfs.VFS, err error) {
+	p := w.provider.Proxy()
+	VFS, secret, err := p.CallAccessKey(accessKeyID, r.RemoteAddr, false)
 	if err != nil {
 		return nil, err
 	}
-	return VFS, err
+	errCode := signature.V4SignVerifyWithSecret(r, secret)
+	if errCode == signature.ErrNone {
+		return VFS, nil
+	}
+	// Only a signature mismatch can be cured by a rotated secret, so
+	// only then is the proxy worth consulting again - other failures
+	// (expired request, bad date, missing headers) must not make
+	// every bad request run the proxy.
+	if signature.GetAPIError(errCode).Code == "SignatureDoesNotMatch" {
+		VFS, secret, err = p.CallAccessKey(accessKeyID, r.RemoteAddr, true)
+		if err != nil {
+			return nil, err
+		}
+		errCode = signature.V4SignVerifyWithSecret(r, secret)
+		if errCode == signature.ErrNone {
+			return VFS, nil
+		}
+	}
+	return nil, fmt.Errorf("signature verification failed: %s", signature.GetAPIError(errCode).Code)
 }
 
 // Bind register the handler to http.Router
@@ -177,59 +205,49 @@ func (w *Server) Shutdown() error {
 	return err
 }
 
-func authPairMiddleware(next http.Handler, ws *Server) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		accessKey, _ := parseAccessKeyID(r)
-		// set the auth pair
-		authPair := map[string]string{
-			accessKey: ws.s3Secret,
-		}
-		ws.faker.AddAuthKeys(authPair)
-		next.ServeHTTP(w, r)
-	})
-}
-
+// proxyAuthMiddleware authenticates each request via the auth proxy,
+// storing the VFS it returns in the request context, and refuses
+// requests the proxy does not accept.
 func proxyAuthMiddleware(next http.Handler, ws *Server) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		accessKey, _ := parseAccessKeyID(r)
-		value, err := ws.auth(r, accessKey)
+		accessKey, errCode := parseAccessKeyID(r)
+		if errCode != signature.ErrNone {
+			fs.Infof(r.URL.Path, "%s: Auth failed: no access key ID in request", r.RemoteAddr)
+			accessDenied(w)
+			return
+		}
+		VFS, err := ws.auth(r, accessKey)
 		if err != nil {
 			fs.Infof(r.URL.Path, "%s: Auth failed: %v", r.RemoteAddr, err)
+			accessDenied(w)
+			return
 		}
-		if value != nil {
-			r = r.WithContext(context.WithValue(r.Context(), ctxKeyID, value))
-		}
-
+		r = r.WithContext(context.WithValue(r.Context(), ctxKeyID, VFS))
 		next.ServeHTTP(w, r)
 	})
 }
 
-func parseAccessKeyID(r *http.Request) (accessKey string, error signature.ErrorCode) {
-	v4Auth := r.Header.Get("Authorization")
-	req, err := signature.ParseSignV4(v4Auth)
-	if err != signature.ErrNone {
-		return "", err
-	}
+// accessDenied writes an S3 AccessDenied error response
+func accessDenied(w http.ResponseWriter) {
+	w.Header().Set("Content-Type", "application/xml")
+	w.WriteHeader(http.StatusForbidden)
+	_, _ = w.Write([]byte(`<?xml version="1.0" encoding="UTF-8"?><Error><Code>AccessDenied</Code><Message>Access Denied</Message></Error>`))
+}
 
+// parseAccessKeyID returns the access key ID from the request's
+// Authorization header or presigned URL query parameters
+func parseAccessKeyID(r *http.Request) (accessKey string, errCode signature.ErrorCode) {
+	v4Auth := r.Header.Get("Authorization")
+	if v4Auth == "" {
+		// Presigned URLs carry the credential in the query string
+		q := r.URL.Query()
+		if q.Get("X-Amz-Signature") != "" {
+			v4Auth = fmt.Sprintf("%s Credential=%s, SignedHeaders=%s, Signature=%s", q.Get("X-Amz-Algorithm"), q.Get("X-Amz-Credential"), q.Get("X-Amz-SignedHeaders"), q.Get("X-Amz-Signature"))
+		}
+	}
+	req, errCode := signature.ParseSignV4(v4Auth)
+	if errCode != signature.ErrNone {
+		return "", errCode
+	}
 	return req.Credential.GetAccessKey(), signature.ErrNone
 }
-
-func stringToMd5Hash(s string) string {
-	hasher := md5.New()
-	hasher.Write([]byte(s))
-	return hex.EncodeToString(hasher.Sum(nil))
-}
-
-func getAuthSecret(authPair []string) string {
-	if len(authPair) == 0 {
-		return ""
-	}
-
-	splited := strings.Split(authPair[0], ",")
-	if len(splited) != 2 {
-		return ""
-	}
-
-	secret := strings.TrimSpace(splited[1])
-	return secret
-}

cmd/serve/servetest/proxy_code.go

@@ -1,12 +1,18 @@
 //go:build ignore
 
 // A simple auth proxy for testing purposes
+//
+// For S3 access key auth (no "pass" or "public_key" in the input) the
+// access key ID and secret are read from the environment variable
+// RCLONE_TEST_PROXY_AUTH_KEY as "accessKeyID,secretAccessKey" and any
+// other access key ID is refused.
 package main
 
 import (
 	"encoding/json"
 	"log"
 	"os"
+	"strings"
 )
 
 func main() {
@@ -28,6 +34,19 @@ func main() {
 		"_root":    root,
 		"_obscure": "pass",
 	}
+
+	_, havePass := in["pass"]
+	_, havePublicKey := in["public_key"]
+	if !havePass && !havePublicKey {
+		accessKeyID, secret, ok := strings.Cut(os.Getenv("RCLONE_TEST_PROXY_AUTH_KEY"), ",")
+		if !ok {
+			log.Fatal("RCLONE_TEST_PROXY_AUTH_KEY not set")
+		}
+		if in["user"] != accessKeyID {
+			log.Fatalf("unknown access key ID %q", in["user"])
+		}
+		out["_secret_access_key"] = secret
+	}
 	json.NewEncoder(os.Stdout).Encode(&out)
 	if err != nil {
 		log.Fatal(err)