filer: fix credential handling on the proxyChunkId path (#10434)

* filer: claim the base fid when minting a volume read token

GenJwtForVolumeServer stamped the fid verbatim, but the volume server strips a
trailing _N delta suffix before comparing the claim, so a token minted for a
batch-assigned fid like 3,01637037d6_1 was checked against 3,01637037d6 and
never matched. Reading such a chunk through the filer returned 401 wherever
jwt.signing.read.key was configured.

Strip the suffix before minting, via a helper shared with the proxyChunkId
validation that was already doing the same thing inline.

* filer: don't mint a volume write token for an anonymous proxy caller

The ?proxyChunkId= branch dispatches and returns before the JWT gate, so
whatever credential the proxy attaches is reachable without authentication.
It attached a token from maybeGetVolumeReadJwtAuthorizationToken, which fell
back to the write signing key when jwt.signing.read.key was unset -- the
configuration scaffold/security.toml recommends for a filer, since read JWTs
are only supported in a master+volume setup. An anonymous
DELETE /?proxyChunkId=<fid> therefore arrived at the volume server holding a
write-key token scoped to that fid, and the volume server honored it.

Sign read tokens with the read key only. The fallback bought nothing on a
read anyway: a volume server enforces read JWTs solely when that same key is
set, so when the fallback fired the read was unchecked regardless.

Mint only for reads. Writers proxied through the filer carry their own volume
JWT from AssignVolume, forwarded with the rest of the caller's headers, so
weed mount -filerProxy uploads are unaffected. Moving the dispatch below the
JWT gate instead would have broken them, since that token is signed with
jwt.signing rather than jwt.filer_signing.

On a read with nothing to mint, drop the caller's Authorization rather than
relaying it: there it is a filer credential, and forwarding it would hand a
volume server a token it never used to see.

* filer: keep proxied writes out of the read concurrency semaphore

The semaphore is named and documented for reads -- it exists so replication
bursts can't open hundreds of connections to one volume server -- but it was
applied to every proxied method. A write queued behind sixteen in-flight reads
can wait past the 10s default expiry of the AssignVolume token it carries, and
the volume server then answers 401. shouldReassignUpload treats a 4xx as final,
so the uploader replays the same expired token instead of re-assigning and the
write fails up to the caller.

This only became reachable once the filer stopped re-minting a fresh token
after the wait.

weed/server/filer_server_handlers_proxy.go

@@ -50,11 +50,20 @@ func releaseProxySemaphore(host string) {
 	}
 }
 
+// isProxyReadMethod reports whether a proxied request only reads. Everything
+// else is treated as a write for both credential and concurrency purposes.
+func isProxyReadMethod(method string) bool {
+	return method == http.MethodGet || method == http.MethodHead
+}
+
 // baseFileId strips the trailing _N delta suffix that batch assigns append to a
 // fid, and only that: the suffix must be a non-empty run of digits, otherwise
-// the fid is returned whole for the caller to reject.
+// the fid is returned whole for the caller to reject. The volume server compares
+// a JWT's fid claim against the stripped form (see
+// VolumeServer.maybeCheckJwtAuthorization), so anything minting or parsing a fid
+// on this side has to agree with it.
 //
-// The volume server strips at the last "_" unconditionally, which is safe there
+// That server strips at the last "_" unconditionally, which is safe there
 // because its fid already came out of a path the mux parsed and so cannot hold
 // a "/". Here the value is raw query input, and an unguarded strip would reduce
 // "3,01637037d6_1/../../status" to a valid fid and wave the traversal through.
@@ -106,10 +115,17 @@ func (fs *FilerServer) proxyToVolumeServer(w http.ResponseWriter, r *http.Reques
 		return
 	}
 
