From 0ca1c198216803b518e1e2d112bdd26f7b5d6cb2 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 18 Sep 2026 01:01:04 -0700 Subject: [PATCH] s3api: unify auth error handling across s3tables, iceberg and lance (#11381) * s3api: fail closed when S3 Tables signature verification fails * s3api: avoid nil Account dereference in S3 Tables auth log * iceberg: return auth error instead of falling back to DefaultAllow * lance: return auth error instead of falling back to DefaultAllow * s3api: stop trusting client-supplied s3-account-id The header is set by the server after successful authentication; scrub inbound values alongside the other internal headers, and apply the same admin guard to the header fallback branch of getAccountID that the identity branch already has. * test: cover table-catalog auth wrappers and principal resolution * test: configure anonymous identity where catalog clients do not sign * s3api: scrub s3-account-id after signature verification --- test/s3tables/lifecycle/cluster_test.go | 7 + test/testutil/helpers.go | 3 + weed/s3api/auth_credentials.go | 2 + weed/s3api/iceberg/server.go | 31 ++-- weed/s3api/lance/server.go | 25 ++- weed/s3api/s3api_bucket_handlers_misc_test.go | 5 + weed/s3api/s3api_tables.go | 18 +- weed/s3api/s3api_tables_auth_test.go | 171 ++++++++++++++++++ weed/s3api/s3tables/handler.go | 4 +- weed/s3api/s3tables/handler_principal_test.go | 33 ++++ 10 files changed, 256 insertions(+), 43 deletions(-) create mode 100644 weed/s3api/s3api_tables_auth_test.go create mode 100644 weed/s3api/s3tables/handler_principal_test.go diff --git a/test/s3tables/lifecycle/cluster_test.go b/test/s3tables/lifecycle/cluster_test.go index 5c5236596..2ab04812e 100644 --- a/test/s3tables/lifecycle/cluster_test.go +++ b/test/s3tables/lifecycle/cluster_test.go @@ -157,6 +157,12 @@ func newEnvironment() (*environment, error) { } func (env *environment) start() error { + // The catalog clients here do not sign, so anonymous access is configured explicitly. + iamConfigPath, err := testutil.WriteIAMConfig(env.dataDir, accessKey, secretKey) + if err != nil { + return fmt.Errorf("write IAM config: %w", err) + } + ctx, cancel := context.WithCancel(context.Background()) env.weedCancel = cancel @@ -171,6 +177,7 @@ func (env *environment) start() error { "-s3.port.grpc", fmt.Sprintf("%d", env.s3GrpcPort), "-s3.port.iceberg", fmt.Sprintf("%d", env.icebergPort), "-s3.port.lance", fmt.Sprintf("%d", env.lancePort), + "-s3.config", iamConfigPath, "-ip.bind", "0.0.0.0", "-dir", env.dataDir, ) diff --git a/test/testutil/helpers.go b/test/testutil/helpers.go index 7272722ca..268b35b29 100644 --- a/test/testutil/helpers.go +++ b/test/testutil/helpers.go @@ -58,6 +58,9 @@ func WriteIAMConfig(dir, accessKey, secretKey string) (string, error) { "Tagging", "Write" ] + }, + { + "name": "anonymous" } ] }`, accessKey, secretKey) diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 428f42b1d..387f51275 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1679,6 +1679,8 @@ func (iam *IdentityAccessManagement) authenticateRequestInternal(r *http.Request // for every auth path — prevents privilege escalation via header injection. r.Header.Del(s3_constants.SeaweedFSPrincipalHeader) r.Header.Del(s3_constants.SeaweedFSSessionTokenHeader) + // Deferred so signature verification can still read a client-signed value. + defer r.Header.Del(s3_constants.AmzAccountId) reqAuthType := getRequestAuthType(r) diff --git a/weed/s3api/iceberg/server.go b/weed/s3api/iceberg/server.go index 33164df50..5266b8144 100644 --- a/weed/s3api/iceberg/server.go +++ b/weed/s3api/iceberg/server.go @@ -250,25 +250,20 @@ func (s *Server) Auth(handler http.HandlerFunc) http.HandlerFunc { identityName, identity, errCode := s.authenticator.AuthenticateRequest(r) if errCode != s3err.ErrNone { - // If authentication failed but DefaultAllow is enabled, proceed without identity - if s.authenticator.DefaultAllow() { - glog.V(2).Infof("Iceberg: AuthenticateRequest failed (%v), but DefaultAllow is true, proceeding", errCode) - } else { - apiErr := s3err.GetAPIError(errCode) - errorType := "RESTException" - switch apiErr.HTTPStatusCode { - case http.StatusForbidden: - errorType = "ForbiddenException" - case http.StatusUnauthorized: - errorType = "NotAuthorizedException" - case http.StatusBadRequest: - errorType = "BadRequestException" - case http.StatusInternalServerError: - errorType = "InternalServerError" - } - writeError(w, apiErr.HTTPStatusCode, errorType, apiErr.Description) - return + apiErr := s3err.GetAPIError(errCode) + errorType := "RESTException" + switch apiErr.HTTPStatusCode { + case http.StatusForbidden: + errorType = "ForbiddenException" + case http.StatusUnauthorized: + errorType = "NotAuthorizedException" + case http.StatusBadRequest: + errorType = "BadRequestException" + case http.StatusInternalServerError: + errorType = "InternalServerError" } + writeError(w, apiErr.HTTPStatusCode, errorType, apiErr.Description) + return } if identityName != "" || identity != nil { diff --git a/weed/s3api/lance/server.go b/weed/s3api/lance/server.go index c94121b82..0218556fe 100644 --- a/weed/s3api/lance/server.go +++ b/weed/s3api/lance/server.go @@ -155,21 +155,18 @@ func (s *Server) Auth(handler http.HandlerFunc) http.HandlerFunc { identityName, identity, errCode := s.authenticator.AuthenticateRequest(r) if errCode != s3err.ErrNone { - if !s.authenticator.DefaultAllow() { - apiErr := s3err.GetAPIError(errCode) - code := codeInternal - switch apiErr.HTTPStatusCode { - case http.StatusForbidden: - code = codePermissionDenied - case http.StatusUnauthorized: - code = codeUnauthenticated - case http.StatusBadRequest: - code = codeInvalidInput - } - writeError(w, r, apiErr.HTTPStatusCode, code, apiErr.Description) - return + apiErr := s3err.GetAPIError(errCode) + code := codeInternal + switch apiErr.HTTPStatusCode { + case http.StatusForbidden: + code = codePermissionDenied + case http.StatusUnauthorized: + code = codeUnauthenticated + case http.StatusBadRequest: + code = codeInvalidInput } - glog.V(2).Infof("lance: authentication failed (%v) but the gateway is open, proceeding", errCode) + writeError(w, r, apiErr.HTTPStatusCode, code, apiErr.Description) + return } if identityName != "" || identity != nil { diff --git a/weed/s3api/s3api_bucket_handlers_misc_test.go b/weed/s3api/s3api_bucket_handlers_misc_test.go index 8f8081e90..90ab5ec52 100644 --- a/weed/s3api/s3api_bucket_handlers_misc_test.go +++ b/weed/s3api/s3api_bucket_handlers_misc_test.go @@ -162,6 +162,11 @@ func TestGetBucketOwnershipControlsDefaultsToBucketOwnerEnforced(t *testing.T) { }) req := newBucketRequest(http.MethodGet, "b", "ownershipControls=", "") req.Header.Set(s3_constants.AmzAccountId, AccountAdmin.Id) + req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), &Identity{ + Name: "admin", + Account: &AccountAdmin, + Actions: []Action{s3_constants.ACTION_ADMIN}, + })) rec := httptest.NewRecorder() s3a.GetBucketOwnershipControls(rec, req) diff --git a/weed/s3api/s3api_tables.go b/weed/s3api/s3api_tables.go index 2c76b91d7..f64e315b1 100644 --- a/weed/s3api/s3api_tables.go +++ b/weed/s3api/s3api_tables.go @@ -701,20 +701,18 @@ func (s3a *S3ApiServer) authenticateS3Tables(f http.HandlerFunc) http.HandlerFun // Use AuthSignatureOnly to authenticate the request without authorizing specific actions identity, errCode := s3a.iam.AuthSignatureOnly(r) if errCode != s3err.ErrNone { - // If IAM is enabled but DefaultAllow is true, we can proceed even if unauthenticated - // authorization checks in handlers will then use DefaultAllow logic. - if s3a.iam.iamIntegration != nil && s3a.iam.iamIntegration.DefaultAllow() { - glog.V(2).Infof("S3Tables: AuthSignatureOnly failed (%v), but DefaultAllow is true, proceeding", errCode) - } else { - glog.Errorf("S3Tables: AuthSignatureOnly failed: %v", errCode) - s3err.WriteErrorResponse(w, r, errCode) - return - } + glog.Errorf("S3Tables: AuthSignatureOnly failed: %v", errCode) + s3err.WriteErrorResponse(w, r, errCode) + return } // Store the authenticated identity in request context if identity != nil && identity.Name != "" { - glog.V(2).Infof("S3Tables: authenticated identity Name=%s Account.Id=%s", identity.Name, identity.Account.Id) + accountId := "" + if identity.Account != nil { + accountId = identity.Account.Id + } + glog.V(2).Infof("S3Tables: authenticated identity Name=%s Account.Id=%s", identity.Name, accountId) r = r.WithContext(recordIdentityInContext(r, identity)) } else { glog.V(2).Infof("S3Tables: authenticated identity is nil or empty name") diff --git a/weed/s3api/s3api_tables_auth_test.go b/weed/s3api/s3api_tables_auth_test.go new file mode 100644 index 000000000..6ab18d59d --- /dev/null +++ b/weed/s3api/s3api_tables_auth_test.go @@ -0,0 +1,171 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws/credentials" + v4 "github.com/aws/aws-sdk-go/aws/signer/v4" + "github.com/seaweedfs/seaweedfs/weed/s3api/iceberg" + "github.com/seaweedfs/seaweedfs/weed/s3api/lance" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// setupDefaultAllowAuthServer builds the wiring NewS3ApiServer produces when +// S3 credentials are configured but no -iam.config file was given: auth is +// enforced while the IAM policy engine still defaults to allow. +func setupDefaultAllowAuthServer(t *testing.T) *S3ApiServer { + t.Helper() + s3a := setupRoutingTestServer(t) + manager, err := loadIAMManagerFromConfig("", + func() string { return "localhost:8888" }, + func() string { return "test-signing-key" }) + require.NoError(t, err) + require.True(t, manager.DefaultAllow()) + s3a.iam.iamIntegration = NewS3IAMIntegration(manager, "") + return s3a +} + +func forgedS3TablesRequest(t *testing.T, method, target string) *http.Request { + t.Helper() + req, err := http.NewRequest(method, "http://localhost"+target, nil) + require.NoError(t, err) + req.Header.Set("Authorization", "AWS4-HMAC-SHA256 Credential=AAAA/20260917/us-east-1/s3tables/aws4_request, SignedHeaders=host, Signature=00") + return req +} + +func TestS3TablesAuthRejectsFailedSignature(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + + req := forgedS3TablesRequest(t, http.MethodGet, "/buckets") + req.Header.Set(s3_constants.AmzAccountId, "admin") + + reached := false + rr := httptest.NewRecorder() + s3a.authenticateS3Tables(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.False(t, reached, "failed authentication must not reach the S3 Tables handler") + assert.GreaterOrEqual(t, rr.Code, http.StatusBadRequest) + assert.Empty(t, req.Header.Get(s3_constants.AmzAccountId), "client-supplied account header must not survive authentication") +} + +func TestS3TablesAuthControlDataPlane(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + + req := forgedS3TablesRequest(t, http.MethodGet, "/buckets") + reached := false + rr := httptest.NewRecorder() + s3a.iam.Auth(func(w http.ResponseWriter, r *http.Request) { + reached = true + }, s3_constants.ACTION_READ)(rr, req) + + assert.False(t, reached, "control: data plane must reject the same forged request") + assert.GreaterOrEqual(t, rr.Code, http.StatusBadRequest) +} + +func TestS3TablesAuthOpenWhenAuthDisabled(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + s3a.iam.isAuthEnabled = false + + req := forgedS3TablesRequest(t, http.MethodGet, "/buckets") + reached := false + rr := httptest.NewRecorder() + s3a.authenticateS3Tables(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.True(t, reached, "zero-config gateway keeps serving unauthenticated requests") + assert.Equal(t, http.StatusOK, rr.Code) +} + +func TestS3TablesAuthSignedRequestPassess(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + + req, err := http.NewRequest(http.MethodGet, "http://localhost/buckets", nil) + require.NoError(t, err) + creds := credentials.NewStaticCredentials(routingTestAccessKey, routingTestSecretKey, "") + _, err = v4.NewSigner(creds).Sign(req, strings.NewReader(""), "s3tables", "us-east-1", time.Now()) + require.NoError(t, err) + + reached := false + rr := httptest.NewRecorder() + s3a.authenticateS3Tables(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.True(t, reached, "properly signed request must still pass; got %d %s", rr.Code, rr.Body.String()) +} + +func TestSignedAccountHeaderDoesNotReachHandler(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + + req, err := http.NewRequest(http.MethodGet, "http://localhost/buckets", nil) + require.NoError(t, err) + req.Header.Set(s3_constants.AmzAccountId, "admin") + signRoutingTestRequest(t, req, "", "s3tables") + require.Contains(t, req.Header.Get("Authorization"), "s3-account-id", "the header must be covered by the signature") + + reached := false + rr := httptest.NewRecorder() + s3a.authenticateS3Tables(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.True(t, reached, "a signature covering s3-account-id must still verify; got %d %s", rr.Code, rr.Body.String()) + assert.Empty(t, req.Header.Get(s3_constants.AmzAccountId), "the signed-in header value must not survive authentication") +} + +func TestIcebergAuthRejectsFailedSignature(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + server := iceberg.NewServer(nil, s3a) + + req := forgedS3TablesRequest(t, http.MethodGet, "/v1/namespaces") + reached := false + rr := httptest.NewRecorder() + server.Auth(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.False(t, reached, "failed authentication must not reach the Iceberg handler") + assert.NotEqual(t, http.StatusOK, rr.Code) +} + +func TestLanceAuthRejectsFailedSignature(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + server := lance.NewServer(nil, s3a) + + req := forgedS3TablesRequest(t, http.MethodGet, "/v1/namespace/list") + reached := false + rr := httptest.NewRecorder() + server.Auth(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })(rr, req) + + assert.False(t, reached, "failed authentication must not reach the Lance handler") + assert.NotEqual(t, http.StatusOK, rr.Code) +} + +func TestAuthSignatureOnlyScrubsAccountHeader(t *testing.T) { + s3a := setupDefaultAllowAuthServer(t) + + req := forgedS3TablesRequest(t, http.MethodGet, "/buckets") + req.Header.Set(s3_constants.AmzAccountId, "admin") + + _, errCode := s3a.iam.AuthSignatureOnly(req) + assert.NotEqual(t, s3err.ErrNone, errCode) + assert.Empty(t, req.Header.Get(s3_constants.AmzAccountId)) +} diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index b9d3501c0..3f2fedc32 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -283,7 +283,9 @@ func (h *S3TablesHandler) getAccountID(r *http.Request) string { if accountID := r.Header.Get(s3_constants.AmzAccountId); accountID != "" { if principal := normalizePrincipalID(accountID); principal != "" { - return principal + if principal != s3_constants.AccountAdminId || hasAdminAction(getIdentityActions(r)) { + return principal + } } } return h.accountID diff --git a/weed/s3api/s3tables/handler_principal_test.go b/weed/s3api/s3tables/handler_principal_test.go new file mode 100644 index 000000000..28ac9c9ee --- /dev/null +++ b/weed/s3api/s3tables/handler_principal_test.go @@ -0,0 +1,33 @@ +package s3tables + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/stretchr/testify/assert" +) + +func TestGetAccountIDRejectsHeaderAdmin(t *testing.T) { + h := NewS3TablesHandler() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set(s3_constants.AmzAccountId, s3_constants.AccountAdminId) + + assert.NotEqual(t, s3_constants.AccountAdminId, h.getAccountID(req), + "a client-supplied account header must not resolve to the admin principal") +} + +func TestGetAccountIDHeaderBranchResolvesCaller(t *testing.T) { + h := NewS3TablesHandler() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set(s3_constants.AmzAccountId, "alice") + + assert.Equal(t, "alice", h.getAccountID(req)) +} + +func TestCheckPermissionDenyPolicyBindsNonAdmin(t *testing.T) { + denyAll := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:DeleteTableBucket"}]}` + assert.False(t, CheckPermissionWithContext("s3tables:DeleteTableBucket", "mallory", "owner123", denyAll, "arn:aws:s3tables:us-east-1:000000000000:bucket/victim", + &PolicyContext{DefaultAllow: true})) +}