From 0742d316aa739420bf36d40c9738d6b0f42cd614 Mon Sep 17 00:00:00 2001 From: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> Date: Wed, 4 Nov 2026 23:12:25 +0530 Subject: [PATCH] fix(s3): accept fresh admin tokens when context carries stale authorization (#12390) (#12397) * fix(s3): accept fresh admin tokens when context carries stale authorization (#12390) Signed-off-by: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> * security: share the admin Bearer check between S3 and admin-plane peers Move the authorization scan into security.CheckGrpcAdminToken, built on IsFilerAdminToken, and call it from CheckGrpcAdminPeer and the S3 IAM cache check. Both now accept a valid admin token in any authorization value instead of S3 re-implementing the decode and claim checks. Co-Authored-By: Claude Opus 5.5 * filer: accept a fresh admin token after a stale one on IAM gRPC The S3 IAM cache check is meant to mirror the filer's, so route the filer's checkAdminAuth through security.CheckGrpcAdminToken too. IamGrpcStore appends its token to the caller's context, so a forwarded older token would otherwise still shadow it here. Co-Authored-By: Claude Opus 5.5 * security: parse at most four authorization values per admin call Scanning every value let one request repeat a junk Bearer token (cheap over HPACK) tens of thousands of times and burn about half a second of JWT parsing on an unauthenticated port. Real callers send their own token plus at most one forwarded from their context. Co-Authored-By: Claude Opus 5.5 * credential: match the other WithS3InternalAdminAuth replace fix The plugin worker fix makes the same set-instead-of-append change to WithS3InternalAdminAuth with different code and a different test at the same spot, so whichever lands second would conflict. Use the identical code and test so the two merge cleanly in either order. Co-Authored-By: Claude Opus 5.5 * security: test an expired admin token ahead of a fresh one Co-Authored-By: Claude Opus 5.5 * security: test that a same-key session token is not an admin token Co-Authored-By: Claude Opus 5.5 * filer: test IAM gRPC with only invalid authorization values Co-Authored-By: Claude Opus 5.5 --------- Signed-off-by: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> Co-authored-by: Chris Lu Co-authored-by: Claude Opus 5.5 --- weed/s3api/s3api_server_grpc.go | 24 +-------- weed/s3api/s3api_server_grpc_test.go | 21 ++++++++ weed/security/grpc_peer.go | 32 +++++++++--- weed/security/grpc_peer_test.go | 50 +++++++++++++++++++ weed/server/filer_server_handlers_iam_grpc.go | 27 +--------- .../filer_server_handlers_iam_grpc_test.go | 25 ++++++++++ 6 files changed, 123 insertions(+), 56 deletions(-) diff --git a/weed/s3api/s3api_server_grpc.go b/weed/s3api/s3api_server_grpc.go index 3613d993de..123a5fc2f3 100644 --- a/weed/s3api/s3api_server_grpc.go +++ b/weed/s3api/s3api_server_grpc.go @@ -2,13 +2,11 @@ package s3api import ( "context" - "strings" "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" "github.com/seaweedfs/seaweedfs/weed/security" "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -30,27 +28,7 @@ func (s3a *S3ApiServer) checkAdminAuth(ctx context.Context) error { if len(signingKey) == 0 { return checkLocalPeer(ctx) } - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - return status.Error(codes.Unauthenticated, "missing metadata") - } - authHeaders := md.Get("authorization") - if len(authHeaders) == 0 { - return status.Error(codes.Unauthenticated, "missing authorization metadata") - } - raw := strings.TrimSpace(authHeaders[0]) - parts := strings.Fields(raw) - if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") || parts[1] == "" { - return status.Error(codes.Unauthenticated, "authorization header must use Bearer scheme") - } - parsed, err := security.DecodeJwt(signingKey, security.EncodedJwt(parts[1]), &security.SeaweedFilerAdminClaims{}) - if err != nil || parsed == nil || !parsed.Valid { - return status.Error(codes.Unauthenticated, "invalid admin token") - } - if claims, ok := parsed.Claims.(*security.SeaweedFilerAdminClaims); !ok || claims.SessionId != "" { - return status.Error(codes.Unauthenticated, "invalid admin token") - } - return nil + return security.CheckGrpcAdminToken(ctx, signingKey) } func checkLocalPeer(ctx context.Context) error { diff --git a/weed/s3api/s3api_server_grpc_test.go b/weed/s3api/s3api_server_grpc_test.go index 1b3cf89018..10113cad2f 100644 --- a/weed/s3api/s3api_server_grpc_test.go +++ b/weed/s3api/s3api_server_grpc_test.go @@ -140,6 +140,27 @@ func TestS3IamCache_ValidToken_WritesIdentity(t *testing.T) { } } +func TestS3IamCache_ValidTokenFollowingStaleToken_Succeeds(t *testing.T) { + s := newTestS3IamCacheServer(t, testS3IamCacheSigningKey) + bad := security.GenJwtForFilerAdmin(security.SigningKey("a-different-key"), 60) + good := security.GenJwtForFilerAdmin(security.SigningKey(testS3IamCacheSigningKey), 60) + multiCtx := metadata.NewIncomingContext(context.Background(), metadata.MD{ + "authorization": []string{ + security.BearerPrefix + string(bad), + security.BearerPrefix + string(good), + }, + }) + _, err := s.PutIdentity(multiCtx, &iam_pb.PutIdentityRequest{ + Identity: &iam_pb.Identity{ + Name: "pushed_multi", + Actions: []string{"Admin"}, + }, + }) + if err != nil { + t.Fatalf("PutIdentity with valid token following stale token: unexpected error %v", err) + } +} + // Without a signing key remote callers cannot be authenticated, so admin // RPCs are restricted to local clients: unix socket, loopback, or the // server's own interface addresses. Remote peers and missing peer info are diff --git a/weed/security/grpc_peer.go b/weed/security/grpc_peer.go index 9a2844067d..268d09edcf 100644 --- a/weed/security/grpc_peer.go +++ b/weed/security/grpc_peer.go @@ -25,6 +25,18 @@ func CheckGrpcAdminPeer(ctx context.Context, signingKey SigningKey) error { if len(signingKey) == 0 { return CheckLocalGrpcPeer(ctx) } + return CheckGrpcAdminToken(ctx, signingKey) +} + +// maxGrpcAdminAuthValues bounds the JWT parses one request can trigger; a +// caller sends its own token plus at most one forwarded from its context. +const maxGrpcAdminAuthValues = 4 + +// CheckGrpcAdminToken accepts the call when one of the first +// maxGrpcAdminAuthValues authorization values is a Bearer admin token signed +// by signingKey, so a fresh token still counts when the caller's context also +// forwards an older one. +func CheckGrpcAdminToken(ctx context.Context, signingKey SigningKey) error { md, ok := metadata.FromIncomingContext(ctx) if !ok { return status.Error(codes.Unauthenticated, "missing metadata") @@ -33,15 +45,21 @@ func CheckGrpcAdminPeer(ctx context.Context, signingKey SigningKey) error { if len(authHeaders) == 0 { return status.Error(codes.Unauthenticated, "missing authorization metadata") } - raw := strings.TrimSpace(authHeaders[0]) - parts := strings.Fields(raw) - if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") || parts[1] == "" { - return status.Error(codes.Unauthenticated, "authorization header must use Bearer scheme") + if len(authHeaders) > maxGrpcAdminAuthValues { + authHeaders = authHeaders[:maxGrpcAdminAuthValues] } - if !IsFilerAdminToken(signingKey, EncodedJwt(parts[1])) { - return status.Error(codes.Unauthenticated, "invalid admin token") + err := status.Error(codes.Unauthenticated, "authorization header must use Bearer scheme") + for _, raw := range authHeaders { + parts := strings.Fields(raw) + if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") || parts[1] == "" { + continue + } + if IsFilerAdminToken(signingKey, EncodedJwt(parts[1])) { + return nil + } + err = status.Error(codes.Unauthenticated, "invalid admin token") } - return nil + return err } // IsFilerAdminToken reports whether token is an admin token signed by diff --git a/weed/security/grpc_peer_test.go b/weed/security/grpc_peer_test.go index 8a6127d072..c67383c478 100644 --- a/weed/security/grpc_peer_test.go +++ b/weed/security/grpc_peer_test.go @@ -5,8 +5,11 @@ import ( "crypto/tls" "crypto/x509" "net" + "slices" "testing" + "time" + jwt "github.com/golang-jwt/jwt/v5" "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" @@ -66,3 +69,50 @@ func TestGrpcAdminPeerCredentials(t *testing.T) { t.Fatalf("expected one authorization entry, got %v", md) } } + +func TestCheckGrpcAdminToken(t *testing.T) { + key := SigningKey("test-signing-key-0123456789abcdef") + fresh := "Bearer " + string(GenJwtForFilerAdmin(key, 60)) + stale := "Bearer " + string(GenJwtForFilerAdmin(SigningKey("another-key"), 60)) + expired, signErr := jwt.NewWithClaims(jwt.SigningMethodHS256, SeaweedFilerAdminClaims{ + RegisteredClaims: jwt.RegisteredClaims{ExpiresAt: jwt.NewNumericDate(time.Now().Add(-time.Hour))}, + }).SignedString([]byte(key)) + if signErr != nil { + t.Fatalf("sign expired token: %v", signErr) + } + session, signErr := jwt.NewWithClaims(jwt.SigningMethodHS256, SeaweedFilerAdminClaims{ + SessionId: "session", + RegisteredClaims: jwt.RegisteredClaims{ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour))}, + }).SignedString([]byte(key)) + if signErr != nil { + t.Fatalf("sign session token: %v", signErr) + } + + for name, tc := range map[string]struct { + values []string + wantErr string + }{ + "stale then fresh": {[]string{stale, fresh}, ""}, + "fresh then garbage": {[]string{fresh, "garbage"}, ""}, + "expired then fresh": {[]string{"Bearer " + expired, fresh}, ""}, + "only expired": {[]string{"Bearer " + expired}, "invalid admin token"}, + "only stale": {[]string{stale}, "invalid admin token"}, + "same-key session": {[]string{"Bearer " + session}, "invalid admin token"}, + "basic then stale": {[]string{"Basic dXNlcjpwYXNz", stale}, "invalid admin token"}, + "no bearer value": {[]string{"Basic dXNlcjpwYXNz", "garbage"}, "authorization header must use Bearer scheme"}, + "lowercase scheme ok": {[]string{"bearer " + fresh[len("Bearer "):]}, ""}, + "fresh past the cap": {append(slices.Repeat([]string{stale}, maxGrpcAdminAuthValues), fresh), "invalid admin token"}, + } { + ctx := metadata.NewIncomingContext(context.Background(), metadata.MD{"authorization": tc.values}) + err := CheckGrpcAdminToken(ctx, key) + if tc.wantErr == "" { + if err != nil { + t.Errorf("%s: unexpected error %v", name, err) + } + continue + } + if status.Code(err) != codes.Unauthenticated || status.Convert(err).Message() != tc.wantErr { + t.Errorf("%s: got %v, want Unauthenticated %q", name, err, tc.wantErr) + } + } +} diff --git a/weed/server/filer_server_handlers_iam_grpc.go b/weed/server/filer_server_handlers_iam_grpc.go index 508dcd2c3b..d92543c2fe 100644 --- a/weed/server/filer_server_handlers_iam_grpc.go +++ b/weed/server/filer_server_handlers_iam_grpc.go @@ -3,7 +3,6 @@ package weed_server import ( "context" "encoding/json" - "strings" "github.com/seaweedfs/seaweedfs/weed/credential" "github.com/seaweedfs/seaweedfs/weed/glog" @@ -12,7 +11,6 @@ import ( "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" "github.com/seaweedfs/seaweedfs/weed/security" "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -48,30 +46,7 @@ func (s *IamGrpcServer) checkAdminAuth(ctx context.Context) error { if len(s.adminSigningKey) == 0 { return nil } - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - return status.Error(codes.Unauthenticated, "missing metadata") - } - authHeaders := md.Get("authorization") - if len(authHeaders) == 0 { - return status.Error(codes.Unauthenticated, "missing authorization metadata") - } - // RFC 6750 ยง2.1: the "Bearer" auth-scheme name is case-insensitive. - // Tolerate surrounding whitespace and any spacing between scheme and token. - raw := strings.TrimSpace(authHeaders[0]) - parts := strings.Fields(raw) - if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") || parts[1] == "" { - return status.Error(codes.Unauthenticated, "authorization header must use Bearer scheme") - } - token := parts[1] - parsed, err := security.DecodeJwt(s.adminSigningKey, security.EncodedJwt(token), &security.SeaweedFilerAdminClaims{}) - if err != nil || parsed == nil || !parsed.Valid { - return status.Error(codes.Unauthenticated, "invalid admin token") - } - if claims, ok := parsed.Claims.(*security.SeaweedFilerAdminClaims); !ok || claims.SessionId != "" { - return status.Error(codes.Unauthenticated, "invalid admin token") - } - return nil + return security.CheckGrpcAdminToken(ctx, s.adminSigningKey) } ////////////////////////////////////////////////// diff --git a/weed/server/filer_server_handlers_iam_grpc_test.go b/weed/server/filer_server_handlers_iam_grpc_test.go index b6964c58bf..af5397da7d 100644 --- a/weed/server/filer_server_handlers_iam_grpc_test.go +++ b/weed/server/filer_server_handlers_iam_grpc_test.go @@ -117,6 +117,31 @@ func TestIamGrpc_ValidToken_ReachesHandler(t *testing.T) { } } +func TestIamGrpc_ValidTokenAfterStaleToken_ReachesHandler(t *testing.T) { + s := newTestIamGrpcServer(t) + stale := security.GenJwtForFilerAdmin(security.SigningKey("a-different-key"), 60) + good := security.GenJwtForFilerAdmin(security.SigningKey(testIamSigningKey), 60) + ctx := metadata.NewIncomingContext(context.Background(), metadata.MD{ + "authorization": []string{"Bearer " + string(stale), "Bearer " + string(good)}, + }) + if _, err := s.ListUsers(ctx, &iam_pb.ListUsersRequest{}); err != nil { + t.Fatalf("ListUsers with a valid token after a stale one: unexpected error %v", err) + } +} + +func TestIamGrpc_AllInvalidTokens_Unauthenticated(t *testing.T) { + s := newTestIamGrpcServer(t) + stale := security.GenJwtForFilerAdmin(security.SigningKey("a-different-key"), 60) + other := security.GenJwtForFilerAdmin(security.SigningKey("another-different-key"), 60) + ctx := metadata.NewIncomingContext(context.Background(), metadata.MD{ + "authorization": []string{"Bearer " + string(stale), "Bearer " + string(other)}, + }) + _, err := s.ListUsers(ctx, &iam_pb.ListUsersRequest{}) + if got, want := status.Code(err), codes.Unauthenticated; got != want { + t.Fatalf("ListUsers with two invalid tokens: got code %v, want %v (err=%v)", got, want, err) + } +} + func TestIamGrpc_NoSigningKey_Unauthenticated_Allowed(t *testing.T) { // Auth is opt-in: when the server is built without a signing key, every // RPC is accepted regardless of (or in the absence of) metadata so the