diff --git a/weed/server/filer_server_handlers_proxy.go b/weed/server/filer_server_handlers_proxy.go index 09e52132d..87ab82622 100644 --- a/weed/server/filer_server_handlers_proxy.go +++ b/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) diff --git a/weed/server/filer_server_handlers_proxy_test.go b/weed/server/filer_server_handlers_proxy_test.go index 8fe654b5a..bf7c0a2f2 100644 --- a/weed/server/filer_server_handlers_proxy_test.go +++ b/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 diff --git a/weed/server/filer_server_handlers_read.go b/weed/server/filer_server_handlers_read.go index 48c7342a7..b2f9fe8b5 100644 --- a/weed/server/filer_server_handlers_read.go +++ b/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))) }