From 21054b6c1846dc8574bd0103679d65f9f7671e63 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 12 May 2026 12:35:27 -0700 Subject: [PATCH] volume: fail closed in admin gRPC gate when no whitelist is configured (#9440) Add Guard.IsAdminAuthorized, a fail-closed variant of IsWhiteListed, and use it to gate destructive volume admin RPCs. IsWhiteListed keeps its allow-all-when-empty semantics for HTTP compatibility. For TCP peers with an empty whitelist, off-host callers are rejected but loopback (127.0.0.0/8, ::1) is still trusted. A volume server commonly cohabits with the master/filer on a single host and in integration-test clusters; the loopback exception keeps cluster-internal admin traffic working without -whiteList while still locking out off-host attackers. Non-TCP peers (in-process / bufconn / unix-socket) bypass the host check entirely. When `weed server` runs master+volume+filer in a single process the master dials the volume server in-process and the peer address surfaces as "@", which has no parseable IP. Such a caller shares our OS process and cannot be spoofed by a remote attacker, so we treat it as trusted by construction. The gate also tolerates a nil guard (developmental / embedded path) and only enforces once a guard is wired up. UpdateWhiteList skips entries whose CIDR fails to parse so the IP-iteration path can no longer hit a nil *net.IPNet. --- weed/command/scaffold/security.toml | 6 ++ weed/security/guard.go | 31 +++++++ weed/security/guard_whitelist_test.go | 95 +++++++++++++++++++-- weed/server/volume_grpc_admin.go | 44 ++++++---- weed/server/volume_grpc_admin_test.go | 115 ++++++++++++++++++++++++++ 5 files changed, 264 insertions(+), 27 deletions(-) create mode 100644 weed/server/volume_grpc_admin_test.go diff --git a/weed/command/scaffold/security.toml b/weed/command/scaffold/security.toml index b99c2fd8c4..0ca3da4cf5 100644 --- a/weed/command/scaffold/security.toml +++ b/weed/command/scaffold/security.toml @@ -198,5 +198,11 @@ kek = "" key = "" # white list. It's checking request ip address. +# Destructive volume admin gRPCs (DeleteCollection, VolumeDelete, +# VolumeMarkReadonly, VolumeMarkWritable, VolumeUnmount, VolumeMount, +# VolumeServerLeave, AllocateVolume, VolumeConfigure, EC/vacuum) require a +# populated white_list. If left empty, the volume server rejects those RPCs +# from off-host peers (loopback callers are still trusted); either list the +# admin IPs/CIDRs here or pass them via the -whiteList CLI flag. [guard] white_list = "" diff --git a/weed/security/guard.go b/weed/security/guard.go index ea71fc9540..ae11ca39d4 100644 --- a/weed/security/guard.go +++ b/weed/security/guard.go @@ -36,6 +36,13 @@ Generating JWT: 2. optionally set "exp", "nbf" fields, in Unix time, the number of seconds elapsed since January 1, 1970 UTC. +There are two whitelist check entry points with different defaults: + - IsWhiteListed: allow-all when the whitelist is empty. Kept for HTTP + compatibility (e.g. Guard.WhiteList middleware on read-mostly routes). + - IsAdminAuthorized: fail-closed when the whitelist is empty. Used to gate + destructive admin endpoints so an unconfigured deployment does not + accept them from arbitrary peers. + Referenced: https://github.com/pkieltyka/jwtauth/blob/master/jwtauth.go */ @@ -108,6 +115,10 @@ func (g *Guard) checkWhiteList(w http.ResponseWriter, r *http.Request) error { // IsWhiteListed returns true if the given host IP is allowed by the guard. // When no whitelist is configured (security inactive), all hosts are allowed. +// This allow-all-when-empty behaviour is kept for HTTP compatibility: many +// non-write HTTP routes wrap themselves with Guard.WhiteList and must remain +// reachable on default deployments. Callers gating destructive endpoints must +// use IsAdminAuthorized instead. func (g *Guard) IsWhiteListed(host string) bool { if !g.isWriteActive { return true @@ -129,6 +140,25 @@ func (g *Guard) IsWhiteListed(host string) bool { return false } +// IsAdminAuthorized is the explicit, fail-closed equivalent of IsWhiteListed +// for destructive admin endpoints. Unlike IsWhiteListed, it returns false +// when no whitelist is configured, so callers must opt in by populating the +// whitelist (CLI -whiteList or guard.white_list in security.toml). +// +// Loopback peers are trusted even when the whitelist is empty: a volume +// server commonly cohabits with the master/filer on a single host (and on +// every integration-test cluster), and an in-process loopback caller is +// equivalent to local-host trust. Off-host callers remain denied. +func (g *Guard) IsAdminAuthorized(host string) bool { + if g.isEmptyWhiteList { + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + return false + } + return g.IsWhiteListed(host) +} + func (g *Guard) UpdateWhiteList(whiteList []string) { whiteListIp := make(map[string]struct{}) whiteListCIDR := make(map[string]*net.IPNet) @@ -137,6 +167,7 @@ func (g *Guard) UpdateWhiteList(whiteList []string) { _, cidrnet, err := net.ParseCIDR(ip) if err != nil { glog.Errorf("Parse CIDR %s in whitelist failed: %v", ip, err) + continue } whiteListCIDR[ip] = cidrnet } else { diff --git a/weed/security/guard_whitelist_test.go b/weed/security/guard_whitelist_test.go index 5f304d06ca..a4b77048e6 100644 --- a/weed/security/guard_whitelist_test.go +++ b/weed/security/guard_whitelist_test.go @@ -2,12 +2,11 @@ package security import "testing" -// TestIsWhiteListedEmptyConfigAllowsEverything pins the contract that the -// gRPC admin auth helper relies on: when no whitelist and no signing key -// are configured, IsWhiteListed accepts any host (including the empty -// string and unparseable peer addresses like the gRPC "@" passthrough -// form). Otherwise insecure default deployments would lock themselves out -// of every destructive admin RPC. +// TestIsWhiteListedEmptyConfigAllowsEverything pins the HTTP-side contract: +// when no whitelist and no signing key are configured, IsWhiteListed accepts +// any host. This is the allow-all-when-empty path that Guard.WhiteList +// middleware on read-mostly HTTP routes relies on; the destructive gRPC +// admin gate uses IsAdminAuthorized instead, which is fail-closed. func TestIsWhiteListedEmptyConfigAllowsEverything(t *testing.T) { g := NewGuard(nil, "", 0, "", 0) for _, host := range []string{"", "@", "127.0.0.1", "::1", "garbage:value"} { @@ -38,9 +37,9 @@ func TestIsWhiteListedWithListRejectsUnknown(t *testing.T) { } func TestIsWhiteListedWithSigningKeyButNoWhitelistAllowsAll(t *testing.T) { - // JWT-only mode: signing key set, whitelist empty. Without this branch - // the gRPC admin auth helper would falsely deny in-process callers when - // the operator wires up signing keys but hasn't enumerated IPs. + // JWT-only mode: signing key set, whitelist empty. IsWhiteListed must + // still accept every host because read-side HTTP middleware piggybacks + // on it; admin gating uses IsAdminAuthorized, which is fail-closed. g := NewGuard(nil, "deadbeef", 0, "", 0) for _, host := range []string{"", "@", "127.0.0.1"} { if !g.IsWhiteListed(host) { @@ -48,3 +47,81 @@ func TestIsWhiteListedWithSigningKeyButNoWhitelistAllowsAll(t *testing.T) { } } } + +// TestIsAdminAuthorizedEmptyWhitelistDeniesNonLoopback pins the fail-closed +// contract the volume gRPC admin gate depends on: with no whitelist +// configured every off-host peer is rejected, even when a signing key is +// set. Loopback peers are still trusted (see +// TestIsAdminAuthorizedEmptyWhitelistAllowsLoopback) because a volume server +// commonly cohabits with master/filer on a single host. +func TestIsAdminAuthorizedEmptyWhitelistDeniesNonLoopback(t *testing.T) { + cases := []*Guard{ + NewGuard(nil, "", 0, "", 0), + NewGuard(nil, "deadbeef", 0, "", 0), + NewGuard([]string{}, "", 0, "", 0), + } + for i, g := range cases { + for _, host := range []string{"", "@", "10.0.0.1", "192.168.0.5", "8.8.8.8"} { + if g.IsAdminAuthorized(host) { + t.Errorf("case %d: empty whitelist should deny host=%q", i, host) + } + } + } +} + +// TestIsAdminAuthorizedEmptyWhitelistAllowsLoopback pins the loopback +// exception: when no whitelist is configured, in-process callers from +// 127.0.0.0/8 or ::1 are still authorized. Without this, single-host +// clusters and integration tests (which never set -whiteList) would lose +// every admin RPC the moment fail-closed semantics kicked in. +func TestIsAdminAuthorizedEmptyWhitelistAllowsLoopback(t *testing.T) { + cases := []*Guard{ + NewGuard(nil, "", 0, "", 0), + NewGuard(nil, "deadbeef", 0, "", 0), + NewGuard([]string{}, "", 0, "", 0), + } + for i, g := range cases { + for _, host := range []string{"127.0.0.1", "127.0.0.5", "::1"} { + if !g.IsAdminAuthorized(host) { + t.Errorf("case %d: empty whitelist should still allow loopback host=%q", i, host) + } + } + } +} + +func TestIsAdminAuthorizedWithIPList(t *testing.T) { + g := NewGuard([]string{"10.0.0.1"}, "", 0, "", 0) + cases := []struct { + host string + want bool + }{ + {"10.0.0.1", true}, + {"10.0.0.2", false}, + {"", false}, + {"@", false}, + } + for _, tc := range cases { + if got := g.IsAdminAuthorized(tc.host); got != tc.want { + t.Errorf("IsAdminAuthorized(%q) = %v want %v", tc.host, got, tc.want) + } + } +} + +func TestIsAdminAuthorizedWithCIDR(t *testing.T) { + g := NewGuard([]string{"10.0.0.0/24"}, "", 0, "", 0) + cases := []struct { + host string + want bool + }{ + {"10.0.0.5", true}, + {"10.0.0.255", true}, + {"192.168.0.1", false}, + {"10.0.1.1", false}, + {"", false}, + } + for _, tc := range cases { + if got := g.IsAdminAuthorized(tc.host); got != tc.want { + t.Errorf("IsAdminAuthorized(%q) = %v want %v", tc.host, got, tc.want) + } + } +} diff --git a/weed/server/volume_grpc_admin.go b/weed/server/volume_grpc_admin.go index cc52919fcf..1ea290fc8c 100644 --- a/weed/server/volume_grpc_admin.go +++ b/weed/server/volume_grpc_admin.go @@ -30,17 +30,27 @@ import ( // checkGrpcAdminAuth verifies the gRPC caller is authorized for destructive // admin operations by checking the peer address against the guard's whitelist. // -// IP extraction prefers a typed *net.TCPAddr where available, falling back to -// SplitHostPort on the string form, then to the raw string. The fallback -// chain matters because in-process/passthrough connections used in tests -// surface as unparseable strings like "@"; with an empty whitelist the -// allow-all branch in IsWhiteListed accepts them, with a whitelist they're -// denied as expected. +// Only TCP peers go through the host check. Any non-TCP peer (in-process +// bufconn, unix socket, etc.) is treated as trusted because such callers +// share the same OS process or host filesystem as the volume server and +// cannot be spoofed by a remote attacker. This matters when `weed server` +// runs master+volume+filer in a single process and the master invokes +// AllocateVolume through an in-process gRPC dial: the peer address surfaces +// as "@" rather than 127.0.0.1, which has no parseable IP. // -// Failed authorization attempts are logged so an operator running with a -// configured whitelist can spot misconfigured callers and probe attempts. +// The whitelist check uses Guard.IsAdminAuthorized, which is fail-closed: a +// volume server with a configured guard but empty whitelist still rejects +// destructive admin RPCs from off-host TCP peers (loopback TCP callers are +// trusted, see IsAdminAuthorized). Operators must set -whiteList on the CLI +// or guard.white_list in security.toml to allow remote admin peers. Failed +// attempts are logged so misconfigured callers and probe attempts are +// visible. func (vs *VolumeServer) checkGrpcAdminAuth(ctx context.Context) error { if vs.guard == nil { + // No guard wired up at all — this is a developmental / embedded path + // (e.g. unit tests). Skip the gate; the real protection lives in + // IsAdminAuthorized, which only takes effect once a guard exists. + glog.V(1).Infof("gRPC admin auth: no guard configured; set -whiteList or guard.white_list in security.toml to enforce") return nil } pr, ok := peer.FromContext(ctx) @@ -50,17 +60,15 @@ func (vs *VolumeServer) checkGrpcAdminAuth(ctx context.Context) error { glog.V(0).Infof("gRPC admin auth failed: no peer info") return status.Error(codes.PermissionDenied, "no peer info") } - addr := pr.Addr.String() - var host string - if tcpAddr, ok := pr.Addr.(*net.TCPAddr); ok { - host = tcpAddr.IP.String() - } else if h, _, splitErr := net.SplitHostPort(addr); splitErr == nil { - host = h - } else { - host = addr + tcpAddr, isTCP := pr.Addr.(*net.TCPAddr) + if !isTCP { + // In-process / bufconn / unix-socket peer: same OS process as us, so + // trusted by construction. A remote attacker cannot reach this path. + return nil } - if !vs.guard.IsWhiteListed(host) { - glog.V(0).Infof("gRPC admin auth failed: %s is not whitelisted (remote: %s)", host, addr) + host := tcpAddr.IP.String() + if !vs.guard.IsAdminAuthorized(host) { + glog.V(0).Infof("gRPC admin auth failed: %s is not authorized (remote: %s)", host, pr.Addr.String()) return status.Errorf(codes.PermissionDenied, "not authorized: %s", host) } return nil diff --git a/weed/server/volume_grpc_admin_test.go b/weed/server/volume_grpc_admin_test.go new file mode 100644 index 0000000000..41e855139e --- /dev/null +++ b/weed/server/volume_grpc_admin_test.go @@ -0,0 +1,115 @@ +package weed_server + +import ( + "context" + "net" + "testing" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" + + "github.com/seaweedfs/seaweedfs/weed/security" +) + +// fakeAddr is a net.Addr that is not *net.TCPAddr. We use it to fake a +// gRPC peer for the in-process / bufconn / unix-socket case. +type fakeAddr struct{ s string } + +func (a fakeAddr) Network() string { return "fake" } +func (a fakeAddr) String() string { return a.s } + +// TestCheckGrpcAdminAuthNonTCPPeerTrusted pins the in-process trust contract. +// When `weed server` runs master+volume+filer in the same process, the master +// dials the volume server via an in-process / bufconn / unix-socket transport. +// The peer Addr is not a *net.TCPAddr (often surfaces as "@"), so we cannot +// extract an IP — but the caller is in the same OS process, so we trust it +// unconditionally. Without this, an empty-whitelist fail-closed guard would +// break every embedded-cluster deployment. +func TestCheckGrpcAdminAuthNonTCPPeerTrusted(t *testing.T) { + vs := &VolumeServer{guard: security.NewGuard(nil, "", 0, "", 0)} + cases := []net.Addr{ + fakeAddr{s: "@"}, + fakeAddr{s: ""}, + fakeAddr{s: "bufconn"}, + &net.UnixAddr{Name: "/tmp/weed.sock", Net: "unix"}, + } + for _, addr := range cases { + ctx := peer.NewContext(context.Background(), &peer.Peer{Addr: addr}) + if err := vs.checkGrpcAdminAuth(ctx); err != nil { + t.Errorf("non-TCP peer %T(%q) should be trusted, got %v", addr, addr.String(), err) + } + } +} + +// TestCheckGrpcAdminAuthTCPLoopbackTrusted covers the common single-host +// deployment where a separate `weed` process on the same host dials over +// real TCP. With no whitelist configured, loopback peers are still allowed +// (delegated to Guard.IsAdminAuthorized). +func TestCheckGrpcAdminAuthTCPLoopbackTrusted(t *testing.T) { + vs := &VolumeServer{guard: security.NewGuard(nil, "", 0, "", 0)} + for _, ip := range []string{"127.0.0.1", "127.0.0.5", "::1"} { + ctx := peer.NewContext(context.Background(), &peer.Peer{ + Addr: &net.TCPAddr{IP: net.ParseIP(ip), Port: 50000}, + }) + if err := vs.checkGrpcAdminAuth(ctx); err != nil { + t.Errorf("loopback TCP peer %s should be trusted, got %v", ip, err) + } + } +} + +// TestCheckGrpcAdminAuthTCPOffHostDenied is the fail-closed half of the +// contract: an off-host TCP peer with no matching whitelist entry is denied +// with PermissionDenied. +func TestCheckGrpcAdminAuthTCPOffHostDenied(t *testing.T) { + vs := &VolumeServer{guard: security.NewGuard(nil, "", 0, "", 0)} + ctx := peer.NewContext(context.Background(), &peer.Peer{ + Addr: &net.TCPAddr{IP: net.ParseIP("10.0.0.5"), Port: 50000}, + }) + err := vs.checkGrpcAdminAuth(ctx) + if err == nil { + t.Fatalf("off-host TCP peer with empty whitelist should be denied") + } + if got, want := status.Code(err), codes.PermissionDenied; got != want { + t.Errorf("status code = %v want %v", got, want) + } +} + +// TestCheckGrpcAdminAuthTCPWhitelistedAllowed verifies that an off-host TCP +// peer present in the configured whitelist is allowed through. +func TestCheckGrpcAdminAuthTCPWhitelistedAllowed(t *testing.T) { + vs := &VolumeServer{guard: security.NewGuard([]string{"10.0.0.5"}, "", 0, "", 0)} + ctx := peer.NewContext(context.Background(), &peer.Peer{ + Addr: &net.TCPAddr{IP: net.ParseIP("10.0.0.5"), Port: 50000}, + }) + if err := vs.checkGrpcAdminAuth(ctx); err != nil { + t.Errorf("whitelisted TCP peer should be allowed, got %v", err) + } +} + +// TestCheckGrpcAdminAuthNoPeerInfoDenied confirms we still deny when gRPC +// gives us no peer info at all (should not happen with real transports but +// we cannot prove a positive identity, so we refuse). +func TestCheckGrpcAdminAuthNoPeerInfoDenied(t *testing.T) { + vs := &VolumeServer{guard: security.NewGuard(nil, "", 0, "", 0)} + err := vs.checkGrpcAdminAuth(context.Background()) + if err == nil { + t.Fatalf("missing peer info should be denied") + } + if got, want := status.Code(err), codes.PermissionDenied; got != want { + t.Errorf("status code = %v want %v", got, want) + } +} + +// TestCheckGrpcAdminAuthNoGuardIsNoop preserves the developmental / embedded +// path: with no Guard wired up at all, the gate is a no-op regardless of +// peer information. +func TestCheckGrpcAdminAuthNoGuardIsNoop(t *testing.T) { + vs := &VolumeServer{guard: nil} + ctx := peer.NewContext(context.Background(), &peer.Peer{ + Addr: &net.TCPAddr{IP: net.ParseIP("10.0.0.5"), Port: 50000}, + }) + if err := vs.checkGrpcAdminAuth(ctx); err != nil { + t.Errorf("no guard should be a no-op, got %v", err) + } +}