-	// urlStrings from LookupFileId already contain the fileId in the path
+	fs.proxyToVolumeServerURL(w, r, fileId, urlStrings[rand.IntN(len(urlStrings))])
+}
+
+// proxyToVolumeServerURL forwards the request to one already-resolved volume
+// server URL.
+func (fs *FilerServer) proxyToVolumeServerURL(w http.ResponseWriter, r *http.Request, fileId, targetURL string) {
+	ctx := r.Context()
+
+	// targetURL from LookupFileId already contains the fileId in the path
 	// (e.g. http://server:8080/6,08136bdce4). Forward the caller's query params
 	// (e.g. readDeleted=true from weed mount) but drop the internal proxyChunkId.
-	targetURL := urlStrings[rand.IntN(len(urlStrings))]
 	query := r.URL.Query()
 	query.Del("proxyChunkId")
 	if encoded := query.Encode(); encoded != "" {
@@ -123,14 +139,20 @@ func (fs *FilerServer) proxyToVolumeServer(w http.ResponseWriter, r *http.Reques
 		return
 	}
 
-	// Limit concurrent requests per volume server to prevent overload
-	volumeHost := proxyReq.URL.Host
-	if err := acquireProxySemaphore(ctx, volumeHost); err != nil {
-		glog.V(0).InfofCtx(ctx, "proxy to %s cancelled while waiting: %v", volumeHost, err)
-		w.WriteHeader(http.StatusServiceUnavailable)
-		return
+	// Limit concurrent reads per volume server to prevent overload. Writes are
+	// deliberately exempt: a proxied write carries the caller's AssignVolume
+	// token, which expires 10s after the assign by default, so queueing one here
+	// can push it past expiry and turn it into a 401 the uploader does not
+	// re-assign on.
+	if isProxyReadMethod(r.Method) {
+		volumeHost := proxyReq.URL.Host
+		if err := acquireProxySemaphore(ctx, volumeHost); err != nil {
+			glog.V(0).InfofCtx(ctx, "proxy to %s cancelled while waiting: %v", volumeHost, err)
+			w.WriteHeader(http.StatusServiceUnavailable)
+			return
+		}
+		defer releaseProxySemaphore(volumeHost)
 	}
-	defer releaseProxySemaphore(volumeHost)
 
 	proxyReq.Header.Set("Host", r.Host)
 	proxyReq.Header.Set("X-Forwarded-For", r.RemoteAddr)
@@ -142,9 +164,24 @@ func (fs *FilerServer) proxyToVolumeServer(w http.ResponseWriter, r *http.Reques
 		}
 	}
 
-	// volume server may require a read JWT even though the proxy endpoint doesn't
-	if jwt := fs.maybeGetVolumeReadJwtAuthorizationToken(fileId); jwt != "" {
-		proxyReq.Header.Set("Authorization", security.BearerPrefix+jwt)
+	// Decide the volume credential explicitly rather than letting the copied
+	// header stand, because the two directions need opposite handling.
+	//
+	// Reads: the volume server may require a read JWT even though the proxy
+	// endpoint doesn't, so mint one. When there is nothing to mint, drop the
+	// caller's Authorization instead of relaying it -- on this path it is a
+	// filer credential, and a volume server has no business seeing one.
+	//
+	// Writes: never mint. This branch runs ahead of the filer's JWT gate, so a
+	// token minted here would be signed for an unauthenticated caller. A
+	// legitimate writer already carries its own volume JWT from AssignVolume,
+	// so that one is forwarded untouched.
+	if isProxyReadMethod(r.Method) {
+		if jwt := fs.maybeGetVolumeReadJwtAuthorizationToken(fileId); jwt != "" {
+			proxyReq.Header.Set("Authorization", security.BearerPrefix+jwt)
+		} else {
+			proxyReq.Header.Del("Authorization")
+		}
 	}
 
 	proxyResponse, postErr := util_http.GetGlobalHttpClient().Do(proxyReq)

weed/server/filer_server_handlers_proxy_test.go

@@ -4,12 +4,240 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
+
+	"github.com/seaweedfs/seaweedfs/weed/security"
 )
 
+const (
+	proxyTestWriteKey = "cluster-write-key"
+	proxyTestReadKey  = "cluster-read-key"
+	proxyTestVid      = "3"
+	proxyTestFid      = "01637037d6"
+	proxyTestFileId   = proxyTestVid + "," + proxyTestFid
+)
+
+// proxyTestVolume is a stand-in volume server that records what the filer
+// actually sent. Recording arrival separately from the header is what keeps the
+// negative assertions honest: an absent Authorization and a request that never
+// left the filer are otherwise indistinguishable.
+type proxyTestVolume struct {
+	*httptest.Server
+	hits atomic.Int32
+	auth atomic.Value // string
+}
+
+func newProxyTestVolume(t *testing.T) *proxyTestVolume {
+	t.Helper()
+	v := &proxyTestVolume{}
+	v.auth.Store("")
+	v.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		v.hits.Add(1)
+		v.auth.Store(r.Header.Get("Authorization"))
+	}))
+	t.Cleanup(func() {
+		v.Close()
+		// proxyToVolumeServerURL keys the semaphore map by host, and every
+		// httptest server binds a fresh port; drop ours so -count=N runs do not
+		// grow the map without bound.
+		if u, err := url.Parse(v.URL); err == nil {
+			proxySemaphores.Delete(u.Host)
+		}
+	})
+	return v
+}
+
+func (v *proxyTestVolume) seenAuth() string { return v.auth.Load().(string) }
+
+func (v *proxyTestVolume) requireReached(t *testing.T) {
+	t.Helper()
+	if v.hits.Load() == 0 {
+		t.Fatal("request never reached the volume server, so the assertion below proves nothing")
+	}
+}
+
+// Everything the filer can hand a caller on the proxy path is reachable without
+// authentication, because the branch runs ahead of the filer's JWT gate. With
+// only a write key configured it must therefore mint nothing at all.
+func TestProxyMintsNothingWithoutReadKey(t *testing.T) {
+	fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, "", 10)}
+
+	if jwt := fs.maybeGetVolumeReadJwtAuthorizationToken(proxyTestFileId); jwt != "" {
+		t.Fatalf("minted %q with no read key configured", jwt)
+	}
+}
+
+// A configured read key still yields a read token, and it stays read-only.
+func TestProxyReadTokenIsReadOnly(t *testing.T) {
+	fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+
+	jwt := fs.maybeGetVolumeReadJwtAuthorizationToken(proxyTestFileId)
+	if jwt == "" {
+		t.Fatal("no read token minted despite a configured read key")
+	}
+
+	vs := &VolumeServer{guard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+
+	read := httptest.NewRequest(http.MethodGet, "http://volume:8080/"+proxyTestFileId, nil)
+	read.Header.Set("Authorization", security.BearerPrefix+jwt)
+	if !vs.maybeCheckJwtAuthorization(read, proxyTestVid, proxyTestFid, false) {
+		t.Fatal("read token rejected on a read")
+	}
+
+	write := httptest.NewRequest(http.MethodDelete, "http://volume:8080/"+proxyTestFileId, nil)
+	write.Header.Set("Authorization", security.BearerPrefix+jwt)
+	if vs.maybeCheckJwtAuthorization(write, proxyTestVid, proxyTestFid, true) {
+		t.Fatal("read token authorized a write")
+	}
+}
+
+// Writes must reach the volume server carrying the caller's own AssignVolume
+// token and nothing else. POST is the method every in-tree proxied uploader
+// actually sends, so it leads the table.
+func TestProxyWriteCarriesOnlyCallerCredential(t *testing.T) {
+	callerToken := security.BearerPrefix + string(security.GenJwtForVolumeServer(security.SigningKey(proxyTestWriteKey), 10, proxyTestFileId))
+
+	for _, tc := range []struct {
+		name    string
+		method  string
+		readKey string
+		sent    string
+		want    string
+	}{
+		{"anonymous post", http.MethodPost, "", "", ""},
+		{"anonymous post with read key", http.MethodPost, proxyTestReadKey, "", ""},
+		{"anonymous delete", http.MethodDelete, "", "", ""},
+		{"anonymous delete with read key", http.MethodDelete, proxyTestReadKey, "", ""},
+		{"anonymous put with read key", http.MethodPut, proxyTestReadKey, "", ""},
+		{"caller token forwarded on post", http.MethodPost, proxyTestReadKey, callerToken, callerToken},
+		{"caller token forwarded on delete", http.MethodDelete, "", callerToken, callerToken},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			volume := newProxyTestVolume(t)
+			fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, tc.readKey, 10)}
+
+			r := httptest.NewRequest(tc.method, "http://filer:8888/?proxyChunkId="+proxyTestFileId, nil)
+			if tc.sent != "" {
+				r.Header.Set("Authorization", tc.sent)
+			}
+			fs.proxyToVolumeServerURL(httptest.NewRecorder(), r, proxyTestFileId, volume.URL+"/"+proxyTestFileId)
+
+			volume.requireReached(t)
+			if got := volume.seenAuth(); got != tc.want {
+				t.Fatalf("volume server saw Authorization %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+// Reads keep the minted token so weed mount can read through the proxy against a
+// volume server that enforces read JWTs -- and the minted token must *replace*
+// whatever the caller sent, not be appended alongside it.
+func TestProxyReadReplacesCallerCredential(t *testing.T) {
+	volume := newProxyTestVolume(t)
+	fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+
+	r := httptest.NewRequest(http.MethodGet, "http://filer:8888/?proxyChunkId="+proxyTestFileId, nil)
+	r.Header.Set("Authorization", security.BearerPrefix+"caller-supplied-token")
+	fs.proxyToVolumeServerURL(httptest.NewRecorder(), r, proxyTestFileId, volume.URL+"/"+proxyTestFileId)
+
+	volume.requireReached(t)
+	seen := volume.seenAuth()
+	if seen == security.BearerPrefix+"caller-supplied-token" {
+		t.Fatal("caller's token reached the volume server instead of the minted one")
+	}
+
+	vs := &VolumeServer{guard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+	check := httptest.NewRequest(http.MethodGet, "http://volume:8080/"+proxyTestFileId, nil)
+	check.Header.Set("Authorization", seen)
+	if !vs.maybeCheckJwtAuthorization(check, proxyTestVid, proxyTestFid, false) {
+		t.Fatalf("forwarded token %q did not authorize the read", seen)
+	}
+}
+
+// With no read key there is nothing to mint, and the caller's Authorization on
+// the read path is a filer credential -- it must be dropped, not relayed to a
+// volume server that has no business seeing it.
+func TestProxyReadDropsCallerCredentialWhenNothingMinted(t *testing.T) {
+	for _, method := range []string{http.MethodGet, http.MethodHead} {
+		t.Run(method, func(t *testing.T) {
+			volume := newProxyTestVolume(t)
+			fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, "", 10)}
+
+			r := httptest.NewRequest(method, "http://filer:8888/?proxyChunkId="+proxyTestFileId, nil)
+			r.Header.Set("Authorization", security.BearerPrefix+"filer-credential")
+			fs.proxyToVolumeServerURL(httptest.NewRecorder(), r, proxyTestFileId, volume.URL+"/"+proxyTestFileId)
+
+			volume.requireReached(t)
+			if got := volume.seenAuth(); got != "" {
+				t.Fatalf("volume server saw Authorization %q, want it dropped", got)
+			}
+		})
+	}
+}
+
+// Writes must not queue behind the read semaphore: a proxied write carries an
+// AssignVolume token that expires 10s after the assign by default, and waiting
+// for a read slot can push it past expiry.
+func TestProxyWriteBypassesReadSemaphore(t *testing.T) {
+	volume := newProxyTestVolume(t)
+	host := volume.Listener.Addr().String()
+
+	// Fill every read slot for this host and never release them.
+	for i := 0; i < proxyReadConcurrencyPerVolumeServer; i++ {
+		if err := acquireProxySemaphore(context.Background(), host); err != nil {
+			t.Fatalf("fill slot %d: %v", i, err)
+		}
+	}
+	defer func() {
+		for i := 0; i < proxyReadConcurrencyPerVolumeServer; i++ {
+			releaseProxySemaphore(host)
+		}
+	}()
+
+	fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, "", 10)}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+	r := httptest.NewRequest(http.MethodPost, "http://filer:8888/?proxyChunkId="+proxyTestFileId, nil).WithContext(ctx)
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		fs.proxyToVolumeServerURL(httptest.NewRecorder(), r, proxyTestFileId, volume.URL+"/"+proxyTestFileId)
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(3 * time.Second):
+		t.Fatal("proxied write blocked on the read semaphore")
+	}
+	volume.requireReached(t)
+}
+
+// The volume server strips a _N delta suffix before comparing the fid claim, so
+// a token minted for the suffixed form would never validate.
+func TestProxyReadTokenMatchesDeltaFid(t *testing.T) {
+	const deltaFileId = proxyTestFileId + "_1"
+
+	fs := &FilerServer{volumeGuard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+	jwt := fs.maybeGetVolumeReadJwtAuthorizationToken(deltaFileId)
+	if jwt == "" {
+		t.Fatal("no read token minted for a delta fid")
+	}
+
+	vs := &VolumeServer{guard: security.NewGuard([]string{}, proxyTestWriteKey, 10, proxyTestReadKey, 10)}
+	r := httptest.NewRequest(http.MethodGet, "http://volume:8080/"+deltaFileId, nil)
+	r.Header.Set("Authorization", security.BearerPrefix+jwt)
+	if !vs.maybeCheckJwtAuthorization(r, proxyTestVid, proxyTestFid+"_1", false) {
+		t.Fatal("token minted for a delta fid did not authorize the read")
+	}
+}
+
 func TestValidateProxyChunkId(t *testing.T) {
 	for _, tc := range []struct {
 		fileId string

weed/server/filer_server_handlers_read.go

@@ -260,16 +260,15 @@ func (fs *FilerServer) GetOrHeadHandler(w http.ResponseWriter, r *http.Request)
 }
 
 func (fs *FilerServer) maybeGetVolumeReadJwtAuthorizationToken(fileId string) string {
-	// Generate a read JWT for volume server access. If the dedicated
-	// read key (jwt.signing.read.key) is not configured, fall back to the
-	// general signing key (jwt.signing.key) so the proxy can still
-	// authenticate to volume servers that require JWT.
+	// Only ever sign with the read key. A volume server enforces read JWTs
+	// solely when jwt.signing.read.key is set, so falling back to the write key
+	// buys no access on a read -- it only hands out a token that would authorize
+	// a write.
 	key := fs.volumeGuard.ReadSigningKey()
-	if len(key) == 0 {
-		key = fs.volumeGuard.SigningKey()
-	}
 	if len(key) == 0 {
 		return ""
 	}
-	return string(security.GenJwtForVolumeServer(key, fs.volumeGuard.ReadExpiresAfterSec(), fileId))
+	// Claim the base fid: the volume server strips a _N delta suffix before
+	// comparing, so a token claiming the suffixed form never matches.
+	return string(security.GenJwtForVolumeServer(key, fs.volumeGuard.ReadExpiresAfterSec(), baseFileId(fileId)))
 }