From 87796306e23515d20600b24191cbcad2028cb66f Mon Sep 17 00:00:00 2001 From: niksis02 Date: Mon, 31 Aug 2026 19:37:58 +0400 Subject: [PATCH] fix: make --chuid/--chgid usable with the standalone IAM service With the posix backend running --chuid/--chgid against the standalone IAM service, CreateBucket failed for every bucket name and left a half-created directory behind. Bucket ownership is fixed to the gateway's root account there, and that account was constructed from the root credentials alone, so its UserID/GroupID stayed at zero and the gateway tried to chown each new bucket to uid/gid 0 - something a process that is not root can never do. Root-account object writes failed the same way, because the identity the S3 request path uses for root also comes from the root credentials and never from the IAM backend. On top of that, the failed chown returned before the acl xattr was written, so the leftover directory made every later request for that name fail with "get bucket acl: no such key" until it was removed by hand. The standalone IAM client now reports the root account with UserID, GroupID and ProjectID taken from --iam-standalone-default-uid, -gid and -project-id, returning a copy so the stored root account keeps the credentials it is compared against. ResolveDerivedKey copies that same identity onto root when the IAM backend fixes bucket ownership to the root access key, which keeps root's own writes consistent with the buckets root owns. CreateBucket now removes the bucket directory, its sidecar attributes and its versioning directory on any failure after the mkdir, so a failed create leaves nothing behind and the name stays retryable. A chown EPERM reports the target uid/gid, the flags that asked for it and the process euid/egid instead of a bare "operation not permitted", and the posix backend warns at startup when chuid/chgid are set on an unprivileged gateway. The built-in IAM backends do not fix bucket ownership, so root and every other account reach the storage backend exactly as before. --- auth/fixed_bucket_owner.go | 24 ++++++++ auth/iam_standalone.go | 26 +++++++-- auth/iam_standalone_test.go | 70 +++++++++++++++++++++++ auth/signing_key_provider.go | 2 +- backend/posix/posix.go | 94 ++++++++++++++++++++++++++++--- backend/posix/with_otmpfile.go | 6 +- backend/posix/without_otmpfile.go | 4 +- 7 files changed, 206 insertions(+), 20 deletions(-) diff --git a/auth/fixed_bucket_owner.go b/auth/fixed_bucket_owner.go index d6e233a6..2ddebe67 100644 --- a/auth/fixed_bucket_owner.go +++ b/auth/fixed_bucket_owner.go @@ -34,3 +34,27 @@ func ResolveFixedBucketOwner(iam IAMService) (Account, bool) { return fbo.BucketOwner(), true } + +// rootIdentity returns the account a storage backend should see for a request +// signed with the gateway's root credentials. The S3 request path knows root +// only by its access key and secret, so root would otherwise reach the +// backend with the zero uid/gid — which the posix backend's --chuid/--chgid +// then tries to chown to, an operation an unprivileged gateway can never +// perform. +// +// An IAM backend that fixes bucket ownership to root also defines the POSIX +// identity root owns those buckets with, so take it from there: root's own +// object writes then land with the same ownership as the buckets root owns. +// Backends that do not fix ownership resolve a real per-account uid/gid for +// every other account and keep root exactly as it was. +func rootIdentity(iam IAMService, root Account) Account { + owner, fixed := ResolveFixedBucketOwner(iam) + if !fixed || owner.Access != root.Access { + return root + } + + root.UserID = owner.UserID + root.GroupID = owner.GroupID + root.ProjectID = owner.ProjectID + return root +} diff --git a/auth/iam_standalone.go b/auth/iam_standalone.go index 8fa19fba..a177c336 100644 --- a/auth/iam_standalone.go +++ b/auth/iam_standalone.go @@ -87,8 +87,11 @@ type IAMServiceStandaloneConfig struct { ClientCert string ClientCertKey string ServerCA string - // DefaultUserID/GroupID/ProjectID are assigned to every resolved - // (non-root) account. The standalone IAM service's user model + // DefaultUserID/GroupID/ProjectID are assigned to every account this + // client resolves, the locally-held root account included: bucket + // ownership is fixed to root here, so root must carry the same POSIX + // identity as everyone else or a backend chowning to it would target + // uid/gid 0. The standalone IAM service's user model // (iamapi/types.User, mirroring real AWS IAM) has no POSIX uid/gid/ // project-id concept, so there is no per-user value to fetch instead — // every standalone-backed account shares one POSIX identity for @@ -412,7 +415,7 @@ func (s *IAMServiceStandalone) DeriveSigningKey(access, sessionToken, date, regi if sessionToken != "" { return nil, Account{}, ErrInvalidSessionToken } - return sigv4auth.DeriveKey(s.rootAcc.Secret, date, region, service), s.rootAcc, nil + return sigv4auth.DeriveKey(s.rootAcc.Secret, date, region, service), s.rootAccount(), nil } var resp private.DeriveSigningKeyResponse @@ -541,7 +544,7 @@ func decisionFromWireValue(v string) policyDecision { // one per key. func (s *IAMServiceStandalone) GetUserAccount(access string) (Account, error) { if access == s.rootAcc.Access { - return s.rootAcc, nil + return s.rootAccount(), nil } accounts, err := s.resolveAccountDetails([]string{access}) @@ -581,7 +584,7 @@ func (s *IAMServiceStandalone) resolveAccountDetails(accesses []string) ([]resol remoteIdx := make([]int, 0, len(accesses)) for i, access := range accesses { if access == s.rootAcc.Access { - out[i] = resolvedAccount{Found: true, Account: s.rootAcc} + out[i] = resolvedAccount{Found: true, Account: s.rootAccount()} continue } remote = append(remote, access) @@ -641,7 +644,18 @@ func (s *IAMServiceStandalone) ResolveAccounts(accessKeyIDs []string) ([]string, // BucketOwner implements FixedBucketOwner: every bucket is owned by the // gateway's root account, the only account this process knows locally. func (s *IAMServiceStandalone) BucketOwner() Account { - return s.rootAcc + return s.rootAccount() +} + +// rootAccount returns the root account as an identity: a copy of the locally +// held root credentials carrying the same POSIX identity every other +// standalone-backed account gets. +func (s *IAMServiceStandalone) rootAccount() Account { + acc := s.rootAcc + acc.UserID = s.cfg.DefaultUserID + acc.GroupID = s.cfg.DefaultGroupID + acc.ProjectID = s.cfg.DefaultProjectID + return acc } // CreateAccount is not supported diff --git a/auth/iam_standalone_test.go b/auth/iam_standalone_test.go index 57167711..8bdeec60 100644 --- a/auth/iam_standalone_test.go +++ b/auth/iam_standalone_test.go @@ -619,3 +619,73 @@ func serveFakePrivate(t *testing.T, protocol string, status int, body string) st return sockPath } + +// TestIAMServiceStandaloneRootCarriesPosixIdentity covers the identity a +// storage backend chowns to. Bucket ownership is fixed to root here, so a +// root account left at uid/gid 0 makes the posix backend's --chuid/--chgid +// target root for every bucket and for root's own object writes — which an +// unprivileged gateway can never do. +func TestIAMServiceStandaloneRootCarriesPosixIdentity(t *testing.T) { + _, sock := standaloneTestServer(t) + + rootAcc := Account{Access: standaloneTestRootAccess, Secret: standaloneTestRootSecret, Role: RoleAdmin} + client, err := NewIAMServiceStandalone(rootAcc, IAMServiceStandaloneConfig{ + Endpoint: sock, + DefaultUserID: 1001, + DefaultGroupID: 1002, + DefaultProjectID: 1003, + }) + if err != nil { + t.Fatalf("NewIAMServiceStandalone: %v", err) + } + defer client.Shutdown() + + checkIDs := func(what string, acc Account) { + t.Helper() + if acc.UserID != 1001 || acc.GroupID != 1002 || acc.ProjectID != 1003 { + t.Errorf("%s posix ids = %v/%v/%v, want 1001/1002/1003", + what, acc.UserID, acc.GroupID, acc.ProjectID) + } + } + + owner, fixed := ResolveFixedBucketOwner(client) + if !fixed { + t.Fatal("ResolveFixedBucketOwner: standalone client must fix bucket ownership") + } + if owner.Access != standaloneTestRootAccess { + t.Errorf("bucket owner = %q, want the root account %q", owner.Access, standaloneTestRootAccess) + } + checkIDs("BucketOwner()", owner) + + // The same identity must come back wherever root is resolved, so that a + // bucket root owns and an object root writes get the same ownership. + acc, err := client.GetUserAccount(standaloneTestRootAccess) + if err != nil { + t.Fatalf("GetUserAccount(root): %v", err) + } + checkIDs("GetUserAccount(root)", acc) + + yyyymmdd := time.Now().UTC().Format(sigv4auth.YYYYMMDD) + _, acc, err = client.DeriveSigningKey(standaloneTestRootAccess, "", yyyymmdd, "us-east-1", "s3") + if err != nil { + t.Fatalf("DeriveSigningKey(root): %v", err) + } + checkIDs("DeriveSigningKey(root)", acc) + + missing, err := client.ResolveAccounts([]string{standaloneTestRootAccess}) + if err != nil { + t.Fatalf("ResolveAccounts(root): %v", err) + } + if len(missing) != 0 { + t.Errorf("ResolveAccounts(root) = %v, want the root account to resolve", missing) + } + + // The stored root account is compared against by credential, and must + // keep the credentials it was constructed with. + if client.rootAcc != rootAcc { + t.Errorf("stored root account was mutated: %+v, want %+v", client.rootAcc, rootAcc) + } + if acc.Secret != standaloneTestRootSecret || acc.Role != RoleAdmin { + t.Errorf("root identity lost its credentials or role: %+v", acc) + } +} diff --git a/auth/signing_key_provider.go b/auth/signing_key_provider.go index aff059f7..9b9f4945 100644 --- a/auth/signing_key_provider.go +++ b/auth/signing_key_provider.go @@ -103,7 +103,7 @@ func ResolveDerivedKey(iam IAMService, root Account, access, sessionToken, date, if sessionToken != "" { return nil, Account{}, ErrInvalidSessionToken } - return sigv4auth.DeriveKey(root.Secret, date, region, service), root, nil + return sigv4auth.DeriveKey(root.Secret, date, region, service), rootIdentity(iam, root), nil } if skp, ok := iam.(SigningKeyProvider); ok { return skp.DeriveSigningKey(access, sessionToken, date, region, service) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 900d38a1..e4a6e945 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -319,6 +319,20 @@ func New(rootdir string, meta meta.MetadataStorer, opts PosixOpts) (*Posix, erro fmt.Println("Using sidecar directory for metadata:", sidecardirAbs) } + // A gateway that is not root can only chown a file to an account whose + // uid/gid it already has, so --chuid/--chgid either do nothing or fail + // every write for every other account. That is a configuration mistake + // worth naming at startup rather than one InternalError per request, but + // it is not fatal: the process may hold CAP_CHOWN without being root, and + // a setup where every account shares the gateway's own uid/gid is a + // legitimate no-op. + euid, egid := os.Geteuid(), os.Getegid() + if (opts.ChownUID || opts.ChownGID) && euid != 0 { + fmt.Printf("Warning: --chuid/--chgid requested, but the gateway runs as euid %v/egid %v: "+ + "writes for any account with a different uid/gid will fail unless this process can chown\n", + euid, egid) + } + newDirPerm := defaultNewDirPerm if opts.newDirPermSet { newDirPerm = opts.NewDirPerm.Perm() @@ -332,8 +346,8 @@ func New(rootdir string, meta meta.MetadataStorer, opts PosixOpts) (*Posix, erro meta: meta, rootfd: f, rootdir: rootdir, - euid: os.Geteuid(), - egid: os.Getegid(), + euid: euid, + egid: egid, chownuid: opts.ChownUID, chowngid: opts.ChownGID, bucketlinks: opts.BucketLinks, @@ -615,7 +629,7 @@ func (p *Posix) HeadBucket(ctx context.Context, input *s3.HeadBucketInput) (*s3. return &s3.HeadBucketOutput{}, nil } -func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, acl []byte) error { +func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, acl []byte) (err error) { release, err := p.acquireActionSlot(ctx) if err != nil { return err @@ -664,10 +678,23 @@ func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, a return fmt.Errorf("mkdir bucket: %w", err) } + // The directory now exists but is not yet a usable bucket: until the acl + // xattr below is stored, every request for this name — a retry of this + // same CreateBucket included — fails with "get bucket acl: no such key", + // and the name stays poisoned until someone removes the directory by + // hand. Undo the mkdir on any failure from here on so the name stays + // retryable. + defer func() { + if err == nil { + return + } + p.removePartialBucket(bucket) + }() + if doChown { - err := os.Chown(bucket, uid, gid) + err = os.Chown(bucket, uid, gid) if err != nil { - return fmt.Errorf("chown bucket: %w", err) + return p.chownErr(bucket, uid, gid, err) } } @@ -720,6 +747,57 @@ func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, a return nil } +// removePartialBucket undoes a partially completed CreateBucket: the bucket +// directory, any metadata stored for it (which may live in a sidecar +// directory outside the bucket) and its versioning directory. Failures here +// are logged rather than returned — the caller is already failing with the +// error that matters, and reporting a cleanup failure instead would hide it. +func (p *Posix) removePartialBucket(bucket string) { + if err := os.RemoveAll(bucket); err != nil { + debuglogger.Logf("failed to remove partially created bucket (%q): %v", bucket, err) + } + if err := p.meta.DeleteAttributes(bucket, ""); err != nil { + debuglogger.Logf("failed to delete partially created bucket sidecar attributes (%q): %v", bucket, err) + } + if p.versioningEnabled() { + err := os.RemoveAll(filepath.Join(p.versioningDir, bucket)) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + debuglogger.Logf("failed to remove partially created bucket version directory (%q): %v", bucket, err) + } + } +} + +// mkdirAll is backend.MkdirAll with this backend's directory permissions and +// the same chown annotation CreateBucket uses: an EPERM here is the +// --chuid/--chgid misconfiguration, not a filesystem problem. Only EPERM is +// rewritten, so callers matching on EROFS or ErrObjectParentIsFile are +// unaffected. +func (p *Posix) mkdirAll(path string, uid, gid int, doChown bool) error { + err := backend.MkdirAll(path, uid, gid, doChown, p.newDirPerm) + if doChown && errors.Is(err, syscall.EPERM) { + return p.chownErr(path, uid, gid, err) + } + return err +} + +// chownErr annotates a chown failure with the configuration that asked for +// it. EPERM is not transient here: an unprivileged gateway can never give a +// file away to another uid or to a group it is not in, so every write for +// that account fails the same way until the configuration changes. Name the +// flags and the process identity that make the request impossible rather +// than leaving a bare "operation not permitted" in the log. +func (p *Posix) chownErr(name string, uid, gid int, err error) error { + // The errno is wrapped rather than the *fs.PathError it arrived in: the + // PathError repeats a path this message already names, and errors.Is + // still matches EPERM either way. + var errno syscall.Errno + if errors.As(err, &errno) && errno == syscall.EPERM { + return fmt.Errorf("chown %v to %v:%v: %w: --chuid/--chgid need a privileged gateway, but this one runs as euid %v/egid %v", + name, uid, gid, errno, p.euid, p.egid) + } + return fmt.Errorf("chown %v: %w", name, err) +} + func (p *Posix) isBucketEmpty(bucket string) error { if p.versioningEnabled() { ents, err := os.ReadDir(filepath.Join(p.versioningDir, bucket)) @@ -2447,7 +2525,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C dir := filepath.Dir(objname) if dir != "" { uid, gid, doChown := p.getChownIDs(acct) - err = backend.MkdirAll(dir, uid, gid, doChown, p.newDirPerm) + err = p.mkdirAll(dir, uid, gid, doChown) if err != nil { return res, "", err } @@ -4075,7 +4153,7 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje return s3response.PutObjectOutput{}, err } - err = backend.MkdirAll(name, uid, gid, doChown, p.newDirPerm) + err = p.mkdirAll(name, uid, gid, doChown) if err != nil { if errors.Is(err, syscall.EDQUOT) { return s3response.PutObjectOutput{}, s3err.GetAPIError(s3err.ErrQuotaExceeded) @@ -4270,7 +4348,7 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje dir := filepath.Dir(name) if dir != "" { - err = backend.MkdirAll(dir, uid, gid, doChown, p.newDirPerm) + err = p.mkdirAll(dir, uid, gid, doChown) if err != nil { return s3response.PutObjectOutput{}, s3err.GetAPIError(s3err.ErrExistingObjectIsDirectory) } diff --git a/backend/posix/with_otmpfile.go b/backend/posix/with_otmpfile.go index a1e73e48..173765b3 100644 --- a/backend/posix/with_otmpfile.go +++ b/backend/posix/with_otmpfile.go @@ -135,7 +135,7 @@ func (p *Posix) openTmpFile(dir, bucket, obj string, size int64, acct auth.Accou err := f.Chown(uid, gid) if err != nil { f.Close() - return nil, fmt.Errorf("set temp file ownership: %w", err) + return nil, fmt.Errorf("set temp file ownership: %w", p.chownErr(filepath.Join(bucket, obj), uid, gid, err)) } } @@ -143,7 +143,7 @@ func (p *Posix) openTmpFile(dir, bucket, obj string, size int64, acct auth.Accou } func (p *Posix) openMkTemp(dir, bucket, obj string, size int64, dofalloc bool, uid, gid int, doChown bool, allowODirect odirectPolicy) (*tmpfile, error) { - err := backend.MkdirAll(dir, uid, gid, doChown, p.newDirPerm) + err := p.mkdirAll(dir, uid, gid, doChown) if err != nil { if errors.Is(err, syscall.EROFS) { return nil, s3err.GetAPIError(s3err.ErrMethodNotAllowed) @@ -206,7 +206,7 @@ func (p *Posix) openMkTemp(dir, bucket, obj string, size int64, dofalloc bool, u if err != nil { f.Close() os.Remove(f.Name()) - return nil, fmt.Errorf("set temp file ownership: %w", err) + return nil, fmt.Errorf("set temp file ownership: %w", p.chownErr(filepath.Join(bucket, obj), uid, gid, err)) } } diff --git a/backend/posix/without_otmpfile.go b/backend/posix/without_otmpfile.go index 8c03bf09..c303b168 100644 --- a/backend/posix/without_otmpfile.go +++ b/backend/posix/without_otmpfile.go @@ -60,7 +60,7 @@ func (p *Posix) openTmpFile(dir, bucket, obj string, size int64, acct auth.Accou // Create a temp file for upload while in progress (see link comments below). var err error - err = backend.MkdirAll(dir, uid, gid, doChown, p.newDirPerm) + err = p.mkdirAll(dir, uid, gid, doChown) if err != nil { if errors.Is(err, syscall.EROFS) { return nil, s3err.GetAPIError(s3err.ErrMethodNotAllowed) @@ -81,7 +81,7 @@ func (p *Posix) openTmpFile(dir, bucket, obj string, size int64, acct auth.Accou if err != nil { f.Close() os.Remove(f.Name()) - return nil, fmt.Errorf("set temp file ownership: %w", err) + return nil, fmt.Errorf("set temp file ownership: %w", p.chownErr(filepath.Join(bucket, obj), uid, gid, err)) } }