From e8e883c35e20d14e4bd6cdc4bbb0e6cd7df6b853 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 24 Aug 2026 09:37:37 +0100 Subject: [PATCH] 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 | 180 +++++++++++++++++++++++++++--- cmd/serve/proxy/proxy_code.go | 19 ++++ cmd/serve/proxy/proxy_test.go | 132 +++++++++++++++++++++- cmd/serve/s3/s3.go | 2 +- cmd/serve/s3/s3_test.go | 99 +++++++++++++++- cmd/serve/s3/serve_s3.md | 6 + cmd/serve/s3/server.go | 130 +++++++++++---------- cmd/serve/servetest/proxy_code.go | 19 ++++ 8 files changed, 510 insertions(+), 77 deletions(-) diff --git a/cmd/serve/proxy/proxy.go b/cmd/serve/proxy/proxy.go index 29532bf04..5718588b3 100644 --- a/cmd/serve/proxy/proxy.go +++ b/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) diff --git a/cmd/serve/proxy/proxy_code.go b/cmd/serve/proxy/proxy_code.go index cc703e209..4fe45508b 100644 --- a/cmd/serve/proxy/proxy_code.go +++ b/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) diff --git a/cmd/serve/proxy/proxy_test.go b/cmd/serve/proxy/proxy_test.go index 5f347284e..411288773 100644 --- a/cmd/serve/proxy/proxy_test.go +++ b/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") +} diff --git a/cmd/serve/s3/s3.go b/cmd/serve/s3/s3.go index 94cb75ca5..5dd9df778 100644 --- a/cmd/serve/s3/s3.go +++ b/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 == "" { diff --git a/cmd/serve/s3/s3_test.go b/cmd/serve/s3/s3_test.go index f789fc6e4..dbec07a8e 100644 --- a/cmd/serve/s3/s3_test.go +++ b/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) { diff --git a/cmd/serve/s3/serve_s3.md b/cmd/serve/s3/serve_s3.md index 4ba573b88..495efb80a 100644 --- a/cmd/serve/s3/serve_s3.md +++ b/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 diff --git a/cmd/serve/s3/server.go b/cmd/serve/s3/server.go index 6b5a58f74..eedf65c59 100644 --- a/cmd/serve/s3/server.go +++ b/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(`AccessDeniedAccess Denied`)) +} +// 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 -} diff --git a/cmd/serve/servetest/proxy_code.go b/cmd/serve/servetest/proxy_code.go index 25499b6bc..10e9a3f98 100644 --- a/cmd/serve/servetest/proxy_code.go +++ b/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)