fix: return default bucket acl if none exists

We were trying to parse a non existing acl and returning an internal server error due to invalid json acl data. If the bucket acl does not exist, return a default acl with the root account as the owner. This fixes #1060, but does not address the invalid acl format from s3cmd reported in #963.
versity · Feb 11, 2025 · edb47a9 · edb47a9
1 parent 2d75bae
commit edb47a9
Show file tree

Hide file tree

Showing 14 changed files with 106 additions and 12 deletions.
diff --git a/auth/acl.go b/auth/acl.go
@@ -193,14 +193,25 @@ func ParseACL(data []byte) (ACL, error) {
 	return acl, nil
 }
 
-func ParseACLOutput(data []byte) (GetBucketAclOutput, error) {
+func ParseACLOutput(data []byte, owner string) (GetBucketAclOutput, error) {
+	grants := []Grant{}
+
+	if len(data) == 0 {
+		return GetBucketAclOutput{
+			Owner: &types.Owner{
+				ID: &owner,
+			},
+			AccessControlList: AccessControlList{
+				Grants: grants,
+			},
+		}, nil
+	}
+
 	var acl ACL
 	if err := json.Unmarshal(data, &acl); err != nil {
 		return GetBucketAclOutput{}, fmt.Errorf("parse acl: %w", err)
 	}
 
-	grants := []Grant{}
-
 	for _, elem := range acl.Grantees {
 		acs := elem.Access
 		grants = append(grants, Grant{

diff --git a/auth/iam.go b/auth/iam.go
@@ -79,6 +79,7 @@ func updateAcc(acc *Account, props MutableProps) {
 type IAMService interface {
 	CreateAccount(account Account) error
 	GetUserAccount(access string) (Account, error)
+	GetRootAccount() Account
 	UpdateUserAccount(access string, props MutableProps) error
 	DeleteUserAccount(access string) error
 	ListUserAccounts() ([]Account, error)
@@ -161,7 +162,7 @@ func New(o *Opts) (IAMService, error) {
 	default:
 		// if no iam options selected, default to the single user mode
 		fmt.Println("No IAM service configured, enabling single account mode")
-		return IAMServiceSingle{}, nil
+		return IAMServiceSingle{RootAcct: o.RootAccount}, nil
 	}
 
 	if err != nil {

diff --git a/auth/iam_cache.go b/auth/iam_cache.go
@@ -170,6 +170,11 @@ func (c *IAMCache) GetUserAccount(access string) (Account, error) {
 	return a, nil
 }
 
+// GetRootAccount returns the root account.
+func (s *IAMCache) GetRootAccount() Account {
+	return s.service.GetRootAccount()
+}
+
 // DeleteUserAccount deletes account from IAM service and cache
 func (c *IAMCache) DeleteUserAccount(access string) error {
 	err := c.service.DeleteUserAccount(access)

diff --git a/auth/iam_internal.go b/auth/iam_internal.go
@@ -123,6 +123,11 @@ func (s *IAMServiceInternal) GetUserAccount(access string) (Account, error) {
 	return acct, nil
 }
 
+// GetRootAccount returns the root account.
+func (s *IAMServiceInternal) GetRootAccount() Account {
+	return s.rootAcc
+}
+
 // UpdateUserAccount updates the specified user account fields. Returns
 // ErrNoSuchUser if the account does not exist.
 func (s *IAMServiceInternal) UpdateUserAccount(access string, props MutableProps) error {

diff --git a/auth/iam_ipa.go b/auth/iam_ipa.go
@@ -208,6 +208,10 @@ func (ipa *IpaIAMService) GetUserAccount(access string) (Account, error) {
 	return account, nil
 }
 
+func (ipa *IpaIAMService) GetRootAccount() Account {
+	return ipa.rootAcc
+}
+
 func (ipa *IpaIAMService) UpdateUserAccount(access string, props MutableProps) error {
 	return fmt.Errorf("not implemented")
 }

diff --git a/auth/iam_ldap.go b/auth/iam_ldap.go
@@ -126,6 +126,10 @@ func (ld *LdapIAMService) GetUserAccount(access string) (Account, error) {
 	}, nil
 }
 
+func (ld *LdapIAMService) GetRootAccount() Account {
+	return ld.rootAcc
+}
+
 func (ld *LdapIAMService) UpdateUserAccount(access string, props MutableProps) error {
 	req := ldap.NewModifyRequest(fmt.Sprintf("%v=%v, %v", ld.accessAtr, access, ld.queryBase), nil)
 	if props.Secret != nil {

diff --git a/auth/iam_s3_object.go b/auth/iam_s3_object.go
@@ -150,6 +150,10 @@ func (s *IAMServiceS3) GetUserAccount(access string) (Account, error) {
 	return acct, nil
 }
 
+func (s *IAMServiceS3) GetRootAccount() Account {
+	return s.rootAcc
+}
+
 func (s *IAMServiceS3) UpdateUserAccount(access string, props MutableProps) error {
 	s.Lock()
 	defer s.Unlock()

diff --git a/auth/iam_single.go b/auth/iam_single.go
@@ -19,36 +19,43 @@ import (
 )
 
 // IAMServiceSingle manages the single tenant (root-only) IAM service
-type IAMServiceSingle struct{}
+type IAMServiceSingle struct {
+	RootAcct Account
+}
 
 var _ IAMService = &IAMServiceSingle{}
 
 // CreateAccount not valid in single tenant mode
-func (IAMServiceSingle) CreateAccount(account Account) error {
+func (s IAMServiceSingle) CreateAccount(account Account) error {
 	return s3err.GetAPIError(s3err.ErrAdminMethodNotSupported)
 }
 
 // GetUserAccount no accounts in single tenant mode
-func (IAMServiceSingle) GetUserAccount(access string) (Account, error) {
+func (s IAMServiceSingle) GetUserAccount(access string) (Account, error) {
 	return Account{}, s3err.GetAPIError(s3err.ErrAdminMethodNotSupported)
 }
 
+// GetRootAccount returns the root account
+func (s IAMServiceSingle) GetRootAccount() Account {
+	return s.RootAcct
+}
+
 // UpdateUserAccount no accounts in single tenant mode
-func (IAMServiceSingle) UpdateUserAccount(access string, props MutableProps) error {
+func (s IAMServiceSingle) UpdateUserAccount(access string, props MutableProps) error {
 	return s3err.GetAPIError(s3err.ErrAdminMethodNotSupported)
 }
 
 // DeleteUserAccount no accounts in single tenant mode
-func (IAMServiceSingle) DeleteUserAccount(access string) error {
+func (s IAMServiceSingle) DeleteUserAccount(access string) error {
 	return s3err.GetAPIError(s3err.ErrAdminMethodNotSupported)
 }
 
 // ListUserAccounts no accounts in single tenant mode
-func (IAMServiceSingle) ListUserAccounts() ([]Account, error) {
+func (s IAMServiceSingle) ListUserAccounts() ([]Account, error) {
 	return []Account{}, s3err.GetAPIError(s3err.ErrAdminMethodNotSupported)
 }
 
 // Shutdown graceful termination of service
-func (IAMServiceSingle) Shutdown() error {
+func (s IAMServiceSingle) Shutdown() error {
 	return nil
 }
diff --git a/auth/iam_vault.go b/auth/iam_vault.go
@@ -148,6 +148,10 @@ func (vt *VaultIAMService) GetUserAccount(access string) (Account, error) {
 	return acc, nil
 }
 
+func (vt *VaultIAMService) GetRootAccount() Account {
+	return vt.rootAcc
+}
+
 func (vt *VaultIAMService) UpdateUserAccount(access string, props MutableProps) error {
 	//TODO: We need something like a transaction here ?
 	acc, err := vt.GetUserAccount(access)

diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go
@@ -642,6 +642,10 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error {
 	isRoot := ctx.Locals("isRoot").(bool)
 	parsedAcl := ctx.Locals("parsedAcl").(auth.ACL)
 
+	if parsedAcl.Owner == "" && c.iam != nil {
+		parsedAcl.Owner = c.iam.GetRootAccount().Access
+	}
+
 	if ctx.Request().URI().QueryArgs().Has("tagging") {
 		err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{
 			Readonly:      c.readonly,
@@ -921,7 +925,7 @@ func (c S3ApiController) ListActions(ctx *fiber.Ctx) error {
 				})
 		}
 
-		res, err := auth.ParseACLOutput(data)
+		res, err := auth.ParseACLOutput(data, parsedAcl.Owner)
 		return SendXMLResponse(ctx, res, err,
 			&MetaOpts{
 				Logger:      c.logger,

diff --git a/s3api/controllers/iam_moq_test.go b/s3api/controllers/iam_moq_test.go
diff --git a/s3api/middlewares/acl-parser.go b/s3api/middlewares/acl-parser.go
@@ -74,6 +74,11 @@ func AclParser(be backend.Backend, logger s3log.AuditLogger, readonly bool) fibe
 			return controllers.SendResponse(ctx, err, &controllers.MetaOpts{Logger: logger})
 		}
 
+		// if owner is not set, set default owner to root account
+		if parsedAcl.Owner == "" {
+			parsedAcl.Owner = ctx.Locals("rootAccess").(string)
+		}
+
 		ctx.Locals("parsedAcl", parsedAcl)
 		return ctx.Next()
 	}

diff --git a/s3api/middlewares/authentication.go b/s3api/middlewares/authentication.go
@@ -76,6 +76,7 @@ func VerifyV4Signature(root RootUserConfig, iam auth.IAMService, logger s3log.Au
 		}
 
 		ctx.Locals("isRoot", authData.Access == root.Access)
+		ctx.Locals("rootAccess", root.Access)
 
 		account, err := acct.getAccount(authData.Access)
 		if err == auth.ErrNoSuchUser {

diff --git a/s3api/middlewares/presign-auth.go b/s3api/middlewares/presign-auth.go
@@ -43,6 +43,8 @@ func VerifyPresignedV4Signature(root RootUserConfig, iam auth.IAMService, logger
 		}
 
 		ctx.Locals("isRoot", authData.Access == root.Access)
+		ctx.Locals("rootAccess", root.Access)
+
 		account, err := acct.getAccount(authData.Access)
 		if err == auth.ErrNoSuchUser {
 			return sendResponse(ctx, s3err.GetAPIError(s3err.ErrInvalidAccessKeyID), logger, mm)