From eb159f31816dead8fa8a34373f3f38624dffe885 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 11 Nov 2026 10:35:00 -0800 Subject: [PATCH] lance: deregister and re-register need UpdateTable (#12552) * lance: require UpdateTable to deregister a table Co-Authored-By: Claude Opus 5.5 * lance: require UpdateTable to register over an existing table Co-Authored-By: Claude Opus 5.5 * lance: test that GetTable alone cannot deregister or re-register a table Co-Authored-By: Claude Opus 5.5 * lance: a register that finds a table the caller cannot read needs UpdateTable to bring it back Co-Authored-By: Claude Opus 5.5 * lance: decide declare write access by whether the table existed, not by what the caller can read Co-Authored-By: Claude Opus 5.5 * lance: test register and declare by a caller that cannot read the table Co-Authored-By: Claude Opus 5.5 * lance: register clears the marker on the create branch only after checking it Co-Authored-By: Claude Opus 5.5 * lance: keep a register whose table was created when the marker check fails Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- weed/s3api/lance/handlers_table.go | 33 ++++++++++++++++--- weed/s3api/lance/handlers_test.go | 53 +++++++++++++++++++++++++++--- weed/s3api/lance/storage.go | 14 ++++++++ 3 files changed, 92 insertions(+), 8 deletions(-) diff --git a/weed/s3api/lance/handlers_table.go b/weed/s3api/lance/handlers_table.go index 417a734bca..e67c01363d 100644 --- a/weed/s3api/lance/handlers_table.go +++ b/weed/s3api/lance/handlers_table.go @@ -135,7 +135,7 @@ func (s *Server) handleDeclareTable(w http.ResponseWriter, r *http.Request) { return } - _, lookupErr := s.loadLanceTable(r, bucket, ns, name) + _, lookupErr := s.loadLanceTableInternal(r, bucket, ns, name) location, err = s.createTable(r, bucket, ns, name, location) if err != nil { writeStorageError(w, r, err) @@ -143,7 +143,8 @@ func (s *Server) handleDeclareTable(w http.ResponseWriter, r *http.Request) { } // A declare that creates the table may write it: a Lance commit is a direct // S3 write that never revisits the catalog, so the create authorization is - // the write evidence. One that only finds the table answers like describe. + // the write evidence. One that only finds the table answers like describe, + // including a table the caller cannot read, which CreateTable still returns. var writable bool if isNotFound(lookupErr) { writable = s.mayWriteNamespace(r, bucket, ns) @@ -382,6 +383,7 @@ func (s *Server) handleRegisterTable(w http.ResponseWriter, r *http.Request) { return } + restore := true existing, err := s.loadLanceTable(r, bucket, ns, name) switch { case err != nil && !isNotFound(err): @@ -403,6 +405,10 @@ func (s *Server) handleRegisterTable(w http.ResponseWriter, r *http.Request) { writeStorageError(w, r, err) return } + if err := s.authorizeUpdateTable(r, bucket, ns, name); err != nil { + writeStorageError(w, r, err) + return + } if existing.location != location { // Repointing the name at another dataset is an update. Dropping and // recreating the entry would take the old dataset's files with it, @@ -417,11 +423,26 @@ func (s *Server) handleRegisterTable(w http.ResponseWriter, r *http.Request) { writeStorageError(w, r, err) return } + // CreateTable also answers for a table the caller cannot read, so + // bringing that one back still needs UpdateTable. + deregistered, _, stateErr := s.datasetState(r, location) + if stateErr != nil { + glog.V(1).Infof("lance: could not check %s for %s: %v", deregisteredMarker, location, stateErr) + } + if deregistered { + if err := s.authorizeUpdateTable(r, bucket, ns, name); err != nil { + writeStorageError(w, r, err) + return + } + } + restore = deregistered } // Registering a deregistered dataset brings it back. - if err := s.removeMarker(r, location, deregisteredMarker); err != nil { - glog.V(2).Infof("lance: could not clear %s for %s: %v", deregisteredMarker, location, err) + if restore { + if err := s.removeMarker(r, location, deregisteredMarker); err != nil { + glog.V(2).Infof("lance: could not clear %s for %s: %v", deregisteredMarker, location, err) + } } writeJSON(w, http.StatusOK, RegisterTableResponse{ @@ -462,6 +483,10 @@ func (s *Server) handleDeregisterTable(w http.ResponseWriter, r *http.Request) { writeStorageError(w, r, err) return } + if err := s.authorizeUpdateTable(r, bucket, ns, name); err != nil { + writeStorageError(w, r, err) + return + } // Deregistering is a state, not a deletion. Dropping the catalog entry would // take the dataset with it, because the entry is the dataset directory and diff --git a/weed/s3api/lance/handlers_test.go b/weed/s3api/lance/handlers_test.go index 32fd3a89a9..e7471b44b1 100644 --- a/weed/s3api/lance/handlers_test.go +++ b/weed/s3api/lance/handlers_test.go @@ -451,14 +451,18 @@ func TestDataPlaneIsUnsupported(t *testing.T) { } // createOnlyAuthenticator signs in an identity that may create and read tables -// but not update them. -type createOnlyAuthenticator struct{} +// but not update them; noRead also takes away reading. +type createOnlyAuthenticator struct{ noRead bool } -func (createOnlyAuthenticator) AuthenticateRequest(*http.Request) (string, interface{}, s3err.ErrorCode) { +func (a createOnlyAuthenticator) AuthenticateRequest(*http.Request) (string, interface{}, s3err.ErrorCode) { + actions := []string{"s3tables:CreateTable", "s3tables:GetTable"} + if a.noRead { + actions = actions[:1] + } return "creator", &struct { Name string Actions []string - }{Name: "creator", Actions: []string{"s3tables:CreateTable", "s3tables:GetTable"}}, s3err.ErrNone + }{Name: "creator", Actions: actions}, s3err.ErrNone } func (createOnlyAuthenticator) DefaultAllow() bool { return false } @@ -495,6 +499,47 @@ func TestDeclareVendsWriteOnlyForATableItCreates(t *testing.T) { } } +// Deregistering hides a table and registering over one brings it back, so +// both change its catalog state and need UpdateTable, not just GetTable. +func TestDeregisterAndReregisterNeedUpdateTable(t *testing.T) { + h := newTestHarness(t) + h.createBucket(t, "lnc", s3tables.FormatLance) + h.mustDo(t, http.MethodPost, "/v1/namespace/lnc$ns/create", `{}`, http.StatusOK) + h.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/declare", `{}`, http.StatusOK) + + server := NewServer(s3tables.NewManagerClient(h.filer.Client), createOnlyAuthenticator{}) + router := mux.NewRouter().SkipClean(true) + server.RegisterRoutes(router) + reader := &testHarness{router: router, filer: h.filer, admin: h.admin, server: server} + + reader.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/describe", `{}`, http.StatusOK) + reader.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/deregister", `{}`, http.StatusForbidden) + if h.filer.Get("/buckets/lnc/ns/theirs", deregisteredMarker) != nil { + t.Fatal("a refused deregister left its marker") + } + + h.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/deregister", `{}`, http.StatusOK) + reader.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/register", `{"location":"s3://lnc/ns/theirs"}`, http.StatusForbidden) + if h.filer.Get("/buckets/lnc/ns/theirs", deregisteredMarker) == nil { + t.Fatal("a refused register brought the table back") + } + + // Without GetTable the table looks absent, but CreateTable still returns it. + blindServer := NewServer(s3tables.NewManagerClient(h.filer.Client), createOnlyAuthenticator{noRead: true}) + blindServer.SetCredentialVendor(recordingVendor{}) + blindRouter := mux.NewRouter().SkipClean(true) + blindServer.RegisterRoutes(blindRouter) + blind := &testHarness{router: blindRouter, filer: h.filer, admin: h.admin, server: blindServer} + + blind.mustDo(t, http.MethodPost, "/v1/table/lnc$ns$theirs/register", `{"location":"s3://lnc/ns/theirs"}`, http.StatusNotFound) + if got := blind.vendedKey(t, "/v1/table/lnc$ns$theirs/declare", ""); got == "read-write" { + t.Error("a declare that found an unreadable table vended write credentials") + } + if h.filer.Get("/buckets/lnc/ns/theirs", deregisteredMarker) == nil { + t.Fatal("a caller without GetTable brought the table back") + } +} + // An in-process describe for a trusted caller resolves the table without the // caller's catalog grants, and still refuses a deregistered one. func TestDescribeTableInternalIgnoresCatalogGrants(t *testing.T) { diff --git a/weed/s3api/lance/storage.go b/weed/s3api/lance/storage.go index 3b8f9076ca..d18de7deb0 100644 --- a/weed/s3api/lance/storage.go +++ b/weed/s3api/lance/storage.go @@ -224,6 +224,20 @@ func (s *Server) mayUpdateTable(r *http.Request, bucket string, ns []string, nam return err == nil } +// authorizeUpdateTable checks the grant a change to the table's catalog state +// needs; a cluster process goes through the trusted manager. +func (s *Server) authorizeUpdateTable(r *http.Request, bucket string, ns []string, name string) error { + req := &s3tables.UpdateTableRequest{ + TableBucketARN: bucketARN(bucket), + Namespace: ns, + Name: name, + } + identityName := s3_constants.GetIdentityNameFromContext(r) + return s.filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + return s.manager(r).AuthorizeUpdateTable(r.Context(), s3tables.NewManagerClient(client), req, identityName) + }) +} + // mayWriteNamespace keeps the vector namespace of a vector bucket out of reach // of s3tables grants: its index data is written only by trusted callers, and // the vector bucket policy gates everyone else through S3 Vectors.