diff --git a/weed/s3api/s3tables/filer_ops.go b/weed/s3api/s3tables/filer_ops.go index 50c59376f3..abfd70e624 100644 --- a/weed/s3api/s3tables/filer_ops.go +++ b/weed/s3api/s3tables/filer_ops.go @@ -1091,6 +1091,24 @@ func RootClaimed(roots map[string][]string, selfPath, root string) bool { return false } +// rootFilesShared reports whether a record other than selfPath keeps its files +// at root, above it or under it. Unlike RootClaimed it ignores where the other +// records themselves sit: a table created under a renamed table's old name has +// its entry on that directory and its files elsewhere. +func rootFilesShared(roots map[string][]string, selfPath, root string) bool { + for recordPath, others := range roots { + if recordPath == selfPath { + continue + } + for _, other := range others { + if PathsOverlap(other, root) { + return true + } + } + } + return false +} + // checkLocationUnclaimed returns ErrLocationInUse when root, the one the record // at recordPath would have, is claimed by another record. func checkLocationUnclaimed(ctx context.Context, client filer_pb.SeaweedFilerClient, bucketName, recordPath, root string) error { @@ -1174,6 +1192,27 @@ func (h *S3TablesHandler) refuseClaimedLocation(w http.ResponseWriter, r *http.R return err } +// refuseUpdatedMetadataLocation answers an update whose metadata location the +// record at recordPath, whose stored metadata is current, may not take; see +// validateUpdatedMetadataLocation. +func (h *S3TablesHandler) refuseUpdatedMetadataLocation(w http.ResponseWriter, r *http.Request, filerClient FilerClient, bucketName, namespace, recordPath string, current *tableMetadataInternal, metadataLocation string) error { + var checkErr error + err := validateUpdatedMetadataLocation(metadataLocation, bucketName, namespace, recordPath, current, func(root string) (shared bool, err error) { + checkErr = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + roots, err := CatalogRecordRoots(r.Context(), client, bucketName) + shared = err == nil && rootFilesShared(roots, recordPath, root) + return err + }) + return shared, checkErr + }) + if checkErr != nil { + h.writeError(w, http.StatusInternalServerError, ErrCodeInternalError, "failed to check location") + } else if err != nil { + h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) + } + return err +} + // claimTreeDirectory writes a fresh delete claim onto the directory at dirPath // while it still carries expected, and returns the claim with the attributes it // left. A claim seen in expected, one a delete that died left, is replaced like diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index c0205bf73f..5d9cb71d92 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -2004,7 +2004,9 @@ func (h *S3TablesHandler) authorizeUpdateTable(w http.ResponseWriter, r *http.Re return nil, err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { + // Where the location may point depends on the stored record, so only its + // shape is checked here; the rest waits for authorization below. + if _, err := validateMetadataLocationShape(req.MetadataLocation, bucketName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return nil, err } @@ -2107,6 +2109,12 @@ func (h *S3TablesHandler) authorizeUpdateTable(w http.ResponseWriter, r *http.Re return nil, NewAuthError("UpdateTable", principal, "not authorized to update table") } + // After authorization, so a caller who may not update the table learns + // nothing about where its files live. + if err := h.refuseUpdatedMetadataLocation(w, r, filerClient, bucketName, namespaceName, tablePath, &metadata, req.MetadataLocation); err != nil { + return nil, err + } + return &updateTableTarget{ bucketName: bucketName, namespaceName: namespaceName, diff --git a/weed/s3api/s3tables/handler_update_after_rename_test.go b/weed/s3api/s3tables/handler_update_after_rename_test.go new file mode 100644 index 0000000000..492e58b5aa --- /dev/null +++ b/weed/s3api/s3tables/handler_update_after_rename_test.go @@ -0,0 +1,248 @@ +package s3tables + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" +) + +// A rename is catalog-only: a table or view renamed into another namespace keeps +// its files, and so the directory its metadata location names, under the +// namespace it came from. Its next update writes there and must be accepted, +// while an update that moves it elsewhere stays confined to its new namespace. + +func addRenameNamespace(t *testing.T, fs *s3tablestest.MemFiler, namespace string) { + t.Helper() + nsMeta, err := json.Marshal(namespaceMetadata{Namespace: []string{namespace}, OwnerAccountID: DefaultAccountID}) + require.NoError(t, err) + fs.Put(GetTableBucketPath(renameTestBucket), namespace, map[string][]byte{ExtendedKeyMetadata: nsMeta}) +} + +// renamedAcrossNamespaces seeds ns/t, whose files sit under ns/t, and renames it +// to ns2/t. +func renamedAcrossNamespaces(t *testing.T) (*s3tablestest.MemFiler, *Manager) { + t.Helper() + fs, m := startRenameManager(t) + addRenameNamespace(t, fs, "ns2") + require.NoError(t, runRename(t, m, fs, &RenameTableRequest{TableBucketARN: mustBucketARN(t), + SourceNamespace: []string{"ns"}, SourceName: "t", DestNamespace: []string{"ns2"}, DestName: "t"})) + return fs, m +} + +func seedRenameTable(t *testing.T, fs *s3tablestest.MemFiler, namespace, name string) { + t.Helper() + meta, err := json.Marshal(tableMetadataInternal{Name: name, Namespace: namespace, Format: "ICEBERG", OwnerAccountID: DefaultAccountID, + MetadataVersion: 1, MetadataLocation: "s3://" + renameTestBucket + "/" + namespace + "/" + name + "/metadata/v1.metadata.json"}) + require.NoError(t, err) + fs.Put(GetNamespacePath(renameTestBucket, namespace), name, map[string][]byte{ExtendedKeyMetadata: meta}) +} + +func updateTableAt(ctx context.Context, t *testing.T, m *Manager, fs *s3tablestest.MemFiler, namespace, location string) error { + return m.Execute(ctx, NewManagerClient(fs.Client), "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{namespace}, Name: "t", MetadataVersion: 4, MetadataLocation: location}, nil, "") +} + +func s3TablesErrorText(err error) string { + var tableErr *S3TablesError + if errors.As(err, &tableErr) { + return tableErr.Type + ": " + tableErr.Message + } + if err != nil { + return err.Error() + } + return "" +} + +func TestUpdateAfterRenameAcrossNamespacesKeepsItsDirectory(t *testing.T) { + fs, m := renamedAcrossNamespaces(t) + location := "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json" + require.NoError(t, updateTableAt(context.Background(), t, m, fs, "ns2", location)) + + got, err := runGetTable(t, m, fs, "ns2", "t") + require.NoError(t, err) + require.Equal(t, location, got.MetadataLocation) +} + +func TestUpdateAfterRenameAcrossNamespacesRefusesOtherDirectoriesOfTheOldNamespace(t *testing.T) { + fs, m := renamedAcrossNamespaces(t) + err := updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v4.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2") +} + +func TestUpdateAfterRenameAcrossNamespacesRefusesAnotherTablesDirectory(t *testing.T) { + // One the old namespace holds: not the table's own directory, and outside + // its current namespace. + fs, m := renamedAcrossNamespaces(t) + seedRenameTable(t, fs, "ns", "u") + err := updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/u/metadata/v9.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2") + + // One the current namespace holds: inside the namespace, but claimed. + fs, m = renamedAcrossNamespaces(t) + seedRenameTable(t, fs, "ns2", "w") + err = updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns2/w/metadata/v9.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), ErrCodeConflict+": location is in use") +} + +// A table created under the renamed table's old name has its entry on that +// directory and its files in a directory of its own, so the renamed table still +// keeps its files there alone. The record is seeded rather than created, since +// a rename may keep the old name taken. +func TestUpdateAfterRenameAcrossNamespacesKeepsItsDirectoryUnderANewTableOfTheOldName(t *testing.T) { + ctx := context.Background() + fs, m := renamedAcrossNamespaces(t) + meta, err := json.Marshal(tableMetadataInternal{Name: "t", Namespace: "ns", Format: "ICEBERG", OwnerAccountID: DefaultAccountID, + MetadataVersion: 1, WarehouseLocation: "s3://" + renameTestBucket + "/ns/t-fresh"}) + require.NoError(t, err) + fs.Put(GetNamespacePath(renameTestBucket, "ns"), "t", map[string][]byte{ExtendedKeyMetadata: meta}) + + location := "s3://" + renameTestBucket + "/ns/t/metadata/v4.metadata.json" + require.NoError(t, updateTableAt(ctx, t, m, fs, "ns2", location)) + got, err := runGetTable(t, m, fs, "ns2", "t") + require.NoError(t, err) + require.Equal(t, location, got.MetadataLocation) +} + +// A record written before locations were confined to their namespace and +// claimed can keep its files in another table's directory. It gets no +// exception: it stays held to its namespace, as before. +func TestUpdateRefusesADirectoryOutsideTheNamespaceThatAnotherTableShares(t *testing.T) { + fs, m := startRenameManager(t) + addRenameNamespace(t, fs, "ns2") + meta, err := json.Marshal(tableMetadataInternal{Name: "t", Namespace: "ns2", Format: "ICEBERG", OwnerAccountID: DefaultAccountID, + MetadataVersion: 3, MetadataLocation: "s3://" + renameTestBucket + "/ns/t/metadata/v3.metadata.json"}) + require.NoError(t, err) + fs.Put(GetNamespacePath(renameTestBucket, "ns2"), "t", map[string][]byte{ExtendedKeyMetadata: meta}) + + err = updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/t/metadata/v4.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2") +} + +func TestUpdateAfterRenameAcrossNamespacesMayMoveIntoItsNamespace(t *testing.T) { + fs, m := renamedAcrossNamespaces(t) + require.NoError(t, updateTableAt(context.Background(), t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns2/t-moved/metadata/v4.metadata.json")) +} + +func TestUpdateStillConfinesAMoveToTheTablesNamespace(t *testing.T) { + fs, m := startRenameManager(t) + addRenameNamespace(t, fs, "ns2") + err := updateTableAt(context.Background(), t, m, fs, "ns", "s3://"+renameTestBucket+"/ns2/elsewhere/metadata/v4.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns") +} + +func TestUpdateLocationIsCheckedAfterAuthorization(t *testing.T) { + fs, m := renamedAcrossNamespaces(t) + m.SetTrusted(false) + m.SetDefaultAllow(false) + stranger := &testIdentity{Name: "stranger", Account: &testIdentityAccount{Id: "stranger"}} + ctx := s3_constants.SetIdentityInContext(context.Background(), stranger) + + err := updateTableAt(ctx, t, m, fs, "ns2", "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v4.metadata.json") + require.Error(t, err) + require.NotContains(t, s3TablesErrorText(err), "must be within namespace", + "a caller who may not update the table must not learn where its files may go") +} + +func renameViewAcrossNamespaces(t *testing.T) (*s3tablestest.MemFiler, *Manager) { + t.Helper() + fs, m := startRenameManager(t) + seedView(t, fs, "v") + addRenameNamespace(t, fs, "ns2") + require.NoError(t, m.Execute(context.Background(), NewManagerClient(fs.Client), "RenameView", &RenameTableRequest{ + TableBucketARN: mustBucketARN(t), SourceNamespace: []string{"ns"}, SourceName: "v", DestNamespace: []string{"ns2"}, DestName: "v", + }, nil, "")) + return fs, m +} + +func updateViewAt(t *testing.T, m *Manager, fs *s3tablestest.MemFiler, location string) error { + return m.Execute(context.Background(), NewManagerClient(fs.Client), "UpdateView", &UpdateViewRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns2"}, Name: "v", MetadataVersion: 2, MetadataLocation: location}, nil, "") +} + +func TestUpdateViewAfterRenameAcrossNamespacesKeepsItsDirectory(t *testing.T) { + fs, m := renameViewAcrossNamespaces(t) + location := "s3://" + renameTestBucket + "/ns/v/metadata/v2.metadata.json" + require.NoError(t, updateViewAt(t, m, fs, location)) + + entry := fs.Get(GetNamespacePath(renameTestBucket, "ns2"), "v") + require.NotNil(t, entry) + var stored tableMetadataInternal + require.NoError(t, json.Unmarshal(entry.Extended[ExtendedKeyMetadata], &stored)) + require.Equal(t, location, stored.MetadataLocation) +} + +func TestUpdateViewAfterRenameAcrossNamespacesRefusesOtherDirectories(t *testing.T) { + fs, m := renameViewAcrossNamespaces(t) + err := updateViewAt(t, m, fs, "s3://"+renameTestBucket+"/ns/elsewhere/metadata/v2.metadata.json") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns2") +} + +// The same-directory exception compares the directory the location names. A +// location whose directory is the bucket root (s3://bucket/metadata/...) names +// no table directory, so it must not match the root a record falls back to and +// must meet the namespace check, as it did before the exception existed. +func TestUpdateRefusesAMetadataDirectoryAtTheBucketRoot(t *testing.T) { + bucketRoot := "s3://" + renameTestBucket + "/metadata/v9.metadata.json" + ctx := context.Background() + + t.Run("table created without a location", func(t *testing.T) { + fs, m := startRenameManager(t) + require.NoError(t, m.Execute(ctx, NewManagerClient(fs.Client), "CreateTable", &CreateTableRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, Name: "fresh", Format: "ICEBERG"}, nil, "")) + err := m.Execute(ctx, NewManagerClient(fs.Client), "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, Name: "fresh", MetadataVersion: 2, MetadataLocation: bucketRoot}, nil, "") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns") + }) + t.Run("table stored at its name path", func(t *testing.T) { + fs, m := startRenameManager(t) + err := updateTableAt(ctx, t, m, fs, "ns", bucketRoot) + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns") + }) + t.Run("view", func(t *testing.T) { + fs, m := startRenameManager(t) + seedView(t, fs, "v") + err := m.Execute(ctx, NewManagerClient(fs.Client), "UpdateView", &UpdateViewRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, Name: "v", MetadataVersion: 2, MetadataLocation: bucketRoot}, nil, "") + require.Error(t, err) + require.Contains(t, s3TablesErrorText(err), "must be within namespace ns") + }) +} + +// A table created without a location keeps its files in the warehouse directory +// it was given; its first commit writes there, also after a rename across +// namespaces. +func TestUpdateOfATableCreatedWithoutALocationWritesItsWarehouse(t *testing.T) { + ctx := context.Background() + fs, m := startRenameManager(t) + addRenameNamespace(t, fs, "ns2") + client := NewManagerClient(fs.Client) + require.NoError(t, m.Execute(ctx, client, "CreateTable", &CreateTableRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns"}, Name: "fresh", Format: "ICEBERG"}, nil, "")) + created, err := runGetTable(t, m, fs, "ns", "fresh") + require.NoError(t, err) + require.NotEmpty(t, created.WarehouseLocation) + + require.NoError(t, runRename(t, m, fs, &RenameTableRequest{TableBucketARN: mustBucketARN(t), + SourceNamespace: []string{"ns"}, SourceName: "fresh", DestNamespace: []string{"ns2"}, DestName: "fresh"})) + location := created.WarehouseLocation + "/metadata/v1.metadata.json" + require.NoError(t, m.Execute(ctx, client, "UpdateTable", &UpdateTableRequest{TableBucketARN: mustBucketARN(t), + Namespace: []string{"ns2"}, Name: "fresh", MetadataVersion: 1, MetadataLocation: location}, nil, "")) + + got, err := runGetTable(t, m, fs, "ns2", "fresh") + require.NoError(t, err) + require.Equal(t, location, got.MetadataLocation) +} diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index 2bc8ffca0a..7023518eb6 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -467,7 +467,8 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req return nil, err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { + // As for tables: the shape now, where it may point once authorized. + if _, err := validateMetadataLocationShape(req.MetadataLocation, bucketName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return nil, err } @@ -511,6 +512,10 @@ func (h *S3TablesHandler) authorizeUpdateView(w http.ResponseWriter, r *http.Req return nil, NewAuthError("UpdateView", accountID, "not authorized to update view") } + if err := h.refuseUpdatedMetadataLocation(w, r, filerClient, bucketName, namespaceName, viewPath, &metadata, req.MetadataLocation); err != nil { + return nil, err + } + return &updateViewTarget{ bucketName: bucketName, namespaceName: namespaceName, diff --git a/weed/s3api/s3tables/utils.go b/weed/s3api/s3tables/utils.go index 9ef77bb2ca..a8bbc87e19 100644 --- a/weed/s3api/s3tables/utils.go +++ b/weed/s3api/s3tables/utils.go @@ -139,12 +139,63 @@ func ValidateMetadataLocation(metadataLocation, bucketName, namespace string) er if metadataLocation == "" { return nil } - bucket, tablePath, err := parseS3Location(metadataLocation) + tablePath, err := validateMetadataLocationShape(metadataLocation, bucketName) if err != nil { return err } + if !strings.HasPrefix(tablePath, namespace+"/") { + return fmt.Errorf("metadata location must be within namespace %s", namespace) + } + return nil +} + +// validateUpdatedMetadataLocation is ValidateMetadataLocation for an update of +// the record at recordPath, whose stored metadata is current. A location +// outside the record's namespace is accepted only when it keeps the record in +// the directory it already holds its files under and rootShared reports that +// no other record keeps its files there: a rename across namespaces moves the +// record and leaves its files behind, while a record written before locations +// were confined and claimed may share another table's directory. A location +// that moves the record to another directory must lie inside the namespace, +// and the update still refuses one another record claims. +func validateUpdatedMetadataLocation(metadataLocation, bucketName, namespace, recordPath string, current *tableMetadataInternal, rootShared func(root string) (bool, error)) error { + err := ValidateMetadataLocation(metadataLocation, bucketName, namespace) + if err == nil { + return nil + } + // A malformed location is refused whatever directory it cleans up to. + if _, shapeErr := validateMetadataLocationShape(metadataLocation, bucketName); shapeErr != nil { + return shapeErr + } + // The directory the location itself names, with no fallback: CatalogRecordRoot + // replaces a directory outside the bucket subtree (s3://bucket/metadata/...) + // with the record's warehouse or name path, which would match the current + // root and let a location no load can resolve skip the namespace check. + dir := TableDataDirFromMetadataLocation(metadataLocation) + if !strings.HasPrefix(dir, GetTableBucketPath(bucketName)+"/") || dir != current.root(bucketName, recordPath) { + return err + } + if shared, sharedErr := rootShared(dir); sharedErr != nil { + return sharedErr + } else if shared { + return err + } + return nil +} + +// validateMetadataLocationShape checks what a metadata location must satisfy +// whatever record it belongs to: it names the table bucket and a table path of +// safe segments. It returns that path. An empty location passes. +func validateMetadataLocationShape(metadataLocation, bucketName string) (string, error) { + if metadataLocation == "" { + return "", nil + } + bucket, tablePath, err := parseS3Location(metadataLocation) + if err != nil { + return "", err + } if bucket != bucketName { - return fmt.Errorf("metadata location must be within bucket %s", bucketName) + return "", fmt.Errorf("metadata location must be within bucket %s", bucketName) } hasSegment := false for _, segment := range strings.Split(tablePath, "/") { @@ -152,17 +203,14 @@ func ValidateMetadataLocation(metadataLocation, bucketName, namespace string) er continue } if segment == "." || segment == ".." || strings.ContainsAny(segment, "\\\x00") { - return fmt.Errorf("invalid metadata location path") + return "", fmt.Errorf("invalid metadata location path") } hasSegment = true } if !hasSegment { - return fmt.Errorf("metadata location must include a table path") + return "", fmt.Errorf("metadata location must include a table path") } - if !strings.HasPrefix(tablePath, namespace+"/") { - return fmt.Errorf("metadata location must be within namespace %s", namespace) - } - return nil + return tablePath, nil } // ValidateEncryptionConfiguration admits only the AES256 declaration, under the