Merge pull request #2336 from versity/sis/standalone-iam-chuid-ownership

fix: make --chuid/--chgid usable with the standalone IAM service

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
+}

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

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)
+	}
+}

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)

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)
 		}

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))
 		}
 	}
 

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))
 		}
 	}