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