From 0b5fff2ccd97e1e2b8b86a1812a310bd07add1e0 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 27 Aug 2026 16:25:56 -0700 Subject: [PATCH] filer, s3: reuse the volume server's guarded remote-storage client builder (#10990) * volume: build the guarded remote storage client through a shared helper Fold the endpoint validation, credential check and rebinding-safe dialer that FetchAndWriteNeedle applies before dialing a caller-supplied remote storage endpoint into a single BuildGuardedRemoteStorageClient helper, so other callers that dial the same endpoints can reuse it. No behavior change on this path. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * filer: build the remote-mount stream client through the guarded helper streamFromRemote serves a cold remote-only entry straight from its mounted origin. Build its client through BuildGuardedRemoteStorageClient so the same endpoint checks the volume server applies cover this read path too. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * s3: build the remote-mount stream client through the guarded helper openRemoteStream serves a remote-mounted object straight from its origin when the local read cannot. Build its client through the same guarded helper so the endpoint checks apply here as well. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN --- weed/s3api/s3api_object_handlers.go | 3 +- weed/server/filer_server_handlers_read.go | 2 +- weed/server/volume_grpc_remote.go | 60 ++++++++++++++--------- weed/server/volume_grpc_remote_test.go | 33 +++++++++++++ 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index f45f4166c..2d5542515 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -24,6 +24,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/pb/remote_pb" "github.com/seaweedfs/seaweedfs/weed/remote_storage" "github.com/seaweedfs/seaweedfs/weed/security" + weed_server "github.com/seaweedfs/seaweedfs/weed/server" "github.com/seaweedfs/seaweedfs/weed/util" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" @@ -3337,7 +3338,7 @@ func (s3a *S3ApiServer) openRemoteStream(ctx context.Context, bucket, object str return nil, err } - client, err := remote_storage.GetRemoteStorage(storageConf) + client, err := weed_server.BuildGuardedRemoteStorageClient(ctx, storageConf, false) if err != nil { return nil, err } diff --git a/weed/server/filer_server_handlers_read.go b/weed/server/filer_server_handlers_read.go index e12bf1445..2992f3132 100644 --- a/weed/server/filer_server_handlers_read.go +++ b/weed/server/filer_server_handlers_read.go @@ -286,7 +286,7 @@ func (fs *FilerServer) streamFromRemote(ctx context.Context, dir, name string, o if err != nil { return nil, err } - client, err := remote_storage.GetRemoteStorage(storageConf) + client, err := BuildGuardedRemoteStorageClient(ctx, storageConf, false) if err != nil { return nil, err } diff --git a/weed/server/volume_grpc_remote.go b/weed/server/volume_grpc_remote.go index f4d775469..4eedef097 100644 --- a/weed/server/volume_grpc_remote.go +++ b/weed/server/volume_grpc_remote.go @@ -350,6 +350,42 @@ func checkGcsCredentials(creds string) error { return nil } +// BuildGuardedRemoteStorageClient builds a remote storage client whose dial +// path is validated against the SSRF deny-list and pinned against DNS +// rebinding, unless allowUntrusted is set. It is the single builder for every +// caller that dials a caller-influenced RemoteConf endpoint: the volume +// FetchAndWriteNeedle write path and the filer and S3 gateway remote-mount read +// paths, which otherwise reach the same s3manager sink unguarded. +func BuildGuardedRemoteStorageClient(ctx context.Context, remoteConf *remote_pb.RemoteConf, allowUntrusted bool) (remote_storage.RemoteStorageClient, error) { + if !allowUntrusted { + if remoteConf.GetType() == "gcs" { + if credsErr := checkGcsCredentials(remoteConf.GetGcsGoogleApplicationCredentials()); credsErr != nil { + return nil, fmt.Errorf("reject remote credentials: %w", credsErr) + } + } + if endpoint, makeClient, ok := guardedRemoteClient(remoteConf); ok { + if validateErr := validateRemoteEndpoint(ctx, endpoint); validateErr != nil { + return nil, fmt.Errorf("reject remote endpoint: %w", validateErr) + } + // Build a one-shot client whose dial path re-validates the resolved + // IP every time. This pins the validated endpoint against DNS + // rebinding (a hostname that resolves to a public IP for + // validateRemoteEndpoint and then flips to 127.0.0.1 / 169.254.x.x + // when the SDK dials). + client, err := makeClient(newGuardedHTTPClient(endpoint)) + if err != nil { + return nil, fmt.Errorf("get remote client: %w", err) + } + return client, nil + } + } + client, err := remote_storage.GetRemoteStorage(remoteConf) + if err != nil { + return nil, fmt.Errorf("get remote client: %w", err) + } + return client, nil +} + func (vs *VolumeServer) FetchAndWriteNeedle(ctx context.Context, req *volume_server_pb.FetchAndWriteNeedleRequest) (resp *volume_server_pb.FetchAndWriteNeedleResponse, err error) { if err := vs.checkGrpcAdminAuth(ctx); err != nil { return nil, err @@ -366,29 +402,9 @@ func (vs *VolumeServer) FetchAndWriteNeedle(ctx context.Context, req *volume_ser remoteConf := req.RemoteConf - if !vs.AllowUntrustedRemoteEndpoints && remoteConf.GetType() == "gcs" { - if credsErr := checkGcsCredentials(remoteConf.GetGcsGoogleApplicationCredentials()); credsErr != nil { - return nil, fmt.Errorf("reject remote credentials: %w", credsErr) - } - } - - var client remote_storage.RemoteStorageClient - var getClientErr error - if endpoint, makeClient, ok := guardedRemoteClient(remoteConf); ok && !vs.AllowUntrustedRemoteEndpoints { - if validateErr := validateRemoteEndpoint(ctx, endpoint); validateErr != nil { - return nil, fmt.Errorf("reject remote endpoint: %w", validateErr) - } - // Build a one-shot client whose dial path re-validates the resolved - // IP every time. This pins the validated endpoint against DNS - // rebinding (a hostname that resolves to a public IP for - // validateRemoteEndpoint and then flips to 127.0.0.1 / 169.254.x.x - // when the SDK dials). - client, getClientErr = makeClient(newGuardedHTTPClient(endpoint)) - } else { - client, getClientErr = remote_storage.GetRemoteStorage(remoteConf) - } + client, getClientErr := BuildGuardedRemoteStorageClient(ctx, remoteConf, vs.AllowUntrustedRemoteEndpoints) if getClientErr != nil { - return nil, fmt.Errorf("get remote client: %w", getClientErr) + return nil, getClientErr } remoteStorageLocation := req.RemoteLocation diff --git a/weed/server/volume_grpc_remote_test.go b/weed/server/volume_grpc_remote_test.go index ddfa4862c..0d3b2bf08 100644 --- a/weed/server/volume_grpc_remote_test.go +++ b/weed/server/volume_grpc_remote_test.go @@ -638,3 +638,36 @@ func TestGuardedReplicaDialerRebind(t *testing.T) { t.Fatalf("private peer must be allowed by the replica dialer, got %v", perr) } } + +// TestBuildGuardedRemoteStorageClient confirms the shared builder refuses a +// caller-influenced endpoint that resolves to a blocked address, and a gcs +// credentials path, while allowUntrusted falls back to the plain builder. +func TestBuildGuardedRemoteStorageClient(t *testing.T) { + loopbackS3 := &remote_pb.RemoteConf{ + Name: "poc", + Type: "s3", + S3Endpoint: "http://127.0.0.1:8000", + S3AccessKey: "k", + S3SecretKey: "s", + S3Region: "us-east-1", + } + if _, err := BuildGuardedRemoteStorageClient(context.Background(), loopbackS3, false); err == nil { + t.Error("expected a loopback s3 endpoint to be rejected") + } else if !strings.Contains(err.Error(), "reject remote endpoint") { + t.Errorf("error = %v, want reject remote endpoint", err) + } + if _, err := BuildGuardedRemoteStorageClient(context.Background(), loopbackS3, true); err != nil { + t.Errorf("allowUntrusted should build the client: %v", err) + } + + gcsPathCreds := &remote_pb.RemoteConf{ + Name: "poc", + Type: "gcs", + GcsGoogleApplicationCredentials: "/etc/hostname", + } + if _, err := BuildGuardedRemoteStorageClient(context.Background(), gcsPathCreds, false); err == nil { + t.Error("expected a gcs credentials path to be rejected") + } else if !strings.Contains(err.Error(), "reject remote credentials") { + t.Errorf("error = %v, want reject remote credentials", err) + } +}